From 55535552fae6fe30aca20dc179c7e865533b04c4 Mon Sep 17 00:00:00 2001 From: Calum Murray Date: Thu, 1 Aug 2024 10:19:03 -0400 Subject: [PATCH 1/2] fix: http probe downgrades from http2 to http1 when it encounters an error Signed-off-by: Calum Murray --- pkg/queue/health/probe.go | 11 +++++---- pkg/queue/health/probe_test.go | 42 ++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 4 deletions(-) diff --git a/pkg/queue/health/probe.go b/pkg/queue/health/probe.go index 2aa0d6e27e4d..23c893b4388b 100644 --- a/pkg/queue/health/probe.go +++ b/pkg/queue/health/probe.go @@ -162,18 +162,21 @@ func http2UpgradeProbe(config HTTPProbeConfigOptions) (int, error) { func HTTPProbe(config HTTPProbeConfigOptions) error { if config.MaxProtoMajor == 0 { // If we don't know if the connection supports HTTP2, we will try it. - // Once we get a non-error response, we won't try again. // If maxProto is 0, container is not ready, so we don't know whether http2 is supported. // If maxProto is 1, we know we're ready, but we also can't upgrade, so just return. // If maxProto is 2, we know we can upgrade to http2 + // If we get an error response, we will assume that the connection is HTTP1, as we can not guarantee if the service can handle the OPTIONS request maxProto, err := http2UpgradeProbe(config) - if err != nil { - return fmt.Errorf("failed to run HTTP2 upgrade probe with error: %w", err) - } config.MaxProtoMajor = maxProto if config.MaxProtoMajor == 1 { return nil } + + if err != nil { + // we default to HTTP1 because the options request did not work + // however, we do not know if the container is ready yet, so we still need to probe + config.MaxProtoMajor = 1 + } } httpClient := &http.Client{ Transport: autoDowngradingTransport(config), diff --git a/pkg/queue/health/probe_test.go b/pkg/queue/health/probe_test.go index 3afbbc73eb04..bec2e6eac11a 100644 --- a/pkg/queue/health/probe_test.go +++ b/pkg/queue/health/probe_test.go @@ -201,6 +201,48 @@ func TestHTTPProbeAutoHTTP2(t *testing.T) { } } +func TestHTTPProbeAutoHTTP2FailedUpgrade(t *testing.T) { + h2cHeaders := map[string]string{ + "Connection": "Upgrade, HTTP2-Settings", + "Upgrade": "h2c", + } + expectedPath := "/health" + var callCount atomic.Int32 + + server := newH2cTestServer(t, func(w http.ResponseWriter, r *http.Request) { + count := callCount.Inc() + if r.Method == http.MethodOptions { + // make sure that the correct headers are present on the options request + for key, value := range h2cHeaders { + if r.Header.Get(key) != value { + t.Errorf("Key %v = %v was supposed to be present in the request", key, value) + } + } + w.WriteHeader(http.StatusInternalServerError) + } else { + w.WriteHeader(http.StatusOK) + } + if count > 2 { + t.Error("Expected probe to make two requests only") + } + }) + + action := newHTTPGetAction(t, server.URL) + action.Path = expectedPath + + config := HTTPProbeConfigOptions{ + Timeout: time.Second, + HTTPGetAction: action, + MaxProtoMajor: 0, + } + if err := HTTPProbe(config); err != nil { + t.Error("Expected probe to succeed but it failed with", err) + } + if count := callCount.Load(); count != 2 { + t.Errorf("Unexpected call count %d", count) + } +} + func TestHTTPSchemeProbeSuccess(t *testing.T) { server := newTestServer(t, func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) From cd205ce44f93ffc3d7ea7e157c09574e8a1ba78a Mon Sep 17 00:00:00 2001 From: Calum Murray Date: Thu, 1 Aug 2024 10:47:46 -0400 Subject: [PATCH 2/2] fix: retry the options request until a probe succeeds for a protocol Signed-off-by: Calum Murray --- pkg/queue/health/probe.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/pkg/queue/health/probe.go b/pkg/queue/health/probe.go index 23c893b4388b..33fe024a0cbf 100644 --- a/pkg/queue/health/probe.go +++ b/pkg/queue/health/probe.go @@ -160,7 +160,8 @@ func http2UpgradeProbe(config HTTPProbeConfigOptions) (int, error) { // HTTPProbe checks that HTTP connection can be established to the address. func HTTPProbe(config HTTPProbeConfigOptions) error { - if config.MaxProtoMajor == 0 { + configMaxProto := config.MaxProtoMajor + if configMaxProto == 0 { // If we don't know if the connection supports HTTP2, we will try it. // If maxProto is 0, container is not ready, so we don't know whether http2 is supported. // If maxProto is 1, we know we're ready, but we also can't upgrade, so just return. @@ -199,6 +200,10 @@ func HTTPProbe(config HTTPProbeConfigOptions) error { res, err := httpClient.Do(req) if err != nil { + if configMaxProto == 0 { + // we guessed that http1 was supported, but it also failed. Let's try the options request again next probe + config.MaxProtoMajor = 0 + } return err } @@ -210,6 +215,10 @@ func HTTPProbe(config HTTPProbeConfigOptions) error { }() if !isHTTPProbeReady(res) { + if configMaxProto == 0 { + // we guessed that http1 was supported, but it also failed. Let's try the options request again next probe + config.MaxProtoMajor = 0 + } return fmt.Errorf("HTTP probe did not respond Ready, got status code: %d", res.StatusCode) }