fix: delete/don't create deployment resources if using .remote field (#…

…1545) * fix: delete/dont create deployemnt resources if using .remote field Signed-off-by: Anand Kumar Singh <[email protected]> * fix: skip service creation if route is enabled Signed-off-by: Anand Kumar Singh <[email protected]> * update documentation Signed-off-by: Anand Kumar Singh <[email protected]> * add unit-tests Signed-off-by: Anand Kumar Singh <[email protected]> * add IsRemote for redis and repo-server component Signed-off-by: Anand Kumar Singh <[email protected]> * fix docs Signed-off-by: Anand Kumar Singh <[email protected]> * fix test Signed-off-by: Anand Kumar Singh <[email protected]> * rebase Signed-off-by: Anand Kumar Singh <[email protected]> * fix: goimports error Signed-off-by: Anand Kumar Singh <[email protected]> * fix: isRemote enabled should delete Signed-off-by: Anand Kumar Singh <[email protected]> --------- Signed-off-by: Anand Kumar Singh <[email protected]>
argoproj-labs · Oct 3, 2024 · 2ef2bc4 · 2ef2bc4
1 parent 14032c7
commit 2ef2bc4
Show file tree

Hide file tree

Showing 14 changed files with 305 additions and 9 deletions.
diff --git a/api/v1beta1/argocd_types.go b/api/v1beta1/argocd_types.go
@@ -465,6 +465,10 @@ func (a *ArgoCDRedisSpec) IsEnabled() bool {
 	return a.Enabled == nil || (a.Enabled != nil && *a.Enabled)
 }
 
