connectivity: Add encryption tests #1241

brb · 2022-11-29T11:31:15Z

Please see commit msgs.

Example run - https://github.com/cilium/cilium/actions/runs/3573711857/jobs/6008107171.

Going to be used by the DP conformance suite to test WireGuard/IPsec. Signed-off-by: Martynas Pumputis <[email protected]>

This is going to be used for exec'ing from a concurrent goroutine, and then reading std{out,err} from another one. Signed-off-by: Martynas Pumputis <[email protected]>

pchaigno

Nice commit history and comments! ❤️

LGTM, only nits below.

Out of scope, but would be nice to check the same with a service translation on the path.

connectivity/tests/encryption.go

brb · 2022-12-05T15:31:40Z

Out of scope, but would be nice to check the same with a service translation on the path.

Yep, it's planned with cilium/cilium#19401.

connectivity/check/features.go

connectivity/tests/encryption.go

Going to be used by the upcoming encryption tests. Signed-off-by: Martynas Pumputis <[email protected]>

Signed-off-by: Martynas Pumputis <[email protected]>

The pod-to-pod encryption test checks connectivity between pods, and whether no unencrypted traffic is leaked. All subtle details are in the code comments. Signed-off-by: Martynas Pumputis <[email protected]>

brb · 2022-12-06T14:34:08Z

EKS jobs are failing due to the provisioning issues. Previously, all tests passed. Applied only minor changes. Marking as ready to merge.

brb · 2022-12-07T15:32:42Z

@tklauser from @cilium/cli will do a review before merging.

tklauser

Thanks for the clean commit history and for adding explanatory comments in the test ❤️

connectivity/tests/encryption.go

joamaki · 2022-12-07T15:26:31Z

connectivity/check/context.go

