From 4522505bc6942a96f9d976bccaaa5b12e94b61f0 Mon Sep 17 00:00:00 2001
From: Tanmay Jain <103629776+tanmayja@users.noreply.github.com>
Date: Wed, 24 Jan 2024 17:12:34 +0530
Subject: [PATCH] Allow loadbalancer spec update. (#263)

* Allow loadbalancer spec update.
---
 api/v1/aerospikecluster_validating_webhook.go | 20 -------
 controllers/service.go                        | 53 ++++++++++++++-----
 test/cluster_test.go                          |  3 +-
 test/services_test.go                         |  4 +-
 4 files changed, 44 insertions(+), 36 deletions(-)

diff --git a/api/v1/aerospikecluster_validating_webhook.go b/api/v1/aerospikecluster_validating_webhook.go
index e23fdcf76..d27173cb9 100644
--- a/api/v1/aerospikecluster_validating_webhook.go
+++ b/api/v1/aerospikecluster_validating_webhook.go
@@ -123,14 +123,6 @@ func (c *AerospikeCluster) ValidateUpdate(oldObj runtime.Object) (admission.Warn
 		return nil, err
 	}
 
-	// Validate Load Balancer update
-	if err := validateLoadBalancerUpdate(
-		aslog, c.Spec.SeedsFinderServices.LoadBalancer,
-		old.Spec.SeedsFinderServices.LoadBalancer,
-	); err != nil {
-		return nil, err
-	}
-
 	// Validate RackConfig update
 	return nil, c.validateRackUpdate(aslog, old)
 }
@@ -1274,18 +1266,6 @@ func getNamespaceReplicationFactor(nsConf map[string]interface{}) (int, error) {
 	return rf, nil
 }
 
-func validateLoadBalancerUpdate(
-	aslog logr.Logger, newLBSpec, oldLBSpec *LoadBalancerSpec,
-) error {
-	aslog.Info("Validate LoadBalancer update")
-
-	if oldLBSpec != nil && !reflect.DeepEqual(oldLBSpec, newLBSpec) {
-		return fmt.Errorf("cannot update existing LoadBalancer Service")
-	}
-
-	return nil
-}
-
 func validateSecurityConfigUpdate(
 	newVersion, oldVersion string, newSpec, oldSpec *AerospikeConfigSpec,
 ) error {
diff --git a/controllers/service.go b/controllers/service.go
index 6df06edc7..b95a68974 100644
--- a/controllers/service.go
+++ b/controllers/service.go
@@ -158,22 +158,49 @@ func (r *SingleClusterReconciler) createOrUpdateSTSLoadBalancerSvc() error {
 	r.Log.Info("LoadBalancer service already exist for cluster, checking for update",
 		"name", utils.NamespacedName(service.Namespace, service.Name))
 
-	if len(service.Spec.Ports) == 1 && service.Spec.Ports[0].Port == servicePort.Port &&
-		service.Spec.Ports[0].TargetPort == servicePort.TargetPort {
-		r.Log.Info("LoadBalancer service update not required, skipping",
-			"name", utils.NamespacedName(service.Namespace, service.Name))
-		return nil
+	return r.updateLBService(service, &servicePort, loadBalancer)
+}
+
+func (r *SingleClusterReconciler) updateLBService(service *corev1.Service, servicePort *corev1.ServicePort,
+	loadBalancer *asdbv1.LoadBalancerSpec) error {
+	updateLBService := false
+
+	if len(service.Spec.Ports) != 1 || service.Spec.Ports[0].Port != servicePort.Port ||
+		service.Spec.Ports[0].TargetPort != servicePort.TargetPort ||
+		service.Spec.Ports[0].Name != servicePort.Name {
+		updateLBService = true
+		service.Spec.Ports = []corev1.ServicePort{
+			*servicePort,
+		}
 	}
 
-	service.Spec.Ports = []corev1.ServicePort{
-		servicePort,
+	if !reflect.DeepEqual(service.ObjectMeta.Annotations, loadBalancer.Annotations) {
+		service.ObjectMeta.Annotations = loadBalancer.Annotations
+		updateLBService = true
 	}
-	if err := r.Client.Update(
-		context.TODO(), service, updateOption,
-	); err != nil {
-		return fmt.Errorf(
-			"failed to update service %s: %v", serviceName, err,
-		)
+
+	if !reflect.DeepEqual(service.Spec.LoadBalancerSourceRanges, loadBalancer.LoadBalancerSourceRanges) {
+		service.Spec.LoadBalancerSourceRanges = loadBalancer.LoadBalancerSourceRanges
+		updateLBService = true
+	}
+
+	if !reflect.DeepEqual(service.Spec.ExternalTrafficPolicy, loadBalancer.ExternalTrafficPolicy) {
+		service.Spec.ExternalTrafficPolicy = loadBalancer.ExternalTrafficPolicy
+		updateLBService = true
+	}
+
+	if updateLBService {
+		if err := r.Client.Update(
+			context.TODO(), service, updateOption,
+		); err != nil {
+			return fmt.Errorf(
+				"failed to update service %s: %v", service.Name, err,
+			)
+		}
+	} else {
+		r.Log.Info("LoadBalancer service update not required, skipping",
+			"name", utils.NamespacedName(service.Namespace, service.Name))
+		return nil
 	}
 
 	r.Log.Info("LoadBalancer service updated",
diff --git a/test/cluster_test.go b/test/cluster_test.go
index 697256638..ff5997c95 100644
--- a/test/cluster_test.go
+++ b/test/cluster_test.go
@@ -261,7 +261,8 @@ func clusterWithMaxIgnorablePod(ctx goctx.Context) {
 						err = k8sClient.Get(ctx, types.NamespacedName{Name: ignorePodName,
 							Namespace: clusterNamespacedName.Namespace}, pod)
 
-						return *pod.Status.ContainerStatuses[0].Started && pod.Status.ContainerStatuses[0].Ready
+						return len(pod.Status.ContainerStatuses) != 0 && *pod.Status.ContainerStatuses[0].Started &&
+							pod.Status.ContainerStatuses[0].Ready
 					}, 1*time.Minute).Should(BeTrue())
 
 					Eventually(func() error {
diff --git a/test/services_test.go b/test/services_test.go
index b7e49e8d8..10d79fbcb 100644
--- a/test/services_test.go
+++ b/test/services_test.go
@@ -57,7 +57,7 @@ var _ = Describe(
 		)
 
 		It(
-			"Validate LB can not be updated", func() {
+			"Validate LB can be updated", func() {
 				By("DeployCluster")
 				clusterNamespacedName := getNamespacedName(
 					"load-balancer-invalid", namespace,
@@ -76,7 +76,7 @@ var _ = Describe(
 				Expect(err).ToNot(HaveOccurred())
 				aeroCluster.Spec.SeedsFinderServices.LoadBalancer.ExternalTrafficPolicy = "Cluster"
 				err = updateCluster(k8sClient, ctx, aeroCluster)
-				Expect(err).To(HaveOccurred())
+				Expect(err).ToNot(HaveOccurred())
 
 				err = deleteCluster(k8sClient, ctx, aeroCluster)
 				Expect(err).ToNot(HaveOccurred())