OCPBUGS-13190: Avoid spurious updates for internalTrafficPolicy

assets/router/service-cloud.yaml

@@ -14,6 +14,7 @@ spec:
   # This also has the effect of marking LB pool targets as unhealthy when no
   # router pods are present on a node behind the service.
   externalTrafficPolicy: Local
+  internalTrafficPolicy: Cluster
   ports:
   - name: http
     protocol: TCP

assets/router/service-internal.yaml

@@ -5,6 +5,7 @@ apiVersion: v1
 # name, namespace and annotations are set at runtime.
 spec:
   type: ClusterIP
+  internalTrafficPolicy: Cluster
   ports:
   - name: http
     port: 80

pkg/operator/controller/ingress/internal_service.go

@@ -120,7 +120,12 @@ func internalServiceChanged(current, expected *corev1.Service) (bool, *corev1.Se
 		// Ignore fields that the API, other controllers, or user may
 		// have modified.
 		cmpopts.IgnoreFields(corev1.ServicePort{}, "NodePort"),
-		cmpopts.IgnoreFields(corev1.ServiceSpec{}, "ClusterIP", "ClusterIPs", "ExternalIPs", "HealthCheckNodePort"),
+		cmpopts.IgnoreFields(corev1.ServiceSpec{},
+			"ClusterIP", "ClusterIPs",
+			"ExternalIPs",
+			"HealthCheckNodePort",
+			"IPFamilies", "IPFamilyPolicy",
+		),
 		cmp.Comparer(cmpServiceAffinity),
 		cmpopts.EquateEmpty(),
 	}
@@ -132,6 +137,7 @@ func internalServiceChanged(current, expected *corev1.Service) (bool, *corev1.Se
 		cmpopts.IgnoreMapEntries(func(k, _ string) bool {
 			return !managedInternalServiceAnnotations.Has(k)
 		}),
+		cmpopts.EquateEmpty(),
 	}
 	if !cmp.Equal(current.Annotations, expected.Annotations, annotationCmpOpts...) {
 		changed = true

pkg/operator/controller/ingress/internal_service_test.go

@@ -33,6 +33,7 @@ func Test_desiredInternalIngressControllerService(t *testing.T) {
 	svc := desiredInternalIngressControllerService(ic, deploymentRef)
 
 	assert.Equal(t, "ClusterIP", string(svc.Spec.Type))
+	assert.Equal(t, "Cluster", string(*svc.Spec.InternalTrafficPolicy))
 	assert.Equal(t, map[string]string{
 		"service.alpha.openshift.io/serving-cert-secret-name": "router-metrics-certs-default",
 	}, svc.Annotations)
@@ -89,6 +90,14 @@ func Test_internalServiceChanged(t *testing.T) {
 			},
 			expect: false,
 		},
+		{
+			description: "if .spec.internalTrafficPolicy changes",
+			mutate: func(svc *corev1.Service) {
+				v := corev1.ServiceInternalTrafficPolicyLocal
+				svc.Spec.InternalTrafficPolicy = &v
+			},
+			expect: true,
+		},
 		{
 			description: "if the service.alpha.openshift.io/serving-cert-secret-name annotation changes",
 			mutate: func(svc *corev1.Service) {
@@ -157,6 +166,21 @@ func Test_internalServiceChanged(t *testing.T) {
 			},
 			expect: true,
 		},
+		{
+			description: "if .spec.ipFamilies is defaulted",
+			mutate: func(service *corev1.Service) {
+				service.Spec.IPFamilies = []corev1.IPFamily{corev1.IPv4Protocol}
+			},
+			expect: false,
+		},
+		{
+			description: "if .spec.ipFamilyPolicy is defaulted",
+			mutate: func(service *corev1.Service) {
+				v := corev1.IPFamilyPolicySingleStack
+				service.Spec.IPFamilyPolicy = &v
+			},
+			expect: false,
+		},
 	}
 
 	for _, tc := range testCases {
@@ -210,3 +234,32 @@ func Test_internalServiceChanged(t *testing.T) {
 		})
 	}
 }
