udp: add router for UDP proxy by zhxie · Pull Request #18791 · envoyproxy/envoy

zhxie · 2021-10-27T03:21:16Z

Signed-off-by: Xie Zhihao zhihao.xie@intel.com

Commit Message: udp: add router for UDP proxy
Additional Description: match and route UDP datagrams to different clusters
Risk Level: Medium
Testing: Unit and integration
Docs Changes: UDP proxy example configuration
Release Notes: Added
Platform Specific Features: N/A
Fixes #17652

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

repokitteh-read-only · 2021-10-27T03:21:19Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #18791 was opened by zhxie.

see: more, trace.

repokitteh-read-only · 2021-10-27T03:21:24Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #18791 was opened by zhxie.

see: more, trace.

zhxie · 2021-10-27T03:22:54Z

Though this PR is working in progress, docs changed and testings have not been completed, I would like to ask @mattklein123 for a pre-review.

This patch is a update to #17864 which uses the generic matching API #17633 you metioned, but since the prefix/custom generic matching API is not completed, This patch does not support prefix-range-like matching compared with #17864.

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

repokitteh-read-only · 2021-10-27T06:52:54Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #18791 was synchronize by zhxie.

see: more, trace.

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

mattklein123

Thanks very nice. Flushing out some small comments.

/wait

mattklein123 · 2022-03-30T17:34:16Z

docs/root/version_history/current.rst

  useful in support systems such as CI, CD, etc. The
  :ref:`schema validator check tool <install_tools_schema_validator_check_tool>` has been added
  to the tools image.
+* udp_proxy: added :ref:`matcher <envoy_v3_api_field_extensions.filters.udp.udp_proxy.v3.UdpProxyConfig.matcher>` to support matching and routing to different clusters.


Please add a note in deprecated below for the deprecated field.

mattklein123 · 2022-03-30T17:35:33Z

source/extensions/filters/udp/udp_proxy/router/router.h

