Refactors makeBackendRequest error handling (#1563)

* Removes unreachable branches from `net.Error` status code mapping * Splits roundTripper creation (fastcgi) and actual backend round trip Signed-off-by: Alexander Yastrebov <[email protected]>
zalando · Dec 1, 2020 · 315cdb5 · 315cdb5
1 parent a427211
commit 315cdb5
Show file tree

Hide file tree

Showing 2 changed files with 90 additions and 56 deletions.
diff --git a/proxy/proxy.go b/proxy/proxy.go
@@ -881,7 +881,6 @@ func (p *Proxy) makeUpgradeRequest(ctx *context, req *http.Request) error {
 }
 
 func (p *Proxy) makeBackendRequest(ctx *context) (*http.Response, *proxyError) {
-	var err error
 	req, endpoint, err := mapRequest(ctx.request, ctx.route, ctx.outgoingHost, p.flags.HopHeadersRemoval(), ctx.StateBag())
 	if err != nil {
 		p.log.Errorf("could not map backend request, caused by: %v", err)
@@ -902,6 +901,12 @@ func (p *Proxy) makeBackendRequest(ctx *context) (*http.Response, *proxyError) {
 		return nil, &proxyError{handled: true}
 	}
 
+	roundTripper, err := p.getRoundTripper(ctx, req)
+	if err != nil {
+		p.log.Errorf("Failed to get roundtripper: %v", err)
+		return nil, &proxyError{err: err, code: http.StatusBadGateway}
+	}
+
 	bag := ctx.StateBag()
 	spanName, ok := bag[tracingfilter.OpenTracingProxySpanKey].(string)
 	if !ok {
@@ -927,38 +932,7 @@ func (p *Proxy) makeBackendRequest(ctx *context) (*http.Response, *proxyError) {
 	ctx.proxySpan.LogKV("http_roundtrip", StartEvent)
 	req = injectClientTrace(req, ctx.proxySpan)
 
-	var response *http.Response
-	switch req.URL.Scheme {
-	case "fastcgi":
-		f := "index.php"
-		if sf, ok := ctx.StateBag()["fastCgiFilename"]; ok {
-			f = sf.(string)
-		}
-		rt, err := fastcgi.NewRoundTripper(p.log, req.URL.Host, f)
-		if err != nil {
-			p.log.Errorf("Failed to create fastcgi roundtripper: %v", err)
-
-			return nil, &proxyError{err: err}
-		}
-
-		// FastCgi expects the Host to be in form host:port
-		// It will then be split and added as 2 separate params to the backend process
-		if _, _, err := net.SplitHostPort(req.Host); err != nil {
-			req.Host = req.Host + ":" + req.URL.Port()
-		}
-
-		// RemoteAddr is needed to pass to the backend process as param
-		req.RemoteAddr = ctx.request.RemoteAddr
-
-		response, err = rt.RoundTrip(req)
-		if err != nil {
-			p.log.Errorf("Failed to roundtrip to fastcgi: %v", err)
-
-			return nil, &proxyError{err: err}
-		}
-	default:
-		response, err = p.roundTripper.RoundTrip(req)
-	}
+	response, err := roundTripper.RoundTrip(req)
 
 	ctx.proxySpan.LogKV("http_roundtrip", EndEvent)
 	if err != nil {
@@ -975,31 +949,16 @@ func (p *Proxy) makeBackendRequest(ctx *context) (*http.Response, *proxyError) {
 		} else if nerr, ok := err.(net.Error); ok {
 			p.log.Errorf("net.Error during backend roundtrip to %s: timeout=%v temporary=%v: %v", ctx.route.Backend, nerr.Timeout(), nerr.Temporary(), err)
 			//p.lb.AddHealthcheck(ctx.route.Backend)
+			var status int
 			if nerr.Timeout() {
-				p.tracing.setTag(ctx.proxySpan, HTTPStatusCodeTag, uint16(http.StatusGatewayTimeout))
-				return nil, &proxyError{
-					err:  err,
-					code: http.StatusGatewayTimeout,
-				}
-			} else if !nerr.Temporary() {
-				p.tracing.setTag(ctx.proxySpan, HTTPStatusCodeTag, uint16(http.StatusServiceUnavailable))
-				return nil, &proxyError{
-					err:  err,
-					code: http.StatusServiceUnavailable,
-				}
-			} else if !nerr.Timeout() && nerr.Temporary() {
-				p.log.Errorf("Backend error see https://github.com/zalando/skipper/issues/768: %v", err)
-				p.tracing.setTag(ctx.proxySpan, HTTPStatusCodeTag, uint16(http.StatusServiceUnavailable))
-				return nil, &proxyError{
-					err:  err,
-					code: http.StatusServiceUnavailable,
-				}
+				status = http.StatusGatewayTimeout
 			} else {
-				p.tracing.setTag(ctx.proxySpan, HTTPStatusCodeTag, uint16(http.StatusInternalServerError))
-				return nil, &proxyError{
-					err:  err,
-					code: http.StatusInternalServerError,
-				}
+				status = http.StatusServiceUnavailable
+			}
+			p.tracing.setTag(ctx.proxySpan, HTTPStatusCodeTag, uint16(status))
+			return nil, &proxyError{
+				err:  err,
+				code: status,
 			}
 		}
 
@@ -1016,6 +975,33 @@ func (p *Proxy) makeBackendRequest(ctx *context) (*http.Response, *proxyError) {
 	return response, nil
 }
 
+func (p *Proxy) getRoundTripper(ctx *context, req *http.Request) (http.RoundTripper, error) {
+	switch req.URL.Scheme {
+	case "fastcgi":
+		f := "index.php"
+		if sf, ok := ctx.StateBag()["fastCgiFilename"]; ok {
+			f = sf.(string)
+		}
+		rt, err := fastcgi.NewRoundTripper(p.log, req.URL.Host, f)
+		if err != nil {
+			return nil, err
+		}
+
+		// FastCgi expects the Host to be in form host:port
+		// It will then be split and added as 2 separate params to the backend process
+		if _, _, err := net.SplitHostPort(req.Host); err != nil {
+			req.Host = req.Host + ":" + req.URL.Port()
+		}
+
+		// RemoteAddr is needed to pass to the backend process as param
+		req.RemoteAddr = ctx.request.RemoteAddr
+
+		return rt, nil
+	default:
+		return p.roundTripper, nil
+	}
+}
+
 func (p *Proxy) checkBreaker(c *context) (func(bool), bool) {
 	if p.breakers == nil {
 		return nil, true

diff --git a/proxy/proxy_test.go b/proxy/proxy_test.go
@@ -639,6 +639,27 @@ func TestFastCgi(t *testing.T) {
 	}
 }
 
+func TestFastCgiServiceUnavailable(t *testing.T) {
+	tp, err := newTestProxy(`fastcgi: * -> "fastcgi://invalid.test"`, FlagsNone)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer tp.close()
+
+	ps := httptest.NewServer(tp.proxy)
+	defer ps.Close()
+
+	rsp, err := http.Get(ps.URL)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer rsp.Body.Close()
+
+	if rsp.StatusCode != http.StatusBadGateway {
+		t.Fatalf("expected 502, got: %v", rsp)
+	}
+}
+
 // This test is sensitive for timing, and occasionally fails.
 // To run this test, set `-args stream` for the test command.
 func TestStreaming(t *testing.T) {
@@ -1402,6 +1423,33 @@ func TestRoundtripperRetry(t *testing.T) {
 	}
 }
 
+func TestResponseHeaderTimeout(t *testing.T) {
+	s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		time.Sleep(5 * time.Microsecond)
+	}))
+	defer s.Close()
+
+	p, err := newTestProxyWithParams(fmt.Sprintf(`* -> "%s"`, s.URL), Params{ResponseHeaderTimeout: 1 * time.Microsecond})
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer p.proxy.Close()
+
+	ps := httptest.NewServer(p.proxy)
+	defer ps.Close()
+
+	// Prevent retry
+	rsp, err := http.Post(ps.URL, "text/plain", strings.NewReader("payload"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer rsp.Body.Close()
+
+	if rsp.StatusCode != http.StatusGatewayTimeout {
+		t.Fatalf("expected 504, got: %v", rsp)
+	}
+}
+
 func TestBranding(t *testing.T) {
 	routesTpl := `
         shunt: Path("/shunt") -> status(200) -> <shunt>;