transport: retry HTTP 429 (Too Many Requests) (#2301)

Fixes #2111. As filed by knqyf263, 429 responses are never retried by
the default retry transport even though TooManyRequestsErrorCode is
already classified temporary in transport/error.go.

The retry transport in pkg/v1/remote/transport/retry.go short-circuits
on its codes list before consulting Temporary(); since 429 isn't in
defaultRetryStatusCodes, the conversion to a temporary transport.Error
never happens and retry.IsTemporary never gets called.

The fix is symmetric across the two parallel classifications:

  * defaultRetryStatusCodes in pkg/v1/remote/options.go now includes
    http.StatusTooManyRequests, so the retry transport wraps a 429
    response into a temporary *transport.Error and retries it through
    the existing backoff (Duration 1s, Factor 3, Steps 3).

  * temporaryStatusCodes in pkg/v1/remote/transport/error.go also
    gains http.StatusTooManyRequests, bringing the raw-status fallback
    path into alignment with temporaryErrorCodes which has already
    been classifying TooManyRequestsErrorCode as temporary.

The exhausted *transport.Error still carries StatusCode == 429, so
pkg/gcrane/copy.go's hasStatusCode helper continues to route 429s
through GCRBackoff at the application layer. The two retry layers
compose: ~1.3s of transport-level retry per request, then ~6s+ of
copy-level retry per image. No existing test expectations change.

Out of scope here: honoring the Retry-After response header. The retry
backoff is currently static; honoring server-dictated delays would
need plumbing through the retry/wait package. Worth a follow-up.

Tests:

  * pkg/v1/remote/transport/retry_test.go gains
    TestRetryTransport_TooManyRequests, which exercises three 429s
    through the retry transport and asserts both that all three
    attempts are made and that the final response surfaces with
    StatusCode == 429 (so gcrane's hasStatusCode keeps matching).
    It also pins the new entry in temporaryStatusCodes.

  * pkg/v1/remote/options_test.go (new file) pins 429 in the default
    retry status code list. A future refactor that drops 429 will
    fail loudly before the regression ships.

Verified locally:
  go test ./... (full suite green)
  gofmt + goimports clean
  golangci-lint v2.11: 0 issues
  woke clean on touched files
  reviewdog/action-misspell US: clean on the diff

Author: iahsanGill

pkg/v1/remote/options.go

@@ -93,6 +93,7 @@ var fastBackoff = Backoff{
 
 var defaultRetryStatusCodes = []int{
 	http.StatusRequestTimeout,
+	http.StatusTooManyRequests, // 429: OCI distribution-spec rate limit; TooManyRequestsErrorCode is already classified temporary in transport/error.go
 	http.StatusInternalServerError,
 	http.StatusBadGateway,
 	http.StatusServiceUnavailable,

pkg/v1/remote/options_test.go

@@ -0,0 +1,35 @@
+// Copyright 2018 Google LLC All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package remote
+
+import (
+	"net/http"
+	"testing"
+)
+
+// TestDefaultRetryStatusCodes_Includes429 pins HTTP 429 (Too Many
+// Requests) in the default retry status code list. The retry transport
+// short-circuits on this list before consulting Temporary(), so 429 has
+// to live here even though TooManyRequestsErrorCode is already classified
+// temporary in transport/error.go. If a future refactor drops 429 this
+// test fails loudly before the regression ships.
+func TestDefaultRetryStatusCodes_Includes429(t *testing.T) {
+	for _, c := range defaultRetryStatusCodes {
+		if c == http.StatusTooManyRequests {
+			return
+		}
+	}
+	t.Fatal("defaultRetryStatusCodes should include http.StatusTooManyRequests so registries returning 429 are retried by the default transport")
+}

pkg/v1/remote/transport/error.go

@@ -151,6 +151,7 @@ var temporaryErrorCodes = map[ErrorCode]struct{}{
 
 var temporaryStatusCodes = map[int]struct{}{
 	http.StatusRequestTimeout:      {},
+	http.StatusTooManyRequests:     {}, // matches TooManyRequestsErrorCode in temporaryErrorCodes
 	http.StatusInternalServerError: {},
 	http.StatusBadGateway:          {},
 	http.StatusServiceUnavailable:  {},

pkg/v1/remote/transport/retry_test.go

@@ -142,6 +142,57 @@ func TestRetryDefaults(t *testing.T) {
 	}
 }
 
+// TestRetryTransport_TooManyRequests covers two invariants that consumers
+// (including pkg/gcrane/copy.go's outer retry loop) depend on once 429 is
+// in the default retry list:
+//
+//  1. A 429 response is wrapped into a temporary *Error and retried up to
+//     the configured Steps.
+//  2. After retries exhaust, the *Error surfaced to the caller still has
+//     StatusCode == 429 so callers like gcrane's hasStatusCode helper can
+//     route their own application-level backoff (GCRBackoff) on top.
+func TestRetryTransport_TooManyRequests(t *testing.T) {
+	mt := mockTransport{
+		resps: []*http.Response{
+			resp(http.StatusTooManyRequests),
+			resp(http.StatusTooManyRequests),
+			resp(http.StatusTooManyRequests),
+		},
+	}
+
+	tr := NewRetry(&mt,
+		WithRetryBackoff(retry.Backoff{Steps: 3}),
+		WithRetryPredicate(retry.IsTemporary),
+		WithRetryStatusCodes(http.StatusTooManyRequests),
+	)
+
+	req, err := http.NewRequestWithContext(context.Background(), "GET", "example.com", nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	out, _ := tr.RoundTrip(req)
+
+	if mt.count != 3 {
+		t.Errorf("expected 3 attempts (1 + 2 retries), got %d", mt.count)
+	}
+	if out == nil || out.StatusCode != http.StatusTooManyRequests {
+		t.Errorf("final response should still surface 429 status; got %v", out)
+	}
+}
+
+// TestTemporaryStatusCodes_Includes429 keeps temporaryStatusCodes (the
+// raw-status fallback path) in sync with temporaryErrorCodes (the
+// parsed-body path), which already contains TooManyRequestsErrorCode.
+// A registry returning 429 with no structured body should still be
+// classified temporary so downstream consumers retrying on
+// transport.Error.Temporary() behave consistently with consumers that
+// retry on a TOOMANYREQUESTS error code in the body.
+func TestTemporaryStatusCodes_Includes429(t *testing.T) {
+	if _, ok := temporaryStatusCodes[http.StatusTooManyRequests]; !ok {
+		t.Fatal("temporaryStatusCodes should contain http.StatusTooManyRequests for parity with temporaryErrorCodes[TooManyRequestsErrorCode]")
+	}
+}
+
 func TestTimeoutContext(t *testing.T) {
 	tr := NewRetry(http.DefaultTransport)