Repository: linode/linode-cloud-controller-manager
Pull request #139 (2023-11-08 10:21:34)

Do not ensure/update LBs if nodes are empty
### PR description

Should our list of `nodes` be empty, we shouldn't reconfigure our NBs, because we can't call the Linode endpoint with an empty list of nodes.

### General:

* [x] Have you removed all sensitive information, including but not limited to access keys and passwords?
* [x] Have you checked to ensure there aren't other open or closed [Pull Requests](../../pulls) for the same bug/feature/question?

### Pull Request Guidelines:

1. [x] Does your submission pass tests?
1. [x] Have you added tests? 
1. [x] Are you addressing a single feature in this PR? 
1. [x] Are your commits atomic, addressing one change per commit?
1. [x] Are you following the conventions of the language? 
1. [x] Have you saved your large formatting changes for a different PR, so we can focus on your work?
1. [x] Have you explained your rationale for why this feature is needed? 
1. [ ] Have you linked your PR to an [open issue](https://blog.github.com/2013-05-14-closing-issues-via-pull-requests/)




Patch:
diff --git a/cloud/linode/loadbalancers.go b/cloud/linode/loadbalancers.go
index b3e4cda..28f3505 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"
@@ -49,6 +50,10 @@ const (
 	annLinodeHostnameOnlyIngress = "service.beta.kubernetes.io/linode-loadbalancer-hostname-only-ingress"
 )
 
+var (
+	errNoNodesAvailable = errors.New("no nodes available for nodebalancer")
+)
+
 type lbNotFoundError struct {
 	serviceNn      string
 	nodeBalancerID int
@@ -254,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()
@@ -584,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))
 


Test patch:
diff --git a/cloud/linode/loadbalancers_test.go b/cloud/linode/loadbalancers_test.go
index f4ad2c6..199ce6d 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 {
@@ -223,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)
@@ -1550,6 +1557,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"