fix: Cluster-scoped Rollouts should be restricted to user-defined namespace

Jayendra Parsai · jgwest · commit aea86d7ae590 · 2024-10-04T05:47:12.000-04:00
Signed-off-by: Jayendra Parsai &lt;jparsai@jparsai-thinkpadp1gen4i.remote.csb&gt;
diff --git a/api/v1alpha1/argorollouts_types.go b/api/v1alpha1/argorollouts_types.go
@@ -99,6 +99,7 @@ const (
 	RolloutManagerReasonErrorOccurred                       = "ErrorOccurred"
 	RolloutManagerReasonMultipleClusterScopedRolloutManager = "MultipleClusterScopedRolloutManager"
 	RolloutManagerReasonInvalidScoped                       = "InvalidRolloutManagerScope"
+	RolloutManagerReasonInvalidNamespace                    = "InvalidRolloutManagerNamespace"
 )
 
 type ResourceMetadata struct {
diff --git a/controllers/argorollouts_controller_test.go b/controllers/argorollouts_controller_test.go
@@ -3,6 +3,7 @@ package rollouts
 import (
 	"context"
 	"fmt"
+	"os"
 
 	rolloutsmanagerv1alpha1 "github.com/argoproj-labs/argo-rollouts-manager/api/v1alpha1"
 	"github.com/argoproj-labs/argo-rollouts-manager/tests/e2e/fixture/k8s"
@@ -28,6 +29,14 @@ var _ = Describe("RolloutManagerReconciler tests", func() {
 	BeforeEach(func() {
 		ctx = context.Background()
 		rm = makeTestRolloutManager()
+
+		By("Set Env variable.")
+		os.Setenv(ClusterScopedArgoRolloutsNamespaces, rm.Namespace)
+	})
+
+	AfterEach(func() {
+		By("Unset Env variable.")
+		os.Unsetenv(ClusterScopedArgoRolloutsNamespaces)
 	})
 
 	When("NAMESPACE_SCOPED_ARGO_ROLLOUTS environment variable is set to False.", func() {
@@ -201,6 +210,9 @@ var _ = Describe("RolloutManagerReconciler tests", func() {
 			rm2.Name = "test-rm"
 			rm2.Namespace = "test-ns"
 
+			By("Update Env variable.")
+			os.Setenv(ClusterScopedArgoRolloutsNamespaces, rm.Namespace+","+rm2.Namespace)
+
 			Expect(createNamespace(r, rm2.Namespace)).To(Succeed())
 			Expect(r.Client.Create(ctx, rm2)).ToNot(HaveOccurred())
 
diff --git a/controllers/default.go b/controllers/default.go
@@ -35,4 +35,7 @@ const (
 	// NamespaceScopedArgoRolloutsController is an environment variable that can be used to configure scope of Argo Rollouts controller
 	// Set true to allow only namespace-scoped Argo Rollouts controller deployment and false for cluster-scoped
 	NamespaceScopedArgoRolloutsController = "NAMESPACE_SCOPED_ARGO_ROLLOUTS"
+
+	// ClusterScopedArgoRolloutsNamespaces is an environment variable that can be used to configure namespaces that are allowed to host cluster-scoped Argo Rollouts
+	ClusterScopedArgoRolloutsNamespaces = "CLUSTER_SCOPED_ARGO_ROLLOUTS_NAMESPACES"
 )
diff --git a/controllers/reconcile.go b/controllers/reconcile.go
@@ -29,6 +29,12 @@ func (r *RolloutManagerReconciler) reconcileRolloutsManager(ctx context.Context,
 			rr.condition = createCondition(err.Error(), rolloutsmanagerv1alpha1.RolloutManagerReasonInvalidScoped)
 			return *rr, nil
 		}
+
+		if invalidRolloutNamespace(err) {
+			rr.condition = createCondition(err.Error(), rolloutsmanagerv1alpha1.RolloutManagerReasonInvalidNamespace)
+			return *rr, nil
+		}
+
 		log.Error(err, "failed to validate RolloutManager's scope.")
 		return wrapCondition(createCondition(err.Error())), err
 	}
diff --git a/controllers/resources_test.go b/controllers/resources_test.go
@@ -3,6 +3,7 @@ package rollouts
 import (
 	"context"
 	"fmt"
+	"os"
 
 	"github.com/argoproj-labs/argo-rollouts-manager/api/v1alpha1"
 	monitoringv1 "github.com/coreos/prometheus-operator/pkg/apis/monitoring/v1"
@@ -780,6 +781,12 @@ var _ = Describe("Resource creation and cleanup tests", func() {
 	})
 
 	Context("Resource Cleanup test", func() {
+
+		AfterEach(func() {
+			By("Unset Env variable.")
+			os.Unsetenv(ClusterScopedArgoRolloutsNamespaces)
+		})
+
 		a := makeTestRolloutManager()
 		tt := []struct {
 			name     string
@@ -846,6 +853,9 @@ var _ = Describe("Resource creation and cleanup tests", func() {
 			When(test.name, func() {
 				It("Cleans up all resources created for RolloutManager", func() {
 
+					By("Set Env variable.")
+					os.Setenv(ClusterScopedArgoRolloutsNamespaces, a.Namespace)
+
 					ctx := context.Background()
 					req := reconcile.Request{
 						NamespacedName: types.NamespacedName{
@@ -890,6 +900,14 @@ var _ = Describe("Resource creation and cleanup tests", func() {
 					Namespace: a.Namespace,
 				},
 			}
+
+			By("Set Env variable.")
+			os.Setenv(ClusterScopedArgoRolloutsNamespaces, a.Namespace)
+		})
+
+		AfterEach(func() {
+			By("Unset Env variable.")
+			os.Unsetenv(ClusterScopedArgoRolloutsNamespaces)
 		})
 
 		It("Verify whether RolloutManager creating ServiceMonitor", func() {
diff --git a/controllers/utils.go b/controllers/utils.go
@@ -17,9 +17,10 @@ import (
 )
 
 const (
-	UnsupportedRolloutManagerConfiguration   = "when there exists a cluster-scoped RolloutManager on the cluster, there may not exist another: only a single cluster-scoped RolloutManager is supported"
-	UnsupportedRolloutManagerClusterScoped   = "when Subscription has environment variable NAMESPACE_SCOPED_ARGO_ROLLOUTS set to True, there may not exist any cluster-scoped RolloutManagers: in this case, only namespace-scoped RolloutManager resources are supported"
-	UnsupportedRolloutManagerNamespaceScoped = "when Subscription has environment variable NAMESPACE_SCOPED_ARGO_ROLLOUTS set to False, there may not exist any namespace-scoped RolloutManagers: only a single cluster-scoped RolloutManager is supported"
+	UnsupportedRolloutManagerConfiguration          = "when there exists a cluster-scoped RolloutManager on the cluster, there may not exist another: only a single cluster-scoped RolloutManager is supported"
+	UnsupportedRolloutManagerClusterScoped          = "when Subscription has environment variable NAMESPACE_SCOPED_ARGO_ROLLOUTS set to True, there may not exist any cluster-scoped RolloutManagers: in this case, only namespace-scoped RolloutManager resources are supported"
+	UnsupportedRolloutManagerNamespaceScoped        = "when Subscription has environment variable NAMESPACE_SCOPED_ARGO_ROLLOUTS set to False, there may not exist any namespace-scoped RolloutManagers: only a single cluster-scoped RolloutManager is supported"
+	UnsupportedRolloutManagerClusterScopedNamespace = "Namespace is not specified in CLUSTER_SCOPED_ARGO_ROLLOUTS_NAMESPACES environment variable of Subscription resource. If you wish to install a cluster-scoped Argo Rollouts instance outside the default namespace, ensure it is defined in CLUSTER_SCOPED_ARGO_ROLLOUTS_NAMESPACES"
 )
 
 // pluginItem is a clone of PluginItem from "github.com/argoproj/argo-rollouts/utils/plugin/types"
@@ -235,11 +236,44 @@ func validateRolloutsScope(cr rolloutsmanagerv1alpha1.RolloutManager, namespaceS
 			}, fmt.Errorf(UnsupportedRolloutManagerNamespaceScoped)
 		}
 
-		// allow only cluster-scoped RolloutManager
+		// if cluster-scoped RolloutManager being reconciled, is not specified in CLUSTER_SCOPED_ARGO_ROLLOUTS_NAMESPACES environment variable of Subscription resource,
+		// then don't allow it.
+		if !allowedClusterScopedNamespace(cr) {
+
+			phaseFailure := rolloutsmanagerv1alpha1.PhaseFailure
+
+			return &reconcileStatusResult{
+				rolloutController: &phaseFailure,
+				phase:             &phaseFailure,
+			}, errors.New(UnsupportedRolloutManagerClusterScopedNamespace)
+		}
+
+		// allow cluster-scoped Rollouts for namespaces specified in CLUSTER_SCOPED_ARGO_ROLLOUTS_NAMESPACES environment variable of Subscription resource
 		return nil, nil
 	}
 }
 
+// allowedClusterScopedNamespace will check that current namespace is allowed to host cluster-scoped Argo Rollouts.
+func allowedClusterScopedNamespace(cr rolloutsmanagerv1alpha1.RolloutManager) bool {
+	clusterConfigNamespaces := splitList(os.Getenv(ClusterScopedArgoRolloutsNamespaces))
+	if len(clusterConfigNamespaces) > 0 {
+		for _, n := range clusterConfigNamespaces {
+			if n == cr.Namespace {
+				return true
+			}
+		}
+	}
+	return false
+}
+
+func splitList(s string) []string {
+	elems := strings.Split(s, ",")
+	for i := range elems {
+		elems[i] = strings.TrimSpace(elems[i])
+	}
+	return elems
+}
+
 // checkForExistingRolloutManager will return error if more than one cluster-scoped RolloutManagers are created.
 // because only one cluster-scoped or all namespace-scoped RolloutManagers are supported.
 func checkForExistingRolloutManager(ctx context.Context, k8sClient client.Client, cr rolloutsmanagerv1alpha1.RolloutManager) (*reconcileStatusResult, error) {
@@ -290,6 +324,10 @@ func invalidRolloutScope(err error) bool {
 		err.Error() == UnsupportedRolloutManagerNamespaceScoped
 }
 
+func invalidRolloutNamespace(err error) bool {
+	return err.Error() == UnsupportedRolloutManagerClusterScopedNamespace
+}
+
 // updateStatusConditionOfRolloutManager calls Set Condition of RolloutManager status
 func updateStatusConditionOfRolloutManager(ctx context.Context, rr reconcileStatusResult, rm *rolloutsmanagerv1alpha1.RolloutManager, k8sClient client.Client, log logr.Logger) error {
 
diff --git a/controllers/utils_test.go b/controllers/utils_test.go
@@ -2,6 +2,7 @@ package rollouts
 
 import (
 	"context"
+	"os"
 
 	rolloutsmanagerv1alpha1 "github.com/argoproj-labs/argo-rollouts-manager/api/v1alpha1"
 	monitoringv1 "github.com/coreos/prometheus-operator/pkg/apis/monitoring/v1"
@@ -360,9 +361,19 @@ var _ = Describe("validateRolloutsScope tests", func() {
 
 	When("NAMESPACE_SCOPED_ARGO_ROLLOUTS environment variable is set to False.", func() {
 
+		BeforeEach(func() {
+			By("Set Env variable.")
+			os.Setenv(ClusterScopedArgoRolloutsNamespaces, rolloutsManager.Namespace)
+		})
+
+		AfterEach(func() {
+			By("Unset Env variable.")
+			os.Unsetenv(ClusterScopedArgoRolloutsNamespaces)
+		})
+
 		namespaceScopedArgoRolloutsController := false
 
-		It("should not return error, if cluster-scoped RolloutManager is created.", func() {
+		It("should not return error, if cluster-scoped RolloutManager is created in a namespace specified in env variable.", func() {
 
 			By("Create cluster-scoped RolloutManager.")
 			Expect(k8sClient.Create(ctx, &rolloutsManager)).To(Succeed())
@@ -373,6 +384,22 @@ var _ = Describe("validateRolloutsScope tests", func() {
 			Expect(rr).To(BeNil())
 		})
 
+		It("should return error, if cluster-scoped RolloutManager is created in a namespace which is not specified in env variable.", func() {
+
+			By("Unset Env variable.")
+			os.Unsetenv(ClusterScopedArgoRolloutsNamespaces)
+
+			By("Create cluster-scoped RolloutManager.")
+			Expect(k8sClient.Create(ctx, &rolloutsManager)).To(Succeed())
+
+			By("Verify there is no error returned.")
+			rr, err := validateRolloutsScope(rolloutsManager, namespaceScopedArgoRolloutsController)
+			Expect(err).To(HaveOccurred())
+			Expect(invalidRolloutNamespace(err)).To(BeTrue())
+			Expect(*rr.phase).To(Equal(rolloutsmanagerv1alpha1.PhaseFailure))
+			Expect(*rr.rolloutController).To(Equal(rolloutsmanagerv1alpha1.PhaseFailure))
+		})
+
 		It("should return error, if namespace-scoped RolloutManager is created.", func() {
 
 			By("Create namespace-scoped RolloutManager.")
diff --git a/docs/usage/getting_started.md b/docs/usage/getting_started.md
@@ -29,3 +29,49 @@ kubectl get all
 
 If you would like to understand the siginificance of each rollout controller resource created by the operator, please go through the official rollouts controller [docs](https://argo-rollouts.readthedocs.io/en/stable/).
 
+
+
+
+## Namespace Scoped Rollouts Instance
+
+A namespace-scoped Rollouts instance can manage Rollouts resources of same namespace it is deployed into. To deploy a namespace-scoped Rollouts instance set `spec.namespaceScoped` field to `true`.
+
+```yml
+apiVersion: argoproj.io/v1alpha1
+kind: RolloutManager
+metadata:
+  name: argo-rollout
+spec:
+  namespaceScoped: true
+```
+
+
+## Cluster Scoped Rollouts Instance
+
+A cluster-scoped Rollouts instance can manage Rollouts resources from other namespaces as well. To install a cluster-scoped Rollouts instance first you need to add `NAMESPACE_SCOPED_ARGO_ROLLOUTS` and `CLUSTER_SCOPED_ARGO_ROLLOUTS_NAMESPACES` environment variables in subscription resource. If `NAMESPACE_SCOPED_ARGO_ROLLOUTS` is set to `false` then only you are allowed to create a cluster-scoped instance and then you need to provide list of namespaces that are allowed host a cluster-scoped Rollouts instance via `CLUSTER_SCOPED_ARGO_ROLLOUTS_NAMESPACES` environment variable.  
+
+```yml
+apiVersion: operators.coreos.com/v1alpha1
+kind: Subscription
+metadata:
+  name: argo-operator
+spec:
+  config:
+   env: 
+    - name: NAMESPACE_SCOPED_ARGO_ROLLOUTS
+      value: 'false'   
+    - name: CLUSTER_SCOPED_ARGO_ROLLOUTS_NAMESPACES
+      value: <list of namespaces of cluster-scoped Argo CD instances>
+  (...)
+```
+
+Now set `spec.namespaceScoped` field to `false` to create a Rollouts instance.
+
+```yml
+apiVersion: argoproj.io/v1alpha1
+kind: RolloutManager
+metadata:
+  name: argo-rollout
+spec:
+  namespaceScoped: false
+```
diff --git a/hack/run-upstream-argo-rollouts-e2e-tests.sh b/hack/run-upstream-argo-rollouts-e2e-tests.sh
@@ -47,6 +47,10 @@ if [ -z "$SKIP_RUN_STEP" ]; then
   set +e
 
   rm -f /tmp/e2e-operator-run.log || true
+  
+  # Set namespaces used for cluster-scoped e2e tests
+  export CLUSTER_SCOPED_ARGO_ROLLOUTS_NAMESPACES="argo-rollouts"
+  
   go run ./cmd/main.go 2>&1 | tee /tmp/e2e-operator-run.log &
 
   set -e
diff --git a/hack/start-rollouts-manager-for-e2e-tests.sh b/hack/start-rollouts-manager-for-e2e-tests.sh
@@ -18,6 +18,9 @@ set -ex
 
 make install generate fmt vet
 
+# Set namespaces used for cluster-scoped e2e tests
+export CLUSTER_SCOPED_ARGO_ROLLOUTS_NAMESPACES="argo-rollouts,test-rom-ns-1,rom-ns-1"
+
 if [ "$RUN_IN_BACKGROUND" == "true" ]; then
   go run ./cmd/main.go 2>&1 | tee /tmp/e2e-operator-run.log &
 else
diff --git a/tests/e2e/cluster-scoped/cluster_scoped_rollouts_test.go b/tests/e2e/cluster-scoped/cluster_scoped_rollouts_test.go
@@ -282,6 +282,31 @@ var _ = Describe("Cluster-scoped RolloutManager tests", func() {
 			utils.ValidateArgoRolloutsResources(ctx, k8sClient, nsName2, testServiceNodePort_31002, testServiceNodePort_32002)
 		})
 
+		/*
+			In this test a cluster-scoped RolloutManager is created in one namespace, but namespace is not specified in CLUSTER_SCOPED_ARGO_ROLLOUTS_NAMESPACES env variable, hence operator should not allow to create Rollouts instance.
+		*/
+		It("Namespace is not specified in CLUSTER_SCOPED_ARGO_ROLLOUTS_NAMESPACES, then cluster-scoped RolloutManager should not be allowed.", func() {
+
+			nsName := "test-ns"
+			By("Create namespace.")
+			Expect(utils.CreateNamespace(ctx, k8sClient, nsName)).To(Succeed())
+
+			rolloutsManager, err := utils.CreateRolloutManager(ctx, k8sClient, "test-rollouts-manager-1", nsName, false)
+			Expect(err).ToNot(HaveOccurred())
+
+			By("Verify that RolloutManager is successfully created.")
+			Eventually(rolloutsManager, "1m", "5s").Should(rmFixture.HavePhase(rmv1alpha1.PhaseFailure))
+
+			By("Verify that Status.Condition is now having error message.")
+			Eventually(rolloutsManager, "1m", "1s").Should(rmFixture.HaveCondition(
+				metav1.Condition{
+					Type:    rmv1alpha1.RolloutManagerConditionType,
+					Status:  metav1.ConditionFalse,
+					Reason:  rmv1alpha1.RolloutManagerReasonInvalidNamespace,
+					Message: controllers.UnsupportedRolloutManagerClusterScopedNamespace,
+				}))
+		})
+
 		It("After creating 2 cluster-scoped RolloutManager in a namespace, delete 1st RolloutManager and verify it removes the Failed status of 2nd RolloutManager", func() {
 			By("1st RM: Create cluster-scoped RolloutManager in a namespace.")
 			rolloutsManagerCl, err := utils.CreateRolloutManager(ctx, k8sClient, "test-rollouts-manager-1", fixture.TestE2ENamespace, false)

Original file line number	Diff line number	Diff line change
`@@ -99,6 +99,7 @@ const (`
`99`	`99`	`RolloutManagerReasonErrorOccurred = "ErrorOccurred"`
`100`	`100`	`RolloutManagerReasonMultipleClusterScopedRolloutManager = "MultipleClusterScopedRolloutManager"`
`101`	`101`	`RolloutManagerReasonInvalidScoped = "InvalidRolloutManagerScope"`
	`102`	`+ RolloutManagerReasonInvalidNamespace = "InvalidRolloutManagerNamespace"`
`102`	`103`	`)`
`103`	`104`
`104`	`105`	`type ResourceMetadata struct {`
Original file line number	Diff line number	Diff line change
`@@ -35,4 +35,7 @@ const (`
`35`	`35`	`// NamespaceScopedArgoRolloutsController is an environment variable that can be used to configure scope of Argo Rollouts controller`
`36`	`36`	`// Set true to allow only namespace-scoped Argo Rollouts controller deployment and false for cluster-scoped`
`37`	`37`	`NamespaceScopedArgoRolloutsController = "NAMESPACE_SCOPED_ARGO_ROLLOUTS"`
	`38`	`+`
	`39`	`+ // ClusterScopedArgoRolloutsNamespaces is an environment variable that can be used to configure namespaces that are allowed to host cluster-scoped Argo Rollouts`
	`40`	`+ ClusterScopedArgoRolloutsNamespaces = "CLUSTER_SCOPED_ARGO_ROLLOUTS_NAMESPACES"`
`38`	`41`	`)`
Original file line number	Diff line number	Diff line change
`@@ -29,6 +29,12 @@ func (r *RolloutManagerReconciler) reconcileRolloutsManager(ctx context.Context,`
`29`	`29`	`rr.condition = createCondition(err.Error(), rolloutsmanagerv1alpha1.RolloutManagerReasonInvalidScoped)`
`30`	`30`	`return *rr, nil`
`31`	`31`	`}`
	`32`	`+`
	`33`	`+ if invalidRolloutNamespace(err) {`
	`34`	`+ rr.condition = createCondition(err.Error(), rolloutsmanagerv1alpha1.RolloutManagerReasonInvalidNamespace)`
	`35`	`+ return *rr, nil`
	`36`	`+ }`
	`37`	`+`
`32`	`38`	`log.Error(err, "failed to validate RolloutManager's scope.")`
`33`	`39`	`return wrapCondition(createCondition(err.Error())), err`
`34`	`40`	`}`