@@ -623,3 +623,8 @@ func (ct *ConnectivityTest) K8sClient() *k8s.Client {
 func (ct *ConnectivityTest) NodesWithoutCilium() []string {
 	return ct.nodesWithoutCilium
 }
+
+func (ct *ConnectivityTest) Feature(f Feature) (FeatureStatus, bool) {
+	s, ok := ct.features[f]


return ct.feature[f] works too.

Unfortunately, this won't work:

# github.com/cilium/cilium-cli/connectivity/check connectivity/check/context.go:628:9: not enough return values have (FeatureStatus) want (FeatureStatus, bool)

Also see golang/go#6230.

joamaki · 2022-12-07T16:02:12Z

connectivity/tests/encryption.go

+		iface = "cilium_" + tunnelFeat.Mode // E.g. cilium_vxlan
+	} else {
+		cmd := []string{"/bin/sh", "-c",
+			fmt.Sprintf("ip -o r g %s from %s | grep -oP '(?<=dev )[^ ]+'",


going via ip -json may sometimes by nicer way to do this, but I'm fine with this as long as it doesn't get any more complex.

Yep, but unfortunately this brings the runtime dependency for jq.

joamaki · 2022-12-08T15:28:16Z

connectivity/tests/encryption.go

+		// might terminate the tcpdump process before it gets a chance to dump
+		// its captures.
+		cmd := []string{
+			"tcpdump", "-i", iface, "--immediate-mode", "-w", "/tmp/foo.pcap",


Would be better to generate a unique(ish) filename for this and mention that this is part of the cilium-cli encryption test. "cilium-test-encryption-XXX.pcap" where XXX is unix time?

joamaki · 2022-12-08T15:29:02Z

connectivity/tests/encryption.go

+	// Redirect stderr to /dev/null, as tcpdump logs to stderr, and ExecInPod
+	// will return an error if any char is written to stderr. Anyway, the count
+	// is written to stdout.
+	cmd := []string{"/bin/sh", "-c", "tcpdump -r /tmp/foo.pcap --count 2>/dev/null"}


add && rm -f /tmp/foo.pcap to not leave the file around.

Hm, don't we want to keep it around so that the workflows can collect it as part of the artifacts? It might be useful to debug failures.

The encryption test details - cilium/cilium-cli#1241. Signed-off-by: Martynas Pumputis <[email protected]>

[ upstream commit 843c072 ] The encryption test details - cilium/cilium-cli#1241. Signed-off-by: Martynas Pumputis <[email protected]> Signed-off-by: Joe Stringer <[email protected]>

When running in ENI mode, the outgoing interface of pod originating traffic is different from the one that would be used by host originating traffic towards the same destination. This breaks the current pod-to-pod encryption validation, as the source interface for the tcpdump filter is determined based on the routes towards the given destination only. Let's update the source interface determination to additionally consider the source address. This approach had been initially suggested by Paul Chaignon in cilium#1241, but then reverted in [1] because `ip route get` returns and error in case the `from` address is not assigned to any local interface. We can work around this by specifying an input interface: let's use lo as it should be always present. [1]: 2fc0835 ("connectivity: Fix iface derivation in encrypt tests") Signed-off-by: Marco Iorio <[email protected]>

When running in ENI mode, the outgoing interface of pod originating traffic is different from the one that would be used by host originating traffic towards the same destination. This breaks the current pod-to-pod encryption validation, as the source interface for the tcpdump filter is determined based on the routes towards the given destination only. Let's update the source interface determination to additionally consider the source address. This approach had been initially suggested by Paul Chaignon in #1241, but then reverted in [1] because `ip route get` returns and error in case the `from` address is not assigned to any local interface. We can work around this by specifying an input interface: let's use lo as it should be always present. [1]: 2fc0835 ("connectivity: Fix iface derivation in encrypt tests") Signed-off-by: Marco Iorio <[email protected]>

connectivity: Add feature to detect encryption

3eefe3d

Going to be used by the DP conformance suite to test WireGuard/IPsec. Signed-off-by: Martynas Pumputis <[email protected]>

brb temporarily deployed to ci November 29, 2022 11:31 Inactive

k8s: Add ExecInPodWithWriters

ff7f687

This is going to be used for exec'ing from a concurrent goroutine, and then reading std{out,err} from another one. Signed-off-by: Martynas Pumputis <[email protected]>

brb force-pushed the pr/brb/ci-dp-encrypt branch from f41f715 to f6edf73 Compare November 29, 2022 13:37

brb temporarily deployed to ci November 29, 2022 13:37 Inactive

brb added the area/CI Continuous Integration testing issue or flake label Nov 29, 2022

brb requested a review from pchaigno November 29, 2022 13:38

brb marked this pull request as ready for review November 29, 2022 13:39

brb requested review from a team as code owners November 29, 2022 13:39

brb requested review from tommyp1ckles and joamaki November 29, 2022 13:39

brb force-pushed the pr/brb/ci-dp-encrypt branch from f6edf73 to 967e017 Compare November 29, 2022 13:44

brb temporarily deployed to ci November 29, 2022 13:44 Inactive

pchaigno approved these changes Nov 30, 2022

View reviewed changes

brb force-pushed the pr/brb/ci-dp-encrypt branch from 967e017 to 80b0cff Compare December 5, 2022 15:26

brb temporarily deployed to ci December 5, 2022 15:26 Inactive

tommyp1ckles approved these changes Dec 5, 2022

View reviewed changes

connectivity/check/features.go Outdated Show resolved Hide resolved

connectivity/tests/encryption.go Outdated Show resolved Hide resolved

brb added 3 commits December 6, 2022 14:51

connectivity: Add feature to detect tunneling mode

ca40cdb

Going to be used by the upcoming encryption tests. Signed-off-by: Martynas Pumputis <[email protected]>

connectivity: Add feature getter

196c927

Signed-off-by: Martynas Pumputis <[email protected]>

connectivity: Add encryption test

be0cf10

The pod-to-pod encryption test checks connectivity between pods, and whether no unencrypted traffic is leaked. All subtle details are in the code comments. Signed-off-by: Martynas Pumputis <[email protected]>

brb force-pushed the pr/brb/ci-dp-encrypt branch from 80b0cff to be0cf10 Compare December 6, 2022 13:54

brb temporarily deployed to ci December 6, 2022 13:54 Inactive

brb added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. ci/hyperjump and removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Dec 6, 2022

tklauser self-requested a review December 7, 2022 15:37

tklauser reviewed Dec 7, 2022

View reviewed changes

connectivity/tests/encryption.go Show resolved Hide resolved

connectivity/tests/encryption.go Show resolved Hide resolved

brb requested a review from tklauser December 8, 2022 05:57

joamaki approved these changes Dec 8, 2022

View reviewed changes

brb added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Dec 10, 2022

tklauser approved these changes Dec 12, 2022

View reviewed changes

tklauser merged commit 998ef8a into master Dec 12, 2022

tklauser deleted the pr/brb/ci-dp-encrypt branch December 12, 2022 09:59

brb added a commit to cilium/cilium that referenced this pull request Dec 12, 2022

gh/workflows: Enable encryption tests in conformance DP

3b7ddd8

The encryption test details - cilium/cilium-cli#1241. Signed-off-by: Martynas Pumputis <[email protected]>

brb added a commit to cilium/cilium that referenced this pull request Dec 14, 2022

gh/workflows: Enable encryption tests in conformance DP

7e1c22a

The encryption test details - cilium/cilium-cli#1241. Signed-off-by: Martynas Pumputis <[email protected]>

pchaigno pushed a commit to cilium/cilium that referenced this pull request Dec 14, 2022

gh/workflows: Enable encryption tests in conformance DP

843c072

The encryption test details - cilium/cilium-cli#1241. Signed-off-by: Martynas Pumputis <[email protected]>

brb added a commit to cilium/cilium that referenced this pull request Dec 20, 2022

gh/workflows: Enable encryption tests in conformance DP

5f240b9

The encryption test details - cilium/cilium-cli#1241. Signed-off-by: Martynas Pumputis <[email protected]>

brb mentioned this pull request Feb 15, 2023

Add encryption validation in connectivity test #50

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

connectivity: Add encryption tests #1241

connectivity: Add encryption tests #1241

brb commented Nov 29, 2022 •

edited

Loading

pchaigno left a comment

brb commented Dec 5, 2022

brb commented Dec 6, 2022

brb commented Dec 7, 2022

tklauser left a comment

joamaki Dec 7, 2022

tklauser Dec 12, 2022

joamaki Dec 7, 2022

brb Dec 9, 2022

joamaki Dec 8, 2022

joamaki Dec 8, 2022

pchaigno Dec 9, 2022

connectivity: Add encryption tests #1241

connectivity: Add encryption tests #1241

Conversation

brb commented Nov 29, 2022 • edited Loading

pchaigno left a comment

Choose a reason for hiding this comment

brb commented Dec 5, 2022

brb commented Dec 6, 2022

brb commented Dec 7, 2022

tklauser left a comment

Choose a reason for hiding this comment

joamaki Dec 7, 2022

Choose a reason for hiding this comment

tklauser Dec 12, 2022

Choose a reason for hiding this comment

joamaki Dec 7, 2022

Choose a reason for hiding this comment

brb Dec 9, 2022

Choose a reason for hiding this comment

joamaki Dec 8, 2022

Choose a reason for hiding this comment

joamaki Dec 8, 2022

Choose a reason for hiding this comment

pchaigno Dec 9, 2022

Choose a reason for hiding this comment

brb commented Nov 29, 2022 •

edited

Loading