CNTRLPLANE-1137: Add e2e test for ARO HCP with Cilium Network Policies - #7077
CNTRLPLANE-1137: Add e2e test for ARO HCP with Cilium Network Policies#7077wewang58 wants to merge 4 commits into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
do-not-merge/needs-area
area/testing
WalkthroughAdds end-to-end Cilium connectivity testing and runtime Cilium installation support: new e2e test, Cilium install/config helpers, YAML download + SSA apply and readiness wait utilities, an ExternalCNIProvider option/flag with pre-test gating for Cilium, and Tekton task bundle digest updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
wewang58
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@wewang58: This pull request references CNTRLPLANE-1137 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
1df4df1 to
420498f
Compare
|
@wewang58: This pull request references CNTRLPLANE-1137 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
420498f to
7bb3d01
Compare
|
Now : Pod cilium-olm-844cdcc94b-tc9xh: Phase=Running, Ready=true is running, but DaemonSet cilium status: Desired=2, Current=0, Ready=0, Available=0, reason is nodes are not ready, should check more. |
7bb3d01 to
431d342
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
test/e2e/cilium_network_policy_test.go(1 hunks)test/e2e/util/hypershift_framework.go(2 hunks)test/e2e/util/util.go(4 hunks)
431d342 to
312235e
Compare
openshift-merge-robot
added
the
needs-rebase
312235e to
8cfd0a1
Compare
openshift-merge-robot
removed
the
needs-rebase
|
@wewang58: This pull request references CNTRLPLANE-1137 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
bryan-cox
left a comment
bryan-cox
left a comment
There was a problem hiding this comment.
Your git commit does not follow conventional commit standard so I would expect verify to fail.
| // in ARO HCP guest clusters. This test covers:Verifying Cilium installation | ||
| func InstallCiliumNetworkPolicies(t *testing.T, ctx context.Context, guestClient crclient.Client, hostedCluster *hyperv1.HostedCluster) { | ||
| t.Run("InstallCiliumNetworkPolicies", func(t *testing.T) { | ||
| if hostedCluster.Spec.Platform.Type != hyperv1.AzurePlatform { |
There was a problem hiding this comment.
I think we could just drop this if statement since the one on L1204 should be sufficient.
| return true | ||
| } | ||
| // // Diagnose pods if any exist but aren't ready | ||
| // podList := &corev1.PodList{} |
There was a problem hiding this comment.
This should all be removed if its not needed.
| }).Execute(&clusterOpts, globalOpts.Platform, globalOpts.ArtifactDir, "cilium-connectivity", globalOpts.ServiceAccountSigningKey) | ||
| } | ||
|
|
||
| // ptrBool returns a pointer to a bool |
There was a problem hiding this comment.
I would drop this and use the k8s library for this. There are examples in our codebase for this.
| t.Skip("test only supported on ARO HCP clusters") | ||
| } | ||
| //Skip if NetworkType is not Other (Cilium) | ||
| if hostedCluster.Spec.Networking.NetworkType != hyperv1.Other { |
There was a problem hiding this comment.
Where is this NetworkType set on HostedCluster? For conformance tests, we do that by passing --network-type to hypershift cluster cluster but this is E2E test suite.
There was a problem hiding this comment.
I suppose this check is now redundant when we already check globalOpts.ExternalCNIProvider above?
| } | ||
| defer cleanup() | ||
|
|
||
| t.Run("WaitForCiliumPodsReady", func(t *testing.T) { |
There was a problem hiding this comment.
Could this wait be moved to the place where Cilium is installed? It's better to fail early when installing. Here we can probably just check once that the pods are there.
| func (h *hypershiftTest) before(hostedCluster *hyperv1.HostedCluster, opts *PlatformAgnosticOptions, platform hyperv1.PlatformType) { | ||
| h.Run("ValidateHostedCluster", func(t *testing.T) { | ||
| if platform != hyperv1.NonePlatform { | ||
| if platform != hyperv1.NonePlatform && hostedCluster.Spec.Networking.NetworkType != hyperv1.Other { |
There was a problem hiding this comment.
I would like to know where the NetworkType is set (see my previous comment).
My feeling is that there could be a flag to install Cilium instead of the default CNI. Then we could get rid of these specific checks for azureutil.IsAroHCP() and platform == hyperv1.AzurePlatform and if platform == hyperv1.AzurePlatform && azureutil.IsAroHCP() && hostedCluster.Spec.Networking.NetworkType == hyperv1.Other
We'd simply install Cilium when required.
I also think that this test can be usable on other platforms, not just Azure/ARO. This test could be run also on AWS, etc. So, having the option to choose Cilium more speficically would be useful. Also, we might need to add Tigera Calico. This will also have NetworkType==Other. The current checks would then allow running the Cilium test with Tigera Calico, which is wrong.
There was a problem hiding this comment.
OK. I see it here: https://github.com/openshift/release/pull/71182/files
In that case, the "Other" network stack will be used for all hosted clusters that are created in the E2E test suite. Is this intentional? I thought just this single hosted cluster could install Cilium and run the test.
There was a problem hiding this comment.
This is just my debug pr:https://github.com/openshift/release/pull/71182/files , will close it after debugging it.
There was a problem hiding this comment.
@mgencur I don’t quite understand what you mean by this comment. I think this check is correct and necessary — for HostedClusters with the Other network type, we should skip the validation to avoid waiting for node readiness.
There was a problem hiding this comment.
Sorry. This should have been on line 140 below.
Unrelated comment that is applicable for line 127: The user can still choose hyperv1.Calico but the condition just checks for hyperv1.Other. So, currently it would be validating the cluster for Calico, and fail.
|
|
||
| // InstallCiliumNetworkPolicies validates that Cilium network policies are properly enforced | ||
| // in ARO HCP guest clusters. This test covers:Verifying Cilium installation | ||
| func InstallCiliumNetworkPolicies(t *testing.T, ctx context.Context, guestClient crclient.Client, hostedCluster *hyperv1.HostedCluster) { |
There was a problem hiding this comment.
This installs the whole Cilium, not just network policies. Maybe this function could be called just "InstallCilium" ?
|
|
||
| const ( | ||
| // CiliumVersion is the version of Cilium used for installation and connectivity tests | ||
| CiliumVersion = "1.15.1" |
There was a problem hiding this comment.
Maybe there's a better way to put this constant. I just can't find any.
There was a problem hiding this comment.
Yes, I think it’s better to read the value from the CILIUM_VERSION environment variable first, and use this constant only as a fallback. This way, when we update the version in the Prow job, we only need to modify the e2e job there, without requiring a new PR in Hypershift.
7b91115 to
9a2b24f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/e2e/util/util.go (1)
1415-1421: Add timeout to HTTP request to prevent indefinite hangs.The
http.Getcall lacks a timeout, which could cause the test to hang indefinitely if the remote server is unresponsive. This was flagged in a previous review but appears unaddressed.func ApplyYAMLFromURL(ctx context.Context, c crclient.Client, url string, defaultNamespace ...string) error { // Download YAML content - resp, err := http.Get(url) + httpClient := &http.Client{Timeout: 30 * time.Second} + resp, err := httpClient.Get(url) if err != nil { return fmt.Errorf("failed to download manifest from %s: %w", url, err) }
🧹 Nitpick comments (5)
test/e2e/util/cilium.go (3)
150-150: Useptr.Tofrom importedk8s.io/utils/ptrinstead of localptrBoolhelper.The file already imports
k8s.io/utils/ptr(Line 18) and usesptr.To(true)inCiliumSCCManifest()(Line 58). For consistency, useptr.To(false)here instead of the localptrBool(false)helper.- AllowPrivilegeEscalation: ptrBool(false), + AllowPrivilegeEscalation: ptr.To(false),
292-294: Remove redundantptrBoolhelper.This helper duplicates functionality already available via
ptr.To[bool]from the importedk8s.io/utils/ptrpackage. After updating the usage on Line 150, this function can be removed.-func ptrBool(b bool) *bool { - return &b -}
234-257: Test completion logic may miss failures that occur after the sleep period.The
WaitForConnectivityTestCompletionsubtest sleeps for a fixed duration, then checks if pods are still running. However, if a pod fails and restarts during or after the sleep, the check on Line 247 (pod.Status.Phase != corev1.PodRunning) would pass, potentially masking transient failures. Consider also checkingRestartCountto detect pods that crashed and recovered.failedPods := []string{} for _, pod := range podList.Items { if pod.Status.Phase != corev1.PodRunning { failedPods = append(failedPods, fmt.Sprintf("%s (phase: %s)", pod.Name, pod.Status.Phase)) + } else { + // Check for pods that crashed and restarted + for _, cs := range pod.Status.ContainerStatuses { + if cs.RestartCount > 0 { + failedPods = append(failedPods, fmt.Sprintf("%s (container %s restarted %d times)", pod.Name, cs.Name, cs.RestartCount)) + } + } } }test/e2e/util/util.go (2)
1451-1458: Expand cluster-scoped resource detection for namespace defaulting.The check for cluster-scoped resources only covers four kinds. Other common cluster-scoped resources like
PersistentVolume,StorageClass,ClusterIssuer,SecurityContextConstraints, etc., would incorrectly have a namespace set if passed through this function. Consider using the REST mapper to determine if a resource is namespaced, or expand the list.// Check if this is a namespaced resource (not cluster-scoped like ClusterRole) kind := obj.GetKind() isClusterScoped := kind == "ClusterRole" || kind == "ClusterRoleBinding" || - kind == "CustomResourceDefinition" || kind == "Namespace" + kind == "CustomResourceDefinition" || kind == "Namespace" || + kind == "PersistentVolume" || kind == "StorageClass" || + kind == "SecurityContextConstraints" || kind == "PriorityClass" || + kind == "ClusterIssuer" || kind == "ValidatingWebhookConfiguration" || + kind == "MutatingWebhookConfiguration"Alternatively, for a more robust solution, consider using the client's REST mapper to determine if a resource is namespaced.
1390-1398: Fragile DaemonSet name matching logic.The condition
strings.HasPrefix(ds.Name, "cilium-") && !strings.Contains(ds.Name, "operator")is brittle and may break if Cilium changes its naming conventions. Consider documenting the expected DaemonSet name or making the check more explicit.- // Look for the main Cilium DaemonSet (usually named "cilium") - if ds.Name == "cilium" || strings.HasPrefix(ds.Name, "cilium-") && !strings.Contains(ds.Name, "operator") { + // Look for the main Cilium agent DaemonSet + // The olm-for-cilium operator creates a DaemonSet named "cilium" + if ds.Name == "cilium" { ciliumDaemonSet = &dsList.Items[i] t.Logf("Found Cilium DaemonSet: %s", ds.Name) return true }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
test/e2e/cilium_network_policy_test.go(1 hunks)test/e2e/util/cilium.go(1 hunks)test/e2e/util/util.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/cilium_network_policy_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/e2e/util/cilium.gotest/e2e/util/util.go
🧬 Code graph analysis (2)
test/e2e/util/cilium.go (2)
test/e2e/util/util.go (5)
CiliumVersion(85-85)ApplyYAMLFromURL(1415-1469)CiliumDefaultTimeout(90-90)CiliumLongPollInterval(94-94)CiliumConnectivityWaitDuration(95-95)support/util/util.go (1)
DeleteIfNeeded(142-144)
test/e2e/util/util.go (1)
test/e2e/util/cilium.go (3)
CiliumNamespaceManifest(33-45)CiliumSCCManifest(48-81)CiliumManifestURLs(84-100)
🔇 Additional comments (3)
test/e2e/util/util.go (3)
194-222: LGTM: Well-structured wait helpers.The new
WaitForDeploymentAvailableandWaitForDaemonSetReadyhelpers follow the existing patterns in the codebase, use Gomega'sEventuallywith proper assertions, and provide good logging. These are useful general-purpose utilities that complement the Cilium-specific test code.
83-96: LGTM: Cilium constants and version handling.The CiliumVersion is now properly read from the environment without panicking in init(), addressing previous review feedback. The timeout constants are well-organized and provide appropriate values for different Cilium operations.
1471-1497: LGTM: Network config extraction with sensible defaults.The
getCiliumNetworkConfigfunction now properly initializes defaults before attempting to read from the cluster, addressing previous review feedback. The IPv4 detection logic usingstrings.Contains(clusterNet.CIDR, ".")is a reasonable heuristic for distinguishing IPv4 from IPv6 CIDRs.
| var notReadyPods []string | ||
| for _, pod := range podList.Items { | ||
| ready := false | ||
| for _, condition := range pod.Status.Conditions { | ||
| if condition.Type == corev1.PodReady && condition.Status == corev1.ConditionTrue { | ||
| ready = true | ||
| break | ||
| } | ||
| } | ||
| if !ready { | ||
| notReadyPods = append(notReadyPods, fmt.Sprintf("%s (phase: %s)", pod.Name, pod.Status.Phase)) | ||
| } | ||
| } |
There was a problem hiding this comment.
For loop-based checks, we can use HaveEach，E.g
g.Expect(podList.Items).To(HaveEach(
HaveField("Status.Conditions", ContainElement(
And(
HaveField("Type", corev1.PodReady),
HaveField("Status", corev1.ConditionTrue),
),
)),
))
There was a problem hiding this comment.
have learned more.
| var notReadyPods []string | ||
| for _, pod := range podList.Items { | ||
| ready := false | ||
| for _, condition := range pod.Status.Conditions { | ||
| if condition.Type == corev1.PodReady && condition.Status == corev1.ConditionTrue { | ||
| ready = true | ||
| break | ||
| } | ||
| } | ||
| if !ready { | ||
| notReadyPods = append(notReadyPods, fmt.Sprintf("Pod %s is not ready yet, phase: %s", pod.Name, pod.Status.Phase)) | ||
| } | ||
| } | ||
| g.Expect(notReadyPods).To(BeEmpty(), "some pods are not ready") |
Update imports in test/e2e/util/cilium.go to satisfy gci linter requirements. Refactor Cilium namespace, SCC creation, and manifest URLs in test/e2e/util/util.go to use helper functions CiliumNamespaceManifest, CiliumSCCManifest, and CiliumManifestURLs defined in test/e2e/util/cilium.go. Fixed missing import 'k8s.io/utils/ptr' in cilium.go and unused import in util.go. Added generic WaitForDeploymentAvailable and WaitForDaemonSetReady functions in test/e2e/util/util.go and updated InstallCilium to use them.
8103a14 to
eed9103
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
test/e2e/util/cilium.go (1)
235-237: Accept Succeeded pods in connectivity test completion check.Connectivity test pods legitimately finish with
Phase=Succeededafter completing their checks. The current code treats any non-Running pod as a failure, causing false positives when tests complete successfully.Apply this diff to accept both Running and Succeeded phases:
- if pod.Status.Phase != corev1.PodRunning { - failedPods = append(failedPods, fmt.Sprintf("%s (phase: %s)", pod.Name, pod.Status.Phase)) - } + switch pod.Status.Phase { + case corev1.PodRunning, corev1.PodSucceeded: + // ok - test is running or completed successfully + default: + failedPods = append(failedPods, fmt.Sprintf("%s (phase: %s)", pod.Name, pod.Status.Phase)) + }test/e2e/util/util.go (1)
1417-1421: Add timeout to HTTP request to prevent indefinite hangs.The
http.Getcall lacks a timeout, which can cause the test to hang indefinitely if the remote server is unresponsive. This is critical for CI/CD reliability.Apply this diff to add a timeout:
func ApplyYAMLFromURL(ctx context.Context, c crclient.Client, url string, defaultNamespace ...string) error { // Download YAML content - resp, err := http.Get(url) + httpClient := &http.Client{Timeout: 30 * time.Second} + resp, err := httpClient.Get(url) if err != nil { return fmt.Errorf("failed to download manifest from %s: %w", url, err) }
🧹 Nitpick comments (1)
test/e2e/util/cilium.go (1)
144-144: Useptr.Tofor consistency.Line 144 uses a custom
ptrBoolhelper, butk8s.io/utils/ptris already imported. For consistency with line 58 (inCiliumSCCManifest) and the rest of the codebase, useptr.To(false)instead.Apply this diff:
- AllowPrivilegeEscalation: ptrBool(false), + AllowPrivilegeEscalation: ptr.To(false),Once updated, the
ptrBoolhelper function at lines 280-282 can be removed entirely.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
test/e2e/cilium_network_policy_test.go(1 hunks)test/e2e/util/cilium.go(1 hunks)test/e2e/util/util.go(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/e2e/cilium_network_policy_test.gotest/e2e/util/util.gotest/e2e/util/cilium.go
🧬 Code graph analysis (3)
test/e2e/cilium_network_policy_test.go (2)
api/hypershift/v1beta1/hostedcluster_types.go (1)
AzurePlatform(1154-1154)support/azureutil/azureutil.go (1)
IsAroHCP(199-201)
test/e2e/util/util.go (2)
support/azureutil/azureutil.go (1)
IsAroHCP(199-201)test/e2e/util/cilium.go (3)
CiliumNamespaceManifest(33-45)CiliumSCCManifest(48-81)CiliumManifestURLs(84-100)
test/e2e/util/cilium.go (2)
test/e2e/util/util.go (7)
CiliumLongTimeout(91-91)CiliumDefaultPollInterval(93-93)CiliumVersion(85-85)ApplyYAMLFromURL(1415-1469)CiliumDefaultTimeout(90-90)CiliumLongPollInterval(94-94)CiliumConnectivityWaitDuration(95-95)support/util/util.go (1)
DeleteIfNeeded(142-144)
🔇 Additional comments (5)
test/e2e/cilium_network_policy_test.go (1)
28-53: LGTM!The test structure is clean and properly delegates to utility functions for resource management. Platform and CNI provider gating logic is correct.
test/e2e/util/util.go (4)
83-96: LGTM!The Cilium version and timeout constants are appropriately defined. The environment variable approach allows flexibility without crashing the test suite when
CILIUM_VERSIONis unset.
194-222: LGTM!The wait functions follow established patterns and use Gomega assertions correctly. They provide appropriate logging and timeout handling.
1471-1497: LGTM!The function properly initializes defaults before attempting to fetch network configuration, ensuring valid values are always returned even when the guest cluster network resource is unavailable.
1499-1568: LGTM!The CiliumConfig creation function correctly uses integer types for numeric fields like
clusterPoolIPv4MaskSize. The configuration structure follows Cilium's expected schema.
|
/retest-required |
| } | ||
|
|
||
| // CiliumSCCManifest returns the SecurityContextConstraints for Cilium. | ||
| func CiliumSCCManifest() *securityv1.SecurityContextConstraints { |
| // CiliumNamespace is the namespace where Cilium agent pods run. | ||
| CiliumNamespace = "cilium" | ||
| // CiliumTestNamespace is the namespace created for Cilium connectivity tests. | ||
| CiliumTestNamespace = "cilium-test" | ||
| // CiliumTestServiceAccount is the name of the service account used for Cilium connectivity tests. | ||
| CiliumTestServiceAccount = "default" |
| ) | ||
|
|
||
| // CiliumNamespaceManifest returns the cilium namespace with the required PodSecurity labels. | ||
| func CiliumNamespaceManifest() *corev1.Namespace { |
| } | ||
|
|
||
| // CiliumManifestURLs returns the list of Cilium manifest URLs for a given version. | ||
| func CiliumManifestURLs(version string) []string { |
|
|
||
| var ( | ||
| // CiliumVersion is read from CILIUM_VERSION at runtime. When empty, the Cilium tests should skip. | ||
| CiliumVersion = os.Getenv("CILIUM_VERSION") |
There was a problem hiding this comment.
- This needs to be public?
- Could we move the cilium related vars to the cilium.go file?
There was a problem hiding this comment.
Yes, moved the vars to cilium.go
| const ( | ||
| // Generic timeouts and intervals for Cilium tests | ||
| CiliumDefaultTimeout = 10 * time.Minute | ||
| CiliumLongTimeout = 20 * time.Minute | ||
| CiliumShortTimeout = 2 * time.Minute | ||
| CiliumDefaultPollInterval = 10 * time.Second | ||
| CiliumLongPollInterval = 15 * time.Second | ||
| CiliumConnectivityWaitDuration = 60 * time.Second | ||
| ) | ||
|
|
There was a problem hiding this comment.
- This needs to be public?
- Could we move the cilium related vars to the cilium.go file?
| // Split multi-document YAML using yaml decoder | ||
| decoder := k8syaml.NewYAMLOrJSONDecoder(bytes.NewReader(yamlContent), 4096) | ||
|
|
||
| for { |
This commit refactors the E2E test utility code by moving Cilium-related
functions from 'test/e2e/util/util.go' to 'test/e2e/util/cilium.go' to
improve organization and reduce file size.
Specific changes:
- Moved 'InstallCilium', 'GetCiliumNetworkConfig', and 'CreateCiliumConfig'
to 'test/e2e/util/cilium.go'.
- Added necessary constants ('CiliumConfigGroup', 'CiliumConfigVersion', etc.)
to 'test/e2e/util/cilium.go'.
- Updated 'test/e2e/util/util.go' to remove the moved functions and unused imports.
- Fixed import cycles and ensured proper package structure.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/e2e/util/util.go (1)
1206-1262: Add timeout to HTTP request to prevent indefinite hangs.The
http.Get(url)call at line 1210 lacks a timeout, which can cause tests to hang indefinitely if the remote server is unresponsive. This was flagged in a previous review but hasn't been addressed yet.Apply this diff:
func ApplyYAMLFromURL(ctx context.Context, c crclient.Client, url string, defaultNamespace ...string) error { // Download YAML content - resp, err := http.Get(url) + httpClient := &http.Client{Timeout: 30 * time.Second} + resp, err := httpClient.Get(url) if err != nil { return fmt.Errorf("failed to download manifest from %s: %w", url, err) }
🧹 Nitpick comments (4)
test/e2e/util/util.go (3)
180-193: Consider clarifying "available" vs "fully ready" semantics.
WaitForDeploymentAvailableonly checksReadyReplicas > 0, meaning it succeeds when at least one replica is ready, not necessarily all desired replicas. If this is intentional to test basic availability rather than full readiness, consider adding a comment to clarify. If full readiness is needed, compareReadyReplicasagainstdeployment.Status.Replicas.
195-208: Consider checking full DaemonSet readiness.Similar to
WaitForDeploymentAvailable, this function only verifiesNumberReady > 0rather than comparing againstDesiredNumberScheduled. If partial readiness is sufficient for test purposes, this is fine; otherwise, consider checkingds.Status.NumberReady == ds.Status.DesiredNumberScheduled && ds.Status.ObservedGeneration == ds.Generation.
1243-1252: Consider dynamic cluster-scope detection.The hardcoded list of cluster-scoped kinds (
ClusterRole,ClusterRoleBinding,CustomResourceDefinition,Namespace) may miss other cluster-scoped resources. For a more robust solution, consider using the discovery client or meta.RESTMapper to check resource scope dynamically. However, if the test only deals with these specific kinds, the current approach is acceptable.test/e2e/util/cilium.go (1)
304-312: Consider using label selectors instead of name matching.The DaemonSet detection logic at line 307 uses complex string matching:
ds.Name == "cilium" || strings.HasPrefix(ds.Name, "cilium-") && !strings.Contains(ds.Name, "operator"). While this works, it's brittle and depends on naming conventions. Consider using label selectors (e.g.,app=ciliumorcomponent=agent) to identify the correct DaemonSet more reliably.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
test/e2e/util/cilium.go(1 hunks)test/e2e/util/util.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/e2e/util/util.gotest/e2e/util/cilium.go
🧬 Code graph analysis (1)
test/e2e/util/cilium.go (2)
support/azureutil/azureutil.go (1)
IsAroHCP(199-201)test/e2e/util/util.go (3)
ApplyYAMLFromURL(1208-1262)WaitForDeploymentAvailable(181-193)WaitForDaemonSetReady(196-208)
🔇 Additional comments (2)
test/e2e/util/cilium.go (2)
481-505: LGTM: Good defensive programming with sane defaults.The function correctly initializes default values before attempting to retrieve the Network configuration, ensuring valid output even when the cluster configuration is unavailable. The IPv4-specific detection (checking for "." in CIDR) is appropriate for the dual-stack context mentioned in the comment.
538-606: LGTM: Correctly uses integer for mask size.The function properly uses
hostPrefixas anint32at line 572 for theclusterPoolIPv4MaskSizefield, addressing the previous review comment about type validation failures when passing a string.
| g.Eventually(func() bool { | ||
| ciliumConfig := &unstructured.Unstructured{} | ||
| ciliumConfig.SetGroupVersionKind(schema.GroupVersionKind{ | ||
| Group: CiliumConfigGroup, | ||
| Version: CiliumConfigVersion, | ||
| Kind: CiliumConfigKind, | ||
| }) | ||
| err := guestClient.Get(ctx, crclient.ObjectKey{Name: CiliumConfigName, Namespace: CiliumNamespace}, ciliumConfig) | ||
| if err != nil { | ||
| t.Logf("CiliumConfig not found yet: %v", err) | ||
| return false | ||
| } | ||
|
|
||
| // Verify the clusterPoolIPv4MaskSize is set correctly | ||
| spec, found, err := unstructured.NestedMap(ciliumConfig.Object, "spec") | ||
| if err != nil || !found { | ||
| t.Logf("Failed to get spec from CiliumConfig: %v", err) | ||
| return false | ||
| } | ||
| ipam, found, err := unstructured.NestedMap(spec, "ipam") | ||
| if err != nil || !found { | ||
| t.Logf("Failed to get ipam from CiliumConfig spec: %v", err) | ||
| return false | ||
| } | ||
| operator, found, err := unstructured.NestedMap(ipam, "operator") | ||
| if err != nil || !found { | ||
| t.Logf("Failed to get operator from CiliumConfig ipam: %v", err) | ||
| return false | ||
| } | ||
| maskSize, found, err := unstructured.NestedInt64(operator, "clusterPoolIPv4MaskSize") | ||
| if err != nil || !found { | ||
| t.Logf("Failed to get clusterPoolIPv4MaskSize: %v", err) | ||
| return false | ||
| } | ||
| if maskSize != int64(hostPrefix) { | ||
| t.Logf("CiliumConfig clusterPoolIPv4MaskSize is %d, expected %d. Updating...", maskSize, hostPrefix) | ||
| // Update it again if the operator overwrote it | ||
| desiredConfig := CreateCiliumConfig(podCIDR, hostPrefix) | ||
| ciliumConfig.Object["spec"] = desiredConfig.Object["spec"] | ||
| err = guestClient.Update(ctx, ciliumConfig) | ||
| if err != nil { | ||
| t.Logf("Failed to update CiliumConfig: %v", err) | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| t.Logf("CiliumConfig has correct clusterPoolIPv4MaskSize: %d", maskSize) | ||
| return true | ||
| }, CiliumShortTimeout, CiliumDefaultPollInterval).Should(BeTrue(), "CiliumConfig should have correct configuration") // Wait for operator to create DaemonSet |
There was a problem hiding this comment.
Potential infinite update conflict with operator.
The Eventually loop verifies the CiliumConfig has the correct clusterPoolIPv4MaskSize, and if the operator has overwritten it (lines 280-288), it updates the config again. If the operator continuously reverts these changes, this creates a retry loop that will run until timeout. While the timeout prevents a true infinite loop, it masks the underlying configuration conflict and wastes test time.
Consider failing fast after detecting that the operator reverted the configuration, or add a retry counter to fail after a few attempts rather than continuing until timeout.
t.Log("Verifying CiliumConfig has correct IPAM configuration")
+ updateAttempts := 0
+ const maxUpdateAttempts = 3
g.Eventually(func() bool {
ciliumConfig := &unstructured.Unstructured{}
// ... existing Get logic ...
if maskSize != int64(hostPrefix) {
t.Logf("CiliumConfig clusterPoolIPv4MaskSize is %d, expected %d. Updating...", maskSize, hostPrefix)
+ updateAttempts++
+ if updateAttempts > maxUpdateAttempts {
+ t.Fatalf("CiliumConfig was reverted by operator %d times, failing test", maxUpdateAttempts)
+ }
// Update it again if the operator overwrote it
desiredConfig := CreateCiliumConfig(podCIDR, hostPrefix)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In test/e2e/util/cilium.go around lines 244 to 292, the Eventually loop updates
CiliumConfig when clusterPoolIPv4MaskSize is wrong which can cause repeated
operator overwrite retries until timeout; change this to detect operator
reversion and fail fast or cap retries: introduce a local retry counter (e.g.
attempts := 0 before Eventually), when you detect maskSize != hostPrefix
increment attempts and if attempts > maxAttempts (e.g. 3) call t.Fatalf or use
Gomega Expect/Fail with a clear message and stop further updates; otherwise
perform at most one update per detection (leave the existing update call) and
return false to allow Immediately re-check, ensuring the loop exits early on
repeated reverts.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/e2e/util/util.go (1)
1203-1259: Add HTTP timeout and context usage in ApplyYAMLFromURL
ApplyYAMLFromURLuses a barehttp.Getwith no timeout, so a slow or hung endpoint can stall the entire e2e run indefinitely. This was already noted in a previous review and still applies here.Consider wiring the existing
ctxinto the request and setting an explicit timeout on the client, e.g.:func ApplyYAMLFromURL(ctx context.Context, c crclient.Client, url string, defaultNamespace ...string) error { - // Download YAML content - resp, err := http.Get(url) + // Download YAML content with timeout and context + httpClient := &http.Client{Timeout: 30 * time.Second} + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + if err != nil { + return fmt.Errorf("failed to build request for %s: %w", url, err) + } + resp, err := httpClient.Do(req) if err != nil { return fmt.Errorf("failed to download manifest from %s: %w", url, err) } defer resp.Body.Close()You can keep the rest of the function as‑is. This will prevent CI from hanging when the manifest host is unreachable or slow, while still propagating clear errors.
🧹 Nitpick comments (1)
test/e2e/util/util.go (1)
177-205: Strengthen readiness checks for Deployment/DaemonSet helpersBoth helpers currently treat “at least one ready pod” as “ready”, which can let tests proceed while most replicas are still pending or failing:
WaitForDeploymentAvailableusesdeployment.Status.ReadyReplicas > 0.WaitForDaemonSetReadyusesds.Status.NumberReady > 0.For gating installation flows (like Cilium) this can hide partial rollout and create flaky e2e behavior. It’s better to align with typical K8s readiness semantics (all desired pods ready, observed generation caught up).
Consider tightening the predicates, e.g.:
func WaitForDeploymentAvailable(ctx context.Context, t *testing.T, client crclient.Client, name, namespace string, timeout, interval time.Duration) { g := NewWithT(t) t.Logf("Waiting for deployment %s/%s to be ready", namespace, name) g.Eventually(func() bool { deployment := &appsv1.Deployment{} - err := client.Get(ctx, crclient.ObjectKey{Name: name, Namespace: namespace}, deployment) - if err != nil { + if err := client.Get(ctx, crclient.ObjectKey{Name: name, Namespace: namespace}, deployment); err != nil { t.Logf("Failed to get deployment %s/%s: %v", namespace, name, err) return false } - return deployment.Status.ReadyReplicas > 0 + // Ensure controller has observed the latest spec + if deployment.Generation > deployment.Status.ObservedGeneration { + return false + } + // Ensure all desired replicas are ready when replicas is specified + if deployment.Spec.Replicas != nil && deployment.Status.ReadyReplicas < *deployment.Spec.Replicas { + return false + } + return deployment.Status.ReadyReplicas > 0 }, timeout, interval).Should(BeTrue(), fmt.Sprintf("deployment %s/%s should be ready", namespace, name)) } func WaitForDaemonSetReady(ctx context.Context, t *testing.T, client crclient.Client, name, namespace string, timeout, interval time.Duration) { g := NewWithT(t) t.Logf("Waiting for DaemonSet %s/%s to be ready", namespace, name) g.Eventually(func() bool { ds := &appsv1.DaemonSet{} - err := client.Get(ctx, crclient.ObjectKey{Name: name, Namespace: namespace}, ds) - if err != nil { + if err := client.Get(ctx, crclient.ObjectKey{Name: name, Namespace: namespace}, ds); err != nil { t.Logf("Failed to get DaemonSet %s/%s: %v", namespace, name, err) return false } - return ds.Status.NumberReady > 0 + if ds.Generation > ds.Status.ObservedGeneration { + return false + } + return ds.Status.NumberReady == ds.Status.DesiredNumberScheduled }, timeout, interval).Should(BeTrue(), fmt.Sprintf("DaemonSet %s/%s should be ready", namespace, name)) }This keeps the helpers generic but makes “ready” mean “fully rolled out”, which should give more reliable signal for CNI and other critical components.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
test/e2e/cilium_network_policy_test.go(1 hunks)test/e2e/util/cilium.go(1 hunks)test/e2e/util/util.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/util/cilium.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/e2e/cilium_network_policy_test.gotest/e2e/util/util.go
🧬 Code graph analysis (1)
test/e2e/cilium_network_policy_test.go (3)
test/e2e/util/hypershift_framework.go (1)
NewHypershiftTest(66-78)support/azureutil/azureutil.go (1)
IsAroHCP(199-201)test/e2e/util/cilium.go (1)
EnsureCiliumConnectivityTestResources(327-475)
🔇 Additional comments (1)
test/e2e/cilium_network_policy_test.go (1)
1-53: Cilium connectivity test wiring and gating look appropriateThe test is cleanly scoped to Azure + ARO HCP with
ExternalCNIProvider == "cilium", reuses the shared hypershift test harness, and centralizes the Cilium connectivity setup/cleanup inEnsureCiliumConnectivityTestResources. No issues from this wrapper layer.
cf0e120 to
572db3e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/e2e/util/util.go (1)
1206-1211: Add timeout to HTTP request to prevent indefinite hangs.The
http.Getcall lacks a timeout, which could cause the test to hang indefinitely if the remote server is unresponsive. This is particularly important for CI environments where tests need to fail fast.Apply this diff to add a timeout:
func ApplyYAMLFromURL(ctx context.Context, c crclient.Client, url string, defaultNamespace ...string) error { // Download YAML content - resp, err := http.Get(url) + httpClient := &http.Client{Timeout: 30 * time.Second} + resp, err := httpClient.Get(url) if err != nil { return fmt.Errorf("failed to download manifest from %s: %w", url, err) }
🧹 Nitpick comments (1)
test/e2e/util/cilium.go (1)
535-537: Consider usingptr.To[bool]from k8s.io/utils/ptr.The
ptrBoolhelper duplicates functionality already available viaptr.To(b)which is imported in this package. This is a minor nit but would reduce code duplication.-func ptrBool(b bool) *bool { - return &b -}Then update line 371 to use
ptr.To(false)instead ofptrBool(false).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
test/e2e/cilium_network_policy_test.go(1 hunks)test/e2e/util/cilium.go(1 hunks)test/e2e/util/util.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/e2e/util/util.gotest/e2e/cilium_network_policy_test.gotest/e2e/util/cilium.go
🧬 Code graph analysis (2)
test/e2e/cilium_network_policy_test.go (2)
api/hypershift/v1beta1/hostedcluster_types.go (1)
AzurePlatform(1154-1154)support/azureutil/azureutil.go (1)
IsAroHCP(199-201)
test/e2e/util/cilium.go (3)
support/azureutil/azureutil.go (1)
IsAroHCP(199-201)test/e2e/util/util.go (3)
ApplyYAMLFromURL(1205-1272)WaitForDeploymentAvailable(180-192)WaitForDaemonSetReady(195-207)support/util/util.go (1)
DeleteIfNeeded(142-144)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Red Hat Konflux / hypershift-cli-mce-211-on-pull-request
- GitHub Check: Red Hat Konflux / hypershift-operator-main-on-pull-request
- GitHub Check: Red Hat Konflux / hypershift-release-mce-211-on-pull-request
🔇 Additional comments (8)
test/e2e/cilium_network_policy_test.go (1)
28-52: LGTM - Test structure follows e2e framework patterns correctly.The test properly:
- Runs in parallel with
t.Parallel()- Has layered platform guards (Azure platform, Cilium CNI provider, ARO HCP check)
- Uses context with cancellation
- Correctly defers cleanup function returned by
EnsureCiliumConnectivityTestResourcestest/e2e/util/util.go (2)
179-207: LGTM - Readiness helpers follow consistent patterns.Both
WaitForDeploymentAvailableandWaitForDaemonSetReadyuse the standard Gomega Eventually pattern with configurable timeout and interval. TheReadyReplicas > 0/NumberReady > 0check is a reasonable minimum bar for the Cilium installation use case.
1222-1268: LGTM - YAML parsing and Server-Side Apply implementation is robust.Good implementation choices:
- Uses
k8syaml.NewYAMLOrJSONDecoderfor multi-document YAML support- Properly uses RESTMapper to determine if resources are namespaced
- Server-Side Apply with
ForceOwnershipand field owner ensures clean resource management- Handles empty documents gracefully
test/e2e/util/cilium.go (5)
64-113: LGTM - Manifest helpers are well-structured.
CiliumNamespaceManifestcorrectly applies privileged PodSecurity labels required for Cilium.CiliumSCCManifestprovides the necessary capabilities (NET_ADMIN, SYS_ADMIN, etc.) for Cilium's networking operations with appropriate service account bindings.
134-172: LGTM - InstallCilium handles namespace creation robustly.The function properly handles both the case where the namespace doesn't exist (creates it) and where it already exists (updates labels). The ARO HCP guard at the top ensures this only runs on supported platforms.
481-507: LGTM - GetCiliumNetworkConfig has sensible defaults and IPv4 extraction.Good defensive programming:
- Provides defaults (
10.132.0.0/14, hostPrefix 23) before attempting to read cluster network config- Correctly filters for IPv4 CIDRs using string detection
- Only overrides hostPrefix if non-zero in the source
509-533: LGTM - Cleanup uses recommended DeleteIfNeeded helper.The cleanup function correctly uses
hyperutil.DeleteIfNeeded(per past review suggestion) and logs warnings rather than failing the test on cleanup errors, which is appropriate behavior.
539-608: LGTM - CreateCiliumConfig builds a comprehensive CiliumConfig CR.The config correctly uses:
hostPrefixasint32(addresses past review about type validation)- Proper IPAM cluster-pool mode configuration
- CNI chaining settings for multus integration
- Appropriate tolerations for scheduling on all nodes
|
/test verify |
3e0d80b to
2e6af87
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/e2e/util/cilium.go (1)
460-465: Fix pod phase check to allow successfully completed pods.Some connectivity test pods legitimately complete with
Phase=Succeededafter running their tests. The current check treats any non-Runningpod as a failure, causing false test failures even when all connectivity tests pass successfully.Apply this diff:
failedPods := []string{} for _, pod := range podList.Items { - if pod.Status.Phase != corev1.PodRunning { - failedPods = append(failedPods, fmt.Sprintf("%s (phase: %s)", pod.Name, pod.Status.Phase)) - } + switch pod.Status.Phase { + case corev1.PodRunning, corev1.PodSucceeded: + // Pod is running or completed successfully + default: + failedPods = append(failedPods, fmt.Sprintf("%s (phase: %s)", pod.Name, pod.Status.Phase)) + } }
🧹 Nitpick comments (1)
test/e2e/util/cilium.go (1)
535-537: Consider using ptr.To for consistency.The codebase already imports and uses
ptr.To(line 90), making this helper redundant. Usingptr.To(false)instead ofptrBool(false)would eliminate duplication and improve consistency.-func ptrBool(b bool) *bool { - return &b -}Then replace line 371:
- AllowPrivilegeEscalation: ptrBool(false), + AllowPrivilegeEscalation: ptr.To(false),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
test/e2e/cilium_network_policy_test.go(1 hunks)test/e2e/util/cilium.go(1 hunks)test/e2e/util/util.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/util/util.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/e2e/cilium_network_policy_test.gotest/e2e/util/cilium.go
🧬 Code graph analysis (2)
test/e2e/cilium_network_policy_test.go (5)
api/hypershift/v1beta1/hostedcluster_types.go (1)
AzurePlatform(1154-1154)test/e2e/util/hypershift_framework.go (1)
NewHypershiftTest(66-78)support/azureutil/azureutil.go (1)
IsAroHCP(199-201)test/e2e/util/util.go (1)
WaitForGuestClient(355-403)test/e2e/util/cilium.go (1)
EnsureCiliumConnectivityTestResources(331-479)
test/e2e/util/cilium.go (2)
support/azureutil/azureutil.go (1)
IsAroHCP(199-201)test/e2e/util/util.go (3)
ApplyYAMLFromURL(1205-1272)WaitForDeploymentAvailable(180-192)WaitForDaemonSetReady(195-207)
🔇 Additional comments (1)
test/e2e/cilium_network_policy_test.go (1)
27-52: LGTM!The test structure is clean and follows e2e testing best practices. Platform and CNI provider gating is appropriately placed, and cleanup is properly deferred.
|
/retest-required |
|
/retest |
2e6af87 to
a657d3a
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: wewang58 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
test/e2e/util/util.go (1)
1203-1272: Add HTTP timeout and use context for manifest downloads
ApplyYAMLFromURLuses a barehttp.Getwithout timeout or context binding. A hung or slow endpoint can block the entire e2e run indefinitely, which is especially risky for CI.Reusing
ctxand a client with a finite timeout will make this robust.Suggested change:
func ApplyYAMLFromURL(ctx context.Context, c crclient.Client, url string, defaultNamespace ...string) error { - // Download YAML content - resp, err := http.Get(url) + // Download YAML content with context and timeout + httpClient := &http.Client{Timeout: 30 * time.Second} + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + if err != nil { + return fmt.Errorf("failed to build request for %s: %w", url, err) + } + + resp, err := httpClient.Do(req) if err != nil { return fmt.Errorf("failed to download manifest from %s: %w", url, err) } defer resp.Body.Close()The rest of the function can remain as-is. This ensures downloads honor test cancellation and don’t hang forever on network issues.
test/e2e/util/cilium.go (1)
449-471: Connectivity-check should not treatSucceededpods as failuresIn
WaitForConnectivityTestCompletion, any pod not inRunningphase is added tofailedPods. The Cilium connectivity suite creates jobs whose pods legitimately complete withPhase=Succeeded. After the 60 s wait, those pods will be Succeeded, and this logic will report them as failures even when the test passed.Adjust the phase check to allow
Succeededand only flag truly bad phases (e.g.Failed,Unknown):- failedPods := []string{} - for _, pod := range podList.Items { - if pod.Status.Phase != corev1.PodRunning { - failedPods = append(failedPods, fmt.Sprintf("%s (phase: %s)", pod.Name, pod.Status.Phase)) - } - } + failedPods := []string{} + for _, pod := range podList.Items { + switch pod.Status.Phase { + case corev1.PodRunning, corev1.PodSucceeded: + // ok – still running or completed successfully + default: + failedPods = append(failedPods, fmt.Sprintf("%s (phase: %s)", pod.Name, pod.Status.Phase)) + } + }This keeps real failures visible while letting successfully completed connectivity-check pods pass.
🧹 Nitpick comments (1)
test/e2e/util/cilium.go (1)
32-62: Guard Cilium tests whenCILIUM_VERSIONis unset
CiliumVersionis read once fromCILIUM_VERSIONand then used to build manifest URLs and the connectivity-check path. If the env var is missing or misconfigured, the helpers will fail with HTTP 404s from GitHub rather than cleanly skipping.Consider an early guard in Cilium-specific helpers (e.g.
InstallCilium,EnsureCiliumConnectivityTestResources) that skips the subtests whenCiliumVersion == "", so only Cilium jobs that explicitly configure the version run these flows.Example (inside
InstallCilium’s top-levelt.Run):func InstallCilium(t *testing.T, ctx context.Context, guestClient crclient.Client, hostedCluster *hyperv1.HostedCluster) { t.Run("InstallCilium", func(t *testing.T) { + if CiliumVersion == "" { + t.Skip("CILIUM_VERSION not set; skipping Cilium installation") + }You can apply a similar check before using
CiliumVersioninEnsureCiliumConnectivityTestResources.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
.tekton/pipelines/common-operator-build.yaml(1 hunks)test/e2e/cilium_network_policy_test.go(1 hunks)test/e2e/util/cilium.go(1 hunks)test/e2e/util/util.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/e2e/cilium_network_policy_test.gotest/e2e/util/util.gotest/e2e/util/cilium.go
🧬 Code graph analysis (2)
test/e2e/cilium_network_policy_test.go (2)
api/hypershift/v1beta1/hostedcluster_types.go (1)
AzurePlatform(1154-1154)support/azureutil/azureutil.go (1)
IsAroHCP(199-201)
test/e2e/util/cilium.go (1)
test/e2e/util/util.go (3)
ApplyYAMLFromURL(1205-1272)WaitForDeploymentAvailable(180-192)WaitForDaemonSetReady(195-207)
🔇 Additional comments (3)
.tekton/pipelines/common-operator-build.yaml (1)
225-241: Bundle digest bump looks fineOnly the Tekton bundle digest for
deprecated-image-checkwas updated; no structural changes to the pipeline. This is safe as long as the referenced bundle exists in the registry.test/e2e/util/util.go (1)
179-207: Deployment/DaemonSet wait helpers are appropriate for e2e usageThe
WaitForDeploymentAvailableandWaitForDaemonSetReadyhelpers correctly wrap GomegaEventuallyaround status checks and will fail tests when readiness isn’t reached within the configured timeout. They’re a good fit for the new Cilium flows.test/e2e/cilium_network_policy_test.go (1)
27-51: Cilium connectivity test wiring and scoping look correctThe test is properly scoped to Azure ARO HCP clusters with
ExternalCNIProvider == "cilium", uses the shared Hypershift e2e framework, and defers cleanup viaEnsureCiliumConnectivityTestResources. This is an appropriate, isolated e2e for Cilium connectivity.
|
@jparrill can you help to review my pr when you have time? thanks. |
a657d3a to
c3ab188
Compare
This commit synchronizes the common-operator-build pipeline configuration with the latest upstream changes to ensure consistency and compatibility.
c3ab188 to
48f367c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
test/e2e/util/util.go (1)
1207-1211: Add timeout to HTTP request to prevent test hangs.The
http.Getcall at line 1207 lacks a timeout, which can cause the test to hang indefinitely if the manifest server is unresponsive or slow. This is critical for CI environments where tests must fail fast.Apply this diff to add a 30-second timeout:
func ApplyYAMLFromURL(ctx context.Context, c crclient.Client, url string, defaultNamespace ...string) error { // Download YAML content - resp, err := http.Get(url) + httpClient := &http.Client{Timeout: 30 * time.Second} + resp, err := httpClient.Get(url) if err != nil { return fmt.Errorf("failed to download manifest from %s: %w", url, err) }Note: A past review comment indicated this was addressed in commit b4f1edd, but the current code still lacks the timeout. Please verify if the fix was reverted or needs to be re-applied.
test/e2e/util/cilium.go (1)
460-471: Allow successfully completed pods in connectivity test validation.Lines 462-464 treat any pod not in
Phase=Runningas a failure. However, some Cilium connectivity test pods (such as test runners) legitimately complete withPhase=Succeededafter running their tests. This will cause false failures when these pods finish successfully.Apply this diff to accept both Running and Succeeded pods:
failedPods := []string{} for _, pod := range podList.Items { - if pod.Status.Phase != corev1.PodRunning { - failedPods = append(failedPods, fmt.Sprintf("%s (phase: %s)", pod.Name, pod.Status.Phase)) - } + switch pod.Status.Phase { + case corev1.PodRunning, corev1.PodSucceeded: + // Pod is running or completed successfully + default: + failedPods = append(failedPods, fmt.Sprintf("%s (phase: %s)", pod.Name, pod.Status.Phase)) + } }Note: A past review comment indicated this was addressed in commit a657d3a, but the current code still rejects Succeeded pods. Please verify if the fix was reverted or needs to be re-applied.
🧹 Nitpick comments (1)
test/e2e/util/cilium.go (1)
244-292: Consider capping retries to detect operator conflicts faster.The
Eventuallyloop at lines 244-292 will retry updating the CiliumConfig if the operator overwritesclusterPoolIPv4MaskSize. While the timeout prevents indefinite looping, a conflict where the operator continuously reverts the configuration will waste up to 2 minutes of test time before failing.Consider adding a retry counter to fail fast after detecting repeated operator reverts:
t.Log("Verifying CiliumConfig has correct IPAM configuration") + updateAttempts := 0 + const maxUpdateAttempts = 3 g.Eventually(func() bool { ciliumConfig := &unstructured.Unstructured{} // ... existing code to get and verify config ... if maskSize != int64(hostPrefix) { t.Logf("CiliumConfig clusterPoolIPv4MaskSize is %d, expected %d. Updating...", maskSize, hostPrefix) + updateAttempts++ + if updateAttempts > maxUpdateAttempts { + t.Fatalf("CiliumConfig was reverted by operator %d times, configuration conflict detected", maxUpdateAttempts) + } // Update it again if the operator overwrote itThis is optional as the current timeout-based approach is functional, but faster feedback can improve debugging when configuration conflicts occur.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
.tekton/pipelines/common-operator-build.yaml(16 hunks)test/e2e/cilium_network_policy_test.go(1 hunks)test/e2e/util/cilium.go(1 hunks)test/e2e/util/util.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .tekton/pipelines/common-operator-build.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/e2e/util/util.gotest/e2e/cilium_network_policy_test.gotest/e2e/util/cilium.go
🧬 Code graph analysis (1)
test/e2e/util/cilium.go (2)
support/azureutil/azureutil.go (1)
IsAroHCP(199-201)test/e2e/util/util.go (3)
ApplyYAMLFromURL(1205-1272)WaitForDeploymentAvailable(180-192)WaitForDaemonSetReady(195-207)
🔇 Additional comments (1)
test/e2e/cilium_network_policy_test.go (1)
27-52: LGTM: Well-structured test with appropriate guards and cleanup.The test properly uses parallel execution, has early skip conditions for platform and CNI provider, validates ARO HCP environment, and ensures proper resource cleanup via defer. The integration with the Hypershift e2e framework is correct.
|
@wewang58: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
8 participants
What this PR does / why we need it:
This PR implements cilium installation and check basic network policy scenarios test in aks env.
Which issue(s) this PR fixes:
Fixes https://issues.redhat.com/browse/CNTRLPLANE-1137
Special notes for your reviewer:
Checklist: