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

Signed-off-by: Rizwana777 <rizwananaaz177@gmail.com>
Assisted-by: Claude

Author: Rizwana777

controllers/gitopsservice_controller.go

@@ -248,86 +248,114 @@ 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 {
+	gitopsserviceNamespacedName := types.NamespacedName{
+		Name:      serviceName,
+		Namespace: namespace,
+	}
+
+	if !r.DisableDefaultInstall {
+		// 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 {
-			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
+			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,
-	}
 
-	r.cleanKAMResources(ctx, reqLogger)
+		r.cleanKAMResources(ctx, reqLogger)
 
-	if !r.DisableDefaultInstall {
-		// Create/reconcile the default Argo CD instance, unless default install is disabled
+		// 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)
 		}
-	} 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)
+
+		// Reconcile backend service
+		if result, err := r.reconcileBackend(gitopsserviceNamespacedName, instance, reqLogger); err != nil {
+			return result, err
 		}
-	}
 
-	if result, err := r.reconcileBackend(gitopsserviceNamespacedName, instance, reqLogger); err != nil {
-		return result, err
-	}
+		// Reconcile console plugin (only when default install is enabled)
+		dynamicPluginStartOCPVersion := os.Getenv(dynamicPluginStartOCPVersionEnv)
+		if dynamicPluginStartOCPVersion == "" {
+			dynamicPluginStartOCPVersion = common.DefaultDynamicPluginStartOCPVersion
+		}
 
-	dynamicPluginStartOCPVersion := os.Getenv(dynamicPluginStartOCPVersionEnv)
-	if dynamicPluginStartOCPVersion == "" {
-		dynamicPluginStartOCPVersion = common.DefaultDynamicPluginStartOCPVersion
-	}
+		OCPVersion, err := util.GetClusterVersion(r.Client)
+		if err != nil {
+			log.Printf("Unable to get cluster version: %v", err)
+			return reconcile.Result{}, nil
+		}
 
-	OCPVersion, err := util.GetClusterVersion(r.Client)
-	if err != nil {
-		log.Printf("Unable to get cluster version: %v", err)
-		return reconcile.Result{}, nil
-	}
+		v1, err := version.NewVersion(OCPVersion)
+		if err != nil {
+			log.Printf("Unable to retrieve current OCP version: %v", err)
+			return reconcile.Result{}, nil
+		}
+		realVersion := v1.Segments()
+		if len(realVersion) < 2 {
+			log.Printf("OCP version %q has fewer than 2 segments, skipping plugin reconciliation", OCPVersion)
+			return reconcile.Result{}, nil
+		}
+		realMajorVersion := realVersion[0]
+		realMinorVersion := realVersion[1]
 
-	v1, err := version.NewVersion(OCPVersion)
-	if err != nil {
-		log.Printf("Unable to retrieve current OCP version: %v", err)
-		return reconcile.Result{}, nil
-	}
-	realVersion := v1.Segments()
-	realMajorVersion := realVersion[0]
-	realMinorVersion := realVersion[1]
+		v2, err := version.NewVersion(dynamicPluginStartOCPVersion)
+		if err != nil {
+			return reconcile.Result{}, nil
+		}
+		startVersion := v2.Segments()
+		if len(startVersion) < 2 {
+			log.Printf("DYNAMIC_PLUGIN_START_OCP_VERSION %q has fewer than 2 segments, skipping plugin reconciliation", dynamicPluginStartOCPVersion)
+			return reconcile.Result{}, nil
+		}
+		startMajorVersion := startVersion[0]
+		startMinorVersion := startVersion[1]
 
-	v2, err := version.NewVersion(dynamicPluginStartOCPVersion)
-	if err != nil {
+		if realMajorVersion >= startMajorVersion && (realMajorVersion > startMajorVersion || realMinorVersion >= startMinorVersion) {
+			// Reconcile plugin only if OCP version supports it
+			return r.reconcilePlugin(instance, request)
+		}
 		return reconcile.Result{}, nil
-	}
-	startVersion := v2.Segments()
-	startMajorVersion := startVersion[0]
-	startMinorVersion := startVersion[1]
+	} 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 realMajorVersion < startMajorVersion || (realMajorVersion == startMajorVersion && realMinorVersion < startMinorVersion) {
-		// Skip plugin reconciliation if real OCP version is less than dynamic plugin start OCP version
+		// When default install is disabled, only reconcile backend if namespace exists and is not being deleted
+		namespaceRef := newRestrictedNamespace(namespace)
+		err := r.Client.Get(ctx, types.NamespacedName{Name: namespace}, namespaceRef)
+		if err == nil {
+			// Check if namespace is being deleted
+			if namespaceRef.DeletionTimestamp != nil {
+				// Namespace is being deleted, skip backend reconciliation
+				return reconcile.Result{}, nil
+			}
+			// Namespace exists and is not terminating, reconcile backend
+			if result, err := r.reconcileBackend(gitopsserviceNamespacedName, instance, reqLogger); err != nil {
+				return result, err
+			}
+		} else if !errors.IsNotFound(err) {
+			return reconcile.Result{}, err
+		}
+		// If namespace doesn't exist, skip backend reconciliation and plugin reconciliation
 		return reconcile.Result{}, nil
-	} else {
-		return r.reconcilePlugin(instance, request)
 	}
 }
 
