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

connectivity: Misc enc test case changes #2055

Merged
merged 7 commits into from
Nov 2, 2023
Merged

connectivity: Misc enc test case changes #2055

merged 7 commits into from
Nov 2, 2023

Conversation

brb
Copy link
Member

@brb brb commented Oct 18, 2023

No description provided.

@brb brb temporarily deployed to ci October 18, 2023 14:34 — with GitHub Actions Inactive
@brb brb force-pushed the pr/brb/conn-enc-improvements branch from 639bbda to 8b5a580 Compare October 19, 2023 09:23
@brb brb temporarily deployed to ci October 19, 2023 09:23 — with GitHub Actions Inactive
@brb brb force-pushed the pr/brb/conn-enc-improvements branch from 8b5a580 to 03246d7 Compare October 19, 2023 11:25
@brb brb temporarily deployed to ci October 19, 2023 11:26 — with GitHub Actions Inactive
@brb brb temporarily deployed to ci October 19, 2023 11:45 — with GitHub Actions Inactive
@brb brb changed the title WIP: misc connectivity enc improvements connectivity: Misc enc test case changes Oct 19, 2023
@brb brb requested a review from giorio94 October 19, 2023 12:13
giorio94
giorio94 reviewed Oct 20, 2023
View reviewed changes
Copy link
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

Cool! A few comment inline.

