Tighten registry HTTP transport connection timeouts - #206
Draft
bdehamer wants to merge 5 commits into
Draft
Conversation
bdehamer
force-pushed
the
bdehamer-registry-transport-timeouts
branch
from
August 22, 2026 14:30
2cc95d3 to
34a9988
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a shared registry HTTP transport with shorter connection and response timeouts to improve retry behavior.
Changes:
- Clones and tunes
remote.DefaultTransport. - Reuses the transport across registry requests.
- Adds transport configuration tests.
Show a summary per file
| File | Description |
|---|---|
pkg/fetcher/bundle.go |
Defines and wires the tuned registry transport. |
pkg/fetcher/bundle_test.go |
Tests transport settings and option construction. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Balanced
The provider fetches attestation bundles through go-containerregistry's DefaultTransport, whose 30s dial and 10s TLS-handshake timeouts are far longer than a single fetch attempt (-bundle-timeout) and the fail-closed admission-webhook deadline. When a request is routed (via a geo-replicated registry's global endpoint) to a degraded replica, it stalls in connection setup and is cancelled at the deadline without ever retrying against a healthy replica — so the retry loop effectively collapses to one attempt. Install a shared http.Transport (cloned from remote.DefaultTransport, so the idle-pool sizes and HTTP/2 tuning are preserved) whose connection-phase timeouts are bounded below the per-attempt budget, and wire it in via remote.WithTransport. A stalled phase is then abandoned early, leaving parent budget for retryBundle to open a fresh connection (which the global endpoint may route to a healthy replica). Rather than bake in constants tuned for one deployment, the phase timeouts are a decomposition of the per-attempt budget and are derived from it: - dial = 0.6 * bundle-timeout - TLS handshake = 0.6 * bundle-timeout - response header = 0.8 * bundle-timeout (floored at 250ms) This keeps the provider correct by default at any -bundle-timeout, which matters for a general-purpose OSS provider run against registries with very different latency profiles. Operators can still pin an individual phase via -registry-dial-timeout / -registry-tls-handshake-timeout / -registry-response-header-timeout (0 = derive); a positive override must be less than bundle-timeout, validated at startup, so a phase timeout can never be silently swallowed by the attempt's context deadline. The transport is a singleton (shared connection pool) rebuilt once at startup by ConfigureTransport after flags are parsed. The provider can only couple transport timeouts to bundle-timeout; the outer invariant (attempts*bundle-timeout + delays < Provider.timeout < webhook timeout) spans gatekeeper/k8s config and remains a deployment-time concern. At the deployed bundle-timeout of 2.5s the derived values are dial 1.5s / TLS 1.5s / response-header 2s. Defense-in-depth alongside the retry budget and the cache + singleflight de-duplication shipped in v0.2.x (#192). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: adc26119-a4fc-4118-82b4-0601ae564252
bdehamer
force-pushed
the
bdehamer-registry-transport-timeouts
branch
from
August 22, 2026 19:56
34a9988 to
f5b702e
Compare
…he dialer Follow-up to review feedback on the derived transport timeouts: - pickPhaseTimeout could return a value at or above bundleTimeout when the 250ms floor exceeded a pathologically small budget (e.g. a 100ms bundle-timeout floored to 250ms), which broke the "each phase fires before the attempt context" invariant. Cap the floor so a derived phase is always strictly below bundleTimeout, falling back to the fractional value (which is < budget because every fraction is < 1) for tiny budgets. - Extract newRegistryDialer so the resolved dial timeout is unit-testable (an http.Transport's DialContext closure hides it), and add a behavioral test that dials an unrouted TEST-NET-1 address and asserts the dial aborts at ~DialTimeout rather than go-containerregistry's stock 30s — giving the central dial-timeout change real regression coverage. - Strengthen the GetRemoteOptions test to assert the tuned transport option is present (option count) rather than merely non-empty, so dropping remote.WithTransport regresses. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: adc26119-a4fc-4118-82b4-0601ae564252
golangci-lint (revive) flagged the derived-timeout change: - The three exported *Override vars shared one doc comment; revive requires each exported identifier's comment to begin with its own name. Give each var its own doc comment. - resolveTransportTimeouts used a bare return with named results; revive's bare-return rule wants explicit return expressions. No behavior change. Verified with golangci-lint run ./pkg/fetcher/... (0 issues). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: adc26119-a4fc-4118-82b4-0601ae564252
Earlier review feedback pushed the derived phase timeouts to always stay strictly below bundleTimeout, which meant a very small -bundle-timeout produced sub-250ms (even sub-10ms) dial/TLS timeouts. That trades a slow fetch for a guaranteed connection failure on normal latency — a dial or TLS handshake routinely needs more than a few milliseconds. Treat 250ms as a hard floor instead, applied even when it exceeds bundleTimeout. A -bundle-timeout below the floor cannot fetch a bundle over TLS from a real registry regardless of 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 timeout. This means a derived phase timeout can be larger than bundleTimeout; the code comment calls that out explicitly as intended behavior. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: adc26119-a4fc-4118-82b4-0601ae564252
Only reject negative override values. Rejecting an override >= bundle-timeout is inconsistent with the derived 250ms floor, which may itself exceed a very small per-attempt budget: in both cases the attempt context simply fires first. An operator may legitimately want to keep a usable connection-setup timeout (e.g. a slow TLS-terminating proxy) even when it is larger than the budget, and a per-attempt budget small enough to make that matter is operator error rather than something to guard against here. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: adc26119-a4fc-4118-82b4-0601ae564252
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The provider fetches attestation bundles through go-containerregistry's
remote.DefaultTransport, whose 30s dial and 10s TLS-handshake timeouts are far longer than a single fetch attempt (-bundle-timeout, deployed at 2.5s) and the 10s fail-closed admission-webhook deadline.This PR installs a shared
http.Transport(cloned fromremote.DefaultTransport, preserving its idle-pool sizes / HTTP/2 tuning) whose connection-phase timeouts are derived from the per-attempt budget, wired in viaremote.WithTransport.The phase timeouts are treated as a decomposition of the per-attempt budget and are derived from
-bundle-timeout:-bundle-timeout-bundle-timeout=2.5sEach derived phase is floored at 250ms to avoid sub-100ms timeouts on normal latency.
Operators can still pin an individual phase —
-registry-dial-timeout,-registry-tls-handshake-timeout,-registry-response-header-timeout(each0= derive). Only negative overrides are rejected; an override may exceed-bundle-timeout— like the 250ms floor — in which case the attempt's context deadline simply fires first.Why
Bounding each connection phase well under go-containerregistry's stock 30s/10s defaults lets a stalled connection fail fast, so the retry establishes a fresh connection — which the global endpoint may route to a healthy replica — instead of burning the deadline in a stalled dial/handshake.
Design notes
-bundle-timeout.-bundle-timeoutif an operator wants it to.-bundle-timeout. The upward invariant —attempts × bundle-timeout + delays < Provider.timeout < webhook timeoutSeconds— spans gatekeeper/k8s config the provider can't observe, so it stays a deployment-time concern.