feat: prevent openshift-gitops namespace creation when DISABLE_DEFAULT_ARGOCD_INSTANCE=true

controllers/gitopsservice_controller.go

@@ -258,29 +258,6 @@ func (r *ReconcileGitopsService) Reconcile(ctx context.Context, request reconcil
 		return reconcile.Result{}, err
 	}
 
-	// Create namespace if it doesn't already exist
-	namespaceRef := newRestrictedNamespace(namespace)
-	err = r.Client.Get(ctx, types.NamespacedName{Name: namespace}, namespaceRef)
-	if err != nil {
-		if errors.IsNotFound(err) {
-			reqLogger.Info("Creating a new Namespace", "Name", namespace)
-			ensureInfraNodeSelectorAnnotation(namespaceRef, instance.Spec.RunOnInfra)
-			err = r.Client.Create(ctx, namespaceRef)
-			if err != nil {
-				return reconcile.Result{}, err
-			}
-		} else {
-			return reconcile.Result{}, err
-		}
-	} else {
-		if ensureNamespaceMetadata(namespaceRef, instance.Spec.RunOnInfra) {
-			err = r.Client.Update(context.TODO(), namespaceRef)
-			if err != nil {
-				return reconcile.Result{}, err
-			}
-		}
-	}
-
 	gitopsserviceNamespacedName := types.NamespacedName{
 		Name:      serviceName,
 		Namespace: namespace,
@@ -293,22 +270,54 @@ func (r *ReconcileGitopsService) Reconcile(ctx context.Context, request reconcil
 	}
 
 	if !r.DisableDefaultInstall {
-		// Create/reconcile the default Argo CD instance, unless default install is disabled
+		// Create namespace if it doesn't already exist (only when default install is enabled)
+		namespaceRef := newRestrictedNamespace(namespace)
+		err = r.Client.Get(ctx, types.NamespacedName{Name: namespace}, namespaceRef)
+		if err != nil {
+			if errors.IsNotFound(err) {
+				reqLogger.Info("Creating a new Namespace", "Name", namespace)
+				ensureInfraNodeSelectorAnnotation(namespaceRef, instance.Spec.RunOnInfra)
+				err = r.Client.Create(ctx, namespaceRef)
+				if err != nil {
+					return reconcile.Result{}, err
+				}
+			} else {
+				return reconcile.Result{}, err
+			}
+		} else {
+			if ensureNamespaceMetadata(namespaceRef, instance.Spec.RunOnInfra) {
+				err = r.Client.Update(context.TODO(), namespaceRef)
+				if err != nil {
+					return reconcile.Result{}, err
+				}
+			}
+		}
+
+		// Create/reconcile the default Argo CD instance
 		if result, err := r.reconcileDefaultArgoCDInstance(instance, reqLogger); err != nil {
 			return result, fmt.Errorf("unable to reconcile default Argo CD instance: %v", err)
 		}
+
+		// Reconcile backend service
+		if result, err := r.reconcileBackend(gitopsserviceNamespacedName, instance, reqLogger); err != nil {
+			return result, err
+		}
 	} else {
 		// If installation of default Argo CD instance is disabled, make sure it doesn't exist,
 		// deleting it if necessary
 		if err := r.ensureDefaultArgoCDInstanceDoesntExist(); err != nil {
 			return reconcile.Result{}, fmt.Errorf("unable to ensure non-existence of default Argo CD instance: %v", err)
 		}
-	}
 
-	if result, err := r.reconcileBackend(gitopsserviceNamespacedName, instance, reqLogger); err != nil {
-		return result, err
+		// The backend is part of the default install, so it is not created either. Remove whatever a
+		// previous reconcile created for it.
+		if err := r.cleanupBackendResources(ctx, gitopsserviceNamespacedName, reqLogger); err != nil {
+			return reconcile.Result{}, err
+		}
 	}
 
+	// The console plugin is decoupled from the default Argo CD instance, so it is reconciled into
+	// its own namespace regardless of whether the default install is disabled.
 	if r.PluginNamespace != namespace {
 		pluginNS := &corev1.Namespace{}
 		err = r.Client.Get(ctx, types.NamespacedName{Name: r.PluginNamespace}, pluginNS)
@@ -428,7 +437,9 @@ func (r *ReconcileGitopsService) ensureDefaultArgoCDInstanceDoesntExist() error
 		}
 	}
 
