From f07f8a682aee749b6226b30b686f914822f8ee78 Mon Sep 17 00:00:00 2001 From: Ryan Phillips Date: Mon, 20 Apr 2026 12:27:59 -0500 Subject: [PATCH 1/2] tls: add read deadline to containers/image registry connections A stalled TLS connection to a container registry (e.g. quay.io) can cause image pulls to hang indefinitely. The HTTP response body read blocks forever in tls.Conn.Read with no timeout, starving the entire pull pipeline and leaving pods stuck in ContainerCreating for hours. Wrap the HTTP transport dialer with a deadlineConn that enforces a 5-minute read deadline via SetReadDeadline on every Read call. When triggered, bodyReader treats the timeout the same as ECONNRESET and attempts a Range-based reconnect to resume the download. Also add a 2-minute ResponseHeaderTimeout to the transport. Ref: https://redhat.atlassian.net/browse/OCPBUGS-79544 Signed-off-by: Ryan Phillips add --- image/docker/body_reader.go | 13 ++++++++++++- image/docker/docker_client.go | 31 +++++++++++++++++++++++++++++++ image/types/types.go | 6 ++++++ 3 files changed, 49 insertions(+), 1 deletion(-) 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/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/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"), From 9333c626bd71179ff1d2b4447db4274ddf2f910a Mon Sep 17 00:00:00 2001 From: Ryan Phillips Date: Mon, 20 Apr 2026 12:35:25 -0500 Subject: [PATCH 2/2] tests: add test cases for the read deadlines Signed-off-by: Ryan Phillips --- image/docker/body_reader_test.go | 58 ++++++++++++++++++++++++++++++ image/docker/docker_client_test.go | 51 ++++++++++++++++++++++++++ 2 files changed, 109 insertions(+) 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_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"