Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Adds a new NOTRACK rule for node-local-dns #24230

Merged
merged 1 commit into from Mar 22, 2023
Merged

Adds a new NOTRACK rule for node-local-dns #24230

merged 1 commit into from Mar 22, 2023

Conversation

Weil0ng
Copy link
Contributor

@Weil0ng Weil0ng commented Mar 8, 2023 •
edited

When node-local-dns replies to non-host pod, it goes through the prerouting chain again (nld is non-host when deployed using LRP). Adds a NOTRACK rule to exempt this path from kernel conntrack too.

Fixes: #24229

@Weil0ng Weil0ng requested a review from aditighag March 8, 2023 00:39
@Weil0ng Weil0ng requested a review from a team as a code owner March 8, 2023 00:39
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 8, 2023
@Weil0ng Weil0ng added the release-note/misc This PR makes changes that have no direct user impact. label Mar 8, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 8, 2023
@Weil0ng
Copy link
Contributor Author

Weil0ng commented Mar 8, 2023

Manually tested in a GKE cluster with Cilium and NLD in non-host mode.

aditighag
aditighag reviewed Mar 8, 2023
View reviewed changes
Copy link
Member

@aditighag aditighag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @Weil0ng! 👋
I'm trying to jog my memory about an issue where we had to add an accept rule to allow traffic in some cases. It'll be great if we could have a table that we can document in the code around which rules are added in input/output rules for node-local dns pods. Wdyt?

Have you given any thoughts to test this as part of CI, and verify that the changes are not breaking anything? We now have kind based tests where we could deploy node-local dns DS along with LRP. Happy to collaborate on this.

pkg/datapath/iptables/iptables.go Outdated Show resolved Hide resolved
@Weil0ng
Copy link
Contributor Author

Weil0ng commented Mar 10, 2023

Hey @Weil0ng! wave I'm trying to jog my memory about an issue where we had to add an accept rule to allow traffic in some cases. It'll be great if we could have a table that we can document in the code around which rules are added in input/output rules for node-local dns pods. Wdyt?

Have you given any thoughts to test this as part of CI, and verify that the changes are not breaking anything? We now have kind based tests where we could deploy node-local dns DS along with LRP. Happy to collaborate on this.

Hi @aditighag :) I believe you were refering to #17585 with the accept rule we added before. I've updated the comment in the code to capture all possible packet paths, PTAL. I'd be happy to add a CI test for node-local-dns, but I wonder if we can do that as a follow up to this PR? I'm not too familiar with the new test infra so it may take me some time to add the tests.

@Weil0ng
Copy link
Contributor Author

Weil0ng commented Mar 15, 2023

Friendly ping :)

@aditighag
Copy link
Member

Hey @Weil0ng! wave I'm trying to jog my memory about an issue where we had to add an accept rule to allow traffic in some cases. It'll be great if we could have a table that we can document in the code around which rules are added in input/output rules for node-local dns pods. Wdyt?
Have you given any thoughts to test this as part of CI, and verify that the changes are not breaking anything? We now have kind based tests where we could deploy node-local dns DS along with LRP. Happy to collaborate on this.

Hi @aditighag :) I believe you were refering to #17585 with the accept rule we added before. I've updated the comment in the code to capture all possible packet paths, PTAL. I'd be happy to add a CI test for node-local-dns, but I wonder if we can do that as a follow up to this PR? I'm not too familiar with the new test infra so it may take me some time to add the tests.

I'm okay with deferring the CI tests to a follow up PR, but it'll be great to have a plan of action. I'll file a new issue targeted for 1.14, and assign both of us as the assignees. Hope that sounds reasonable.

As for this PR, thanks for adding more comments around the different rules. I'm a bit unclear about effects of having asymmetric iptable rules across cilium versions (e.g., upgrade).

Could you share details of how you tested the change?

@Weil0ng
Copy link
Contributor Author

Weil0ng commented Mar 17, 2023 •
edited

I'm okay with deferring the CI tests to a follow up PR, but it'll be great to have a plan of action. I'll file a new issue targeted for 1.14, and assign both of us as the assignees. Hope that sounds reasonable.

Sg, thanks!

As for this PR, thanks for adding more comments around the different rules. I'm a bit unclear about effects of having asymmetric iptable rules across cilium versions (e.g., upgrade).

Could you share details of how you tested the change?

I manually updated the Cilium image in a running cluster with node-local-dns pods, Cilium upon restart will remove all rules/chains added previously by Cilium itself and reapply the rules and I can see on the node that the correct new set of NOTRACK rules are being placed.

@Weil0ng
Copy link
Contributor Author

Weil0ng commented Mar 21, 2023

test-me-please

@Weil0ng
Adds a new NOTRACK rule for node-local-dns
db08b1e 
When node-local-dns replies to non-host pod, it goes through the
prerouting chain again (nld is non-host when deployed using LRP).
- Adds a NOTRACK rule to exempt this path from kernel conntrack
- Adds ACCEPT rule explicitly in forward chain to not rely on the
  default policy to be future-proof

Fixes: 24229

Signed-off-by: Weilong Cui <cuiwl@google.com>
@Weil0ng Weil0ng mentioned this pull request Mar 21, 2023
Open
aditighag
aditighag approved these changes Mar 21, 2023
View reviewed changes
Copy link
Member

@aditighag aditighag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
As discussed, I've filed - #24510 for CI test follow-up.

@aditighag
Copy link
Member

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 21, 2023
@aditighag aditighag merged commit cf0eae1 into cilium:master Mar 22, 2023
42 checks passed
@akhilles akhilles mentioned this pull request Jul 29, 2023
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liuyuan10 liuyuan10 liuyuan10 approved these changes

@akhilles akhilles akhilles left review comments

@aditighag aditighag aditighag approved these changes

Assignees
No one assigned
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

InstallNoTrackRules creates asymmetric iptables rules in CILIUM_PRE_raw
4 participants
@Weil0ng @aditighag @liuyuan10 @akhilles