Tighten registry HTTP transport connection timeouts - #206
Merged
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
Member
|
This looks very promising, adding a connect timeout is the natural next step 🚀 |
bdehamer
marked this pull request as ready for review
August 24, 2026 15:31
kommendorkapten
approved these changes
Aug 24, 2026
bdehamer
added a commit
that referenced
this pull request
Aug 25, 2026
The connection-establishment timeouts added in #206 (dial, TLS handshake, response header) only govern setting up a brand-new connection, and ResponseHeaderTimeout stops once headers arrive. None of them bound a stall during the response *body* read on an already-established connection — so a wedged reused keep-alive connection could hang a manifest/referrers/blob fetch past the retry envelope all the way to the outer admission deadline (the incident: reason=canceled, step=descriptor, status=0, idle CPU, no network error). Add deadlineRoundTripper: a RoundTripper decorator that wraps each registry request in context.WithTimeout and wraps resp.Body so the deadline also covers body reads (the same mechanism http.Client.Timeout uses internally). go-containerregistry v0.21.9 exposes only remote.WithTransport — no http.Client hook — so the wall must be a decorator. It is applied in GetRemoteOptions over the shared *http.Transport, so #206's connection-phase timeouts still apply beneath it, and it protects every caller of that transport. New flag -registry-request-timeout (default 0 = derive from bundle-timeout, a safe ceiling that won't fire before the per-attempt context). Only negative values are rejected. The registry timeout overrides are grouped into a registryTimeouts struct so configureBundleFetcher stays within revive's argument-count limit. Note on scope: an earlier draft of this change also added HTTP/2 keepalive (ReadIdleTimeout/PingTimeout). It was dropped after verifying the entire dxcrprod fetch path is HTTP/1.1 — the azurecr.io registry frontend (OpenResty) and the blob backend it 307-redirects to (*.blob.core.windows.net, Azure Blob Storage) both decline h2 via ALPN. With no HTTP/2 connection anywhere in the path, keepalive would be inert; the overall request wall is the fix that actually applies. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
bdehamer
added a commit
that referenced
this pull request
Aug 26, 2026
* Tune registry connection pool to self-heal dead connections The connection-establishment timeouts in #206 only bound setting up a *new* connection. A connection that a load balancer or gateway silently drops while it sits idle in the keep-alive pool is invisible to them: the next request that reuses it stalls until the outer admission deadline (the class-H "reused connection stall" — idle pods, status=0, no network error). ACR's whole fetch path is HTTP/1.1 (the azurecr.io OpenResty frontend and the *.blob.core.windows.net backend both decline h2), so HTTP/2 keepalive PINGs don't apply — the lever that does is the TCP/pool lifecycle. Tighten three transport knobs so a wedged idle connection is retired before it can be reused, and cap how many can pile up: - dialer KeepAlive 30s -> 10s: keep pooled connections warm so an idle intermediary (e.g. an Azure Load Balancer with a ~4-minute idle cutoff) is less likely to reap them silently. - IdleConnTimeout 90s -> 10s: we close an idle connection well before those cutoffs, so a silently-dropped one is retired by us instead of lingering to stall the next request. - MaxIdleConns/PerHost 100/50 -> 25/25: bound dead-connection accumulation. Sized from logs — peak in-flight fetches per pod is single-digit (5-7 across normal, busy, and incident windows), and each fetch touches two hosts (registry + the blob backend it 307-redirects to), so ~25 covers the reuse working set with headroom. Exposed as fetcher package vars with sensible defaults and optional override flags (-registry-dial-keep-alive, -registry-idle-conn-timeout, -registry-max-idle-conns, -registry-max-idle-conns-per-host), validated and wired in a new configureRegistryPool helper before the transport is built. Zero keeps net/http semantics (no limit / use default); only negatives are rejected. Defaults are safe, so no deployment change is required to get the fix. Complementary to #208 (overall per-request wall): that bounds a request once wedged; this stops connections from becoming wedge-prone. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Clarify keep-alive zero semantics; de-duplicate pool test Address review feedback on the connection-pool tuning: - registry-dial-keep-alive is a net.Dialer.KeepAlive, not an http.Transport field, so 0 selects Go's default keep-alive period (~15s), not "the net/http default". Correct the flag help, the configureRegistryPool doc (which distinguishes dialer keep-alive from the http.Transport pool fields), and the DialKeepAlive var comment. - Drop the duplicate package-var assertions in TestNewRegistryTransportUsesResolvedTimeouts: it now asserts only the default values, and TestNewRegistryTransportAppliesPoolTuning already proves the fields are wired (not hardcoded) via non-default values. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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.