udp: Add use_original_src_ip feature on udp proxy #12586

chadr123 · 2020-08-11T04:46:23Z

Commit Message:
There is a similar feature for no snat but it only works for tcp case.
The envoy supports filter structure so that we can add or remove the filter dynamically.
But the udp load banalcer has a limitation that can have only one filter.

So, we cannot add more filters on udp load banalcer.
So, the new option is introduced that name is use_original_src_ip on udp_proxy filter.
If it is set as true, all packets that start from envoy can have original source ip address that
same as sender's ip address.

Fixes #12513, #12277

Signed-off-by: DongRyeol Cha dr83.cha@samsung.com
Additional Description: None
Risk Level: Low
Testing: bazel test
Docs Changes: Add a new option that name is use_original_src_ip to UdpProxyConfig.
Release Notes: changed current.rst in this PR.

repokitteh-read-only · 2020-08-11T04:46:29Z

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

🐱

Caused by: #12586 was opened by chadr123.

see: more, trace.

mattklein123

Thanks for working on this. A few comments to get started. I would also like @danzh2010 to take a look. I'm not sure if it's possible but is there any way to do an integration test over localhost?

/wait

api/envoy/extensions/filters/udp/udp_proxy/v3/udp_proxy.proto

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

chadr123 · 2020-08-12T01:49:56Z

Thanks for working on this. A few comments to get started. I would also like @danzh2010 to take a look. I'm not sure if it's possible but is there any way to do an integration test over localhost?

/wait

I'm also not sure if it is possible to do an integration test over localhost but I will check if it is possible.

chadr123 · 2020-08-13T05:51:01Z

@mattklein123 I faced an one problem to write some integration tests for this patch.
This patch requires the CAP_NET_ADMIN capability because it set the IP_TRANSPARENT option to socket.
By my testing, the calling of "do_ci.sh bazel.*" does not enable the CAP_NET_ADMIN so that the integration test is failing.
Do you know how to give the CAP_NET_ADMIN capability when I invoke the do_ci.sh?
I found the related PR(#5268) but there are not enough information so that I don't know how to use it with do_ci.sh.

lizan · 2020-08-13T06:09:32Z

@chadr123 CI already has NET_ADMIN: https://github.com/envoyproxy/envoy/blob/master/ci/run_envoy_docker.sh#L32

chadr123 · 2020-08-13T06:27:51Z

@chadr123 CI already has NET_ADMIN: https://github.com/envoyproxy/envoy/blob/master/ci/run_envoy_docker.sh#L32

Thanks! I will check it more. :)

There is a similar feature for no snat but it only works for tcp case. The envoy supports filter structure so that we can add or remove the filter dynamically. But the udp load banalcer has a limitation that can have only one filter. So, we cannot add more filters on udp load banalcer. So, the new option is introduced that name is use_original_src_ip on udp_proxy filter. If it is set as true, all packets that start from envoy can have original source ip address that same as sender's ip address. Fixes envoyproxy#12513, envoyproxy#12277 Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>

Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>

… phase Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>

chadr123 · 2020-08-13T08:20:17Z

I found that the "--cap-add NET_ADMIN" does not work properly because capabilities are not propagated to the actual test program.

$ cat /proc/25004/comm
udp_proxy_filte
$ cat /proc/25004/status | grep Cap
CapInh: 00000000a80c35fb
CapPrm: 0000000000000000
CapEff: 0000000000000000
CapBnd: 00000000a80c35fb
CapAmb: 0000000000000000

The CapPrm is empty.

So, I more investigated this and I found that there is a issue that set the capabilities to child processes.
Here are processes that the run_envoy_docker.sh invoked.

