Repository navigation
CNTRLPLANE-1137: Add e2e test for ARO HCP with Cilium Network Policies #7077
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.
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
Changes from 1 commit
c49ae1d
eed9103
73d2966
48f367c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,257 @@ | ||
| //go:build e2e | ||
| // +build e2e | ||
|
|
||
| package e2e | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "testing" | ||
| "time" | ||
|
|
||
| . "github.com/onsi/gomega" | ||
| securityv1 "github.com/openshift/api/security/v1" | ||
| hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" | ||
| "github.com/openshift/hypershift/support/azureutil" | ||
| e2eutil "github.com/openshift/hypershift/test/e2e/util" | ||
|
|
||
| corev1 "k8s.io/api/core/v1" | ||
| apierrors "k8s.io/apimachinery/pkg/api/errors" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| crclient "sigs.k8s.io/controller-runtime/pkg/client" | ||
| ) | ||
|
|
||
| // TestCiliumConnectivity validates Cilium connectivity using the official Cilium connectivity test suite. | ||
| // This test is specifically for ARO HCP clusters with Cilium as the network provider. | ||
| // | ||
| // The test performs the following steps: | ||
| // 1.Check cilium agent pods ready | ||
| // 2. Creates cilium-test namespace with appropriate labels | ||
| // 3. Deploys Cilium connectivity test pods | ||
| // 4. Waits for all test pods to be ready and running | ||
| // 5. Cleans up test resources. | ||
| func TestCiliumConnectivity(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| ctx, cancel := context.WithCancel(testContext) | ||
| defer cancel() | ||
|
|
||
| clusterOpts := globalOpts.DefaultClusterOptions(t) | ||
|
|
||
| if globalOpts.Platform != hyperv1.AzurePlatform { | ||
| t.Skip("Skipping test because it requires Azure platform") | ||
| } | ||
|
|
||
| if globalOpts.ExternalCNIProvider != "cilium" { | ||
| t.Skipf("skip cilium connection test if e2e.external-cni-provider is not cilium") | ||
| } | ||
|
|
||
| e2eutil.NewHypershiftTest(t, ctx, func(t *testing.T, g Gomega, mgtClient crclient.Client, hostedCluster *hyperv1.HostedCluster) { | ||
| if !azureutil.IsAroHCP() { | ||
| t.Skip("test only supported on ARO HCP clusters") | ||
| } | ||
| guestClient := e2eutil.WaitForGuestClient(t, ctx, mgtClient, hostedCluster) | ||
|
|
||
| testNamespace := "cilium-test" | ||
|
|
||
| // Cleanup function | ||
| cleanup := func() { | ||
| t.Log("Cleaning up Cilium connectivity test resources") | ||
|
|
||
| // Delete SCC | ||
| scc := &securityv1.SecurityContextConstraints{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "cilium-test", | ||
| }, | ||
| } | ||
| if err := guestClient.Delete(ctx, scc); err != nil && !apierrors.IsNotFound(err) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you can use |
||
| t.Logf("Warning: failed to delete SCC: %v", err) | ||
| } | ||
|
|
||
| // Delete namespace | ||
| ns := &corev1.Namespace{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: testNamespace, | ||
| }, | ||
| } | ||
| if err := guestClient.Delete(ctx, ns); err != nil && !apierrors.IsNotFound(err) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above |
||
| t.Logf("Warning: failed to delete namespace: %v", err) | ||
| } | ||
|
|
||
| t.Log("Cleanup completed") | ||
| } | ||
| defer cleanup() | ||
|
|
||
| t.Run("CheckCiliumPodsRunning", func(t *testing.T) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I mentioned in another comment, Isolate the whole E2E without the cleanup in their own function and call it EnsureXXXXXX. |
||
| g := NewWithT(t) | ||
|
|
||
| t.Log("Check cilium pods to be running") | ||
| g.Eventually(func() bool { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe you can use here
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mgencur and i discussed the issue before: #7077 (comment) |
||
| podList := &corev1.PodList{} | ||
| err := guestClient.List(ctx, podList, crclient.InNamespace("cilium")) | ||
| if err != nil { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| t.Logf("Failed to list Cilium pods: %v", err) | ||
| return false | ||
| } | ||
| t.Logf("Found %d Cilium pods", len(podList.Items)) | ||
|
|
||
| // Check all pods are Ready | ||
| 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)) | ||
| } | ||
| } | ||
| if len(notReadyPods) > 0 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The deployment verification could pass with at least 1 replica running, what about if other replicas are not available? It's there any functional issue at Cilium level?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need confirm all pods( cilium-olm, cilium-operator and cilium) pods are all running, then cni can work correctly. |
||
| t.Logf("Pods not ready: %v", notReadyPods) | ||
| return false | ||
| } | ||
| return true | ||
| }, e2eutil.CiliumPodsReadyTimeout, e2eutil.CiliumPodsReadyPollInterval).Should(BeTrue(), "all Cilium pods should be running") | ||
|
|
||
| t.Log("All Cilium pods are running") | ||
| }) | ||
|
|
||
| t.Run("CreateSecurityContextConstraints", func(t *testing.T) { | ||
| g := NewWithT(t) | ||
|
|
||
| t.Log("Creating SecurityContextConstraints for Cilium connectivity test") | ||
| scc := &securityv1.SecurityContextConstraints{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "cilium-test", | ||
| }, | ||
| AllowHostPorts: true, | ||
| AllowHostNetwork: true, | ||
| AllowHostDirVolumePlugin: false, | ||
| AllowHostIPC: false, | ||
| AllowHostPID: false, | ||
| AllowPrivilegeEscalation: ptrBool(false), | ||
| AllowPrivilegedContainer: false, | ||
| ReadOnlyRootFilesystem: false, | ||
| RequiredDropCapabilities: []corev1.Capability{}, | ||
| RunAsUser: securityv1.RunAsUserStrategyOptions{Type: securityv1.RunAsUserStrategyMustRunAsRange}, | ||
| SELinuxContext: securityv1.SELinuxContextStrategyOptions{Type: securityv1.SELinuxStrategyMustRunAs}, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are any of these suitable for parameterization? |
||
| Users: []string{"system:serviceaccount:cilium-test:default"}, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SA name should be part of the const. |
||
| } | ||
|
|
||
| err := guestClient.Create(ctx, scc) | ||
| if err != nil && !apierrors.IsAlreadyExists(err) { | ||
| g.Expect(err).ToNot(HaveOccurred(), "failed to create SCC") | ||
| } | ||
|
|
||
| t.Log("SecurityContextConstraints created successfully") | ||
| }) | ||
|
|
||
| t.Run("CreateTestNamespace", func(t *testing.T) { | ||
| g := NewWithT(t) | ||
|
|
||
| t.Log("Creating cilium-test namespace") | ||
| ns := &corev1.Namespace{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: testNamespace, | ||
| Labels: map[string]string{ | ||
| "security.openshift.io/scc.podSecurityLabelSync": "false", | ||
| "pod-security.kubernetes.io/enforce": "privileged", | ||
| "pod-security.kubernetes.io/audit": "privileged", | ||
| "pod-security.kubernetes.io/warn": "privileged", | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| err := guestClient.Create(ctx, ns) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe it's worth to make |
||
| if err != nil && !apierrors.IsAlreadyExists(err) { | ||
| g.Expect(err).ToNot(HaveOccurred(), "failed to create namespace") | ||
| } | ||
|
|
||
| t.Log("Test namespace created successfully") | ||
| }) | ||
|
|
||
| t.Run("DeployCiliumConnectivityTest", func(t *testing.T) { | ||
| g := NewWithT(t) | ||
|
|
||
| t.Logf("Deploying Cilium connectivity test from version %s", e2eutil.CiliumVersion) | ||
| connectivityTestURL := fmt.Sprintf("https://raw.githubusercontent.com/cilium/cilium/%s/examples/kubernetes/connectivity-check/connectivity-check.yaml", e2eutil.CiliumVersion) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The URL should be a const/var. Maybe the version a const and the var a mixture of the URL + version. |
||
|
|
||
| err := e2eutil.ApplyYAMLFromURL(ctx, guestClient, connectivityTestURL, testNamespace) | ||
| g.Expect(err).ToNot(HaveOccurred(), "failed to apply connectivity test manifest") | ||
|
|
||
| t.Log("Connectivity test manifests applied successfully") | ||
| }) | ||
|
|
||
| t.Run("WaitForConnectivityTestPodsReady", func(t *testing.T) { | ||
| g := NewWithT(t) | ||
|
|
||
| t.Log("Waiting for connectivity test pods to be ready") | ||
| g.Eventually(func() bool { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| podList := &corev1.PodList{} | ||
| err := guestClient.List(ctx, podList, crclient.InNamespace(testNamespace)) | ||
| if err != nil { | ||
| t.Logf("Failed to list pods: %v", err) | ||
| return false | ||
| } | ||
| if len(podList.Items) == 0 { | ||
| t.Log("No pods found in cilium-test namespace yet") | ||
| return false | ||
| } | ||
|
|
||
| t.Logf("Found %d pods in cilium-test namespace", len(podList.Items)) | ||
|
|
||
| // Check all pods are Ready | ||
| 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 { | ||
| t.Logf("Pod %s is not ready yet, phase: %s", pod.Name, pod.Status.Phase) | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| }, e2eutil.CiliumConnectivityTestPodsTimeout, e2eutil.CiliumConnectivityTestPodsPollInterval).Should(BeTrue(), "all connectivity test pods should be ready") | ||
|
|
||
| t.Log("All connectivity test pods are ready") | ||
| }) | ||
|
|
||
| t.Run("WaitForConnectivityTestCompletion", func(t *testing.T) { | ||
| g := NewWithT(t) | ||
|
|
||
| t.Logf("Waiting %v for connectivity tests to run", e2eutil.CiliumConnectivityTestDuration) | ||
| time.Sleep(e2eutil.CiliumConnectivityTestDuration) | ||
|
|
||
| t.Log("Verifying all test pods are still running") | ||
| podList := &corev1.PodList{} | ||
| err := guestClient.List(ctx, podList, crclient.InNamespace(testNamespace)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above. |
||
| g.Expect(err).ToNot(HaveOccurred(), "should be able to list test 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)) | ||
| } | ||
| } | ||
|
|
||
| if len(failedPods) > 0 { | ||
| t.Errorf("Found %d pods not in Running phase: %v", len(failedPods), failedPods) | ||
| } else { | ||
| t.Logf("All %d connectivity test pods are running successfully", len(podList.Items)) | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
Resolved
|
||
| }) | ||
|
|
||
| t.Log("Cilium connectivity test completed successfully") | ||
| }).Execute(&clusterOpts, globalOpts.Platform, globalOpts.ArtifactDir, "cilium-connectivity", globalOpts.ServiceAccountSigningKey) | ||
| } | ||
|
|
||
| func ptrBool(b bool) *bool { | ||
| return &b | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,7 +48,8 @@ type PlatformAgnosticOptions struct { | |
| PowerVSPlatform powervs.RawCreateOptions | ||
| OpenStackPlatform openstack.RawCreateOptions | ||
|
|
||
| ExtOIDCConfig *ExtOIDCConfig | ||
| ExtOIDCConfig *ExtOIDCConfig | ||
| ExternalCNIProvider string | ||
| } | ||
|
|
||
| type hypershiftTestFunc func(t *testing.T, g Gomega, mgtClient crclient.Client, hostedCluster *hyperv1.HostedCluster) | ||
|
|
@@ -124,7 +125,7 @@ func (h *hypershiftTest) Execute(opts *PlatformAgnosticOptions, platform hyperv1 | |
| // runs before each test. | ||
| 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 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would like to know where the NetworkType is set (see my previous comment).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK. I see it here: https://github.com/openshift/release/pull/71182/files
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is just my debug pr:https://github.com/openshift/release/pull/71182/files , will close it after debugging it.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| if opts.AWSPlatform.EndpointAccess == string(hyperv1.Private) { | ||
| ValidatePrivateCluster(t, h.ctx, h.client, hostedCluster, opts) | ||
| } else { | ||
|
|
@@ -137,6 +138,25 @@ func (h *hypershiftTest) before(hostedCluster *hyperv1.HostedCluster, opts *Plat | |
| ValidateAuthenticationSpec(t, h.ctx, h.client, hostedCluster, opts.ExtOIDCConfig) | ||
| } | ||
| } | ||
|
|
||
| if opts.ExternalCNIProvider == "cilium" { | ||
| // Only install Cilium when there are worker nodes configured. | ||
| // The cilium-olm deployment requires worker nodes to schedule its pods. | ||
| // TestNodePool sets NodePoolReplicas=0 and creates NodePools later in individual tests, | ||
| // so we skip Cilium installation during the initial cluster validation phase. | ||
| if !util.IsPrivateHC(hostedCluster) { | ||
| if opts.NodePoolReplicas == 0 { | ||
| t.Fatal("NodePool replicas must be positive for Cilium to install.") | ||
| } | ||
| guestClient := WaitForGuestClient(t, context.Background(), h.client, hostedCluster) | ||
| InstallCilium(t, context.Background(), guestClient, hostedCluster) | ||
| // wait hosted cluster ready | ||
| WaitForNReadyNodes(t, context.Background(), guestClient, opts.NodePoolReplicas, platform) | ||
| WaitForImageRollout(t, context.Background(), h.client, hostedCluster) | ||
| ValidateHostedClusterConditions(t, context.Background(), h.client, hostedCluster, true, 10*time.Minute) | ||
| } | ||
|
|
||
| } | ||
| }) | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of putting an anonymous function here, I would create their own cleanup function under
test/e2e/util. And not just for that, I would create an EnsureXXXXX function for the whole non-cleanup function, so the code would be more readable and the cleanup function could be reused.