Repository navigation
Conversation
…e-port peer An SDS server for sds.example.com can return a certificate that also covers foo.example.com. A client can then coalesce foo.example.com requests onto an HTTP/2 connection to the SDS listener, so a same-port peer must not stay on HTTP/2 just because its own SANs do not overlap the SDS listener hostname. This test fails until the translator treats every same-port peer of an SDS-backed listener as overlapping. Assisted-by: Claude Opus 5.5 Signed-off-by: YuMin Kim <yuminkim.dev@gmail.com>
…tener When an SDS-backed listener had an explicit hostname, only same-port peers whose known certificate SANs overlapped that hostname were downgraded to HTTP/1.1. The SDS certificate is opaque and may cover more names than the listener hostname, so a client could coalesce requests for a peer onto an HTTP/2 connection to the SDS listener. Treat every valid same-port peer of an SDS-backed listener as overlapping, unless ALPN is configured with ClientTrafficPolicy. The sds-listener golden output now reports the condition and tlsOverlaps on inline-same-port, and the TLS termination docs describe the new default. Fixes envoyproxy#9697 Assisted-by: Claude Opus 5.5 Signed-off-by: YuMin Kim <yuminkim.dev@gmail.com>
Assisted-by: Claude Opus 5.5 Signed-off-by: YuMin Kim <yuminkim.dev@gmail.com>
✅ 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 an SDS-backed HTTPS listener has an explicit hostname,
checkOverlappingCertificatesonly downgraded same-port peers whose known certificate SANs overlapped that hostname. The SDS certificate is opaque to Envoy Gateway, and nothing requires it to cover only the listener hostname: an SDS server forsds.example.comcan return a certificate that also coversfoo.example.com. A client can then open an HTTP/2 connection to the SDS listener and coalescefoo.example.comrequests onto it, while thefoo.example.comlistener stays on HTTP/2 because its SANs don't matchsds.example.com.This PR follows the approach in the issue: when two or more valid HTTPS listeners share a port and at least one of them is SDS-backed, every listener on that port gets
tlsOverlapsand the existingOverlappingTLSConfig=True/SDSCertificateOpaquecondition, unless ALPN is configured explicitly with ClientTrafficPolicy. The SAN-vs-hostname loop is removed, since it no longer changes the result. Listeners on other ports, invalid peers, and a standalone SDS listener are unaffected (the existing test cases for these still pass).Behavior change to note: with an SDS listener on a shared port, inline-certificate peers with unrelated hostnames now default to HTTP/1.1 where they used to keep HTTP/2.
Commits:
test(gatewayapi): updates theSDS certificate leaves distinct-hostname peers on HTTP/2case (nowSDS certificate disables HTTP/2 on distinct-hostname peers) to expect the condition andtlsOverlapson all three listeners. It fails on main.fix(gatewayapi): the fix, the regeneratedsds-listener.out.yamlgolden file (inline-same-portnow gets the condition andtlsOverlaps: true), the TLS termination task docs, and a release note.Tests:
go test ./internal/gatewayapi/... ./internal/xds/... ./internal/cmd/egctl/...passgo test ./internal/gatewayapi -run TestTranslate --override-testdata=truegolangci-lint runon./internal/gatewayapi/is cleanRelease note:
release-notes/current/bug_fixes/9697-sds-same-port-peers-http1.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 #9697
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.)site/content/en/latest/tasks/security/tls-termination.md.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
🤖 Generated with Claude Code
https://claude.ai/code/session_012bPQTxDmyCAwQaT3PtwxBr