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.

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

agent/datapath: Do not explicitly expose NodePort via cilium_host #11692

Merged
merged 1 commit into from May 26, 2020

Conversation

brb
Copy link
Member

@brb brb commented May 26, 2020

As we are planning to get rid of cilium_host and accessing a NodePort
svc via its IP addr doesn't make sense (because the IP addr is not
static), do not expose "ipv{4,6}(cilium_host):NodePort" in the BPF LB
maps.

Unfortunately, the service is still reachable via the cilium_host IP
addr because of the wildcard lookup in bpf_sock. However, remove this
svc access from the docs to avoid any surprises after cilium_host has
been removed.

Accessing a NodePort service via cilium_host IP addr is no longer recommended.

@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 May 26, 2020
@brb brb requested review from borkmann and a team May 26, 2020 11:51
@brb brb requested review from a team as code owners May 26, 2020 11:51
@brb brb requested a review from a team May 26, 2020 11:51
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 May 26, 2020
@brb
Copy link
Member Author

brb commented May 26, 2020

test-me-please

@brb brb changed the title datapath: Do not explicitly expose NodePort via cilium_host agent/datapath: Do not explicitly expose NodePort via cilium_host May 26, 2020
@coveralls
Copy link

coveralls commented May 26, 2020

Coverage Status

Coverage decreased (-0.02%) to 36.868% when pulling 0302aa3 on pr/brb/rm-ciliumhost-nodeport into 84db4f6 on master.

@@ -21,7 +21,7 @@ For installing ``kubeadm`` and for more provisioning options please refer to

Cilium's kube-proxy replacement depends on the :ref:`host-services` feature,
therefore a v4.19.57, v5.1.16, v5.2.0 or more recent Linux kernel is required.
We recommend a v5.3 or even more recent Linux kernel such as v5.8 as Cilium
We recommend a v5.3 or even more recent Linux kernel such as v5.7 as Cilium
Copy link
Member

Choose a reason for hiding this comment

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

Why changing? 5.8 will have the getpeername hook which is why I stated it here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Considering that it will take a while until v5.8 has been released, it might discourage users to try the replacement. We could update the docs after the release.

The same logic applies to 5.7 - it hasn't been released yet. However, considering that it's in rc7, it should be released before we release Cilium v1.8.

Copy link
Member

Choose a reason for hiding this comment

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

This would probably be less ambiguous if we just stated the facts:

"Linux kernels 5.3 and 5.8 add additional features that Cilium can use to further optimize the kube-proxy replacement implementation."

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, will add it in a separate PR.

@brb brb force-pushed the pr/brb/rm-ciliumhost-nodeport branch from 8479382 to 0302aa3 Compare May 26, 2020 17:28
As we are planning to get rid of cilium_host and accessing a NodePort
svc via its IP addr doesn't make sense (because the IP addr is not
static), do not expose "ipv{4,6}(cilium_host):NodePort" in the BPF LB
maps.

Unfortunately, the service is still reachable via the cilium_host IP
addr because of the wildcard lookup in bpf_sock. However, remove this
svc access from the docs to avoid any surprises after cilium_host has
been removed.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@borkmann borkmann merged commit cd91f29 into master May 26, 2020
1.8.0 automation moved this from In progress to Merged May 26, 2020
@borkmann borkmann deleted the pr/brb/rm-ciliumhost-nodeport branch May 26, 2020 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
No open projects
1.8.0
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

5 participants