diff --git a/proxy/internal/roundtrip/upstream.go b/proxy/internal/roundtrip/upstream.go index 8f582b0d2..d6d11a26a 100644 --- a/proxy/internal/roundtrip/upstream.go +++ b/proxy/internal/roundtrip/upstream.go @@ -10,11 +10,33 @@ import ( ) // upstreamDowngradeTTL is how long an upstream stays pinned to HTTP/1.1 -// after it proved it cannot serve the h2 it advertised. Bounded rather -// than permanent so a fixed or replaced backend returns to h2 without -// restarting the proxy. +// after an h2 failure that only implied it cannot serve h2. Bounded so a +// fixed or replaced backend returns to h2 without restarting the proxy. +// A pin the upstream asked for itself does not expire — see downgrade. const upstreamDowngradeTTL = 10 * time.Minute +// downgrade is an upstream's HTTP/1.1 pin. +type downgrade struct { + // expiry is when the pin lapses and the upstream is offered h2 + // again. The zero time means it never does: the upstream answered + // HTTP_1_1_REQUIRED, which is a statement about how it is + // configured, not a fault that may clear on its own. Re-probing + // that every upstreamDowngradeTTL would buy nothing but a failed + // request per interval, so the pin holds until the transport goes + // away with the proxy or the account's client. + expiry time.Time +} + +// permanent reports whether the upstream asked for this pin itself. +func (d downgrade) permanent() bool { + return d.expiry.IsZero() +} + +// active reports whether the pin still stands at now. +func (d downgrade) active(now time.Time) bool { + return d.permanent() || now.Before(d.expiry) +} + // upstreamTransport carries requests to a single upstream family (one // TLS configuration) and implements what upstreamHTTPAuto means. // @@ -44,9 +66,8 @@ type upstreamTransport struct { fallback *http.Transport mu sync.RWMutex - // downgraded maps an upstream host to the time its HTTP/1.1 pin - // expires. - downgraded map[string]time.Time + // downgraded maps an upstream host to its HTTP/1.1 pin. + downgraded map[string]downgrade } // newUpstreamTransport wraps base for the requested HTTP version. base @@ -62,7 +83,7 @@ func newUpstreamTransport(base *http.Transport, version upstreamHTTPVersion, log primary: base, version: version, logger: logger, - downgraded: make(map[string]time.Time), + downgraded: make(map[string]downgrade), } } @@ -82,7 +103,11 @@ func (t *upstreamTransport) RoundTrip(req *http.Request) (*http.Response, error) return resp, err } - t.markDowngraded(host) + // HTTP_1_1_REQUIRED is the upstream saying it will not serve this + // request over h2 however often it is asked — IIS answers it for + // Windows Authentication and for client-certificate sites, where + // the cause is site configuration rather than a passing fault. + t.markDowngraded(host, isHTTP11Required(err)) retry, ok := replayable(req) if !ok { @@ -111,21 +136,23 @@ func (t *upstreamTransport) mayDowngrade(req *http.Request) bool { } func (t *upstreamTransport) isDowngraded(host string) bool { + now := time.Now() + t.mu.RLock() - expiry, ok := t.downgraded[host] + pin, ok := t.downgraded[host] t.mu.RUnlock() if !ok { return false } - if time.Now().Before(expiry) { + if pin.active(now) { return true } t.mu.Lock() // Re-check under the write lock: a concurrent request may have // re-pinned the host after the read above. - if expiry, ok := t.downgraded[host]; ok && !time.Now().Before(expiry) { + if pin, ok := t.downgraded[host]; ok && !pin.active(time.Now()) { delete(t.downgraded, host) } t.mu.Unlock() @@ -133,24 +160,41 @@ func (t *upstreamTransport) isDowngraded(host string) bool { return false } -func (t *upstreamTransport) markDowngraded(host string) { +// markDowngraded pins host to HTTP/1.1. permanent marks a pin the +// upstream asked for; anything else lapses after upstreamDowngradeTTL so +// a repaired backend is offered h2 again. +func (t *upstreamTransport) markDowngraded(host string, permanent bool) { now := time.Now() + pin := downgrade{expiry: now.Add(upstreamDowngradeTTL)} + if permanent { + pin = downgrade{} + } t.mu.Lock() - _, pinned := t.downgraded[host] - t.downgraded[host] = now.Add(upstreamDowngradeTTL) - for h, expiry := range t.downgraded { - if !now.Before(expiry) { + previous, pinned := t.downgraded[host] + // A permanent pin is never weakened back into an expiring one: the + // upstream has already said h2 is not on offer. + if !pinned || !previous.permanent() { + t.downgraded[host] = pin + } + for h, existing := range t.downgraded { + if !existing.active(now) { delete(t.downgraded, h) } } t.mu.Unlock() - if !pinned { - t.logger.WithField("upstream", host). - Warnf("upstream negotiated HTTP/2 but failed to serve it, using HTTP/1.1 for the next %s (set %s=1.1 to pin it)", - upstreamDowngradeTTL, EnvUpstreamHTTPVersion) + if pinned { + return } + + entry := t.logger.WithField("upstream", host) + if permanent { + entry.Warnf("upstream answered HTTP_1_1_REQUIRED, using HTTP/1.1 for it from now on") + return + } + entry.Warnf("upstream negotiated HTTP/2 but failed to serve it, using HTTP/1.1 for the next %s (set %s=1.1 to pin it)", + upstreamDowngradeTTL, EnvUpstreamHTTPVersion) } // http1 returns the HTTP/1.1-only clone, creating it on first use. @@ -216,9 +260,20 @@ var http2ErrorMarkers = []string{ "stream error: stream ID", // http2.ConnectionError, e.g. "connection error: PROTOCOL_ERROR". "connection error: ", - // The GOAWAY code an upstream sends to say the request must be - // retried over HTTP/1.1. - "HTTP_1_1_REQUIRED", + // The code an upstream sends to say the request must be retried + // over HTTP/1.1, as a GOAWAY or on the stream. + http11RequiredMarker, +} + +// http11RequiredMarker is the error code an upstream sends to say the +// request belongs on HTTP/1.1. Unlike the other markers it is not a +// fault: the upstream is describing its own configuration. +const http11RequiredMarker = "HTTP_1_1_REQUIRED" + +// isHTTP11Required reports whether the upstream itself asked for +// HTTP/1.1, rather than merely failing at h2. +func isHTTP11Required(err error) bool { + return err != nil && strings.Contains(err.Error(), http11RequiredMarker) } // isHTTP2ProtocolError reports whether err says the upstream cannot diff --git a/proxy/internal/roundtrip/upstream_test.go b/proxy/internal/roundtrip/upstream_test.go index cc8b2ab33..6c24e5c47 100644 --- a/proxy/internal/roundtrip/upstream_test.go +++ b/proxy/internal/roundtrip/upstream_test.go @@ -47,6 +47,11 @@ func TestUpstreamTransport_AutoFallsBackOnBrokenHTTP2(t *testing.T) { assert.True(t, mt.insecure.isDowngraded(srv.addr), "the upstream must stay pinned to HTTP/1.1 after proving it cannot serve h2") + mt.insecure.mu.RLock() + pin := mt.insecure.downgraded[srv.addr] + mt.insecure.mu.RUnlock() + assert.True(t, pin.permanent(), + "an upstream answering HTTP_1_1_REQUIRED must not be re-probed for h2") // The second request must not repeat the h2 attempt: the server // counts h2 handshakes, so a repeat would show up here. @@ -110,11 +115,11 @@ func TestUpstreamTransport_MayDowngrade(t *testing.T) { func TestUpstreamTransport_DowngradeExpires(t *testing.T) { transport := newUpstreamTransport(&http.Transport{}, upstreamHTTPAuto, nil) - transport.markDowngraded("backend.invalid:443") + transport.markDowngraded("backend.invalid:443", false) require.True(t, transport.isDowngraded("backend.invalid:443")) transport.mu.Lock() - transport.downgraded["backend.invalid:443"] = time.Now().Add(-time.Second) + transport.downgraded["backend.invalid:443"] = downgrade{expiry: time.Now().Add(-time.Second)} transport.mu.Unlock() assert.False(t, transport.isDowngraded("backend.invalid:443"), @@ -127,13 +132,75 @@ func TestUpstreamTransport_DowngradeExpires(t *testing.T) { func TestUpstreamTransport_DowngradeIsPerUpstream(t *testing.T) { transport := newUpstreamTransport(&http.Transport{}, upstreamHTTPAuto, nil) - transport.markDowngraded("broken.invalid:443") + transport.markDowngraded("broken.invalid:443", false) assert.True(t, transport.isDowngraded("broken.invalid:443")) assert.False(t, transport.isDowngraded("healthy.invalid:443"), "one broken upstream must not drop the others to HTTP/1.1") } +// TestUpstreamTransport_HTTP11RequiredPinIsPermanent covers the IIS +// case: HTTP_1_1_REQUIRED describes how the upstream is configured +// (Windows Authentication, client certificates), so re-probing it every +// upstreamDowngradeTTL would only buy a failed request per interval. +func TestUpstreamTransport_HTTP11RequiredPinIsPermanent(t *testing.T) { + transport := newUpstreamTransport(&http.Transport{}, upstreamHTTPAuto, nil) + transport.markDowngraded("iis.invalid:443", true) + + transport.mu.RLock() + pin := transport.downgraded["iis.invalid:443"] + transport.mu.RUnlock() + + assert.True(t, pin.permanent(), "an upstream that asked for HTTP/1.1 must not be re-probed") + assert.True(t, pin.active(time.Now().Add(100*upstreamDowngradeTTL)), + "a permanent pin must outlive any TTL") +} + +func TestUpstreamTransport_PermanentPinSurvivesLaterFailures(t *testing.T) { + transport := newUpstreamTransport(&http.Transport{}, upstreamHTTPAuto, nil) + transport.markDowngraded("iis.invalid:443", true) + // A later ambiguous failure for the same upstream must not turn the + // permanent pin into an expiring one. + transport.markDowngraded("iis.invalid:443", false) + + transport.mu.RLock() + pin := transport.downgraded["iis.invalid:443"] + transport.mu.RUnlock() + + assert.True(t, pin.permanent(), "a permanent pin must never be weakened") +} + +func TestIsHTTP11Required(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + { + name: "goaway", + err: errors.New(`http2: server sent GOAWAY and closed the connection; LastStreamID=0, ErrCode=HTTP_1_1_REQUIRED, debug=""`), + want: true, + }, + { + name: "stream error", + err: http2.StreamError{StreamID: 1, Code: http2.ErrCodeHTTP11Required}, + want: true, + }, + { + name: "other h2 failure", + err: http2.StreamError{StreamID: 1, Code: http2.ErrCodeProtocol}, + want: false, + }, + {name: "nil", err: nil, want: false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, isHTTP11Required(tc.err)) + }) + } +} + func TestIsHTTP2ProtocolError(t *testing.T) { tests := []struct { name string