Maintain HeadlessService on upgrade

cmd/nginx-ingress/main.go

@@ -11,6 +11,7 @@
 "os"
 "os/signal"
 "path/filepath"
+"reflect"
 "regexp"
 "runtime"
 "strings"
@@ -1089,7 +1090,7 @@
 }
 }
 
-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 @@
 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 @@
 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)
+}
+})
+}
+}