Skip to content

Conversation

smagnani96
Copy link
Contributor

We now rely on tcpdump filters where we expect the default tunnel port to be in use (ex 8472 VXLAN, 6081 Geneve).
This means that CLI tests would fail in all environments where tunnel-port != default.
Let's retrieve the value from the Cilium ConfigMap.

@smagnani96 smagnani96 added kind/enhancement This would improve or streamline existing functionality. area/cli Impacts the command line interface of any command in the repository. release-note/ci This PR makes changes to the CI. labels Apr 2, 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 Apr 2, 2025
@smagnani96 smagnani96 force-pushed the pr/smagnani96/cli-fix-tunnelport branch 4 times, most recently from f25357a to 739d8d3 Compare April 3, 2025 00:24
@sayboras
Copy link
Member

sayboras commented Apr 3, 2025

/test

@smagnani96 smagnani96 marked this pull request as ready for review April 3, 2025 07:42
@smagnani96 smagnani96 requested review from a team as code owners April 3, 2025 07:42
@smagnani96 smagnani96 requested review from pchaigno and squeed April 3, 2025 07:42
@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 Apr 3, 2025
This commit adds TunnePort to the set of features detected from our CLI
during tests. This can be useful from tests to retrieve the current
active tunnel port. The `Enabled` is set to false in case `tunnel-port`
is not present in the ConfigMap, but its value `Mode` is always set to
either the provided port or the default one for the currently active
protocol.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
@smagnani96 smagnani96 force-pushed the pr/smagnani96/cli-fix-tunnelport branch from 739d8d3 to 2a6357d Compare April 3, 2025 22:27
This commit adjusts the current TunnelFilter logic to account for the
current tunnel-port in use when building tcpdump filters for the overlay.
This should prevent CLI tests from failing in all those environments where
tunnel-port is not the default one (ex. 8472 VXLAN, 6081 Geneve).

Given that now the filter `(udp and port tunnelPort)` make use of the
current tunnel port in use, we don't need anymore additional hacks:

- VXLAN: "udp[8:2] = 0x0800" would compare the first two bytes of an UDP
    payload against VXLAN commonly used flags.
- Geneve: we could not use the "geneve" filter, as it shifts offset of a
    filtered packet, which invalidates a filter matching on the outer headers.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
This commit adjusts the overlay tcpdump filters according to the previous
commit changes. Prior to this, we did not account for the current
tunnel-port in use, making this test to fail in all those environment
where the port is not the default one.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
@smagnani96 smagnani96 force-pushed the pr/smagnani96/cli-fix-tunnelport branch from 2a6357d to fbe31bd Compare April 3, 2025 23:10
@smagnani96
Copy link
Contributor Author

/test

@smagnani96 smagnani96 requested a review from squeed April 4, 2025 09:48
@squeed squeed added this pull request to the merge queue Apr 4, 2025
Merged via the queue into main with commit 8be31f4 Apr 4, 2025
223 checks passed
@squeed squeed deleted the pr/smagnani96/cli-fix-tunnelport branch April 4, 2025 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli Impacts the command line interface of any command in the repository. cilium-cli This PR contains changes related with cilium-cli cilium-cli-exclusive This PR only impacts cilium-cli binary kind/enhancement This would improve or streamline existing functionality. 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.

4 participants