Skip to content

cilium-cli: Test TLS with serverNames #35887

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
Merged
merged 1 commit into from
Nov 12, 2024
Merged

cilium-cli: Test TLS with serverNames #35887

merged 1 commit into from
Nov 12, 2024

Conversation

jrajahalme
Copy link
Member

@jrajahalme jrajahalme commented Nov 11, 2024 •
edited
Loading

Add new connectivity tests client-egress-l7-tls-headers-sni and client-egress-tls-sni to test SNI enforcement with and without TLS interception. Regressed patch releases are excluded from the new tests so that CI will not fail on downgrade tests.

Add coverage for SNI enforcement in cilium-cli connectivity tests.

@jrajahalme jrajahalme added area/CI Continuous Integration testing issue or flake release-note/bug This PR fixes an issue in a previous release of Cilium. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. dont-merge/blocked Another PR must be merged before this one. cilium-cli This PR contains changes related with cilium-cli labels Nov 11, 2024
@jrajahalme jrajahalme requested review from a team as code owners November 11, 2024 12:40
This was referenced Nov 11, 2024
Closed
Merged
@jrajahalme jrajahalme added the kind/regression This functionality worked fine before, but was broken in a newer release of Cilium. label Nov 11, 2024
@jrajahalme
Copy link
Member Author

/test

gandro
gandro reviewed Nov 11, 2024
View reviewed changes
@jrajahalme jrajahalme requested a review from gandro November 11, 2024 14:20
@jrajahalme jrajahalme force-pushed the pr/jrajahalme/cilium-cli-test-tls-with-sni-policy branch from b28175e to 4c99f81 Compare November 11, 2024 14:23
@jrajahalme
Copy link
Member Author

/test

gandro
gandro reviewed Nov 11, 2024
View reviewed changes
@jrajahalme jrajahalme removed the request for review from joamaki November 12, 2024 10:49
@jrajahalme jrajahalme force-pushed the pr/jrajahalme/cilium-cli-test-tls-with-sni-policy branch 2 times, most recently from 230052a to 2686216 Compare November 12, 2024 10:52
giorio94
giorio94 approved these changes Nov 12, 2024
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.

My previous concerns have been addressed, approving. Thanks!

@jrajahalme
Copy link
Member Author

Edit: looking at the manifests, you may use cilium/cilium-cli@c8d90c1 if you need to remove the trailing dot from the serverNames field when templating the policy.

Thanks, I could not find this solution by Googling around for 10 minutes, adopted!

(and sorry for accidentally editing your comment)

@giorio94
Copy link
Member

Edit: looking at the manifests, you may use cilium/cilium-cli@c8d90c1 if you need to remove the trailing dot from the serverNames field when templating the policy.

Thanks, I could not find this solution by Googling around for 10 minutes, adopted!

Yeah, that's a (trivial) custom template function, because apparently it is not supported by default.

(and sorry for accidentally editing your comment)

No problem.

@jrajahalme
Copy link
Member Author

Edit: looking at the manifests, you may use cilium/cilium-cli@c8d90c1 if you need to remove the trailing dot from the serverNames field when templating the policy.

Thanks, I could not find this solution by Googling around for 10 minutes, adopted!

No wonder since you had added the function in your PR, nice!

@jrajahalme
Copy link
Member Author

/test

@jrajahalme
cilium-cli: Test TLS with serverNames
47408ed 
Add new connectivity tests client-egress-l7-tls-headers-sni and
client-egress-tls-sni to test SNI enforcement with and without TLS
interception.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme jrajahalme force-pushed the pr/jrajahalme/cilium-cli-test-tls-with-sni-policy branch from 2686216 to 47408ed Compare November 12, 2024 12:20
@jrajahalme
Copy link
Member Author

Envoy got already updated on main rebased.

@jrajahalme
Copy link
Member Author

/test

@jrajahalme jrajahalme added release-note/misc This PR makes changes that have no direct user impact. and removed needs-backport/1.14 needs-backport/1.16 This PR / issue needs backporting to the v1.16 branch kind/regression This functionality worked fine before, but was broken in a newer release of Cilium. release-blocker/1.17 This issue will prevent the release of the next version of Cilium. release-note/bug This PR fixes an issue in a previous release of Cilium. labels Nov 12, 2024
@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 Nov 12, 2024
@squeed squeed added this pull request to the merge queue Nov 12, 2024
Merged via the queue into main with commit 5d3bf04 Nov 12, 2024
210 checks passed
@squeed squeed deleted the pr/jrajahalme/cilium-cli-test-tls-with-sni-policy branch November 12, 2024 15:36
jrajahalme added a commit to jrajahalme/cilium that referenced this pull request Nov 20, 2024
@jrajahalme
cilium-cli: Use unique CNP names
799108c 
'client-egress-l7-tls.yaml' and 'client-egress-l7-tls-sni.yaml' had the
same resource name "l7-policy-tls". Fix this by using the file name
(minus the suffix) as the resource name.

Fixes: cilium#35887

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme jrajahalme mentioned this pull request Nov 20, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Nov 20, 2024
@jrajahalme @tklauser
cilium-cli: Use unique CNP names
dbd2755 
'client-egress-l7-tls.yaml' and 'client-egress-l7-tls-sni.yaml' had the
same resource name "l7-policy-tls". Fix this by using the file name
(minus the suffix) as the resource name.

Fixes: #35887

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@github-actions github-actions bot mentioned this pull request Dec 9, 2024
Open
@sayboras sayboras mentioned this pull request Jan 24, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandro gandro gandro approved these changes

@nathanjsweet nathanjsweet nathanjsweet approved these changes

@rolinh rolinh rolinh approved these changes

@marseel marseel marseel approved these changes

@brlbil brlbil brlbil approved these changes

@sayboras sayboras sayboras approved these changes

@giorio94 giorio94 giorio94 approved these changes

Assignees
No one assigned
Labels
area/CI Continuous Integration testing issue or flake cilium-cli This PR contains changes related with cilium-cli ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@jrajahalme @giorio94 @gandro @nathanjsweet @rolinh @marseel @brlbil @sayboras @squeed