fix(translator): avoid duplicate filter chain for TLS listener without routes#9342
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8408e88276
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| TLS: &ir.TLS{ | ||
| TLSInspectorConfig: &ir.TLSInspectorConfig{ | ||
| SNIs: tcpListener.Hostnames, | ||
| }, |
There was a problem hiding this comment.
When a hostname-only, no-route TLS listener is processed before another TLS listener on the same address/port that enables JA3/JA4 fingerprints, this new synthetic SNI route causes addXdsTCPFilterChain to install tls_inspector for SNI only. addXdsTLSInspectorFilter returns immediately once an inspector exists, so the later listener's fingerprint flags are never enabled and configured %TLS_JA*_FINGERPRINT% values stay empty. Before this branch, a no-route listener without fingerprints did not install the inspector, so the later listener could enable it; please merge/update existing inspector options when adding this placeholder chain.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Is that the desired behavior? As tls_inspector is a listener filter we can't have per filter chain scope? Maybe it's correct as it is to just install the first. @zirain
…t routes
When a TLS protocol listener with a specific hostname has no routes attached,
the translator generated an EmptyCluster placeholder filter chain with no
FilterChainMatch. If an HTTPS listener with wildcard hostname ('*') shared the
same port, both filter chains ended up with an empty matcher, causing Envoy to
reject the xDS update with:
filter chain 'EmptyCluster' has the same matching rules defined as
'<name>/https'. duplicate matcher is: {}
Fix: store the Gateway listener hostname on the TCPListener IR (for TLS
protocol listeners only, since TCP ignores hostnames) and use it as an SNI-
based FilterChainMatch on the empty placeholder filter chain, making it unique
among other wildcard filter chains on the same xDS listener.
Fixes envoyproxy#9341
Signed-off-by: Florian Wiegand <36593900+wiegandf@users.noreply.github.com>
wiegandf
force-pushed
the
fix/tls-listener-no-routes-duplicate-filter-chain
branch
from
8408e88 to
fe869ee
Compare
2 participants
What type of PR is this?
fix(translator)
What this PR does / why we need it:
When a
TLSprotocol listener with a specific hostname has no routes attached, the translator generated anEmptyClusterplaceholder filter chain with noFilterChainMatch. If anHTTPSlistener with wildcard hostname (*) shared the same port, both filter chains ended up with an empty matcher, causing Envoy to reject the xDS update with:This blocks all subsequent xDS updates to that listener until a route is manually added.
The fix stores the Gateway listener hostname on the
TCPListenerIR (forTLSprotocol listeners only —TCPignores hostnames per the Gateway API spec) and uses it as an SNI-basedFilterChainMatchon the empty placeholder filter chain, making it unique from any wildcard filter chains on the same xDS listener.Which issue(s) this PR fixes:
Fixes #9341
Release Notes: Yes