diff --git a/pkg/operator/controller/ingress/load_balancer_service.go b/pkg/operator/controller/ingress/load_balancer_service.go index 57a9480355..04361230f3 100644 --- a/pkg/operator/controller/ingress/load_balancer_service.go +++ b/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) { diff --git a/pkg/operator/controller/ingress/load_balancer_service_test.go b/pkg/operator/controller/ingress/load_balancer_service_test.go index 6c901370cb..15f7541619 100644 --- a/pkg/operator/controller/ingress/load_balancer_service_test.go +++ b/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(¤t, &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