+        Network::Address::InstanceConstSharedPtr source_address) const PURE;
+
+  /**
+   * Returns all cluster names in the router. The UDP proxy filter requires every cluster names for


Suggested change

* Returns all cluster names in the router. The UDP proxy filter requires every cluster names for

* Returns all cluster names in the router. The UDP proxy filter requires every cluster name for

Thanks, changed.

mattklein123 · 2022-03-30T17:37:59Z

source/extensions/filters/udp/udp_proxy/router/router_impl.cc

+RouterImpl::RouterImpl(const envoy::extensions::filters::udp::udp_proxy::v3::UdpProxyConfig& config,
+                       Server::Configuration::ServerFactoryContext& factory_context) {
+  if (config.has_cluster()) {
+    cluster_ = config.cluster();


I think it would be a bit cleaner to just synthesize matcher config for this case. Then you can use unified code for the rest of it? (Basically it makes the deprecated path cheaper to maintain and understand as the only special case is here.)

I am sorry but I cannot catch your idea here. Do you mention we can move cluster_ out of Router?

Ok, I catch your thought finally, I will submit the commit soon.

mattklein123 · 2022-03-30T17:40:01Z

source/extensions/filters/udp/udp_proxy/router/router_impl.cc

+    return cluster_.value();
+  }
+
+  if (source_address->ip()) {


Technically isn't it possible to send UDP/datagrams over a pipe? Or do we block this in config? Either way it seems like this should be handled elsewhere as a config load issue vs. on every route request?

Yes, it is redundant. I will remove it, thanks.

mattklein123 · 2022-03-30T17:41:36Z

source/extensions/filters/udp/udp_proxy/udp_proxy_filter.cc

-    return;
-  }
-
  ENVOY_LOG(debug, "udp proxy: detaching from cluster {}", cluster);


nit: this log is now misleading. I would see if the cluster is used before logging and erasing? If you store an iterator the perf delta will be negligible.

Thanks, fixed.

mattklein123 · 2022-03-30T17:42:53Z

source/extensions/filters/udp/udp_proxy/router/router.h

+  route(Network::Address::InstanceConstSharedPtr destination_address,
+        Network::Address::InstanceConstSharedPtr source_address) const PURE;


nit: I would pass by const ref to avoid shared_ptr inc/dec

Ok, I will make them const.
By the way, InstanceConstSharedPtr is a shared_ptr<const Instance>. I think it is not necessary to change to a const ref since the const Instance cannot be changed.

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

mattklein123

Thanks LGTM other than small comment and fixing CI.

/wait

mattklein123 · 2022-03-31T14:54:26Z

source/extensions/filters/udp/udp_proxy/router/router.h

  virtual const std::string
-  route(Network::Address::InstanceConstSharedPtr destination_address,
-        Network::Address::InstanceConstSharedPtr source_address) const PURE;
+  route(const Network::Address::InstanceConstSharedPtr destination_address,


Sorry I meant const Network::Address::Instance&

envoy/source/extensions/filters/udp/udp_proxy/udp_proxy_filter.cc

Line 50 in eede13d

const std::string& route = config_->route(data.addresses_.local_, data.addresses_.peer_);

In the caller method, we will get a InstanceConstSharedPtr. If we change the Router::route's signature to const Network::Address::Instance&, we have to change the code in the caller to

const std::string& route = config_->route(*data.addresses_.local_, *data.addresses_.peer_);

Do you think it is acceptable?

Fixed, I have updated all methods in the caller chain.

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

zhxie · 2022-04-01T09:06:54Z

It is strange that the patch does not touch any codes in source/common/http but the CI complains low coverage on it.

/retest

repokitteh-read-only · 2022-04-01T09:07:00Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18791 (comment) was created by @zhxie.

see: more, trace.

zhxie · 2022-04-02T15:06:00Z

Flaky macOS test.

/retest

repokitteh-read-only · 2022-04-02T15:06:04Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18791 (comment) was created by @zhxie.

see: more, trace.

mattklein123

Thank you. Great work!

This reverts commit 90a1d4b. Signed-off-by: Ryan Northey <ryan@synca.io>

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

zhxie added 21 commits August 26, 2021 11:27

udp: add route proto

4cfa9c5

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

udp: add router

9891c45

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

udp: route data to clusters

367db00

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

udp: fix format

3bd3085

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

udp: minor type change

4c1b126

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

udp: clean code

3f762f7

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

test: add test for routing in UDP proxy

95e1094

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

test: add test for route matching of UDP proxy

b592612

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

test: fix format

804aedd

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

udp: deprecate api in favor of router

37def08

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

test: prioritize router

77fb547

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

docs: prioritize router

9c71fe4

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

Merge remote-tracking branch 'envoyproxy/main' into udp-proxy-router

b4cbd8d

Merge remote-tracking branch 'envoyproxy/main' into udp-proxy-router

4a201e7

Merge remote-tracking branch 'envoyproxy/main' into udp-proxy-router

b5086f2

router: add network data inputs

42ce9c3

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

udp: migrate to generic matching API

0b14da0

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

router: change cidr data input to IP and port

744d74b

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

udp: fix no cluster initialized when using generic matching

cf61bb7

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

example: update UDP proxy example

cc24008

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

docs: update to generic matching

f5fd481

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

repokitteh-read-only bot added the api label Oct 27, 2021

repokitteh-read-only bot assigned markdroth Oct 27, 2021

zhxie added 2 commits October 27, 2021 13:03

docs: fix orphan proto

f613a87

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

test: update to generic matching

4a2f3f8

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

mattklein123 unassigned markdroth Oct 27, 2021

docs: fix missing label

79d1748

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

mattklein123 requested changes Mar 30, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 30, 2022

zhxie added 5 commits March 31, 2022 11:14

docs: add deprecation notes

f959f11

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

nit: typo

b9ac01e

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

udp: ensure matching

96d1ca1

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

udp: fix cluster removal log

f0eead0

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

udp: change specifier in route

a5fc56d

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

repokitteh-read-only bot removed the waiting label Mar 31, 2022

udp: merge path of cluster and matcher

00a84ef

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

mattklein123 requested changes Mar 31, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 31, 2022

nit: fix typo

eede13d

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

repokitteh-read-only bot removed the waiting label Apr 1, 2022

zhxie added 2 commits April 1, 2022 10:37

udp: update signature of route

0617a91

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

test: fix route

4eeea14

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

zhxie added 2 commits April 1, 2022 17:27

Merge remote-tracking branch 'envoyproxy/main' into udp-proxy-router

dc59d27

Merge remote-tracking branch 'envoyproxy/main' into udp-proxy-router

3f9c62c

mattklein123 approved these changes Apr 5, 2022

View reviewed changes

repokitteh-read-only bot removed the api label Apr 5, 2022

mattklein123 merged commit 90a1d4b into envoyproxy:main Apr 5, 2022

phlax added a commit to phlax/envoy that referenced this pull request Apr 5, 2022

Revert "udp: add router for UDP proxy (envoyproxy#18791)"

2935594

This reverts commit 90a1d4b. Signed-off-by: Ryan Northey <ryan@synca.io>

ravenblackx pushed a commit to ravenblackx/envoy that referenced this pull request Jun 8, 2022

udp: add router for UDP proxy (envoyproxy#18791)

9f2d939

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

RyanTheOptimist mentioned this pull request Jun 14, 2022

Can't use example configuration on UDP Proxy documentation #21617

Closed

zhxie deleted the udp-proxy-router branch September 7, 2022 08:28

	* Returns all cluster names in the router. The UDP proxy filter requires every cluster names for
	* Returns all cluster names in the router. The UDP proxy filter requires every cluster name for

		route(Network::Address::InstanceConstSharedPtr destination_address,
		Network::Address::InstanceConstSharedPtr source_address) const PURE;

Conversation

zhxie commented Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Oct 27, 2021

Uh oh!

repokitteh-read-only bot commented Oct 27, 2021

Uh oh!

zhxie commented Oct 27, 2021

Uh oh!

repokitteh-read-only bot commented Oct 27, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhxie commented Apr 1, 2022

Uh oh!

repokitteh-read-only bot commented Apr 1, 2022

Uh oh!

zhxie commented Apr 2, 2022

Uh oh!

repokitteh-read-only bot commented Apr 2, 2022

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

zhxie commented Oct 27, 2021 •

edited

Loading