Conversation
0a8cd0d to
870a097
Compare
internal/xds/translator/listener.go
Outdated
There was a problem hiding this comment.
From the UDP Stats docs:
The UDP proxy filter emits both its own downstream statistics as well as many of the cluster upstream statistics where applicable. The downstream statistics are rooted at udp.<stat_prefix>.
With that said, statPrefix will cause stats to be generated with a prefix of udp.udp. Maybe use service as the value? Thoughts @arkodg @lizan?
There was a problem hiding this comment.
good point @danehans , I think we followed a similar pattern for other protocols too, will raise a issue to track and fix for all
There was a problem hiding this comment.
Can you create an issue and xref in your comment?
There was a problem hiding this comment.
the xds code is complicated as is, prefer if we moved all these validations checks to higher levels such as IR or Gateway - API
There was a problem hiding this comment.
yeah I prefer that because that layer has the ability to signal back to the user using the Status and it performs a lot of the port conflict checks already today
gateway/internal/gatewayapi/translator.go
Line 226 in 9eecce6
There was a problem hiding this comment.
Is the 1:1 b/c UDPRoute does not support any of the UDP matcher rules, e.g. source IP?
There was a problem hiding this comment.
@danehans Envoy can route UDP diagrams to different upstream clusters according to their soure IP, but the Gateway API doesn't support that. The same as TCPRoute. So I think the current implementaion is fine, if Gateway API add support to UDP/TCP source IP matching, we can revisit here and support it.
Envoy UDP route: https://www.envoyproxy.io/docs/envoy/latest/configuration/listeners/udp_filters/udp_proxy#routing
Gateway UDPRoute API: https://gateway-api.sigs.k8s.io/v1alpha2/references/spec/#gateway.networking.k8s.io/v1alpha2.UDPRouteSpec
There was a problem hiding this comment.
why is the find needed here, is there any case where we want to support udp and another protocol on the same addr:port ?
There was a problem hiding this comment.
No, but we need to throw an error in case there are two listeners on the same UDP ports.
There was a problem hiding this comment.
i think this case should be handled in the Gateway API translator itself, which has enough info to detect this as well as signal back to the user using the UDPRoute Status sub resource, prefer if this code is simpler
There was a problem hiding this comment.
u also enhance this PR with ac
sure
There was a problem hiding this comment.
i think this case should be handled in the Gateway API translator itself, which has enough info to detect this as well as signal back to the user using the
UDPRouteStatussub resource, prefer if this code is simpler
OK, we can remove the check here if Gateway API translator can detect it in the first place.
|
hey @zhaohuabing can you also enhance this PR with access logs for UDP Proxy similar to whats done in #704 |
Signed-off-by: zhaohuabing <zhaohuabing@gmail.com>
Signed-off-by: zhaohuabing <zhaohuabing@gmail.com>
870a097 to
23ebf6f
Compare
Codecov Report
@@ Coverage Diff @@
## main #709 +/- ##
==========================================
+ Coverage 63.35% 63.54% +0.19%
==========================================
Files 47 47
Lines 5834 5920 +86
==========================================
+ Hits 3696 3762 +66
- Misses 1907 1921 +14
- Partials 231 237 +6
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
arkodg
left a comment
There was a problem hiding this comment.
LGTM, looks good, thanks !
xds translator for udp route
This PR is a part of #641
Signed-off-by: zhaohuabing zhaohuabing@gmail.com