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

Drop ephemeral port range check for NodePort from datapath #10782

Merged
merged 4 commits into from
Apr 2, 2020
Merged

Drop ephemeral port range check for NodePort from datapath #10782

merged 4 commits into from
Apr 2, 2020

Conversation

brb
Copy link
Member

@brb brb commented Mar 31, 2020

This PR:

  • Removes the ephemeral port range check from the NodePort BPF.
  • Checks the NodePort range against net.ipv4.ip_local_reserved_ports. Modifies it if overlap with the ephemeral range is detected.
  • Introduces --enable-auto-protect-node-port-range (global.nodePort.autoProtectPortRange in helm) flag do disable the behavior ^^.
  • Extends the kube-proxy-free docs about the range check.

Reviewable per commit.

Protect NodePort port range by appending it to net.ipv4.ip_local_reserved_ports if the range clashes with ephemeral port range

Fix #10261.

@brb brb added pending-review sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. area/daemon Impacts operation of the Cilium daemon. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Mar 31, 2020
@brb brb requested review from a team March 31, 2020 13:30
@brb brb requested a review from a team as a code owner March 31, 2020 13:30
@brb brb requested a review from a team March 31, 2020 13:30
@brb
Copy link
Member Author

brb commented Mar 31, 2020

test-me-please

pchaigno
pchaigno approved these changes Mar 31, 2020
View reviewed changes
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

One nit below.

daemon/cmd/daemon_main.go Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Mar 31, 2020 •
edited
Loading

Coverage Status

Coverage decreased (-0.05%) to 45.472% when pulling d2d5426 on pr/brb/drop-ephermeral-port-check into 26dec4c on master.

@brb brb force-pushed the pr/brb/drop-ephermeral-port-check branch from 9ece33a to a7bdbcb Compare March 31, 2020 14:54
@brb
Copy link
Member Author

brb commented Mar 31, 2020

test-me-please

aanm
aanm requested changes Mar 31, 2020
View reviewed changes
Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

If we are going to backport this it requires upgrade notes.

daemon/cmd/daemon_main.go Show resolved Hide resolved
borkmann
borkmann approved these changes Mar 31, 2020
View reviewed changes
Copy link
Member

@borkmann borkmann left a comment •
edited
Loading

Choose a reason for hiding this comment

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

With @aanm's feedback applied, the rest looks good, thx.

aanm
aanm requested changes Mar 31, 2020
View reviewed changes
daemon/cmd/daemon_main.go Show resolved Hide resolved
@brb brb force-pushed the pr/brb/drop-ephermeral-port-check branch from a7bdbcb to f7324ed Compare April 1, 2020 08:55
@brb
Copy link
Member Author

brb commented Apr 1, 2020

@aanm Updated the upgrade guide.

@brb
Copy link
Member Author

brb commented Apr 1, 2020

test-me-please

@brb
Copy link
Member Author

brb commented Apr 1, 2020

test-docs-please

@brb brb force-pushed the pr/brb/drop-ephermeral-port-check branch from f7324ed to d2d5426 Compare April 2, 2020 07:04
@brb
Copy link
Member Author

brb commented Apr 2, 2020

test-me-please

brb added 2 commits April 2, 2020 09:04
@brb
datapath,daemon: Remove ephemeral port check for NodePort
3736c91 
This commit removes the ephemeral port range check for NodePort from the
BPF datapath.

Instead, in the agent we check whether the NodePort range is covered by
ip_local_reserved_ports. If it's not, then we append the range to
ip_local_reserved_ports. Users can opt out from the latter by setting
--enable-auto-protect-node-port-range=false.

Signed-off-by: Martynas Pumputis <[email protected]>
@brb
helm: Add global.nodePort.autoProtectPortRange
eb32c7b 
The value configures --enable-auto-protect-node-port-range. The default
is true.

Signed-off-by: Martynas Pumputis <[email protected]>
@aanm aanm merged commit 8162f6b into master Apr 2, 2020
@aanm aanm deleted the pr/brb/drop-ephermeral-port-check branch April 2, 2020 12:28
@brb
Copy link
Member Author

brb commented Apr 2, 2020

The 4.19 CI build hit #10821

@pchaigno pchaigno mentioned this pull request Apr 2, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@borkmann borkmann borkmann approved these changes

@aanm aanm aanm approved these changes

@pchaigno pchaigno pchaigno approved these changes

Assignees
No one assigned
Labels
area/daemon Impacts operation of the Cilium daemon. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consider dropping ephermeral port range check in NodePort
6 participants
@brb @coveralls @borkmann @pchaigno @aanm @joestringer