+func (a *ArgoCDRedisSpec) IsRemote() bool {
+	return a.Remote != nil && *a.Remote != ""
+}
+
 // ArgoCDRepoSpec defines the desired state for the Argo CD repo server component.
 type ArgoCDRepoSpec struct {
 
@@ -535,6 +539,10 @@ func (a *ArgoCDRepoSpec) IsEnabled() bool {
 	return a.Enabled == nil || (a.Enabled != nil && *a.Enabled)
 }
 
+func (a *ArgoCDRepoSpec) IsRemote() bool {
+	return a.Remote != nil && *a.Remote != ""
+}
+
 // ArgoCDRouteSpec defines the desired state for an OpenShift Route.
 type ArgoCDRouteSpec struct {
 	// Annotations is the map of annotations to use for the Route resource.

diff --git a/controllers/argocd/deployment.go b/controllers/argocd/deployment.go
@@ -357,7 +357,7 @@ func getDexServerAddress(cr *argoproj.ArgoCD) string {
 
 // getRepoServerAddress will return the Argo CD repo server address.
 func getRepoServerAddress(cr *argoproj.ArgoCD) string {
-	if cr.Spec.Repo.Remote != nil && *cr.Spec.Repo.Remote != "" {
+	if cr.Spec.Repo.IsRemote() {
 		return *cr.Spec.Repo.Remote
 	}
 	return fqdnServiceRef("repo-server", common.ArgoCDDefaultRepoServerPort, cr)
@@ -533,6 +533,9 @@ func (r *ReconcileArgoCD) reconcileRedisDeployment(cr *argoproj.ArgoCD, useTLS b
 			// Deployment exists but component enabled flag has been set to false, delete the Deployment
 			log.Info("Redis exists but should be disabled. Deleting existing redis.")
 			return r.Client.Delete(context.TODO(), deploy)
+		} else if cr.Spec.Redis.IsRemote() {
+			log.Info("Redis remote exists but redis deployment should be disabled. Deleting existing redis.")
+			return r.Client.Delete(context.TODO(), deploy)
 		}
 		if cr.Spec.HA.Enabled {
 			// Deployment exists but HA enabled flag has been set to true, delete the Deployment
@@ -575,7 +578,7 @@ func (r *ReconcileArgoCD) reconcileRedisDeployment(cr *argoproj.ArgoCD, useTLS b
 		return nil // Deployment found with nothing to do, move along...
 	}
 
-	if cr.Spec.Redis.IsEnabled() && cr.Spec.Redis.Remote != nil && *cr.Spec.Redis.Remote != "" {
+	if cr.Spec.Redis.IsEnabled() && cr.Spec.Redis.IsRemote() {
 		log.Info("Custom Redis Endpoint. Skipping starting redis.")
 		return nil
 	}
@@ -1113,6 +1116,9 @@ func (r *ReconcileArgoCD) reconcileRepoDeployment(cr *argoproj.ArgoCD, useTLSFor
 			log.Info("Existing ArgoCD Repo Server found but should be disabled. Deleting Repo Server")
 			// Delete existing deployment for ArgoCD Repo Server, if any ..
 			return r.Client.Delete(context.TODO(), existing)
+		} else if cr.Spec.Repo.IsRemote() {
+			log.Info("Repo Server remote field exists, Repo Server deployment should be disabled. Deleting Repo Server.")
+			return r.Client.Delete(context.TODO(), deploy)
 		}
 
 		changed := false
@@ -1187,6 +1193,11 @@ func (r *ReconcileArgoCD) reconcileRepoDeployment(cr *argoproj.ArgoCD, useTLSFor
 		return nil // Deployment found with nothing to do, move along...
 	}
 
+	if cr.Spec.Redis.IsEnabled() && cr.Spec.Repo.IsRemote() {
+		log.Info("Custom Repo Endpoint. Skipping starting Repo Server.")
+		return nil
+	}
+
 	if !cr.Spec.Repo.IsEnabled() {
 		log.Info("ArgoCD Repo Server disabled. Skipping starting ArgoCD Repo Server.")
 		return nil

diff --git a/controllers/argocd/deployment_test.go b/controllers/argocd/deployment_test.go
@@ -2291,3 +2291,58 @@ func TestReconcileArgoCD_reconcileRepoDeployment_serviceAccount(t *testing.T) {
 		})
 	}
 }
+
+// If `remote` field is used in CR, then the component resources should not be created
+func TestReconcileArgoCD_reconcileRedisWithRemote(t *testing.T) {
+	cr := makeTestArgoCD()
+
+	resObjs := []client.Object{cr}
+	subresObjs := []client.Object{cr}
+	runtimeObjs := []runtime.Object{}
+	sch := makeTestReconcilerScheme(argoproj.AddToScheme)
+	cl := makeTestReconcilerClient(sch, resObjs, subresObjs, runtimeObjs)
+	r := makeTestReconciler(cl, sch)
+
+	redisRemote := "https://remote.redis.instance"
+
+	cr.Spec.Redis.Remote = &redisRemote
+	assert.NoError(t, r.reconcileRedisDeployment(cr, false))
+
+	d := &appsv1.Deployment{}
+
+	assert.ErrorContains(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: cr.Name + "-redis", Namespace: cr.Namespace}, d),
+		"deployments.apps \""+cr.Name+"-redis\" not found")
+
+	// once remote is set to nil, reconciliation should trigger deployment resource creation
+	cr.Spec.Redis.Remote = nil
+
+	assert.NoError(t, r.reconcileRedisDeployment(cr, false))
+	assert.NoError(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: cr.Name + "-redis", Namespace: cr.Namespace}, d))
+}
+
+func TestReconcileArgoCD_reconcileRepoServerWithRemote(t *testing.T) {
+	cr := makeTestArgoCD()
+
+	resObjs := []client.Object{cr}
+	subresObjs := []client.Object{cr}
+	runtimeObjs := []runtime.Object{}
+	sch := makeTestReconcilerScheme(argoproj.AddToScheme)
+	cl := makeTestReconcilerClient(sch, resObjs, subresObjs, runtimeObjs)
+	r := makeTestReconciler(cl, sch)
+
+	repoServerRemote := "https://remote.repo-server.instance"
+
+	cr.Spec.Repo.Remote = &repoServerRemote
+	assert.NoError(t, r.reconcileRepoDeployment(cr, false))
+
+	d := &appsv1.Deployment{}
+
+	assert.ErrorContains(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: cr.Name + "-repo-server", Namespace: cr.Namespace}, d),
+		"deployments.apps \""+cr.Name+"-repo-server\" not found")
+
+	// once remote is set to nil, reconciliation should trigger deployment resource creation
+	cr.Spec.Repo.Remote = nil
+
+	assert.NoError(t, r.reconcileRepoDeployment(cr, false))
+	assert.NoError(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: cr.Name + "-repo-server", Namespace: cr.Namespace}, d))
+}
diff --git a/controllers/argocd/service.go b/controllers/argocd/service.go
@@ -276,6 +276,9 @@ func (r *ReconcileArgoCD) reconcileRedisService(cr *argoproj.ArgoCD) error {
 		if cr.Spec.HA.Enabled {
 			return r.Client.Delete(context.TODO(), svc)
 		}
+		if cr.Spec.Redis.IsRemote() {
+			return r.Client.Delete(context.TODO(), svc)
+		}
 		return nil // Service found, do nothing
 	}
 
