Skip to content

cli: Add the check for l7 proxy for to-fqdns test #40549

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
Jul 29, 2025
Merged

cli: Add the check for l7 proxy for to-fqdns test #40549

merged 1 commit into from
Jul 29, 2025

Conversation

vipul-21
Copy link
Contributor

@vipul-21 vipul-21 commented Jul 16, 2025 •
edited
Loading

Issue

PR #38750 introduced an IPv6 check for FQDN and other scenarios, gated by the external-target-ipv6-capable flag. However, deploying a DNS-only CNP on a cluster where the L7 proxy is disabled causes the test to fail with a timeout.

On clusters without L7 proxy enabled, the Cilium CLI still applies the policy and marks it as "changed" since it's accepted by Kubernetes. However, the policy isn't actually enforced by Cilium due to unsupported L7 features — which is only visible in the Cilium agent logs:

level=warning msg="Unable to add CiliumNetworkPolicy" ciliumNetworkPolicyName=client-egress-only-dns error="Invalid CiliumNetworkPolicy spec: L7 policy is not supported since L7 proxy is not enabled"

Since the CLI detects two policies as applied but only one is enforced, it waits for the policy revision to increment by 2. But only one policy is truly applied, so the revision increases by 1, resulting in a timeout:

📜 Applying CiliumNetworkPolicy 'client-egress-to-fqdns-one.one.one.one' to namespace 'cilium-test-1' on cluster kind-kind..
  ℹ️  📜 Successfully applied CiliumNetworkPolicy 'client-egress-to-fqdns-one.one.one.one' to namespace 'cilium-test-1' on cluster kind-kind
  ℹ️  Changed: true
  ℹ️  📜 Applying CiliumNetworkPolicy 'client-egress-only-dns' to namespace 'cilium-test-1' on cluster kind-kind..
  ℹ️  📜 Successfully applied CiliumNetworkPolicy 'client-egress-only-dns' to namespace 'cilium-test-1' on cluster kind-kind
  ℹ️  Changed: true
  🐛 Waiting for Cilium agents to increment policy revisions by map[kind-kind:2]
  🐛 Current policy revisions: map[kube-system/cilium-74ngv:13 kube-system/cilium-nthvw:13]
  🐛 Policy difference detected, waiting for Cilium agents to increment policy revisions..

CLI applies both policies and assumes they are valid.
It does not validate if policies are supported by Cilium (e.g., L7 rules without L7 proxy).
It then waits for policy revisions based on the number of applied resources, not actual enforcement.

Fix

Since to-fqdns is not valid feature without l7 proxy, add that to the test.

Add l7 proxy check for `to-fqdns` connectivity test

@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 Jul 16, 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 Jul 16, 2025
@vipul-21 vipul-21 force-pushed the singhvipul/cil-cli branch 2 times, most recently from 0bf4164 to e4f490d Compare July 17, 2025 00:09
@vipul-21 vipul-21 marked this pull request as ready for review July 17, 2025 01:01
@vipul-21 vipul-21 requested a review from a team as a code owner July 17, 2025 01:01
@vipul-21 vipul-21 requested a review from christarazi July 17, 2025 01:01
@vipul-21
Copy link
Contributor Author

/test

@vipul-21
Copy link
Contributor Author

/ci-clustermesh

@vipul-21
Copy link
Contributor Author

Running clustermesh upgrade again because of the failure: https://github.com/cilium/cilium/actions/runs/16333665383/job/46141499817. There is an open #39855 that has a label of it being a flaky.

@joestringer joestringer added the release-note/ci This PR makes changes to the CI. label Jul 17, 2025
@joestringer
Copy link
Member

Thanks for the PR 🙏 . Could you break down the change into two commits, one that does the refactor and one that makes the logical change? It's pretty hard to review the diff when there are both refactor and functional changes in the same commit.

@vipul-21 vipul-21 force-pushed the singhvipul/cil-cli branch from e4f490d to fb7476c Compare July 17, 2025 21:17
@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 Jul 17, 2025
@vipul-21 vipul-21 force-pushed the singhvipul/cil-cli branch from fb7476c to dfa94ba Compare July 17, 2025 21:17
@joestringer joestringer requested review from a team and tklauser and removed request for a team July 17, 2025 21:21
tklauser
tklauser reviewed Jul 18, 2025
View reviewed changes
@vipul-21 vipul-21 force-pushed the singhvipul/cil-cli branch 3 times, most recently from c99b2dc to 73dd0f0 Compare July 23, 2025 17:23
@vipul-21 vipul-21 changed the title cli: skip the to-fqdns test if external-target-ipv6-capable:false cli: Add the check for l7 proxy for to-fqdns test Jul 23, 2025
@vipul-21
Copy link
Contributor Author

/test

@vipul-21
Copy link
Contributor Author

/ci-clustermesh

@vipul-21
Copy link
Contributor Author

Restarted conformance cluster mesh as it was failing due to open issue #39370

@vipul-21
Copy link
Contributor Author

/ci-clustermesh

@vipul-21
Add L7proxy check for to-fqdns
99b3349 
Signed-off-by: Vipul Singh <singhvipul@microsoft.com>
@vipul-21 vipul-21 force-pushed the singhvipul/cil-cli branch from 73dd0f0 to 99b3349 Compare July 28, 2025 16:31
@vipul-21
Copy link
Contributor Author

/test

@joestringer joestringer enabled auto-merge July 28, 2025 18:23
@joestringer joestringer added dont-merge/bad-bot To prevent MLH from marking ready-to-merge. and removed dont-merge/bad-bot To prevent MLH from marking ready-to-merge. labels Jul 28, 2025
@joestringer joestringer added this pull request to the merge queue Jul 28, 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 Jul 28, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 28, 2025
@joestringer joestringer added this pull request to the merge queue Jul 29, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 29, 2025
@joestringer joestringer added this pull request to the merge queue Jul 29, 2025
Merged via the queue into cilium:main with commit daa9fa2 Jul 29, 2025
68 checks passed
@camrynl camrynl mentioned this pull request Jul 29, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tklauser tklauser tklauser left review comments

@joestringer joestringer joestringer approved these changes

@christarazi christarazi christarazi approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vipul-21 @joestringer @tklauser @christarazi