+
+// TestInternalServiceChangedEmptyAnnotations verifies that a service with null
+// .metadata.annotations and a service with empty .metadata.annotations are
+// considered equal.
+func TestInternalServiceChangedEmptyAnnotations(t *testing.T) {
+	svc1 := corev1.Service{
+		ObjectMeta: metav1.ObjectMeta{
+			Annotations: nil,
+		},
+	}
+	svc2 := corev1.Service{
+		ObjectMeta: metav1.ObjectMeta{
+			Annotations: map[string]string{},
+		},
+	}
+	testCases := []struct {
+		description      string
+		current, desired *corev1.Service
+	}{
+		{"null to empty", &svc1, &svc2},
+		{"empty to null", &svc2, &svc1},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.description, func(t *testing.T) {
+			changed, _ := internalServiceChanged(tc.current, tc.desired)
+			assert.False(t, changed)
+		})
+	}
+}

pkg/operator/controller/ingress/load_balancer_service_test.go

@@ -6,6 +6,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/assert"
+
 	corev1 "k8s.io/api/core/v1"
 
 	configv1 "github.com/openshift/api/config/v1"
@@ -1236,3 +1238,32 @@ func TestUpdateLoadBalancerServiceSourceRanges(t *testing.T) {
 		})
 	}
 }
+
+// TestLoadBalancerServiceChangedEmptyAnnotations verifies that a service with null
+// .metadata.annotations and a service with empty .metadata.annotations are
+// considered equal.
+func TestLoadBalancerServiceChangedEmptyAnnotations(t *testing.T) {
+	svc1 := corev1.Service{
+		ObjectMeta: metav1.ObjectMeta{
+			Annotations: nil,
+		},
+	}
+	svc2 := corev1.Service{
+		ObjectMeta: metav1.ObjectMeta{
+			Annotations: map[string]string{},
+		},
+	}
+	testCases := []struct {
+		description      string
+		current, desired *corev1.Service
+	}{
+		{"null to empty", &svc1, &svc2},
+		{"empty to null", &svc2, &svc1},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.description, func(t *testing.T) {
+			changed, _ := loadBalancerServiceChanged(tc.current, tc.desired)
+			assert.False(t, changed)
+		})
+	}
+}

pkg/operator/controller/ingress/nodeport_service.go

@@ -98,6 +98,7 @@ func desiredNodePortService(ic *operatorv1.IngressController, deploymentRef meta
 	}
 
 	name := controller.NodePortServiceName(ic)
