Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x/net/http2: server too verbose logging in case of "connection reset by peer" on posix platforms #61684

Open
0xC001C0DEBA5E opened this issue Aug 1, 2023 · 1 comment
Open

x/net/http2: server too verbose logging in case of "connection reset by peer" on posix platforms #61684

0xC001C0DEBA5E opened this issue Aug 1, 2023 · 1 comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@0xC001C0DEBA5E
Copy link

0xC001C0DEBA5E commented Aug 1, 2023 •
edited

What version of Go are you using (go version)?

1.20.3

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

linux

What did you do?

we use krakend (developed in golang) in our RedHat OpenShift environment and have the following problems with the setup and are looking for support on the following questions. We get these (verbose) log lines from krakend (Go-Application) when the OpenShift internal haproxy makes healthchecks (when using more than one pod). This happend very often:
2023/07/28 08:50:18 http2: server: error reading preface from client <ip>:60524: read tcp <ip>:8443-><ip>:60524: read: connection reset by peer

Tried to disable these with “http2debug”, but in this case it seems that these GODEBUG values will not be used. Thought this will be the case according to this RedHat article - but it does not work:
image
We reported this issue to the krakend Developers in the Gophers slack channel but it seems to be an issue with the net/http2 package.

Some "research" in the h2_bundle

I found the source of the loglines here. The function used is “condlogf”.
image

Within this function the following if-case seems to be the decision point for selecting the “vlogf” or the “logf” function. “vlogf” evaluates the variable “http2VerboseLogs” for verbose logging which is set by the http2debug GODEBUG value (btw “logf” will not evaluate this value):
image

My question: in this case I expected to go into the “vlogf” case (Boring, expected):
image

This is the function “http2isClosedConnError” with the check if a connection is already closed (with a string contains check):
image
Should this not be the case in our “reset by peer” too?

Interestingly, in the Windows-case this will be checked too (but not in the posix platform case):
image
Should this check for the string “use of closed network connection” not be improved with errors.Is? It seems to be possible to check for “reset by peer” error with errors.Is(err, syscall.ECONNRESET).

What do you think?

What did you expect to see?

If not set the GODEBUG value http2debug=1: No verbose log lines in case of "connection reset by peer".
If set the GODEBUG value http2debug=1: Verbose log lines in case of "connection reset by peer".

What did you see instead?

Not set the GODEBUG value http2debug=1: Verbose log lines in case of "connection reset by peer".
@gopherbot gopherbot added this to the Unreleased milestone Aug 1, 2023
@0xC001C0DEBA5E 0xC001C0DEBA5E changed the title x/net: http2 server too verbose logging in case of "connection reset by peer" on posix platforms x/net/http2: server too verbose logging in case of "connection reset by peer" on posix platforms Aug 1, 2023
@0xC001C0DEBA5E 0xC001C0DEBA5E changed the title x/net/http2: server too verbose logging in case of "connection reset by peer" on posix platforms x/net: x/net/http2 server too verbose logging in case of "connection reset by peer" on posix platforms Aug 1, 2023
@0xC001C0DEBA5E 0xC001C0DEBA5E changed the title x/net: x/net/http2 server too verbose logging in case of "connection reset by peer" on posix platforms x/net: x/net/http2: server too verbose logging in case of "connection reset by peer" on posix platforms Aug 1, 2023
@seankhliao seankhliao changed the title x/net: x/net/http2: server too verbose logging in case of "connection reset by peer" on posix platforms x/net/http2: server too verbose logging in case of "connection reset by peer" on posix platforms Aug 1, 2023
@dr2chase dr2chase added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 3, 2023
@dr2chase
Copy link
Contributor

dr2chase commented Aug 3, 2023

@neild @tombergan

mmatczuk added a commit to saucelabs/forwarder that referenced this issue Oct 16, 2023
@mmatczuk
martian: modernize isClosedConnError
28b2bc2 
This addresses the issue golang/go#61684 and partially the TODO.
It allows to handle ECONNABORTED and ECONNRESET "like Windows".
The string search does not always work.

It also checks for io.EOF and io.ErrUnexpectedEOF the clean close error. Typically, those need to be checked alongside isClosedConnError.
mmatczuk added a commit to saucelabs/forwarder that referenced this issue Oct 16, 2023
@mmatczuk
martian: modernize isClosedConnError
f930ad7 
This addresses the issue golang/go#61684 and partially the TODO.
It allows to handle ECONNABORTED and ECONNRESET "like Windows".
The string search does not always work.

It also checks for io.EOF and io.ErrUnexpectedEOF the clean close error. Typically, those need to be checked alongside isClosedConnError.
mmatczuk added a commit to saucelabs/forwarder that referenced this issue Oct 16, 2023
@mmatczuk
martian: modernize isClosedConnError
7295244 
This addresses the issue golang/go#61684 and partially the TODO.
It allows to handle ECONNABORTED and ECONNRESET "like Windows".
The string search does not always work.

It also checks for io.EOF and io.ErrUnexpectedEOF the clean close error. Typically, those need to be checked alongside isClosedConnError.
mmatczuk added a commit to saucelabs/forwarder that referenced this issue Oct 17, 2023
@mmatczuk
martian: modernize isClosedConnError
83b4607 
This addresses the issue golang/go#61684 and partially the TODO.
It allows to handle ECONNABORTED and ECONNRESET "like Windows".
The string search does not always work.

It also checks for io.EOF and io.ErrUnexpectedEOF the clean close error. Typically, those need to be checked alongside isClosedConnError.
mmatczuk added a commit to saucelabs/forwarder that referenced this issue Oct 17, 2023
@mmatczuk
martian: modernize isClosedConnError
c58f86e 
This addresses the issue golang/go#61684 and partially the TODO.
It allows to handle ECONNABORTED and ECONNRESET "like Windows".
The string search does not always work.

It also checks for io.EOF and io.ErrUnexpectedEOF the clean close error. Typically, those need to be checked alongside isClosedConnError.
mmatczuk added a commit to saucelabs/forwarder that referenced this issue Oct 17, 2023
@mmatczuk
martian: modernize isClosedConnError
4474a8d 
This addresses the issue golang/go#61684 and partially the TODO.
It allows to handle ECONNABORTED and ECONNRESET "like Windows".
The string search does not always work.

It also checks for io.EOF and io.ErrUnexpectedEOF the clean close error. Typically, those need to be checked alongside isClosedConnError.
mmatczuk added a commit to saucelabs/martian that referenced this issue Oct 17, 2023
@mmatczuk
martian: modernize isClosedConnError
f16e3dc 
This addresses the issue golang/go#61684 and partially the TODO.
It allows to handle ECONNABORTED and ECONNRESET "like Windows".
The string search does not always work.

It also checks for io.EOF and io.ErrUnexpectedEOF the clean close error. Typically, those need to be checked alongside isClosedConnError.
mmatczuk added a commit to saucelabs/martian that referenced this issue Oct 18, 2023
@mmatczuk
martian: modernize isClosedConnError
be44794 
This addresses the issue golang/go#61684 and partially the TODO.
It allows to handle ECONNABORTED and ECONNRESET "like Windows".
The string search does not always work.

It also checks for io.EOF and io.ErrUnexpectedEOF the clean close error. Typically, those need to be checked alongside isClosedConnError.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
3 participants
@0xC001C0DEBA5E @dr2chase @gopherbot