refactor: update mergeMap to initialize nil destination maps and return the merged map, adjusting its callers and tests.

Author: 08volt

providers/gce/gce_annotations.go

@@ -197,15 +197,26 @@ func GetLoadBalancerAnnotationSubnet(service *v1.Service) string {
 	return ""
 }
 
-// mergeMap merges the update map into the dst map.
-// Keys in dst are overwritten by values from update.
-// If a value in the update map is an empty string, the key is removed from dst.
-func mergeMap(dst, update map[string]string) {
+// mergeMap returns a new map containing the merged content of existing and update.
+// Keys in existing are overwritten by values from update.
+// If a value in the update map is an empty string, the key is removed from the returned map.
+// The existing map is not modified.
+func mergeMap(existing, update map[string]string) map[string]string {
+	if existing == nil && len(update) == 0 {
+		return nil
+	}
+	res := make(map[string]string)
+	for k, v := range existing {
+		if _, exists := update[k]; exists && update[k] != "" {
+			res[k] = update[k]
+		} else if !exists {
+			res[k] = v
+		}
+	}
 	for k, v := range update {
-		if v == "" {
-			delete(dst, k)
-		} else {
-			dst[k] = v
+		if _, exists := existing[k]; !exists && v != "" {
+			res[k] = v
 		}
 	}
+	return res
 }

providers/gce/gce_annotations_test.go

@@ -20,6 +20,8 @@ limitations under the License.
 package gce
 
 import (
+	"maps"
+	"reflect"
 	"testing"
 
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud"
@@ -119,10 +121,199 @@ func TestMergeMap(t *testing.T) {
 			updates:        map[string]string{},
 			expectedResult: map[string]string{"key1": "val1"},
 		},
+		{
+			desc:           "empty updates should result in no change",
+			existing:       nil,
+			updates:        map[string]string{},
+			expectedResult: nil,
+		},
+		{
+			desc:     "nil existing map should be initialized and updated",
+			existing: nil,
+			updates:  map[string]string{"key": "val"},
+			expectedResult: map[string]string{
+				"key": "val",
+			},
+		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
-			mergeMap(tc.existing, tc.updates)
-			assert.Equal(t, tc.expectedResult, tc.existing)
+			originalExisting := maps.Clone(tc.existing)
+
+			result := mergeMap(tc.existing, tc.updates)
+			assert.Equal(t, tc.expectedResult, result)
+
+			if tc.existing != nil {
+				assert.Equal(t, originalExisting, tc.existing, "Input map should not be modified")
+			}
+		})
+	}
+}
+
+func TestComputeNewAnnotationsIfNeeded(t *testing.T) {
+	testCases := []struct {
+		desc           string
+		svc            *v1.Service
+		newAnnotations map[string]string
+		expectUpdate   bool
+		expectedAnns   map[string]string
+	}{
+		{
+			desc: "nil annotations in svc, new annotations added",
+			svc: &v1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:        "test-svc",
+					Namespace:   "test-ns",
+					Annotations: nil,
+				},
+			},
+			newAnnotations: map[string]string{"key1": "val1"},
+			expectUpdate:   true,
+			expectedAnns:   map[string]string{"key1": "val1"},
+		},
+		{
+			desc: "empty annotations in svc, new annotations added",
+			svc: &v1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:        "test-svc",
+					Namespace:   "test-ns",
+					Annotations: map[string]string{},
+				},
+			},
+			newAnnotations: map[string]string{"key1": "val1"},
+			expectUpdate:   true,
+			expectedAnns:   map[string]string{"key1": "val1"},
+		},
+		{
+			desc: "existing annotations, new annotations merged",
+			svc: &v1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-svc",
+					Namespace: "test-ns",
+					Annotations: map[string]string{
+						"existing1": "old1",
+						"key1":      "oldVal1",
+					},
+				},
+			},
+			newAnnotations: map[string]string{"key1": "val1", "key2": "val2"},
+			expectUpdate:   true,
+			expectedAnns: map[string]string{
+				"existing1": "old1",
+				"key1":      "val1",
+				"key2":      "val2",
+			},
+		},
+		{
+			desc: "existing annotations, key removed",
+			svc: &v1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-svc",
+					Namespace: "test-ns",
+					Annotations: map[string]string{
+						"key1": "val1",
+						"key2": "val2",
+					},
+				},
+			},
+			newAnnotations: map[string]string{"key1": ""},
+			expectUpdate:   true,
+			expectedAnns:   map[string]string{"key2": "val2"},
+		},
+		{
+			desc: "no changes to annotations",
+			svc: &v1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-svc",
+					Namespace: "test-ns",
+					Annotations: map[string]string{
+						"key1": "val1",
+					},
+				},
+			},
+			newAnnotations: map[string]string{"key1": "val1"},
+			expectUpdate:   false,
+			expectedAnns:   nil,
+		},
+		{
+			desc: "nil newAnnotations",
+			svc: &v1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-svc",
+					Namespace: "test-ns",
+					Annotations: map[string]string{
+						"key1": "val1",
+					},
+				},
+			},
+			newAnnotations: nil,
+			expectUpdate:   false,
+			expectedAnns:   nil,
+		},
+		{
+			desc: "empty newAnnotations",
+			svc: &v1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-svc",
+					Namespace: "test-ns",
+					Annotations: map[string]string{
+						"key1": "val1",
+					},
+				},
+			},
+			newAnnotations: map[string]string{},
+			expectUpdate:   false,
+			expectedAnns:   nil,
+		},
+		{
+			desc: "nil annotations in svc, empty new annotations",
+			svc: &v1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:        "test-svc",
+					Namespace:   "test-ns",
+					Annotations: nil,
+				},
+			},
+			newAnnotations: map[string]string{},
+			expectUpdate:   false,
+			expectedAnns:   nil,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			// Capture original annotations to check for unintended modifications
+			originalAnnotations := make(map[string]string)
+			if tc.svc.Annotations != nil {
+				for k, v := range tc.svc.Annotations {
+					originalAnnotations[k] = v
+				}
+			}
+
+			newMeta, needsUpdate := computeNewAnnotationsIfNeeded(tc.svc, tc.newAnnotations)
+
+			assert.Equal(t, tc.expectUpdate, needsUpdate)
+
+			if tc.expectUpdate {
+				assert.NotNil(t, newMeta)
+				assert.True(t, reflect.DeepEqual(tc.expectedAnns, newMeta.Annotations), "Expected annotations: %v, got: %v", tc.expectedAnns, newMeta.Annotations)
+				// Ensure the map was actually changed from the original
+				assert.False(t, reflect.DeepEqual(originalAnnotations, newMeta.Annotations), "Annotations should have changed, but match original")
+			} else {
+				assert.Nil(t, newMeta)
+				// Ensure original service annotations are not modified
+				if len(originalAnnotations) == 0 && tc.svc.Annotations == nil {
+					// Special case: nil to nil is fine
+				} else {
+					assert.True(t, reflect.DeepEqual(originalAnnotations, tc.svc.Annotations), "Original svc.Annotations should not be modified on no update")
+				}
+			}
+
+			// Always check that the original service object's annotations map instance is not a different instance if no update was needed.
+			if !needsUpdate {
+				if len(originalAnnotations) > 0 || (len(originalAnnotations) == 0 && tc.svc.Annotations != nil) {
+					assert.True(t, reflect.DeepEqual(originalAnnotations, tc.svc.Annotations), "Original svc.Annotations instance should not change when no update is needed")
+				}
+			}
 		})
 	}
 }

providers/gce/gce_loadbalancer.go

@@ -405,7 +405,7 @@ func hasLoadBalancerPortsError(service *v1.Service) bool {
 // This function is used by L4 LB controllers.
 func computeNewAnnotationsIfNeeded(svc *v1.Service, newAnnotations map[string]string) (*metav1.ObjectMeta, bool) {
 	newObjectMeta := svc.ObjectMeta.DeepCopy()
-	mergeMap(newObjectMeta.Annotations, newAnnotations)
+	newObjectMeta.Annotations = mergeMap(newObjectMeta.Annotations, newAnnotations)
 	if reflect.DeepEqual(svc.Annotations, newObjectMeta.Annotations) {
 		return nil, false
 	}