From a0ab4c5a166cc9b0260104d26e10998dc23645b9 Mon Sep 17 00:00:00 2001 From: Peter Brachwitz Date: Thu, 11 Apr 2024 17:31:18 +0200 Subject: [PATCH] Support loadBalancerClass in service reconciliation (#7706) From the k8s docs: // loadBalancerClass is the class of the load balancer implementation this Service belongs to. // If specified, the value of this field must be a label-style identifier, with an optional prefix, // e.g. "internal-vip" or "example.com/internal-vip". Unprefixed names are reserved for end-users. // This field can only be set when the Service type is 'LoadBalancer'. If not set, the default load // balancer implementation is used, today this is typically done through the cloud provider integration, // but should apply for any default implementation. If set, it is assumed that a load balancer // implementation is watching for Services with a matching class. Any default load balancer // implementation (e.g. cloud providers) should ignore Services that set this field. // This field can only be set when creating or updating a Service to type 'LoadBalancer'. // Once set, it can not be changed. This field will be wiped when a service is updated to a non 'LoadBalancer' type. AWS EKS uses this field on EKS clusters > 1.24 that have upgraded to the new AWS LoadBalancer Controller https://docs.aws.amazon.com/eks/latest/userguide/aws-load-balancer-controller.html Behaviour this PR should implement: default the loadBalancerClass from the server side values if no value is set in the ECK service spec recreate the service if the value set in the ECK service spec differes from the server side value allow resetting the value only if the type of the service is no longer LoadBalancer --- pkg/controller/common/service_control.go | 18 +++ pkg/controller/common/service_control_test.go | 115 ++++++++++++++++-- 2 files changed, 124 insertions(+), 9 deletions(-) diff --git a/pkg/controller/common/service_control.go b/pkg/controller/common/service_control.go index f98476b41a..551552b239 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 && !reflect.DeepEqual(expected.Spec.LoadBalancerClass, reconciled.Spec.LoadBalancerClass) { + return true + } + return false } @@ -139,6 +144,19 @@ func applyServerSideValues(expected, reconciled *corev1.Service) { if expected.Spec.InternalTrafficPolicy == nil { 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 a3f0a2af1e..6404017907 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,48 @@ func Test_needsDelete(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{ + 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 +300,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 { @@ -521,29 +564,83 @@ 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: ptr(corev1.ServiceInternalTrafficPolicyCluster), + InternalTrafficPolicy: pointer(corev1.ServiceInternalTrafficPolicyCluster), + ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyCluster, + AllocateLoadBalancerNodePorts: ptr.To(true), + }}, + }, + want: corev1.Service{Spec: corev1.ServiceSpec{ + InternalTrafficPolicy: pointer(corev1.ServiceInternalTrafficPolicyCluster), + ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyCluster, + AllocateLoadBalancerNodePorts: ptr.To(true), + }}, + }, + { + name: "Expected InternalTrafficPolicy/ExternalTrafficPolicy/AllocateLoadBalancerPorts are used if not empty", + args: args{ + expected: corev1.Service{Spec: corev1.ServiceSpec{ + InternalTrafficPolicy: pointer(corev1.ServiceInternalTrafficPolicyLocal), + ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, + AllocateLoadBalancerNodePorts: ptr.To(false), + }}, + reconciled: corev1.Service{Spec: corev1.ServiceSpec{ + InternalTrafficPolicy: pointer(corev1.ServiceInternalTrafficPolicyCluster), + ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyCluster, + AllocateLoadBalancerNodePorts: ptr.To(true), + }}, + }, + want: corev1.Service{Spec: corev1.ServiceSpec{ + InternalTrafficPolicy: pointer(corev1.ServiceInternalTrafficPolicyLocal), + ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, + AllocateLoadBalancerNodePorts: ptr.To(false), + }}, + }, + { + 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{ - InternalTrafficPolicy: ptr(corev1.ServiceInternalTrafficPolicyCluster), + Type: corev1.ServiceTypeLoadBalancer, + LoadBalancerClass: ptr.To("explicit.lb/class"), }}, }, { - name: "Expected InternalTrafficPolicy is used if not empty", + name: "Expected LoadBalancerClass can be reset if Type is no longer LoadBalancer", args: args{ expected: corev1.Service{Spec: corev1.ServiceSpec{ - InternalTrafficPolicy: ptr(corev1.ServiceInternalTrafficPolicyLocal), + 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, }}, }, }