Skip to content

daemon: Ignore cilium_* interfaces when deriving NodePort device#16104

Merged
borkmann merged 1 commit intocilium:masterfrom
eyanulis:gh16019-exclude-cilium-interfaces-nodeport
May 19, 2021
Merged

daemon: Ignore cilium_* interfaces when deriving NodePort device#16104
borkmann merged 1 commit intocilium:masterfrom
eyanulis:gh16019-exclude-cilium-interfaces-nodeport

Conversation

@eyanulis
Copy link
Copy Markdown
Contributor

@eyanulis eyanulis commented May 12, 2021

Any Cilium-created interface (cilium_host, etc) will never be a valid
interface for kube-proxy-replacement NodePort (or direct routing). In
certain cases, it is possible for the NodePort auto-derivation code to
select one of these interfaces. This notably happens when the k8s node
IP is an IPv6 address: the node IP is cloned to cilium_host, and the IP
(sans netmask) is used as a map key - so cilium_host may be viewed as
the only interface with an address matching the node IP.

Add a check bypassing any interface whose name is prefixed with
cilium_ during NodePort device detection.

Add a test mimicking the IPv6 cilium_host case: node IP assigned to a
"real" interface and a cilium_foo interface, we should ignore
cilium_foo.

Fixes: #16019

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Thanks for contributing!

@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 May 12, 2021
@maintainer-s-little-helper
Copy link
Copy Markdown

maintainer-s-little-helper Bot commented May 12, 2021

Commit 102d4515a6b8c84d8aca5f947e568c42ca1b0953 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@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. and removed dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. labels May 12, 2021
@eyanulis eyanulis marked this pull request as ready for review May 12, 2021 03:39
@eyanulis eyanulis requested review from a team and jibi May 12, 2021 03:39
@eyanulis eyanulis mentioned this pull request May 12, 2021
Closed
@borkmann borkmann added kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. labels May 12, 2021
@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 May 12, 2021
@borkmann borkmann added area/daemon Impacts operation of the Cilium daemon. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels May 12, 2021
@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 May 12, 2021
@borkmann borkmann added area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. needs-backport/1.10 labels May 12, 2021
brb
brb requested changes May 12, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@brb brb 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.

Thanks for the contribution! Looks good, just two nits.

Comment thread daemon/cmd/kube_proxy_replacement.go Outdated
Copy link
Copy Markdown
Member

@brb brb May 12, 2021

Choose a reason for hiding this comment

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

Nit: s/skipping interface/skipping interface for device detection/.

Copy link
Copy Markdown
Contributor Author

@eyanulis eyanulis May 13, 2021

Choose a reason for hiding this comment

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

Updated, thanks.

Comment thread daemon/cmd/kube_proxy_replacement.go Outdated
Copy link
Copy Markdown
Member

@brb brb May 12, 2021

Choose a reason for hiding this comment

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

Nit: s/NodePort device/device/ (it's not only used for NodePort).

Copy link
Copy Markdown
Contributor Author

@eyanulis eyanulis May 13, 2021

Choose a reason for hiding this comment

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

Updated, thanks. Also s/in/for/ to be consistent with various messages around it.

jibi
jibi reviewed May 12, 2021
View reviewed changes
Comment thread daemon/cmd/kube_proxy_replacement.go Outdated
Copy link
Copy Markdown
Member

@jibi jibi May 12, 2021

Choose a reason for hiding this comment

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

nit: no need for these continues

Copy link
Copy Markdown
Contributor Author

@eyanulis eyanulis May 13, 2021

Choose a reason for hiding this comment

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

Thanks for catching that, I've removed them. I had gone back and forth between having an "implicit" and "explicit" else condition and the continues would have been required for the implicit condition, but I forgot to remove them after settling on an explicit else.

@eyanulis
daemon: Ignore cilium_* interfaces when deriving NodePort device
dc6988b 
Any Cilium-created interface (cilium_host, etc) will never be a valid
interface for kube-proxy-replacement NodePort (or direct routing). In
certain cases, it is possible for the NodePort auto-derivation code to
select one of these interfaces. This notably happens when the k8s node
IP is an IPv6 address: the node IP is cloned to cilium_host, and the IP
(sans netmask) is used as a map key - so cilium_host may be viewed as
the only interface with an address matching the node IP.

Add a check bypassing any interface whose name is prefixed with
"cilium_" during NodePort device detection.

Add a test mimicking the IPv6 cilium_host case: node IP assigned to a
"real" interface and a "cilium_foo" interface, we should ignore
"cilium_foo".

Fixes: #16019

Signed-off-by: Eric M. Yanulis <eric@eyanulis.net>
@eyanulis
Copy link
Copy Markdown
Contributor Author

eyanulis commented May 13, 2021

@brb @jibi Thank you both for the review. Most recent force-push addresses the comments.

brb
brb approved these changes May 16, 2021
View reviewed changes
Copy link
Copy Markdown
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.

Thanks!

@jibi
Copy link
Copy Markdown
Member

jibi commented May 17, 2021

test-me-please

@eyanulis
Copy link
Copy Markdown
Contributor Author

eyanulis commented May 18, 2021

It looks like most(/all?) of the failing tests are Github flakes from the first run (HTTP 500 during checkout, etc).

Is there any way to rerun them? I'm not terribly familiar with how this repo is wired up to Github Actions but it doesn't seem like they re-ran after the test-me-please comment above from @jibi.

@jibi jibi closed this May 19, 2021
@jibi jibi reopened this May 19, 2021
@jibi
Copy link
Copy Markdown
Member

jibi commented May 19, 2021

It looks like most(/all?) of the failing tests are Github flakes from the first run (HTTP 500 during checkout, etc).

Is there any way to rerun them? I'm not terribly familiar with how this repo is wired up to Github Actions but it doesn't seem like they re-ran after the test-me-please comment above from @jibi.

I think GH was having some issues yesterday with actions. Closing and reopening the PR should have restarted them

@joamaki
Copy link
Copy Markdown
Contributor

joamaki commented May 19, 2021

test-1.16-netnext

@joamaki
Copy link
Copy Markdown
Contributor

joamaki commented May 19, 2021

The tests relevant to this PR are passing, so marking this ready for merge.

@joamaki joamaki added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 19, 2021
@borkmann borkmann merged commit 9366190 into cilium:master May 19, 2021
This was referenced May 21, 2021
Merged
Merged
@aanm aanm mentioned this pull request May 28, 2021
Merged
@aanm aanm mentioned this pull request Jun 16, 2021
Merged
@eyanulis eyanulis deleted the gh16019-exclude-cilium-interfaces-nodeport branch August 30, 2021 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brb brb brb approved these changes

@borkmann borkmann borkmann approved these changes

@joamaki joamaki joamaki approved these changes

+1 more reviewer

@jibi jibi jibi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/daemon Impacts operation of the Cilium daemon. area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. kind/bug This is a bug in the Cilium logic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Cilium kube-proxy-replacement sometimes binds to cilium_host, breaking NodePort

9 participants

@eyanulis @jibi @joamaki @brb @borkmann @kaworu @joestringer @nbusseneau @aanm