From 7da7ae9ee114c66a177577a61c343b29d8ba6272 Mon Sep 17 00:00:00 2001 From: Ondrej Kokes Date: Wed, 8 Nov 2023 10:50:15 +0100 Subject: [PATCH 1/2] Do not ensure/update LBs if nodes are empty --- cloud/linode/loadbalancers.go | 10 ++++++ cloud/linode/loadbalancers_test.go | 54 ++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+) diff --git a/cloud/linode/loadbalancers.go b/cloud/linode/loadbalancers.go index b3e4cda2..a94ef46b 100644 --- a/cloud/linode/loadbalancers.go +++ b/cloud/linode/loadbalancers.go @@ -49,6 +49,10 @@ const ( annLinodeHostnameOnlyIngress = "service.beta.kubernetes.io/linode-loadbalancer-hostname-only-ingress" ) +var ( + errNoNodesAvailable = fmt.Errorf("no nodes available for nodebalancer") +) + type lbNotFoundError struct { serviceNn string nodeBalancerID int @@ -212,6 +216,9 @@ func (l *loadbalancers) GetLoadBalancer(ctx context.Context, clusterName string, // // EnsureLoadBalancer will not modify service or nodes. func (l *loadbalancers) EnsureLoadBalancer(ctx context.Context, clusterName string, service *v1.Service, nodes []*v1.Node) (lbStatus *v1.LoadBalancerStatus, err error) { + if len(nodes) == 0 { + return nil, fmt.Errorf("%w: cluster %s, service %s", errNoNodesAvailable, clusterName, getServiceNn(service)) + } ctx = sentry.SetHubOnContext(ctx) sentry.SetTag(ctx, "cluster_name", clusterName) sentry.SetTag(ctx, "service", service.Name) @@ -342,6 +349,9 @@ func (l *loadbalancers) updateNodeBalancer(ctx context.Context, service *v1.Serv // UpdateLoadBalancer updates the NodeBalancer to have configs that match the Service's ports func (l *loadbalancers) UpdateLoadBalancer(ctx context.Context, clusterName string, service *v1.Service, nodes []*v1.Node) (err error) { + if len(nodes) == 0 { + return fmt.Errorf("%w: cluster %s, service %s", errNoNodesAvailable, clusterName, getServiceNn(service)) + } ctx = sentry.SetHubOnContext(ctx) sentry.SetTag(ctx, "cluster_name", clusterName) sentry.SetTag(ctx, "service", service.Name) diff --git a/cloud/linode/loadbalancers_test.go b/cloud/linode/loadbalancers_test.go index f4ad2c6f..e9eb65e7 100644 --- a/cloud/linode/loadbalancers_test.go +++ b/cloud/linode/loadbalancers_test.go @@ -2,6 +2,7 @@ package linode import ( "context" + stderrors "errors" "fmt" "net/http" "net/http/httptest" @@ -175,6 +176,10 @@ func TestCCMLoadBalancers(t *testing.T) { name: "Cleanup does not call the API unless Service annotated", f: testCleanupDoesntCall, }, + { + name: "Update Load Balancer - No Nodes", + f: testUpdateLoadBalancerNoNodes, + }, } for _, tc := range testCases { @@ -1550,6 +1555,55 @@ func testCleanupDoesntCall(t *testing.T, client *linodego.Client, fakeAPI *fakeA }) } +func testUpdateLoadBalancerNoNodes(t *testing.T, client *linodego.Client, _ *fakeAPI) { + svc := &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: randString(10), + UID: "foobar123", + Annotations: map[string]string{}, + }, + Spec: v1.ServiceSpec{ + Ports: []v1.ServicePort{ + { + Name: randString(10), + Protocol: "http", + Port: int32(80), + NodePort: int32(8080), + }, + }, + }, + } + + lb := &loadbalancers{client, "us-west", nil} + defer lb.EnsureLoadBalancerDeleted(context.TODO(), "linodelb", svc) + + fakeClientset := fake.NewSimpleClientset() + lb.kubeClient = fakeClientset + + nodeBalancer, err := client.CreateNodeBalancer(context.TODO(), linodego.NodeBalancerCreateOptions{ + Region: lb.zone, + }) + if err != nil { + t.Fatalf("failed to create NodeBalancer: %s", err) + } + svc.Status.LoadBalancer = *makeLoadBalancerStatus(svc, nodeBalancer) + stubService(fakeClientset, svc) + svc.ObjectMeta.SetAnnotations(map[string]string{ + annLinodeNodeBalancerID: strconv.Itoa(nodeBalancer.ID), + }) + + // setup done, test ensure/update + nodes := []*v1.Node{} + + if _, err = lb.EnsureLoadBalancer(context.TODO(), "linodelb", svc, nodes); !stderrors.Is(err, errNoNodesAvailable) { + t.Errorf("EnsureLoadBalancer should return %v, got %v", errNoNodesAvailable, err) + } + + if err := lb.UpdateLoadBalancer(context.TODO(), "linodelb", svc, nodes); !stderrors.Is(err, errNoNodesAvailable) { + t.Errorf("UpdateLoadBalancer should return %v, got %v", errNoNodesAvailable, err) + } +} + func testGetNodeBalancerForServiceIDDoesNotExist(t *testing.T, client *linodego.Client, _ *fakeAPI) { lb := &loadbalancers{client, "us-west", nil} bogusNodeBalancerID := "123456" From 40b79e3ea9671c8fa6cdec71cd9582014c85ffcf Mon Sep 17 00:00:00 2001 From: Ondrej Kokes Date: Wed, 8 Nov 2023 11:33:34 +0100 Subject: [PATCH 2/2] move node handling into unexported methods --- cloud/linode/loadbalancers.go | 16 +++++++++------- cloud/linode/loadbalancers_test.go | 4 +++- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/cloud/linode/loadbalancers.go b/cloud/linode/loadbalancers.go index a94ef46b..28f3505a 100644 --- a/cloud/linode/loadbalancers.go +++ b/cloud/linode/loadbalancers.go @@ -3,6 +3,7 @@ package linode import ( "context" "encoding/json" + "errors" "fmt" "net/http" "os" @@ -50,7 +51,7 @@ const ( ) var ( - errNoNodesAvailable = fmt.Errorf("no nodes available for nodebalancer") + errNoNodesAvailable = errors.New("no nodes available for nodebalancer") ) type lbNotFoundError struct { @@ -216,9 +217,6 @@ func (l *loadbalancers) GetLoadBalancer(ctx context.Context, clusterName string, // // EnsureLoadBalancer will not modify service or nodes. func (l *loadbalancers) EnsureLoadBalancer(ctx context.Context, clusterName string, service *v1.Service, nodes []*v1.Node) (lbStatus *v1.LoadBalancerStatus, err error) { - if len(nodes) == 0 { - return nil, fmt.Errorf("%w: cluster %s, service %s", errNoNodesAvailable, clusterName, getServiceNn(service)) - } ctx = sentry.SetHubOnContext(ctx) sentry.SetTag(ctx, "cluster_name", clusterName) sentry.SetTag(ctx, "service", service.Name) @@ -261,6 +259,10 @@ func (l *loadbalancers) EnsureLoadBalancer(ctx context.Context, clusterName stri //nolint:funlen func (l *loadbalancers) updateNodeBalancer(ctx context.Context, service *v1.Service, nodes []*v1.Node, nb *linodego.NodeBalancer) (err error) { + if len(nodes) == 0 { + return fmt.Errorf("%w: service %s", errNoNodesAvailable, getServiceNn(service)) + } + connThrottle := getConnectionThrottle(service) if connThrottle != nb.ClientConnThrottle { update := nb.GetUpdateOptions() @@ -349,9 +351,6 @@ func (l *loadbalancers) updateNodeBalancer(ctx context.Context, service *v1.Serv // UpdateLoadBalancer updates the NodeBalancer to have configs that match the Service's ports func (l *loadbalancers) UpdateLoadBalancer(ctx context.Context, clusterName string, service *v1.Service, nodes []*v1.Node) (err error) { - if len(nodes) == 0 { - return fmt.Errorf("%w: cluster %s, service %s", errNoNodesAvailable, clusterName, getServiceNn(service)) - } ctx = sentry.SetHubOnContext(ctx) sentry.SetTag(ctx, "cluster_name", clusterName) sentry.SetTag(ctx, "service", service.Name) @@ -594,6 +593,9 @@ func (l *loadbalancers) addTLSCert(ctx context.Context, service *v1.Service, nbC // buildLoadBalancerRequest returns a linodego.NodeBalancer // requests for service across nodes. func (l *loadbalancers) buildLoadBalancerRequest(ctx context.Context, clusterName string, service *v1.Service, nodes []*v1.Node) (*linodego.NodeBalancer, error) { + if len(nodes) == 0 { + return nil, fmt.Errorf("%w: cluster %s, service %s", errNoNodesAvailable, clusterName, getServiceNn(service)) + } ports := service.Spec.Ports configs := make([]*linodego.NodeBalancerConfigCreateOptions, 0, len(ports)) diff --git a/cloud/linode/loadbalancers_test.go b/cloud/linode/loadbalancers_test.go index e9eb65e7..199ce6d0 100644 --- a/cloud/linode/loadbalancers_test.go +++ b/cloud/linode/loadbalancers_test.go @@ -228,7 +228,9 @@ func testCreateNodeBalancer(t *testing.T, client *linodego.Client, _ *fakeAPI) { } lb := &loadbalancers{client, "us-west", nil} - var nodes []*v1.Node + nodes := []*v1.Node{ + {ObjectMeta: metav1.ObjectMeta{Name: "node-1"}}, + } nb, err := lb.buildLoadBalancerRequest(context.TODO(), "linodelb", svc, nodes) if err != nil { t.Fatal(err)