fix(tcp): add SNI-based filter chain matching for TLS passthrough empty routes#8521
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 canceled.
|
eb41a3a to
3a7c801
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8521 +/- ##
==========================================
+ Coverage 74.16% 74.20% +0.04%
==========================================
Files 242 242
Lines 37603 37614 +11
==========================================
+ Hits 27889 27913 +24
+ Misses 7773 7763 -10
+ Partials 1941 1938 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
371856b to
3a68ca2
Compare
|
hey @OliverBailey can you sign your commit, and force push to fix DCO ? |
| // Only add the filter chain once per Envoy listener; multiple IR listeners may share | ||
| // the same address/port and map to the same xDS listener. | ||
| filterChainExists := false | ||
| for _, fc := range xdsListener.FilterChains { |
There was a problem hiding this comment.
do we need the for loop ?
could we get by with a bool like isEmptyClusterSet and for the first pass we could create it, but for the second pass, we'd avoid it ?
There was a problem hiding this comment.
I like this approach. Only track the xDS listeners that already have the empty filter chain. It may need to be a map to allow for listeners on different ports.
emptyFilterChainAdded := make(map[string]bool)
There was a problem hiding this comment.
Replaced the for-loop with emptyFilterChainAdded := make(map[string]bool) keyed by xDS listener name, per stekole's suggestion below. No loop needed — we just check/set the map entry once per xDS listener.
| } | ||
| // Set Passthrough flag for TLS passthrough mode | ||
| if listener.Protocol == gwapiv1.TLSProtocolType && listener.TLS != nil && listener.TLS.Mode != nil { | ||
| irListener.Passthrough = *listener.TLS.Mode == gwapiv1.TLSModePassthrough |
There was a problem hiding this comment.
This sets the passthrough but does anything read it?
There was a problem hiding this comment.
Nothing read it; you're right.
After the translator was simplified to use a single EmptyCluster for all empty-route listeners regardless of mode, the Passthrough field became dead code.
Removed the field from TCPListener, the setter in listener.go, and all testdata references.
TLS passthrough routing is unaffected: SNI matching still works via TLSInspectorConfig.SNIs on each TCPRoute.
acf0dd8 to
12302aa
Compare
…ty routes This fix addresses the issue where multiple TLS passthrough listeners on the same port without routes would generate duplicate empty filter chains without unique matching rules, causing Envoy to reject the configuration with: "filter chain 'EmptyCluster' has the same matching rules defined as 'EmptyCluster'" Changes: - Add Hostnames field to TCPListener IR to capture SNI values from Gateway API - Populate hostnames from Gateway API listener.Hostname for TLS protocol - Detect TLS passthrough by presence of hostnames (SNI matching required) - Create unique EmptyCluster per TLS passthrough listener with SNI-based matching - Add TLSInspectorConfig with SNIs to enable filterChainMatch generation - Add test coverage for multiple TLS passthrough listeners without routes This ensures each TLS passthrough listener gets a unique filter chain with serverNames matching, allowing Envoy to properly route based on SNI. Builds on work from envoyproxy#7912 by @Aditya7880900936 which addressed the unique cluster naming issue. This PR extends it to properly implement SNI-based filter chain matching as noted in the review comments by @sudiptob2. Fixes envoyproxy#7912 Signed-off-by: Oliver Bailey <github@obailey.co.uk>
- Fix indentation in tcp-multiple-tls-passthrough-no-routes.yaml - List items should not be indented per project yamllint config Signed-off-by: Oliver Bailey <github@obailey.co.uk>
Address review feedback by decoupling TLS passthrough detection from hostname presence. Using len(hostnames) > 0 as a signal for passthrough was fragile and could misidentify: - TLS termination listeners with hostnames as passthrough (false positive) - Catch-all passthrough listeners without hostnames (false negative) Changes: - Add explicit Passthrough bool field to TCPListener IR - Set Passthrough from Gateway API listener.TLS.Mode == Passthrough - Update translator to check tcpListener.Passthrough instead of len(hostnames) - Update test data to include passthrough field in test fixtures This makes the code more maintainable and self-documenting, with clear separation between passthrough mode and SNI hostname configuration. Signed-off-by: Oliver Bailey <github@obailey.co.uk>
… single EmptyCluster - Remove Hostnames []string field from TCPListener IR; SNI is already captured in TLSInspectorConfig.SNIs on each TCPRoute (per arkodg) - Replace Hostnames-based passthrough detection with the existing explicit Passthrough bool field throughout - Empty-route listeners (passthrough and non-passthrough) now share a single EmptyCluster instead of creating per-listener duplicates (per arkodg) - Update all affected testdata to reflect the above changes Signed-off-by: Oliver Bailey <github@obailey.co.uk>
… emptyFilterChainAdded map - Remove Passthrough bool from TCPListener IR - it was set in listener.go but never read by the translator (per stekole) - Remove corresponding setter in listener.go and passthrough: true from testdata output files - Replace per-listener for-loop filter chain existence check with a map[string]bool keyed by xDS listener name (per arkodg + stekole) - TLS passthrough routing is unaffected: SNI matching still works via TLSInspectorConfig.SNIs on each TCPRoute Signed-off-by: Oliver Bailey <github@obailey.co.uk>
12302aa to
ff7d3e3
Compare
Apologies, completely forgot for that specific commit. Done, and re-signed the commits and pushed back up. |
|
/retest |
Description
Fixes the bug where multiple TCP/TLS listeners on the same port without any attached Routes would cause Envoy to reject the entire listener configuration with:
The root cause: when multiple IR listeners share the same address:port (mapping to a single xDS listener), the old code called
addXdsTCPFilterChainonce per IR listener, adding identicalEmptyClusterfilter chains to the same xDS listener — which Envoy considers invalid duplicate matchers.Changes
internal/xds/translator/translator.goemptyFilterChainAdded := make(map[string]bool)keyed by xDS listener nameEmptyClusterfilter chain is now added at most once per xDS listener, regardless of how many IR listeners share the same portEmptyClusterroute object is also created once and reused (sharedEmptyTCPRoute)Test coverage
tcp-multiple-tls-passthrough-no-routestest: 3 IR listeners on the same port with no routes → singleEmptyClustercluster, single filter chain on the xDS listenerAfter this fix
Closes
Closes #7866
Checklist
go test ./internal/gatewayapi/... ./internal/xds/translator/... ./internal/ir/...)