-	// Delete the existing Argo CD instance, if it exists
+	// Delete the existing Argo CD instance, if it exists. The namespace it lives in is deliberately
+	// left in place, because it may hold resources that were created outside of the operator. It is up
+	// to the user to clean up the namespace and anything else they put in it.
 	existingArgoCD := &argoapp.ArgoCD{}
 	err = r.Client.Get(context.TODO(), types.NamespacedName{Name: defaultArgoCDInstance.Name, Namespace: defaultArgoCDInstance.Namespace}, existingArgoCD)
 	if err == nil {
@@ -445,6 +456,44 @@ func (r *ReconcileGitopsService) ensureDefaultArgoCDInstanceDoesntExist() error
 	return nil
 }
 
+// cleanupBackendResources deletes the resources that reconcileBackend creates, and only those. The
+// namespace they live in is deliberately left in place, since it may hold resources that were
+// created outside of the operator, and cleaning those up is left to the user.
+//
+// The ClusterRole and ClusterRoleBinding are cluster-scoped, so they would survive even if the user
+// did delete the namespace, and are otherwise only garbage collected once the GitopsService CR
+// itself is deleted. Leaving the binding behind would re-grant cluster-wide access as soon as a
+// ServiceAccount of the same name in that namespace exists again.
+//
+// Keep in sync with reconcileBackend.
+func (r *ReconcileGitopsService) cleanupBackendResources(ctx context.Context, gitopsserviceNamespacedName types.NamespacedName, reqLogger logr.Logger) error {
+
+	// Deleted in this order so the workload is told to stop before the RBAC it runs with is
+	// revoked.
+	backendResources := []struct {
+		kind   string
+		object client.Object
+	}{
+		{"Deployment", &appsv1.Deployment{ObjectMeta: backendDeploymentObjectMeta(gitopsserviceNamespacedName)}},
+		{"Service", newBackendService(gitopsserviceNamespacedName)},
+		{"ClusterRoleBinding", newClusterRoleBinding(gitopsserviceNamespacedName)},
+		{"ClusterRole", newClusterRole(gitopsserviceNamespacedName)},
+		{"ServiceAccount", newServiceAccount(gitopsserviceNamespacedName)},
+	}
+
+	for _, resource := range backendResources {
+		if err := r.Client.Delete(ctx, resource.object); err != nil {
+			if !errors.IsNotFound(err) {
+				return fmt.Errorf("failed to delete backend %s %q: %w", resource.kind, resource.object.GetName(), err)
+			}
+		} else {
+			reqLogger.Info("Deleted backend "+resource.kind, "Name", resource.object.GetName())
+		}
+	}
+
+	return nil
+}
+
 func (r *ReconcileGitopsService) reconcileDefaultArgoCDInstance(instance *pipelinesv1alpha1.GitopsService, reqLogger logr.Logger) (reconcile.Result, error) {
 
 	defaultArgoCDInstance, err := argocd.NewCR(common.ArgoCDInstanceName, serviceNamespace, r.Client)
@@ -929,13 +978,20 @@ func newBackendDeployment(ns types.NamespacedName, crImagePullPolicy corev1.Pull
 	}
 
 	deploymentObj := &appsv1.Deployment{
-		ObjectMeta: objectMeta(ns.Name, ns.Namespace),
+		ObjectMeta: backendDeploymentObjectMeta(ns),
 		Spec:       deploymentSpec,
 	}
 
 	return deploymentObj
 }
 