@@ -298,6 +301,11 @@ func (r *ReconcileArgoCD) reconcileRedisService(cr *argoproj.ArgoCD) error {
 		},
 	}
 
+	if cr.Spec.Redis.IsEnabled() && cr.Spec.Redis.IsRemote() {
+		log.Info("Skipping service creation, redis remote is enabled")
+		return nil
+	}
+
 	if err := controllerutil.SetControllerReference(cr, svc, r.Scheme); err != nil {
 		return err
 	}
@@ -361,6 +369,10 @@ func (r *ReconcileArgoCD) reconcileRepoService(cr *argoproj.ArgoCD) error {
 		if ensureAutoTLSAnnotation(r.Client, svc, common.ArgoCDRepoServerTLSSecretName, cr.Spec.Repo.WantsAutoTLS()) {
 			return r.Client.Update(context.TODO(), svc)
 		}
+		if cr.Spec.Repo.IsRemote() {
+			log.Info("skip creating repo server service, repo remote is enabled")
+			return r.Client.Delete(context.TODO(), svc)
+		}
 		return nil // Service found, do nothing
 	}
 
@@ -388,6 +400,11 @@ func (r *ReconcileArgoCD) reconcileRepoService(cr *argoproj.ArgoCD) error {
 		},
 	}
 
+	if cr.Spec.Repo.IsEnabled() && cr.Spec.Repo.IsRemote() {
+		log.Info("skip creating repo server service, repo remote is enabled")
+		return nil
+	}
+
 	if err := controllerutil.SetControllerReference(cr, svc, r.Scheme); err != nil {
 		return err
 	}

diff --git a/controllers/argocd/service_test.go b/controllers/argocd/service_test.go
@@ -4,12 +4,14 @@ import (
 	"context"
 	"testing"
 
+	"k8s.io/apimachinery/pkg/api/errors"
+
+	"k8s.io/apimachinery/pkg/types"
+
 	"github.com/stretchr/testify/assert"
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	argoproj "github.com/argoproj-labs/argocd-operator/api/v1beta1"
@@ -131,3 +133,46 @@ func TestReconcileServerService(t *testing.T) {
 		assert.Equal(t, a.Spec.Server.Service.Type, serverService.Spec.Type)
 	})
 }
+
+// If `remote` field is used in CR, then the component resources should not be created
+func TestReconcileArgoCD_reconcileRedisWithRemoteEn(t *testing.T) {
+	cr := makeTestArgoCD()
+
+	resObjs := []client.Object{cr}
+	subresObjs := []client.Object{cr}
+	runtimeObjs := []runtime.Object{}
+	sch := makeTestReconcilerScheme(argoproj.AddToScheme)
+	cl := makeTestReconcilerClient(sch, resObjs, subresObjs, runtimeObjs)
+	r := makeTestReconciler(cl, sch)
+
+	redisRemote := "https://remote.redis.instance"
+
+	cr.Spec.Redis.Remote = &redisRemote
+	assert.NoError(t, r.reconcileRedisService(cr))
+
+	s := &corev1.Service{}
+
+	assert.ErrorContains(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: cr.Name + "-redis", Namespace: cr.Namespace}, s),
+		"services \"argocd-redis\" not found")
+}
+
+func TestReconcileArgoCD_reconcileRepoServerWithRemoteEnabled(t *testing.T) {
+	cr := makeTestArgoCD()
+
+	resObjs := []client.Object{cr}
+	subresObjs := []client.Object{cr}
+	runtimeObjs := []runtime.Object{}
+	sch := makeTestReconcilerScheme(argoproj.AddToScheme)
+	cl := makeTestReconcilerClient(sch, resObjs, subresObjs, runtimeObjs)
+	r := makeTestReconciler(cl, sch)
+
+	repoServerRemote := "https://remote.repo-server.instance"
+
+	cr.Spec.Repo.Remote = &repoServerRemote
+	assert.NoError(t, r.reconcileRepoService(cr))
+
+	s := &corev1.Service{}
+
+	assert.ErrorContains(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: cr.Name + "-repo-server", Namespace: cr.Namespace}, s),
+		"services \"argocd-repo-server\" not found")
+}
diff --git a/docs/reference/argocd.md b/docs/reference/argocd.md
@@ -896,6 +896,7 @@ DisableTLSVerification | false | defines whether the redis server should be acce
 Image | `redis` | The container image for Redis. This overrides the `ARGOCD_REDIS_IMAGE` environment variable.
 Resources | [Empty] | The container compute resources.
 Version | 5.0.3 (SHA) | The tag to use with the Redis container image.
+Remote | "" | Specifies the remote URL of redis running in external clusters, also disables Redis component. This field is optional.
 
 ### Redis Example
 

diff --git a/docs/usage/enabling-disabling-argocd-core-components.md b/docs/usage/enabling-disabling-argocd-core-components.md
@@ -31,7 +31,7 @@ In this example, only the controller component is disabled, while all other comp
 
 # Specifying External URLs for Redis and RepoServer Components
 When disabling core components like Redis or Repo Server, you may wish to provide an external URL for components running in external clusters. The remote URL can be set using the `spec.<component>.remote` flag (where the component can only be `redis` or `repo`).
-
+Using `spec.<component>.remote` will skip the deployment & service creation. 
 For example,
 
 ```yaml
@@ -41,8 +41,5 @@ metadata:
   name: example
 spec:
   repo:
-    enabled: false
     remote: 'https://www.example.com/repo-server'
-```
-
-!!! Note: The remote flag can only be set if the enabled flag of the component is set to false.
+```
diff --git a/tests/k8s/1-043_validate_cleanup_when_using_remote/01-assert.yaml b/tests/k8s/1-043_validate_cleanup_when_using_remote/01-assert.yaml
@@ -0,0 +1,50 @@
+# Increase the timeout for the first test because it needs to download
+# a number of container images
+apiVersion: kuttl.dev/v1beta1
+kind: TestAssert
+timeout: 720
+---
+apiVersion: argoproj.io/v1alpha1
+kind: ArgoCD
+metadata:
+  name: example-argocd
+status:
+  phase: Available
+---
+apiVersion: apps/v1
+kind: StatefulSet
+metadata:
+  name: example-argocd-application-controller
+status:
+  readyReplicas: 1
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: example-argocd-redis
+status:
+  readyReplicas: 1
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: example-argocd-repo-server
+status:
+  readyReplicas: 1
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: example-argocd-server
+status:
+  readyReplicas: 1
+---
+apiVersion: v1
+kind: Service
+metadata:
+  name: example-argocd-repo-server
+---
+apiVersion: v1
+kind: Service
+metadata:
+  name: example-argocd-redis
diff --git a/tests/k8s/1-043_validate_cleanup_when_using_remote/01-basic.yaml b/tests/k8s/1-043_validate_cleanup_when_using_remote/01-basic.yaml
@@ -0,0 +1,7 @@
+apiVersion: argoproj.io/v1alpha1
+kind: ArgoCD
+metadata:
+  name: example-argocd
+  labels:
+    example: basic
+spec: {}
diff --git a/tests/k8s/1-043_validate_cleanup_when_using_remote/02-add-remote.yaml b/tests/k8s/1-043_validate_cleanup_when_using_remote/02-add-remote.yaml
@@ -0,0 +1,11 @@
+apiVersion: argoproj.io/v1alpha1
+kind: ArgoCD
+metadata:
+  name: example-argocd
+  labels:
+    example: basic
+spec:
+  redis:
+    remote: https://redis.remote.host:6379
+  repo:
+    remote: https://repo-server.remote.host:8081
diff --git a/tests/k8s/1-043_validate_cleanup_when_using_remote/02-error.yaml b/tests/k8s/1-043_validate_cleanup_when_using_remote/02-error.yaml
@@ -0,0 +1,30 @@
+# Increase the timeout for the first test because it needs to download
+# a number of container images
+apiVersion: kuttl.dev/v1beta1
+kind: TestStep
+commands:
+  - script: sleep 30
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: example-argocd-redis
+status:
+  readyReplicas: 1
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: example-argocd-repo-server
+status:
+  readyReplicas: 1
+---
+apiVersion: v1
+kind: Service
+metadata:
+  name: example-argocd-repo-server
+---
+apiVersion: v1
+kind: Service
+metadata:
+  name: example-argocd-redis