diff --git a/image/docker/body_reader.go b/image/docker/body_reader.go index 3c612f2688..6583c9d1da 100644 --- a/image/docker/body_reader.go +++ b/image/docker/body_reader.go @@ -7,6 +7,7 @@ import ( "io" "math" "math/rand/v2" + "net" "net/http" "net/url" "strconv" @@ -17,6 +18,16 @@ import ( "github.com/sirupsen/logrus" ) +// isRetryableNetworkError returns true for network errors that indicate +// a stalled or interrupted connection that may be resolved by reconnecting. +func isRetryableNetworkError(err error) bool { + var netErr net.Error + if errors.As(err, &netErr) && netErr.Timeout() { + return true + } + return false +} + const ( // bodyReaderMinimumProgress is the minimum progress we consider a good reason to retry bodyReaderMinimumProgress = 1 * 1024 * 1024 @@ -147,7 +158,7 @@ func (br *bodyReader) Read(p []byte) (int, error) { br.lastSuccessTime = time.Now() return n, err // Unlike the default: case, don’t log anything. - case errors.Is(err, io.ErrUnexpectedEOF) || errors.Is(err, syscall.ECONNRESET): + case errors.Is(err, io.ErrUnexpectedEOF) || errors.Is(err, syscall.ECONNRESET) || isRetryableNetworkError(err): originalErr := err redactedURL := br.logURL.Redacted() if err := br.errorIfNotReconnecting(originalErr, redactedURL); err != nil { diff --git a/image/docker/body_reader_test.go b/image/docker/body_reader_test.go index 0011582b7a..3808d61178 100644 --- a/image/docker/body_reader_test.go +++ b/image/docker/body_reader_test.go @@ -2,7 +2,9 @@ package docker import ( "errors" + "fmt" "math" + "net" "net/http" "testing" "time" @@ -12,6 +14,62 @@ import ( "github.com/stretchr/testify/require" ) +// timeoutValueError implements net.Error with a configurable Timeout() return value. +type timeoutValueError struct { + isTimeout bool +} + +func (e *timeoutValueError) Error() string { return "network error" } +func (e *timeoutValueError) Timeout() bool { return e.isTimeout } +func (e *timeoutValueError) Temporary() bool { return false } + +func TestIsRetryableNetworkError(t *testing.T) { + for _, c := range []struct { + name string + err error + expected bool + }{ + { + // A direct timeout error from the network layer should be retryable. + name: "net.Error with Timeout() true", + err: &timeoutValueError{isTimeout: true}, + expected: true, + }, + { + // Timeout errors wrapped by fmt.Errorf should still be detected + // via errors.As unwrapping. + name: "wrapped net.Error with Timeout() true", + err: fmt.Errorf("read failed: %w", &timeoutValueError{isTimeout: true}), + expected: true, + }, + { + // A net.Error that is not a timeout (e.g. a connection refused) + // should not be retryable. + name: "net.Error with Timeout() false", + err: &timeoutValueError{isTimeout: false}, + expected: false, + }, + { + // A plain error with no net.Error in the chain should not be retryable. + name: "plain error", + err: errors.New("something broke"), + expected: false, + }, + { + // net.OpError is the concrete type returned by real socket operations; + // verify that errors.As can unwrap through it to find the timeout. + name: "net.OpError wrapping timeout", + err: &net.OpError{Op: "read", Err: &timeoutValueError{isTimeout: true}}, + expected: true, + }, + } { + t.Run(c.name, func(t *testing.T) { + result := isRetryableNetworkError(c.err) + assert.Equal(t, c.expected, result) + }) + } +} + func TestParseDecimalInString(t *testing.T) { for _, prefix := range []string{"", "text", "0"} { for _, suffix := range []string{"", "text"} { diff --git a/image/docker/docker_client.go b/image/docker/docker_client.go index 4b5f41a4f2..9c152ae92b 100644 --- a/image/docker/docker_client.go +++ b/image/docker/docker_client.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "io" + "net" "net/http" "net/url" "os" @@ -963,6 +964,22 @@ func (bt *bearerToken) readFromHTTPResponseBody(res *http.Response) error { return nil } +// deadlineConn wraps a net.Conn to enforce a read deadline on every Read call. +// If no data arrives within the configured readTimeout, the Read returns a +// net.Error with Timeout() == true, which bodyReader treats as a reconnectable +// condition and retries with a Range request. +type deadlineConn struct { + net.Conn + readTimeout time.Duration +} + +func (c *deadlineConn) Read(p []byte) (int, error) { + if err := c.Conn.SetReadDeadline(time.Now().Add(c.readTimeout)); err != nil { + return 0, err + } + return c.Conn.Read(p) +} + // detectPropertiesHelper performs the work of detectProperties which executes // it at most once. func (c *dockerClient) detectPropertiesHelper(ctx context.Context) error { @@ -973,6 +990,19 @@ func (c *dockerClient) detectPropertiesHelper(ctx context.Context) error { } tr := tlsclientconfig.NewTransport() tr.TLSClientConfig = c.tlsClientConfig + // Wrap the dialer to set per-read deadlines on the underlying connection. + // This ensures that a stalled registry response (e.g. TLS read that never + // returns data) will time out instead of blocking indefinitely. + if c.sys != nil && c.sys.DockerReadTimeout > 0 { + originalDial := tr.DialContext + tr.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) { + conn, err := originalDial(ctx, network, addr) + if err != nil { + return nil, err + } + return &deadlineConn{Conn: conn, readTimeout: c.sys.DockerReadTimeout}, nil + } + } // if set DockerProxyURL explicitly, use the DockerProxyURL instead of system proxy if c.sys != nil && c.sys.DockerProxyURL != nil { tr.Proxy = http.ProxyURL(c.sys.DockerProxyURL) @@ -982,6 +1012,7 @@ func (c *dockerClient) detectPropertiesHelper(ctx context.Context) error { return c.sys.DockerProxy(request.URL) } } + tr.ResponseHeaderTimeout = 2 * time.Minute c.client = &http.Client{Transport: tr} ping := func(scheme string) error { diff --git a/image/docker/docker_client_test.go b/image/docker/docker_client_test.go index 229fea332f..b62781d1fb 100644 --- a/image/docker/docker_client_test.go +++ b/image/docker/docker_client_test.go @@ -6,6 +6,7 @@ import ( "context" "fmt" "io" + "net" "net/http" "net/http/httptest" "net/url" @@ -20,6 +21,56 @@ import ( "go.podman.io/image/v5/types" ) +// TestDeadlineConnReadTimeout verifies that a stalled connection (no data +// arriving) returns a timeout error after the configured readTimeout. +// This is the key behavior that lets bodyReader treat it as a reconnectable +// condition and resume the download with a Range request. +func TestDeadlineConnReadTimeout(t *testing.T) { + server, client := net.Pipe() + defer server.Close() + defer client.Close() + + dc := &deadlineConn{ + Conn: client, + readTimeout: 50 * time.Millisecond, + } + + // No data is written to the server side, so the read should time out. + buf := make([]byte, 64) + _, err := dc.Read(buf) + require.Error(t, err) + + // Verify the error satisfies net.Error and reports as a timeout, which + // is what isRetryableNetworkError checks. + var netErr net.Error + require.ErrorAs(t, err, &netErr) + assert.True(t, netErr.Timeout(), "expected a timeout error") +} + +// TestDeadlineConnReadSuccess verifies that the deadline wrapper does not +// interfere with normal reads — when data arrives promptly, it is returned +// without error. +func TestDeadlineConnReadSuccess(t *testing.T) { + server, client := net.Pipe() + defer server.Close() + defer client.Close() + + dc := &deadlineConn{ + Conn: client, + readTimeout: 5 * time.Second, + } + + expected := []byte("hello") + go func() { + _, _ = server.Write(expected) + }() + + buf := make([]byte, 64) + n, err := dc.Read(buf) + require.NoError(t, err) + assert.Equal(t, expected, buf[:n]) +} + func TestDockerCertDir(t *testing.T) { const nondefaultFullPath = "/this/is/not/the/default/full/path" const nondefaultPerHostDir = "/this/is/not/the/default/certs.d" diff --git a/image/types/types.go b/image/types/types.go index 1c0007e6e4..5580b8f08c 100644 --- a/image/types/types.go +++ b/image/types/types.go @@ -679,6 +679,12 @@ type SystemContext struct { // If set, this takes precedence over DockerProxyURL. The function should return the proxy URL to use, // or nil if no proxy should be used for the given request. DockerProxy func(reqURL *url.URL) (*url.URL, error) + // DockerReadTimeout is the maximum duration a single read from a registry + // connection can block without returning any data before the connection is + // considered stalled. When triggered, the read returns a timeout error + // which allows the download to be resumed with a Range request. + // If zero, no per-read deadline is enforced. + DockerReadTimeout time.Duration // === docker/daemon.Transport overrides === // A directory containing a CA certificate (ending with ".crt"),