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) + } +}