From 54a1b8f8612a78856be676dec5c3d83e93060c93 Mon Sep 17 00:00:00 2001 From: Tanmay Jain Date: Wed, 13 Dec 2023 14:29:48 +0530 Subject: [PATCH 1/3] Allow loadbalancer spec update. --- api/v1/aerospikecluster_validating_webhook.go | 4 +- controllers/service.go | 79 +++++++++++++++---- test/services_test.go | 4 +- 3 files changed, 69 insertions(+), 18 deletions(-) diff --git a/api/v1/aerospikecluster_validating_webhook.go b/api/v1/aerospikecluster_validating_webhook.go index 96d8e9454..4c3f9d7be 100644 --- a/api/v1/aerospikecluster_validating_webhook.go +++ b/api/v1/aerospikecluster_validating_webhook.go @@ -1278,8 +1278,8 @@ func validateLoadBalancerUpdate( ) error { aslog.Info("Validate LoadBalancer update") - if oldLBSpec != nil && !reflect.DeepEqual(oldLBSpec, newLBSpec) { - return fmt.Errorf("cannot update existing LoadBalancer Service") + if oldLBSpec != nil && oldLBSpec.PortName != newLBSpec.PortName { + return fmt.Errorf("cannot update existing LoadBalancer Service name") } return nil diff --git a/controllers/service.go b/controllers/service.go index 6df06edc7..9e75d735f 100644 --- a/controllers/service.go +++ b/controllers/service.go @@ -158,22 +158,65 @@ 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 + aerospikePortNameFound := false + + for idx := range service.Spec.Ports { + if service.Spec.Ports[idx].Name == servicePort.Name { + aerospikePortNameFound = true + + if service.Spec.Ports[idx].Port != servicePort.Port || + service.Spec.Ports[idx].TargetPort != servicePort.TargetPort { + service.Spec.Ports[idx].Port = servicePort.Port + service.Spec.Ports[idx].TargetPort = servicePort.TargetPort + updateLBService = true + } + + break + } } - service.Spec.Ports = []corev1.ServicePort{ - servicePort, + // If there is no port name set for in port added by operator, then set it and overwrite ports set by user. + if !aerospikePortNameFound { + service.Spec.Ports = []corev1.ServicePort{ + *servicePort, + } + + 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.ObjectMeta.Annotations, loadBalancer.Annotations) { + service.ObjectMeta.Annotations = loadBalancer.Annotations + updateLBService = true + } + + 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", @@ -351,9 +394,17 @@ func (r *SingleClusterReconciler) getLBServicePort(loadBalancer *asdbv1.LoadBala port = targetPort } + var portName string + if loadBalancer.PortName == "" { + // If port is specified in CR. + portName = "aerospikeport" + } else { + portName = loadBalancer.PortName + } + return corev1.ServicePort{ Port: port, - Name: loadBalancer.PortName, + Name: portName, TargetPort: intstr.FromInt(int(targetPort)), } } 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()) From 2aa2834b61e9c38dc7524b97775ba0c8403f2e4e Mon Sep 17 00:00:00 2001 From: Tanmay Jain Date: Wed, 13 Dec 2023 16:52:40 +0530 Subject: [PATCH 2/3] overwrite user specified port in lb service --- api/v1/aerospikecluster_validating_webhook.go | 20 ----------- controllers/service.go | 34 +++---------------- 2 files changed, 5 insertions(+), 49 deletions(-) diff --git a/api/v1/aerospikecluster_validating_webhook.go b/api/v1/aerospikecluster_validating_webhook.go index 4c3f9d7be..707d4a054 100644 --- a/api/v1/aerospikecluster_validating_webhook.go +++ b/api/v1/aerospikecluster_validating_webhook.go @@ -122,14 +122,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) } @@ -1273,18 +1265,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 && oldLBSpec.PortName != newLBSpec.PortName { - return fmt.Errorf("cannot update existing LoadBalancer Service name") - } - - return nil -} - func validateSecurityConfigUpdate( newVersion, oldVersion string, newSpec, oldSpec *AerospikeConfigSpec, ) error { diff --git a/controllers/service.go b/controllers/service.go index 9e75d735f..b95a68974 100644 --- a/controllers/service.go +++ b/controllers/service.go @@ -164,30 +164,14 @@ func (r *SingleClusterReconciler) createOrUpdateSTSLoadBalancerSvc() error { func (r *SingleClusterReconciler) updateLBService(service *corev1.Service, servicePort *corev1.ServicePort, loadBalancer *asdbv1.LoadBalancerSpec) error { updateLBService := false - aerospikePortNameFound := false - for idx := range service.Spec.Ports { - if service.Spec.Ports[idx].Name == servicePort.Name { - aerospikePortNameFound = true - - if service.Spec.Ports[idx].Port != servicePort.Port || - service.Spec.Ports[idx].TargetPort != servicePort.TargetPort { - service.Spec.Ports[idx].Port = servicePort.Port - service.Spec.Ports[idx].TargetPort = servicePort.TargetPort - updateLBService = true - } - - break - } - } - - // If there is no port name set for in port added by operator, then set it and overwrite ports set by user. - if !aerospikePortNameFound { + 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, } - - updateLBService = true } if !reflect.DeepEqual(service.ObjectMeta.Annotations, loadBalancer.Annotations) { @@ -394,17 +378,9 @@ func (r *SingleClusterReconciler) getLBServicePort(loadBalancer *asdbv1.LoadBala port = targetPort } - var portName string - if loadBalancer.PortName == "" { - // If port is specified in CR. - portName = "aerospikeport" - } else { - portName = loadBalancer.PortName - } - return corev1.ServicePort{ Port: port, - Name: portName, + Name: loadBalancer.PortName, TargetPort: intstr.FromInt(int(targetPort)), } } From 3efce42604ed8ecc12df34f56c0accaf54f36cf1 Mon Sep 17 00:00:00 2001 From: Tanmay Jain Date: Tue, 2 Jan 2024 15:19:06 +0530 Subject: [PATCH 3/3] Fixing testcase --- test/cluster_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 {