+// backendDeploymentObjectMeta identifies the backend Deployment. It is shared by newBackendDeployment
+// and cleanupBackendResources, so that the Deployment is deleted under exactly the name it is created
+// with.
+func backendDeploymentObjectMeta(ns types.NamespacedName) metav1.ObjectMeta {
+	return objectMeta(ns.Name, ns.Namespace)
+}
+
 func newBackendService(ns types.NamespacedName) *corev1.Service {
 
 	spec := corev1.ServiceSpec{

controllers/gitopsservice_controller_test.go

@@ -221,21 +221,25 @@ func TestReconcileDisableDefault(t *testing.T) {
 
 	argoCD := &argoapp.ArgoCD{}
 
-	// ArgoCD instance SHOULD NOT created (in openshift-gitops namespace)
+	// ArgoCD instance SHOULD NOT be created (in openshift-gitops namespace)
 	if err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: common.ArgoCDInstanceName, Namespace: serviceNamespace},
 		argoCD); err == nil || !errors.IsNotFound(err) {
 
 		t.Fatalf("ArgoCD instance should not exist in namespace, error: %v", err)
 	}
 
-	// openshift-gitops namespace SHOULD be created
+	// openshift-gitops namespace SHOULD NOT be created when DISABLE_DEFAULT_ARGOCD_INSTANCE is true
 	err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: serviceNamespace}, &corev1.Namespace{})
-	assertNoError(t, err)
+	if err == nil || !errors.IsNotFound(err) {
+		t.Fatalf("Namespace should not exist when DISABLE_DEFAULT_ARGOCD_INSTANCE is true, error: %v", err)
+	}
 
-	// backend Deployment SHOULD be created
+	// backend Deployment SHOULD NOT be created (no namespace to deploy into)
 	deploy := &appsv1.Deployment{}
 	err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: serviceName, Namespace: serviceNamespace}, deploy)
-	assertNoError(t, err)
+	if err == nil || !errors.IsNotFound(err) {
+		t.Fatalf("Backend deployment should not exist when namespace doesn't exist, error: %v", err)
+	}
 
 }
 
@@ -275,13 +279,36 @@ func TestReconcileDisableDefault_DeleteIfAlreadyExists(t *testing.T) {
 		t.Fatalf("ArgoCD instance should not exist in namespace, error: %v", err)
 	}
 
-	// openshift-gitops namespace SHOULD still exist
-	err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: serviceNamespace}, &corev1.Namespace{})
-	assertNoError(t, err)
+	// The resources the operator created for the backend SHOULD be deleted. The ClusterRole and
+	// ClusterRoleBinding are owned by the GitopsService CR, but that CR is never deleted while the
+	// operator is installed, so garbage collection will not remove them. A surviving binding would
+	// re-grant cluster-wide access as soon as a ServiceAccount of the same name exists again.
+	backendName := types.NamespacedName{Name: serviceName, Namespace: serviceNamespace}
+	prefixedName := types.NamespacedName{Name: gitopsServicePrefix + serviceName, Namespace: serviceNamespace}
+	clusterRoleName := types.NamespacedName{Name: gitopsServicePrefix + serviceName}
+
+	backendResources := []struct {
+		kind   string
+		name   types.NamespacedName
+		object client.Object
+	}{
+		{"Deployment", backendName, &appsv1.Deployment{}},
+		{"Service", backendName, &corev1.Service{}},
+		{"ClusterRoleBinding", clusterRoleName, &rbacv1.ClusterRoleBinding{}},
+		{"ClusterRole", clusterRoleName, &rbacv1.ClusterRole{}},
+		{"ServiceAccount", prefixedName, &corev1.ServiceAccount{}},
+	}
 
-	// backend Deployment SHOULD still exist
-	deploy := &appsv1.Deployment{}
-	err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: serviceName, Namespace: serviceNamespace}, deploy)
+	for _, resource := range backendResources {
+		err = fakeClient.Get(context.TODO(), resource.name, resource.object)
+		if err == nil || !errors.IsNotFound(err) {
+			t.Fatalf("backend %s should be deleted when DISABLE_DEFAULT_ARGOCD_INSTANCE is enabled, error: %v", resource.kind, err)
+		}
+	}
+
+	// The openshift-gitops namespace itself SHOULD still exist, because it may contain resources
+	// that were created outside of the operator. Cleaning it up is left to the user.
+	err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: serviceNamespace}, &corev1.Namespace{})
 	assertNoError(t, err)
 
 }

