Tighten registry HTTP transport connection timeouts

cmd/aaop/aaop.go

@@ -45,7 +45,12 @@ var (
 	bundleMaxAttempts = flag.Int("bundle-max-attempts", 3, "max attempts to fetch a bundle")
 	bundleTimeout     = flag.Duration("bundle-timeout", 3*time.Second, "timeout for a single attempt to fetch a bundle")
 	bundleDelay       = flag.Duration("bundle-delay", 0, "delay between attempts to fetch a bundle")
-	updateCABundle    = flag.Bool("update-ca-bundle", false, "regularly update the Provider's caBundle field")
+
+	registryDialTimeout           = flag.Duration("registry-dial-timeout", 0, "override TCP dial timeout to the registry; 0 derives it from bundle-timeout")
+	registryTLSHandshakeTimeout   = flag.Duration("registry-tls-handshake-timeout", 0, "override TLS handshake timeout to the registry; 0 derives it from bundle-timeout")
+	registryResponseHeaderTimeout = flag.Duration("registry-response-header-timeout", 0, "override wait for the registry's response headers; 0 derives it from bundle-timeout")
+
+	updateCABundle = flag.Bool("update-ca-bundle", false, "regularly update the Provider's caBundle field")
 )
 
 const (
@@ -69,7 +74,8 @@ func main() {
 	var err error
 
 	flag.Parse()
-	if err := configureBundleFetcher(*bundleMaxAttempts, *bundleTimeout, *bundleDelay); err != nil {
+	if err := configureBundleFetcher(*bundleMaxAttempts, *bundleTimeout, *bundleDelay,
+		*registryDialTimeout, *registryTLSHandshakeTimeout, *registryResponseHeaderTimeout); err != nil {
 		log.Fatal(err)
 	}
 
@@ -187,7 +193,8 @@ func main() {
 	slog.Info("server shut down gracefully")
 }
 
-func configureBundleFetcher(maxAttempts int, timeout, delay time.Duration) error {
+func configureBundleFetcher(maxAttempts int, timeout, delay time.Duration,
+	dialTimeout, tlsHandshakeTimeout, responseHeaderTimeout time.Duration) error {
 	if maxAttempts < 1 {
 		return errors.New("bundle-max-attempts must be greater than zero")
 	}
@@ -197,10 +204,35 @@ func configureBundleFetcher(maxAttempts int, timeout, delay time.Duration) error
 	if delay < 0 {
 		return errors.New("bundle-delay must not be negative")
 	}
+	// The registry connection-phase timeouts are overrides: 0 means "derive
+	// from bundle-timeout". Only negative values are rejected. An override may
+	// exceed bundle-timeout — consistent with the derived 250ms floor, we let an
+	// operator keep a usable connection-setup timeout even when it is larger
+	// than the per-attempt budget (the attempt context simply fires first).
+	// Setting a per-attempt budget that small is treated as operator error, not
+	// something to guard against here.
+	for _, o := range []struct {
+		name  string
+		value time.Duration
+	}{
+		{"registry-dial-timeout", dialTimeout},
+		{"registry-tls-handshake-timeout", tlsHandshakeTimeout},
+		{"registry-response-header-timeout", responseHeaderTimeout},
+	} {
+		if o.value < 0 {
+			return fmt.Errorf("%s must not be negative", o.name)
+		}
+	}
 
 	fetcher.MaxAttempts = maxAttempts
 	fetcher.Timeout = timeout
 	fetcher.Delay = delay
+	fetcher.DialTimeoutOverride = dialTimeout
+	fetcher.TLSHandshakeTimeoutOverride = tlsHandshakeTimeout
+	fetcher.ResponseHeaderTimeoutOverride = responseHeaderTimeout
+	// Rebuild the shared registry transport now that Timeout and the overrides
+	// are set, so its connection-phase timeouts track the per-attempt budget.
+	fetcher.ConfigureTransport()
 	return nil
 }
 

cmd/aaop/aaop_test.go

@@ -13,27 +13,86 @@ func TestConfigureBundleFetcher(t *testing.T) {
 	originalMaxAttempts := fetcher.MaxAttempts
 	originalTimeout := fetcher.Timeout
 	originalDelay := fetcher.Delay
+	originalDial := fetcher.DialTimeoutOverride
+	originalTLS := fetcher.TLSHandshakeTimeoutOverride
+	originalResponseHeader := fetcher.ResponseHeaderTimeoutOverride
 	t.Cleanup(func() {
 		fetcher.MaxAttempts = originalMaxAttempts
 		fetcher.Timeout = originalTimeout
 		fetcher.Delay = originalDelay
+		fetcher.DialTimeoutOverride = originalDial
+		fetcher.TLSHandshakeTimeoutOverride = originalTLS
+		fetcher.ResponseHeaderTimeoutOverride = originalResponseHeader
+		fetcher.ConfigureTransport()
 	})
 
-	err := configureBundleFetcher(5, 750*time.Millisecond, 25*time.Millisecond)
+	// Zero overrides: phase timeouts are derived from bundle-timeout.
+	err := configureBundleFetcher(5, 750*time.Millisecond, 25*time.Millisecond, 0, 0, 0)
 
 	require.NoError(t, err)
 	assert.Equal(t, 5, fetcher.MaxAttempts)
 	assert.Equal(t, 750*time.Millisecond, fetcher.Timeout)
 	assert.Equal(t, 25*time.Millisecond, fetcher.Delay)
+	assert.Zero(t, fetcher.DialTimeoutOverride)
+	assert.Zero(t, fetcher.TLSHandshakeTimeoutOverride)
+	assert.Zero(t, fetcher.ResponseHeaderTimeoutOverride)
+}
+
+func TestConfigureBundleFetcherHonorsTransportOverrides(t *testing.T) {
+	originalTimeout := fetcher.Timeout
+	originalDial := fetcher.DialTimeoutOverride
+	originalTLS := fetcher.TLSHandshakeTimeoutOverride
+	originalResponseHeader := fetcher.ResponseHeaderTimeoutOverride
+	t.Cleanup(func() {
+		fetcher.Timeout = originalTimeout
+		fetcher.DialTimeoutOverride = originalDial
+		fetcher.TLSHandshakeTimeoutOverride = originalTLS
+		fetcher.ResponseHeaderTimeoutOverride = originalResponseHeader
+		fetcher.ConfigureTransport()
+	})
+
+	err := configureBundleFetcher(3, 5*time.Second, 0,
+		1*time.Second, 2*time.Second, 3*time.Second)
+
+	require.NoError(t, err)
+	assert.Equal(t, 1*time.Second, fetcher.DialTimeoutOverride)
+	assert.Equal(t, 2*time.Second, fetcher.TLSHandshakeTimeoutOverride)
+	assert.Equal(t, 3*time.Second, fetcher.ResponseHeaderTimeoutOverride)
+}
+
+func TestConfigureBundleFetcherAllowsOverrideAboveBudget(t *testing.T) {
+	originalTimeout := fetcher.Timeout
+	originalDial := fetcher.DialTimeoutOverride
+	originalTLS := fetcher.TLSHandshakeTimeoutOverride
+	originalResponseHeader := fetcher.ResponseHeaderTimeoutOverride
+	t.Cleanup(func() {
+		fetcher.Timeout = originalTimeout
+		fetcher.DialTimeoutOverride = originalDial
+		fetcher.TLSHandshakeTimeoutOverride = originalTLS
+		fetcher.ResponseHeaderTimeoutOverride = originalResponseHeader
+		fetcher.ConfigureTransport()
+	})
+
+	// An override larger than bundle-timeout is accepted: like the derived
+	// 250ms floor, an operator may keep a usable connection-setup timeout even
+	// when it exceeds the per-attempt budget. Only negative values are rejected.
+	err := configureBundleFetcher(3, 1*time.Second, 0,
+		5*time.Second, 0, 0)
+
+	require.NoError(t, err)
+	assert.Equal(t, 5*time.Second, fetcher.DialTimeoutOverride)
 }
 
 func TestConfigureBundleFetcherRejectsInvalidValues(t *testing.T) {
 	tests := []struct {
-		name        string
-		maxAttempts int
-		timeout     time.Duration
-		delay       time.Duration
-		errorText   string
+		name                  string
+		maxAttempts           int
+		timeout               time.Duration
+		delay                 time.Duration
+		dialTimeout           time.Duration
+		tlsHandshakeTimeout   time.Duration
+		responseHeaderTimeout time.Duration
+		errorText             string
 	}{
 		{
 			name:        "zero attempts",
@@ -60,6 +119,13 @@ func TestConfigureBundleFetcherRejectsInvalidValues(t *testing.T) {
 			delay:       -time.Millisecond,
 			errorText:   "bundle-delay must not be negative",
 		},
+		{
+			name:        "negative dial timeout",
+			maxAttempts: 1,
+			timeout:     time.Second,
+			dialTimeout: -time.Millisecond,
+			errorText:   "registry-dial-timeout must not be negative",
+		},
 	}
 
 	for _, test := range tests {
@@ -68,7 +134,8 @@ func TestConfigureBundleFetcherRejectsInvalidValues(t *testing.T) {
 			fetcher.Timeout = 9 * time.Second
 			fetcher.Delay = 9 * time.Millisecond
 
-			err := configureBundleFetcher(test.maxAttempts, test.timeout, test.delay)
+			err := configureBundleFetcher(test.maxAttempts, test.timeout, test.delay,
+				test.dialTimeout, test.tlsHandshakeTimeout, test.responseHeaderTimeout)
 
 			require.EqualError(t, err, test.errorText)
 			assert.Equal(t, 9, fetcher.MaxAttempts)

pkg/fetcher/bundle.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"io"
 	"log/slog"
+	"net"
 	"net/http"
 	"runtime"
 	"strings"
@@ -31,8 +32,121 @@ var (
 	Timeout = time.Second * 3
 	// Delay between attempts to fetch bundles.
 	Delay = time.Duration(0)
+
+	// DialTimeoutOverride optionally pins the registry TCP dial timeout. Zero
+	// (the default) derives it from Timeout via resolveTransportTimeouts; a
+	// positive value is used verbatim.
+	DialTimeoutOverride = time.Duration(0)
+	// TLSHandshakeTimeoutOverride optionally pins the registry TLS-handshake
+	// timeout. Zero derives it from Timeout; a positive value is used verbatim.
+	TLSHandshakeTimeoutOverride = time.Duration(0)
+	// ResponseHeaderTimeoutOverride optionally pins the wait for the registry's
+	// response headers. Zero derives it from Timeout; a positive value is used
+	// verbatim. The overrides let an operator with an unusual registry or
+	// forward proxy tune a single phase without restating the whole budget.
+	ResponseHeaderTimeoutOverride = time.Duration(0)
 )
 
+// Registry connection-phase timeouts are a decomposition of the per-attempt
+// fetch budget (Timeout), not independently chosen constants: go-containerregistry's
+// stock transport uses a 30s dial and 10s TLS-handshake timeout — both longer
+// than a typical fetch attempt — so a request routed (via the registry's global
+// endpoint) to a degraded geo-replica can burn the whole deadline in connection
+// setup and be cancelled without ever retrying against a healthy replica. Each
+// phase is instead bounded below Timeout so a stall is abandoned early, leaving
+// parent (gatekeeper / admission-webhook) budget for retryBundle to open a
+// fresh connection. Deriving from Timeout keeps the provider correct by default
+// at any -bundle-timeout rather than baking in values tuned for one deployment.
+const (
+	// dialTimeoutFraction and tlsHandshakeTimeoutFraction bound the two
+	// sequential phases of connection establishment; responseHeaderTimeoutFraction
+	// bounds the wait for the first response byte after the request is written.
+	dialTimeoutFraction           = 0.6
+	tlsHandshakeTimeoutFraction   = 0.6
+	responseHeaderTimeoutFraction = 0.8
+	// minPhaseTimeout is a hard floor on every derived phase timeout. Below
+	// ~250ms the dial and TLS-handshake phases start tripping on normal network
+	// latency, turning a healthy-but-not-instant connection into a spurious
+	// failure. The floor is applied even when it exceeds a very small
+	// -bundle-timeout (see pickPhaseTimeout).
+	minPhaseTimeout = 250 * time.Millisecond
+)
+
+// registryTransport is the shared HTTP transport used for all registry access.
+// It is created once so the underlying connection pool is reused across
+// requests (creating a transport per request would defeat pooling). It is
+// rebuilt by ConfigureTransport once flags are parsed; until then it reflects
+// the default Timeout.
+var registryTransport = newRegistryTransport()
+
+// ConfigureTransport rebuilds the shared registry transport from the current
+// Timeout and phase overrides. Call it once at startup after Timeout and the
+// *Override vars are set (e.g. from flags). It reassigns a package-level var and
+// is not safe to call concurrently with in-flight registry fetches.
+func ConfigureTransport() {
+	registryTransport = newRegistryTransport()
+}
+
+// resolveTransportTimeouts returns the effective dial, TLS-handshake, and
+// response-header timeouts for a per-attempt budget of bundleTimeout. A positive
+// *Override is honored verbatim; a zero override derives that phase as a
+// fraction of bundleTimeout, floored at minPhaseTimeout (see pickPhaseTimeout —
+// for a bundleTimeout below the floor the returned value can exceed it).
+func resolveTransportTimeouts(bundleTimeout time.Duration) (dial, tlsHandshake, responseHeader time.Duration) {
+	dial = pickPhaseTimeout(DialTimeoutOverride, bundleTimeout, dialTimeoutFraction)
+	tlsHandshake = pickPhaseTimeout(TLSHandshakeTimeoutOverride, bundleTimeout, tlsHandshakeTimeoutFraction)
+	responseHeader = pickPhaseTimeout(ResponseHeaderTimeoutOverride, bundleTimeout, responseHeaderTimeoutFraction)
+	return dial, tlsHandshake, responseHeader
+}
+
+// pickPhaseTimeout returns override when positive, otherwise fraction*bundleTimeout
+// floored at minPhaseTimeout (which may exceed bundleTimeout for a very small
+// budget — see the note inside).
+func pickPhaseTimeout(override, bundleTimeout time.Duration, fraction float64) time.Duration {
+	if override > 0 {
+		return override
+	}
+	derived := time.Duration(float64(bundleTimeout) * fraction)
+	// Apply a hard floor. We deliberately keep it even when it exceeds
+	// bundleTimeout: shrinking a dial or TLS-handshake timeout to a few
+	// milliseconds to fit a tiny per-attempt budget would just trade a slow
+	// fetch for a guaranteed connection failure on normal latency. A
+	// -bundle-timeout below the floor cannot fetch a bundle over TLS from a
+	// real registry no matter how we size these phases, so in that (already
+	// broken) configuration we keep a usable floor and let the attempt context
+	// win — rather than derive an unusable sub-250ms timeout. This means a
+	// phase timeout CAN be larger than bundleTimeout; that is intended.
+	if derived < minPhaseTimeout {
+		derived = minPhaseTimeout
+	}
+	return derived
+}
+
+// newRegistryDialer builds the dialer used by the registry transport, carrying
+// the resolved per-attempt dial timeout. It is a separate function so the dial
+// timeout is unit-testable: an http.Transport's DialContext is an opaque closure
+// whose timeout cannot be read back off the transport.
+func newRegistryDialer(timeout time.Duration) *net.Dialer {
+	return &net.Dialer{
+		Timeout:   timeout,
+		KeepAlive: 30 * time.Second,
+	}
+}
+
+// newRegistryTransport returns an http.Transport based on go-containerregistry's
+// DefaultTransport but with the connection-establishment timeouts resolved from
+// the current per-attempt budget (see resolveTransportTimeouts). Cloning the
+// default preserves the other tuned fields (idle-connection pool sizes, HTTP/2,
+// proxy) while overriding only the timeouts.
+func newRegistryTransport() *http.Transport {
+	dial, tlsHandshake, responseHeader := resolveTransportTimeouts(Timeout)
+	t := remote.DefaultTransport.(*http.Transport).Clone()
+	t.DialContext = newRegistryDialer(dial).DialContext
+	t.TLSHandshakeTimeout = tlsHandshake
+	t.ResponseHeaderTimeout = responseHeader
+	return t
+}
+
 // FailureKind is a stable, low-cardinality classification of why a bundle
 // fetch failed. It is safe to use as a metric label value and a log field.
 type FailureKind string
@@ -396,6 +510,7 @@ func GetRemoteOptions(kc authn.Keychain) []remote.Option {
 	var opts = []remote.Option{
 		remote.WithUserAgent(UserAgentString),
 		remote.WithAuthFromKeychain(kc),
+		remote.WithTransport(registryTransport),
 	}
 
 	return opts