ci: add fqdn with cilium local redirect policy test#3543
Merged
Conversation
28144ad to
fe3aea4
Compare
Contributor
Author
|
/azp run Azure Container Networking PR |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for testing Cilium Network Policies with FQDN local redirect by introducing new utility functions, updating command execution retries, and expanding integration tests.
- Added functions to parse, create, and delete CiliumNetworkPolicy resources.
- Extended the testing framework with a new FQDN policy manifest and updated test logic to validate DNS redirection and metric counting.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/internal/kubernetes/utils_parse.go | Added mustParseCNP to support parsing CiliumNetworkPolicy from file. |
| test/internal/kubernetes/utils_delete.go | Added MustDeleteCiliumNetworkPolicy for handling CiliumNetworkPolicy deletion. |
| test/internal/kubernetes/utils_create.go | Introduced mustCreateCiliumNetworkPolicy with new logging for creation. |
| test/internal/kubernetes/utils.go | Added MustSetupCNP, modified ExecCmdOnPod to use ExecCmdOnPodOnce and retry logic. |
| test/integration/manifests/cilium/lrp/fqdn-cnp.yaml | Created a manifest for FQDN-based CiliumNetworkPolicy testing. |
| test/integration/lrp/lrp_test.go | Refactored LRP test to use a common setup function and return a pod struct. |
| test/integration/lrp/lrp_fqdn_test.go | Added integration tests to validate FQDN policies with multiple test cases. |
Comments suppressed due to low confidence (3)
test/internal/kubernetes/utils_create.go:195
- [nitpick] Error handling in mustCreateCiliumNetworkPolicy differs from mustCreateCiliumLocalRedirectPolicy (which uses panic). Consider aligning the error handling strategy for consistency.
log.Fatal(errors.Wrap(err, "failed to delete cilium network policy"))
test/internal/kubernetes/utils.go:498
- The ExecCmdOnPod function now wraps ExecCmdOnPodOnce with a retrier, but the retry logic and error handling appear to have been reorganized. Please verify that the retry semantics match the intended behavior as described in the PR.
retrier := retry.Retrier{Attempts: ShortRetryAttempts, Delay: RetryDelay}
test/integration/lrp/lrp_test.go:127
- [nitpick] The variable 'selectedClientPod' now holds a Pod struct instead of a string, and the subsequent log uses 'selectedClientPod.Name'. Ensure that the variable name reflects its structure (e.g. 'selectedClientPod' clearly indicates it is a pod object) for clarity.
selectedClientPod := TakeOne(clientPods.Items)
jpayne3506
reviewed
Apr 1, 2025
vipul-21
reviewed
Apr 1, 2025
leverages must delete functions during creation changes log.fatal to panic since log fatal will immediately exit, skipping all defers leverages wait for daemonset instead of wait for pods adds retry parameter to exec cmd on pod, adjusting associated calls incorporates exec cmd on pod error into lrp test
22fd545 to
3591a97
Compare
vipul-21
reviewed
Apr 1, 2025
remove checking for answer string as it only appears in non authoritative dns responses
vipul-21
previously approved these changes
Apr 1, 2025
jpayne3506
approved these changes
Apr 2, 2025
vipul-21
approved these changes
Apr 2, 2025
Contributor
Author
|
/azp run Azure Container Networking PR |
|
Azure Pipelines successfully started running 1 pipeline(s). |
sivakami-projects
pushed a commit
that referenced
this pull request
Oct 23, 2025
* refactor lrp setup * create lrp test case func * add k8 boilerplate for cnp * add lrp fqdn test and yaml * address linter issue * address feedback * address feedback leverages must delete functions during creation changes log.fatal to panic since log fatal will immediately exit, skipping all defers leverages wait for daemonset instead of wait for pods adds retry parameter to exec cmd on pod, adjusting associated calls incorporates exec cmd on pod error into lrp test * add case without explicit dns server remove checking for answer string as it only appears in non authoritative dns responses * improve debug message * adjust test domain name
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reason for Change:
Adds a test to validate functionality of a cilium local redirect policy when a cilium network policy is applied-- for example, confirms whether certain dns requests are redirected and increment a metric when they are allowed in the network policy (and confirms that they don't increment the counter when not allowed). Updates necessary utility methods and creates a function that issues an exec on pod command but does not retry on failure.
Issue Fixed:
N/A
Requirements:
Notes:
Example run: https://msazure.visualstudio.com/One/_build/results?buildId=119685764&view=logs&j=a725b465-af6c-5b16-2e20-4e0bf1d0563e&t=d9595ca1-827d-5105-2385-c679bcc42718
Nightly: https://msazure.visualstudio.com/One/_build/results?buildId=119489437&view=results
ACN PR run (no regression): https://msazure.visualstudio.com/One/_build/results?buildId=119699714&view=results