Merge pull request #2454 from mtrmac/confirm-k8s-hypothesis

Add more context to errors when obtaining a registry access token
containers · Jun 24, 2024 · 619f28e · 619f28e
2 parents 85f07ef + 2044bb9
commit 619f28e
Show file tree

Hide file tree

Showing 2 changed files with 65 additions and 71 deletions.
diff --git a/docker/docker_client.go b/docker/docker_client.go
@@ -147,10 +147,22 @@ const (
 	noAuth
 )
 
-func newBearerTokenFromJSONBlob(blob []byte) (*bearerToken, error) {
+// newBearerTokenFromHTTPResponseBody parses a http.Response to obtain a bearerToken.
+// The caller is still responsible for ensuring res.Body is closed.
+func newBearerTokenFromHTTPResponseBody(res *http.Response) (*bearerToken, error) {
+	blob, err := iolimits.ReadAtMost(res.Body, iolimits.MaxAuthTokenBodySize)
+	if err != nil {
+		return nil, err
+	}
+
 	token := new(bearerToken)
 	if err := json.Unmarshal(blob, &token); err != nil {
-		return nil, err
+		const bodySampleLength = 50
+		bodySample := blob
+		if len(bodySample) > bodySampleLength {
+			bodySample = bodySample[:bodySampleLength]
+		}
+		return nil, fmt.Errorf("decoding bearer token (last URL %q, body start %q): %w", res.Request.URL.Redacted(), string(bodySample), err)
 	}
 	if token.Token == "" {
 		token.Token = token.AccessToken
@@ -827,12 +839,7 @@ func (c *dockerClient) getBearerTokenOAuth2(ctx context.Context, challenge chall
 		return nil, err
 	}
 
-	tokenBlob, err := iolimits.ReadAtMost(res.Body, iolimits.MaxAuthTokenBodySize)
-	if err != nil {
-		return nil, err
-	}
-
-	return newBearerTokenFromJSONBlob(tokenBlob)
+	return newBearerTokenFromHTTPResponseBody(res)
 }
 
 func (c *dockerClient) getBearerToken(ctx context.Context, challenge challenge,
@@ -878,12 +885,8 @@ func (c *dockerClient) getBearerToken(ctx context.Context, challenge challenge,
 	if err := httpResponseToError(res, "Requesting bearer token"); err != nil {
 		return nil, err
 	}
-	tokenBlob, err := iolimits.ReadAtMost(res.Body, iolimits.MaxAuthTokenBodySize)
-	if err != nil {
-		return nil, err
-	}
 
-	return newBearerTokenFromJSONBlob(tokenBlob)
+	return newBearerTokenFromHTTPResponseBody(res)
 }
 
 // detectPropertiesHelper performs the work of detectProperties which executes

diff --git a/docker/docker_client_test.go b/docker/docker_client_test.go
@@ -5,8 +5,10 @@ import (
 	"bytes"
 	"context"
 	"fmt"
+	"io"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"path/filepath"
 	"strings"
 	"testing"
@@ -91,72 +93,61 @@ func TestDockerCertDir(t *testing.T) {
 	}
 }
 
-func TestNewBearerTokenFromJsonBlob(t *testing.T) {
-	expected := &bearerToken{Token: "IAmAToken", ExpiresIn: 100, IssuedAt: time.Unix(1514800802, 0)}
-	tokenBlob := []byte(`{"token":"IAmAToken","expires_in":100,"issued_at":"2018-01-01T10:00:02+00:00"}`)
-	token, err := newBearerTokenFromJSONBlob(tokenBlob)
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-
-	assertBearerTokensEqual(t, expected, token)
-}
-
-func TestNewBearerAccessTokenFromJsonBlob(t *testing.T) {
-	expected := &bearerToken{Token: "IAmAToken", ExpiresIn: 100, IssuedAt: time.Unix(1514800802, 0)}
-	tokenBlob := []byte(`{"access_token":"IAmAToken","expires_in":100,"issued_at":"2018-01-01T10:00:02+00:00"}`)
-	token, err := newBearerTokenFromJSONBlob(tokenBlob)
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-
-	assertBearerTokensEqual(t, expected, token)
-}
-
-func TestNewBearerTokenFromInvalidJsonBlob(t *testing.T) {
-	tokenBlob := []byte("IAmNotJson")
-	_, err := newBearerTokenFromJSONBlob(tokenBlob)
-	if err == nil {
-		t.Fatalf("unexpected an error unmarshaling JSON")
+// testTokenHTTPResponse creates just enough of a *http.Response to work with newBearerTokenFromHTTPResponseBody.
+func testTokenHTTPResponse(t *testing.T, body string) *http.Response {
+	requestURL, err := url.Parse("https://example.com/token")
+	require.NoError(t, err)
+	return &http.Response{
+		Body: io.NopCloser(bytes.NewReader([]byte(body))),
+		Request: &http.Request{
+			Method: "",
+			URL:    requestURL,
+		},
 	}
 }
 
-func TestNewBearerTokenSmallExpiryFromJsonBlob(t *testing.T) {
-	expected := &bearerToken{Token: "IAmAToken", ExpiresIn: 60, IssuedAt: time.Unix(1514800802, 0)}
-	tokenBlob := []byte(`{"token":"IAmAToken","expires_in":1,"issued_at":"2018-01-01T10:00:02+00:00"}`)
-	token, err := newBearerTokenFromJSONBlob(tokenBlob)
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
+func TestNewBearerTokenFromHTTPResponseBody(t *testing.T) {
+	for _, c := range []struct {
+		input    string
+		expected *bearerToken // or nil if a failure is expected
+	}{
+		{ // Invalid JSON
+			input:    "IAmNotJson",
+			expected: nil,
+		},
+		{ // "token"
+			input:    `{"token":"IAmAToken","expires_in":100,"issued_at":"2018-01-01T10:00:02+00:00"}`,
+			expected: &bearerToken{Token: "IAmAToken", ExpiresIn: 100, IssuedAt: time.Unix(1514800802, 0)},
+		},
+		{ // "access_token"
+			input:    `{"access_token":"IAmAToken","expires_in":100,"issued_at":"2018-01-01T10:00:02+00:00"}`,
+			expected: &bearerToken{Token: "IAmAToken", ExpiresIn: 100, IssuedAt: time.Unix(1514800802, 0)},
+		},
+		{ // Small expiry
+			input:    `{"token":"IAmAToken","expires_in":1,"issued_at":"2018-01-01T10:00:02+00:00"}`,
+			expected: &bearerToken{Token: "IAmAToken", ExpiresIn: 60, IssuedAt: time.Unix(1514800802, 0)},
+		},
+	} {
+		token, err := newBearerTokenFromHTTPResponseBody(testTokenHTTPResponse(t, c.input))
+		if c.expected == nil {
+			assert.Error(t, err, c.input)
+		} else {
+			require.NoError(t, err, c.input)
+			assert.Equal(t, c.expected.Token, token.Token, c.input)
+			assert.Equal(t, c.expected.ExpiresIn, token.ExpiresIn, c.input)
+			assert.True(t, c.expected.IssuedAt.Equal(token.IssuedAt),
+				"expected [%s] to equal [%s], it did not", token.IssuedAt, c.expected.IssuedAt)
+		}
 	}
-
-	assertBearerTokensEqual(t, expected, token)
 }
 
-func TestNewBearerTokenIssuedAtZeroFromJsonBlob(t *testing.T) {
+func TestNewBearerTokenFromHTTPResponseBodyIssuedAtZero(t *testing.T) {
 	zeroTime := time.Time{}.Format(time.RFC3339)
 	now := time.Now()
-	tokenBlob := []byte(fmt.Sprintf(`{"token":"IAmAToken","expires_in":100,"issued_at":"%s"}`, zeroTime))
-	token, err := newBearerTokenFromJSONBlob(tokenBlob)
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-
-	if token.IssuedAt.Before(now) {
-		t.Fatalf("expected [%s] not to be before [%s]", token.IssuedAt, now)
-	}
-
-}
-
-func assertBearerTokensEqual(t *testing.T, expected, subject *bearerToken) {
-	if expected.Token != subject.Token {
-		t.Fatalf("expected [%s] to equal [%s], it did not", subject.Token, expected.Token)
-	}
-	if expected.ExpiresIn != subject.ExpiresIn {
-		t.Fatalf("expected [%d] to equal [%d], it did not", subject.ExpiresIn, expected.ExpiresIn)
-	}
-	if !expected.IssuedAt.Equal(subject.IssuedAt) {
-		t.Fatalf("expected [%s] to equal [%s], it did not", subject.IssuedAt, expected.IssuedAt)
-	}
+	tokenBlob := fmt.Sprintf(`{"token":"IAmAToken","expires_in":100,"issued_at":"%s"}`, zeroTime)
+	token, err := newBearerTokenFromHTTPResponseBody(testTokenHTTPResponse(t, string(tokenBlob)))
+	require.NoError(t, err)
+	assert.False(t, token.IssuedAt.Before(now), "expected [%s] not to be before [%s]", token.IssuedAt, now)
 }
 
 func TestUserAgent(t *testing.T) {