From f28b8fee7fd706444ba2b460a9a40f026f8625dc Mon Sep 17 00:00:00 2001 From: Antonio Ojea Date: Thu, 30 May 2024 22:49:35 +0000 Subject: [PATCH 1/2] fix loadbalancer status comparison The loadbalancer status has added new fields during the latest releases, but the helper function used by the service load balancer controller was not updated with all the new fields, and for the new IPMode field it was not taking into consideration that the field is a pointer. Instead of checking fields one by one use the DeepEqual function that provides semantic equality for these types. --- .../cloud-provider/service/helpers/helper.go | 28 +--- .../service/helpers/helper_test.go | 150 ++++++++++++++++++ 2 files changed, 152 insertions(+), 26 deletions(-) diff --git a/staging/src/k8s.io/cloud-provider/service/helpers/helper.go b/staging/src/k8s.io/cloud-provider/service/helpers/helper.go index fd436c3d37d..53fb92fe43b 100644 --- a/staging/src/k8s.io/cloud-provider/service/helpers/helper.go +++ b/staging/src/k8s.io/cloud-provider/service/helpers/helper.go @@ -23,6 +23,7 @@ import ( "strings" v1 "k8s.io/api/core/v1" + apiequality "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/strategicpatch" @@ -124,7 +125,7 @@ func HasLBFinalizer(service *v1.Service) bool { // LoadBalancerStatusEqual checks if load balancer status are equal func LoadBalancerStatusEqual(l, r *v1.LoadBalancerStatus) bool { - return ingressSliceEqual(l.Ingress, r.Ingress) + return apiequality.Semantic.DeepEqual(l.Ingress, r.Ingress) } // PatchService patches the given service's Status or ObjectMeta based on the original and @@ -160,28 +161,3 @@ func getPatchBytes(oldSvc, newSvc *v1.Service) ([]byte, error) { return patchBytes, nil } - -func ingressSliceEqual(lhs, rhs []v1.LoadBalancerIngress) bool { - if len(lhs) != len(rhs) { - return false - } - for i := range lhs { - if !ingressEqual(&lhs[i], &rhs[i]) { - return false - } - } - return true -} - -func ingressEqual(lhs, rhs *v1.LoadBalancerIngress) bool { - if lhs.IP != rhs.IP { - return false - } - if lhs.Hostname != rhs.Hostname { - return false - } - if lhs.IPMode != rhs.IPMode { - return false - } - return true -} diff --git a/staging/src/k8s.io/cloud-provider/service/helpers/helper_test.go b/staging/src/k8s.io/cloud-provider/service/helpers/helper_test.go index 58c0be8831b..2199aeba2f7 100644 --- a/staging/src/k8s.io/cloud-provider/service/helpers/helper_test.go +++ b/staging/src/k8s.io/cloud-provider/service/helpers/helper_test.go @@ -26,6 +26,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes/fake" utilnet "k8s.io/utils/net" + "k8s.io/utils/ptr" ) /* @@ -347,3 +348,152 @@ func Test_getPatchBytes(t *testing.T) { func addAnnotations(svc *v1.Service) { svc.Annotations["foo"] = "bar" } + +func TestLoadBalancerStatusEqual(t *testing.T) { + tests := []struct { + name string + l *v1.LoadBalancerStatus + r *v1.LoadBalancerStatus + want bool + }{ + { + name: "empty", + l: &v1.LoadBalancerStatus{}, + r: &v1.LoadBalancerStatus{}, + want: true, + }, + { + name: "same ip", + l: &v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + {IP: "192.168.0.1"}, + }, + }, + r: &v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + {IP: "192.168.0.1"}, + }, + }, + want: true, + }, + { + name: "different ip", + l: &v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + {IP: "192.168.0.2"}, + }, + }, + r: &v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + {IP: "192.168.0.1"}, + }, + }, + want: false, + }, + { + name: "same ipmode", + l: &v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + {IP: "192.168.0.1", IPMode: ptr.To(v1.LoadBalancerIPModeVIP)}, + }, + }, + r: &v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + {IP: "192.168.0.1", IPMode: ptr.To(v1.LoadBalancerIPModeVIP)}, + }, + }, + want: true, + }, + { + name: "only one ipmode set", + l: &v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + {IP: "192.168.0.1"}, + }, + }, + r: &v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + {IP: "192.168.0.1", IPMode: ptr.To(v1.LoadBalancerIPModeVIP)}, + }, + }, + want: false, + }, + { + name: "different ipmode", + l: &v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + {IP: "192.168.0.1", IPMode: ptr.To(v1.LoadBalancerIPModeProxy)}, + }, + }, + r: &v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + {IP: "192.168.0.1", IPMode: ptr.To(v1.LoadBalancerIPModeVIP)}, + }, + }, + want: false, + }, + { + name: "same ports", + l: &v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + {IP: "192.168.0.1", Ports: []v1.PortStatus{{Port: 80, Protocol: v1.ProtocolTCP}}}, + }, + }, + r: &v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + {IP: "192.168.0.1", Ports: []v1.PortStatus{{Port: 80, Protocol: v1.ProtocolTCP}}}, + }, + }, + want: true, + }, + { + name: "same ports different protocol", + l: &v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + {IP: "192.168.0.1", Ports: []v1.PortStatus{{Port: 80, Protocol: v1.ProtocolTCP}}}, + }, + }, + r: &v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + {IP: "192.168.0.1", Ports: []v1.PortStatus{{Port: 80, Protocol: v1.ProtocolUDP}}}, + }, + }, + want: false, + }, + { + name: "only one port", + l: &v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + {IP: "192.168.0.1", Ports: []v1.PortStatus{{Port: 80, Protocol: v1.ProtocolTCP}}}, + }, + }, + r: &v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + {IP: "192.168.0.1", Ports: []v1.PortStatus{}}, + }, + }, + want: false, + }, + { + name: "only one port", + l: &v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + {IP: "192.168.0.1", Ports: []v1.PortStatus{{Port: 80, Protocol: v1.ProtocolTCP}}}, + }, + }, + r: &v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + {IP: "192.168.0.1", Ports: []v1.PortStatus{}}, + }, + }, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := LoadBalancerStatusEqual(tt.l, tt.r); got != tt.want { + t.Errorf("LoadBalancerStatusEqual() = %v, want %v", got, tt.want) + } + }) + } +} From 59adf3f83318795c7eed7321646b196a953bd26e Mon Sep 17 00:00:00 2001 From: Antonio Ojea Date: Thu, 30 May 2024 23:03:49 +0000 Subject: [PATCH 2/2] remove unused function LoadBalancerStatusEqual It is duplicated in the cloud provider package, and is only used there for the service load balancer controller. --- pkg/apis/core/v1/helper/helpers.go | 29 --------- pkg/apis/core/v1/helper/helpers_test.go | 83 ------------------------- 2 files changed, 112 deletions(-) diff --git a/pkg/apis/core/v1/helper/helpers.go b/pkg/apis/core/v1/helper/helpers.go index 932e3ac6921..96accbd3f69 100644 --- a/pkg/apis/core/v1/helper/helpers.go +++ b/pkg/apis/core/v1/helper/helpers.go @@ -140,35 +140,6 @@ func IsServiceIPSet(service *v1.Service) bool { return service.Spec.ClusterIP != v1.ClusterIPNone && service.Spec.ClusterIP != "" } -// LoadBalancerStatusEqual evaluates the given load balancers' ingress IP addresses -// and hostnames and returns true if equal or false if otherwise -// TODO: make method on LoadBalancerStatus? -func LoadBalancerStatusEqual(l, r *v1.LoadBalancerStatus) bool { - return ingressSliceEqual(l.Ingress, r.Ingress) -} - -func ingressSliceEqual(lhs, rhs []v1.LoadBalancerIngress) bool { - if len(lhs) != len(rhs) { - return false - } - for i := range lhs { - if !ingressEqual(&lhs[i], &rhs[i]) { - return false - } - } - return true -} - -func ingressEqual(lhs, rhs *v1.LoadBalancerIngress) bool { - if lhs.IP != rhs.IP { - return false - } - if lhs.Hostname != rhs.Hostname { - return false - } - return true -} - // GetAccessModesAsString returns a string representation of an array of access modes. // modes, when present, are always in the same order: RWO,ROX,RWX,RWOP. func GetAccessModesAsString(modes []v1.PersistentVolumeAccessMode) string { diff --git a/pkg/apis/core/v1/helper/helpers_test.go b/pkg/apis/core/v1/helper/helpers_test.go index e691153e4e5..af9a4771fa9 100644 --- a/pkg/apis/core/v1/helper/helpers_test.go +++ b/pkg/apis/core/v1/helper/helpers_test.go @@ -723,86 +723,3 @@ func TestHugePageUnitSizeFromByteSize(t *testing.T) { } } } - -func TestLoadBalancerStatusEqual(t *testing.T) { - - testCases := []struct { - left *v1.LoadBalancerStatus - right *v1.LoadBalancerStatus - name string - expectVal bool - }{{ - name: "left equals right", - left: &v1.LoadBalancerStatus{ - Ingress: []v1.LoadBalancerIngress{{ - IP: "1.1.1.1", - Hostname: "host1", - }}, - }, - right: &v1.LoadBalancerStatus{ - Ingress: []v1.LoadBalancerIngress{{ - IP: "1.1.1.1", - Hostname: "host1", - }}, - }, - expectVal: true, - }, { - name: "length of LoadBalancerIngress slice is not equal", - left: &v1.LoadBalancerStatus{ - Ingress: []v1.LoadBalancerIngress{{ - IP: "1.1.1.1", - Hostname: "host1", - }, { - IP: "1.1.1.2", - Hostname: "host1", - }}, - }, - right: &v1.LoadBalancerStatus{ - Ingress: []v1.LoadBalancerIngress{{ - IP: "1.1.1.1", - Hostname: "host1", - }}, - }, - expectVal: false, - }, { - name: "LoadBalancerIngress ip is not equal", - left: &v1.LoadBalancerStatus{ - Ingress: []v1.LoadBalancerIngress{{ - IP: "1.1.1.2", - Hostname: "host1", - }}, - }, - right: &v1.LoadBalancerStatus{ - Ingress: []v1.LoadBalancerIngress{{ - IP: "1.1.1.1", - Hostname: "host1", - }}, - }, - expectVal: false, - }, { - name: "LoadBalancerIngress hostname is not equal", - left: &v1.LoadBalancerStatus{ - Ingress: []v1.LoadBalancerIngress{{ - IP: "1.1.1.1", - Hostname: "host2", - }}, - }, - right: &v1.LoadBalancerStatus{ - Ingress: []v1.LoadBalancerIngress{{ - IP: "1.1.1.1", - Hostname: "host1", - }}, - }, - expectVal: false, - }} - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - v := LoadBalancerStatusEqual(tc.left, tc.right) - if v != tc.expectVal { - t.Errorf("test %s failed. left input=%v, right input=%v, Got %v but expected %v", - tc.name, tc.left, tc.right, v, tc.expectVal) - } - }) - } -}