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

net/http/httputil: ReverseProxy doesn't handle connection/stream reset properly #34413

Open
jaricftw opened this issue Sep 19, 2019 · 4 comments
Open

net/http/httputil: ReverseProxy doesn't handle connection/stream reset properly #34413

jaricftw opened this issue Sep 19, 2019 · 4 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@jaricftw
Copy link

jaricftw commented Sep 19, 2019 •
edited

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

$ go version
net/http/httputil: ReverseProxy

Does this issue reproduce with the latest release?

yes

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/jzhan/Library/Caches/go-build"
GOENV="/Users/jzhan/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/jzhan/gocode"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.13/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.13/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/4p/ch6dsl490v9227wwhd9rnb7w0000gn/T/go-build248622450=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I am trying to test the behavior of HTTP forwarding using httputil.Reverseproxy. Especially the behavior when the backend server closes the connection (for HTTP/1) or resets the stream (for HTTP/2). To test the stream reset behavior, the server uses panic(http.ErrAbortHandler) in the handler.

Wrote some tests for these.

For HTTP1: https://github.com/jaricftw/go-http-reverseproxy/blob/master/http1_test.go#L20, which tests:
1). http1 client -> http1 server
2). http1 client -> httputil.Reverseproxy -> http1 server

For HTTP2: https://github.com/jaricftw/go-http-reverseproxy/blob/master/http2_test.go#L25, similarly:
1). h2c client -> h2c server
2). h2c client -> httputil.Reverseproxy (with h2c setup) -> h2c server

What did you expect to see?

I would expect all tests to pass. i.e. the response for W/ and W/O reverseproxy should be the same in the stream reset scenario

What did you see instead?

In the happy case, w/ and w/o reverseproxy tests all passed, for both HTTP/1 and HTTP/2.

However, I see different responses W/ and W/O reverseproxy for the connection/stream reset test cases:

For HTTP/1:
W/O reverseproxy:

PASS: TestHTTP1/direct,_server_panic (0.00s)
        http1_test.go:62: got err: Get http://127.0.0.1:6666: EOF
        http1_test.go:63: got resp: <nil>

W/ reverseproxy:

FAIL: TestHTTP1/via_proxy,_server_panic (0.00s)
        http1_test.go:62: got err: <nil>
        http1_test.go:63: got resp: &{502 Bad Gateway 502 HTTP/1.1 1 1 map[Content-Length:[0] Date:[Thu, 19 Sep 2019 20:52:56 GMT]] {} 0 [] false false map[] 0xc0001da300 <nil>}
        require.go:248:
                Error Trace:    http1_test.go:65
                Error:          An error is expected but got nil.
                Test:           TestHTTP1/via_proxy,_server_panic

For HTTP/2:
W/O reverseproxy:

PASS: TestHTTP2/direct,_server_panic (0.00s)
        http2_test.go:78: got err: Get http://127.0.0.1:7777: stream error: stream ID 3; INTERNAL_ERROR
        http2_test.go:79: got resp: <nil>

W/ reverseproxy:

FAIL: TestHTTP2/via_proxy,_server_panic (0.00s)
        http2_test.go:78: got err: <nil>
        http2_test.go:79: got resp: &{502 Bad Gateway 502 HTTP/2.0 2 0 map[Content-Length:[0] Date:[Thu, 19 Sep 2019 19:01:15 GMT]] 0xc00022e700 0 [] false false map[] 0xc000250800 <nil>}
        require.go:248:
                Error Trace:    http2_test.go:81
                Error:          An error is expected but got nil.
                Test:           TestHTTP2/via_proxy,_server_panic
FAIL
@jaricftw jaricftw changed the title net/http/httputil: ReverseProxy doesn't handle stream reset properly for HTTP2 net/http/httputil: ReverseProxy doesn't handle connection/stream reset properly Sep 19, 2019
@toothrot
Copy link
Contributor

Hi @jaricftw,

A 502 from a reverse proxy seems reasonable to me in this scenario, but I am not as familiar with http/2.0 as the package owner @bradfitz.

@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 19, 2019
@toothrot toothrot added this to the Go1.14 milestone Sep 19, 2019
@jaricftw
Copy link
Author

jaricftw commented Sep 19, 2019 •
edited

@toothrot I've update the issue to include HTTP/1 as well, and it seems neither HTTP/1 connection close nor HTTP/2 stream reset is handled by the reverse proxy --- a default 502 will be returned to the client.

The risk would be the proxy doesn't close the connection to the client in HTTP/1, or doesn't send a RST_STREAM to the client in HTTP/2?

@bradfitz
Copy link
Contributor

You can change the ReverseProxy's default 502 behavior by setting your own ErrorHandler with whatever policy you'd like: https://golang.org/pkg/net/http/httputil/#ReverseProxy.ErrorHandler

The risk would be the proxy doesn't close the connection to the client in HTTP/1, or doesn't send a RST_STREAM to the client in HTTP/2?

Sorry, I don't follow. Could you reword? Or back up? I read your test code above (thanks for the nice bug report!) but I'm confused by this latest statement/question.

@jaricftw
Copy link
Author

jaricftw commented Oct 7, 2019

The risk would be the proxy doesn't close the connection to the client in HTTP/1, or doesn't send a RST_STREAM to the client in HTTP/2?

Sorry, I don't follow. Could you reword? Or back up? I read your test code above (thanks for the nice bug report!) but I'm confused by this latest statement/question.

Sorry for the confusion. I don't have enough tests to back it up yet. I meant that the connection error seen by the proxy is not properly returned to the client, which may confuse the client upon receiving a regular status code 502. E.g. client may choose its retry mechanism based on the response code/error. And I am assuming the client <-> proxy connection/stream will still be treated "connected" from the client perspective, although the proxy->server one is closed?

@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@guggero guggero mentioned this issue May 10, 2021
Merged
@PyvesB PyvesB mentioned this issue Jan 31, 2022
Closed
9 tasks
@PyvesB PyvesB mentioned this issue Feb 7, 2022
Merged
9 tasks
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
Backlog
Development

No branches or pull requests
4 participants
@bradfitz @toothrot @rsc @jaricftw