feat(translator): Support extension server hooks for TCP and UDP listeners in addition to HTTP listeners. #3522
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3522 +/- ##
==========================================
+ Coverage 68.17% 68.22% +0.05%
==========================================
Files 168 168
Lines 20496 20560 +64
==========================================
+ Hits 13973 14027 +54
- Misses 5513 5521 +8
- Partials 1010 1012 +2 ☔ View full report in Codecov by Sentry. |
|
/retest |
1 similar comment
|
/retest |
| errs = errors.Join(errs, err) | ||
| } | ||
|
|
||
| if err := processTCPListenerXdsTranslation(tCtx, ir.TCP, ir.AccessLog, ir.Metrics); err != nil { | ||
| if err := processUDPListenerXdsTranslation(tCtx, xdsIR.UDP, xdsIR.AccessLog, xdsIR.Metrics, listenerExtPolicies); err != nil { |
There was a problem hiding this comment.
looks like we are passing listenerExtPolicies to processUDPListenerXdsTranslation so that we can append per listener type's ExtensionRefs to it. Can we instead move that out of the pure xds translations into an extension specific function like notifyExtensionServerAboutListeners ?
There was a problem hiding this comment.
The translation functions (processUDPListenerXdsTranslation, processTCPListenerXdsTranslation and processHTTPListenerXdsTranslation) map the relevant ExtensionRefs from the IR listener representation to the xDS listener representation. The listenerExtPolicies is keyed by the xDS listener name, not the IR listener name, where it collects the extension policies from the IR.
Filling up the listenerExtPolicies needs to be done in the xDS translation function - it's not possible to have a notifyExtensionServerAboutListeners that doesn't redo part of the translation code.
| if !found { | ||
| pols = nil | ||
| } else { | ||
| slices.SortFunc(pols, func(l, r *ir.UnstructuredRef) int { |
There was a problem hiding this comment.
this looks new, we are trying to ensure we pass in fixed order ?
There was a problem hiding this comment.
It's not about ensuring that we pass the policies in a fixed order. It's so that we don't duplicate them. The code sorts the list of extension policies and then compacts the list with slices.CompactFunc to remove duplicates.
See the extensionpolicy-tcp-udp-http unit test for an example of how this can happen. The TL;DR is that when there are multiple listeners defined in the same gateway resource where the port is the same but one is for HTTP and the other is TCP, then in the translation to xDS the same xDS listener is reused because it's the same port.
This code section is about making sure that if the same extension server policy is attached to different IR level constructs that map to the same xDS level construct, then the extension server won't receive the same policy twice.
There was a problem hiding this comment.
ah go it, thoughts on adding a findIRListenesrByHostPort method similar to
gateway/internal/xds/translator/translator.go
Line 598 in e866ec1
this way we wont need to embed any extension policy code into the listener translator functions, mantain any state in memory, or perform any of these SortFuncs here (which may be hard to comprehend for a new reader)
There was a problem hiding this comment.
Changed the logic to work like this. Not sure it's better, computation-wise.
There was a problem hiding this comment.
thanks, agree, its not better computation wise, but should improve readability and maintenance
|
@envoyproxy/gateway-maintainers PTAL |
liorokman
force-pushed
the
support-tcp-udp-listener-hooks
branch
from
a213c4b to
060ace7
Compare
|
/retest |
|
hey @liorokman can you rebase, the |
HTTP listeners. Signed-off-by: Lior Okman <lior.okman@sap.com>
Signed-off-by: Lior Okman <lior.okman@sap.com>
translation logic. Signed-off-by: Lior Okman <lior.okman@sap.com>
Signed-off-by: Lior Okman <lior.okman@sap.com>
liorokman
force-pushed
the
support-tcp-udp-listener-hooks
branch
from
0b72ccb to
1551343
Compare
Signed-off-by: Lior Okman <lior.okman@sap.com>
Signed-off-by: Lior Okman <lior.okman@sap.com>
|
/retest |
|
/retest |
What this PR does / why we need it:
This PR calls the extension server "Listener" hook for TCP and UDP listeners. The hook is called only once for each XDS listener.
I kept the hook name as it was before (
PostHTTPListenerModify) for backwards compatibility reasons.Which issue(s) this PR fixes:
Fixes #2673