diff --git a/pkg/registry/client.go b/pkg/registry/client.go index 4d7da78dc..045904c13 100644 --- a/pkg/registry/client.go +++ b/pkg/registry/client.go @@ -30,6 +30,7 @@ import ( "os" "sort" "strings" + "time" "github.com/Masterminds/semver/v3" "github.com/opencontainers/image-spec/specs-go" @@ -95,10 +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{ + Timeout: defaultHTTPClientTimeout, 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{ diff --git a/pkg/registry/client_test.go b/pkg/registry/client_test.go index 97b7c7c4c..88ea7eef0 100644 --- a/pkg/registry/client_test.go +++ b/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) +}