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

cilium: disable bind-protection in kube-proxy free probe mode #14182

Merged
merged 1 commit into from
Nov 26, 2020

Conversation

borkmann
Copy link
Member

@borkmann borkmann commented Nov 26, 2020

The probe mode is expected to only run alongside kube-proxy as hybrid.
There was confusion that the kube-proxy log was throwing (harmless) warnings
to its log that it could not bind sockets to service ports in the hostns.
This is due to Cilium performing bind protection right out of the bind(2)
syscall with eBPF. To avoid this confusion, defer to kube-proxy to bind
sockets instead. This is less efficient and consuming more resources, but
if users want to avoid the overhead, they would run kube-proxy free in strict
mode anyway where Cilium does the bind protection by default anyway.

Signed-off-by: Daniel Borkmann daniel@iogearbox.net

The probe mode is expected to only run alongside kube-proxy as hybrid.
There was confusion that the kube-proxy log was throwing (harmless) warnings
to its log that it could not bind sockets to service ports in the hostns.
This is due to Cilium performing bind protection right out of the bind(2)
syscall with eBPF. To avoid this confusion, defer to kube-proxy to bind
sockets instead. This is less efficient and consuming more resources, but
if users want to avoid the overhead, they would run kube-proxy free in strict
mode anyway where Cilium does the bind protection by default anyway.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
@borkmann borkmann requested review from brb, tgraf, pchaigno and a team November 26, 2020 12:21
@borkmann borkmann requested a review from a team as a code owner November 26, 2020 12:21
@borkmann borkmann requested review from a team and qmonnet November 26, 2020 12:21
@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 Nov 26, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Nov 26, 2020
@borkmann borkmann added the release-note/misc This PR makes changes that have no direct user impact. label Nov 26, 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 26, 2020
@borkmann borkmann added area/daemon Impacts operation of the Cilium daemon. needs-backport/1.9 labels Nov 26, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.9.1 Nov 26, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.8.6 Nov 26, 2020
// We let kube-proxy do the less efficient bind-protection in
// this case to avoid the latter throwing (harmless) warnings
// to its log that bind request is rejected.
option.Config.NodePortBindProtection = false
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to log something (e.g., info message) when we overwrite this setting?

Copy link
Member Author

@borkmann borkmann Nov 26, 2020

Choose a reason for hiding this comment

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

I was thinking about it, definitely not a warn, but won't people just ignore and never read the info messages? How effective is it? Dunno.. either way from user side it should be transparent. One thing we could do which I think makes sense either way is to extend cilium status --verbose with the full kpr settings so all is visible at once.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking that anyone who notices the option is not taking effect (because it was overwritten) will likely check the logs. Apart from the documentation (static & verbose), the logs are pretty much the only opportunity we have of explaining what's happening (at runtime) to users. I agree it shouldn't be a warning.

👍 on cilium status --verbose

Copy link
Member Author

@borkmann borkmann Nov 26, 2020

Choose a reason for hiding this comment

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

Ok, I'll extend status in general on kpr a bit, but as separate PR then. (and can also add the info msg)

@borkmann
Copy link
Member Author

test-me-please

@borkmann borkmann merged commit 2a3e5d4 into master Nov 26, 2020
@borkmann borkmann deleted the pr/bind-probe branch November 26, 2020 17:45
This was referenced Nov 30, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.6 Nov 30, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.9 in 1.9.1 Nov 30, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.9 to Backport done to v1.9 in 1.9.1 Dec 2, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.9 to Backport done to v1.9 in 1.9.1 Dec 2, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.8 to Backport done to v1.8 in 1.8.6 Dec 2, 2020
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/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.8.6
Backport done to v1.8
1.9.1
Backport done to v1.9
Development

Successfully merging this pull request may close these issues.

None yet

7 participants