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 pod annotation to manage iptables NOTRACK rules. #13805

Merged
merged 1 commit into from
Dec 1, 2020
Merged

Adds pod annotation to manage iptables NOTRACK rules. #13805

merged 1 commit into from
Dec 1, 2020

Conversation

Weil0ng
Copy link
Contributor

@Weil0ng Weil0ng commented Oct 29, 2020 •
edited

This adds handler of a new pod annotation no-track-port: <port-number>. When this annotation is in place, 5 NOTRACK rules are automatically inserted into the root-ns iptables for the pod at the port specified. The 5 rules are:

  • NOTRACK in filter table, INPUT chain (ingress)
  • NOTRACK in raw table, PREROUTING chain (ingress)
  • NOTRACK in raw table, OUTPUT chain (ingress)
  • NOTRACK in raw table, OUTPUT chain (egress)
  • NOTRACK in filter table, OUTPUT chain (egress)

The effect of these rules is essentially all traffic going to/from that specified port will skip kernel conntrack. One particular use case is to achieve feature parity with nodelocaldns (https://github.com/kubernetes/enhancements/blob/master/keps/sig-network/0030-nodelocal-dns-cache.md#iptables-notrack).

The rules are managed via a new event type EndpointNoTrackEvent at endpoint creation/deletion time and in pod watcher (when annotation changes).

Fixes: #13686

@Weil0ng Weil0ng requested review from brb, joestringer, aditighag and a team October 29, 2020 02:21
@maintainer-s-little-helper

This comment has been minimized.

Sign in to view
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Oct 29, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Oct 29, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Oct 29, 2020
@Weil0ng Weil0ng requested a review from a team as a code owner October 29, 2020 02:25
@Weil0ng Weil0ng force-pushed the nodelocaldns branch 2 times, most recently from 16355a6 to c556ba2 Compare October 29, 2020 02:27
brb
brb reviewed Oct 29, 2020
View reviewed changes
Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

Some questions / comments before doing a more thorough review.

pkg/datapath/iptables/iptables.go Show resolved Hide resolved
pkg/k8s/watchers/pod.go Outdated Show resolved Hide resolved
@Weil0ng Weil0ng force-pushed the nodelocaldns branch 2 times, most recently from 9334219 to 40cb542 Compare October 30, 2020 19:27
@aanm aanm added dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs release-note/misc This PR makes changes that have no direct user impact. labels Nov 2, 2020
@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 Nov 2, 2020
@Weil0ng
Copy link
Contributor Author

Weil0ng commented Nov 20, 2020

I think all previous comments are addressed :) PTAL @joestringer @aditighag @brb

joestringer
joestringer reviewed Nov 23, 2020
View reviewed changes
pkg/endpoint/events.go Outdated Show resolved Hide resolved
joestringer
joestringer reviewed Nov 23, 2020
View reviewed changes
pkg/endpoint/events.go Outdated Show resolved Hide resolved
joestringer
joestringer requested changes Nov 23, 2020
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Couple of other minor deviations compared to similar surrounding code.

joestringer
joestringer requested changes Nov 24, 2020
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

I must've missed my coffee before my earlier review today 😅 . A few more pretty minor tidyup suggestions below but after those this should be good to merge.

pkg/datapath/iptables/iptables.go Outdated Show resolved Hide resolved
pkg/endpoint/events.go Outdated Show resolved Hide resolved
pkg/endpoint/events.go Outdated Show resolved Hide resolved
pkg/endpoint/events.go Show resolved Hide resolved
@joestringer
Copy link
Member

test-me-please

@Weil0ng
Copy link
Contributor Author

Weil0ng commented Nov 24, 2020

hmm, I don't think GKE failure is PR related? seems like a test setup issue.

@joestringer
Copy link
Member

joestringer commented Nov 24, 2020 •
edited

Kernel-4.19 failure also looks unrelated to this PR, likely related to #14129 . Filed #14159 to track.

@Weil0ng
Copy link
Contributor Author

Weil0ng commented Nov 25, 2020

kernel-netnext seems to be failing for the same reason as kernel-4.19 (unrelated to this PR).

@joestringer
Copy link
Member

#14159 was now resolved, can re-run CI.

@joestringer
Copy link
Member

test-me-please

@Weil0ng
Adds iptables managment for nodelocaldns.
e34476e 
We watch for a new annotation "io.cilium.no-track-port" when
updating endpoints. Combined with adding such annotation to
nodelocaldns's yaml, this should bring back the NOTRACK rules for
nodelocaldns.

Signed-off-by: Weilong Cui <cuiwl@google.com>
@Weil0ng
Copy link
Contributor Author

Weil0ng commented Nov 26, 2020

rebased, rerun CI

@Weil0ng
Copy link
Contributor Author

Weil0ng commented Nov 26, 2020

test-me-please

@Weil0ng
Copy link
Contributor Author

Weil0ng commented Nov 30, 2020

retest-4.9

@Weil0ng
Copy link
Contributor Author

Weil0ng commented Nov 30, 2020

4.9 failure seems unrelated

@Weil0ng
Copy link
Contributor Author

Weil0ng commented Nov 30, 2020

retest-4.9

@joestringer joestringer merged commit e463cfe into cilium:master Dec 1, 2020
@joestringer
Copy link
Member

@Weil0ng would you mind updating the PR description to briefly describe the path that this PR ended up taking? It could be useful to reference in future.

@Weil0ng
Copy link
Contributor Author

Weil0ng commented Dec 1, 2020

@Weil0ng would you mind updating the PR description to briefly describe the path that this PR ended up taking? It could be useful to reference in future.

Done :)

@brb
Copy link
Member

brb commented Dec 4, 2020

Nitpicking, but as there is no release note, therefore (AFAIK) the title will be used in the 1.10 release notes. I read the title as if NOTRACK rule can be added only for nodelocaldns pods, which is not true.

@Weil0ng Weil0ng mentioned this pull request Dec 9, 2020
Merged
@Weil0ng Weil0ng changed the title Adds iptables NOTRACK rules for nodelocaldns. Adds pod annotation to manage iptables NOTRACK rules. Dec 10, 2020
@Weil0ng
Copy link
Contributor Author

Weil0ng commented Dec 10, 2020

Nitpicking, but as there is no release note, therefore (AFAIK) the title will be used in the 1.10 release notes. I read the title as if NOTRACK rule can be added only for nodelocaldns pods, which is not true.

Sorry just was this, edited the title to be more generic.

@pchaigno pchaigno added the sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. label Mar 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brb brb brb left review comments

@joestringer joestringer joestringer approved these changes

@aditighag aditighag aditighag approved these changes

@nathanjsweet nathanjsweet Awaiting requested review from nathanjsweet

Assignees

@nathanjsweet nathanjsweet

@joestringer joestringer

@aditighag aditighag

Labels
release-note/misc This PR makes changes that have no direct user impact. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
No open projects
1.10.0
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

NOTRACK rules for node-local-dns
7 participants
@Weil0ng @joestringer @brb @aditighag @nathanjsweet @pchaigno @aanm