chadr 25583 25568 0 16:39 pts/1 00:00:00 docker run --rm -it -e HTTP_PROXY= -e HTTPS_PROXY= -e NO_PROXY=localhost,127.0.0.1 -u root:root -v /home/chadr/hdd/tmp/envoy-docker-build:/build -v /var/run/docker.sock:/var/run/docker.sock -e BAZEL_BUILD_EXTRA_OPTIONS -e BAZEL_EXTRA_TEST_OPTIONS -e BAZEL_REMOTE_CACHE -e ENVOY_STDLIB -e BUILD_REASON -e BAZEL_REMOTE_INSTANCE -e GCP_SERVICE_ACCOUNT_KEY -e NUM_CPUS -e ENVOY_RBE -e FUZZIT_API_KEY -e ENVOY_BUILD_IMAGE -e ENVOY_SRCDIR -e ENVOY_BUILD_TARGET -e SYSTEM_PULLREQUEST_TARGETBRANCH -e SYSTEM_PULLREQUEST_PULLREQUESTNUMBER -e GCS_ARTIFACT_BUCKET -e BUILD_SOURCEBRANCHNAME -e BAZELISK_BASE_URL -e ENVOY_BUILD_ARCH -v /home/chadr/hdd/project/forks/envoy:/source --cap-add SYS_PTRACE --cap-add NET_RAW --cap-add NET_ADMIN envoyproxy/envoy-build-ubuntu:923df85a4ba7f30dcd0cb6b0c6d8d604f0e20f48 /bin/bash -lc groupadd --gid 1000 -f envoygroup && useradd -o --uid 1000 --gid 1000 --no-create-home --home-dir /build envoybuild && usermod -a -G pcap envoybuild && sudo -EHs -u envoybuild bash -c "cd /source && ./ci/do_ci.sh bazel.dev //test/extensions/filters/udp/udp_proxy:udp_proxy_filter_test"
root 25668 25645 0 16:39 pts/0 00:00:00 sudo -EHs -u envoybuild bash -c cd /source && ./ci/do_ci.sh bazel.dev //test/extensions/filters/udp/udp_proxy:udp_proxy_filter_test
chadr 25732 25668 0 16:39 pts/0 00:00:00 /bin/bash ./ci/do_ci.sh bazel.dev //test/extensions/filters/udp/udp_proxy:udp_proxy_filter_test

And here are the caps for each processes.

$ cat /proc/25668/status | grep Cap
CapInh: 00000000a80c35fb
CapPrm: 00000000a80c35fb
CapEff: 00000000a80c35fb
CapBnd: 00000000a80c35fb
CapAmb: 0000000000000000
$ cat /proc/25732/status | grep Cap
CapInh: 00000000a80c35fb
CapPrm: 0000000000000000
CapEff: 0000000000000000
CapBnd: 00000000a80c35fb
CapAmb: 0000000000000000

You can see that the 25668 process has proper capabilities but 25732 does not.
The 25668 is the "sudo -EHs -u envoybuild bash -c cd /source && ./ci/do_ci.sh bazel.dev //test/extensions/filters/udp/udp_proxy:udp_proxy_filter_test" command.

And the 25732 is the "/bin/bash ./ci/do_ci.sh bazel.dev //test/extensions/filters/udp/udp_proxy:udp_proxy_filter_test" command.

I think that the capabilities are dropped by sudo but I'm not sure.
I will check it more and fix it if possible.

I will let you know the result soon.

chadr123 · 2020-08-13T11:53:21Z

I tried to find the solution but it seems that we could not.
Because the CapPrm can be calculated by following fomula :

P'(permitted) = (P(inheritable) & F(inheritable)) |
(F(permitted) & cap_bset) | P'(ambient)
P denotes the value of a thread capability set before the execve(2)
P' denotes the value of a thread capability set after the execve(2)
F denotes a file capability set
cap_bset is the value of the capability bounding set.

It means that if we want to set the net_admin capability to test programs, we should set the net_admin to all each test program binaries because the file capability is always applied by 'and' condition.
The last thing that the ambient is an 'or' condition but it is supported since linux kernel v4.3 and I could not set the ambient capability with normal user privilege because as you know the bazel test will run with envoybuild user.

