Skip to content

net/http/httputil: httputil.ReverseProxy does not call ModifyResponse on failed requests #21255

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
Closed
sebsprenger opened this issue Aug 1, 2017 · 8 comments
Closed

net/http/httputil: httputil.ReverseProxy does not call ModifyResponse on failed requests #21255

sebsprenger opened this issue Aug 1, 2017 · 8 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Go1.11

Comments

@sebsprenger
Copy link

sebsprenger commented Aug 1, 2017 •
edited
Loading

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

go version go1.8.3 darwin/amd64

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/sprengers/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/71/lrpk1tm559gbz8rbgym9nwbw0000gp/T/go-build165819743=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

I setup a httputils.ReverseProxy with a ModifyResponse method:
https://play.golang.org/p/U7R9-oxBfq

What did you expect to see?

I expected that the EYE-CATCHER ON RESPONSE log entry would appear when calling /invalid as well.

What did you see instead?

The EYE-CATCHER ON RESPONSE log entry only came up on /google but not on /invalid.

Looking at the code of httputil.ReverseProxy: If there is a proxy error then ServeHTTP immediately returns without applying ModifyResponse:
https://github.com/golang/go/blob/master/src/net/http/httputil/reverseproxy.go#L202

ModifyResponse is only applied in the good case:
https://github.com/golang/go/blob/master/src/net/http/httputil/reverseproxy.go#L222
@ALTree ALTree changed the title httputil.ReverseProxy does not call ModifyResponse on failed requests net/http/httputil: httputil.ReverseProxy does not call ModifyResponse on failed requests Aug 1, 2017
@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 1, 2017
@ALTree ALTree added this to the Go1.10 milestone Aug 1, 2017
@smasher164
Copy link
Member

smasher164 commented Aug 2, 2017 •
edited
Loading 

res, err := transport.RoundTrip(outreq)
merr := p.ModifyResponse(res)

which one should be logged? merr, err, or both?
Edit: I assume that ModifyResponse has priority. However, if it returns nil, would the original error value still be used to write a StatusBadGateway?

@sebsprenger
Copy link
Author

There is the Director to modify the input request and there is ModifyResponse to modify the output response. IMHO ModifyResponse ought to be ultimately responsible, because otherwise what's the point?

It would be good though, if ModifyResponse had access to some sort of feedback from the actual proxy to take that into consideration (e.g. error as argument).

@smasher164
Copy link
Member

to take that into consideration (e.g. error as argument).

The api for ModifyResponse can't change, so a simpler solution might be to just log the other error before checking this one.
However it would have to be ModifyResponse's responsibility to return an error for a nil *http.Response.

If no one minds, I can submit a CL for this. /cc @bradfitz

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/54030 mentions this issue: net/http/httputil: allow ReverseProxy to call ModifyResponse on failed requests

@smasher164
Copy link
Member

smasher164 commented Oct 12, 2017 •
edited
Loading

Any update on this issue/CL? /cc @mvdan

@bradfitz bradfitz mentioned this issue Nov 13, 2017
Closed
@bradfitz bradfitz reopened this Jan 6, 2018
@bradfitz bradfitz modified the milestones: Go1.10, Go1.11 Jan 6, 2018
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/86435 mentions this issue: Revert "net/http/httputil: allow ReverseProxy to call ModifyResponse on failed requests"

gopherbot pushed a commit that referenced this issue Jan 6, 2018
@bradfitz
Revert "net/http/httputil: allow ReverseProxy to call ModifyResponse …

Verified

This commit was signed with the committer’s verified signature.
hoel-bagard Hoël Bagard
GPG key ID: 4F2E04C61B7ED6D3
Verified
Learn about vigilant mode
24df1d0 
…on failed requests"

This reverts commit https://golang.org/cl/54030

Reason for revert: to not paint ourselves into a corner.
See #23009

Fixes #23009
Updates #21255

Change-Id: I68caab078839b9d2bf645a7bbed8405a5a30cd22
Reviewed-on: https://go-review.googlesource.com/86435
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/86476 mentions this issue: doc/go1.10: remove ReverseProxy TODO

gopherbot pushed a commit that referenced this issue Jan 6, 2018
@bradfitz
doc/go1.10: remove ReverseProxy TODO

Verified

This commit was signed with the committer’s verified signature.
hoel-bagard Hoël Bagard
GPG key ID: 4F2E04C61B7ED6D3
Verified
Learn about vigilant mode
3ea72fb 
No longer needs to be done.

Updates #23009
Updates #21255

Change-Id: I78e9e29a923dc03dea89ff3a5bf60f2e0bd0c0aa
Reviewed-on: https://go-review.googlesource.com/86476
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/77410 mentions this issue: http/http/httputil: add ReverseProxy.ErrorHandler

@golang golang locked and limited conversation to collaborators Jul 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Go1.11
Development

No branches or pull requests
5 participants
@bradfitz @ALTree @sebsprenger @gopherbot @smasher164