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

eyanulis · 2021-05-12T02:47:36Z

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 · 2021-05-12T02:49:22Z

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

brb

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

brb · 2021-05-12T08:35:31Z

daemon/cmd/kube_proxy_replacement.go

+			// ifIndex or if its name begins with cilium_.
+			if link, err := netlink.LinkByIndex(a.LinkIndex); err != nil {
+				log.WithError(err).WithField(logfields.LinkIndex, a.LinkIndex).Warn(
+					"Unable to resolve link from ifIndex, skipping interface")


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

Updated, thanks.

brb · 2021-05-12T08:36:14Z

daemon/cmd/kube_proxy_replacement.go

+				continue
+			} else if strings.HasPrefix(link.Attrs().Name, "cilium_") {
+				log.WithField(logfields.Device, link.Attrs().Name).Debug(
+					"Skipping Cilium-generated interface in NodePort device detection")


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

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

jibi · 2021-05-12T09:12:04Z

daemon/cmd/kube_proxy_replacement.go

+			if link, err := netlink.LinkByIndex(a.LinkIndex); err != nil {
+				log.WithError(err).WithField(logfields.LinkIndex, a.LinkIndex).Warn(
+					"Unable to resolve link from ifIndex, skipping interface")
+				continue


nit: no need for these continues

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.

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 · 2021-05-13T01:46:43Z

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

brb

Thanks!

jibi · 2021-05-17T07:34:44Z

test-me-please

eyanulis · 2021-05-18T01:57:42Z

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 · 2021-05-19T11:27:33Z

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 · 2021-05-19T14:51:18Z

test-1.16-netnext

joamaki · 2021-05-19T14:52:35Z

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

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 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 marked this pull request as ready for review May 12, 2021 03:39

eyanulis requested review from a team and jibi May 12, 2021 03:39

maintainer-s-little-helper bot assigned jibi May 12, 2021

eyanulis mentioned this pull request May 12, 2021

pkg/datapath: Assign correct IPv6 router IP to cilium_host interface #16044

Closed

borkmann assigned brb May 12, 2021

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 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 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 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 added sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. needs-backport/1.10 labels May 12, 2021

maintainer-s-little-helper bot added this to Needs backport from master in 1.10.0-rc2 May 12, 2021

borkmann added the needs-backport/1.9 label May 12, 2021

maintainer-s-little-helper bot added this to Needs backport from master in 1.9.7 May 12, 2021

borkmann approved these changes May 12, 2021

View reviewed changes

borkmann assigned joamaki May 12, 2021

brb requested changes May 12, 2021

View reviewed changes

jibi reviewed May 12, 2021

View reviewed changes

joestringer added this to Needs backport from master in 1.9.8 May 13, 2021

joestringer removed this from Needs backport from master in 1.9.7 May 13, 2021

brb approved these changes May 16, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned jibi May 17, 2021

jibi closed this May 19, 2021

jibi reopened this May 19, 2021

joamaki approved these changes May 19, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned joamaki May 19, 2021

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 merged commit 9366190 into cilium:master May 19, 2021

aanm added this to Needs backport from master in 1.10.1 May 19, 2021

aanm removed this from Needs backport from master in 1.10.0-rc3 May 19, 2021

nbusseneau added backport-pending/1.10 and removed needs-backport/1.10 labels May 21, 2021

This was referenced May 21, 2021

v1.10 backports 2021-05-21 #16269

Merged

v1.9 backports 2021-05-21 #16270

Merged

nbusseneau added backport-pending/1.9 and removed needs-backport/1.9 labels May 21, 2021

aanm moved this from Needs backport from master to Backport pending to v1.10 in 1.10.1 May 25, 2021

kaworu added backport-done/1.9 and removed backport-pending/1.9 labels May 27, 2021

aanm moved this from Needs backport from master to Backport done to v1.9 in 1.9.8 May 27, 2021

aanm mentioned this pull request May 28, 2021

Prepare for release v1.9.8 #16344

Merged

aanm added backport-done/1.10 and removed backport-pending/1.10 labels May 28, 2021

aanm moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.1 Jun 16, 2021

aanm mentioned this pull request Jun 16, 2021

Prepare for release v1.10.1 #16544

Merged

eyanulis deleted the gh16019-exclude-cilium-interfaces-nodeport branch August 30, 2021 05:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

eyanulis commented May 12, 2021

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

brb left a comment •

edited

Loading

brb May 12, 2021

eyanulis May 13, 2021

brb May 12, 2021

eyanulis May 13, 2021

jibi May 12, 2021

eyanulis May 13, 2021

eyanulis commented May 13, 2021

brb left a comment

jibi commented May 17, 2021

eyanulis commented May 18, 2021

jibi commented May 19, 2021

joamaki commented May 19, 2021

joamaki commented May 19, 2021

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

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

Conversation

eyanulis commented May 12, 2021

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

brb left a comment • edited Loading

Choose a reason for hiding this comment

brb May 12, 2021

Choose a reason for hiding this comment

eyanulis May 13, 2021

Choose a reason for hiding this comment

brb May 12, 2021

Choose a reason for hiding this comment

eyanulis May 13, 2021

Choose a reason for hiding this comment

jibi May 12, 2021

Choose a reason for hiding this comment

eyanulis May 13, 2021

Choose a reason for hiding this comment

eyanulis commented May 13, 2021

brb left a comment

Choose a reason for hiding this comment

jibi commented May 17, 2021

eyanulis commented May 18, 2021

jibi commented May 19, 2021

joamaki commented May 19, 2021

joamaki commented May 19, 2021

brb left a comment •

edited

Loading