diff --git a/cmd/aaop/aaop.go b/cmd/aaop/aaop.go index 637434e..c044cd4 100644 --- a/cmd/aaop/aaop.go +++ b/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 } diff --git a/cmd/aaop/aaop_test.go b/cmd/aaop/aaop_test.go index 4a20f9c..5c71377 100644 --- a/cmd/aaop/aaop_test.go +++ b/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) diff --git a/pkg/fetcher/bundle.go b/pkg/fetcher/bundle.go index b75cccb..a75417c 100644 --- a/pkg/fetcher/bundle.go +++ b/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 diff --git a/pkg/fetcher/bundle_test.go b/pkg/fetcher/bundle_test.go index 34eae39..235fcea 100644 --- a/pkg/fetcher/bundle_test.go +++ b/pkg/fetcher/bundle_test.go @@ -285,3 +285,141 @@ func TestRetryBundleStopsOnNotFound(t *testing.T) { assert.Equal(t, 1, attempts, "a 404 must not be retried") assert.Equal(t, 1, fe.Attempts) } + +func TestResolveTransportTimeoutsDerivesFromBudget(t *testing.T) { + // With no overrides, each phase is a fraction of the per-attempt budget. + // At a 2.5s budget this reproduces the values this change originally + // shipped as constants (dial 1.5s, TLS 1.5s, response-header 2s), so the + // derivation is a generalization of that calibration, not a behavior change + // for the deploying environment. + dial, tlsHandshake, responseHeader := resolveTransportTimeouts(2500 * time.Millisecond) + assert.Equal(t, 1500*time.Millisecond, dial) + assert.Equal(t, 1500*time.Millisecond, tlsHandshake) + assert.Equal(t, 2000*time.Millisecond, responseHeader) + + // Every derived phase must stay strictly under the budget so a stall is + // abandoned before the attempt's context deadline. + assert.Less(t, dial, 2500*time.Millisecond) + assert.Less(t, tlsHandshake, 2500*time.Millisecond) + assert.Less(t, responseHeader, 2500*time.Millisecond) +} + +func TestResolveTransportTimeoutsHonorsOverrides(t *testing.T) { + defer restoreTransportOverrides(DialTimeoutOverride, TLSHandshakeTimeoutOverride, ResponseHeaderTimeoutOverride) + + DialTimeoutOverride = 400 * time.Millisecond + TLSHandshakeTimeoutOverride = 0 // still derived + ResponseHeaderTimeoutOverride = 900 * time.Millisecond + + dial, tlsHandshake, responseHeader := resolveTransportTimeouts(2 * time.Second) + assert.Equal(t, 400*time.Millisecond, dial, "positive override used verbatim") + assert.Equal(t, 1200*time.Millisecond, tlsHandshake, "zero override derives from budget") + assert.Equal(t, 900*time.Millisecond, responseHeader, "positive override used verbatim") +} + +func TestResolveTransportTimeoutsFloorsSmallBudget(t *testing.T) { + // For a modestly small budget the floor keeps phase timeouts off sub-100ms + // values that would trip on normal latency, while still staying under the + // budget. At 300ms every phase (0.6/0.6/0.8 -> 180/180/240ms) is floored to + // 250ms, which remains < 300ms. + dial, tlsHandshake, responseHeader := resolveTransportTimeouts(300 * time.Millisecond) + assert.Equal(t, minPhaseTimeout, dial) + assert.Equal(t, minPhaseTimeout, tlsHandshake) + assert.Equal(t, minPhaseTimeout, responseHeader) + assert.Less(t, dial, 300*time.Millisecond) +} + +func TestResolveTransportTimeoutsAppliesHardFloorBelowBudget(t *testing.T) { + // For a pathologically small budget the 250ms floor intentionally wins even + // though it exceeds the budget: we keep a usable dial/handshake floor rather + // than derive an unusable sub-10ms timeout. The attempt context simply fires + // first in this already-broken configuration. + const budget = 100 * time.Millisecond + dial, tlsHandshake, responseHeader := resolveTransportTimeouts(budget) + assert.Equal(t, minPhaseTimeout, dial) + assert.Equal(t, minPhaseTimeout, tlsHandshake) + assert.Equal(t, minPhaseTimeout, responseHeader) + assert.Greater(t, dial, budget, "the floor is kept even when it exceeds the budget") +} + +func TestNewRegistryDialerUsesResolvedTimeout(t *testing.T) { + // The dialer must carry the resolved dial timeout (the transport's + // DialContext closure hides it, so this is where dial-timeout regressions + // are caught). + d := newRegistryDialer(1234 * time.Millisecond) + assert.Equal(t, 1234*time.Millisecond, d.Timeout) + assert.Equal(t, 30*time.Second, d.KeepAlive) +} + +func TestRegistryTransportDialContextEnforcesDialTimeout(t *testing.T) { + defer restoreTransportOverrides(DialTimeoutOverride, TLSHandshakeTimeoutOverride, ResponseHeaderTimeoutOverride) + DialTimeoutOverride = 150 * time.Millisecond + tr := newRegistryTransport() + + // 192.0.2.1 is TEST-NET-1 (RFC 5737): reserved and unrouted, so a dial is + // dropped and blocks until the dial timeout fires rather than getting a + // fast connection-refused. This exercises the transport's DialContext + // end-to-end and would block far past the assertion window if the dialer + // reverted to go-containerregistry's stock 30s default. + start := time.Now() + conn, err := tr.DialContext(context.Background(), "tcp", "192.0.2.1:80") + elapsed := time.Since(start) + if conn != nil { + conn.Close() + } + + require.Error(t, err) + assert.Less(t, elapsed, 5*time.Second, "dial must abort at ~DialTimeout, not the stock 30s") +} + +func TestNewRegistryTransportUsesResolvedTimeouts(t *testing.T) { + defer restoreTransportOverrides(DialTimeoutOverride, TLSHandshakeTimeoutOverride, ResponseHeaderTimeoutOverride) + DialTimeoutOverride, TLSHandshakeTimeoutOverride, ResponseHeaderTimeoutOverride = 0, 0, 0 + + tr := newRegistryTransport() + + _, wantTLS, wantResponseHeader := resolveTransportTimeouts(Timeout) + assert.Equal(t, wantTLS, tr.TLSHandshakeTimeout) + assert.Equal(t, wantResponseHeader, tr.ResponseHeaderTimeout) + + // Cloning the default transport must preserve its connection-pool tuning + // rather than resetting to the net/http zero values. + assert.Equal(t, 100, tr.MaxIdleConns) + assert.Equal(t, 50, tr.MaxIdleConnsPerHost) + assert.True(t, tr.ForceAttemptHTTP2) + require.NotNil(t, tr.DialContext) +} + +func TestConfigureTransportRebuildsForCurrentTimeout(t *testing.T) { + originalTimeout := Timeout + originalTransport := registryTransport + defer func() { + Timeout = originalTimeout + registryTransport = originalTransport + }() + + Timeout = 4 * time.Second + ConfigureTransport() + + _, wantTLS, wantResponseHeader := resolveTransportTimeouts(4 * time.Second) + require.NotNil(t, registryTransport) + assert.Equal(t, wantTLS, registryTransport.TLSHandshakeTimeout) + assert.Equal(t, wantResponseHeader, registryTransport.ResponseHeaderTimeout) +} + +func TestGetRemoteOptionsIncludesTunedTransport(t *testing.T) { + // GetRemoteOptions must reuse the shared, tuned transport singleton so the + // connection pool is shared across requests. remote.Option is an opaque + // closure, so the transport can't be read back out; assert the option count + // instead, which regresses if remote.WithTransport(registryTransport) is + // dropped from the list. + require.NotNil(t, registryTransport) + opts := GetRemoteOptions(nil) + assert.Len(t, opts, 3) +} + +func restoreTransportOverrides(dial, tlsHandshake, responseHeader time.Duration) { + DialTimeoutOverride = dial + TLSHandshakeTimeoutOverride = tlsHandshake + ResponseHeaderTimeoutOverride = responseHeader +}