@@ -404,6 +432,13 @@ func (r *ReconcileGitopsService) ensureDefaultArgoCDInstanceDoesntExist() error
 		return err
 	}
 
+	// Also delete the namespace when DISABLE_DEFAULT_ARGOCD_INSTANCE is true
+	if err := r.Client.Delete(context.TODO(), argocdNS); err != nil {
+		if !errors.IsNotFound(err) {
+			return fmt.Errorf("failed to delete openshift-gitops namespace: %w", err)
+		}
+	}
+
 	return nil
 }
 

controllers/gitopsservice_controller_test.go

@@ -154,21 +154,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)
+	}
 
 }
 
@@ -208,14 +212,11 @@ func TestReconcileDisableDefault_DeleteIfAlreadyExists(t *testing.T) {
 		t.Fatalf("ArgoCD instance should not exist in namespace, error: %v", err)
 	}
 
-	// openshift-gitops namespace SHOULD still exist
+	// openshift-gitops namespace SHOULD be deleted when DISABLE_DEFAULT_ARGOCD_INSTANCE is enabled
 	err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: serviceNamespace}, &corev1.Namespace{})
-	assertNoError(t, err)
-
-	// backend Deployment SHOULD still exist
-	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("Namespace should be deleted when DISABLE_DEFAULT_ARGOCD_INSTANCE is enabled, error: %v", 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 and the `openshift-gitops` namespace can be deleted by adding an environmental variable `DISABLE_DEFAULT_ARGOCD_INSTANCE` with the value `true` in the Subscription resource. This will completely remove the default installation.
 
 To disable the default instance, edit the Subscription and add the following:
 
@@ -65,7 +65,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, they will be deleted.
 
 `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

@@ -121,7 +121,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 instance **and the `openshift-gitops` namespace** to be deleted. This completely removes the default installation. Argo CD instances in other namespaces should not be affected.
 
    On OpenShift Console, go to 
 
@@ -222,7 +222,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 the namespace already exists, it will be deleted along with the ArgoCD instance.</td>
   </tr>
   <tr>
     <td>SERVER_CLUSTER_ROLE</td>

hack/non-olm-install/README.md

@@ -81,7 +81,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, they will be deleted. |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() {

test/openshift/e2e/ginkgo/sequential/1-018_validate_disable_default_instance_test.go

@@ -29,6 +29,7 @@ import (
 	statefulsetFixture "github.com/redhat-developer/gitops-operator/test/openshift/e2e/ginkgo/fixture/statefulset"
 	"github.com/redhat-developer/gitops-operator/test/openshift/e2e/ginkgo/fixture/utils"
 	appsv1 "k8s.io/api/apps/v1"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
@@ -145,9 +146,21 @@ var _ = Describe("GitOps Operator Sequential E2E Tests", func() {
 			}
 			Eventually(gitopsServerDepl).Should(k8sFixture.NotExistByName())
 
+			By("verifying openshift-gitops namespace no longer exists")
+			openshiftGitopsNS := &corev1.Namespace{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "openshift-gitops",
+				},
+			}
+			Eventually(openshiftGitopsNS).Should(k8sFixture.NotExistByName())
+
 			By("remove the DISABLE_DEFAULT_ARGOCD_INSTANCE env var we set above")
 			fixture.RestoreSubcriptionToDefault()
 
+			By("verifying openshift-gitops namespace is recreated")
+			Eventually(openshiftGitopsNS, "3m", "5s").Should(k8sFixture.ExistByName())
+
+			By("verifying ArgoCD CR is recreated")
 			Eventually(openshiftGitopsArgoCD, "3m", "5s").Should(k8sFixture.ExistByName())
 			Eventually(openshiftGitopsArgoCD, "5m", "5s").Should(argocdFixture.BeAvailable())