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.

Sign up for GitHub

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

Jump to bottom

datapath/test: Do not SNAT for WORLD_ID and enable BPF masquerading by default in CI #11426

Merged
merged 8 commits into from May 19, 2020
Merged

datapath/test: Do not SNAT for WORLD_ID and enable BPF masquerading by default in CI #11426

merged 8 commits into from May 19, 2020

Conversation

brb
Copy link
Member

@brb brb commented May 8, 2020 •
edited

See commit msgs.

Depends on #11572 (let's merge this PR, and then close #11572).

Partially_fix #11442

@brb brb added the area/CI-improvement Topic or proposal to improve the Continuous Integration workflow label May 8, 2020
@brb brb requested a review from a team as a code owner May 8, 2020 15:49
@maintainer-s-little-helper
Copy link

Please set the appropriate release note label.

1 similar comment
@maintainer-s-little-helper
Copy link

Please set the appropriate release note label.

@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 May 8, 2020
@brb brb added the release-note/ci This PR makes changes to the CI. label May 8, 2020
@brb
Copy link
Member Author

brb commented May 8, 2020

test-me-please

@brb
Copy link
Member Author

brb commented May 8, 2020

test-focus K8sDatapath*

@brb
Copy link
Member Author

brb commented May 8, 2020

test-me-please

2 similar comments
@brb
Copy link
Member Author

brb commented May 9, 2020

test-me-please

@nebril
Copy link
Member

nebril commented May 11, 2020

test-me-please

@brb
Copy link
Member Author

brb commented May 11, 2020

I might need to fix #11464 before this PR can be merged (otherwise, I might introduce flakes)

@maintainer-s-little-helper
Copy link

Commit 961b375843569b39ae8123d32bced835420c8afe 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

@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 May 11, 2020
@coveralls
Copy link

coveralls commented May 11, 2020 •
edited

Coverage Status

Coverage increased (+0.003%) to 37.024% when pulling f99682d on pr/brb/ci-bpf-masq-by-default into c99b97a on master.

@brb brb force-pushed the pr/brb/ci-bpf-masq-by-default branch from 961b375 to bf48cac Compare May 11, 2020 16:36
@brb brb requested a review from a team May 11, 2020 16:36
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 11, 2020
@brb
Copy link
Member Author

brb commented May 11, 2020

test-me-please

@brb brb force-pushed the pr/brb/ci-bpf-masq-by-default branch from bf48cac to 9740e82 Compare May 12, 2020 08:28
@brb
Copy link
Member Author

brb commented May 12, 2020

test-me-please

1 similar comment
@brb
Copy link
Member Author

brb commented May 12, 2020

test-me-please

@brb brb force-pushed the pr/brb/ci-bpf-masq-by-default branch from 4a384a1 to 12b58cb Compare May 13, 2020 14:13
@brb
Copy link
Member Author

brb commented May 13, 2020

test-me-please

@brb brb changed the title test: Enable BPF masquerading test: Enable BPF masquerading by default May 13, 2020
@brb brb force-pushed the pr/brb/ci-bpf-masq-by-default branch from 12b58cb to e44ad57 Compare May 13, 2020 14:56
@brb brb force-pushed the pr/brb/ci-bpf-masq-by-default branch from 457d243 to d304749 Compare May 15, 2020 15:30
@brb
Copy link
Member Author

brb commented May 15, 2020

retest-net-next

@brb
Copy link
Member Author

brb commented May 15, 2020

test-focus K8sKubeProxyFree*

@brb
Copy link
Member Author

brb commented May 15, 2020

test-me-please

@brb
test: Enable BPF masquerading
9e10595 
Enable BPF masquerading for the {{4.19,net-next},no-kube-proxy} CI jobs.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb brb force-pushed the pr/brb/ci-bpf-masq-by-default branch from d304749 to 95b44a6 Compare May 15, 2020 20:19
@brb
Copy link
Member Author

brb commented May 15, 2020

retest-net-next

@brb
Copy link
Member Author

brb commented May 17, 2020

test-focus K8sKubeProxy*

1 similar comment
@brb
Copy link
Member Author

brb commented May 18, 2020

test-focus K8sKubeProxy*

brb added 7 commits May 18, 2020 09:11
@brb
test: Disable BPF masq for K8sKubeProxyFreeMatrix
9f2b671 
Disable the BPF masq in the vxlan tests until PublicInterfaceName is
decluttered. The communication between pod and remote node has to be
SNAT'd in the case of vxlan, which is currently not feasible, as
bpf_netdev is loaded only on PublicInterfaceName.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
test: Do not set devices in DatapathConfiguration BPF masq tests
a8ba80c 
As we enabled the BPF masq by default, the correct devices (default and
private) are already set by DeployCilium().

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
test: Set single --device in K8sUpdates test
33258c7 
Cilium < v1.8 doesn't support multi-dev, so installing e.g. v1.7 with
multiple devices (set by default in overwriteHelmOptions()) will crash
cilium-agent.

Therefore, in the upgrade test use a single device.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
test: Get rid of dedicated BPF masq test cases
675b384 
As BPF masq is enabled by default, we don't need a dedicated deployments
(and test cases) to test the feature.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
datapath: Do not SNAT if dst id is WORLD_ID
0ef19e9 
Unfortunately, we cannot SNAT a packet from a local endpoint only if
dst sec id is WORLD_ID. The problem with this is that in the case of
an FQDN policy, a world destination can get its own sec id, which makes
a packet destined to such target to bypass the check, and bypass the
SNAT.

To fix this, we SNAT a packet from a local endpoint if dst is neither
from native-routing-cidr nor REMOTE_HOST_ID.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
datapath: Set ipv4-alloc-range if native-routing-cidr is not available
620907e 
Since we changed the SNAT logic in the previous commit, we need to set
IPV4_SNAT_EXCLUSION_DST_CIDR (prev. IPV4_NATIVE_ROUTING_CIDR) in any
case, otherwise traffic between pods on different nodes will be SNAT'd.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
datapath: Fix removal of bpf_netdev
f99682d 
The name of a bpf_netdev object file which is attached to a native
device has changed its name - from "bpf_netdev.o" to
"bpf_netdev_${NATIVE_DEV_IFACE}.o". E.g.:

$ sudo tc filter show dev enp0s8 ingress
filter protocol all pref 1 bpf chain 0
filter protocol all pref 1 bpf chain 0 handle 0x1 bpf_netdev_enp0s8.o:[from-netdev] direct-action not_in_hw id 982 tag feca2ca7f6f80c7e jited

The change of the name broke the removal of bpf_netdev from previously
used native devs.

Fixes: a695f53 ("Endpoint for host")
Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb brb force-pushed the pr/brb/ci-bpf-masq-by-default branch from 95b44a6 to f99682d Compare May 18, 2020 07:24
@brb
Copy link
Member Author

brb commented May 18, 2020

retest-net-next

@brb
Copy link
Member Author

brb commented May 18, 2020

retest-4.19

@brb
Copy link
Member Author

brb commented May 18, 2020

retest-4.9

@brb
Copy link
Member Author

brb commented May 18, 2020

retest-runtime

borkmann
borkmann approved these changes May 18, 2020
View reviewed changes
Copy link
Member

@borkmann borkmann left a comment

Choose a reason for hiding this comment

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

as per discussion offline

@brb
Copy link
Member Author

brb commented May 18, 2020

retest-runtime

@brb
Copy link
Member Author

brb commented May 18, 2020

CI runtime failed:

[2020-05-18T15:26:35.027Z] �[91m�[1m[Fail] �[0m�[90mRuntimePrivilegedUnitTests �[0m�[91m�[1m[It] Run Tests �[0m
[2020-05-18T15:26:35.027Z] �[37m/home/jenkins/workspace/Cilium-PR-Runtime-4.9/runtime-gopath/src/github.com/cilium/cilium/test/runtime/privileged_tests.go:47�[0m
[2020-05-18T15:26:35.027Z] 
[2020-05-18T15:26:35.027Z] �[1m�[91mRan 72 of 393 Specs in 4779.419 seconds�[0m
[2020-05-18T15:26:35.028Z] �[1m�[91mFAIL!�[0m -- �[32m�[1m71 Passed�[0m | �[91m�[1m1 Failed�[0m | �[33m�[1m9 Pending�[0m | �[36m�[1m312 Skipped�[0m

@brb
Copy link
Member Author

brb commented May 18, 2020

retest-runtime

@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 May 18, 2020
@tklauser tklauser merged commit ed59264 into master May 19, 2020
1.8.0 automation moved this from In progress to Merged May 19, 2020
@tklauser tklauser deleted the pr/brb/ci-bpf-masq-by-default branch May 19, 2020 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@borkmann borkmann borkmann approved these changes

@nebril nebril nebril approved these changes

@qmonnet qmonnet qmonnet approved these changes

Assignees
No one assigned
Labels
area/CI-improvement Topic or proposal to improve the Continuous Integration workflow ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/ci This PR makes changes to the CI.
Projects
No open projects
1.8.0
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@brb @nebril @coveralls @borkmann @qmonnet @tklauser