Skip to content

Commit

Permalink
Support loadBalancerClass in service reconciliation (#7706)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@pebrc
pebrc authored Apr 11, 2024
1 parent 1d87557 commit a0ab4c5
Show file tree
Hide file tree
Showing 2 changed files with 124 additions and 9 deletions.
18 changes: 18 additions & 0 deletions pkg/controller/common/service_control.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -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.
Expand Down
115 changes: 106 additions & 9 deletions pkg/controller/common/service_control_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand All @@ -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 {
Expand Down Expand Up @@ -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,
}},
},
}
Expand Down

0 comments on commit a0ab4c5

Please sign in to comment.