-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
udp: Add use_original_src_ip feature on udp proxy #12586
Conversation
7c4cc75
to
916129b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
@mattklein123 I faced an one problem to write some integration tests for this patch. |
@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>
Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>
Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>
Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>
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>
I found that the "--cap-add NET_ADMIN" does not work properly because capabilities are not propagated to the actual test program.
The CapPrm is empty. So, I more investigated this and I found that there is a issue that set the capabilities to child processes.
And here are the caps for each processes.
You can see that the 25668 process has proper capabilities but 25732 does not. 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 let you know the result soon. |
I tried to find the solution but it seems that we could not.
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. Do you have any ideas? |
Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>
Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>
1ae81cd
to
3e72fa8
Compare
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. |
Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>
Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>
Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>
@danzh2010 All comments are addressed. Please check it. :) |
Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>
Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>
/retest |
Retrying Azure Pipelines, to retry CircleCI checks, use |
/retest |
Retrying Azure Pipelines, to retry CircleCI checks, use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks LGTM with small comments.
/wait
@@ -12,6 +15,16 @@ UdpProxyFilter::UdpProxyFilter(Network::UdpReadFilterCallbacks& callbacks, | |||
: UdpListenerReadFilter(callbacks), config_(config), | |||
cluster_update_callbacks_( | |||
config->clusterManager().addThreadLocalClusterUpdateCallbacks(*this)) { | |||
bool check_v4only = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: const
callbacks.udpListener().localAddress()->ip()->version() == Network::Address::IpVersion::v4; | ||
if (config_->usingOriginalSrcIp() && | ||
!Api::OsSysCallsSingleton::get().supportsIpTransparent(check_v4only)) { | ||
throw Network::CreateListenerException( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
/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>
Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>
Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>
@mattklein123 @danzh2010 I think probably all comments are addressed. Please check it again. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
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.