From 4cde74e0ce8d932e8497277131a3619aa8d11f0b Mon Sep 17 00:00:00 2001 From: Miciah Masters Date: Fri, 5 May 2023 18:04:20 -0400 Subject: [PATCH 1/2] Avoid spurious updates for internalTrafficPolicy Specify spec.internalTrafficPolicy on NodePort- and ClusterIP-type services that the operator manages. Also, ignore updates to the spec.ipFamilies and spec.ipFamilyPolicy fields. Before this commit, the update logic for NodePort- and ClusterIP-type services would try to revert the default values that the API set for these fields. This commit fixes OCPBUGS-13190. https://issues.redhat.com/browse/OCPBUGS-13190 * assets/router/service-cloud.yaml: * assets/router/service-internal.yaml: Specify "internalTrafficPolicy: Cluster". * pkg/manifests/bindata.go: Regenerate. * pkg/operator/controller/ingress/internal_service.go (internalServiceChanged): Ignore spec.ipFamilies and spec.ipFamilyPolicy. * pkg/operator/controller/ingress/internal_service_test.go (Test_desiredInternalIngressControllerService): Verify that spec.internalServiceChanged is set to "Cluster". (Test_internalServiceChanged): Verify that changes to spec.internalTrafficPolicy are detected and that changes to spec.ipFamilies and spec.ipFamilyPolicy are ignored. * pkg/operator/controller/ingress/nodeport_service.go (desiredNodePortService): Set spec.internalTrafficPolicy to "Cluster". (nodePortServiceChanged): Ignore spec.ipFamilies and spec.ipFamilyPolicy. * pkg/operator/controller/ingress/nodeport_service_test.go (TestDesiredNodePortService): Verify that spec.internalTrafficPolicy is set to "Cluster". (TestNodePortServiceChanged): Verify that changes to spec.internalTrafficPolicy are detected and that changes to spec.ipFamilies and spec.ipFamilyPolicy are ignored. --- assets/router/service-cloud.yaml | 1 + assets/router/service-internal.yaml | 1 + pkg/manifests/bindata.go | 16 ++++++------ .../controller/ingress/internal_service.go | 7 ++++- .../ingress/internal_service_test.go | 24 +++++++++++++++++ .../controller/ingress/nodeport_service.go | 9 ++++++- .../ingress/nodeport_service_test.go | 26 +++++++++++++++++++ 7 files changed, 74 insertions(+), 10 deletions(-) diff --git a/assets/router/service-cloud.yaml b/assets/router/service-cloud.yaml index ac030f5983..fa618a6170 100644 --- a/assets/router/service-cloud.yaml +++ b/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 diff --git a/assets/router/service-internal.yaml b/assets/router/service-internal.yaml index 675e9d274e..411de483a5 100644 --- a/assets/router/service-internal.yaml +++ b/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 diff --git a/pkg/manifests/bindata.go b/pkg/manifests/bindata.go index 7721736a27..1f4bc1b28f 100644 --- a/pkg/manifests/bindata.go +++ b/pkg/manifests/bindata.go @@ -13,8 +13,8 @@ // assets/router/metrics/role.yaml (291B) // assets/router/namespace.yaml (858B) // assets/router/service-account.yaml (213B) -// assets/router/service-cloud.yaml (631B) -// assets/router/service-internal.yaml (432B) +// assets/router/service-cloud.yaml (664B) +// assets/router/service-internal.yaml (465B) // manifests/00-cluster-role.yaml (3.205kB) // manifests/00-custom-resource-definition-internal.yaml (7.756kB) // manifests/00-custom-resource-definition.yaml (101.34kB) @@ -362,7 +362,7 @@ func assetsRouterServiceAccountYaml() (*asset, error) { return a, nil } -var _assetsRouterServiceCloudYaml = []byte("\x1f\x8b\x08\x00\x00\x00\x00\x00\x00\xff\x7c\x90\x41\x6b\x14\x41\x10\x85\xef\xfd\x2b\x1e\xec\x39\x41\x31\x07\x99\x63\x72\x12\x82\x2c\xb8\x78\xaf\xf4\xd4\xec\x34\xe9\xa9\x6a\xaa\x6a\x56\xf7\xdf\xcb\xf4\xec\x82\xa2\x78\xec\x07\xf5\xfa\x7b\xdf\x01\xaf\x4a\x23\x9e\xa9\x92\x64\x36\x7c\x63\xbb\x94\xcc\x08\x45\xab\x94\x19\x45\x30\x99\x4a\x40\x27\xc4\xcc\x30\x5d\x83\x6d\x8b\x73\xd5\x75\x04\xcb\xa5\x98\xca\xc2\x12\xfe\x98\x0e\xf8\x22\x67\x63\x77\xbc\xa8\x84\x69\xad\x6c\xf0\xc6\xb9\x4c\x25\xe3\x42\x75\x65\x07\x19\x83\x5a\xab\x85\x47\x50\xc0\x56\x89\xb2\xf0\x63\x7a\x2f\x32\x0e\x77\x82\x44\xad\x7c\x67\xf3\xa2\x32\xe0\xf2\x31\x2d\x1c\x34\x52\xd0\x90\x80\x03\xbe\xd2\xc2\x28\x0e\xe7\xf8\xa3\x02\x10\x5a\xd8\x1b\x65\x1e\xa0\x8d\xc5\xe7\x32\xc5\x43\xd9\xa1\x12\x50\xe9\x8d\xab\x6f\x25\xd8\x18\x86\xdb\x9e\xb4\x31\x6e\x69\x5c\x1b\x0f\xdd\xc9\x5d\x49\x02\x9c\x2b\xe7\x50\xfb\xfb\x6c\x63\x39\xcd\xc5\x41\xd5\x15\x33\x79\x77\xc4\xd3\xc4\xb9\x1b\x5b\xc8\xde\x8b\x9c\xf1\xfa\x8c\xa6\x5a\x11\x64\x67\x0e\x07\x39\x56\x99\x99\x6a\xcc\x57\xfc\x98\x59\x20\xda\xcb\x6e\x7a\x9b\x8e\xbb\xa7\x66\xec\xbc\xd9\x17\x10\x44\x47\xc6\x1b\xcf\x45\xc6\xfe\x8f\xef\xaa\xb6\xd9\xfc\x33\xd8\x84\xea\xc9\x68\x9a\x4a\x3e\x6a\x2d\xf9\xba\x0d\xc9\x54\x13\xd0\xd4\xa2\xaf\x7e\xe8\x82\x06\xcc\x11\xad\xaf\x69\xa6\xa1\x59\xeb\x80\xd3\xcb\x71\x4f\xd4\x62\xc0\xe7\x0f\xfd\xb1\x03\x1f\x7b\x74\xbb\xf9\xbd\xc2\xff\xdb\xf1\xf4\xf4\xe9\x9f\x25\x9e\x7e\x05\x00\x00\xff\xff\x56\xdc\x0d\xe9\x77\x02\x00\x00") +var _assetsRouterServiceCloudYaml = []byte("\x1f\x8b\x08\x00\x00\x00\x00\x00\x00\xff\x7c\x90\xc1\x6a\xdc\x4c\x10\x84\xef\xf3\x14\x05\x7b\xb6\xf9\x7f\xe2\x43\xd0\xd1\x7b\x0a\x98\xb0\x90\x25\xf7\xf6\xa8\xb5\x1a\x3c\xea\x1e\xba\x5b\x9b\xec\xdb\x07\x8d\xd6\x90\x10\x93\xa3\x4a\x74\x4d\x7d\xdf\x01\x2f\x4a\x23\x9e\xa9\x92\x64\x36\x7c\x63\xbb\x96\xcc\x08\x45\xab\x94\x19\x45\x30\x99\x4a\x40\x27\xc4\xcc\x30\x5d\x83\x6d\x8b\x73\xd5\x75\x04\xcb\xb5\x98\xca\xc2\x12\xfe\x98\x0e\xf8\x22\x17\x63\x77\x1c\x55\xc2\xb4\x56\x36\x78\xe3\x5c\xa6\x92\x71\xa5\xba\xb2\x83\x8c\x41\xad\xd5\xc2\x23\x28\x60\xab\x44\x59\xf8\x31\xbd\x15\x19\x87\xf7\x05\x89\x5a\xf9\xce\xe6\x45\x65\xc0\xf5\xff\xb4\x70\xd0\x48\x41\x43\x02\x0e\xf8\x4a\x0b\xa3\x38\x9c\xe3\x8f\x0a\x40\x68\x61\x6f\x94\x79\x80\x36\x16\x9f\xcb\x14\x0f\x65\x1f\x95\x80\x4a\xaf\x5c\x7d\x2b\xc1\xb6\x61\xb8\xf3\xa4\x6d\xe3\x96\xc6\xad\xf1\xd0\x9d\xbc\x2b\x49\x80\x73\xe5\x1c\x6a\x7f\x9f\x6d\x5b\xce\x73\x71\x50\x75\xc5\x4c\xde\x1d\xf1\x34\x71\xee\xc6\x16\xb2\xb7\x22\x17\xbc\x3c\xa3\xa9\x56\x04\xd9\x85\xc3\x41\x8e\x55\x66\xa6\x1a\xf3\x0d\x3f\x66\x16\x88\xf6\xb2\xbb\xde\xa6\xe3\xee\xa9\x19\x3b\x6f\xf6\x05\x04\xd1\x91\xf1\xca\x73\x91\xb1\xbf\xe3\xbb\xaa\x0d\x9b\x7f\x06\x9b\x50\x3d\x1b\x4d\x53\xc9\x27\xad\x25\xdf\x36\x90\x4c\x35\x01\x45\x3e\xfc\x7d\xac\xab\xef\x18\x4d\x2d\xba\x96\x87\x6e\x70\xc0\x1c\xd1\x3a\x6e\x33\x0d\xcd\x5a\x07\x9c\x8f\xa7\x3d\x51\x8b\x01\x9f\xff\xeb\x1f\x3b\xd1\xa9\x47\xf7\x9b\xdf\x2b\xfc\x9f\x1d\x4f\x4f\x9f\x3e\x2c\xf1\xf4\x2b\x00\x00\xff\xff\x92\x50\xdd\xb2\x98\x02\x00\x00") func assetsRouterServiceCloudYamlBytes() ([]byte, error) { return bindataRead( @@ -377,12 +377,12 @@ func assetsRouterServiceCloudYaml() (*asset, error) { return nil, err } - info := bindataFileInfo{name: "assets/router/service-cloud.yaml", size: 631, mode: os.FileMode(420), modTime: time.Unix(1, 0)} - a := &asset{bytes: bytes, info: info, digest: [32]uint8{0xcd, 0x97, 0xc4, 0xab, 0x8b, 0xa9, 0xa1, 0x47, 0xf7, 0xf, 0xeb, 0x38, 0x1b, 0xc2, 0xf7, 0x8c, 0xd9, 0xba, 0x35, 0x9b, 0x1, 0x67, 0x26, 0xd7, 0x3f, 0x6d, 0xa5, 0x5f, 0xf0, 0x92, 0x84, 0x8f}} + info := bindataFileInfo{name: "assets/router/service-cloud.yaml", size: 664, mode: os.FileMode(420), modTime: time.Unix(1, 0)} + a := &asset{bytes: bytes, info: info, digest: [32]uint8{0x31, 0x78, 0x7e, 0x75, 0x3, 0xab, 0x1, 0x5e, 0xc5, 0xb3, 0x4e, 0x7f, 0x4f, 0xd1, 0xf1, 0x90, 0xd5, 0x12, 0x57, 0xbc, 0xdf, 0x47, 0xed, 0xc2, 0xbe, 0x35, 0x9a, 0x75, 0x4c, 0xe5, 0xdb, 0xce}} return a, nil } -var _assetsRouterServiceInternalYaml = []byte("\x1f\x8b\x08\x00\x00\x00\x00\x00\x00\xff\x8c\xcf\x31\x6b\xfb\x30\x10\x05\xf0\x5d\x9f\xe2\x41\xd6\xff\xbf\x34\x24\x94\x56\x6b\xa6\x6c\x86\x96\xee\x87\x7c\x49\x8e\xca\x92\xb8\x3b\xbb\xf4\xdb\x97\x38\x35\xb8\x74\xc9\x22\x90\x4e\xef\xf7\xb8\x0d\x0e\x79\x34\x67\xc5\x2b\xeb\x24\x89\xf1\x29\x7e\x41\xcf\x27\x1a\xb3\x63\xa2\x3c\xb2\x85\x0d\x8e\xe5\xac\x6c\x86\x43\x2d\xae\x35\x67\x56\x58\xe3\x24\x27\x49\xa0\x52\xaa\x93\x4b\x2d\x06\x52\x06\xb5\x96\x85\x7b\x90\x43\xc7\xe2\x32\xf0\x43\xf8\x90\xd2\xc7\xa5\x23\x50\x93\x77\x56\x93\x5a\x22\xa6\x6d\xd8\xa0\xd0\xc0\xff\xe6\xd3\x1a\x25\x06\x95\xfe\x0f\x6b\xec\xbf\xc8\x6b\x7f\x0c\x80\x7f\x35\x8e\xcb\x1a\xc7\x2e\x00\xad\xaa\xdb\x75\xf4\x7f\x26\x23\x2e\xee\x2d\x00\xb7\x49\xc4\xf3\xe3\xed\xa2\xd5\x6b\xaa\x39\xe2\xed\xd0\xcd\x2f\x4e\x7a\x66\xef\xe6\x4f\x3f\x99\x35\x61\x2b\x63\xbf\xdf\xdd\x89\xd8\x4a\x19\xd8\x55\xd2\xda\xd9\xbe\xec\x9e\xee\x80\x96\xe0\x77\x00\x00\x00\xff\xff\x4c\x3f\xad\x47\xb0\x01\x00\x00") +var _assetsRouterServiceInternalYaml = []byte("\x1f\x8b\x08\x00\x00\x00\x00\x00\x00\xff\x8c\xcf\x41\x4b\x03\x31\x10\x05\xe0\x7b\x7e\xc5\x83\x5e\x55\x2c\x2d\xa2\xb9\xf6\xd4\xdb\x82\xc5\xfb\x90\x9d\x6d\x07\xd3\x24\xcc\xcc\x56\xfa\xef\xa5\x5b\x2b\x2b\x5e\x7a\x09\x24\x99\xf7\x3d\x66\x81\x4d\x1e\xcd\x59\xf1\xce\x7a\x92\xc4\xf8\x12\x3f\xa0\xe7\x81\xc6\xec\x38\x51\x1e\xd9\xc2\x02\xdb\xb2\x57\x36\xc3\xa6\x16\xd7\x9a\x33\x2b\xac\x71\x92\x41\x12\xa8\x94\xea\xe4\x52\x8b\x81\x94\x41\xad\x65\xe1\x1e\xe4\xd0\xb1\xb8\x1c\xf9\x29\x7c\x4a\xe9\xe3\xad\x23\x50\x93\x0f\x56\x93\x5a\x22\x4e\xcb\xb0\x40\xa1\x23\x3f\x4c\xa7\x35\x4a\x0c\x2a\xfd\x3f\xd6\xd8\xff\x90\x97\xfe\x18\x00\x3f\x37\x8e\xb7\x35\xb6\x5d\x00\xa4\x38\x6b\xa1\xbc\x53\x1a\x06\x49\x5d\xcd\x92\xce\xbf\x23\x01\x68\x55\xdd\x2e\xd9\xc7\xa9\x33\xe2\xe0\xde\x02\x70\xfd\x89\x78\x7d\xbe\x5e\xb4\x7a\x4d\x35\x47\xec\x36\xdd\xf4\xe2\xa4\x7b\xf6\x6e\x1a\xfa\xc9\xcc\x09\x9b\x19\xeb\xf5\xea\x4e\xc4\x66\xca\x91\x5d\x25\xcd\x9d\xe5\xdb\xea\xe5\x0e\xe8\x16\xfc\x0e\x00\x00\xff\xff\xc4\x03\x5b\xfe\xd1\x01\x00\x00") func assetsRouterServiceInternalYamlBytes() ([]byte, error) { return bindataRead( @@ -397,8 +397,8 @@ func assetsRouterServiceInternalYaml() (*asset, error) { return nil, err } - info := bindataFileInfo{name: "assets/router/service-internal.yaml", size: 432, mode: os.FileMode(420), modTime: time.Unix(1, 0)} - a := &asset{bytes: bytes, info: info, digest: [32]uint8{0x72, 0x14, 0xe2, 0x6f, 0x8f, 0xec, 0x74, 0xa4, 0xd0, 0x80, 0x68, 0x9f, 0x6e, 0xbf, 0x5e, 0x81, 0x88, 0xae, 0xfe, 0xf6, 0x98, 0x8a, 0xc3, 0x57, 0x9d, 0x68, 0x65, 0x25, 0x6f, 0x10, 0xa5, 0x97}} + info := bindataFileInfo{name: "assets/router/service-internal.yaml", size: 465, mode: os.FileMode(420), modTime: time.Unix(1, 0)} + a := &asset{bytes: bytes, info: info, digest: [32]uint8{0xdb, 0xec, 0xb8, 0x22, 0xb8, 0xbc, 0x71, 0x52, 0x6d, 0x67, 0x50, 0x74, 0x5f, 0x62, 0xd4, 0xc1, 0x54, 0x4e, 0x90, 0x91, 0x23, 0x5b, 0xd5, 0x67, 0x3d, 0x49, 0x26, 0xb3, 0xb2, 0x22, 0xf5, 0x2a}} return a, nil } diff --git a/pkg/operator/controller/ingress/internal_service.go b/pkg/operator/controller/ingress/internal_service.go index ee4b224f0d..0a3c726fad 100644 --- a/pkg/operator/controller/ingress/internal_service.go +++ b/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(), } diff --git a/pkg/operator/controller/ingress/internal_service_test.go b/pkg/operator/controller/ingress/internal_service_test.go index 8187b94815..41e901a726 100644 --- a/pkg/operator/controller/ingress/internal_service_test.go +++ b/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 { diff --git a/pkg/operator/controller/ingress/nodeport_service.go b/pkg/operator/controller/ingress/nodeport_service.go index 1897566f1c..9cfb8ed283 100644 --- a/pkg/operator/controller/ingress/nodeport_service.go +++ b/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(), } diff --git a/pkg/operator/controller/ingress/nodeport_service_test.go b/pkg/operator/controller/ingress/nodeport_service_test.go index b78a9d33c4..91d2b09f9b 100644 --- a/pkg/operator/controller/ingress/nodeport_service_test.go +++ b/pkg/operator/controller/ingress/nodeport_service_test.go @@ -14,6 +14,7 @@ import ( func TestDesiredNodePortService(t *testing.T) { trueVar := true + internalTrafficPolicyCluster := corev1.ServiceInternalTrafficPolicyCluster deploymentRef := metav1.OwnerReference{ APIVersion: "apps/v1", Kind: "Deployment", @@ -51,6 +52,7 @@ func TestDesiredNodePortService(t *testing.T) { }, Spec: corev1.ServiceSpec{ ExternalTrafficPolicy: "Local", + InternalTrafficPolicy: &internalTrafficPolicyCluster, Ports: []corev1.ServicePort{ { Name: "http", @@ -92,6 +94,7 @@ func TestDesiredNodePortService(t *testing.T) { }, Spec: corev1.ServiceSpec{ ExternalTrafficPolicy: "Local", + InternalTrafficPolicy: &internalTrafficPolicyCluster, Ports: []corev1.ServicePort{ { Name: "http", @@ -183,6 +186,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 +264,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 { From 701f646aba19171dacb55e3af9e78a70fa4df46c Mon Sep 17 00:00:00 2001 From: Miciah Masters Date: Mon, 8 May 2023 10:08:56 -0400 Subject: [PATCH 2/2] Ignore updates for null versus empty ennotations Ignore updates to annotations when the update is from null to empty or vice versa. * pkg/operator/controller/ingress/internal_service.go (internalServiceChanged): Use EquateEmpty when comparing annotations. * pkg/operator/controller/ingress/internal_service_test.go (TestInternalServiceChangedEmptyAnnotations): New test to verify that internalServiceChanged treats empty and null annotations as equal. * pkg/operator/controller/ingress/load_balancer_service_test.go (TestLoadBalancerServiceChangedEmptyAnnotations): New test to verify that loadBalancerServiceChanged treats empty and null annotations as equal. * pkg/operator/controller/ingress/nodeport_service.go (nodePortServiceChanged): Use EquateEmpty when comparing annotations. * pkg/operator/controller/ingress/nodeport_service_test.go (TestNodePortServiceChangedEmptyAnnotations): New test to verify that loadBalancerServiceChanged treats empty and null annotations as equal. --- .../controller/ingress/internal_service.go | 1 + .../ingress/internal_service_test.go | 29 +++++++++++++++++ .../ingress/load_balancer_service_test.go | 31 +++++++++++++++++++ .../controller/ingress/nodeport_service.go | 1 + .../ingress/nodeport_service_test.go | 31 +++++++++++++++++++ 5 files changed, 93 insertions(+) diff --git a/pkg/operator/controller/ingress/internal_service.go b/pkg/operator/controller/ingress/internal_service.go index 0a3c726fad..36a2f441f6 100644 --- a/pkg/operator/controller/ingress/internal_service.go +++ b/pkg/operator/controller/ingress/internal_service.go @@ -137,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 diff --git a/pkg/operator/controller/ingress/internal_service_test.go b/pkg/operator/controller/ingress/internal_service_test.go index 41e901a726..729bda5ece 100644 --- a/pkg/operator/controller/ingress/internal_service_test.go +++ b/pkg/operator/controller/ingress/internal_service_test.go @@ -234,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) + }) + } +} diff --git a/pkg/operator/controller/ingress/load_balancer_service_test.go b/pkg/operator/controller/ingress/load_balancer_service_test.go index 978f473118..156d026039 100644 --- a/pkg/operator/controller/ingress/load_balancer_service_test.go +++ b/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" @@ -1229,3 +1231,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) + }) + } +} diff --git a/pkg/operator/controller/ingress/nodeport_service.go b/pkg/operator/controller/ingress/nodeport_service.go index 9cfb8ed283..c503712200 100644 --- a/pkg/operator/controller/ingress/nodeport_service.go +++ b/pkg/operator/controller/ingress/nodeport_service.go @@ -214,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 diff --git a/pkg/operator/controller/ingress/nodeport_service_test.go b/pkg/operator/controller/ingress/nodeport_service_test.go index 91d2b09f9b..0ab6af467a 100644 --- a/pkg/operator/controller/ingress/nodeport_service_test.go +++ b/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" @@ -335,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) + }) + } +}