CNTRLPLANE-3371: Fix AllowedCIDRs e2e test for Route-based KAS

test/e2e/create_cluster_test.go

@@ -116,8 +116,11 @@ func TestCreateCluster(t *testing.T) {
 		e2eutil.EnsureDefaultSecurityGroupTags(t, ctx, mgtClient, hostedCluster, clusterOpts)
 
 		if globalOpts.Platform == hyperv1.AzurePlatform {
-			e2eutil.EnsureKubeAPIServerAllowedCIDRs(t, ctx, mgtClient, guestConfig, hostedCluster)
+			// WI webhook must run before AllowedCIDRs. AllowedCIDRs blocks and restores
+			// all KAS traffic; the webhook sidecar (FailurePolicy: Ignore) may not be
+			// ready during recovery, silently skipping mutation on pods created in that window.
 			e2eutil.EnsureAzureWorkloadIdentityWebhookMutation(t, ctx, guestClient)
+			e2eutil.EnsureKubeAPIServerAllowedCIDRs(t, ctx, mgtClient, guestConfig, hostedCluster)
 		}
 
 		e2eutil.EnsureGlobalPullSecret(t, ctx, mgtClient, hostedCluster, globalOpts.AdditionalPullSecretFile)

test/e2e/util/azure.go

@@ -10,6 +10,7 @@ import (
 	. "github.com/onsi/gomega"
 
 	corev1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/utils/ptr"
@@ -38,7 +39,7 @@ func ValidateAzureWorkloadIdentityWebhookMutation(t testing.TB, ctx context.Cont
 	}
 	g.Expect(guestClient.Create(ctx, serviceAccount)).To(Succeed(), "failed to create test service account")
 
-	pod := &corev1.Pod{
+	podTemplate := &corev1.Pod{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "azure-wi-webhook-test-pod",
 			Namespace: nsName,
@@ -70,34 +71,33 @@ func ValidateAzureWorkloadIdentityWebhookMutation(t testing.TB, ctx context.Cont
 			RestartPolicy: corev1.RestartPolicyNever,
 		},
 	}
-	g.Expect(guestClient.Create(ctx, pod)).To(Succeed(), "failed to create pod for webhook mutation test")
 
-	EventuallyObject(
-		t,
-		ctx,
-		"Azure workload identity webhook to mutate test pod",
-		func(ctx context.Context) (*corev1.Pod, error) {
-			mutatedPod := &corev1.Pod{}
-			err := guestClient.Get(ctx, types.NamespacedName{Name: pod.Name, Namespace: pod.Namespace}, mutatedPod)
-			return mutatedPod, err
-		},
-		[]Predicate[*corev1.Pod]{
-			func(mutatedPod *corev1.Pod) (bool, string, error) {
-				if hasProjectedTokenVolume(mutatedPod.Spec.Volumes) {
-					return true, "", nil
-				}
-				return false, "expected projected service account token volume to be injected", nil
-			},
-			func(mutatedPod *corev1.Pod) (bool, string, error) {
-				if hasAzureFederatedTokenEnv(mutatedPod.Spec.Containers) {
-					return true, "", nil
-				}
-				return false, "expected AZURE_FEDERATED_TOKEN_FILE env var in pod containers", nil
-			},
-		},
-		WithTimeout(3*time.Minute),
-		WithInterval(5*time.Second),
-	)
+	// The WI webhook is a MutatingAdmissionWebhook with FailurePolicy: Ignore
+	// that only fires on pod CREATE. If the webhook sidecar isn't ready when
+	// the pod is created, the pod is admitted without mutation and no amount
+	// of GET polling can recover. Delete and recreate each iteration to
+	// re-trigger admission until the webhook is ready.
+	g.Eventually(func(g Gomega) {
+		existing := &corev1.Pod{}
+		err := guestClient.Get(ctx, types.NamespacedName{Name: podTemplate.Name, Namespace: podTemplate.Namespace}, existing)
+		if err == nil {
+			g.Expect(guestClient.Delete(ctx, existing)).To(Succeed(), "failed to delete pod for retry")
+			g.Eventually(func() bool {
+				err := guestClient.Get(ctx, types.NamespacedName{Name: podTemplate.Name, Namespace: podTemplate.Namespace}, &corev1.Pod{})
+				return apierrors.IsNotFound(err)
+			}).WithContext(ctx).WithTimeout(30*time.Second).WithPolling(time.Second).Should(BeTrue(), "pod should be deleted before retry")
+		} else {
+			g.Expect(apierrors.IsNotFound(err)).To(BeTrue(), "unexpected error getting existing pod: %v", err)
+		}
+
+		fresh := podTemplate.DeepCopy()
+		g.Expect(guestClient.Create(ctx, fresh)).To(Succeed(), "failed to create pod for webhook mutation test")
+
+		mutated := &corev1.Pod{}
+		g.Expect(guestClient.Get(ctx, types.NamespacedName{Name: fresh.Name, Namespace: fresh.Namespace}, mutated)).To(Succeed())
+		g.Expect(hasProjectedTokenVolume(mutated.Spec.Volumes)).To(BeTrue(), "expected projected service account token volume to be injected")
+		g.Expect(hasAzureFederatedTokenEnv(mutated.Spec.Containers)).To(BeTrue(), "expected AZURE_FEDERATED_TOKEN_FILE env var in pod containers")
+	}).WithContext(ctx).WithTimeout(3 * time.Minute).WithPolling(10 * time.Second).Should(Succeed())
 }
 
 func EnsureAzureWorkloadIdentityWebhookMutation(t *testing.T, ctx context.Context, guestClient crclient.Client) {

test/e2e/util/util.go

@@ -3602,17 +3602,23 @@ func ValidateKubeAPIServerAllowedCIDRs(t testing.TB, ctx context.Context, mgmtCl
 			}
 		})
 		g.Expect(err).NotTo(HaveOccurred(), "failed to restore HostedCluster API server CIDRs")
-	}()
 
-	kubeClient, err := kubeclient.NewForConfig(guestConfig)
-	g.Expect(err).NotTo(HaveOccurred())
+		// Verify KAS is reachable on the original transport before returning. The
+		// AllowedCIDRs test uses cfg.Dial to create isolated transports, but subsequent
+		// tests share the original guestConfig's transport. Without this wait, the next
+		// test may start before Azure LB propagation completes the CIDR restoration.
+		g.Eventually(func(g Gomega) {
+			client, err := kubeclient.NewForConfig(guestConfig)
+			g.Expect(err).ToNot(HaveOccurred(), "failed to create kubeclient for transport recovery")
+			_, err = client.ServerVersion()
+			g.Expect(err).ToNot(HaveOccurred(), "KAS should be reachable on original transport after CIDR cleanup")
+		}).WithContext(ctx).WithTimeout(5 * time.Minute).WithPolling(10 * time.Second).Should(Succeed())
+	}()
 
 	// ensure that kube-apiserver is not reachable from anywhere
-	ensureAPIServerAllowedCIDRs(ctx, t, g, mgmtClient, kubeClient, hc, []string{"0.0.0.0/32"}, false)
+	ensureAPIServerAllowedCIDRs(ctx, t, g, mgmtClient, guestConfig, hc, []string{"0.0.0.0/32"}, false)
 	// ensure kube-apiserver is reachable when allowed CIDRs allow access from everywhere
-	// This is useful for testing purposes, as it allows us to access the kube-apiserver from any IP
-	// In a production environment, this should be restricted to specific CIDRs
-	ensureAPIServerAllowedCIDRs(ctx, t, g, mgmtClient, kubeClient, hc, append([]string{"0.0.0.0/0"}, generateTestCIDRs250()...), true)
+	ensureAPIServerAllowedCIDRs(ctx, t, g, mgmtClient, guestConfig, hc, append([]string{"0.0.0.0/0"}, generateTestCIDRs250()...), true)
 }
 
 func EnsureKubeAPIServerAllowedCIDRs(t *testing.T, ctx context.Context, mgmtClient crclient.Client, guestConfig *rest.Config, hc *hyperv1.HostedCluster) {
@@ -3621,7 +3627,7 @@ func EnsureKubeAPIServerAllowedCIDRs(t *testing.T, ctx context.Context, mgmtClie
 	})
 }
 
-func ensureAPIServerAllowedCIDRs(ctx context.Context, t testing.TB, g Gomega, mgmtClient crclient.Client, guestClient *kubeclient.Clientset, hc *hyperv1.HostedCluster, allowedCIDRs []string, shouldBeReachable bool) {
+func ensureAPIServerAllowedCIDRs(ctx context.Context, t testing.TB, g Gomega, mgmtClient crclient.Client, guestConfig *rest.Config, hc *hyperv1.HostedCluster, allowedCIDRs []string, shouldBeReachable bool) {
 	expectedCIDRs := make([]hyperv1.CIDRBlock, len(allowedCIDRs))
 	for i, cidr := range allowedCIDRs {
 		expectedCIDRs[i] = hyperv1.CIDRBlock(cidr)
@@ -3660,8 +3666,40 @@ func ensureAPIServerAllowedCIDRs(ctx context.Context, t testing.TB, g Gomega, mg
 			"HCP AllowedCIDRBlocks should match the HostedCluster spec")
 	}).WithContext(ctx).WithTimeout(time.Minute * 3).WithPolling(time.Second * 5).Should(Succeed())
 
+	// Wait for the CPO to reconcile the downstream service with the expected LoadBalancerSourceRanges.
+	// The target service depends on the APIServer publishing strategy:
+	// - Route: the "router" LB service carries the CIDRs
+	// - LoadBalancer: the KAS LB service itself carries the CIDRs
+	targetSvc := allowedCIDRsTargetService(hc, hcpNamespace)
+	if targetSvc != nil {
+		expectedSourceRanges := slices.Clone(allowedCIDRs)
+		slices.Sort(expectedSourceRanges)
+		t.Logf("Waiting for service %s/%s LoadBalancerSourceRanges to match %d CIDRs", targetSvc.Namespace, targetSvc.Name, len(expectedSourceRanges))
+		g.Eventually(func(g Gomega) {
+			svc := &corev1.Service{}
+			err := mgmtClient.Get(ctx, crclient.ObjectKeyFromObject(targetSvc), svc)
+			g.Expect(err).ToNot(HaveOccurred(), "failed to get service %s/%s", targetSvc.Namespace, targetSvc.Name)
+			actualSourceRanges := slices.Clone(svc.Spec.LoadBalancerSourceRanges)
+			slices.Sort(actualSourceRanges)
+			g.Expect(actualSourceRanges).To(Equal(expectedSourceRanges),
+				"service %s/%s LoadBalancerSourceRanges should match expected CIDRs", targetSvc.Namespace, targetSvc.Name)
+		}).WithContext(ctx).WithTimeout(time.Minute * 3).WithPolling(time.Second * 5).Should(Succeed())
+	} else {
+		t.Log("No downstream LB service identified for this cluster configuration; skipping LoadBalancerSourceRanges wait")
+	}
+
+	// A fresh kubeclient is created on every poll iteration because Go's HTTP/2 transport
+	// keeps a single TCP connection open and multiplexes all requests over it. If a poll
+	// succeeds before the LB source-range block takes effect, every later poll reuses that
+	// same connection and never sees the block. Setting cfg.Dial to a new net.Dialer gives
+	// each config a unique pointer in client-go's TLS transport cache, which forces a brand
+	// new TCP connection per iteration so each poll independently tests reachability.
 	g.Eventually(func(g Gomega) {
-		_, err = guestClient.ServerVersion()
+		cfg := rest.CopyConfig(guestConfig)
+		cfg.Dial = (&net.Dialer{Timeout: 30 * time.Second, KeepAlive: 30 * time.Second}).DialContext
+		freshClient, err := kubeclient.NewForConfig(cfg)
+		g.Expect(err).ToNot(HaveOccurred(), "failed to create kubeclient")
+		_, err = freshClient.ServerVersion()
 		if shouldBeReachable {
 			g.Expect(err).ToNot(HaveOccurred(), "kube-apiserver should be reachable")
 		} else {
@@ -3670,6 +3708,35 @@ func ensureAPIServerAllowedCIDRs(ctx context.Context, t testing.TB, g Gomega, mg
 	}).WithContext(ctx).WithTimeout(time.Minute * 3).WithPolling(time.Second * 5).Should(Succeed())
 }
 
+// allowedCIDRsTargetService returns the LoadBalancer service that enforces AllowedCIDRBlocks
+// based on the HostedCluster's APIServer publishing strategy. Returns nil when no LB service
+// carries source ranges (private clusters, NodePort, ARO HCP).
+// Mirrors CPO's API server and router service reconciliation logic.
+func allowedCIDRsTargetService(hc *hyperv1.HostedCluster, hcpNamespace string) *corev1.Service {
+	if !netutil.IsPublicHC(hc) {
+		return nil
+	}
+	strategy := netutil.ServicePublishingStrategyByTypeByHC(hc, hyperv1.APIServer)
+	if strategy == nil {
+		return nil
+	}
+	switch strategy.Type {
+	case hyperv1.Route:
+		if azureutil.IsAroHCP() {
+			return nil
+		}
+		return cpomanifests.RouterPublicService(hcpNamespace)
+	case hyperv1.LoadBalancer:
+		if hc.Spec.Platform.Type == hyperv1.AzurePlatform ||
+			(hc.Annotations != nil && hc.Annotations[hyperv1.ManagementPlatformAnnotation] == string(hyperv1.AzurePlatform)) {
+			return cpomanifests.KubeAPIServerServiceAzureLB(hcpNamespace)
+		}
+		return cpomanifests.KubeAPIServerService(hcpNamespace)
+	default:
+		return nil
+	}
+}
+
 // generateTestCIDRs250 is a helper to generate 250 /32 CIDRs starting at 250.250.250.1
 func generateTestCIDRs250() []string {
 	cidrs := make([]string, 0, 250)

test/e2e/util/util_test.go

@@ -7,9 +7,129 @@ import (
 
 	. "github.com/onsi/gomega"
 
+	hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1"
+	"github.com/openshift/hypershift/support/azureutil"
 	"github.com/openshift/hypershift/support/certs"
+
+	"k8s.io/utils/ptr"
 )
 
+func TestAllowedCIDRsTargetService(t *testing.T) {
+	const ns = "test-hcp"
+
+	publicHC := func(platform hyperv1.PlatformType, svcType hyperv1.PublishingStrategyType) *hyperv1.HostedCluster {
+		hc := &hyperv1.HostedCluster{
+			Spec: hyperv1.HostedClusterSpec{
+				Platform: hyperv1.PlatformSpec{Type: platform},
+				Services: []hyperv1.ServicePublishingStrategyMapping{{
+					Service:                   hyperv1.APIServer,
+					ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{Type: svcType},
+				}},
+			},
+		}
+		switch platform {
+		case hyperv1.AWSPlatform:
+			hc.Spec.Platform.AWS = ptr.To(hyperv1.AWSPlatformSpec{EndpointAccess: hyperv1.Public})
+		case hyperv1.AzurePlatform:
+			hc.Spec.Platform.Azure = ptr.To(hyperv1.AzurePlatformSpec{Topology: hyperv1.AzureTopologyPublic})
+		}
+		return hc
+	}
+
+	tests := []struct {
+		name     string
+		hc       *hyperv1.HostedCluster
+		aroHCP   bool
+		wantName string
+		wantNil  bool
+	}{
+		{
+			name:     "When Route strategy on AWS it should return the router service",
+			hc:       publicHC(hyperv1.AWSPlatform, hyperv1.Route),
+			wantName: "router",
+		},
+		{
+			name:     "When Route strategy on Azure self-managed it should return the router service",
+			hc:       publicHC(hyperv1.AzurePlatform, hyperv1.Route),
+			wantName: "router",
+		},
+		{
+			name:    "When Route strategy on ARO HCP it should return nil",
+			hc:      publicHC(hyperv1.AzurePlatform, hyperv1.Route),
+			aroHCP:  true,
+			wantNil: true,
+		},
+		{
+			name:     "When LoadBalancer strategy on Azure it should return the Azure LB service",
+			hc:       publicHC(hyperv1.AzurePlatform, hyperv1.LoadBalancer),
+			wantName: "kube-apiserverlb",
+		},
+		{
+			name: "When LoadBalancer strategy with Azure management annotation it should return the Azure LB service",
+			hc: func() *hyperv1.HostedCluster {
+				hc := publicHC(hyperv1.NonePlatform, hyperv1.LoadBalancer)
+				hc.Annotations = map[string]string{
+					hyperv1.ManagementPlatformAnnotation: string(hyperv1.AzurePlatform),
+				}
+				return hc
+			}(),
+			wantName: "kube-apiserverlb",
+		},
+		{
+			name:     "When LoadBalancer strategy on AWS it should return the KAS service",
+			hc:       publicHC(hyperv1.AWSPlatform, hyperv1.LoadBalancer),
+			wantName: "kube-apiserver",
+		},
+		{
+			name: "When private Azure cluster it should return nil",
+			hc: &hyperv1.HostedCluster{
+				Spec: hyperv1.HostedClusterSpec{
+					Platform: hyperv1.PlatformSpec{
+						Type:  hyperv1.AzurePlatform,
+						Azure: ptr.To(hyperv1.AzurePlatformSpec{Topology: hyperv1.AzureTopologyPrivate}),
+					},
+					Services: []hyperv1.ServicePublishingStrategyMapping{{
+						Service:                   hyperv1.APIServer,
+						ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{Type: hyperv1.Route},
+					}},
+				},
+			},
+			wantNil: true,
+		},
+		{
+			name:    "When NodePort strategy it should return nil",
+			hc:      publicHC(hyperv1.AWSPlatform, hyperv1.NodePort),
+			wantNil: true,
+		},
+		{
+			name: "When no APIServer strategy it should return nil",
+			hc: func() *hyperv1.HostedCluster {
+				hc := publicHC(hyperv1.AWSPlatform, hyperv1.Route)
+				hc.Spec.Services = nil
+				return hc
+			}(),
+			wantNil: true,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			g := NewWithT(t)
+			if tc.aroHCP {
+				azureutil.SetAsAroHCPTest(t)
+			}
+			svc := allowedCIDRsTargetService(tc.hc, ns)
+			if tc.wantNil {
+				g.Expect(svc).To(BeNil())
+			} else {
+				g.Expect(svc).ToNot(BeNil())
+				g.Expect(svc.Name).To(Equal(tc.wantName))
+				g.Expect(svc.Namespace).To(Equal(ns))
+			}
+		})
+	}
+}
+
 // TestGenerateCustomCertificate verifies that our certificate generation works correctly
 func TestGenerateCustomCertificate(t *testing.T) {
 	testsCases := []struct {