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

loader: Fix tunneling when device is set without NodePort #11980

Merged
merged 1 commit into from Jun 9, 2020

Conversation

pchaigno
Copy link
Member

@pchaigno pchaigno commented Jun 9, 2020

Many tests are failing when configuring a device while BPF-based NodePort is disabled (cf. #11972). A bug in the loader causes it to call init.sh with the mode set to direct instead of e.g., vxlan. The cilium_vxlan is then not created (or even removed if it existed) which causes connectivity disruptions in multiple scenarios (mostly multi-node).

This commit fixes the wrong assumption that multiple devices are only set when NodePort is enabled. That assumption might have been correct when that code was introduced (in 3a83623).

Fixes: #11972
Fixes: 3a83623 ("bpf: add support for local NodePort via tunnel")
/cc @brb @borkmann

@pchaigno pchaigno added sig/loader Impacts the loading of BPF programs into the kernel. release-note/bug This PR fixes an issue in a previous release of Cilium. needs-backport/1.8 labels Jun 9, 2020
@pchaigno pchaigno added this to the 1.8 milestone Jun 9, 2020
@pchaigno pchaigno requested review from brb and a team June 9, 2020 11:39
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Jun 9, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.8.0 Jun 9, 2020
Copy link
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.

Nice find, thanks!

pkg/datapath/loader/base.go Outdated Show resolved Hide resolved
Many tests are failing when configuring a device while BPF-based NodePort
is disabled (cf. #11972). A bug in the loader causes it to call init.sh
with the mode set to direct instead of e.g., vxlan. The cilium_vxlan is
then not created (or even removed if it existed) which causes
connectivity disruptions in multiple scenarios (mostly multi-node).

This commit fixes the wrong assumption that multiple devices are only
set when NodePort is enabled. That assumption might have been correct
when that code was introduced (in 3a83623).

Fixes: #11972
Fixes: 3a83623 ("bpf: add support for local NodePort via tunnel")
Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno pchaigno force-pushed the pr/pchaigno/fix-set-device-not-nodeport branch from c161821 to 5615c8b Compare June 9, 2020 11:52
@pchaigno pchaigno requested a review from brb June 9, 2020 11:54
@pchaigno
Copy link
Member Author

pchaigno commented Jun 9, 2020

test-me-please

@coveralls
Copy link

Coverage Status

Coverage increased (+0.009%) to 37.018% when pulling 5615c8b on pr/pchaigno/fix-set-device-not-nodeport into f0a9f85 on master.

@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 Jun 9, 2020
@borkmann borkmann merged commit 749d03a into master Jun 9, 2020
1.8.0 automation moved this from In progress to Merged Jun 9, 2020
@borkmann borkmann deleted the pr/pchaigno/fix-set-device-not-nodeport branch June 9, 2020 14:41
@aanm aanm mentioned this pull request Jun 11, 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.0 Jun 11, 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.0 Jun 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. sig/loader Impacts the loading of BPF programs into the kernel.
Projects
No open projects
1.8.0
  
Merged
1.8.0
Backport done to v1.8
Development

Successfully merging this pull request may close these issues.

Setting devices without NodePort breaks multi-node connectivity
6 participants