Merge pull request #31000 from gcs278/fix-gwapi-upgrade-ipv6-skip

OCPBUGS-83267: Use upgrades.Skippable for Gateway API upgrade test skip logic

Author: openshift-merge-bot[bot]

test/extended/router/gatewayapi_upgrade.go

@@ -30,6 +30,7 @@ type GatewayAPIUpgradeTest struct {
 	startedWithNoOLM      bool // tracks if GatewayAPIWithoutOLM was enabled at start
 	loadBalancerSupported bool
 	managedDNS            bool
+	precheckErr           error // error from Skip() to surface in Setup()
 }
 
 func (t *GatewayAPIUpgradeTest) Name() string {
@@ -40,25 +41,48 @@ func (t *GatewayAPIUpgradeTest) DisplayName() string {
 	return "[sig-network-edge][Feature:Router][apigroup:gateway.networking.k8s.io] Verify Gateway API functionality during upgrade"
 }
 
+// Skip checks if this upgrade test should be skipped. This is called by the
+// disruption framework before Setup.
+func (t *GatewayAPIUpgradeTest) Skip(_ upgrades.UpgradeContext) bool {
+	oc := exutil.NewCLIForMonitorTest("gateway-api-upgrade-skip").AsAdmin()
+
+	t.precheckErr = nil
+	noOLM, err := isNoOLMFeatureGateEnabled(oc)
+	if err != nil {
+		t.precheckErr = fmt.Errorf("failed to check GatewayAPIWithoutOLM feature gate: %w", err)
+		return false
+	}
+
+	skip, reason, err := shouldSkipGatewayAPITests(oc, noOLM)
+	if err != nil {
+		t.precheckErr = fmt.Errorf("failed to check Gateway API skip conditions: %w", err)
+		return false
+	}
+	if skip {
+		g.By(fmt.Sprintf("skipping test: %s", reason))
+		return true
+	}
+
+	return false
+}
+
 // Setup creates Gateway and HTTPRoute resources and tests connectivity
 func (t *GatewayAPIUpgradeTest) Setup(ctx context.Context, f *e2e.Framework) {
 	g.By("Setting up Gateway API upgrade test")
+	o.Expect(t.precheckErr).NotTo(o.HaveOccurred(), "Skip() precheck failed: could not determine if Gateway API upgrade test should run")
 
 	t.oc = exutil.NewCLIWithFramework(f).AsAdmin()
 	t.namespace = f.Namespace.Name
 	t.gatewayName = "upgrade-test-gateway"
 	t.routeName = "test-httproute"
 
-	// Check platform support and get capabilities (LoadBalancer, DNS)
-	t.loadBalancerSupported, t.managedDNS = checkPlatformSupportAndGetCapabilities(t.oc)
+	// Get platform capabilities (skip checks already handled by Skip())
+	t.loadBalancerSupported, t.managedDNS = getPlatformCapabilities(t.oc)
 
 	g.By("Checking if GatewayAPIWithoutOLM feature gate is enabled before upgrade")
-	t.startedWithNoOLM = isNoOLMFeatureGateEnabled(t.oc)
-
-	// Skip on clusters missing OLM/Marketplace capabilities if starting with OLM mode
-	if !t.startedWithNoOLM {
-		exutil.SkipIfMissingCapabilities(t.oc, olmCapabilities...)
-	}
+	var noOLMErr error
+	t.startedWithNoOLM, noOLMErr = isNoOLMFeatureGateEnabled(t.oc)
+	o.Expect(noOLMErr).NotTo(o.HaveOccurred())
 
 	if t.startedWithNoOLM {
 		e2e.Logf("Starting with GatewayAPIWithoutOLM enabled (NO-OLM mode)")
@@ -135,7 +159,8 @@ func (t *GatewayAPIUpgradeTest) Test(ctx context.Context, f *e2e.Framework, done
 	o.Expect(err).NotTo(o.HaveOccurred(), "Gateway should remain programmed")
 
 	g.By("Checking if GatewayAPIWithoutOLM feature gate is enabled after upgrade")
-	endsWithNoOLM := isNoOLMFeatureGateEnabled(t.oc)
+	endsWithNoOLM, err := isNoOLMFeatureGateEnabled(t.oc)
+	o.Expect(err).NotTo(o.HaveOccurred())
 
 	// Determine if migration happened: started with OLM, ended with NO-OLM
 	migrationOccurred := !t.startedWithNoOLM && endsWithNoOLM

test/extended/router/gatewayapicontroller.go

@@ -115,6 +115,7 @@ var _ = g.Describe("[sig-network-edge][OCPFeatureGate:GatewayAPIController][Feat
 		infPoolCRD            = "https://raw.githubusercontent.com/kubernetes-sigs/gateway-api-inference-extension/main/config/crd/bases/inference.networking.k8s.io_inferencepools.yaml"
 		managedDNS            bool
 		loadBalancerSupported bool
+		noOLM                 bool
 	)
 
 	const (
@@ -123,16 +124,16 @@ var _ = g.Describe("[sig-network-edge][OCPFeatureGate:GatewayAPIController][Feat
 		openshiftOperatorsNamespace = "openshift-operators"
 	)
 	g.BeforeEach(func() {
-		// Check platform support and get capabilities (LoadBalancer, DNS)
-		loadBalancerSupported, managedDNS = checkPlatformSupportAndGetCapabilities(oc)
+		noOLM, err = isNoOLMFeatureGateEnabled(oc)
+		o.Expect(err).NotTo(o.HaveOccurred())
 
-		if !isNoOLMFeatureGateEnabled(oc) {
-			// GatewayAPIController without GatewayAPIWithoutOLM featuregate
-			// relies on OSSM OLM operator.
-			// Skipping on clusters which don't have capabilities required
-			// to install an OLM operator.
-			exutil.SkipIfMissingCapabilities(oc, olmCapabilities...)
+		// Check platform support, capabilities, and skip conditions
+		skip, reason, err := shouldSkipGatewayAPITests(oc, noOLM)
+		o.Expect(err).NotTo(o.HaveOccurred())
+		if skip {
+			g.Skip(reason)
 		}
+		loadBalancerSupported, managedDNS = getPlatformCapabilities(oc)
 		// create the default gatewayClass
 		gatewayClass := buildGatewayClass(gatewayClassName, gatewayClassControllerName)
 		_, err = oc.AdminGatewayApiClient().GatewayV1().GatewayClasses().Create(context.TODO(), gatewayClass, metav1.CreateOptions{})
@@ -157,7 +158,7 @@ var _ = g.Describe("[sig-network-edge][OCPFeatureGate:GatewayAPIController][Feat
 			if err := oc.AdminGatewayApiClient().GatewayV1().GatewayClasses().Delete(context.Background(), gatewayClassName, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) {
 				e2e.Failf("Failed to delete GatewayClass %q", gatewayClassName)
 			}
-			if isNoOLMFeatureGateEnabled(oc) {
+			if noOLM {
 				g.By("Waiting for the istiod pod to be deleted")
 				waitForIstiodPodDeletion(oc)
 			} else {
@@ -283,7 +284,7 @@ var _ = g.Describe("[sig-network-edge][OCPFeatureGate:GatewayAPIController][Feat
 	g.It("Ensure OSSM and OLM related resources are created after creating GatewayClass", func() {
 		defer markTestDone(oc, ossmAndOLMResourcesCreated)
 		// these will fail since no OLM Resources will be available
-		if isNoOLMFeatureGateEnabled(oc) {
+		if noOLM {
 			g.Skip("Skip this test since it requires OLM resources")
 		}
 
@@ -313,7 +314,7 @@ var _ = g.Describe("[sig-network-edge][OCPFeatureGate:GatewayAPIController][Feat
 		errCheck := checkGatewayClassCondition(oc, customGatewayClassName, string(gatewayapiv1.GatewayClassConditionStatusAccepted), metav1.ConditionTrue)
 		o.Expect(errCheck).NotTo(o.HaveOccurred(), "GatewayClass %q was not installed and accepted", gwc.Name)
 
-		if isNoOLMFeatureGateEnabled(oc) {
+		if noOLM {
 			g.By("Check the GatewayClass conditions")
 			errCheck = checkGatewayClassCondition(oc, customGatewayClassName, gatewayClassControllerInstalledConditionType, metav1.ConditionTrue)
 			o.Expect(errCheck).NotTo(o.HaveOccurred(), "GatewayClass %q does not have the ControllerInstalled condition", customGatewayClassName)
@@ -340,7 +341,7 @@ var _ = g.Describe("[sig-network-edge][OCPFeatureGate:GatewayAPIController][Feat
 		defaultCheck := checkGatewayClassCondition(oc, gatewayClassName, string(gatewayapiv1.GatewayClassConditionStatusAccepted), metav1.ConditionTrue)
 		o.Expect(defaultCheck).NotTo(o.HaveOccurred())
 
-		if !isNoOLMFeatureGateEnabled(oc) {
+		if !noOLM {
 			g.By("Confirm that ISTIO CR is created and in healthy state")
 			waitForIstioHealthy(oc, 20*time.Minute)
 		}
@@ -482,7 +483,7 @@ var _ = g.Describe("[sig-network-edge][OCPFeatureGate:GatewayAPIController][Feat
 		g.By(fmt.Sprintf("Wait until the istiod deployment in %s namespace is automatically created successfully", ingressNamespace))
 		pollWaitDeploymentCreated(oc, ingressNamespace, istiodDeployment, deployment.CreationTimestamp)
 
-		if !isNoOLMFeatureGateEnabled(oc) {
+		if !noOLM {
 			// delete the istio and check if it is restored
 			g.By(fmt.Sprintf("Try to delete the istio %s", istioName))
 			istioOriginalCreatedTimestamp, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("-n", ingressNamespace, "istio/"+istioName, `-o=jsonpath={.metadata.creationTimestamp}`).Output()
@@ -548,44 +549,80 @@ var _ = g.Describe("[sig-network-edge][OCPFeatureGate:GatewayAPIController][Feat
 	})
 })
 
-// checkPlatformSupportAndGetCapabilities verifies the platform is supported and returns
-// platform capabilities for LoadBalancer services and managed DNS.
-func checkPlatformSupportAndGetCapabilities(oc *exutil.CLI) (loadBalancerSupported bool, managedDNS bool) {
-	// Skip on OKD since OSSM is not available as a community operator
+// shouldSkipGatewayAPITests checks if Gateway API tests should be skipped on this cluster.
+// It returns (skip bool, reason string, err error). An error indicates a failure to
+// determine skip status and should be treated as a test failure, not a skip.
+// This function avoids calling g.Skip() or o.Expect() so it is safe to call from
+// upgrade test Skip() methods that run outside of Ginkgo leaf nodes.
+func shouldSkipGatewayAPITests(oc *exutil.CLI, noOLM bool) (bool, string, error) {
 	// TODO: Determine if we can enable and start testing OKD with Sail Library
 	isokd, err := isOKD(oc)
-	o.Expect(err).NotTo(o.HaveOccurred(), "Failed to get clusterversion to determine if release is OKD")
+	if err != nil {
+		return false, "", fmt.Errorf("failed to determine if release is OKD: %v", err)
+	}
 	if isokd {
-		g.Skip("Skipping on OKD cluster as OSSM is not available as a community operator")
+		return true, "Skipping on OKD cluster as OSSM is not available as a community operator", nil
 	}
 
 	infra, err := oc.AdminConfigClient().ConfigV1().Infrastructures().Get(context.Background(), "cluster", metav1.GetOptions{})
-	o.Expect(err).NotTo(o.HaveOccurred())
-	o.Expect(infra).NotTo(o.BeNil())
+	if err != nil {
+		return false, "", fmt.Errorf("failed to get infrastructure: %v", err)
+	}
 
-	o.Expect(infra.Status.PlatformStatus).NotTo(o.BeNil())
+	if infra.Status.PlatformStatus == nil {
+		return false, "", fmt.Errorf("infrastructure PlatformStatus is nil")
+	}
 	platformType := infra.Status.PlatformStatus.Type
-	o.Expect(platformType).NotTo(o.BeEmpty())
 	switch platformType {
+	case configv1.AWSPlatformType,
+		configv1.AzurePlatformType,
+		configv1.GCPPlatformType,
+		configv1.IBMCloudPlatformType,
+		configv1.VSpherePlatformType,
+		configv1.BareMetalPlatformType,
+		configv1.EquinixMetalPlatformType:
+		// supported
+	default:
+		return true, fmt.Sprintf("Skipping on unsupported platform type %q", platformType), nil
+	}
+
+	ipv6, err := isIPv6OrDualStack(oc)
+	if err != nil {
+		return false, "", fmt.Errorf("failed to check IPv6/dual-stack: %v", err)
+	}
+	if ipv6 {
+		return true, "Skipping Gateway API tests on IPv6/dual-stack cluster", nil
+	}
+
+	if !noOLM {
+		enabled, err := exutil.AllCapabilitiesEnabled(oc, olmCapabilities...)
+		if err != nil {
+			return false, "", fmt.Errorf("failed to check OLM capabilities: %v", err)
+		}
+		if !enabled {
+			return true, "Skipping: OLM/Marketplace capabilities are not enabled and GatewayAPIWithoutOLM is not enabled", nil
+		}
+	}
+
+	return false, "", nil
+}
+
+// getPlatformCapabilities returns platform capabilities for LoadBalancer services and managed DNS.
+func getPlatformCapabilities(oc *exutil.CLI) (loadBalancerSupported bool, managedDNS bool) {
+	infra, err := oc.AdminConfigClient().ConfigV1().Infrastructures().Get(context.Background(), "cluster", metav1.GetOptions{})
+	o.Expect(err).NotTo(o.HaveOccurred())
+	o.Expect(infra.Status.PlatformStatus).NotTo(o.BeNil())
+
+	switch infra.Status.PlatformStatus.Type {
 	case configv1.AWSPlatformType, configv1.AzurePlatformType, configv1.GCPPlatformType, configv1.IBMCloudPlatformType:
-		// Cloud platforms with native LoadBalancer support
 		loadBalancerSupported = true
 	case configv1.VSpherePlatformType, configv1.BareMetalPlatformType, configv1.EquinixMetalPlatformType:
-		// Platforms without native LoadBalancer support (may have MetalLB or similar)
 		loadBalancerSupported = false
-	default:
-		g.Skip(fmt.Sprintf("Skipping on unsupported platform type %q", platformType))
 	}
 
-	// Check if DNS is managed (has public or private zones configured)
 	managedDNS = isDNSManaged(oc)
 
-	// Skip Gateway API tests on IPv6 or dual-stack clusters (any platform)
-	if isIPv6OrDualStack(oc) {
-		g.Skip("Skipping Gateway API tests on IPv6/dual-stack cluster")
-	}
-
-	e2e.Logf("Platform: %s, LoadBalancer supported: %t, DNS managed: %t", platformType, loadBalancerSupported, managedDNS)
+	e2e.Logf("Platform: %s, LoadBalancer supported: %t, DNS managed: %t", infra.Status.PlatformStatus.Type, loadBalancerSupported, managedDNS)
 	return loadBalancerSupported, managedDNS
 }
 
@@ -599,30 +636,34 @@ func isDNSManaged(oc *exutil.CLI) bool {
 
 // isIPv6OrDualStack checks if the cluster is using IPv6 or dual-stack networking.
 // Returns true if any ServiceNetwork CIDR is IPv6 (indicates IPv6-only or dual-stack).
-func isIPv6OrDualStack(oc *exutil.CLI) bool {
+func isIPv6OrDualStack(oc *exutil.CLI) (bool, error) {
 	networkConfig, err := oc.AdminOperatorClient().OperatorV1().Networks().Get(context.Background(), "cluster", metav1.GetOptions{})
-	o.Expect(err).NotTo(o.HaveOccurred(), "Failed to get network config")
+	if err != nil {
+		return false, fmt.Errorf("failed to get network config: %v", err)
+	}
 
 	for _, cidr := range networkConfig.Spec.ServiceNetwork {
 		if utilnet.IsIPv6CIDRString(cidr) {
-			return true
+			return true, nil
 		}
 	}
-	return false
+	return false, nil
 }
 
-func isNoOLMFeatureGateEnabled(oc *exutil.CLI) bool {
+func isNoOLMFeatureGateEnabled(oc *exutil.CLI) (bool, error) {
 	fgs, err := oc.AdminConfigClient().ConfigV1().FeatureGates().Get(context.TODO(), "cluster", metav1.GetOptions{})
-	o.Expect(err).NotTo(o.HaveOccurred(), "Error getting cluster FeatureGates.")
+	if err != nil {
+		return false, fmt.Errorf("failed to get cluster FeatureGates: %v", err)
+	}
 	for _, fg := range fgs.Status.FeatureGates {
 		for _, enabledFG := range fg.Enabled {
 			if enabledFG.Name == "GatewayAPIWithoutOLM" {
 				e2e.Logf("GatewayAPIWithoutOLM featuregate is enabled")
-				return true
+				return true, nil
 			}
 		}
 	}
-	return false
+	return false, nil
 }
 
 func waitForIstioHealthy(oc *exutil.CLI, timeout time.Duration) {