Skip to content

fix: remedies a reconnect-time xDS race#13868

Merged
chandler-solo merged 15 commits into
kgateway-dev:mainfrom
chandler-solo:chandler/fixcontrollerrestartperclientxds
Apr 28, 2026
Merged

fix: remedies a reconnect-time xDS race#13868
chandler-solo merged 15 commits into
kgateway-dev:mainfrom
chandler-solo:chandler/fixcontrollerrestartperclientxds

Conversation

@chandler-solo

@chandler-solo chandler-solo commented Apr 16, 2026

Copy link
Copy Markdown
Contributor

Description

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 not yet present in the snapshot, and Envoy would return
500 responses with the NC flag 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

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.

Additional Notes

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:

go test ./pkg/kgateway/proxy_syncer

Note 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

@gateway-bot gateway-bot added kind/fix Categorizes issue or PR as related to a bug. release-note labels Apr 16, 2026
@chandler-solo
chandler-solo marked this pull request as ready for review April 16, 2026 07:22
Copilot AI review requested due to automatic review settings April 16, 2026 07:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread pkg/kgateway/proxy_syncer/perclient.go Outdated
@chandler-solo chandler-solo added the work in progress Indicates that a PR should not merge because it is a work in progress label Apr 16, 2026
@chandler-solo chandler-solo changed the title fix: remedies a reconnect-time xDS race [WIP] fix: remedies a reconnect-time xDS race Apr 16, 2026
@chandler-solo chandler-solo removed the work in progress Indicates that a PR should not merge because it is a work in progress label Apr 16, 2026
@chandler-solo chandler-solo changed the title [WIP] fix: remedies a reconnect-time xDS race fix: remedies a reconnect-time xDS race Apr 16, 2026
@chandler-solo
chandler-solo requested a review from Copilot April 16, 2026 07:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Comment thread pkg/kgateway/proxy_syncer/perclient.go Outdated
Comment thread pkg/kgateway/proxy_syncer/perclient_test.go

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Comment on lines +166 to +177
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

Copilot AI Apr 18, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment thread pkg/kgateway/proxy_syncer/perclient.go Outdated
@chandler-solo
chandler-solo requested a review from Copilot April 21, 2026 19:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Comment on lines +167 to +179
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
}

Copilot AI Apr 21, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines +280 to +285
func findMissingReferencedClusters(
routes envoycache.Resources,
listeners envoycache.Resources,
clusters map[string]envoycachetypes.ResourceWithTTL,
erroredClusters []string,
) []string {

Copilot AI Apr 21, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment thread pkg/kgateway/proxy_syncer/perclient.go Outdated
Comment on lines +291 to +293
referencedClusters := make(map[string]struct{})
collectResourceClusterReferences(routes, referencedClusters)
collectResourceClusterReferences(listeners, referencedClusters)

Copilot AI Apr 21, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines +407 to +414
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
}

Copilot AI Apr 21, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

g.Consistently(func() int {
return len(snapshots.List())
}, 200*time.Millisecond, 20*time.Millisecond).Should(gomega.Equal(0))

Copilot AI Apr 21, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
}, 200*time.Millisecond, 20*time.Millisecond).Should(gomega.Equal(0))
}, time.Second, 20*time.Millisecond).Should(gomega.Equal(0))

Copilot uses AI. Check for mistakes.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Comment on lines +52 to +64
// 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",
}

Copilot AI Apr 23, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment thread pkg/kgateway/proxy_syncer/perclient.go Outdated
@davidjumani davidjumani added this to the v2.3.0 milestone Apr 28, 2026
@chandler-solo
chandler-solo force-pushed the chandler/fixcontrollerrestartperclientxds branch from 46e492b to 9a1be35 Compare April 28, 2026 15:36
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]>
chandler-solo added a commit to chandler-solo/kgateway that referenced this pull request Jun 10, 2026
…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]>
chandler-solo added a commit to chandler-solo/kgateway that referenced this pull request Jun 10, 2026
…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]>
chandler-solo added a commit to chandler-solo/kgateway that referenced this pull request Jun 10, 2026
…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]>
chandler-solo added a commit to chandler-solo/kgateway that referenced this pull request Jun 10, 2026
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]>
chandler-solo added a commit to chandler-solo/kgateway that referenced this pull request Jun 17, 2026
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]>
chandler-solo added a commit to chandler-solo/kgateway that referenced this pull request Jun 18, 2026
# 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]>
chandler-solo added a commit to chandler-solo/kgateway that referenced this pull request Jun 18, 2026
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]>
chandler-solo added a commit to chandler-solo/kgateway that referenced this pull request Jun 23, 2026
# 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]>
chandler-solo added a commit to chandler-solo/kgateway that referenced this pull request Jun 23, 2026
…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]>
chandler-solo added a commit to chandler-solo/kgateway that referenced this pull request Jul 1, 2026
…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]>
chandler-solo added a commit to chandler-solo/kgateway that referenced this pull request Jul 7, 2026
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]>
chandler-solo added a commit to chandler-solo/kgateway that referenced this pull request Jul 8, 2026
# 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]>
chandler-solo added a commit to chandler-solo/kgateway that referenced this pull request Jul 8, 2026
…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]>
chandler-solo added a commit to chandler-solo/kgateway that referenced this pull request Jul 8, 2026
…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]>
chandler-solo added a commit to chandler-solo/kgateway that referenced this pull request Jul 10, 2026
chandler-solo added a commit to chandler-solo/kgateway that referenced this pull request Jul 10, 2026
chandler-solo added a commit to chandler-solo/kgateway that referenced this pull request Jul 10, 2026
chandler-solo added a commit to chandler-solo/kgateway that referenced this pull request Jul 10, 2026
…way-dev#13958) in favor of a first-connect grace period (kgateway-dev#14380)

Signed-off-by: David L. Chandler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/fix Categorizes issue or PR as related to a bug. release-note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants