Repository navigation
Conversation
Signed-off-by: zirain <zirain2009@gmail.com>
…different TLS When two backendRefs in a route rule point to the same IP or UDS endpoint with different TLS settings, they became localities of one EDS cluster. EDS merges hosts with the same address, so Envoy kept only the first endpoint and its transport socket match, and the other TLS settings (e.g. a rotated client certificate) were never used. Add RouteDestination.HasSharedEndpointsWithDifferentTLS and include it in NeedsClusterPerSetting, so such rules get a cluster per setting and are split with weighted clusters. Assisted-by: Claude Opus 5.5 Signed-off-by: YuMin Kim <yuminkim.dev@gmail.com>
Signed-off-by: YuMin Kim <yuminkim.dev@gmail.com> Assisted-by: Claude Opus 5.5
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
When two backendRefs in one route rule point to the same IP (or UDS) endpoint but carry different TLS settings, for example two Backends with different
clientCertificateRefduring an mTLS client certificate rotation, the xDS translator put them in one EDS cluster as two localities, each with its owntransport_socket_matchesentry. EDS merges hosts with the same address, so Envoy kept only the first endpoint: the second TLS setting was never used and the traffic split between the backendRefs was lost, with no error or warning.This PR adds
RouteDestination.HasSharedEndpointsWithDifferentTLSand adds it toNeedsClusterPerSetting, as suggested in the issue. Such rules now get a cluster per setting, and traffic is split withweighted_clusters. The check:STRICT_DNSclusters don't merge hosts (the/fqdnrule in the fixture keeps its single cluster),host:port, or by path for UDS,TLSUpstreamConfigwithreflect.DeepEqual, so settings without TLS and settings with TLS on the same endpoint also count as different.Rules whose backendRefs share an endpoint with identical TLS settings keep the current single cluster. Giving every shared IP endpoint a cluster per setting, regardless of TLS, is the more conservative alternative described in the issue. I left it out to keep this change small.
This builds on the fixture from #10460. Its commit is cherry-picked unchanged as the first commit, so this PR can be rebased cleanly once #10460 merges. The second commit regenerates the xDS golden output for
backend-mtls-rotation(rule/0 now producesrule/0/backend/0andrule/0/backend/1clusters plus aweighted_clustersroute action) and replaces the fixture's "Known issue" comments with a description of the fixed behavior. The gatewayapi golden output is unchanged, because the split happens in the xDS translator.Tests:
go test ./internal/ir/... ./internal/gatewayapi/... ./internal/xds/... ./internal/cmd/egctl/...passTestRouteDestination_NeedsClusterPerSetting: a shared IP with different TLS, a shared IP with TLS and without TLS, a shared IP with the same TLS, different IPs, and a shared FQDNgo test ./internal/xds/translator ./internal/gatewayapi --override-testdata=truegolangci-lint run/fmt --diffon./internal/ir/...are cleanRelease note:
release-notes/current/bug_fixes/10461-shared-endpoint-tls-cluster-per-setting.md. It is named after the issue number. I can rename it to the PR number if preferred.AI assistance: this change was prepared with Claude Opus 5.5 (Claude Code).
Which issue(s) this PR fixes:
Fixes #10461
PR Checklist
git commit -s). See DCO: Sign your work.make generate gen-check,make lint, and the unit-test/coverage build pass. (Flaky e2e failures are not considered breakages, butgen-check,lint, and coverage MUST pass.)release-notes/current/<section>/<pr-number>-<slug>.md(seerelease-notes/current/README.mdfor sections and naming). N/A if this PR does not contain non-trivial changes.release-notes/current/breaking_changes/.🤖 Generated with Claude Code
https://claude.ai/code/session_012bPQTxDmyCAwQaT3PtwxBr