Conversation
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks for working on this. Starting out with some API comments.
/wait
| @@ -51,6 +52,9 @@ message UdpProxyConfig { | |||
|
|
|||
| // The upstream cluster to connect to. | |||
| string cluster = 2 [(validate.rules).string = {min_len: 1}]; | |||
There was a problem hiding this comment.
Can you deprecate cluster since I think any direct cluster route can be expressed via the new route table?
| // [#protodoc-title: UDP proxy route configuration] | ||
| // UDP proxy :ref:`configuration overview <config_udp_listener_filters_udp_proxy>`. | ||
|
|
||
| message RouteConfiguration { |
There was a problem hiding this comment.
I think for this we should go straight to using the new style matchers as there is no reason here not to support sub-linear matching for CIDR ranges, etc. Can you take a look at #17633 and replicate that here? Obviously some matchers won't be supported and we will have to deal with that, but many would be. cc @snowp who can help with questions.
There was a problem hiding this comment.
I have noticed these components, but they focus on HTTP and may be not suitable for UDP. In fact, network filters have their own route matchers like RocketMQ route configuration for RocketMQ proxy and Thrift route configuration for Thrift proxy. UDP proxy is a UDP listener filter which is similar to network filters, so I am not sure if we can use these new style matchers.
There was a problem hiding this comment.
The new matching API was developed in order to avoid each protocol from having to define their own matching APIs - the API itself should not be HTTP specific, only loosely coupled with HTTP through extensions and the context in which they are used. Happy to chat more about how we can make use of the new API in this context.
There was a problem hiding this comment.
That sounds great! A unified matcher API will bring convenience to development. In UDP, we mainly consider the source prefix and source port.
There was a problem hiding this comment.
Yup as Snow said, the new API was designed for this purpose, so let's please switch to it, even if we only support a small subset of matchers.
/wait
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
github-actions
bot
added
the
stale
|
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
4 participants
Commit Message: udp: add router for UDP proxy
Additional Description: match source prefix and route UDP packets to different clusters
Risk Level: Low
Testing: Unit and integration
Docs Changes: N/A
Release Notes: WIP
Platform Specific Features: N/A
Fixes #17652
Deprecated: the envoy::extensions::filters::udp::udp_proxy::v3::UdpProxyConfig::cluster is deprecated in favor of envoy::extensions::filters::udp::udp_proxy::v3::UdpProxyConfig::route_config