docs/Migration_Guide.md

@@ -6,7 +6,7 @@ For understanding the differences between [Argo CD Community Operator](https://g
 
 **Note**: Installing GitOps operator will create a namespace with the name `openshift-gitops` and an Argo CD instance in the same namespace. This instance can be used for managing your OpenShift cluster configuration. It is enabled with Dex OpenShift connector by default which allows users to log in with their OpenShift credentials.
 
-The default Argo CD instance in the `openshift-gitops` namespace can be deleted by adding an environmental variable `DISABLE_DEFAULT_ARGOCD_INSTANCE` with the value `true` in the Subscription resource.
+The default Argo CD instance in the `openshift-gitops` namespace can be deleted by adding an environmental variable `DISABLE_DEFAULT_ARGOCD_INSTANCE` with the value `true` in the Subscription resource. With this set, the `openshift-gitops` namespace is not created. If it already exists, the operator deletes the resources it created there, but leaves the namespace itself in place, since it may hold resources you created yourself, cleaning those up and removing the namespace is up to you.
 
 To disable the default instance, edit the Subscription and add the following:
 
@@ -69,7 +69,7 @@ Post migration the above environment variables has to be copied to GitOps operat
 
 **Note**:
 GitOps operator supports the below additional environment variables
-`DISABLE_DEFAULT_ARGOCD_INSTANCE`: Disables the installation of default instance in openshift-gitops namespace.
+`DISABLE_DEFAULT_ARGOCD_INSTANCE`: Disables the installation of default instance in openshift-gitops namespace. This prevents the creation of the `openshift-gitops` namespace and ArgoCD instance. If they already exist, the operator deletes only the resources it created in the namespace, the namespace itself and anything else in it is left for you to clean up.
 
 `ARGOCD_CLUSTER_CONFIG_NAMESPACES`: Argo CD is granted permissions to manage specific cluster-scoped resources which include
 platform operators, optional OLM operators, user management, etc. Argo CD is not granted cluster-admin. You can find the complete

docs/OpenShift GitOps Usage Guide.md

@@ -125,7 +125,7 @@ When installing the OpenShift GitOps operator to ROSA/OSD, cluster administrator
 
 To disable the default ‘ready-to-use’ installation of Argo CD: as an admin, update the existing Subscription Object for Gitops Operator and add `DISABLE_DEFAULT_ARGOCD_INSTANCE = true` to the spec.
 
-**Warning**: setting this option to true will cause the existing Argo CD install in the *openshift-gitops* namespace to be deleted. Argo CD instances in other namespaces should not be affected.
+**Warning**: setting this option to true will cause the existing Argo CD install in the *openshift-gitops* namespace to be deleted. The `openshift-gitops` namespace itself is **not** deleted, and it is not created in the first place if it does not already exist. Only the resources the operator created in that namespace are removed, anything else you put there and the namespace itself is left for you to clean up. Argo CD instances in other namespaces should not be affected.
 
    On OpenShift Console, go to 
 
@@ -226,7 +226,7 @@ Updating the following environment variables in the existing Subscription Object
   <tr>
     <td>DISABLE_DEFAULT_ARGOCD_INSTANCE</td>
     <td>false</td>
-    <td>When set to `true`, will disable the default 'ready-to-use' installation of Argo CD in `openshift-gitops` namespace.</td>
+    <td>When set to `true`, will disable the default 'ready-to-use' installation of Argo CD in `openshift-gitops` namespace. This prevents the creation of the `openshift-gitops` namespace and ArgoCD instance. If they already exist, the operator deletes only the resources it created in the namespace; the namespace itself, and anything else in it, is left for you to clean up. See <a href="#installation-of-openshift-gitops-without-ready-to-use-argo-cd-instance-for-rosaosd">the warning above</a> before enabling this.</td>
   </tr>
   <tr>
     <td>SERVER_CLUSTER_ROLE</td>

hack/non-olm-install/README.md

@@ -82,7 +82,7 @@ The following environment variables can be set to configure various options for
 | ----------- | ----------- |------------- |
 | **ARGOCD_CLUSTER_CONFIG_NAMESPACES** |OpenShift GitOps instances in the identified namespaces are granted limited additional permissions to manage specific cluster-scoped resources, which include platform operators, optional OLM operators, user management, etc.Multiple namespaces can be specified via a comma delimited list. | openshift-gitops |
 | **CONTROLLER_CLUSTER_ROLE** | This environment variable enables administrators to configure a common cluster role to use across all managed namespaces in the role bindings the operator creates for the Argo CD application controller. | None |
-| **DISABLE_DEFAULT_ARGOCD_INSTANCE** | When set to `true`, this will disable the default 'ready-to-use' installation of Argo CD in the `openshift-gitops` namespace. |false |
+| **DISABLE_DEFAULT_ARGOCD_INSTANCE** | When set to `true`, this will disable the default 'ready-to-use' installation of Argo CD in the `openshift-gitops` namespace. This prevents the creation of the `openshift-gitops` namespace and ArgoCD instance. If they already exist, the operator deletes only the resources it created in the namespace, the namespace itself and anything else in it is left for you to clean up. |false |
 | **SERVER_CLUSTER_ROLE** |This environment variable enables administrators to configure a common cluster role to use across all of the managed namespaces in the role bindings the operator creates for the Argo CD server. | None |
 | **WATCH_NAMESPACE** | namespaces in which Argo applications can be created | None |
 | **ENABLE_CONVERSION_WEBHOOK** | This environment variable enables conversion webhook to convert v1alpha1 ArgoCD resources to v1beta1 | true |

test/nondefaulte2e/gitops_service_nondefault_test.go

@@ -45,17 +45,25 @@ var _ = Describe("GitOpsServiceNoDefaultInstall", func() {
 			},
 		}
 
-		It("Backend resources are created in 'openshift-gitops' namespace", func() {
-			resourceList := []helper.ResourceList{
-				{
-					Resource: &appsv1.Deployment{},
-					ExpectedResources: []string{
-						"cluster",
-					},
-				},
-			}
-			err := helper.WaitForResourcesByName(k8sClient, resourceList, existingArgoInstance.Namespace, time.Second*180)
-			Expect(err).NotTo(HaveOccurred())
+		It("openshift-gitops namespace should not be created when DISABLE_DEFAULT_ARGOCD_INSTANCE is true", func() {
+			// When DISABLE_DEFAULT_ARGOCD_INSTANCE is true, the namespace should not exist
+			Consistently(func() bool {
+				ns := &corev1.Namespace{}
+				err := k8sClient.Get(context.Background(),
+					types.NamespacedName{Name: existingArgoInstance.Namespace},
+					ns)
+				// Namespace should not exist
+				return kubeerrors.IsNotFound(err)
+			}, time.Second*30, interval).Should(BeTrue(), "openshift-gitops namespace should not exist when DISABLE_DEFAULT_ARGOCD_INSTANCE is true")
+		})
+
+		It("Backend deployment should not be created when DISABLE_DEFAULT_ARGOCD_INSTANCE is true", func() {
+			// Backend deployment should not exist (namespace doesn't exist)
+			deployment := &appsv1.Deployment{}
+			err := k8sClient.Get(context.Background(),
+				types.NamespacedName{Name: "cluster", Namespace: existingArgoInstance.Namespace},
+				deployment)
+			Expect(kubeerrors.IsNotFound(err)).To(BeTrue(), "Backend deployment 'cluster' should not exist when DISABLE_DEFAULT_ARGOCD_INSTANCE is true")
 		})
 
 		It("Default Argo CD instance should not be found", func() {