From 84a1faf8d7b908108d38ca2a68902a8870d8d66e Mon Sep 17 00:00:00 2001 From: Peter Brachwitz Date: Wed, 10 Apr 2024 10:42:57 +0200 Subject: [PATCH 1/4] Support loadBalancerClass in service reconciliation --- pkg/controller/common/service_control.go | 10 +++ pkg/controller/common/service_control_test.go | 87 +++++++++++++++++-- 2 files changed, 90 insertions(+), 7 deletions(-) diff --git a/pkg/controller/common/service_control.go b/pkg/controller/common/service_control.go index f98476b41a..f9a893c0c3 100644 --- a/pkg/controller/common/service_control.go +++ b/pkg/controller/common/service_control.go @@ -71,6 +71,11 @@ func needsRecreate(expected, reconciled *corev1.Service) bool { return true } + // LoadBalancerClass is immutable once set if target type is load balancer + if expected.Spec.Type == corev1.ServiceTypeLoadBalancer && expected.Spec.LoadBalancerClass != reconciled.Spec.LoadBalancerClass { + return true + } + return false } @@ -139,6 +144,11 @@ func applyServerSideValues(expected, reconciled *corev1.Service) { if expected.Spec.InternalTrafficPolicy == nil { expected.Spec.InternalTrafficPolicy = reconciled.Spec.InternalTrafficPolicy } + + // LoadBalancerClass may be defaulted by the API server starting K8s v.1.24 + if expected.Spec.Type == corev1.ServiceTypeLoadBalancer && expected.Spec.LoadBalancerClass == nil { + expected.Spec.LoadBalancerClass = reconciled.Spec.LoadBalancerClass + } } // hasNodePort returns for a given service type, if the service ports have a NodePort or not. diff --git a/pkg/controller/common/service_control_test.go b/pkg/controller/common/service_control_test.go index a3f0a2af1e..8e5301f0f4 100644 --- a/pkg/controller/common/service_control_test.go +++ b/pkg/controller/common/service_control_test.go @@ -12,6 +12,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/utils/ptr" kbv1 "github.com/elastic/cloud-on-k8s/v2/pkg/apis/kibana/v1" "github.com/elastic/cloud-on-k8s/v2/pkg/controller/common/comparison" @@ -154,7 +155,7 @@ func Test_needsUpdate(t *testing.T) { } } -func Test_needsDelete(t *testing.T) { +func Test_needsRecreate(t *testing.T) { type args struct { expected corev1.Service reconciled corev1.Service @@ -246,6 +247,34 @@ func Test_needsDelete(t *testing.T) { }, want: false, }, + { + name: "Needs recreate if LoadBalancerClass is changed", + args: args{ + expected: corev1.Service{Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + LoadBalancerClass: ptr.To("my-customer/lb"), + }}, + reconciled: corev1.Service{Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + LoadBalancerClass: ptr.To("something/else"), + }}, + }, + want: true, + }, + { + name: "Removing the load balancer class is OK if target type is no longer LoadBalancer", + args: args{ + expected: corev1.Service{Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeClusterIP, + LoadBalancerClass: nil, + }}, + reconciled: corev1.Service{Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + LoadBalancerClass: ptr.To("something/else"), + }}, + }, + want: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -257,7 +286,7 @@ func Test_needsDelete(t *testing.T) { } func Test_applyServerSideValues(t *testing.T) { - ptr := func(policyType corev1.ServiceInternalTrafficPolicyType) *corev1.ServiceInternalTrafficPolicyType { + pointer := func(policyType corev1.ServiceInternalTrafficPolicyType) *corev1.ServiceInternalTrafficPolicyType { return &policyType } type args struct { @@ -525,25 +554,69 @@ func Test_applyServerSideValues(t *testing.T) { args: args{ expected: corev1.Service{Spec: corev1.ServiceSpec{}}, reconciled: corev1.Service{Spec: corev1.ServiceSpec{ - InternalTrafficPolicy: ptr(corev1.ServiceInternalTrafficPolicyCluster), + InternalTrafficPolicy: pointer(corev1.ServiceInternalTrafficPolicyCluster), }}, }, want: corev1.Service{Spec: corev1.ServiceSpec{ - InternalTrafficPolicy: ptr(corev1.ServiceInternalTrafficPolicyCluster), + InternalTrafficPolicy: pointer(corev1.ServiceInternalTrafficPolicyCluster), }}, }, { name: "Expected InternalTrafficPolicy is used if not empty", args: args{ expected: corev1.Service{Spec: corev1.ServiceSpec{ - InternalTrafficPolicy: ptr(corev1.ServiceInternalTrafficPolicyLocal), + InternalTrafficPolicy: pointer(corev1.ServiceInternalTrafficPolicyLocal), + }}, + reconciled: corev1.Service{Spec: corev1.ServiceSpec{ + InternalTrafficPolicy: pointer(corev1.ServiceInternalTrafficPolicyCluster), + }}, + }, + want: corev1.Service{Spec: corev1.ServiceSpec{ + InternalTrafficPolicy: pointer(corev1.ServiceInternalTrafficPolicyLocal), + }}, + }, + { + name: "Reconciled LoadBalancerClass is used if the expected one is empty", + args: args{ + expected: corev1.Service{Spec: corev1.ServiceSpec{}}, + reconciled: corev1.Service{Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + LoadBalancerClass: ptr.To("service.k8s.aws/nlb"), + }}, + }, + want: corev1.Service{Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + LoadBalancerClass: ptr.To("service.k8s.aws/nlb"), + }}, + }, + { + name: "Expected LoadBalancerClass is used if not empty", + args: args{ + expected: corev1.Service{Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + LoadBalancerClass: ptr.To("explicit.lb/class"), + }}, + reconciled: corev1.Service{Spec: corev1.ServiceSpec{}}, + }, + want: corev1.Service{Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + LoadBalancerClass: ptr.To("explicit.lb/class"), + }}, + }, + { + name: "Expected LoadBalancerClass can be reset if Type is no longer LoadBalancer", + args: args{ + expected: corev1.Service{Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeClusterIP, + LoadBalancerClass: nil, }}, reconciled: corev1.Service{Spec: corev1.ServiceSpec{ - InternalTrafficPolicy: ptr(corev1.ServiceInternalTrafficPolicyCluster), + Type: corev1.ServiceTypeLoadBalancer, + LoadBalancerClass: ptr.To("service.k8s.aws/nlb"), }}, }, want: corev1.Service{Spec: corev1.ServiceSpec{ - InternalTrafficPolicy: ptr(corev1.ServiceInternalTrafficPolicyLocal), + Type: corev1.ServiceTypeClusterIP, }}, }, } From 749e87814546295d934a1bd8c364bc3058d26f74 Mon Sep 17 00:00:00 2001 From: Peter Brachwitz Date: Thu, 11 Apr 2024 14:31:47 +0200 Subject: [PATCH 2/4] Review feedback: corrected pointer comparison --- pkg/controller/common/service_control.go | 2 +- pkg/controller/common/service_control_test.go | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/pkg/controller/common/service_control.go b/pkg/controller/common/service_control.go index f9a893c0c3..011eb450a6 100644 --- a/pkg/controller/common/service_control.go +++ b/pkg/controller/common/service_control.go @@ -72,7 +72,7 @@ func needsRecreate(expected, reconciled *corev1.Service) bool { } // LoadBalancerClass is immutable once set if target type is load balancer - if expected.Spec.Type == corev1.ServiceTypeLoadBalancer && expected.Spec.LoadBalancerClass != reconciled.Spec.LoadBalancerClass { + if expected.Spec.Type == corev1.ServiceTypeLoadBalancer && !reflect.DeepEqual(expected.Spec.LoadBalancerClass, reconciled.Spec.LoadBalancerClass) { return true } diff --git a/pkg/controller/common/service_control_test.go b/pkg/controller/common/service_control_test.go index 8e5301f0f4..b6aef467f4 100644 --- a/pkg/controller/common/service_control_test.go +++ b/pkg/controller/common/service_control_test.go @@ -247,6 +247,20 @@ func Test_needsRecreate(t *testing.T) { }, want: false, }, + { + name: "No need to recreate if LoadBalancerClass has not changed", + args: args{ + expected: corev1.Service{Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + LoadBalancerClass: ptr.To("my-customer/lb"), + }}, + reconciled: corev1.Service{Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + LoadBalancerClass: ptr.To("my-customer/lb"), + }}, + }, + want: false, + }, { name: "Needs recreate if LoadBalancerClass is changed", args: args{ From 46360e96324974ebfea6cfd528722c057b5f6464 Mon Sep 17 00:00:00 2001 From: Peter Brachwitz Date: Thu, 11 Apr 2024 15:39:54 +0200 Subject: [PATCH 3/4] Review feedback: add additional server-side values missing --- pkg/controller/common/service_control.go | 9 +++++++ pkg/controller/common/service_control_test.go | 24 +++++++++++++------ 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/pkg/controller/common/service_control.go b/pkg/controller/common/service_control.go index 011eb450a6..8c99a2578f 100644 --- a/pkg/controller/common/service_control.go +++ b/pkg/controller/common/service_control.go @@ -145,10 +145,19 @@ func applyServerSideValues(expected, reconciled *corev1.Service) { expected.Spec.InternalTrafficPolicy = reconciled.Spec.InternalTrafficPolicy } + if expected.Spec.ExternalTrafficPolicy == "" { + expected.Spec.ExternalTrafficPolicy = reconciled.Spec.ExternalTrafficPolicy + } + // LoadBalancerClass may be defaulted by the API server starting K8s v.1.24 if expected.Spec.Type == corev1.ServiceTypeLoadBalancer && expected.Spec.LoadBalancerClass == nil { expected.Spec.LoadBalancerClass = reconciled.Spec.LoadBalancerClass } + + if expected.Spec.AllocateLoadBalancerNodePorts == nil { + expected.Spec.AllocateLoadBalancerNodePorts = reconciled.Spec.AllocateLoadBalancerNodePorts + } + } // hasNodePort returns for a given service type, if the service ports have a NodePort or not. diff --git a/pkg/controller/common/service_control_test.go b/pkg/controller/common/service_control_test.go index b6aef467f4..6404017907 100644 --- a/pkg/controller/common/service_control_test.go +++ b/pkg/controller/common/service_control_test.go @@ -564,29 +564,39 @@ func Test_applyServerSideValues(t *testing.T) { }}, }, { - name: "Reconciled InternalTrafficPolicy is used if the expected one is empty", + name: "Reconciled InternalTrafficPolicy/ExternalTrafficPolicy/AllocateLoadBalancerPorts are used if the expected one is empty", args: args{ expected: corev1.Service{Spec: corev1.ServiceSpec{}}, reconciled: corev1.Service{Spec: corev1.ServiceSpec{ - InternalTrafficPolicy: pointer(corev1.ServiceInternalTrafficPolicyCluster), + InternalTrafficPolicy: pointer(corev1.ServiceInternalTrafficPolicyCluster), + ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyCluster, + AllocateLoadBalancerNodePorts: ptr.To(true), }}, }, want: corev1.Service{Spec: corev1.ServiceSpec{ - InternalTrafficPolicy: pointer(corev1.ServiceInternalTrafficPolicyCluster), + InternalTrafficPolicy: pointer(corev1.ServiceInternalTrafficPolicyCluster), + ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyCluster, + AllocateLoadBalancerNodePorts: ptr.To(true), }}, }, { - name: "Expected InternalTrafficPolicy is used if not empty", + name: "Expected InternalTrafficPolicy/ExternalTrafficPolicy/AllocateLoadBalancerPorts are used if not empty", args: args{ expected: corev1.Service{Spec: corev1.ServiceSpec{ - InternalTrafficPolicy: pointer(corev1.ServiceInternalTrafficPolicyLocal), + InternalTrafficPolicy: pointer(corev1.ServiceInternalTrafficPolicyLocal), + ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, + AllocateLoadBalancerNodePorts: ptr.To(false), }}, reconciled: corev1.Service{Spec: corev1.ServiceSpec{ - InternalTrafficPolicy: pointer(corev1.ServiceInternalTrafficPolicyCluster), + InternalTrafficPolicy: pointer(corev1.ServiceInternalTrafficPolicyCluster), + ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyCluster, + AllocateLoadBalancerNodePorts: ptr.To(true), }}, }, want: corev1.Service{Spec: corev1.ServiceSpec{ - InternalTrafficPolicy: pointer(corev1.ServiceInternalTrafficPolicyLocal), + InternalTrafficPolicy: pointer(corev1.ServiceInternalTrafficPolicyLocal), + ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, + AllocateLoadBalancerNodePorts: ptr.To(false), }}, }, { From b5cb6a41f62c26b3e7fe98d486a9cce8f3a8bc34 Mon Sep 17 00:00:00 2001 From: Peter Brachwitz Date: Thu, 11 Apr 2024 16:03:54 +0200 Subject: [PATCH 4/4] remove trailing newline --- pkg/controller/common/service_control.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/controller/common/service_control.go b/pkg/controller/common/service_control.go index 8c99a2578f..551552b239 100644 --- a/pkg/controller/common/service_control.go +++ b/pkg/controller/common/service_control.go @@ -157,7 +157,6 @@ func applyServerSideValues(expected, reconciled *corev1.Service) { if expected.Spec.AllocateLoadBalancerNodePorts == nil { expected.Spec.AllocateLoadBalancerNodePorts = reconciled.Spec.AllocateLoadBalancerNodePorts } - } // hasNodePort returns for a given service type, if the service ports have a NodePort or not.