+	internalTrafficPolicyCluster := corev1.ServiceInternalTrafficPolicyCluster
 	service := &corev1.Service{
 		ObjectMeta: metav1.ObjectMeta{
 			Annotations: map[string]string{},
@@ -112,6 +113,7 @@ func desiredNodePortService(ic *operatorv1.IngressController, deploymentRef meta
 		},
 		Spec: corev1.ServiceSpec{
 			ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyTypeLocal,
+			InternalTrafficPolicy: &internalTrafficPolicyCluster,
 			Ports: []corev1.ServicePort{
 				{
 					Name:       "http",
@@ -195,7 +197,12 @@ func nodePortServiceChanged(current, expected *corev1.Service) (bool, *corev1.Se
 		// Ignore fields that the API, other controllers, or user may
 		// have modified.
 		cmpopts.IgnoreFields(corev1.ServicePort{}, "NodePort"),
-		cmpopts.IgnoreFields(corev1.ServiceSpec{}, "ClusterIP", "ClusterIPs", "ExternalIPs", "HealthCheckNodePort"),
+		cmpopts.IgnoreFields(corev1.ServiceSpec{},
+			"ClusterIP", "ClusterIPs",
+			"ExternalIPs",
+			"HealthCheckNodePort",
+			"IPFamilies", "IPFamilyPolicy",
+		),
 		cmp.Comparer(cmpServiceAffinity),
 		cmpopts.EquateEmpty(),
 	}
@@ -207,6 +214,7 @@ func nodePortServiceChanged(current, expected *corev1.Service) (bool, *corev1.Se
 		cmpopts.IgnoreMapEntries(func(k, _ string) bool {
 			return !managedNodePortServiceAnnotations.Has(k)
 		}),
+		cmpopts.EquateEmpty(),
 	}
 	if !cmp.Equal(current.Annotations, expected.Annotations, annotationCmpOpts...) {
 		changed = true

pkg/operator/controller/ingress/nodeport_service_test.go

@@ -4,6 +4,8 @@ import (
 	"reflect"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
+
 	operatorv1 "github.com/openshift/api/operator/v1"
 
 	corev1 "k8s.io/api/core/v1"
@@ -14,6 +16,7 @@ import (
 
 func TestDesiredNodePortService(t *testing.T) {
 	trueVar := true
+	internalTrafficPolicyCluster := corev1.ServiceInternalTrafficPolicyCluster
 	deploymentRef := metav1.OwnerReference{
 		APIVersion: "apps/v1",
 		Kind:       "Deployment",
@@ -51,6 +54,7 @@ func TestDesiredNodePortService(t *testing.T) {
 				},
 				Spec: corev1.ServiceSpec{
 					ExternalTrafficPolicy: "Local",
+					InternalTrafficPolicy: &internalTrafficPolicyCluster,
 					Ports: []corev1.ServicePort{
 						{
 							Name:       "http",
@@ -92,6 +96,7 @@ func TestDesiredNodePortService(t *testing.T) {
 				},
 				Spec: corev1.ServiceSpec{
 					ExternalTrafficPolicy: "Local",
+					InternalTrafficPolicy: &internalTrafficPolicyCluster,
 					Ports: []corev1.ServicePort{
 						{
 							Name:       "http",
@@ -183,6 +188,14 @@ func TestNodePortServiceChanged(t *testing.T) {
 			},
 			expect: true,
 		},
+		{
+			description: "if .spec.internalTrafficPolicy changes",
+			mutate: func(svc *corev1.Service) {
+				v := corev1.ServiceInternalTrafficPolicyLocal
+				svc.Spec.InternalTrafficPolicy = &v
+			},
+			expect: true,
+		},
 		{
 			description: "if the local-with-fallback annotation changes",
 			mutate: func(svc *corev1.Service) {
@@ -253,6 +266,21 @@ func TestNodePortServiceChanged(t *testing.T) {
 			},
 			expect: true,
 		},
+		{
+			description: "if .spec.ipFamilies is defaulted",
+			mutate: func(service *corev1.Service) {
+				service.Spec.IPFamilies = []corev1.IPFamily{corev1.IPv4Protocol}
+			},
+			expect: false,
+		},
+		{
+			description: "if .spec.ipFamilyPolicy is defaulted",
+			mutate: func(service *corev1.Service) {
+				v := corev1.IPFamilyPolicySingleStack
+				service.Spec.IPFamilyPolicy = &v
+			},
+			expect: false,
+		},
 	}
 
 	for _, tc := range testCases {
@@ -309,3 +337,32 @@ func TestNodePortServiceChanged(t *testing.T) {
 		}
 	}
 }
+
+// TestNodePortServiceChangedEmptyAnnotations verifies that a service with null
+// .metadata.annotations and a service with empty .metadata.annotations are
+// considered equal.
+func TestNodePortServiceChangedEmptyAnnotations(t *testing.T) {
+	svc1 := corev1.Service{
+		ObjectMeta: metav1.ObjectMeta{
+			Annotations: nil,
+		},
+	}
+	svc2 := corev1.Service{
+		ObjectMeta: metav1.ObjectMeta{
+			Annotations: map[string]string{},
+		},
+	}
+	testCases := []struct {
+		description      string
+		current, desired *corev1.Service
+	}{
+		{"null to empty", &svc1, &svc2},
+		{"empty to null", &svc2, &svc1},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.description, func(t *testing.T) {
+			changed, _ := nodePortServiceChanged(tc.current, tc.desired)
+			assert.False(t, changed)
+		})
+	}
+}