fix(registry): apply default timeout to custom clients with zero Timeout

When NewClient is given a custom *http.Client with Timeout left at zero,
apply the same 30s default used for the built-in client (clone so the
caller-owned client is not mutated). Preserve explicit non-zero timeouts.

Add unit tests for default, zero-timeout custom, and explicit override.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>

pkg/registry/client.go

@@ -96,12 +96,21 @@ func NewClient(options ...ClientOption) (*Client, error) {
 	if client.credentialsFile == "" {
 		client.credentialsFile = helmpath.ConfigPath(CredentialsFileBasename)
 	}
+	// Bound registry operations so an unresponsive registry cannot hang forever.
+	// Apply when no client was provided, or when a custom client left Timeout at the
+	// zero value (no timeout). Callers that need an unlimited client can set a large
+	// positive Timeout explicitly.
+	const defaultHTTPClientTimeout = 30 * time.Second
 	if client.httpClient == nil {
 		client.httpClient = &http.Client{
-			// Bound registry operations so an unresponsive registry cannot hang forever.
+			Timeout:   defaultHTTPClientTimeout,
-			Timeout:   30 * time.Second,
 			Transport: NewTransport(client.debug),
 		}
+	} else if client.httpClient.Timeout == 0 {
+		// Clone so we do not mutate a caller-owned *http.Client.
+		httpClient := *client.httpClient
+		httpClient.Timeout = defaultHTTPClientTimeout
+		client.httpClient = &httpClient
 	}
 
 	storeOptions := credentials.StoreOptions{

pkg/registry/client_test.go

@@ -23,6 +23,7 @@ import (
 	"path/filepath"
 	"strings"
 	"testing"
+	"time"
 
 	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
 	"github.com/stretchr/testify/assert"
@@ -153,3 +154,46 @@ func TestWarnIfHostHasPath(t *testing.T) {
 		})
 	}
 }
+
+func TestNewClient_DefaultHTTPClientTimeout(t *testing.T) {
+	t.Parallel()
+
+	c, err := NewClient(ClientOptWriter(io.Discard))
+	require.NoError(t, err)
+	require.NotNil(t, c.httpClient)
+	assert.Equal(t, 30*time.Second, c.httpClient.Timeout)
+}
+
+func TestNewClient_AppliesDefaultTimeoutToCustomClientWithZeroTimeout(t *testing.T) {
+	t.Parallel()
+
+	custom := &http.Client{
+		Transport: http.DefaultTransport,
+		// Timeout left at zero (no timeout)
+	}
+	c, err := NewClient(
+		ClientOptWriter(io.Discard),
+		ClientOptHTTPClient(custom),
+	)
+	require.NoError(t, err)
+	require.NotNil(t, c.httpClient)
+	assert.Equal(t, 30*time.Second, c.httpClient.Timeout)
+	// Caller-owned client must not be mutated.
+	assert.Equal(t, time.Duration(0), custom.Timeout)
+}
+
+func TestNewClient_PreservesExplicitHTTPClientTimeout(t *testing.T) {
+	t.Parallel()
+
+	custom := &http.Client{
+		Timeout:   5 * time.Second,
+		Transport: http.DefaultTransport,
+	}
+	c, err := NewClient(
+		ClientOptWriter(io.Discard),
+		ClientOptHTTPClient(custom),
+	)
+	require.NoError(t, err)
+	require.NotNil(t, c.httpClient)
+	assert.Equal(t, 5*time.Second, c.httpClient.Timeout)
+}