From 489cf6391f818eceaf81ea5dda0d8c46956ad250 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Tue, 2 Apr 2024 21:10:52 +0200 Subject: [PATCH 1/2] Exit early if we don't get a HTTP response at all MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The idea of a StatusTooManyRequests retry loop, or the needsRetryWithUpdatedScope logic, only makes sense if we do get a response; on other errors, we can exit immediately. So do that, and simplify the code. Should not change behavior. Signed-off-by: Miloslav Trmač --- docker/docker_client.go | 17 ++++++++++++----- docker/docker_client_test.go | 22 +++++++--------------- 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/docker/docker_client.go b/docker/docker_client.go index 1f8078bd08..bd93bb5844 100644 --- a/docker/docker_client.go +++ b/docker/docker_client.go @@ -498,8 +498,8 @@ func (c *dockerClient) resolveRequestURL(path string) (*url.URL, error) { // Checks if the auth headers in the response contain an indication of a failed // authorizdation because of an "insufficient_scope" error. If that's the case, // returns the required scope to be used for fetching a new token. -func needsRetryWithUpdatedScope(err error, res *http.Response) (bool, *authScope) { - if err == nil && res.StatusCode == http.StatusUnauthorized { +func needsRetryWithUpdatedScope(res *http.Response) (bool, *authScope) { + if res.StatusCode == http.StatusUnauthorized { challenges := parseAuthHeader(res.Header) for _, challenge := range challenges { if challenge.Scheme == "bearer" { @@ -558,6 +558,9 @@ func (c *dockerClient) makeRequestToResolvedURL(ctx context.Context, method stri attempts := 0 for { res, err := c.makeRequestToResolvedURLOnce(ctx, method, requestURL, headers, stream, streamLen, auth, extraScope) + if err != nil { + return nil, err + } attempts++ // By default we use pre-defined scopes per operation. In @@ -573,19 +576,23 @@ func (c *dockerClient) makeRequestToResolvedURL(ctx context.Context, method stri // We also cannot retry with a body (stream != nil) as stream // was already read if attempts == 1 && stream == nil && auth != noAuth { - if retry, newScope := needsRetryWithUpdatedScope(err, res); retry { + if retry, newScope := needsRetryWithUpdatedScope(res); retry { logrus.Debug("Detected insufficient_scope error, will retry request with updated scope") // Note: This retry ignores extraScope. That’s, strictly speaking, incorrect, but we don’t currently // expect the insufficient_scope errors to happen for those callers. If that changes, we can add support // for more than one extra scope. res, err = c.makeRequestToResolvedURLOnce(ctx, method, requestURL, headers, stream, streamLen, auth, newScope) + if err != nil { + return nil, err + } extraScope = newScope } } - if res == nil || res.StatusCode != http.StatusTooManyRequests || // Only retry on StatusTooManyRequests, success or other failure is returned to caller immediately + + if res.StatusCode != http.StatusTooManyRequests || // Only retry on StatusTooManyRequests, success or other failure is returned to caller immediately stream != nil || // We can't retry with a body (which is not restartable in the general case) attempts == backoffNumIterations { - return res, err + return res, nil } // close response body before retry or context done res.Body.Close() diff --git a/docker/docker_client_test.go b/docker/docker_client_test.go index 67e38764b4..4c57fe02dc 100644 --- a/docker/docker_client_test.go +++ b/docker/docker_client_test.go @@ -4,7 +4,6 @@ import ( "bufio" "bytes" "context" - "errors" "fmt" "net/http" "net/http/httptest" @@ -192,13 +191,6 @@ func TestUserAgent(t *testing.T) { } } -func TestNeedsRetryOnError(t *testing.T) { - needsRetry, _ := needsRetryWithUpdatedScope(errors.New("generic"), nil) - if needsRetry { - t.Fatal("Got needRetry for a connection that included an error") - } -} - var registrySuseComResp = http.Response{ Status: "401 Unauthorized", StatusCode: http.StatusUnauthorized, @@ -227,7 +219,7 @@ func TestNeedsRetryOnInsuficientScope(t *testing.T) { actions: "*", } - needsRetry, scope := needsRetryWithUpdatedScope(nil, &resp) + needsRetry, scope := needsRetryWithUpdatedScope(&resp) if !needsRetry { t.Fatal("Expected needing to retry") @@ -242,7 +234,7 @@ func TestNeedsRetryNoRetryWhenNoAuthHeader(t *testing.T) { resp := registrySuseComResp delete(resp.Header, "Www-Authenticate") - needsRetry, _ := needsRetryWithUpdatedScope(nil, &resp) + needsRetry, _ := needsRetryWithUpdatedScope(&resp) if needsRetry { t.Fatal("Expected no need to retry, as no Authentication headers are present") @@ -255,7 +247,7 @@ func TestNeedsRetryNoRetryWhenNoBearerAuthHeader(t *testing.T) { `OAuth2 realm="https://registry.suse.com/auth",service="SUSE Linux Docker Registry",scope="registry:catalog:*"`, } - needsRetry, _ := needsRetryWithUpdatedScope(nil, &resp) + needsRetry, _ := needsRetryWithUpdatedScope(&resp) if needsRetry { t.Fatal("Expected no need to retry, as no bearer authentication header is present") @@ -268,7 +260,7 @@ func TestNeedsRetryNoRetryWhenNoErrorInBearer(t *testing.T) { `Bearer realm="https://registry.suse.com/auth",service="SUSE Linux Docker Registry",scope="registry:catalog:*"`, } - needsRetry, _ := needsRetryWithUpdatedScope(nil, &resp) + needsRetry, _ := needsRetryWithUpdatedScope(&resp) if needsRetry { t.Fatal("Expected no need to retry, as no insufficient error is present in the authentication header") @@ -281,7 +273,7 @@ func TestNeedsRetryNoRetryWhenInvalidErrorInBearer(t *testing.T) { `Bearer realm="https://registry.suse.com/auth",service="SUSE Linux Docker Registry",scope="registry:catalog:*,error="random_error"`, } - needsRetry, _ := needsRetryWithUpdatedScope(nil, &resp) + needsRetry, _ := needsRetryWithUpdatedScope(&resp) if needsRetry { t.Fatal("Expected no need to retry, as no insufficient_error is present in the authentication header") @@ -294,7 +286,7 @@ func TestNeedsRetryNoRetryWhenInvalidScope(t *testing.T) { `Bearer realm="https://registry.suse.com/auth",service="SUSE Linux Docker Registry",scope="foo:bar",error="insufficient_scope"`, } - needsRetry, _ := needsRetryWithUpdatedScope(nil, &resp) + needsRetry, _ := needsRetryWithUpdatedScope(&resp) if needsRetry { t.Fatal("Expected no need to retry, as no insufficient_error is present in the authentication header") @@ -326,7 +318,7 @@ func TestNeedsNoRetry(t *testing.T) { }, } - needsRetry, _ := needsRetryWithUpdatedScope(nil, &resp) + needsRetry, _ := needsRetryWithUpdatedScope(&resp) if needsRetry { t.Fatal("Got the need to retry, but none should be required") } From 77f9d7b5854e37a734f9582fd301d7d7a2c9be24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Tue, 2 Apr 2024 21:17:04 +0200 Subject: [PATCH 2/2] Make sure we close the response body before retrying MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Not doing so can keep the HTTP client code waiting forever, keeping some goroutines running, and the socket open. Signed-off-by: Miloslav Trmač --- docker/docker_client.go | 1 + 1 file changed, 1 insertion(+) diff --git a/docker/docker_client.go b/docker/docker_client.go index bd93bb5844..79783316c0 100644 --- a/docker/docker_client.go +++ b/docker/docker_client.go @@ -578,6 +578,7 @@ func (c *dockerClient) makeRequestToResolvedURL(ctx context.Context, method stri if attempts == 1 && stream == nil && auth != noAuth { if retry, newScope := needsRetryWithUpdatedScope(res); retry { logrus.Debug("Detected insufficient_scope error, will retry request with updated scope") + res.Body.Close() // Note: This retry ignores extraScope. That’s, strictly speaking, incorrect, but we don’t currently // expect the insufficient_scope errors to happen for those callers. If that changes, we can add support // for more than one extra scope.