Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
udp: Add use_original_src_ip feature on udp proxy #12586
Changes from 41 commits
2e6e73c
3b35051
2f44796
af44b31
c56a26c
f059b0c
247bed0
020bf24
4a4bf65
978fbfb
3e72fa8
4bb08e6
ab200f7
6c8bd24
ec6b11a
1cc9a20
a12ce73
9810807
5970e16
3cb7396
92b552d
66b36b0
2fc489c
a788b97
2a85187
8ce1b01
26d6c22
f6d63fc
b7ea0c2
5622a16
6dafbaf
93fdf3e
e61065d
ced9157
68407ec
5c7ae9f
a57bcdc
643cfc3
5a30f34
10bb89d
19f0dc4
1aba2e4
f0bbb04
13c311f
56316b7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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
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.
@mattklein123 @danzh2010 I have a question. Is it better to throw an exception or do panic?
I'm not sure which is better option.
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.
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.
Ok. I have a question. Originally, I would like to check it on config creation for the UDP proxy. But I could not find how to know the user set the listener's IP as IPv4 or IPv6 in this config creation. That is why I change the checking from config creation to filter creation. Could you give your advise about that? Should I add one more option for that? Or? :)
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.
Sorry I think I missed that. Why do we have to check that? Just because we don't know whether v4 or v6 is working until we check it? Can we just check both? If we need to actually check this at runtime I don't think we can actually raise an exception here. I think we need to just drop the datagram and have a stat or something.
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.
Actually, it is added due to following @danzh2010 's comment : #12586 (comment)
So, I check more linux kernel's implementation, and the ipv6 feature can be disabled during kernel compilation and can be a kernel module so it can be loaded after booting and can be disabled by grub' configuration even if the linux kernel is compiled with ipv6 feature.
I think that there is a advantage that disable the ipv6 on modern common linux distribution.
That is only memory usage but it is not much issue because the memory usage of ipv6 is not so much.
What do you think about that? Should I consider ipv6 disabled environment?
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.
Or how about revert this change and mention the ipv6 stack should be enabled on linux kernel at this option's manual?
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 don't have a strong opinion on this one way or the other, but we just need to make sure the error handling works correctly. If you check this and it can fail at datagram receive time, you need to drop the packet and stat. If you check at config time you can fail then. Up to you as long as we clearly document it.
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.
Ok. Thanks your opinion. :) I will revert previous change and I will mention on document that the ipv6 stack is a mandatory for this new option.