connectivity/tests/encryption.go Outdated
// a packet to the tunnel netdev, and only afterwards redirects to the WG netdev
// for the encryption.
func getInterNodeIface(ctx context.Context, t *check.Test,
client, clientHost, server, serverHost *check.Pod, ipFam features.IPFamily) string {
Copy link
Member

Choose a reason for hiding this comment

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

Nit. I wonder if it could make sense to rename client and server to source and destination for additional clarity, given that this function is used to determine the interface on both sides, with swapped parameters.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will do that in a separate PR!

connectivity/tests/encryption.go Outdated Show resolved Hide resolved
connectivity/tests/encryption.go
func getFilter(ctx context.Context, t *check.Test, client, clientHost *check.Pod,
server, serverHost *check.Pod,
Copy link
Member

Choose a reason for hiding this comment

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

Same here about the possibility of renaming client and server to source and destination for additional clarity.

connectivity/tests/encryption.go Outdated Show resolved Hide resolved
connectivity/tests/encryption.go Outdated Show resolved Hide resolved
connectivity/tests/encryption.go Outdated
Comment on lines 128 to 126
if enc, ok := t.Context().Feature(features.EncryptionPod); tunnelEnabled && ok &&
enc.Enabled && enc.Mode == "wireguard" {
Copy link
Member

Choose a reason for hiding this comment

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

I guess that the version compatibility check would be appropriate also here.

connectivity/tests/encryption.go Outdated Show resolved Hide resolved
connectivity/tests/encryption.go Outdated Show resolved Hide resolved
@brb brb force-pushed the pr/brb/conn-enc-improvements branch from 49d7775 to b3e5158 Compare October 20, 2023 14:07
@brb brb temporarily deployed to ci October 20, 2023 14:07 — with GitHub Actions Inactive
@brb brb force-pushed the pr/brb/conn-enc-improvements branch from b3e5158 to c7b7ce7 Compare October 26, 2023 13:26
@brb brb temporarily deployed to ci October 26, 2023 13:26 — with GitHub Actions Inactive
@brb
connectivity: Rename getSourceAddressFilter to getFilter
ac8d829 
An upcoming commit will move the whole tcpdump filter derivation into
the function.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
connectivity: Derive dstFilter by calling getFilter
78ac466 
Instead of modifying the src filter. A subsequent commit will move
the filter derivation logic into getFilter.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
connecitivity: Set proto filter in getFilter
430181d 
Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
connectivity: Move dstFilter derivation to getFilter
ce840c1 
Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
connectivity: Change iface derivation for WG w/ tunnel
84ce20b 
As the iface derivation now might depend on the src/dst host IP, pass
both host pods to testNoTrafficLeak() invocations in the node-to-node
test case.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
connectivity: Add special case to test WG w/ tunneling
da6ecc7 
Unfortunately, this makes the inverse tests (assertNoLeaks=false) noop
for this special case. But an additional check for leaks is going to be
added outside the CLI.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb brb force-pushed the pr/brb/conn-enc-improvements branch from c7b7ce7 to ce1da30 Compare October 31, 2023 15:43
@brb brb temporarily deployed to ci October 31, 2023 15:43 — with GitHub Actions Inactive
@brb brb requested a review from meyskens October 31, 2023 15:49
@brb brb force-pushed the pr/brb/conn-enc-improvements branch from ce1da30 to 3e4fb5c Compare October 31, 2023 16:45
@brb brb temporarily deployed to ci October 31, 2023 16:45 — with GitHub Actions Inactive
@brb brb requested a review from giorio94 October 31, 2023 20:22
@brb brb marked this pull request as ready for review October 31, 2023 20:22
@brb brb requested review from a team as code owners October 31, 2023 20:22
@brb brb force-pushed the pr/brb/conn-enc-improvements branch from 3e4fb5c to 7f47895 Compare November 1, 2023 06:57
@brb brb temporarily deployed to ci November 1, 2023 06:57 — with GitHub Actions Inactive
@brb brb marked this pull request as draft November 1, 2023 06:59
@brb brb force-pushed the pr/brb/conn-enc-improvements branch from 7f47895 to 23556b9 Compare November 1, 2023 07:12
@brb brb temporarily deployed to ci November 1, 2023 07:12 — with GitHub Actions Inactive
@brb brb mentioned this pull request Nov 1, 2023
Merged
@brb
connectivity: Add feature detection for WG encapsulation
888883f 
Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb brb force-pushed the pr/brb/conn-enc-improvements branch from 23556b9 to 888883f Compare November 1, 2023 10:06
@brb brb temporarily deployed to ci November 1, 2023 10:06 — with GitHub Actions Inactive
@brb brb requested a review from mhofstetter November 1, 2023 10:07
@brb brb marked this pull request as ready for review November 1, 2023 10:07
@brb
Copy link
Member Author

brb commented Nov 1, 2023

@giorio94 PTAL. The main change since your last review is 888883f.

mhofstetter
mhofstetter approved these changes Nov 1, 2023
View reviewed changes
Copy link
Member

@mhofstetter mhofstetter left a comment

Choose a reason for hiding this comment

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

cilium/cli ✔️

giorio94
giorio94 approved these changes Nov 2, 2023
View reviewed changes
Copy link
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

/lgtm

connectivity/tests/encryption.go Show resolved Hide resolved
@brb brb added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. area/CI Continuous Integration testing issue or flake labels Nov 2, 2023
meyskens
meyskens approved these changes Nov 2, 2023
View reviewed changes
Copy link
Member

@meyskens meyskens left a comment

Choose a reason for hiding this comment

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

LGTM

@aanm aanm merged commit 0f51386 into main Nov 2, 2023
17 of 19 checks passed
@aanm aanm deleted the pr/brb/conn-enc-improvements branch November 2, 2023 11:05
brb added a commit to cilium/cilium that referenced this pull request Nov 6, 2023
@brb
ci-e2e: Enable WG encapsulation tests
0c37924 
Also, bump the CLI to include those tests [1].

[1]: cilium/cilium-cli#2055

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb brb mentioned this pull request Nov 6, 2023
Merged
brb added a commit to cilium/cilium that referenced this pull request Nov 6, 2023
@brb
ci-e2e,ipsec: Bump CLI vsn
3c4cd2a 
To include the encryption suite changes [1].

[1]: cilium/cilium-cli#2055

Signed-off-by: Martynas Pumputis <m@lambda.lt>
brb added a commit to cilium/cilium that referenced this pull request Nov 6, 2023
@brb
ci-e2e,ipsec: Bump CLI vsn
d4868e5 
To include the encryption suite changes [1].

[1]: cilium/cilium-cli#2055

Signed-off-by: Martynas Pumputis <m@lambda.lt>
brb added a commit to cilium/cilium that referenced this pull request Nov 6, 2023
@brb
ci-e2e,ipsec: Bump CLI vsn
f72480f 
To include the encryption suite changes [1].

[1]: cilium/cilium-cli#2055

Signed-off-by: Martynas Pumputis <m@lambda.lt>
brb added a commit to cilium/cilium that referenced this pull request Nov 6, 2023
@brb
ci-e2e,ipsec: Bump CLI vsn
72c22af 
To include the encryption suite changes [1].

[1]: cilium/cilium-cli#2055

Signed-off-by: Martynas Pumputis <m@lambda.lt>
brb added a commit to cilium/cilium that referenced this pull request Nov 6, 2023
@brb
ci-e2e: Enable WG encapsulation tests
3d9f2c9 
Also, bump the CLI to include those tests [1].

[1]: cilium/cilium-cli#2055

Signed-off-by: Martynas Pumputis <m@lambda.lt>
brb added a commit to cilium/cilium that referenced this pull request Nov 7, 2023
@brb
ci-e2e,ipsec: Bump CLI vsn
f92ffd1 
To include the encryption suite changes [1] [2]

[1]: cilium/cilium-cli#2055
[2]: cilium/cilium-cli#2089

Signed-off-by: Martynas Pumputis <m@lambda.lt>
aanm pushed a commit to cilium/cilium that referenced this pull request Nov 7, 2023
@brb @aanm
ci-e2e,ipsec: Bump CLI vsn
0af609f 
To include the encryption suite changes [1] [2]

[1]: cilium/cilium-cli#2055
[2]: cilium/cilium-cli#2089

Signed-off-by: Martynas Pumputis <m@lambda.lt>
pjablonski123 pushed a commit to pjablonski123/cilium that referenced this pull request Dec 15, 2023
@brb @pjablonski123
ci-e2e,ipsec: Bump CLI vsn
0b6d0c5 
To include the encryption suite changes [1] [2]

[1]: cilium/cilium-cli#2055
[2]: cilium/cilium-cli#2089

Signed-off-by: Martynas Pumputis <m@lambda.lt>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@meyskens meyskens meyskens approved these changes

@mhofstetter mhofstetter mhofstetter approved these changes

@giorio94 giorio94 giorio94 approved these changes

@jschwinger233 jschwinger233 Awaiting requested review from jschwinger233 jschwinger233 is a code owner automatically assigned from cilium/sig-encryption

Assignees
No one assigned
Labels
area/CI Continuous Integration testing issue or flake ready-to-merge This PR has passed all tests and received consensus from code owners to merge.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@brb @meyskens @mhofstetter @giorio94 @aanm