Do you have any ideas?

Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>

chadr123 · 2020-08-14T06:08:09Z

I have updated my patch without integration test due to it requires the net_admin capability but ci does not support it because it run as normal user that name is envoybuild.
Please let me know if you have any ideas for integration test.

Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>

chadr123 · 2020-08-22T02:33:27Z

@danzh2010 All comments are addressed. Please check it. :)

Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>

chadr123 · 2020-08-23T06:40:10Z

/retest

repokitteh-read-only · 2020-08-23T06:40:14Z

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Cannot retry non-completed check: envoy-presubmit (Linux-x64 compile_time_options), please wait.

🐱

Caused by: a #12586 (comment) was created by @chadr123.

see: more, trace.

chadr123 · 2020-08-23T07:10:25Z

/retest

repokitteh-read-only · 2020-08-23T07:10:29Z

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #12586 (comment) was created by @chadr123.

see: more, trace.

mattklein123

Thanks LGTM with small comments.

/wait

mattklein123 · 2020-08-24T21:25:33Z

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

@@ -12,6 +15,16 @@ UdpProxyFilter::UdpProxyFilter(Network::UdpReadFilterCallbacks& callbacks,
    : UdpListenerReadFilter(callbacks), config_(config),
      cluster_update_callbacks_(
          config->clusterManager().addThreadLocalClusterUpdateCallbacks(*this)) {
+  bool check_v4only =


mattklein123 · 2020-08-24T21:26:50Z

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

+      callbacks.udpListener().localAddress()->ip()->version() == Network::Address::IpVersion::v4;
+  if (config_->usingOriginalSrcIp() &&
+      !Api::OsSysCallsSingleton::get().supportsIpTransparent(check_v4only)) {
+    throw Network::CreateListenerException(


I think this should actually be checked inside the the config creation for the UDP proxy, not here on filter creation. This way it will be checked in config load. In that case you can throw an EnvoyException and it will be a normal error. You should also add tests for this case.

danzh2010 · 2020-08-25T03:42:22Z

/assign danzh2010

This reverts commit ced9157. And mention that ipv6 stack is mandatory. Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>

Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>

chadr123 · 2020-08-26T07:20:04Z

@mattklein123 @danzh2010 I think probably all comments are addressed. Please check it again. :)

mattklein123

Thank you!

comments addressed

chadr123 requested a review from mattklein123 as a code owner August 11, 2020 04:46

repokitteh-read-only bot added the api label Aug 11, 2020

mattklein123 self-assigned this Aug 11, 2020

chadr123 changed the title ~~udp: Add use_original_src_ip feature on udp proxy~~ [WIP] udp: Add use_original_src_ip feature on udp proxy Aug 11, 2020

chadr123 force-pushed the add_no_snat_option branch from 7c4cc75 to 916129b Compare August 11, 2020 10:44

chadr123 changed the title ~~[WIP] udp: Add use_original_src_ip feature on udp proxy~~ udp: Add use_original_src_ip feature on udp proxy Aug 11, 2020

mattklein123 requested changes Aug 11, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 11, 2020

DongRyeol Cha added 9 commits August 13, 2020 15:41

Change the io_handle to socket

3b35051

Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>

Fix ci

2f44796

Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>

Test local_ip is nullptr case

af44b31

Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>

More clarify documents

c56a26c

Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>

For suppression of clang-tidy NewDeleteLeaks

f059b0c

Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>

Use socket option interfaces

247bed0

Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>

Add more information in documents

020bf24

Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>

Check if the platform supports ip tranparent at configuration loading…

4a4bf65

… phase Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>

DongRyeol Cha added 2 commits August 14, 2020 11:22

More document details

978fbfb

Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>

More tests

3e72fa8

Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>

chadr123 force-pushed the add_no_snat_option branch from 1ae81cd to 3e72fa8 Compare August 14, 2020 06:05

repokitteh-read-only bot removed the waiting label Aug 14, 2020

Fix format

4bb08e6

Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>

DongRyeol Cha added 2 commits August 22, 2020 10:55

Merge branch 'master' into add_no_snat_option

643cfc3

Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>

Fix format

5a30f34

Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>

Fix release note

10bb89d

Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>

chadr123 requested a review from mattklein123 August 22, 2020 13:15

Merge branch 'master' into add_no_snat_option

19f0dc4

Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>

mattklein123 requested changes Aug 24, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 24, 2020

repokitteh-read-only bot assigned danzh2010 Aug 25, 2020

DongRyeol Cha added 4 commits August 26, 2020 11:36

Revert "Do not fail if ipv6 not supported when listener's ip is ipv4"

1aba2e4

This reverts commit ced9157. And mention that ipv6 stack is mandatory. Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>

Fix document

f0bbb04

Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>

Merge branch 'master' into add_no_snat_option

13c311f

Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>

Fix format

56316b7

Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>

repokitteh-read-only bot removed the waiting label Aug 26, 2020

chadr123 requested review from mattklein123 and danzh2010 August 26, 2020 07:20

mattklein123 approved these changes Aug 26, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Aug 26, 2020

mattklein123 merged commit a2b11b2 into envoyproxy:master Aug 26, 2020

chadr123 deleted the add_no_snat_option branch August 26, 2020 22:21

phlax mentioned this pull request Oct 3, 2020

sandbox: TLS sandbox #13373

Closed

skiptomyliu mentioned this pull request Mar 23, 2023

udp: support udp proxy to use original_dst_cluster #26285

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

udp: Add use_original_src_ip feature on udp proxy #12586

udp: Add use_original_src_ip feature on udp proxy #12586

chadr123 commented Aug 11, 2020 •

edited

Loading

repokitteh-read-only bot commented Aug 11, 2020

mattklein123 left a comment

chadr123 commented Aug 12, 2020

chadr123 commented Aug 13, 2020

lizan commented Aug 13, 2020

chadr123 commented Aug 13, 2020

chadr123 commented Aug 13, 2020 •

edited

Loading

chadr123 commented Aug 13, 2020 •

edited

Loading

chadr123 commented Aug 14, 2020

chadr123 commented Aug 22, 2020

chadr123 commented Aug 23, 2020

repokitteh-read-only bot commented Aug 23, 2020

chadr123 commented Aug 23, 2020

repokitteh-read-only bot commented Aug 23, 2020

mattklein123 left a comment

mattklein123 Aug 24, 2020

mattklein123 Aug 24, 2020

danzh2010 commented Aug 25, 2020

chadr123 commented Aug 26, 2020

mattklein123 left a comment

udp: Add use_original_src_ip feature on udp proxy #12586

udp: Add use_original_src_ip feature on udp proxy #12586

Conversation

chadr123 commented Aug 11, 2020 • edited Loading

repokitteh-read-only bot commented Aug 11, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

chadr123 commented Aug 12, 2020

chadr123 commented Aug 13, 2020

lizan commented Aug 13, 2020

chadr123 commented Aug 13, 2020

chadr123 commented Aug 13, 2020 • edited Loading

chadr123 commented Aug 13, 2020 • edited Loading

chadr123 commented Aug 14, 2020

chadr123 commented Aug 22, 2020

chadr123 commented Aug 23, 2020

repokitteh-read-only bot commented Aug 23, 2020

chadr123 commented Aug 23, 2020

repokitteh-read-only bot commented Aug 23, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 Aug 24, 2020

Choose a reason for hiding this comment

mattklein123 Aug 24, 2020

Choose a reason for hiding this comment

danzh2010 commented Aug 25, 2020

chadr123 commented Aug 26, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

chadr123 commented Aug 11, 2020 •

edited

Loading

chadr123 commented Aug 13, 2020 •

edited

Loading

chadr123 commented Aug 13, 2020 •

edited

Loading