From da6b8a7e68f71920de9545152714b9066990fc4b Mon Sep 17 00:00:00 2001
From: nmirasch <neus.miras@gmail.com>
Date: Fri, 27 Sep 2024 09:31:47 +0200
Subject: [PATCH] fix: Add Server Service check to apply the changes in case
 the service exists (#1546)

* Add Server Service check to apply the changes in case the service exists

Signed-off-by: nmirasch <neus.miras@gmail.com>

* Add changes to existing service and update

Signed-off-by: nmirasch <neus.miras@gmail.com>

* Add testing

Signed-off-by: nmirasch <neus.miras@gmail.com>

---------

Signed-off-by: nmirasch <neus.miras@gmail.com>
---
 controllers/argocd/service.go      | 38 ++++++++++++++---------
 controllers/argocd/service_test.go | 50 ++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 14 deletions(-)

diff --git a/controllers/argocd/service.go b/controllers/argocd/service.go
index 98bb02707..23dd3180e 100644
--- a/controllers/argocd/service.go
+++ b/controllers/argocd/service.go
@@ -17,6 +17,7 @@ package argocd
 import (
 	"context"
 	"fmt"
+	"reflect"
 
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -422,20 +423,6 @@ func (r *ReconcileArgoCD) reconcileServerMetricsService(cr *argoproj.ArgoCD) err
 // reconcileServerService will ensure that the Service is present for the Argo CD server component.
 func (r *ReconcileArgoCD) reconcileServerService(cr *argoproj.ArgoCD) error {
 	svc := newServiceWithSuffix("server", "server", cr)
-	if argoutil.IsObjectFound(r.Client, cr.Namespace, svc.Name, svc) {
-		if !cr.Spec.Server.IsEnabled() {
-			return r.Client.Delete(context.TODO(), svc)
-		}
-		if ensureAutoTLSAnnotation(r.Client, svc, common.ArgoCDServerTLSSecretName, cr.Spec.Server.WantsAutoTLS()) {
-			return r.Client.Update(context.TODO(), svc)
-		}
-		return nil // Service found, do nothing
-	}
-
-	if !cr.Spec.Repo.IsEnabled() {
-		return nil
-	}
-
 	ensureAutoTLSAnnotation(r.Client, svc, common.ArgoCDServerTLSSecretName, cr.Spec.Server.WantsAutoTLS())
 
 	svc.Spec.Ports = []corev1.ServicePort{
@@ -461,6 +448,29 @@ func (r *ReconcileArgoCD) reconcileServerService(cr *argoproj.ArgoCD) error {
 	if err := controllerutil.SetControllerReference(cr, svc, r.Scheme); err != nil {
 		return err
 	}
+
+	existingSVC := &corev1.Service{}
+	if argoutil.IsObjectFound(r.Client, cr.Namespace, svc.Name, existingSVC) {
+		changed := false
+		if !cr.Spec.Server.IsEnabled() {
+			return r.Client.Delete(context.TODO(), svc)
+		}
+		if ensureAutoTLSAnnotation(r.Client, existingSVC, common.ArgoCDServerTLSSecretName, cr.Spec.Server.WantsAutoTLS()) {
+			changed = true
+		}
+		if !reflect.DeepEqual(svc.Spec.Type, existingSVC.Spec.Type) {
+			existingSVC.Spec.Type = svc.Spec.Type
+			changed = true
+		}
+		if changed {
+			return r.Client.Update(context.TODO(), existingSVC)
+		}
+		return nil
+	}
+
+	if !cr.Spec.Server.IsEnabled() {
+		return nil
+	}
 	return r.Client.Create(context.TODO(), svc)
 }
 
diff --git a/controllers/argocd/service_test.go b/controllers/argocd/service_test.go
index 08c3bc066..a9743a993 100644
--- a/controllers/argocd/service_test.go
+++ b/controllers/argocd/service_test.go
@@ -6,8 +6,10 @@ import (
 
 	"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"
@@ -81,3 +83,51 @@ func TestEnsureAutoTLSAnnotation(t *testing.T) {
 		assert.Equal(t, needUpdate, false)
 	})
 }
+
+func TestReconcileServerService(t *testing.T) {
+	a := makeTestArgoCD()
+	resObjs := []client.Object{a}
+	subresObjs := []client.Object{a}
+	runtimeObjs := []runtime.Object{}
+	sch := makeTestReconcilerScheme(argoproj.AddToScheme)
+	cl := makeTestReconcilerClient(sch, resObjs, subresObjs, runtimeObjs)
+	r := makeTestReconciler(cl, sch)
+	a = makeTestArgoCD(func(a *argoproj.ArgoCD) {
+		a.Spec.Server.Service.Type = "NodePort"
+	})
+	serverService := newServiceWithSuffix("server", "server", a)
+	t.Run("Server Service Created when the Server Service is not found ", func(t *testing.T) {
+		err := r.Client.Get(context.TODO(), types.NamespacedName{
+			Name:      "argocd-server",
+			Namespace: testNamespace,
+		}, serverService)
+		assert.True(t, errors.IsNotFound(err))
+
+		err = r.reconcileServerService(a)
+		assert.NoError(t, err)
+
+		err = r.Client.Get(context.TODO(), types.NamespacedName{
+			Name:      "argocd-server",
+			Namespace: testNamespace,
+		}, serverService)
+		assert.NoError(t, err)
+		assert.Equal(t, a.Spec.Server.Service.Type, serverService.Spec.Type)
+	})
+
+	t.Run("Server Service Type update ", func(t *testing.T) {
+		// Reconcile with previous existing Server Service with a different Type
+		a.Spec.Server.Service.Type = "ClusterIP"
+		assert.NotEqual(t, a.Spec.Server.Service.Type, serverService.Spec.Type)
+
+		err := r.reconcileServerService(a)
+		assert.NoError(t, err)
+
+		// Existing Server is found and has the argoCD new Server Service Type
+		err = r.Client.Get(context.TODO(), types.NamespacedName{
+			Name:      "argocd-server",
+			Namespace: testNamespace,
+		}, serverService)
+		assert.NoError(t, err)
+		assert.Equal(t, a.Spec.Server.Service.Type, serverService.Spec.Type)
+	})
+}