proxy-syncer bug fix, but for a bug that never actually happens#13958
Conversation
Code cleanup only. Signed-off-by: David L. Chandler <[email protected]>
gateway-bot
added
kind/cleanup
There was a problem hiding this comment.
Pull request overview
Cleanup to make snapshotPerClient defensively publish snapshots even when the per-client cluster collection is empty/nil (a path that’s effectively unreachable in normal k8s clusters, but can exist in narrow edge cases), and to pin that behavior with a regression test.
Changes:
- Treat a nil per-client cluster snapshot entry as “zero clusters” (instead of “not ready yet”) so publishing can proceed when there are no referenced backend clusters.
- Add a unit test that constructs redirect-only routes (no cluster references) and asserts a snapshot is still published with an empty CDS.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/kgateway/proxy_syncer/perclient.go | Adjusts readiness gating logic so nil cluster inputs can be treated as empty clusters in edge cases. |
| pkg/kgateway/proxy_syncer/perclient_test.go | Adds a test covering the redirect-only / zero-cluster publish path. |
| if clustersForUcc == nil { | ||
| clustersForUcc = &clustersWithErrors{ | ||
| clusters: envoycache.Resources{ | ||
| Items: map[string]envoycachetypes.ResourceWithTTL{}, |
There was a problem hiding this comment.
When clustersForUcc is nil, the fallback initializes envoycache.Resources with an empty Items map but leaves Version empty. SnapshotCache.SetSnapshot typically validates snapshot consistency and may reject resources with missing versions, which would make this new “publish with zero clusters” path fail at runtime (and the error is currently ignored). Prefer constructing an explicit empty resources set with a non-empty version (e.g., via envoycache.NewResourcesWithTTL("0", nil) or by setting Version explicitly) so the resulting snapshot is always valid.
| Items: map[string]envoycachetypes.ResourceWithTTL{}, | |
| Version: "0", | |
| Items: map[string]envoycachetypes.ResourceWithTTL{}, |
| "a snapshot must publish even when there are zero per-client clusters and zero referenced clusters") | ||
|
|
||
| snap := snapshots.List()[0].snap | ||
| g.Expect(snap.Resources[envoycachetypes.Cluster].Items).To(gomega.BeEmpty()) |
There was a problem hiding this comment.
This new test exercises the “nil clusters => publish empty CDS” path, but it doesn’t assert that the generated Cluster resources have a non-empty Version. Adding an assertion for snap.Resources[envoycachetypes.Cluster].Version would help catch regressions where the snapshot becomes inconsistent and SetSnapshot rejects it.
| g.Expect(snap.Resources[envoycachetypes.Cluster].Items).To(gomega.BeEmpty()) | |
| g.Expect(snap.Resources[envoycachetypes.Cluster].Items).To(gomega.BeEmpty()) | |
| g.Expect(snap.Resources[envoycachetypes.Cluster].Version).NotTo(gomega.BeEmpty()) |
…eway-dev#13958) Signed-off-by: David L. Chandler <[email protected]>
…ns (kgateway-dev#13958)" This reverts commit 3c926bb. Signed-off-by: David L. Chandler <[email protected]>
…ns (kgateway-dev#13958)" This reverts commit 3c926bb. Signed-off-by: David L. Chandler <[email protected]>
…ns (kgateway-dev#13958)" This reverts commit 3c926bb. 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
Code cleanup only re: a copilot review comment on a backport of #13868 : #13947 (comment)
This bug cannot happen because there's always at least one cluster on k8s, but I prefer the "fixed" code.
Change Type
/kind cleanup
Changelog
Additional Notes