net/http/httputil: allow ReverseProxy to call ModifyResponse on failed requests

Previously when RoundTrip returned a non-nil error, the proxy returned a
StatusBadGateway error, instead of first calling ModifyResponse. This
commit first calls ModifyResponse, whether or not the error returned
from RoundTrip is nil.

Also closes response body when ModifyResponse returns an error. See #22658.

Fixes #21255

Change-Id: I5b5bf23a69ae5608f87d4ece756a1b4985ccaa9c
Reviewed-on: https://go-review.googlesource.com/54030
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

diff --git a/src/net/http/httputil/reverseproxy.go b/src/net/http/httputil/reverseproxy.go
index b96bb21019b0fc1492c4086c9f6c75867b38d1d2..aa22d5a2fdd3a22e277cca6acaf9a9406796f5a0 100644 (file)
--- a/src/net/http/httputil/reverseproxy.go
+++ b/src/net/http/httputil/reverseproxy.go
@@ -191,10 +191,11 @@ func (p *ReverseProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
        }
 
        res, err := transport.RoundTrip(outreq)
-       if err != nil {
-               p.logf("http: proxy error: %v", err)
-               rw.WriteHeader(http.StatusBadGateway)
-               return
+       if res == nil {
+               res = &http.Response{
+                       StatusCode: http.StatusBadGateway,
+                       Body:       http.NoBody,
+               }
        }
 
        removeConnectionHeaders(res.Header)
@@ -204,12 +205,16 @@ func (p *ReverseProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
        }
 
        if p.ModifyResponse != nil {
-               if err := p.ModifyResponse(res); err != nil {
+               if err != nil {
                        p.logf("http: proxy error: %v", err)
-                       rw.WriteHeader(http.StatusBadGateway)
-                       res.Body.Close()
-                       return
                }
+               err = p.ModifyResponse(res)
+       }
+       if err != nil {
+               p.logf("http: proxy error: %v", err)
+               rw.WriteHeader(http.StatusBadGateway)
+               res.Body.Close()
+               return
        }
 
        copyHeader(rw.Header(), res.Header)

diff --git a/src/net/http/httputil/reverseproxy_test.go b/src/net/http/httputil/reverseproxy_test.go
index 2232042d3ed14eec76116bc0b778e5b14e3fdb74..822828e5c0dce3724e887dc9d675aba8313fe20f 100644 (file)
--- a/src/net/http/httputil/reverseproxy_test.go
+++ b/src/net/http/httputil/reverseproxy_test.go
@@ -631,6 +631,35 @@ func TestReverseProxyModifyResponse(t *testing.T) {
        }
 }
 
+// Issue 21255. Test ModifyResponse when an error from transport.RoundTrip
+// occurs, and that the proxy returns StatusOK.
+func TestReverseProxyModifyResponse_OnError(t *testing.T) {
+       // Always returns an error
+       errBackend := httptest.NewUnstartedServer(nil)
+       errBackend.Config.ErrorLog = log.New(ioutil.Discard, "", 0) // quiet for tests
+       defer errBackend.Close()
+
+       rpURL, _ := url.Parse(errBackend.URL)
+       rproxy := NewSingleHostReverseProxy(rpURL)
+       rproxy.ModifyResponse = func(resp *http.Response) error {
+               // Will be set for a non-nil error
+               resp.StatusCode = http.StatusOK
+               return nil
+       }
+
+       frontend := httptest.NewServer(rproxy)
+       defer frontend.Close()
+
+       resp, err := http.Get(frontend.URL)
+       if err != nil {
+               t.Fatalf("failed to reach proxy: %v", err)
+       }
+       if resp.StatusCode != http.StatusOK {
+               t.Errorf("err != nil: got res.StatusCode %d; expected %d", resp.StatusCode, http.StatusOK)
+       }
+       resp.Body.Close()
+}
+
 // Issue 16659: log errors from short read
 func TestReverseProxy_CopyBuffer(t *testing.T) {
        backendServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {