fix: reconcile Pooler service and add labels. (#3349)

This patch fixes two issues that we currently have with the Pooler
service.

* we never reconcile the service once is created
* we don't add the proper labels to the created service

Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
(cherry picked from commit b1c8113)

controllers/pooler_resources_test.go

@@ -120,7 +120,7 @@ var _ = Describe("pooler_resources unit tests", func() {
 		})
 
 		By("creating the service", func() {
-			service := pgbouncer.Service(pooler)
+			service := pgbouncer.Service(pooler, cluster)
 			err := poolerReconciler.Create(ctx, service)
 			Expect(err).ToNot(HaveOccurred())
 		})

controllers/pooler_update.go

@@ -52,7 +52,7 @@ func (r *PoolerReconciler) updateOwnedObjects(
 		return err
 	}
 
-	if err := r.updateService(ctx, pooler, resources); err != nil {
+	if err := r.reconcileService(ctx, pooler, resources); err != nil {
 		return err
 	}
 
@@ -116,32 +116,43 @@ func (r *PoolerReconciler) updateDeployment(
 	return nil
 }
 
-// updateService update or create the pgbouncer service as needed
-//
-//nolint:dupl
-func (r *PoolerReconciler) updateService(
+// reconcileService update or create the pgbouncer service as needed
+func (r *PoolerReconciler) reconcileService(
 	ctx context.Context,
 	pooler *apiv1.Pooler,
 	resources *poolerManagedResources,
 ) error {
 	contextLog := log.FromContext(ctx)
+	expectedService := pgbouncer.Service(pooler, resources.Cluster)
+	if err := ctrl.SetControllerReference(pooler, expectedService, r.Scheme); err != nil {
+		return err
+	}
 
 	if resources.Service == nil {
-		service := pgbouncer.Service(pooler)
-		if err := ctrl.SetControllerReference(pooler, service, r.Scheme); err != nil {
-			return err
-		}
-
-		contextLog.Info("Creating service")
-		err := r.Create(ctx, service)
+		contextLog.Info("Creating the service")
+		err := r.Create(ctx, expectedService)
 		if err != nil && !apierrs.IsAlreadyExists(err) {
 			return err
 		}
-		resources.Service = service
+		resources.Service = expectedService
 		return nil
 	}
 
-	return nil
+	patchedService := resources.Service.DeepCopy()
+	patchedService.Spec = expectedService.Spec
+	if patchedService.Labels == nil {
+		patchedService.Labels = make(map[string]string)
+	}
+	utils.MergeMap(patchedService.Labels, expectedService.Labels)
+
+	if reflect.DeepEqual(patchedService.ObjectMeta, resources.Service.ObjectMeta) &&
+		reflect.DeepEqual(patchedService.Spec, resources.Service.Spec) {
+		return nil
+	}
+
+	contextLog.Info("Updating the service metadata")
+
+	return r.Patch(ctx, patchedService, client.MergeFrom(resources.Service))
 }
 
 // updateRBAC update or create the pgbouncer RBAC

controllers/pooler_update_test.go

@@ -214,7 +214,7 @@ var _ = Describe("unit test of pooler_update reconciliation logic", func() {
 		})
 	})
 
-	It("should test the Service update logic", func() {
+	It("should reconcileService works correctly", func() {
 		ctx := context.Background()
 		namespace := newFakeNamespace()
 		cluster := newFakeCNPGCluster(namespace)
@@ -223,20 +223,22 @@ var _ = Describe("unit test of pooler_update reconciliation logic", func() {
 
 		By("making sure the service doesn't exist", func() {
 			svc := &corev1.Service{}
-			expectedSVC := pgbouncer.Service(pooler)
+			expectedSVC := pgbouncer.Service(pooler, cluster)
 			err := k8sClient.Get(ctx, types.NamespacedName{Name: expectedSVC.Name, Namespace: expectedSVC.Namespace}, svc)
 			Expect(apierrors.IsNotFound(err)).To(BeTrue())
 		})
 
-		By("making sure it updateService creates the service", func() {
-			err := poolerReconciler.updateService(ctx, pooler, res)
+		By("making sure it creates the service", func() {
+			err := poolerReconciler.reconcileService(ctx, pooler, res)
 			Expect(err).ToNot(HaveOccurred())
 
 			svc := &corev1.Service{}
-			expectedSVC := pgbouncer.Service(pooler)
+			expectedSVC := pgbouncer.Service(pooler, cluster)
 			err = k8sClient.Get(ctx, types.NamespacedName{Name: expectedSVC.Name, Namespace: expectedSVC.Namespace}, svc)
 			Expect(err).ToNot(HaveOccurred())
 
+			Expect(expectedSVC.Labels[utils.ClusterLabelName]).To(Equal(cluster.Name))
+			Expect(expectedSVC.Labels[utils.PgbouncerNameLabel]).To(Equal(pooler.Name))
 			Expect(expectedSVC.Spec.Selector).To(Equal(svc.Spec.Selector))
 			Expect(expectedSVC.Spec.Ports).To(Equal(svc.Spec.Ports))
 			Expect(expectedSVC.Spec.Type).To(Equal(svc.Spec.Type))
@@ -245,15 +247,32 @@ var _ = Describe("unit test of pooler_update reconciliation logic", func() {
 
 		By("making sure the svc doesn't get updated if there are not changes", func() {
 			previousResourceVersion := res.Service.ResourceVersion
-			err := poolerReconciler.updateService(ctx, pooler, res)
+			err := poolerReconciler.reconcileService(ctx, pooler, res)
 			Expect(err).ToNot(HaveOccurred())
 
 			svc := &corev1.Service{}
-			expectedSVC := pgbouncer.Service(pooler)
+			expectedSVC := pgbouncer.Service(pooler, cluster)
 			err = k8sClient.Get(ctx, types.NamespacedName{Name: expectedSVC.Name, Namespace: expectedSVC.Namespace}, svc)
 			Expect(err).ToNot(HaveOccurred())
 
 			Expect(previousResourceVersion).To(Equal(svc.ResourceVersion))
 		})
+
+		By("making sure it reconciles if differences from the living and expected service are present", func() {
+			previousName := cluster.Name
+			previousResourceVersion := res.Service.ResourceVersion
+			cluster.Name = "new-name"
+
+			err := poolerReconciler.reconcileService(ctx, pooler, res)
+			Expect(err).ToNot(HaveOccurred())
+
+			svc := &corev1.Service{}
+			expectedSVC := pgbouncer.Service(pooler, cluster)
+			err = k8sClient.Get(ctx, types.NamespacedName{Name: expectedSVC.Name, Namespace: expectedSVC.Namespace}, svc)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(previousResourceVersion).ToNot(Equal(svc.ResourceVersion))
+			Expect(expectedSVC.Labels[utils.ClusterLabelName]).ToNot(Equal(previousName))
+			Expect(expectedSVC.Labels[utils.ClusterLabelName]).To(Equal(cluster.Name))
+		})
 	})
 })

pkg/specs/pgbouncer/services.go

@@ -28,11 +28,15 @@ import (
 
 // Service create the specification for the service of
 // pgbouncer
-func Service(pooler *apiv1.Pooler) *corev1.Service {
+func Service(pooler *apiv1.Pooler, cluster *apiv1.Cluster) *corev1.Service {
 	return &corev1.Service{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      pooler.Name,
 			Namespace: pooler.Namespace,
+			Labels: map[string]string{
+				utils.PgbouncerNameLabel: pooler.Name,
+				utils.ClusterLabelName:   cluster.Name,
+			},
 		},
 		Spec: corev1.ServiceSpec{
 			Type: corev1.ServiceTypeClusterIP,

pkg/specs/pgbouncer/services_test.go

@@ -30,7 +30,10 @@ import (
 )
 
 var _ = Describe("Pooler Service", func() {
-	var pooler *apiv1.Pooler
+	var (
+		pooler  *apiv1.Pooler
+		cluster *apiv1.Cluster
+	)
 
 	BeforeEach(func() {
 		pooler = &apiv1.Pooler{
@@ -39,13 +42,21 @@ var _ = Describe("Pooler Service", func() {
 				Namespace: "test-namespace",
 			},
 		}
+		cluster = &apiv1.Cluster{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "test-cluster",
+				Namespace: "test-namespace",
+			},
+		}
 	})
 
 	Context("when creating a Service", func() {
 		It("returns the correct Service", func() {
-			service := Service(pooler)
+			service := Service(pooler, cluster)
 			Expect(service.Name).To(Equal(pooler.Name))
 			Expect(service.Namespace).To(Equal(pooler.Namespace))
+			Expect(service.Labels[utils.ClusterLabelName]).To(Equal(cluster.Name))
+			Expect(service.Labels[utils.PgbouncerNameLabel]).To(Equal(pooler.Name))
 			Expect(service.Spec.Type).To(Equal(corev1.ServiceTypeClusterIP))
 			Expect(service.Spec.Ports).To(ConsistOf(corev1.ServicePort{
 				Name:       "pgbouncer",