diff --git a/controllers/gitopsservice_controller.go b/controllers/gitopsservice_controller.go index d37b887aec1..862e6cda6ca 100644 --- a/controllers/gitopsservice_controller.go +++ b/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{ diff --git a/controllers/gitopsservice_controller_test.go b/controllers/gitopsservice_controller_test.go index 160b96c82ae..6b0f8f1c475 100644 --- a/controllers/gitopsservice_controller_test.go +++ b/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) } diff --git a/docs/Migration_Guide.md b/docs/Migration_Guide.md index 264df7dfb6b..9dd6f757b42 100644 --- a/docs/Migration_Guide.md +++ b/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 diff --git a/docs/OpenShift GitOps Usage Guide.md b/docs/OpenShift GitOps Usage Guide.md index 5afe5ae3ba2..52ff304441d 100644 --- a/docs/OpenShift GitOps Usage Guide.md +++ b/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