From 53a83ff313cb251d45e784cde983a82a47fdd73d Mon Sep 17 00:00:00 2001 From: Ryan Brainard <966764+ryanbrainard@users.noreply.github.com> Date: Fri, 15 Aug 2025 12:58:53 -0400 Subject: [PATCH] fix: preserve response body in retryError for structured error messages The retryError function was consuming the HTTP response body without restoring it for subsequent readers. This caused structured registry error messages to be lost during retries, falling back to generic HTTP status messages instead. This change buffers the response body and creates a new readable stream so that both retryError and subsequent error handling can access the response content. Before: - Without retry: MANIFEST_UNKNOWN: manifest unknown; unknown tag=v1.0.0 - With retry: unexpected status code 404 Not Found After: - Both cases: MANIFEST_UNKNOWN: manifest unknown; unknown tag=v1.0.0 Fixes #2125 --- pkg/v1/remote/error_roundtrip_test.go | 49 ++++++++++++++++ pkg/v1/remote/transport/error.go | 3 + pkg/v1/remote/transport/error_test.go | 82 +++++++++++++++++++++++++++ 3 files changed, 134 insertions(+) diff --git a/pkg/v1/remote/error_roundtrip_test.go b/pkg/v1/remote/error_roundtrip_test.go index 981f8fc97..6425bb386 100644 --- a/pkg/v1/remote/error_roundtrip_test.go +++ b/pkg/v1/remote/error_roundtrip_test.go @@ -125,3 +125,52 @@ func TestBlobStatusCodeReturned(t *testing.T) { t.Errorf("Incorrect status code received, got %v, wanted %v", terr.StatusCode, http.StatusTeapot) } } + +func TestRetryPreservesStructuredErrors(t *testing.T) { + // Test that structured registry errors are preserved when retries are enabled + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/v2/" { + return + } + // Return a structured registry error + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusNotFound) + w.Write([]byte(`{"errors":[{"code":"MANIFEST_UNKNOWN","message":"manifest unknown","detail":"unknown tag=v1.0.0"}]}`)) + }) + + server := httptest.NewServer(handler) + defer server.Close() + + ref, err := name.NewTag(strings.TrimPrefix(server.URL+"/test:v1.0.0", "http://")) + if err != nil { + t.Fatalf("Unable to parse tag: %v", err) + } + + // Test without retry - should get structured error + _, err = remote.Image(ref) + if err == nil { + t.Fatal("Expected error, got nil") + } + withoutRetryMsg := err.Error() + + // Test with retry - should still get structured error (not generic 404) + _, err = remote.Image(ref, remote.WithRetryStatusCodes(http.StatusNotFound)) + if err == nil { + t.Fatal("Expected error, got nil") + } + withRetryMsg := err.Error() + + // Both should contain the structured error message + expectedSubstring := "MANIFEST_UNKNOWN: manifest unknown; unknown tag=v1.0.0" + if !strings.Contains(withoutRetryMsg, expectedSubstring) { + t.Errorf("Without retry error %q should contain %q", withoutRetryMsg, expectedSubstring) + } + if !strings.Contains(withRetryMsg, expectedSubstring) { + t.Errorf("With retry error %q should contain %q", withRetryMsg, expectedSubstring) + } + + // The retry case should NOT contain generic "unexpected status code 404" + if strings.Contains(withRetryMsg, "unexpected status code 404 Not Found") { + t.Errorf("With retry error should not contain generic 404 message, got: %q", withRetryMsg) + } +} diff --git a/pkg/v1/remote/transport/error.go b/pkg/v1/remote/transport/error.go index 482a4adee..91f72de59 100644 --- a/pkg/v1/remote/transport/error.go +++ b/pkg/v1/remote/transport/error.go @@ -15,6 +15,7 @@ package transport import ( + "bytes" "encoding/json" "fmt" "io" @@ -189,6 +190,8 @@ func retryError(resp *http.Response) error { if err != nil { return err } + _ = resp.Body.Close() + resp.Body = io.NopCloser(bytes.NewReader(b)) rerr := makeError(resp, b) rerr.temporary = true diff --git a/pkg/v1/remote/transport/error_test.go b/pkg/v1/remote/transport/error_test.go index 679b6e1a8..310deb741 100644 --- a/pkg/v1/remote/transport/error_test.go +++ b/pkg/v1/remote/transport/error_test.go @@ -234,3 +234,85 @@ func (e *errReadCloser) Read(_ []byte) (int, error) { func (e *errReadCloser) Close() error { return e.err } + +func TestRetryError(t *testing.T) { + tests := []struct { + name string + status int + body string + wantError string + }{ + { + name: "returned error", + status: http.StatusInternalServerError, + body: "boom", + wantError: "unexpected status code 500 Internal Server Error: boom", + }, + { + name: "empty body", + status: http.StatusInternalServerError, + body: "", + wantError: "unexpected status code 500 Internal Server Error", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + resp := &http.Response{ + StatusCode: test.status, + Body: io.NopCloser(bytes.NewBufferString(test.body)), + } + + // Call retryError + err := retryError(resp) + if err == nil { + t.Fatal("retryError() = nil, wanted error") + } + + // Verify the error is marked as temporary + var terr *Error + if !errors.As(err, &terr) { + t.Fatalf("retryError() = %T, wanted *Error", err) + } + if !terr.Temporary() { + t.Error("retryError() should return temporary error") + } + + // Verify error message + if terr.Error() != test.wantError { + t.Errorf("retryError().Error() = %q, want %q", terr.Error(), test.wantError) + } + + // Verify the response body can still be read + body, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("Failed to read response body after retryError: %v", err) + } + if string(body) != test.body { + t.Errorf("Response body after retryError = %q, want %q", string(body), test.body) + } + + // Verify we can read it again (body should be rewindable) + body2, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("Failed to read response body second time: %v", err) + } + if string(body2) != "" { + t.Errorf("Second read should be empty, got %q", string(body2)) + } + }) + } +} + +func TestRetryErrorReadError(t *testing.T) { + expectedErr := errors.New("read error") + resp := &http.Response{ + StatusCode: http.StatusInternalServerError, + Body: &errReadCloser{expectedErr}, + } + + err := retryError(resp) + if !errors.Is(err, expectedErr) { + t.Errorf("retryError() = %v, want %v", err, expectedErr) + } +}