fix: remedies a reconnect-time xDS race#13868
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a reconnect-time xDS race in snapshotPerClient where routes/listeners could be published before all referenced backend clusters are present, causing transient NC/500 responses during controller restarts.
Changes:
- Defers per-client snapshot publication until all clusters referenced by routes/listeners are present (excluding clusters explicitly tracked as errored).
- Adds logic to collect cluster references from RouteConfiguration and TCP proxy listener filters to drive the readiness check.
- Adds regression tests covering both the “partial snapshot is deferred” case and the “errored referenced cluster still publishes” case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/kgateway/proxy_syncer/perclient.go | Adds a referenced-cluster readiness gate to snapshot publishing, plus helper functions to discover referenced clusters from route/listener resources. |
| pkg/kgateway/proxy_syncer/perclient_test.go | Adds regression coverage to ensure snapshots are deferred until referenced clusters exist, while still publishing when referenced clusters are marked errored. |
| if missingClusters := findMissingReferencedClusters( | ||
| listenerRouteSnapshot.Routes, | ||
| listenerRouteSnapshot.Listeners, | ||
| clusterResources.Items, | ||
| clustersForUcc.erroredClusters, | ||
| ); len(missingClusters) > 0 { | ||
| logger.Info( | ||
| "defer building snapshot until all referenced clusters are ready", | ||
| "client", ucc.ResourceName(), | ||
| "missing_clusters", missingClusters, | ||
| ) | ||
| return nil |
There was a problem hiding this comment.
findMissingReferencedClusters walks every route/listener proto (including unpacking Any) on the per-client snapshot hot path. With many connected clients or large LDS/RDS, this can add noticeable CPU/alloc overhead per update. Consider computing/caching the referenced-cluster set once per GatewayXdsResources version (or once per snapshot build) and reusing it across clients, rather than re-traversing protos for every client snapshot.
| if missingClusters := findMissingReferencedClusters( | ||
| listenerRouteSnapshot.Routes, | ||
| listenerRouteSnapshot.Listeners, | ||
| clusterResources.Items, | ||
| clustersForUcc.erroredClusters, | ||
| ); len(missingClusters) > 0 { | ||
| logger.Info( | ||
| "defer building snapshot until all referenced clusters are ready", | ||
| "client", ucc.ResourceName(), | ||
| "missing_clusters", missingClusters, | ||
| ) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
logger.Info here can become very noisy during restarts/reconnect storms (it runs per client and can repeat until readiness). Consider lowering to Debug, adding rate-limiting/sampling, or logging only on state transitions (e.g., first time a client becomes blocked/unblocked) to avoid log volume impacting operations.
| func findMissingReferencedClusters( | ||
| routes envoycache.Resources, | ||
| listeners envoycache.Resources, | ||
| clusters map[string]envoycachetypes.ResourceWithTTL, | ||
| erroredClusters []string, | ||
| ) []string { |
There was a problem hiding this comment.
This performs a full proto-reflection walk of Routes and Listeners for every per-client snapshot evaluation, even though those resources are gateway-scoped and identical across clients. Consider computing/caching the referenced cluster set once per listenerRouteSnapshot version/hash (e.g., store alongside GatewayXdsResources or memoize by resource version) and reusing it across clients to reduce CPU and allocations under many connected clients.
| referencedClusters := make(map[string]struct{}) | ||
| collectResourceClusterReferences(routes, referencedClusters) | ||
| collectResourceClusterReferences(listeners, referencedClusters) |
There was a problem hiding this comment.
This performs a full proto-reflection walk of Routes and Listeners for every per-client snapshot evaluation, even though those resources are gateway-scoped and identical across clients. Consider computing/caching the referenced cluster set once per listenerRouteSnapshot version/hash (e.g., store alongside GatewayXdsResources or memoize by resource version) and reusing it across clients to reduce CPU and allocations under many connected clients.
| if anyMsg, ok := msg.Interface().(*anypb.Any); ok { | ||
| nestedMsg, err := anyMsg.UnmarshalNew() | ||
| if err != nil { | ||
| // Typed extensions whose Go types aren't linked into this binary will fail here; | ||
| // that's expected, but log at debug so genuinely malformed configs are diagnosable. | ||
| logger.Debug("skipping typed_config during cluster reference scan", "type_url", anyMsg.GetTypeUrl(), "error", err) | ||
| return | ||
| } |
There was a problem hiding this comment.
Even at Debug, this log can be emitted repeatedly inside a recursive walk, potentially producing large log volume if there are Any-typed configs that can’t be unmarshaled (and this runs on each recompute). Consider deduplicating (e.g., log once per type_url per process, or once per snapshot version) to keep logs useful while avoiding amplification.
|
|
||
| g.Consistently(func() int { | ||
| return len(snapshots.List()) | ||
| }, 200*time.Millisecond, 20*time.Millisecond).Should(gomega.Equal(0)) |
There was a problem hiding this comment.
The regression assertion relies on a relatively short time window (200ms) to prove “no snapshot was published.” This can be flaky in slow/contended CI or can miss regressions if the buggy publish happens after the window. Consider increasing the consistency window to something more robust, and/or asserting an event-driven signal (e.g., block until the collection processes at least one reconciliation cycle, then assert no snapshot until cluster-b is added).
| }, 200*time.Millisecond, 20*time.Millisecond).Should(gomega.Equal(0)) | |
| }, time.Second, 20*time.Millisecond).Should(gomega.Equal(0)) |
| // startTrafficAndAssertNoErrors runs load through the gateway while restartFunc | ||
| // executes. It fails if hey reports any transport errors or non-200 statuses. | ||
| func (s *testingSuiteKgateway) startTrafficAndAssertNoErrors(duration time.Duration, restartFunc func()) { | ||
| kCli := kubectl.NewCli() | ||
| args := []string{ | ||
| "exec", "-n", "hey", "heygw", "--", | ||
| "hey", "-disable-keepalive", | ||
| "-c", "4", "-q", "10", "--cpus", "1", | ||
| "-z", duration.String(), | ||
| "-m", "GET", "-t", "1", | ||
| "-host", "example.com", | ||
| "http://gw.default.svc.cluster.local:8080", | ||
| } |
There was a problem hiding this comment.
startTrafficAndAssertNoErrors() runs hey for a fixed duration, but the restartFunc can legitimately take longer than that (RestartDeploymentAndWait ultimately uses kubectl rollout status with a much larger timeout). If the rollout exceeds the duration, traffic will stop early and the test can pass without actually covering the full restart window. Consider driving hey’s lifetime off the restartFunc completion (or setting the duration based on the rollout timeout / restart count) so traffic is guaranteed to span the entire restart.
46e492b to
9a1be35
Compare
This fixes a reconnect-time xDS race that can surface during kgateway controller restarts. When a new uniquely connected client is created, `snapshotPerClient` can run in parallel with per-client backend cluster translation. The existing guard only waited for the per-client cluster collection to become non-`nil`, which still allowed a partial CDS snapshot to be published for that client. In that state, routes and listeners could already reference backend clusters that were not yet present in the snapshot, and Envoy would return `500` responses with the `NC` flag until the missing clusters arrived in a later update. This change defers publishing a per-client snapshot until all clusters referenced by the route and listener resources are present in the merged cluster set. Clusters already tracked as errored are excluded from that readiness check so existing error-handling behavior is preserved. /kind fix ```release-note Fix a reconnect-time xDS race where Envoy could briefly receive routes and listeners before all referenced backend clusters were present, causing transient NC/500 responses during controller restart. ``` Added regression coverage for the partial-snapshot case: - a new test that failed on the old code because a snapshot was published before all referenced clusters were ready - a companion test confirming snapshots are still published when a referenced cluster is explicitly marked errored Verified with: ```bash go test ./pkg/kgateway/proxy_syncer ``` Signed-off-by: David L. Chandler <[email protected]>
Signed-off-by: David L. Chandler <[email protected]>
Signed-off-by: David L. Chandler <[email protected]>
…resolution and route-transition readiness # Description **Motivation:** kgateway-dev#13868 made per-client snapshot publication conditional on completeness: `snapshotPerClient` returns nil (a KRT Delete the syncer intentionally ignores) unless every cluster referenced by LDS/RDS is present and every referenced EDS cluster has a CLA. That predicate is evaluated inside an eventually-consistent dataflow with no completeness oracle, so permanent deferral is inherent rather than incidental. The kgateway-dev#14184 family of reports shows the resulting failure modes: a single Envoy stranded on stale endpoints (upstream connect failures to deleted pods), a redeployed backend whose new endpoints are never programmed, and whole-gateway starvation under modest route churn where every defer lists clusters that demonstrably exist. Two adjacent defects ride along: stale CLAs retained in the snapshot make go-control-plane suppress named state-of-the-world ADS EDS responses (`ADS mode: not responding to request ...`), and the previous remedy attempt — gating the whole snapshot on endpoint usability — fixes the warming 503 window at the cost of freezing entire gateways on any scale-to-zero backend. **What changed:** publication is now unconditional; safety is obtained by construction of each snapshot, liveness by level-triggered reconciliation, and make-before-break at the granularity where it is sound. Full design: `devel/architecture/perclient-xds-publication.md`. Publication rules (replacing the three readiness guards): - **R1 — endpoint truth.** A cluster present in current inputs always publishes its current CLA, **including an empty one**: scale-to-zero is truth, and old endpoints are never retained for a known cluster. - **R2 — carry-forward.** A referenced cluster absent from current inputs but present in the previously published snapshot is carried forward together with its CLA, so an internal pipeline hole keeps that one cluster stable while everything else makes progress. - **R3 — per-route holdback.** A referenced cluster with nothing to carry (brand-new, not yet translated) withholds only the route entries / TCP filter chains that target it — held at their previously published version or omitted if new. All other routes publish. - **S4 — route-transition readiness (make-before-break).** A route entry or TCP filter chain that is **new or retargeted** activates only once its new target is usable (non-EDS, or a CLA with at least one non-UNHEALTHY endpoint); until then it holds its previous form. Entries with unchanged targets are never held, so S4 cannot reintroduce the scale-to-zero freeze. Transition checking is skipped on controller cold start (no baseline; Envoy warming covers the window) and on endpoints-only updates (version-gated). - **S2 — EDS subset.** Published EDS resources are exactly the CLAs required by EDS clusters in the same snapshot's CDS, fixing the ADS not-responding failure. The filtered EDS version is derived from both the filtered content and the upstream version: content alone misses the policy-attachment re-warm bump, upstream alone misses remove/re-add transitions — each omission stalls Envoy on `initial_fetch_timeout`, and each has a pinning test driving a real `SnapshotCache.CreateWatch`. - **S1 — safety net.** After pruning, references are re-collected; if a dangling reference survives (a shape the pruner does not model), that update is withheld, the client stays marked stuck, and the heartbeat retries. Envoy never sees a route to a missing cluster. Liveness foundation: - A **demand-driven heartbeat** re-runs the per-client collections when any connected client is stuck (deferred, or connected-but-never-published), bounding internal-hole staleness to one interval (default 30s, `KGW_PERCLIENT_HEARTBEAT_INTERVAL`; jittered; near-zero cost when healthy). - An independent **reclaim loop** clears retained xDS cache entries for clients gone from the connected set past a grace period, fixing the previously documented unbounded SnapshotCache leak. Observability: `kgateway_xds_snapshot_perclient_{defers,recoveries,reclaimed, carried_clusters,held_routes,synthesized_clas}_total`. Escape hatch: `KGW_LEGACY_SNAPSHOT_GATE=true` restores the previous defer-until-complete behavior for one release. **Related issues:** Refs kgateway-dev#14184. Refines kgateway-dev#13868. # Change Type /kind fix # Changelog ```release-note Per-client xDS snapshots are now always published: clusters temporarily missing from internal state are carried forward from the last published snapshot, brand-new or retargeted routes activate only once their backend has a usable endpoint (holding their previous form until then), and endpoint updates — including scale-to-zero — always propagate. This fixes proxies stranded on stale endpoints, redeployed backends never receiving endpoint updates, gateways freezing under route churn, and ADS EDS responses being suppressed by stale endpoint resources. A periodic reconciler bounds recovery from missed internal updates, leaked snapshot-cache entries for departed proxies are reclaimed, and new kgateway_xds_snapshot_perclient_* metrics expose deferral, carry-forward, holdback, and recovery activity. Set KGW_LEGACY_SNAPSHOT_GATE=true to restore the previous behavior. ``` # Additional Notes - Code map: `perclient.go` (guards -> wrapper metadata, S2 filter + combined version, legacy gate), `kube_gw_translator_syncer.go` (publish-time resolution: R2/R1-edge/R3/S4, version recompute only on mutation), `perclient_prune.go` (transition-aware route/TCP-chain holdback, usable predicate), `proxy_syncer.go` (heartbeat + reclaim loops, stuck-client feedback from the safety net), `perclient_reconcile.go`, `metrics.go`, `xdswrapper.go` (`+noKrtEquals` metadata; `Equals` unchanged — the metadata derives from already-compared resource versions). - Tests: one unit test per design rule (R1 empty-CLA truth, R2 carry, R3 omit/hold, S4 hold-retarget-until-usable/omit-new/cold-start-skip/ fast-path-skip/TCP variants, S3 guard, S2 subset, usable predicate, safety-net refusal via an inline-HCM reference, legacy gate); three `CreateWatch`-driven ADS respondability tests (GCP-A1) covering cluster removal, `EdsClusterConfig.service_name`, and remove-then-re-add; the heartbeat/reconciler suites. Full package suite green under `-race`. - e2e: new `XdsWarming` suite (route retarget, weighted split, and new-route scenarios against a real Envoy — black-box-identical behavior to the rejected whole-snapshot usability gate, via S4) and `PerClientXDS` (endpoint-follow regression), both wired into the CI matrix. - Known benign behaviors, documented in code: a hole->no-hole transition can cause one redundant CDS push (content-hash vs upstream-hash version derivation across paths); unnamed route entries / filter chains cannot be matched for holdback and are omitted rather than held; a retarget whose new target never becomes usable holds its previous form indefinitely — by design, mirroring Envoy cluster warming, and visible via `held_routes`. - Deliberately NOT included: re-keying the per-client collections onto the client axis. With the publication invariants and the heartbeat in place it is no longer needed for correctness, and under realistic backend churn it inverts the fan-out unfavorably (every Service create/delete would recompute all clients x all backends). The structural fix for the underlying dropped-recompute behavior is being pursued upstream in istio/krt instead. - Follow-ups: port the `xdscheck` semantic checker and the Lean formal seam (KRT-A1 assumption + liveness theorem) from the formal-methods branch; file the istio/krt issue (drafted). - Backport intent: Phase 1 (publication rules R1-R3 + S2 + heartbeat/reclaim + metrics + escape hatch) is the v2.3.x patch candidate; S4 and the warming e2e are main-first. Signed-off-by: David L. Chandler <[email protected]>
…resolution and route-transition readiness # Description **Motivation:** kgateway-dev#13868 made per-client snapshot publication conditional on completeness: `snapshotPerClient` returns nil (a KRT Delete the syncer intentionally ignores) unless every cluster referenced by LDS/RDS is present and every referenced EDS cluster has a CLA. That predicate is evaluated inside an eventually-consistent dataflow with no completeness oracle, so permanent deferral is inherent rather than incidental. The kgateway-dev#14184 family of reports shows the resulting failure modes: a single Envoy stranded on stale endpoints (upstream connect failures to deleted pods), a redeployed backend whose new endpoints are never programmed, and whole-gateway starvation under modest route churn where every defer lists clusters that demonstrably exist. Two adjacent defects ride along: stale CLAs retained in the snapshot make go-control-plane suppress named state-of-the-world ADS EDS responses (`ADS mode: not responding to request ...`), and the previous remedy attempt — gating the whole snapshot on endpoint usability — fixes the warming 503 window at the cost of freezing entire gateways on any scale-to-zero backend. **What changed:** publication is now unconditional; safety is obtained by construction of each snapshot, liveness by level-triggered reconciliation, and make-before-break at the granularity where it is sound. Full design: `devel/architecture/perclient-xds-publication.md`. Publication rules (replacing the three readiness guards): - **R1 — endpoint truth.** A cluster present in current inputs always publishes its current CLA, **including an empty one**: scale-to-zero is truth, and old endpoints are never retained for a known cluster. - **R2 — carry-forward.** A referenced cluster absent from current inputs but present in the previously published snapshot is carried forward together with its CLA, so an internal pipeline hole keeps that one cluster stable while everything else makes progress. - **R3 — per-route holdback.** A referenced cluster with nothing to carry (brand-new, not yet translated) withholds only the route entries / TCP filter chains that target it — held at their previously published version or omitted if new. All other routes publish. - **S4 — route-transition readiness (make-before-break).** A route entry or TCP filter chain that is **new or retargeted** activates only once its new target is usable (non-EDS, or a CLA with at least one non-UNHEALTHY endpoint); until then it holds its previous form. Entries with unchanged targets are never held, so S4 cannot reintroduce the scale-to-zero freeze. Transition checking is skipped on controller cold start (no baseline; Envoy warming covers the window) and on endpoints-only updates (version-gated). - **S2 — EDS subset.** Published EDS resources are exactly the CLAs required by EDS clusters in the same snapshot's CDS, fixing the ADS not-responding failure. The filtered EDS version is derived from both the filtered content and the upstream version: content alone misses the policy-attachment re-warm bump, upstream alone misses remove/re-add transitions — each omission stalls Envoy on `initial_fetch_timeout`, and each has a pinning test driving a real `SnapshotCache.CreateWatch`. - **S1 — safety net.** After pruning, references are re-collected; if a dangling reference survives (a shape the pruner does not model), that update is withheld, the client stays marked stuck, and the heartbeat retries. Envoy never sees a route to a missing cluster. Liveness foundation: - A **demand-driven heartbeat** re-runs the per-client collections when any connected client is stuck (deferred, or connected-but-never-published), bounding internal-hole staleness to one interval (default 30s, `KGW_PERCLIENT_HEARTBEAT_INTERVAL`; jittered; near-zero cost when healthy). - An independent **reclaim loop** clears retained xDS cache entries for clients gone from the connected set past a grace period, fixing the previously documented unbounded SnapshotCache leak. Observability: `kgateway_xds_snapshot_perclient_{defers,recoveries,reclaimed, carried_clusters,held_routes,synthesized_clas}_total`. Escape hatch: `KGW_LEGACY_SNAPSHOT_GATE=true` restores the previous defer-until-complete behavior for one release. **Related issues:** Refs kgateway-dev#14184. Refines kgateway-dev#13868. # Change Type /kind fix # Changelog ```release-note Per-client xDS snapshots are now always published: clusters temporarily missing from internal state are carried forward from the last published snapshot, brand-new or retargeted routes activate only once their backend has a usable endpoint (holding their previous form until then), and endpoint updates — including scale-to-zero — always propagate. This fixes proxies stranded on stale endpoints, redeployed backends never receiving endpoint updates, gateways freezing under route churn, and ADS EDS responses being suppressed by stale endpoint resources. A periodic reconciler bounds recovery from missed internal updates, leaked snapshot-cache entries for departed proxies are reclaimed, and new kgateway_xds_snapshot_perclient_* metrics expose deferral, carry-forward, holdback, and recovery activity. Set KGW_LEGACY_SNAPSHOT_GATE=true to restore the previous behavior. ``` # Additional Notes - Code map: `perclient.go` (guards -> wrapper metadata, S2 filter + combined version, legacy gate), `kube_gw_translator_syncer.go` (publish-time resolution: R2/R1-edge/R3/S4, version recompute only on mutation), `perclient_prune.go` (transition-aware route/TCP-chain holdback, usable predicate), `proxy_syncer.go` (heartbeat + reclaim loops, stuck-client feedback from the safety net), `perclient_reconcile.go`, `metrics.go`, `xdswrapper.go` (`+noKrtEquals` metadata; `Equals` unchanged — the metadata derives from already-compared resource versions). - Tests: one unit test per design rule (R1 empty-CLA truth, R2 carry, R3 omit/hold, S4 hold-retarget-until-usable/omit-new/cold-start-skip/ fast-path-skip/TCP variants, S3 guard, S2 subset, usable predicate, safety-net refusal via an inline-HCM reference, legacy gate); three `CreateWatch`-driven ADS respondability tests (GCP-A1) covering cluster removal, `EdsClusterConfig.service_name`, and remove-then-re-add; the heartbeat/reconciler suites. Full package suite green under `-race`. - e2e: new `XdsWarming` suite (route retarget, weighted split, and new-route scenarios against a real Envoy — black-box-identical behavior to the rejected whole-snapshot usability gate, via S4) and `PerClientXDS` (endpoint-follow regression), both wired into the CI matrix. - Known benign behaviors, documented in code: a hole->no-hole transition can cause one redundant CDS push (content-hash vs upstream-hash version derivation across paths); unnamed route entries / filter chains cannot be matched for holdback and are omitted rather than held; a retarget whose new target never becomes usable holds its previous form indefinitely — by design, mirroring Envoy cluster warming, and visible via `held_routes`. - Deliberately NOT included: re-keying the per-client collections onto the client axis. With the publication invariants and the heartbeat in place it is no longer needed for correctness, and under realistic backend churn it inverts the fan-out unfavorably (every Service create/delete would recompute all clients x all backends). The structural fix for the underlying dropped-recompute behavior is being pursued upstream in istio/krt instead. - Follow-ups: port the `xdscheck` semantic checker and the Lean formal seam (KRT-A1 assumption + liveness theorem) from the formal-methods branch; file the istio/krt issue (drafted). - Backport intent: Phase 1 (publication rules R1-R3 + S2 + heartbeat/reclaim + metrics + escape hatch) is the v2.3.x patch candidate; S4 and the warming e2e are main-first. Signed-off-by: David L. Chandler <[email protected]>
…ounded warm-up gate # Description **Motivation:** kgateway-dev#13868 made per-client snapshot publication conditional on completeness: `snapshotPerClient` returns nil (a KRT Delete the syncer intentionally ignores) unless every cluster referenced by LDS/RDS is present and every referenced EDS cluster has a CLA. That predicate is evaluated inside an eventually-consistent dataflow with no completeness oracle, so permanent deferral is inherent rather than incidental. The kgateway-dev#14184 family of reports shows the resulting failure modes: a single Envoy stranded on stale endpoints (upstream connect failures to deleted pods), a redeployed backend whose new endpoints are never programmed, and whole-gateway starvation under modest route churn. Two adjacent defects ride along: stale CLAs retained in snapshots make go-control-plane suppress named state-of-the-world ADS EDS responses (`ADS mode: not responding to request ...`), and nothing validated final xDS resources before they reached the cache, so a malformed resource from any plugin could overwrite the last good snapshot. **What changed:** the completeness gate is removed; what remains is scoped to the problem kgateway-dev#13868 actually needed to solve — a reconnecting client briefly receiving a partial CDS — and is bounded by a clock, never by a predicate. Design: `devel/architecture/perclient-xds-publication.md`. - **The transform always builds.** `snapshotPerClient` produces the best snapshot available from current inputs; the readiness guards are deleted. - **Publish-time validation** (before `SetSnapshot`): nil/mistyped/misnamed resources, generated proto validation, duplicate listener filter chain matches, snapshot consistency, and SDS references without their secret always withhold — these indicate bugs, and Envoy keeps the last good snapshot while the reconciler retries. - **Bounded warm-up gate:** missing dataplane cluster references defer publication only while the client has never been published and a warm-up deadline (15s) has not expired. Normal convergence beats the deadline, so first snapshots are coherent; if inputs never complete, publication proceeds with the best available snapshot — deferral is bounded by construction. After first publish, partial snapshots publish immediately with a warning: the affected routes transiently return no-cluster errors (the pre-kgateway-dev#13868 behavior) and heal on the next event or heartbeat tick. - **Missing-CLA synthesis:** a required-but-missing CLA is synthesized empty, keeping the snapshot consistent and avoiding the `initial_fetch_timeout` stall; the real CLA replaces it when produced. Empty CLAs from scale-to-zero always publish — endpoint truth is never withheld or retained. - **S2 (EDS subset):** published EDS resources are exactly the CLAs required by EDS clusters in the same snapshot's CDS, fixing the ADS not-responding failure. The filtered EDS version combines filtered content with the upstream version; each signal alone leaves a watch "up to date" in a case that stalls Envoy (remove/re-add vs the policy-attachment re-warm bump), and each case has a pinning test driving a real `SnapshotCache.CreateWatch`. - **Level-triggered liveness:** a demand-driven heartbeat re-runs the per-client collections whenever a connected client is stuck (deferred, or connected-but-never-published), bounding internal-hole staleness to one interval (default 30s, `KGW_PERCLIENT_HEARTBEAT_INTERVAL`); an independent reclaim loop clears retained cache entries for departed clients, fixing the unbounded SnapshotCache leak. Observability: `kgateway_xds_snapshot_perclient_defers_total{reason}` (reasons `warmup`, `missing_clusters_published`, `invalid_snapshot`, `endpoints_not_ready`), `..._recoveries_total`, `..._reclaimed_total`. **Related issues:** Refs kgateway-dev#14184. Refines kgateway-dev#13868. # Change Type /kind fix # Changelog ```release-note Per-client xDS snapshots are now always published once a client's brief warm-up window completes: endpoint updates (including scale-to-zero) always propagate, a temporarily incomplete input can no longer freeze a gateway or strand a proxy on stale endpoints, and stale endpoint resources no longer suppress ADS EDS responses. Snapshots are validated before publication so malformed resources preserve the last good configuration instead of overwriting it. A periodic reconciler bounds recovery from missed internal updates, and leaked snapshot-cache entries for departed proxies are reclaimed. New kgateway_xds_snapshot_perclient_* metrics expose deferral, recovery, and reclaim activity. ``` # Additional Notes Must be forward-ported to main, but that can consider doing more than this with more risk (see the design doc) to solve even more bugs (transients). Accepted transients, all bounded and all pre-kgateway-dev#13868 behavior: sub-second no-cluster errors on a brand-new route whose backend translates a beat later; up to one heartbeat interval of no-cluster errors on routes affected by an internal pipeline hole. What can no longer happen: permanent stranding, frozen gateways, endpoints failing to propagate, ADS suppression, or a bad resource clobbering a good snapshot. Signed-off-by: David L. Chandler <[email protected]>
direct retry, and atomic publish bookkeeping A review of the per-client publication rework surfaced ten defects, three of them critical liveness/safety gaps in the new policy. This commit fixes all of them, reshaping the publication seam rather than patching each in place. The headline mechanisms: Pending-retry path (closes the unobservable-deadline gap) The warm-up deadline was only evaluated inside syncXds, whose sole caller is the KRT batch subscriber. A withheld publication produces no KRT event, and KRT hash-suppresses unchanged recomputes, so on quiet inputs NOTHING ever re-invoked syncXds after the budget expired: a cold client facing a persistently missing cluster received no xDS config indefinitely — the same stranded-client class this work exists to eliminate. Withheld snapshots are now retained on the reconciler and re-attempted directly by the heartbeat loop each tick (syncXdsAttempt with a sequence guard, so a retry that raced a newer event-driven publish is a no-op). Budget expiry is therefore observed within one heartbeat tick, by construction, with no dependency on the event stream. Incomplete-inputs episodes (generalizes the warm-up gate) The warm-up gate only protected never-published clients. A published client whose rebuild was transiently incomplete — an empty per-client cluster row, a missing CLA — would publish anyway: a state-of-the-world CDS with live clusters removed (whole-gateway connection drain) or a synthesized empty ClusterLoadAssignment (silently wiping a healthy backend's endpoints). Deferral is now keyed to incompleteness, not cold-start: any publish attempt with missing referenced clusters or synthesized CLAs opens a clock-bounded episode (15s) during which updates are withheld and Envoy keeps its last coherent config. Past the budget the snapshot publishes marked DEGRADED — logged, counted (new metric below), and keeping the client stuck so the heartbeat heals it — and the still-open episode lets further incomplete updates flow immediately, so endpoint delivery never freezes (the kgateway-dev#13868 failure mode). Only a clean publish ends the episode. Reconciler rewrite: one clientState, atomic commits The four parallel maps (published/deferred/orphanedSince/warmupStart) collapse into one map[string]*clientState, fixing three defects: - deferred-map leak: a client whose only attempts failed hard validation had neither a published nor a warm-up entry, so its deferred mark survived departure forever, pinning hasStuckClients() true and converting the demand-driven heartbeat back into an unconditional full-fleet recompute every tick for the life of the process. The sweep now drops ALL state for departed clients. - reclaim/reconnect TOCTOU: SetSnapshot ran outside the reconciler lock, so a reclaim pass using a stale connected-set view could clear a snapshot a reconnecting client had just published, leaving the cache empty while the books said published. commitPublish now does SetSnapshot atomically with its bookkeeping under one lock and zeroes the orphan clock, totally ordering publish against reclaim. - rollout noise: every genuine departure was recorded as a defer, holding the stuck signal true for the full reclaim grace (~2.5min) during routine pod churn. Snapshot Delete events are now classified by liveness: still-connected means transform defer (stuck, heal); departed only starts the reclaim clock. Additionally, connected-but-never-published clients count as stuck only when their role actually has a per-gateway snapshot to publish, so an orphaned Envoy (Gateway deleted, unknown role) can no longer keep the heartbeat recomputing forever. Heartbeat covers the whole per-client path MarkDependant was wired into only the two leaf transforms; a dropped recompute on any edge BETWEEN collections (leaf -> clusterSnapshot -> snapshotPerClient) could not be healed, because an unchanged leaf recompute is hash-suppressed and propagates nothing. All per-client transforms now mark the trigger, so a tick re-runs the full path; unchanged outputs still suppress, so a healthy fleet sees no churn. Build-time EDS reconciliation (subsumes publish-time synthesis) The S2 subset filter and the publish-time CLA synthesis derived the same required-set twice with two different version recipes — a split invariant begging to drift. They merge into one build-time step: published CLAs are exactly those required by the snapshot's CDS, with required-but-missing ones synthesized empty and their names reported on the wrapper (so publication can log/count/mark degraded — an empty CLA actively drops endpoints and must never be silent). The version recipe reuses the endpoint pipeline's precomputed per-CLA hashes instead of re-marshaling every CLA per transform run. Always-emit per-client rows; defer on underived inputs The per-client cluster transform now always emits a row once derived, even with zero clusters (a legitimate result for a redirect-only gateway), so a nil FetchOne downstream unambiguously means "not derived yet" and snapshotPerClient defers (clusters_not_ready) instead of substituting an empty cluster set — the substitution was the source of the transient whole-gateway CDS wipe. Publication metadata instead of publish-time proto walks GatewayXdsResources.ReferencedClusters (computed once per gateway) is carried through XdsSnapWrapper, so the missing-cluster check no longer re-walks LDS/RDS with protoreflect + Any unmarshalling per client per event. The secret-reference scan is scoped to the resource types that can carry SDS references (Cluster/Listener/Route), PGV validation uses fail-fast Validate() instead of ValidateAll(), and the duplicated recursive protoreflect walkers (cluster + secret) collapse into one shared visitor-based walker (proto_walk.go). Metrics: withheld vs degraded, by construction perclient_defers_total now strictly means "update withheld" (reasons are constants: incomplete_inputs, invalid_snapshot, clusters_not_ready, endpoints_not_ready); the old missing_clusters_published reason — which incremented a "withheld" counter on a path that published — is replaced by a new counter, perclient_degraded_publishes_total{reason} with missing_clusters / synthesized_load_assignments. Recoveries count only CLEAN publishes after a withhold or degraded publish, so a degraded publish can no longer be booked as a heal. The resource-sync completion metric fires only when a snapshot actually reached the cache. Tests - New regression tests for every defect above: budget expiry driven through the pending-retry path with no KRT event; the sequence guard; deferred-state sweep for never-published departed clients; publish resets the orphan clock; defer-vs-departure Delete classification; unpublishable-role exclusion; degraded-is-not-recovery; episode stays open after a degraded publish and a fresh episode defers for a published client; synthesis reporting and version transitions. - TestPerClientHeartbeat_NoChurnWhenStable now synchronizes on the recompute counter instead of a sleep, so it can no longer pass vacuously before the recompute lands. - The design doc (devel/architecture/perclient-xds-publication.md) is updated to match the implemented policy; the claims the review falsified ("bounded by construction", one-tick heal) are now true by mechanism. Verification: go vet clean; package unit tests green under -race (90 tests); make analyze clean; e2e-tagged build clean; PerClientXDS e2e suite green on k3d against the rebuilt image, with controller logs showing the incomplete-inputs gate engaging on real route-creation transients and resolving within the budget (zero degraded publishes). Signed-off-by: David L. Chandler <[email protected]>
Backport the minimal first-publish bound without weakening the make-before-break behavior fixed by kgateway-dev#13868. The bounded publish path is only safe for Envoys that appear to be cold. A controller-local xDS cache miss is not enough to prove that: after reconnect or controller restart, an Envoy can be new to this controller process while still serving production traffic with config accepted from a previous stream. Track Envoys that report a prior accepted xDS version on their initial request and treat them as warm for deferred snapshot handling. Such clients do not get a deferred/partial snapshot after the first-publish budget expires. They keep their last-good config until kgateway can build a coherent snapshot, which still publishes immediately. Behavior after this change: - coherent snapshots always publish immediately - clients with a local cached snapshot never receive deferred snapshots - clients with no local snapshot but prior xDS version never receive deferred snapshots - clients with no local snapshot and no prior xDS version get the bounded first-publish liveness path This intentionally chooses safety over liveness for clients that may already be serving traffic. Without a real KRT quiescence signal, publishing deferred config to those clients can regress kgateway-dev#13868 by replacing working config with an incomplete SotW snapshot. The tradeoff is that a warm reconnect can remain stale if its per-client inputs never become coherent. Make that stale-risk visible by adding kgateway_xds_snapshot_perclient_deferred_withheld_total, labeled by gateway, namespace, defer reason, and warm reason. The warm reason distinguishes local cache warmth from the prior-xDS-version reconnect case. Per-client withheld age is tracked in memory and emitted through throttled logs rather than as a Prometheus label, avoiding high-cardinality UCC metrics. Tests cover: - prior-version clients do not receive deferred snapshots after the budget - prior-version clients still receive coherent snapshots immediately - cold clients still receive bounded deferred first publish - warm local-cache clients keep the kgateway-dev#13868 behavior - prior-version state is cleared when the UCC disconnects - withheld-deferred metrics include both warm reasons Signed-off-by: David L. Chandler <[email protected]>
# Description Issues kgateway-dev#13868 and kgateway-dev#14184 both lived in *event interleavings* — orderings of cluster/endpoint/route updates that no hand-written test covered. This adds a randomized, model-based regression test that drives the real `snapshotPerClient` KRT transform through long random sequences of input events and, after every event, asserts xDS snapshot invariants on whatever it actually publishes — using the `xdscheck` invariant checker as the oracle. What it exercises and checks (`pkg/kgateway/proxy_syncer/perclient_property_test.go`): - **Events:** cluster add/remove (CDS), endpoints absent/empty/ready (EDS), and route retargets across both direct and weighted route styles. The generator models production coupling (a cluster never appears in CDS without at least an empty CLA) while still allowing the stale CLA-without-cluster case that `filterEndpointResourcesForClusters` must drop. - **Safety invariants** on every published snapshot: dependency closure (route -> cluster, EDS cluster -> CLA), no orphan ClusterLoadAssignments, and EDS version discipline (content change implies version change). - **Progress:** after driving to a coherent, stable input, a coherent snapshot must be published. - **Reproducibility:** each iteration is a fixed seed; on failure it prints the seed and the full event journal. Counts are env-tunable (`XDS_PROP_ITERS` / `XDS_PROP_STEPS` / `XDS_PROP_CLUSTERS` / `XDS_PROP_SEED`); the default is modest (~14s) and the test is skipped under `-short`. It runs clean across deep local sweeps (100x40 and 50x80x5 interleavings). Also adds `pkg/kgateway/proxy_syncer/xds_consistency_probe_test.go`: a focused probe that pins go-control-plane's `ads=true` behavior on an EDS-inconsistent snapshot (a CDS cluster with no CLA). This both documents a finding (below) and guards against a silent behavior change on future go-control-plane bumps. # Change Type /kind cleanup # Changelog ```release-note NONE ``` # Additional Notes The property test surfaced one real-but-benign behavior, verified rather than assumed: - `snapshotPerClient` can publish an EDS-**inconsistent** snapshot — an EDS cluster in the per-client CDS that no route references may be emitted with no ClusterLoadAssignment. The readiness gate only covers route-*referenced* clusters, `MakeConsistent()` is commented out in `kube_gw_translator_syncer.go`, and the `SetSnapshot` error is discarded. - The probe confirms this is tolerated today: under `ads=true`, go-control-plane accepts the snapshot and still serves the healthy clusters' EDS; the orphan cluster simply warms alone with no traffic or stream impact. So this is a latent robustness gap, not a live bug, and is left for a follow-up (re-enable consistency / synthesize empty CLAs / handle the discarded `SetSnapshot` error). One implementation note for reviewers: the liveness check re-drives a terminal gateway-snapshot event in a loop. `krt.NewStaticCollection` does not reliably propagate a terminal recompute through the derived-collection graph after a burst of updates the way real informer-backed KRT does; this was verified (via input-state dumps) to be a test-harness quiescence artifact, not a gate defect. The KRT-delivery liveness concern itself is covered separately. Signed-off-by: David L. Chandler <[email protected]>
kgateway translates backends into Envoy clusters and endpoints **per connected proxy**: the per-client pipeline runs the full backend translation for every `(backend, connected client)` pair. On large fleets this fan-out is `O(backends × clients)`, and it is the work that the readiness gate added in kgateway-dev#13868 can amplify into a stalled per-client snapshot (see kgateway-dev#14184). The `DISABLE_POD_LOCALITY_XDS` environment variable already existed to drop pod locality from a proxy's client identity, collapsing all replicas of a gateway to a single identity. But it only shrank the *inputs* to the existing per-client graph — the same fan-out machinery and per-client snapshot path still ran. This PR makes `DISABLE_POD_LOCALITY_XDS=true` select a genuinely separate, simpler xDS code path. When pod locality is disabled, per-client translation cannot diverge between clients (DestinationRule selection has no labels/namespace to match on, and endpoint prioritization is uniform), so each backend's cluster and endpoints are translated **once** (client-agnostic) and a snapshot is assembled **per gateway role** instead of per connected client. A proxy connecting or disconnecting no longer triggers any backend re-translation, and there are no per-`(client, backend)` snapshot rows. The locality-aware path (the default) is unchanged. The coherence guarantee from kgateway-dev#13868 is preserved on the new path, simply applied per gateway role: a snapshot is withheld (the proxy keeps its last-good config) until every referenced cluster and its EDS resources are present. Refs kgateway-dev#14184. # Change Type /kind feature # Changelog ```release-note When `DISABLE_POD_LOCALITY_XDS=true`, kgateway now uses a locality-agnostic xDS code path that translates each backend once and builds a single xDS snapshot per gateway role, rather than once per connected proxy. This removes the per-proxy translation fan-out for deployments that do not need locality-aware routing (e.g. single-zone clusters with no DestinationRules or locality-based traffic distribution), reducing control-plane work and convergence time. The locality-aware path remains the default and is unchanged. ``` # Additional Notes ## When can I set `DISABLE_POD_LOCALITY_XDS=true`? This flag is **controller-wide** — it changes the xDS path for *every* gateway the controller manages — so it is only safe when **no proxy needs to route differently based on its own location**. Pod locality affects the generated config in these places: 1. **Endpoint prioritization** (EDS, or the inline load assignment of DNS/STATIC clusters), when a Service requests locality-based traffic distribution — `spec.trafficDistribution: PreferClose`, or the `networking.istio.io/traffic-distribution` annotation set to `PreferClose` / `PreferSameZone` / `PreferSameNode` / `PreferNetwork`. 2. **DestinationRule matching and `localityLbSetting`** (CDS + EDS), only when Istio integration is enabled (`KGW_ENABLE_ISTIO_INTEGRATION=true`; off by default). 3. **ServiceEntry / WorkloadEntry endpoint locality** (Istio integration only), which is declared in the resource spec rather than derived from cluster nodes. If none of these is in effect, every proxy already receives identical config, and the locality-agnostic path is behaviorally equivalent — just cheaper. **Safe to enable when all of the following hold:** - The cluster is effectively single-zone for routing — a backend's ready endpoints do not span more than one of the locality dimensions (`region` / `zone` / `subzone`) that any locality config keys on. The simplest sufficient condition for Kubernetes-Service backends: **all nodes are in one region and zone**. - **No DestinationRule** sets `localityLbSetting` (or Istio integration is disabled, in which case DestinationRules are not consumed at all). - **No Service** uses locality-based traffic distribution that would actually split endpoints into priority tiers. - If Istio integration is enabled, **no ServiceEntry or WorkloadEntry declares endpoints with explicit `locality`** that you route across with `PreferClose` or a DestinationRule `localityLbSetting`. ServiceEntry/WorkloadEntry endpoints take their locality from the resource spec, not from cluster nodes — including DNS-resolution ServiceEntries, whose endpoints are inline in the cluster rather than served via EDS — so the "all nodes in one zone" shortcut does not cover them. **Quick rules of thumb:** - **Istio integration off** → only Kubernetes Services matter, so the node-topology check is sufficient: a single-zone cluster with no `trafficDistribution`/`PreferClose` is always safe. - **Istio integration on** → you must also account for spec-declared endpoint locality (ServiceEntry/WorkloadEntry) and DestinationRule `localityLbSetting`, not just node topology. > Note on the single-zone subtlety: even with `PreferClose`/`PreferSameZone` set, if all endpoints share one zone the prioritizer produces a single tier and all traffic flows there regardless — so disabling locality changes nothing observable. It only matters once endpoints actually span the keyed dimension. **Do not set it when:** - Backends have endpoints across **multiple zones (or regions)** and you use any locality-based traffic distribution or DestinationRule `localityLbSetting` — disabling locality would drop zone preference and send cross-zone traffic that would otherwise stay local. - You use **`PreferSameNode`** (keys on node/subzone) on a multi-node cluster. - You use **ServiceEntry/WorkloadEntry with explicit endpoint `locality`** (any resolution, including `DNS`) together with locality-aware routing — those endpoints span localities by declaration regardless of your Kubernetes node topology. - You plan to **expand to multiple zones** — remove the flag *before* doing so to restore locality-aware routing. When any of those apply, leave the flag unset; those deployments stay on the default per-client path. ## Implementation notes - Both paths feed the same xDS publish path, snapshot metrics, and Backend status reporting. - **Correctness** rests on there being no per-client translation divergence when locality is disabled. The in-tree per-client hooks (DestinationRule matching and endpoint locality prioritization, including the inline load assignments of DNS/STATIC ServiceEntry clusters) all key on labels/namespace/locality, which are empty on this path; a controller log notes when Istio integration is enabled alongside the flag. - **Testing:** unit tests for the per-role assembler (per-role coherence-gate deferral preserved; one snapshot per role with shared backend clusters) and a new e2e suite (`PodLocalityXDS`) that sets the flag on the controller and asserts routing plus runtime route convergence. Signed-off-by: David L. Chandler <[email protected]>
# Description A connected Envoy proxy could keep routing to endpoints that no longer exist, surfacing as upstream connection failures on a subset of a gateway's replicas. Both root causes live in `snapshotPerClient` (`pkg/kgateway/proxy_syncer/perclient.go`), the per-client xDS snapshot builder added in kgateway-dev#13868. 1. **Stale ClusterLoadAssignments suppressed EDS responses.** When a cluster left CDS, its CLA could remain in the per-client EDS set. go-control-plane refuses to answer a named (SotW ADS) EDS request when the snapshot holds resources outside the requested set, so the affected proxy stopped receiving EDS updates and sat on its last, stale endpoints. The snapshot now filters EDS down to exactly the EDS clusters present in the same CDS snapshot (honoring `service_name`) and versions that filtered set by content hash, so the EDS version changes whenever the set changes. 2. **Empty ClusterLoadAssignments satisfied the readiness gate.** The publication gate deferred until a referenced EDS cluster's CLA was present, but treated a CLA with zero usable endpoints as ready. Publishing CDS/RDS/LDS while EDS was empty let Envoy finish warming the cluster onto no hosts and drop traffic that was healthy before the update. The gate now requires at least one usable (non-`UNHEALTHY`) endpoint before publishing, preserving make-before-break. ## Tests - `perclient_test.go`: EDS-vs-CDS filtering (stale removal, `service_name`, STATIC exclusion, version-change-on-set-change), the usable-endpoint gate, make-before-break route and weighted-route updates, last-good cache retention during deferral, and named-watch respondability after cluster removal (exercised through go-control-plane's `SnapshotCache`). - `backendtls_snapshot_test.go`: supplies endpoints so existing snapshots still publish under the stricter gate. - New `test/e2e/features/xds_warming` suite: make-before-break, weighted-split, and cold-start scenarios against real Envoy with delayed endpoints. # Change Type /kind fix # Changelog ```release-note Fixed an issue where a connected proxy could keep sending traffic to endpoints that no longer exist. Per-client xDS snapshots now keep endpoint (EDS) resources aligned with the clusters in the same snapshot and wait for at least one usable endpoint before shifting a route onto a cluster. ``` # Additional Notes The usable-endpoint readiness check gates the whole per-client snapshot: if any referenced EDS cluster has no usable endpoint, publication is deferred and Envoy keeps its last good config. This is the intended behavior for make-before-break, but it has a known edge: a referenced backend that is deliberately at zero endpoints (for example, scaled to zero) will hold the snapshot rather than publishing the now-empty assignment, so Envoy retains the previous endpoints for that cluster. Moving the readiness decision to per-cluster granularity -- publish a previously-active cluster's empty assignment while holding only the route flip onto a not-yet-ready new cluster -- is a sensible follow-up. Signed-off-by: David L. Chandler <[email protected]>
…into the carry-forward bound A controller-local cache miss isn't proof an Envoy is cold: after a reconnect or controller restart it can serve prior-accepted config while new to this process. Track prior xDS version; at budget expiry, withhold from such clients instead of publishing an incomplete SotW snapshot (preserves kgateway-dev#13868). Adds deferred_withheld_total{warm_reason}. Signed-off-by: David L. Chandler <[email protected]>
…into the carry-forward bound A controller-local cache miss isn't proof an Envoy is cold: after a reconnect or controller restart it can serve prior-accepted config while new to this process. Track prior xDS version; at budget expiry, withhold from such clients instead of publishing an incomplete SotW snapshot (preserves kgateway-dev#13868). Adds deferred_withheld_total{warm_reason}. Signed-off-by: David L. Chandler <[email protected]>
Review of the base+overlay refactor surfaced three correctness gaps plus cleanups; this commit addresses them. Correctness: - Strict mode no longer validates CLA-less inline-CLA bases. TranslateBackendBase ran the Envoy validator before the per-client CLA exists, so a valid ServiceEntry DNS_ROUND_ROBIN backend (logical-DNS semantics: exactly one endpoint required) failed validation and blackholed for every client. Validation is deferred to ApplyPerClient, which always materializes and validates the complete per-client cluster for these backends. The new BaseCluster.NeedsInlineCLA() is the shared gate. - FetchClustersForClient withholds a CLA-less inline-CLA base when the UCC has no delta for it yet. base and deltas are separate KRT collections with no cross-collection atomicity, so a new UCC could be published a host-less STRICT_DNS/STATIC cluster (503s) during the propagation beat before its delta lands. Withholding lets the snapshot's referenced-cluster deferral hold the publish, matching the pre-split behavior; safe because ApplyPerClient always emits a delta for these clusters. - baseClusterVersion folds the attached-policy hash (backendEndpointVersionHash) into inline-CLA base versions. EndpointInputs carries backend.AttachedPolicies, which PerClientProcessEndpoints hooks consume, and KRT keeps the OLD stored object when Equals returns true — a policy-only change (cluster proto and endpoints unchanged) would otherwise pin per-client CLAs to the stale attachment forever. Mirrors the EDS path in newFinalBackendEndpoints. - ApplyPerClient applies overlays in GroupKind order when more than one matches. Gathering ranges over the ContributedPolicies map, so overlapping overlays could otherwise produce byte-unstable protos that flip ClusterVersion, churn xDS, and defeat interning. Cleanups: - Document the EndpointPlugin hash contract: the returned hash now keys CLA interning across UCCs, so it must capture every per-UCC effect of the plugin, not just serve as a change-detection hint. - Drop the baseByBackend index and BackendResourceName field; base rows are keyed by cluster name, which translation always derives from backendObj.ClusterName() (blackhole included). - Delete the now-unused CombinedTranslator.TranslateEndpoints. - Fold combineEndpointHashes into combineEndpointHash (same package, identical fold). - Update AGENTS.md for the PerClientProcessBackend -> PerClientClusterOverlay rename and the new hook contracts. Known residual (tracked in a follow-up issue with a draft below): a newly connected ambient client can see the un-overlaid base for one KRT beat before its waypoint delta arrives; closing that needs a per-UCC computed marker in the deltas pipeline. --- **Title:** Transient waypoint-redirect gap for newly connected ambient clients under the base/delta per-client cluster split ## Summary Since the base+overlay refactor of per-client cluster translation (#<PR>), a **newly connected** ambient client can be served the un-overlaid base cluster for an `ingress-use-waypoint` Service for one KRT propagation beat, before its per-client waypoint delta is computed. During that beat the gateway routes directly to the Service instead of through the waypoint. This was a known, accepted residual when the PR landed. This issue records the mechanism, why it was safe to ship unresolved, and the fix design. ## Mechanism The refactor split per-client clusters into two KRT collections: - `BaseEnvoyClusters` — one UCC-invariant row per backend, present regardless of which clients are connected. - `PerClientEnvoyClusterDeltas` — sparse rows, emitted only for `(ucc, backend)` pairs where a `PerClientClusterOverlay` applies (waypoint, destrule) or an inline CLA is required. `FetchClustersForClient` merges the two: delta wins, base otherwise. The two collections have no cross-collection atomicity, so when a UCC key appears for the first time, the bases are already visible while the deltas for that UCC do not exist yet. **In the sparse design, "no delta" is the correct steady state for the dominant case** — so the merge cannot distinguish "no overlay applies" from "overlay not yet computed", and it must fall back to the base. Pre-refactor, a single many-collection emitted all of a client's rows atomically per backend; a brand-new client had *no* rows at all until translation completed, and `snapshotPerClient`'s referenced-cluster deferral held the entire publish. The refactor narrowed that all-or-nothing deferral to the cases where the base is affirmatively unpublishable. ## Why this was safe to land 1. **The deterministic half was fixed in the PR.** For inline-CLA backends (ServiceEntry STATIC/DNS), the base has no `LoadAssignment` and `ApplyPerClient` is *guaranteed* to materialize a delta for every UCC. Because the delta always arrives, `FetchClustersForClient` withholds those bases until it does, restoring the pre-split deferral exactly where falling back would have served Envoy a host-less cluster (hard 503s). The residual window applies only to overlays that may legitimately never fire — where withholding is impossible without new machinery. 2. **The exposure is one propagation beat, on a rare trigger.** The window opens only when a *new UCC key* appears (first proxy of a given role/namespace/labels/locality shape connects — not on ordinary reconnects, which reuse the existing UCC and its already-computed deltas). The UCC add itself triggers the deltas recompute, so the gap closes after one KRT pass, typically milliseconds, and is self-healing with no operator action. 3. **The failure mode is degraded, not broken, and the class is pre-existing.** During the beat, traffic takes the same direct-to-Service path every non-ambient client uses; nothing blackholes. An equivalent eventual-consistency window has always existed in this pipeline: labeling a Service `istio.io/ingress-use-waypoint=true` at runtime left already-connected clients routing directly until recomputation, both before and after the refactor. Ambient waypoint attachment is eventually consistent end to end (Istio itself programs it asynchronously), and where destination-side enforcement requires waypoint traversal, direct traffic in the beat is denied rather than silently bypassing L7 policy. The destrule instance of the same window (a new client briefly missing outlier-detection/keepalive overlays) is strictly cosmetic. 4. **The naive fix is more dangerous than the bug.** Withholding *all* bases from a new client until "deltas have been computed" reintroduces the full-publish deferral class that has previously stranded proxies (see kgateway-dev#13868 and the kgateway-dev#14352 analysis: withhold guards that can never be proven satisfied freeze clients indefinitely, and any deferral budget must stay below the startup probe). Trading a milliseconds-scale redirect gap on first connect for a risk of indefinitely blank CDS on every new client shape is a bad trade, and shipping the refactor's O(N*M)->O(N*K) win did not need to wait on getting that design right. ## Proposed fix Make "computed" observable without sacrificing sparseness: emit **one per-UCC marker row** (not per pair) from the deltas pipeline — e.g. a tiny sibling collection keyed by UCC that records the deltas builder has run for that client, emitted even when the result is zero deltas. `FetchClustersForClient` (or `snapshotPerClient`) defers a UCC's *first* publish until its marker exists, with a hard time bound well below the startup probe so a wedged marker degrades to today's behavior instead of stranding the client. Requirements for whoever picks this up: - The marker must be emitted on the zero-delta path — that is the entire point, and the reason counting deltas cannot work. - Bounded deferral, fail-open to the current behavior on timeout. - A regression test that a brand-new ambient UCC never observes the un-overlaid cluster for a waypoint-attached Service (this test is also unwritable today, which is part of why the fix needs the marker). ## References - #<PR> — base+overlay split; includes the inline-CLA withholding fix and `BaseCluster.NeedsInlineCLA()` - `pkg/kgateway/proxy_syncer/backends.go` — merge and withholding logic - kgateway-dev#13868, kgateway-dev#14352 — prior art on why unbounded publish deferral is worse than transient staleness Signed-off-by: David L. Chandler <[email protected]>
# Description A connected Envoy proxy could keep routing to endpoints that no longer exist, surfacing as upstream connection failures on a subset of a gateway's replicas. Both root causes live in `snapshotPerClient` (`pkg/kgateway/proxy_syncer/perclient.go`), the per-client xDS snapshot builder added in kgateway-dev#13868. 1. **Stale ClusterLoadAssignments suppressed EDS responses.** When a cluster left CDS, its CLA could remain in the per-client EDS set. go-control-plane refuses to answer a named (SotW ADS) EDS request when the snapshot holds resources outside the requested set, so the affected proxy stopped receiving EDS updates and sat on its last, stale endpoints. The snapshot now filters EDS down to exactly the EDS clusters present in the same CDS snapshot (honoring `service_name`) and versions that filtered set by content hash, so the EDS version changes whenever the set changes. 2. **Empty ClusterLoadAssignments satisfied the readiness gate.** The publication gate deferred until a referenced EDS cluster's CLA was present, but treated a CLA with zero usable endpoints as ready. Publishing CDS/RDS/LDS while EDS was empty let Envoy finish warming the cluster onto no hosts and drop traffic that was healthy before the update. The gate now requires at least one usable (non-`UNHEALTHY`) endpoint before publishing, preserving make-before-break. ## Tests - `perclient_test.go`: EDS-vs-CDS filtering (stale removal, `service_name`, STATIC exclusion, version-change-on-set-change), the usable-endpoint gate, make-before-break route and weighted-route updates, last-good cache retention during deferral, and named-watch respondability after cluster removal (exercised through go-control-plane's `SnapshotCache`). - `backendtls_snapshot_test.go`: supplies endpoints so existing snapshots still publish under the stricter gate. - New `test/e2e/features/xds_warming` suite: make-before-break, weighted-split, and cold-start scenarios against real Envoy with delayed endpoints. # Change Type /kind fix # Changelog ```release-note Fixed an issue where a connected proxy could keep sending traffic to endpoints that no longer exist. Per-client xDS snapshots now keep endpoint (EDS) resources aligned with the clusters in the same snapshot and wait for at least one usable endpoint before shifting a route onto a cluster. ``` # Additional Notes The usable-endpoint readiness check gates the whole per-client snapshot: if any referenced EDS cluster has no usable endpoint, publication is deferred and Envoy keeps its last good config. This is the intended behavior for make-before-break, but it has a known edge: a referenced backend that is deliberately at zero endpoints (for example, scaled to zero) will hold the snapshot rather than publishing the now-empty assignment, so Envoy retains the previous endpoints for that cluster. Moving the readiness decision to per-cluster granularity -- publish a previously-active cluster's empty assignment while holding only the route flip onto a not-yet-ready new cluster -- is a sensible follow-up. Signed-off-by: David L. Chandler <[email protected]>
…ing the snapshot The heart of the kgateway-dev#14184 fix. The wait-for-consistency gate added by kgateway-dev#13868 withholds a client's ENTIRE snapshot while any referenced cluster is not ready. That all-or-nothing rule turns one unready backend into a whole-gateway freeze: no route, listener, or endpoint update reaches the proxy while anything in its input set is incoherent, and nothing bounds how long that lasts. The gate also counted a present-but-empty ClusterLoadAssignment as ready-blocking, so a Service that legitimately scaled to zero could pin a proxy to endpoints that no longer exist — the original upstream-connect-failure report in kgateway-dev#14184. This commit replaces the whole-snapshot gate with per-cluster publish-time resolution: 1. snapshotPerClient no longer withholds the row. It builds the snapshot unconditionally and annotates the wrapper with what is not ready: missingReferenced (referenced clusters absent from CDS), unusableReferenced (referenced EDS clusters whose CLA has no usable endpoint), and erroredClusters. A wrapper with gaps is marked deferred. Because deferral no longer emits a KRT Delete, a Delete on the per-client collection now reliably means the client departed — a clean seam for follow-up snapshot reclaim (kgateway-dev#14307). 2. syncXds resolves a deferred wrapper per cluster against the currently-published snapshot (resolveDeferredPerCluster): - a previously-published cluster that vanished from the build is carried forward with its CLA — make-before-break, scoped to exactly the missing references instead of freezing everything; - a previously-referenced cluster whose endpoints scaled to zero publishes its truth, the empty CLA, so Envoy stops dialing dead endpoints (fixes the stale-endpoints half of kgateway-dev#14184); - only a route flip onto a NEWLY-referenced not-yet-ready cluster is held: routes/listeners/secrets stay at their published versions while the new cluster's CDS/EDS goes out and warms in the background; the flip publishes once the cluster is usable. The hold gives destination changes a reference-ahead shape — the CDS carrying a new cluster reaches Envoy in an earlier snapshot than the RDS that names it — which no delivery-order mechanism provides by itself. - errored clusters are fail-closed: never resurrected from the published snapshot, even while a flip is held for an unrelated cluster. Serving a cluster's stale pre-error config would bypass the very policy whose failure errored it; Gateway API conformance (BackendTLSPolicyInvalidCACertificateRef) requires the 5xx. The fail-open alternative was rejected in PR kgateway-dev#13976. - a never-published client is withheld until its referenced clusters are ready (cold-start make-before-break, unchanged from the whole-snapshot gate). A follow-up commit bounds this wait. Carried resource sets get a distinct '-carry-' version suffix (deterministic in the carried set), so clients holding a merged snapshot are guaranteed an update push when the real resources arrive; the merge is copy-on-write so the krt-cached wrapper's maps are never mutated. Tests: per-cluster resolution suite (scale-to-zero truth publish, flip-hold isolation, errored fail-closed during a held flip); the whole-snapshot defer tests are rewritten to the built-but-deferred model (wrapper present with recorded gaps, resolution observable through a real go-control-plane snapshot cache via registerSyncXds); snapshot coherence is asserted with go-control-plane's Consistent() plus a route/listener->CDS reference-closure check. The DeleteDuringPartialUpdateRetainsServedCache test is removed: it pinned the old Delete-on-deferral semantics that no longer exist. Co-Authored-By: Claude Fable 5 <[email protected]> Signed-off-by: David L. Chandler <[email protected]>
…t crash-loop The per-cluster resolution in the previous commit withholds a deferred snapshot from a NEVER-published client — there is no last-good config to hold or carry, and publishing routes to unready clusters would 503 them. Correct for the brief converging case, but unbounded: a permanently unready reference (a backend that is down, a dangling backendRef, a misconfiguration) leaves a freshly scheduled gateway pod with no listeners at all. The pod never reports Ready and crash-loops, and every restart reconnects as a brand-new xDS client that re-triggers the full per-client fan-out — the failure feeds itself. This is the 'gateways are unable to spawn' symptom in kgateway-dev#14184. This commit bounds that wait: - firstPublishGate (firstpublish.go) arms one timer per never-published client with a deferred snapshot. If the client's inputs cohere within the budget, the coherent publish wins and discards the timer. At expiry the LATEST deferred snapshot is published as-is — it is always internally consistent (missing EDS references carry synthesized empty assignments), so the pod binds its listeners and becomes Ready while routes to still-unready clusters 503 until they cohere. A bounded degradation strictly beats an unbounded outage for a client with no config at all. - Warm reconnects are excluded from the bound. A controller-local cache miss does not prove a client is cold: after a reconnect or controller restart, an Envoy can be new to this controller process while still serving production traffic with config accepted on a previous stream. The xDS callbacks now record whether a client's initial request on its current stream carried a prior accepted version/nonce (krtcollections.XDSClientState); such clients stay withheld at budget expiry — publishing an incomplete state-of-the-world snapshot would replace the config they are serving, regressing kgateway-dev#13868's make-before-break. The state is per-stream and clears on disconnect. - All snapshot-cache publications go through the gate's lock, so an expiring budget timer can never overwrite a newer coherent snapshot, and a coherent publish always cancels the pending bounded one. Client departure (a wrapper Delete, which after the previous commit reliably means departure or a transient transform-input blip) cancels the pending publish; a live client re-arms it with its next deferred wrapper. - The budget is operator-tunable: KGW_PER_CLIENT_PUBLISH_BUDGET (Settings.PerClientPublishBudget, default 15s). 0 disables the bound entirely — never-published clients wait for coherence with no deadline — as the conservative opt-out. Wiring: NewUniquelyConnectedClients additionally returns the XDSClientState reader; setup threads it through StartConfig into NewProxySyncer/NewProxyTranslator. Tests: cold client publishes at budget (latest deferred wins); prior-xDS-version client stays withheld at expiry but receives coherent snapshots immediately; a coherent publish supersedes the pending bounded one and the canceled timer never overwrites it; departure cancels; budget=0 disables the bound. Co-Authored-By: Claude Fable 5 <[email protected]> Signed-off-by: David L. Chandler <[email protected]>
This reverts commit 91ce39f. Signed-off-by: David L. Chandler <[email protected]>
This reverts commit 91ce39f. Signed-off-by: David L. Chandler <[email protected]>
This reverts commit 91ce39f. Signed-off-by: David L. Chandler <[email protected]>
…way-dev#13958) in favor of a first-connect grace period (kgateway-dev#14380) Signed-off-by: David L. Chandler <[email protected]>
Description
This fixes a reconnect-time xDS race that can surface during kgateway
controller restarts.
When a new uniquely connected client is created,
snapshotPerClientcanrun in parallel with per-client backend cluster translation. The existing
guard only waited for the per-client cluster collection to become
non-
nil, which still allowed a partial CDS snapshot to be published forthat client. In that state, routes and listeners could already reference
backend clusters not yet present in the snapshot, and Envoy would return
500responses with theNCflag until the missing clusters arrived.This change adds a readiness gate that defers publishing a per-client
snapshot until all clusters referenced as dataplane routing targets
(RouteAction / TcpProxy cluster or weighted-cluster specifiers) are
present or explicitly errored in the merged cluster set.
Change Type
/kind fix
Changelog
Additional Notes
Added regression coverage for the partial-snapshot case:
Verified with:
go test ./pkg/kgateway/proxy_syncerNote that the zero-downtime e2e test changes you can see in some commits were not sufficient to reproduce the issue. I think they're reasonable to include, but they didn't help and they might add to flakiness without sufficient benefit, so I removed those e2e changes. I needed hack/race-repro from #13937 to reproduce e2e, and even then I only achieved it on v2.2.x and I can't point to any commit on main but not on v2.2.x that would explain that difficulty. I'd rather not commit that hack script since it will just bit-rot.
This hopes to resolve the issue reported on Slack: https://cloud-native.slack.com/archives/C080D3PJMS4/p1776139323819119