update createHeadlessService() to update labels (#7865)

Author: vepatel

cmd/nginx-ingress/main.go

@@ -11,6 +11,7 @@ import (
 	"os"
 	"os/signal"
 	"path/filepath"
+	"reflect"
 	"regexp"
 	"runtime"
 	"strings"
@@ -1089,7 +1090,7 @@ func updateSelfWithVersionInfo(ctx context.Context, eventLog record.EventRecorde
 	}
 }
 
-func createAndValidateHeadlessService(ctx context.Context, kubeClient *kubernetes.Clientset, cfgParams *configs.ConfigParams, controllerNamespace string, pod *api_v1.Pod) error {
+func createAndValidateHeadlessService(ctx context.Context, kubeClient kubernetes.Interface, cfgParams *configs.ConfigParams, controllerNamespace string, pod *api_v1.Pod) error {
 	l := nl.LoggerFromContext(ctx)
 	owner := pod.ObjectMeta.OwnerReferences[0]
 	name := owner.Name
@@ -1107,13 +1108,7 @@ func createAndValidateHeadlessService(ctx context.Context, kubeClient *kubernete
 	return nil
 }
 
-func createHeadlessService(l *slog.Logger, kubeClient *kubernetes.Clientset, controllerNamespace string, svcName string, configMapNamespacedName string, pod *api_v1.Pod) error {
-	existing, err := kubeClient.CoreV1().Services(controllerNamespace).Get(context.Background(), svcName, meta_v1.GetOptions{})
-	if err == nil && existing != nil {
-		nl.Infof(l, "headless service %s/%s already exists, skipping creating.", controllerNamespace, svcName)
-		return nil
-	}
-
+func createHeadlessService(l *slog.Logger, kubeClient kubernetes.Interface, controllerNamespace string, svcName string, configMapNamespacedName string, pod *api_v1.Pod) error {
 	configMapName := strings.SplitN(configMapNamespacedName, "/", 2)
 	if len(configMapName) != 2 {
 		return fmt.Errorf("wrong syntax for ConfigMap: %q", configMapNamespacedName)
@@ -1125,24 +1120,49 @@ func createHeadlessService(l *slog.Logger, kubeClient *kubernetes.Clientset, con
 		return err
 	}
 
+	requiredSelectors := pod.Labels
+	requiredOwnerReferences := []meta_v1.OwnerReference{
+		{
+			APIVersion:         "v1",
+			Kind:               "ConfigMap",
+			Name:               configMapObj.Name,
+			UID:                configMapObj.UID,
+			Controller:         commonhelpers.BoolToPointerBool(true),
+			BlockOwnerDeletion: commonhelpers.BoolToPointerBool(true),
+		},
+	}
+	existing, err := kubeClient.CoreV1().Services(controllerNamespace).Get(context.Background(), svcName, meta_v1.GetOptions{})
+	if err == nil && existing != nil {
+		needsUpdate := false
+		if !reflect.DeepEqual(existing.Spec.Selector, requiredSelectors) {
+			existing.Spec.Selector = requiredSelectors
+			needsUpdate = true
+		}
+		if !reflect.DeepEqual(existing.OwnerReferences, requiredOwnerReferences) {
+			existing.OwnerReferences = requiredOwnerReferences
+			needsUpdate = true
+		}
+		if needsUpdate {
+			nl.Infof(l, "Headless service %s/%s exists and needs update. Updating...", controllerNamespace, svcName)
+			_, updateErr := kubeClient.CoreV1().Services(controllerNamespace).Update(context.Background(), existing, meta_v1.UpdateOptions{})
+			if updateErr != nil {
+				return fmt.Errorf("failed to update headless service %s/%s: %w", controllerNamespace, svcName, updateErr)
+			}
+			nl.Infof(l, "Successfully updated headless service %s/%s.", controllerNamespace, svcName)
+		}
+		return nil
+	}
+
+	nl.Infof(l, "Headless service %s/%s not found. Creating...", controllerNamespace, svcName)
 	svc := &api_v1.Service{
 		ObjectMeta: meta_v1.ObjectMeta{
-			Name:      svcName,
-			Namespace: controllerNamespace,
-			OwnerReferences: []meta_v1.OwnerReference{
-				{
-					APIVersion:         "v1",
-					Kind:               "ConfigMap",
-					Name:               configMapObj.Name,
-					UID:                configMapObj.UID,
-					Controller:         commonhelpers.BoolToPointerBool(true),
-					BlockOwnerDeletion: commonhelpers.BoolToPointerBool(true),
-				},
-			},
+			Name:            svcName,
+			Namespace:       controllerNamespace,
+			OwnerReferences: requiredOwnerReferences,
 		},
 		Spec: api_v1.ServiceSpec{
 			ClusterIP: api_v1.ClusterIPNone,
-			Selector:  pod.Labels,
+			Selector:  requiredSelectors,
 		},
 	}
 

cmd/nginx-ingress/main_test.go

@@ -3,14 +3,21 @@ package main
 import (
 	"bytes"
 	"context"
+	"fmt"
 	"io"
 	"log/slog"
 	"regexp"
 	"testing"
 
+	"github.com/nginx/kubernetes-ingress/internal/configs/commonhelpers"
 	nl "github.com/nginx/kubernetes-ingress/internal/logger"
 	nic_glog "github.com/nginx/kubernetes-ingress/internal/logger/glog"
 	"github.com/nginx/kubernetes-ingress/internal/logger/levels"
+	"github.com/stretchr/testify/assert"
+	api_v1 "k8s.io/api/core/v1"
+	meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
 	pkgversion "k8s.io/apimachinery/pkg/version"
 	fakediscovery "k8s.io/client-go/discovery/fake"
 	"k8s.io/client-go/kubernetes/fake"
@@ -131,3 +138,176 @@ func TestK8sVersionValidationBad(t *testing.T) {
 		})
 	}
 }
+
+func TestCreateHeadlessService(t *testing.T) {
+	logger := nl.LoggerFromContext(context.Background())
+	controllerNamespace := "default"
+	configMapName := "test-configmap"
+	configMapNamespace := "default"
+	configMapNamespacedName := fmt.Sprintf("%s/%s", configMapNamespace, configMapName)
+	podName := "test-pod"
+	podLabels := map[string]string{"app": "my-app", "pod-hash": "12345"}
+	svcName := "test-hl-service"
+
+	pod := &api_v1.Pod{
+		ObjectMeta: meta_v1.ObjectMeta{
+			Name:      podName,
+			Namespace: controllerNamespace,
+			Labels:    podLabels,
+		},
+	}
+
+	configMap := &api_v1.ConfigMap{
+		ObjectMeta: meta_v1.ObjectMeta{
+			Name:      configMapName,
+			Namespace: configMapNamespace,
+			UID:       types.UID("uid-cm"),
+		},
+	}
+
+	expectedOwnerReferences := []meta_v1.OwnerReference{
+		{
+			APIVersion:         "v1",
+			Kind:               "ConfigMap",
+			Name:               configMap.Name,
+			UID:                configMap.UID,
+			Controller:         commonhelpers.BoolToPointerBool(true),
+			BlockOwnerDeletion: commonhelpers.BoolToPointerBool(true),
+		},
+	}
+
+	testCases := []struct {
+		name                 string
+		existingService      *api_v1.Service
+		expectedAction       string
+		expectedSelector     map[string]string
+		expectedOwnerRefs    []meta_v1.OwnerReference
+		initialClientObjects []runtime.Object
+	}{
+		{
+			name:                 "Create service if none found",
+			expectedAction:       "create",
+			expectedSelector:     podLabels,
+			expectedOwnerRefs:    expectedOwnerReferences,
+			initialClientObjects: []runtime.Object{pod, configMap},
+		},
+		{
+			name: "Skip update if labels and ownerReferences are the same",
+			existingService: &api_v1.Service{
+				ObjectMeta: meta_v1.ObjectMeta{
+					Name:            svcName,
+					Namespace:       controllerNamespace,
+					OwnerReferences: expectedOwnerReferences,
+				},
+				Spec: api_v1.ServiceSpec{
+					Selector: podLabels,
+				},
+			},
+			expectedAction:       "none",
+			expectedSelector:     podLabels,
+			expectedOwnerRefs:    expectedOwnerReferences,
+			initialClientObjects: []runtime.Object{pod, configMap},
+		},
+		{
+			name: "Update service if labels differ",
+			existingService: &api_v1.Service{
+				ObjectMeta: meta_v1.ObjectMeta{
+					Name:            svcName,
+					Namespace:       controllerNamespace,
+					OwnerReferences: expectedOwnerReferences,
+				},
+				Spec: api_v1.ServiceSpec{
+					Selector: map[string]string{"pod-hash": "67890"},
+				},
+			},
+			expectedAction:       "update",
+			expectedSelector:     podLabels,
+			expectedOwnerRefs:    expectedOwnerReferences,
+			initialClientObjects: []runtime.Object{pod, configMap},
+		},
+		{
+			name: "Update service if ownerReferences differ",
+			existingService: &api_v1.Service{
+				ObjectMeta: meta_v1.ObjectMeta{
+					Name:      svcName,
+					Namespace: controllerNamespace,
+					OwnerReferences: []meta_v1.OwnerReference{
+						{Name: "old-owner"},
+					},
+				},
+				Spec: api_v1.ServiceSpec{
+					Selector: podLabels,
+				},
+			},
+			expectedAction:       "update",
+			expectedSelector:     podLabels,
+			expectedOwnerRefs:    expectedOwnerReferences,
+			initialClientObjects: []runtime.Object{pod, configMap},
+		},
+		{
+			name: "Update service if both labels and ownerReferences differ",
+			existingService: &api_v1.Service{
+				ObjectMeta: meta_v1.ObjectMeta{
+					Name:      svcName,
+					Namespace: controllerNamespace,
+					OwnerReferences: []meta_v1.OwnerReference{
+						{Name: "old-owner"},
+					},
+				},
+				Spec: api_v1.ServiceSpec{
+					Selector: map[string]string{"old-label": "true"},
+				},
+			},
+			expectedAction:       "update",
+			expectedSelector:     podLabels,
+			expectedOwnerRefs:    expectedOwnerReferences,
+			initialClientObjects: []runtime.Object{pod, configMap},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			clientObjects := tc.initialClientObjects
+			if tc.existingService != nil {
+				clientObjects = append(clientObjects, tc.existingService)
+			}
+			clientset := fake.NewSimpleClientset(clientObjects...)
+
+			err := createHeadlessService(logger, clientset, controllerNamespace, svcName, configMapNamespacedName, pod)
+			assert.NoError(t, err)
+
+			service, err := clientset.CoreV1().Services(controllerNamespace).Get(context.Background(), svcName, meta_v1.GetOptions{})
+			assert.NoError(t, err, "Failed to get service after create/update")
+
+			if err == nil {
+				assert.Equal(t, tc.expectedSelector, service.Spec.Selector, "Service selector mismatch")
+				assert.Equal(t, tc.expectedOwnerRefs, service.OwnerReferences, "Service OwnerReferences mismatch")
+			}
+
+			actions := clientset.Actions()
+			var serviceCreated, serviceUpdated bool
+			for _, action := range actions {
+				if action.Matches("create", "services") {
+					serviceCreated = true
+				}
+				if action.Matches("update", "services") {
+					serviceUpdated = true
+				}
+			}
+
+			switch tc.expectedAction {
+			case "create":
+				assert.True(t, serviceCreated, "service to be created")
+				assert.False(t, serviceUpdated, "no service update when creation is expected")
+			case "update":
+				assert.True(t, serviceUpdated, "service to be updated")
+				assert.False(t, serviceCreated, "no service creation when update is expected")
+			case "none":
+				assert.False(t, serviceCreated, "no service creation when no action is expected")
+				assert.False(t, serviceUpdated, "no service update when no action is expected")
+			default:
+				t.Fatalf("Invalid expectedAction: %s", tc.expectedAction)
+			}
+		})
+	}
+}