From 26d117dcdbc3e2231bce8aaab8b9d2ecedce05de Mon Sep 17 00:00:00 2001 From: Ogulcan Aydogan Date: Sun, 8 Mar 2026 23:48:12 +0000 Subject: [PATCH 1/2] feat: add retry with exponential backoff for network interactions Wrap HTTP transports with retry logic from oras-go/v2/registry/remote/retry in both NewAuthClient and NewClient. The retry transport uses exponential backoff with jitter (200ms-3s, up to 5 retries) and retries on 5xx errors, 429 Too Many Requests, 408 Request Timeout, and network dial timeouts. - Add withRetry helper to wrap client transports with retry.Transport - Add unit tests for retry behavior, transport wrapping, and user agent Fixes notaryproject/notation#518 Signed-off-by: Ogulcan Aydogan --- internal/httputil/client.go | 22 +++- internal/httputil/client_test.go | 174 +++++++++++++++++++++++++++++++ 2 files changed, 194 insertions(+), 2 deletions(-) create mode 100644 internal/httputil/client_test.go diff --git a/internal/httputil/client.go b/internal/httputil/client.go index 4ac5e39ea..d80127bc6 100644 --- a/internal/httputil/client.go +++ b/internal/httputil/client.go @@ -20,12 +20,15 @@ import ( "github.com/notaryproject/notation/v2/internal/trace" "github.com/notaryproject/notation/v2/internal/version" "oras.land/oras-go/v2/registry/remote/auth" + "oras.land/oras-go/v2/registry/remote/retry" ) var userAgent = "notation/" + version.GetVersion() -// NewAuthClient returns an *auth.Client with debug log and user agent set +// NewAuthClient returns an *auth.Client with retry, debug log and user agent +// set. func NewAuthClient(ctx context.Context, httpClient *http.Client) *auth.Client { + httpClient = withRetry(httpClient) httpClient = trace.SetHTTPDebugLog(ctx, httpClient) client := &auth.Client{ Client: httpClient, @@ -36,12 +39,27 @@ func NewAuthClient(ctx context.Context, httpClient *http.Client) *auth.Client { return client } -// NewClient returns an *http.Client with debug log and user agent set +// NewClient returns an *http.Client with retry, debug log and user agent set. func NewClient(ctx context.Context, client *http.Client) *http.Client { + client = withRetry(client) client = trace.SetHTTPDebugLog(ctx, client) return SetUserAgent(client) } +// withRetry wraps the client's transport with retry logic using exponential +// backoff. It retries on 5xx errors, 429, 408, and network timeouts. +func withRetry(client *http.Client) *http.Client { + if client == nil { + client = &http.Client{} + } + base := client.Transport + if base == nil { + base = http.DefaultTransport + } + client.Transport = retry.NewTransport(base) + return client +} + type userAgentTransport struct { base http.RoundTripper } diff --git a/internal/httputil/client_test.go b/internal/httputil/client_test.go new file mode 100644 index 000000000..fe7e85811 --- /dev/null +++ b/internal/httputil/client_test.go @@ -0,0 +1,174 @@ +// Copyright The Notary Project Authors. +// 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 httputil + +import ( + "context" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + + "oras.land/oras-go/v2/registry/remote/retry" +) + +func TestNewAuthClient(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer ts.Close() + + t.Run("nil client", func(t *testing.T) { + client := NewAuthClient(context.Background(), nil) + if client == nil { + t.Fatal("expected non-nil auth client") + } + if client.Cache == nil { + t.Fatal("expected non-nil cache") + } + }) + + t.Run("custom client", func(t *testing.T) { + httpClient := &http.Client{} + client := NewAuthClient(context.Background(), httpClient) + if client == nil { + t.Fatal("expected non-nil auth client") + } + }) +} + +func TestNewClient(t *testing.T) { + t.Run("nil client", func(t *testing.T) { + client := NewClient(context.Background(), nil) + if client == nil { + t.Fatal("expected non-nil client") + } + }) + + t.Run("custom client", func(t *testing.T) { + httpClient := &http.Client{} + client := NewClient(context.Background(), httpClient) + if client == nil { + t.Fatal("expected non-nil client") + } + }) +} + +func TestWithRetry(t *testing.T) { + t.Run("nil client gets default transport", func(t *testing.T) { + client := withRetry(nil) + if client == nil { + t.Fatal("expected non-nil client") + } + rt, ok := client.Transport.(*retry.Transport) + if !ok { + t.Fatalf("expected *retry.Transport, got %T", client.Transport) + } + if rt.Base != http.DefaultTransport { + t.Fatal("expected base to be http.DefaultTransport") + } + }) + + t.Run("client with nil transport gets default transport", func(t *testing.T) { + client := withRetry(&http.Client{}) + rt, ok := client.Transport.(*retry.Transport) + if !ok { + t.Fatalf("expected *retry.Transport, got %T", client.Transport) + } + if rt.Base != http.DefaultTransport { + t.Fatal("expected base to be http.DefaultTransport") + } + }) + + t.Run("custom transport is preserved", func(t *testing.T) { + custom := &http.Transport{} + client := withRetry(&http.Client{Transport: custom}) + rt, ok := client.Transport.(*retry.Transport) + if !ok { + t.Fatalf("expected *retry.Transport, got %T", client.Transport) + } + if rt.Base != custom { + t.Fatal("expected base to be the custom transport") + } + }) +} + +func TestRetryOnServerError(t *testing.T) { + var attempts atomic.Int32 + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if attempts.Add(1) == 1 { + w.WriteHeader(http.StatusServiceUnavailable) + return + } + w.WriteHeader(http.StatusOK) + })) + defer ts.Close() + + client := NewClient(context.Background(), nil) + resp, err := client.Get(ts.URL) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Fatalf("expected status 200, got %d", resp.StatusCode) + } + if got := attempts.Load(); got != 2 { + t.Fatalf("expected 2 attempts, got %d", got) + } +} + +func TestNoRetryOnClientError(t *testing.T) { + var attempts atomic.Int32 + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + attempts.Add(1) + w.WriteHeader(http.StatusBadRequest) + })) + defer ts.Close() + + client := NewClient(context.Background(), nil) + resp, err := client.Get(ts.URL) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusBadRequest { + t.Fatalf("expected status 400, got %d", resp.StatusCode) + } + if got := attempts.Load(); got != 1 { + t.Fatalf("expected 1 attempt (no retry), got %d", got) + } +} + +func TestSetUserAgent(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + ua := r.Header.Get("User-Agent") + if ua != userAgent { + w.WriteHeader(http.StatusBadRequest) + return + } + w.WriteHeader(http.StatusOK) + })) + defer ts.Close() + + client := NewClient(context.Background(), nil) + resp, err := client.Get(ts.URL) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Fatalf("expected status 200, got %d", resp.StatusCode) + } +} From 1bb9edb98b25a585e79361ab73da7408eb68bf1a Mon Sep 17 00:00:00 2001 From: Ogulcan Aydogan Date: Thu, 19 Mar 2026 07:10:27 +0000 Subject: [PATCH 2/2] fix: remove unused test server in TestNewAuthClient Remove dead httptest.NewServer that was created but never used by any subtests in TestNewAuthClient. Signed-off-by: Ogulcan Aydogan --- internal/httputil/client_test.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/internal/httputil/client_test.go b/internal/httputil/client_test.go index fe7e85811..b797a820f 100644 --- a/internal/httputil/client_test.go +++ b/internal/httputil/client_test.go @@ -24,11 +24,6 @@ import ( ) func TestNewAuthClient(t *testing.T) { - ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - })) - defer ts.Close() - t.Run("nil client", func(t *testing.T) { client := NewAuthClient(context.Background(), nil) if client == nil {