From 52fd4a22e49e9bb0a3947262cce75931b63b209b Mon Sep 17 00:00:00 2001 From: Jayendra Parsai Date: Tue, 1 Oct 2024 13:30:09 +0530 Subject: [PATCH] fix: Cluster-scoped Rollouts should be restricted to user-defined namespace Signed-off-by: Jayendra Parsai --- api/v1alpha1/argorollouts_types.go | 1 + controllers/argorollouts_controller_test.go | 12 +++++ controllers/default.go | 3 ++ controllers/reconcile.go | 6 +++ controllers/resources_test.go | 18 ++++++++ controllers/utils.go | 46 +++++++++++++++++-- controllers/utils_test.go | 29 +++++++++++- docs/usage/getting_started.md | 46 +++++++++++++++++++ hack/run-upstream-argo-rollouts-e2e-tests.sh | 4 ++ hack/start-rollouts-manager-for-e2e-tests.sh | 3 ++ .../cluster_scoped_rollouts_test.go | 25 ++++++++++ 11 files changed, 188 insertions(+), 5 deletions(-) diff --git a/api/v1alpha1/argorollouts_types.go b/api/v1alpha1/argorollouts_types.go index a08d32a..47d0ad5 100644 --- 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 index c9b3e9f..50024c5 100644 --- 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 index ad0b274..eebd583 100644 --- 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 index 77359dc..c57d28d 100644 --- 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 index 7dbd402..df30a4e 100644 --- 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" @@ -809,6 +810,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 @@ -875,6 +882,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{ @@ -919,6 +929,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 index 7b9a460..4a08a8a 100644 --- 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 }, errors.New(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 index b0ffd07..225906f 100644 --- 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 index 97d6a3a..d243e23 100644 --- 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: + (...) +``` + +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 index 98ff305..380b71d 100755 --- a/hack/run-upstream-argo-rollouts-e2e-tests.sh +++ b/hack/run-upstream-argo-rollouts-e2e-tests.sh @@ -52,6 +52,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 index 8b2a89a..2b0202c 100755 --- 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 index 027ca5f..d23613f 100644 --- a/tests/e2e/cluster-scoped/cluster_scoped_rollouts_test.go +++ b/tests/e2e/cluster-scoped/cluster_scoped_rollouts_test.go @@ -283,6 +283,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)