Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix a http.response.Body leak on a permission error #2363

Merged
merged 2 commits into from
Apr 8, 2024
Merged

Fix a http.response.Body leak on a permission error #2363

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
489cf63
Exit early if we don't get a HTTP response at all
mtrmac Apr 2, 2024
77f9d7b
Make sure we close the response body before retrying
mtrmac Apr 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions docker/docker_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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" {
Expand Down Expand Up @@ -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
Expand All @@ -573,19 +576,24 @@ 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")
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.
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()
Expand Down
22 changes: 7 additions & 15 deletions docker/docker_client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"bufio"
"bytes"
"context"
"errors"
"fmt"
"net/http"
"net/http/httptest"
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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")
Expand All @@ -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")
Expand All @@ -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")
Expand All @@ -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")
Expand All @@ -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")
Expand All @@ -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")
Expand Down Expand Up @@ -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")
}
Expand Down