-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
iptables: use ISTIO_OUTPUT
chain for DNS UDP rules
#50915
iptables: use ISTIO_OUTPUT
chain for DNS UDP rules
#50915
Conversation
Hi @eoinfennessy. Thanks for your PR. I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Hi @bleggett, do you think any further changes need to be made to address this issue? |
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 it's a good change but pretty risky - can you confirm the result by getting an iptables-save on a real pod ( the golden files are pretty fake - not sure why we use uid-owner 3 and 4, not something we'll have in practice ) ?
(long term): If you are interested in iptables and capture - I would save the capture in a doc, perhaps one capture from a pod with all iptables-related annotations plus ambient capture - and evaluate the differences and if we can merge the behavior. It is not clear at all why we need 2 different iptables code paths both operating on the pod - in the past ambient was in host namespace and it made sense.
Thanks @costinm!
ISTIO_OUTPUT is created once in the
This jump is currently being created with
Do we want to now create this without the
Yes, I'll do some comparisons with between this and the current behavior for both sidecar and ambient. |
+1 but we are using exactly the same library we use for real pods to generate the result we directly compare against the goldens - the inputs may be different in a real pod, but the rules/tables/chains will not be - that part is deterministic. |
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.
This LGTM conceptually and is a pretty straightforward rename.
We have decent testing around this, so if/when the tests pass, I am good. TY @eoinfennessy !
@@ -616,7 +616,7 @@ func HandleDNSUDP( | |||
ext: ext, | |||
ops: ops, | |||
table: constants.NAT, | |||
chain: constants.OUTPUT, |
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.
You need to insert the jump to the istio chain, since we are no longer using the default one.
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, @bleggett! I've added the required jumps in the latest commit. The integration tests look OK now.
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.
with this one rule
-A OUTPUT -p udp -j ISTIO_OUTPUT
I am not sure i can get what really it fix here
Hi @hzxuzhonghu, this jump is required because all UDP rules have been moved from the main OUTPUT chain to the ISTIO_OUTPUT chain. |
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.
LGTM! ty @eoinfennessy!
Shouldn't we conditionally jump? Like only for dport 53/sport 15053? |
The existing rules (now moved to the custom chain) only apply if the traffic meets those reqs (is udp/ports, etc) so it amounts to the same thing. Putting a rule in a custom chain and jumping to it doesn't change rule evaluation over how they would work in the primary/default chain (where they were before) - if they would all have been skipped in the main chain, they are still all skipped in the secondary chain. If we checked before the jump we would effectively be checking the same properties 2x for every relevant packet. Jumps are supposed to be pretty dumb and have virtually zero cost, they're just pointers - organizational conveniences for the user. |
Please provide a description of this PR:
Fixes #50532