Bug 2097555: Fix loadBalancerServiceAnnotationsChanged check and update

pkg/operator/controller/ingress/load_balancer_service.go

@@ -604,12 +604,16 @@ func loadBalancerServiceChanged(current, expected *corev1.Service) (bool, *corev
 // loadBalancerServiceAnnotationsChanged checks if the annotations on the expected Service
 // match the ones on the current Service.
 func loadBalancerServiceAnnotationsChanged(current, expected *corev1.Service, annotations sets.String) (bool, *corev1.Service) {
-	annotationCmpOpts := []cmp.Option{
-		cmpopts.IgnoreMapEntries(func(k, _ string) bool {
-			return !annotations.Has(k)
-		}),
+	changed := false
+	for annotation := range annotations {
+		currentVal, have := current.Annotations[annotation]
+		expectedVal, want := expected.Annotations[annotation]
+		if (want && (!have || currentVal != expectedVal)) || (have && !want) {
+			changed = true
+			break
+		}
 	}
-	if cmp.Equal(current.Annotations, expected.Annotations, annotationCmpOpts...) {
+	if !changed {
 		return false, nil
 	}
 
@@ -619,7 +623,7 @@ func loadBalancerServiceAnnotationsChanged(current, expected *corev1.Service, an
 		updated.Annotations = map[string]string{}
 	}
 
-	for annotation := range managedLoadBalancerServiceAnnotations {
+	for annotation := range annotations {
 		currentVal, have := current.Annotations[annotation]
 		expectedVal, want := expected.Annotations[annotation]
 		if want && (!have || currentVal != expectedVal) {

pkg/operator/controller/ingress/load_balancer_service_test.go

@@ -14,6 +14,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/intstr"
+	"k8s.io/apimachinery/pkg/util/sets"
 )
 
 func TestDesiredLoadBalancerService(t *testing.T) {
@@ -902,6 +903,104 @@ func TestLoadBalancerServiceChanged(t *testing.T) {
 	}
 }
 
+// TestLoadBalancerServiceAnnotationsChanged verifies that
+// loadBalancerServiceAnnotationsChanged behaves correctly.
+func TestLoadBalancerServiceAnnotationsChanged(t *testing.T) {
+	testCases := []struct {
+		description         string
+		mutate              func(*corev1.Service)
+		currentAnnotations  map[string]string
+		expectedAnnotations map[string]string
+		managedAnnotations  sets.String
+		expect              bool
+	}{
+		{
+			description:         "if current and expected annotations are both empty",
+			currentAnnotations:  map[string]string{},
+			expectedAnnotations: map[string]string{},
+			managedAnnotations:  sets.NewString("foo"),
+			expect:              false,
+		},
+		{
+			description:         "if current annotations is nil and expected annotations is empty",
+			currentAnnotations:  nil,
+			expectedAnnotations: map[string]string{},
+			managedAnnotations:  sets.NewString("foo"),
+			expect:              false,
+		},
+		{
+			description:         "if current annotations is empty and expected annotations is nil",
+			currentAnnotations:  map[string]string{},
+			expectedAnnotations: nil,
+			managedAnnotations:  sets.NewString("foo"),
+			expect:              false,
+		},
+		{
+			description: "if an unmanaged annotation is updated",
+			currentAnnotations: map[string]string{
+				"foo": "bar",
+			},
+			expectedAnnotations: map[string]string{
+				"foo": "bar",
+				"baz": "quux",
+			},
+			managedAnnotations: sets.NewString("foo"),
+			expect:             false,
+		},
+		{
+			description:        "if a managed annotation is set",
+			currentAnnotations: map[string]string{},
+			expectedAnnotations: map[string]string{
+				"foo": "bar",
+			},
+			managedAnnotations: sets.NewString("foo"),
+			expect:             true,
+		},
+		{
+			description: "if a managed annotation is updated",
+			currentAnnotations: map[string]string{
+				"foo": "bar",
+			},
+			expectedAnnotations: map[string]string{
+				"foo": "baz",
+			},
+			managedAnnotations: sets.NewString("foo"),
+			expect:             true,
+		},
+		{
+			description: "if a managed annotation is deleted",
+			currentAnnotations: map[string]string{
+				"foo": "bar",
+			},
+			expectedAnnotations: map[string]string{},
+			managedAnnotations:  sets.NewString("foo"),
+			expect:              true,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.description, func(t *testing.T) {
+			current := corev1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: tc.currentAnnotations,
+				},
+			}
+			expected := corev1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: tc.expectedAnnotations,
+				},
+			}
+			if changed, updated := loadBalancerServiceAnnotationsChanged(&current, &expected, tc.managedAnnotations); changed != tc.expect {
+				t.Errorf("expected loadBalancerServiceAnnotationsChanged to be %t, got %t", tc.expect, changed)
+			} else if changed {
+				if changedAgain, _ := loadBalancerServiceAnnotationsChanged(&expected, updated, tc.managedAnnotations); changedAgain {
+					t.Error("loadBalancerServiceAnnotationsChanged does not behave as a fixed point function")
+				}
+			}
+		})
+	}
+}
+
 func TestServiceIngressOwner(t *testing.T) {
 	testCases := []struct {
 		description string