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

route: sort by priority to identify the default one #18564

Merged
merged 1 commit into from
Jan 24, 2022

Conversation

jibi
Copy link
Member

@jibi jibi commented Jan 20, 2022

In lookupDefaultRoute(), to determine the default route for a given IP
family we list all routes filtering just by the 0.0.0.0 destination.

On some EKS setups where multilple NICs are attached to an instance,
this is not enough to identify a single route, since each interface has
its own default route.

The following example illustrates the problem:

$ ks exec ds/cilium -- ip route list
default via 10.100.1.1 dev eth0
default via 10.100.255.65 dev eth1 metric 10001
169.254.169.254 dev eth0

This will cause the agent to report a fatal error, as it can't find a
single gateway:

cilium-agent level=fatal msg="Error while creating daemon" error="unable to install ip rule for ENI multi-node NodePort: failed to find interface with default route: Found default routes with different netdev ifindices: 2 vs 3" subsys=daemon

To address this issue, this commit changes the logic of
lookupDefaultRoute() to pick the one with the lowest priority.

@jibi jibi added release-note/bug This PR fixes an issue in a previous release of Cilium. area/eni Impacts ENI based IPAM. labels Jan 20, 2022
@jibi jibi requested review from gandro, a team and joamaki January 20, 2022 22:34
@jibi jibi force-pushed the pr/jibi/fix-lookup-default-route-multiple-enis branch from a059b83 to b71aeec Compare January 21, 2022 13:23
@jibi jibi requested a review from joamaki January 21, 2022 13:45
@jibi jibi force-pushed the pr/jibi/fix-lookup-default-route-multiple-enis branch from b71aeec to 35b0cd5 Compare January 21, 2022 14:40
@jibi jibi changed the title route: filter by priority 0 to determine default route route: sort by priority do identify the default one Jan 21, 2022
In lookupDefaultRoute(), to determine the default route for a given IP
family we list all routes filtering just by the 0.0.0.0 destination.

On some EKS setups where multilple NICs are attached to an instance,
this is not enough to identify a single route, since each interface has
its own default route.

The following example illustrates the problem:

    $ ks exec ds/cilium -- ip route list
    default via 10.100.1.1 dev eth0
    default via 10.100.255.65 dev eth1 metric 10001
    169.254.169.254 dev eth0

This will cause the agent to report a fatal error, as it can't find a
single gateway:

    cilium-agent level=fatal msg="Error while creating daemon" error="unable to install ip rule for ENI multi-node NodePort: failed to find interface with default route: Found default routes with different netdev ifindices: 2 vs 3" subsys=daemon

To address this issue, this commit changes the logic of
lookupDefaultRoute() to pick the one with the lowest priority.

Signed-off-by: Gilberto Bertin <gilberto@isovalent.com>
@jibi jibi force-pushed the pr/jibi/fix-lookup-default-route-multiple-enis branch from 35b0cd5 to dacb326 Compare January 21, 2022 14:41
@jibi jibi changed the title route: sort by priority do identify the default one route: sort by priority to identify the default one Jan 21, 2022
@jibi jibi closed this Jan 21, 2022
@jibi jibi reopened this Jan 21, 2022
@maintainer-s-little-helper
Copy link

Job 'Cilium-PR-K8s-1.23-kernel-net-next' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sHealthTest cilium-health

Failure Output

FAIL: failed to ensure kubectl version: failed to run kubectl version

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.23-kernel-net-next so I can create a new GitHub issue to track it.

@jibi
Copy link
Member Author

jibi commented Jan 21, 2022

/test

Job 'Cilium-PR-K8s-1.23-kernel-net-next' hit: #18566 (97.36% similarity)

@jibi jibi added needs-backport/1.11 ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Jan 24, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.11.2 Jan 24, 2022
@kkourt kkourt merged commit f9a6011 into master Jan 24, 2022
@kkourt kkourt deleted the pr/jibi/fix-lookup-default-route-multiple-enis branch January 24, 2022 12:08
@glibsm glibsm added backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed backport-pending/1.11 labels Jan 30, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport done to v1.11 in 1.11.2 Jan 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/eni Impacts ENI based IPAM. backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. 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.11.2
Backport done to v1.11
Development

Successfully merging this pull request may close these issues.

None yet

5 participants