Skip to content

Conversation

smagnani96
Copy link
Contributor

@smagnani96 smagnani96 commented Feb 17, 2025

Partially fixes (only for v2): #37675

Fixes: #37682

Fix egress device computation in cli connectivity pod-to-pod-encryption-v2 tests for AWS chaining mode.

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 17, 2025
@github-actions github-actions bot added cilium-cli This PR contains changes related with cilium-cli cilium-cli-exclusive This PR only impacts cilium-cli binary labels Feb 17, 2025
@smagnani96 smagnani96 force-pushed the pr/smagnani96/fix-awscni-pod-to-pod-encryption-v2 branch from a52b938 to df8e04b Compare February 17, 2025 15:53
@smagnani96 smagnani96 added the release-note/misc This PR makes changes that have no direct user impact. label Feb 17, 2025
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 17, 2025
@smagnani96
Copy link
Contributor Author

/test

@smagnani96
Copy link
Contributor Author

Had to introduce one commit to enable -d -v in connectivity test.
I need this to make sure that, when running AWS tests, they score green for the case we're interested into.

Since we observed a different egress device being selected in our tests when client -> echo-other-node (and not in other combinations), I would like such verbose output and run multiple times /ci-awscni.

That said, this commit is just for test purpose and won't be merged.

Copy link
Contributor

@ldelossa ldelossa left a comment

Choose a reason for hiding this comment

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

Other then my comment, this looks good.

@ldelossa
Copy link
Contributor

/test

@smagnani96 smagnani96 force-pushed the pr/smagnani96/fix-awscni-pod-to-pod-encryption-v2 branch from 33fd562 to 5d6780e Compare February 18, 2025 00:02
@jschwinger233
Copy link
Member

/test

@smagnani96 smagnani96 force-pushed the pr/smagnani96/fix-awscni-pod-to-pod-encryption-v2 branch from 5d6780e to da63240 Compare February 18, 2025 08:51
@smagnani96
Copy link
Contributor Author

smagnani96 commented Feb 18, 2025

/ci-awscni

Both 🟢, but looking at the debug messages it has always been selected client3 -> echo-other-node (using ens5 interface in both).
Let's run again and see if it selects client1 (in that case we should see a tcpdump filter on ens5 and one in ens6)

@smagnani96
Copy link
Contributor Author

smagnani96 commented Feb 18, 2025

/ci-awscni

Same in these runs.

@smagnani96
Copy link
Contributor Author

smagnani96 commented Feb 18, 2025

/ci-awscni

Hurray!
New run with ens6 device selected is 🟢 https://github.com/cilium/cilium/actions/runs/13388161375/job/37402556383#step:21:3349.

Dropping the last commit then, re-running CI and marking ready-for-review.

@smagnani96 smagnani96 force-pushed the pr/smagnani96/fix-awscni-pod-to-pod-encryption-v2 branch 2 times, most recently from b12bb9f to 9f58f85 Compare February 18, 2025 16:29
@smagnani96
Copy link
Contributor Author

/test

@ldelossa
Copy link
Contributor

latest https://github.com/cilium/cilium/actions/runs/13396664630/job/37418038669 failure is not due to this change. Its an existing flake in the AWS-CNI test.

@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 Feb 18, 2025
@smagnani96 smagnani96 marked this pull request as ready for review February 19, 2025 09:30
@smagnani96 smagnani96 requested a review from a team as a code owner February 19, 2025 09:30
@smagnani96 smagnani96 requested a review from pchaigno February 19, 2025 09:30
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

I'm really not confortable with the amount of logic introduced here to solve something that should be relatively trivial. Starting a discussion below 👇 to see if there's any other way.

@pchaigno pchaigno added dont-merge/discussion A discussion is ongoing and should be resolved before merging, regardless of reviews & tests status. and removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Feb 19, 2025
@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 Feb 19, 2025
@smagnani96
Copy link
Contributor Author

Both actions are 🟢

Removing the 3rd commit and going through the process again.

@smagnani96 smagnani96 force-pushed the pr/smagnani96/fix-awscni-pod-to-pod-encryption-v2 branch 2 times, most recently from 32a5ef5 to c76cfda Compare February 20, 2025 10:22
@pchaigno pchaigno removed the request for review from viktor-kurchenko February 20, 2025 10:47
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

Way easier to review now 🤭

@pchaigno pchaigno added release-note/ci This PR makes changes to the CI. and removed dont-merge/discussion A discussion is ongoing and should be resolved before merging, regardless of reviews & tests status. release-note/misc This PR makes changes that have no direct user impact. labels Feb 20, 2025
@smagnani96 smagnani96 force-pushed the pr/smagnani96/fix-awscni-pod-to-pod-encryption-v2 branch from c76cfda to 0bc9c5d Compare February 20, 2025 11:05
@pchaigno pchaigno enabled auto-merge February 20, 2025 11:11
This commit adjusts the current logic in `pod-to-pod-encryption-v2` for
retrieving the egress device from client-server pods and vice-versa.
Before the patch, we solely rely on `ip route get dstIP`, using the
returned device as egress. However, we noticed that in aws-cni chaining
mode, such returned device is not actually used. Instead, looking at the
routing tables in the server node, we found that there's a specific table
with a route that uses a different egress device for that particular IP.

Therefore, the final command to be issued should be:
`ip route get {dstIP} from {srcIP} iif cilium_host`.
The interface specified does not matter since, if present, the route
matching srcIP is not making use of the interface field to match.

The related issue in our previous tests is #37675.

We now fix this for pod-to-pod-encryption-v2, but we might want to adopt
the same logic for v1.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
This commit moves our current parsing logic in pod-to-pod-encryption-v2
from strings to json decode in go structures. This makes our impl a bit
more clear.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
@smagnani96 smagnani96 force-pushed the pr/smagnani96/fix-awscni-pod-to-pod-encryption-v2 branch from 0bc9c5d to 8eb0891 Compare February 20, 2025 14:38
@smagnani96
Copy link
Contributor Author

/test

@pchaigno pchaigno added this pull request to the merge queue Feb 20, 2025
Merged via the queue into main with commit c973554 Feb 20, 2025
210 checks passed
@pchaigno pchaigno deleted the pr/smagnani96/fix-awscni-pod-to-pod-encryption-v2 branch February 20, 2025 17:31
@maintainer-s-little-helper maintainer-s-little-helper bot added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cilium-cli This PR contains changes related with cilium-cli cilium-cli-exclusive This PR only impacts cilium-cli binary 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
None yet
Development

Successfully merging this pull request may close these issues.

CI: AWS-CNI: pod-to-pod-encryption-v2/pod-to-pod-encryption-v2/curl-ipv4 : Expected to capture packets, but none found. This check might be broken.
4 participants