Merge pull request #444 from bharath-b-rh/cm-1141

CM-1141: Drops cluster FeatureSet gate for TP TrustManager feature

Author: openshift-merge-bot[bot]

pkg/features/features.go

@@ -140,19 +140,21 @@ func readClusterPreviewFeatureGate(ctx context.Context, configClient configv1cli
 // (--unsupported-addon-features=IstioCSR=true). FeatureIstioCSR is GA and does not depend on the
 // cluster FeatureSet; the featuregate remains available so users can disable it when unused.
 func IsIstioCSRFeatureGateEnabled() bool {
-	return DefaultFeatureGate.Enabled(v1alpha1.FeatureIstioCSR)
+	if !DefaultFeatureGate.Enabled(v1alpha1.FeatureIstioCSR) {
+		log.V(1).Info("IstioCSR feature: internal featuregate is not enabled")
+		return false
+	}
+	log.V(1).Info("IstioCSR feature: enabled")
+	return true
 }
 
 // IsTrustManagerFeatureGateEnabled reports whether the TrustManager operand may run.
-// When config.openshift.io FeatureGate is served, the cluster must use a preview FeatureSet
-// (e.g. TechPreviewNoUpgrade); spec.featureSet Default does not start TrustManager even if
-// --unsupported-addon-features=TrustManager=true. The internal operator featuregate must also be on.
-// When stateErr is set, TrustManager stays off. When the FeatureGate API is not served, only the
-// internal gate applies.
+//
+// TrustManager remains a TechPreview feature and is gated by the internal operator featuregate
+// (--unsupported-addon-features=TrustManager=true). The cluster FeatureSet check
+// (featuregates.config.openshift.io/cluster spec.featureSet) was removed: for increased adoption.
+// Cluster FeatureSet state is still tracked via FeatureGateState but does not gate TrustManager.
 func (f *FeatureGateState) IsTrustManagerFeatureGateEnabled() bool {
-	if !f.passesClusterPreviewGating(string(v1alpha1.FeatureTrustManager)) {
-		return false
-	}
 	if !DefaultFeatureGate.Enabled(v1alpha1.FeatureTrustManager) {
 		log.V(1).Info("TrustManager feature: internal featuregate is not enabled")
 		return false

pkg/features/features_test.go

@@ -158,8 +158,8 @@ func TestAllowedPreviewClusterFeatureSets(t *testing.T) {
 	}
 }
 
-// TestIsTrustManagerFeatureGateEnabled covers cluster featureset plus TrustManager operator
-// featuregate (--unsupported-addon-features).
+// TestIsTrustManagerFeatureGateEnabled covers the TrustManager operator featuregate
+// (--unsupported-addon-features).
 func TestIsTrustManagerFeatureGateEnabled(t *testing.T) {
 	defer func() {
 		_ = SetupWithFlagValue("TrustManager=false")
@@ -171,9 +171,10 @@ func TestIsTrustManagerFeatureGateEnabled(t *testing.T) {
 		assert func(t *testing.T, st *FeatureGateState)
 	}{
 		{
-			name: "returns false when cluster feature gate discovery fails",
+			name: "returns true when operator featuregate on even if cluster feature gate discovery fails",
 			prep: func(t *testing.T) *configfake.Clientset {
 				t.Helper()
+				require.NoError(t, SetupWithFlagValue("TrustManager=true"))
 				cs := newFakeConfigClient(t, true, clusterFeatureGateObject(ocpfeaturegate.TechPreviewNoUpgrade))
 				cs.AddReactor("get", "resource", func(clientgotesting.Action) (bool, runtime.Object, error) {
 					return true, nil, fmt.Errorf("simulated discovery failure")
@@ -182,13 +183,13 @@ func TestIsTrustManagerFeatureGateEnabled(t *testing.T) {
 			},
 			assert: func(t *testing.T, st *FeatureGateState) {
 				t.Helper()
-				assert.False(t, st.IsTrustManagerFeatureGateEnabled())
+				assert.True(t, st.IsTrustManagerFeatureGateEnabled())
 				require.Error(t, st.Err())
 				assert.True(t, errors.Is(st.Err(), ErrFeatureGateDiscovery))
 			},
 		},
 		{
-			name: "returns false when featuregates/cluster cannot be read",
+			name: "returns true when operator featuregate on even if featuregates/cluster cannot be read",
 			prep: func(t *testing.T) *configfake.Clientset {
 				t.Helper()
 				require.NoError(t, SetupWithFlagValue("TrustManager=true"))
@@ -203,7 +204,7 @@ func TestIsTrustManagerFeatureGateEnabled(t *testing.T) {
 			},
 			assert: func(t *testing.T, st *FeatureGateState) {
 				t.Helper()
-				assert.False(t, st.IsTrustManagerFeatureGateEnabled())
+				assert.True(t, st.IsTrustManagerFeatureGateEnabled())
 				assert.True(t, errors.Is(st.Err(), ErrFeatureGateClusterGet))
 			},
 		},
@@ -221,15 +222,15 @@ func TestIsTrustManagerFeatureGateEnabled(t *testing.T) {
 			},
 		},
 		{
-			name: "returns false when cluster featureset is Default even with operator featuregate set",
+			name: "returns true when cluster featureset is Default with operator featuregate set",
 			prep: func(t *testing.T) *configfake.Clientset {
 				t.Helper()
 				require.NoError(t, SetupWithFlagValue("TrustManager=true"))
 				return newFakeConfigClient(t, true, clusterFeatureGateObject(ocpfeaturegate.Default))
 			},
 			assert: func(t *testing.T, st *FeatureGateState) {
 				t.Helper()
-				assert.False(t, st.IsTrustManagerFeatureGateEnabled())
+				assert.True(t, st.IsTrustManagerFeatureGateEnabled())
 			},
 		},
 		{

pkg/operator/starter.go

@@ -167,12 +167,11 @@ func RunOperator(ctx context.Context, cc *controllercmd.ControllerContext) error
 }
 
 // setupFeatureGates applies operator feature flags from UnsupportedAddonFeatures
-// (--unsupported-addon-features) and builds cluster-side FeatureGateState used for
-// feature preview gating. Transient discovery or featuregates/cluster read errors
+// (--unsupported-addon-features) and builds cluster-side FeatureGateState by reading
+// featuregates/cluster when served. Transient discovery or featuregates/cluster read errors
 // are retried up to three times with 30s backoff; persistent failure leaves stateErr set
-// so feature stays off without aborting the rest of startup. ErrNilConfigClient
-// is not retried. Returns an error only for invalid addon feature syntax or if ctx is
-// canceled while waiting between retries.
+// on FeatureGateState without aborting the rest of startup. Returns an error only for invalid
+// addon feature syntax or if ctx is canceled while waiting between retries.
 func setupFeatureGates(ctx context.Context, configClient configv1client.Interface) (*features.FeatureGateState, error) {
 	const (
 		// maxFeatureGateAttempts caps how many times we call NewFeatureGateState when discovery or

test/e2e/istio_csr_operand_test.go

@@ -261,9 +261,12 @@ var _ = Describe("Istio-CSR operand coverage [apigroup:operator.openshift.io]",
 		By("waiting for IstioCSR Ready=False with multiple-instance rejection message")
 		Expect(waitForIstioCSRConditionMessage(ctx, loader, secondNS.Name, istioCSRResourceName, v1alpha1.Ready, metav1.ConditionFalse, "multiple instances of istiocsr exists", highTimeout, slowPollInterval)).NotTo(HaveOccurred())
 
-		obj, err := loader.DynamicClient.Resource(istiocsrSchema).Namespace(secondNS.Name).Get(ctx, istioCSRResourceName, metav1.GetOptions{})
-		Expect(err).NotTo(HaveOccurred())
-		Expect(obj.GetAnnotations()).To(HaveKey(istioCSRRejectAnnotation))
+		By("waiting for reject-multiple-instance annotation (written after status in a separate API update)")
+		Eventually(func(g Gomega) {
+			obj, err := loader.DynamicClient.Resource(istiocsrSchema).Namespace(secondNS.Name).Get(ctx, istioCSRResourceName, metav1.GetOptions{})
+			g.Expect(err).NotTo(HaveOccurred())
+			g.Expect(obj.GetAnnotations()).To(HaveKey(istioCSRRejectAnnotation))
+		}, lowTimeout, fastPollInterval).Should(Succeed())
 
 		Consistently(func() bool {
 			_, err := clientset.AppsV1().Deployments(secondNS.Name).Get(ctx, istioCSRGRPCServiceName, metav1.GetOptions{})

test/e2e/trustmanager_test.go

@@ -57,7 +57,7 @@ const (
 	trustedCABundleKey           = "ca-bundle.crt"
 )
 
-// Cluster must have an allowed feature set (e.g. TechPreviewNoUpgrade). TrustManager is deployed and reconciled.
+// TrustManager is deployed and reconciled when --unsupported-addon-features=TrustManager=true.
 var _ = Describe("TrustManager", Ordered, Label("Platform:Generic", "Feature:TrustManager", "TechPreview"), func() {
 	var (
 		ctx = context.Background()
@@ -1464,9 +1464,10 @@ var _ = Describe("TrustManager", Ordered, Label("Platform:Generic", "Feature:Tru
 	})
 })
 
-// Cluster has featuregates.config.openshift.io spec.featureSet at Default. TrustManager controller is not enabled; operand should not be deployed.
-// TechPreview:Inverted labels test scenarios that invert the TechPreview test suite—validating behavior when Tech Preview feature is not enabled.
-var _ = Describe("TrustManager with Default feature set", Ordered, Label("Platform:Generic", "Feature:TrustManager", "TechPreview:Inverted"), func() {
+// TrustManager operator feature gate defaults to disabled (unlike IstioCSR). Creating a TrustManager CR
+// must not deploy the operand when --unsupported-addon-features does not enable TrustManager.
+// TechPreview:Inverted labels test scenarios that invert the main TrustManager suite (feature gate off).
+var _ = Describe("TrustManager with operator feature gate disabled", Ordered, Label("Platform:Generic", "Feature:TrustManager", "TechPreview:Inverted"), func() {
 	var (
 		ctx = context.Background()
 
@@ -1475,16 +1476,16 @@ var _ = Describe("TrustManager with Default feature set", Ordered, Label("Platfo
 		originalOperatorLogLevel         string
 	)
 
-	BeforeAll(trustManagerBeforeAll(ctx, &clientset, &originalUnsupportedAddonFeatures, &originalOperatorLogLevel))
+	BeforeAll(trustManagerFeatureGateDisabledBeforeAll(ctx, &clientset, &originalUnsupportedAddonFeatures, &originalOperatorLogLevel))
 
 	AfterAll(trustManagerAfterAll(ctx, &originalUnsupportedAddonFeatures, &originalOperatorLogLevel))
 
 	BeforeEach(trustManagerBeforeEach())
 
 	AfterEach(trustManagerAfterEach(ctx))
 
-	It("should not create ServiceAccount or populate status when cluster feature set is Default", func() {
-		By("creating TrustManager CR with default settings (same as TechPreview scenario)")
+	It("should not create ServiceAccount or populate status when operator feature gate is disabled", func() {
+		By("creating TrustManager CR with default settings")
 		_, err := trustManagerClient().Create(ctx, &v1alpha1.TrustManager{
 			ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
 			Spec: v1alpha1.TrustManagerSpec{
@@ -1497,10 +1498,10 @@ var _ = Describe("TrustManager with Default feature set", Ordered, Label("Platfo
 		_, err = trustManagerClient().Get(ctx, "cluster", metav1.GetOptions{})
 		Expect(err).ShouldNot(HaveOccurred())
 
-		By("verifying ServiceAccount is not created and TrustManager status stays unset (controller not running for Default feature set)")
+		By("verifying ServiceAccount is not created and TrustManager status stays unset (controller not running without feature gate)")
 		Consistently(func(g Gomega) {
 			_, err := clientset.CoreV1().ServiceAccounts(trustManagerNamespace).Get(ctx, trustManagerServiceAccountName, metav1.GetOptions{})
-			g.Expect(apierrors.IsNotFound(err)).To(BeTrue(), "ServiceAccount %s/%s must not exist when cluster feature set is Default", trustManagerNamespace, trustManagerServiceAccountName)
+			g.Expect(apierrors.IsNotFound(err)).To(BeTrue(), "ServiceAccount %s/%s must not exist when TrustManager feature gate is disabled", trustManagerNamespace, trustManagerServiceAccountName)
 
 			tm, err := trustManagerClient().Get(ctx, "cluster", metav1.GetOptions{})
 			g.Expect(err).NotTo(HaveOccurred())
@@ -1620,7 +1621,7 @@ func findSecretRule(rules []rbacv1.PolicyRule, verb string) *rbacv1.PolicyRule {
 
 // trustManagerRemoveStaleOperandServiceAccount deletes the trust-manager operand ServiceAccount if it
 // exists and waits until it is gone. TrustManager CR deletion does not remove this SA today, so a
-// prior e2e run or TechPreview block can leave it and break the Default-feature-set scenario.
+// prior e2e run can leave it behind and affect later tests.
 //
 // TODO: Remove this when TrustManager is GA and operand teardown removes the ServiceAccount (or
 // equivalent) when the TrustManager CR is deleted.
@@ -1633,6 +1634,37 @@ func trustManagerRemoveStaleOperandServiceAccount(ctx context.Context, cs *kuber
 	}, lowTimeout, fastPollInterval).Should(BeTrue())
 }
 
+func trustManagerFeatureGateDisabledBeforeAll(ctx context.Context, clientset **kubernetes.Clientset, unsupportedAddonFeatures, operatorLogLevel *string) func() {
+	return func() {
+		cs, err := kubernetes.NewForConfig(cfg)
+		Expect(err).Should(BeNil())
+		*clientset = cs
+
+		trustManagerRemoveStaleOperandServiceAccount(ctx, cs)
+
+		By("capturing original UNSUPPORTED_ADDON_FEATURES from subscription before patching")
+		*unsupportedAddonFeatures, err = getSubscriptionEnvVar(ctx, loader, "UNSUPPORTED_ADDON_FEATURES")
+		Expect(err).NotTo(HaveOccurred())
+
+		By("capturing original OPERATOR_LOG_LEVEL from subscription before patching")
+		*operatorLogLevel, err = getSubscriptionEnvVar(ctx, loader, "OPERATOR_LOG_LEVEL")
+		Expect(err).NotTo(HaveOccurred())
+
+		fmt.Fprintf(GinkgoWriter, "TrustManager feature gate disabled BeforeAll: captured UNSUPPORTED_ADDON_FEATURES=%q OPERATOR_LOG_LEVEL=%q\n",
+			*unsupportedAddonFeatures, *operatorLogLevel)
+
+		By("ensuring TrustManager operator feature gate is not enabled via subscription")
+		err = patchSubscriptionWithEnvVars(ctx, loader, map[string]string{
+			"UNSUPPORTED_ADDON_FEATURES": "",
+		})
+		Expect(err).NotTo(HaveOccurred())
+
+		By("waiting for operator deployment to rollout without UNSUPPORTED_ADDON_FEATURES")
+		err = waitForDeploymentEnvVarRemovedAndRollout(ctx, operatorNamespace, operatorDeploymentName, "UNSUPPORTED_ADDON_FEATURES", lowTimeout)
+		Expect(err).NotTo(HaveOccurred())
+	}
+}
+
 func trustManagerBeforeAll(ctx context.Context, clientset **kubernetes.Clientset, unsupportedAddonFeatures, operatorLogLevel *string) func() {
 	return func() {
 		cs, err := kubernetes.NewForConfig(cfg)