Skip to content

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

Merged
borkmann merged 1 commit intomasterfrom
pr/pchaigno/fix-set-device-not-nodeport
Jun 9, 2020
Merged

loader: Fix tunneling when device is set without NodePort#11980
borkmann merged 1 commit intomasterfrom
pr/pchaigno/fix-set-device-not-nodeport

Conversation

@pchaigno
Copy link
Copy Markdown
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 area/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 a team and brb June 9, 2020 11:39
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.

Nice find, thanks!

Comment thread pkg/datapath/loader/base.go Outdated
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
Copy Markdown
Member Author

pchaigno commented Jun 9, 2020

test-me-please

Comment thread pkg/datapath/loader/base.go
@coveralls
Copy link
Copy Markdown

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
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/loader Impacts the loading of BPF programs into the kernel. 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

Development

Successfully merging this pull request may close these issues.

Setting devices without NodePort breaks multi-node connectivity

6 participants