From f28b8fee7fd706444ba2b460a9a40f026f8625dc Mon Sep 17 00:00:00 2001 From: Antonio Ojea Date: Thu, 30 May 2024 22:49:35 +0000 Subject: [PATCH] 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) + } + }) + } +}