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

routing: Fix incorrect interface selection for egress pod routes #17169

Merged
merged 2 commits into from Aug 17, 2021

Conversation

pchaigno
Copy link
Member

@pchaigno pchaigno commented Aug 16, 2021

The Configure method relies on the MAC address to select the proper egress interface for new pods (EKS and AKS). Several interfaces can however have the same MAC address, in which case the wrong interface may be selected.

To avoid this, we skip Linux slave devices during the lookup by MAC
address. Thus, in case of slave devices, we will select the master
device.

Fixes: #11073.

@pchaigno pchaigno 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. area/eni Impacts ENI based IPAM. area/azure Impacts Azure based IPAM. needs-backport/1.9 labels Aug 16, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.9.10 Aug 16, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.4 Aug 16, 2021
@pchaigno pchaigno force-pushed the fix-incorrect-interface-selection branch 3 times, most recently from fe03aae to 076c32f Compare August 16, 2021 13:29
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

This will break Azure, but it can work for ENI mode.

pkg/datapath/linux/routing/routing.go Outdated Show resolved Hide resolved
@maintainer-s-little-helper

This comment has been minimized.

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Aug 16, 2021
@pchaigno pchaigno force-pushed the fix-incorrect-interface-selection branch from 5c0f0ee to a1cfbff Compare August 16, 2021 19:35
@maintainer-s-little-helper

This comment has been minimized.

Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

Fix LGTM now, thanks! A few optional nits

pkg/datapath/linux/routing/routing.go Outdated Show resolved Hide resolved
pkg/datapath/linux/routing/routing.go Outdated Show resolved Hide resolved
pkg/datapath/linux/routing/routing.go Outdated Show resolved Hide resolved
@pchaigno pchaigno removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Aug 16, 2021
When setting up the Linux routes and rules for ENI and Azure, we lookup
the interfaces by their MAC addresses. In that case, we want to ensure a
single interface is found for the given MAC address. If several are
found, we throw an error now rather than to fail in a more obscure way
down the line.

Signed-off-by: Paul Chaignon <paul@cilium.io>
The Configure method relies on the MAC address to select the proper
egress interface for new pods (EKS and AKS). Several interfaces can
however have the same MAC address in the case of slave devices. In
such a case, the wrong interface may be selected.

To avoid this, we skip Linux slave devices during the lookup by MAC
address. Thus, in case of slave devices, we will select the master
device.

Fixes: 26308b6 ("Implement support for cilium-health in ENI mode")
Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno pchaigno marked this pull request as ready for review August 16, 2021 22:19
@pchaigno pchaigno requested review from a team and kkourt August 16, 2021 22:19
@pchaigno pchaigno force-pushed the fix-incorrect-interface-selection branch from a1cfbff to 354795b Compare August 16, 2021 22:20
@pchaigno
Copy link
Member Author

test-me-please

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Aug 17, 2021
@ti-mo ti-mo merged commit 3e24551 into cilium:master Aug 17, 2021
@pchaigno pchaigno deleted the fix-incorrect-interface-selection branch August 17, 2021 13:46
@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.10 Aug 17, 2021
@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.10 Aug 17, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.10.4 Aug 18, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.4 Aug 19, 2021
@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.10 Sep 1, 2021
@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.10 Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/azure Impacts Azure based IPAM. area/eni Impacts ENI based IPAM. 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
No open projects
1.10.4
Backport done to v1.10
1.9.10
Backport done to v1.9
Development

Successfully merging this pull request may close these issues.

None yet

6 participants