Repository: linode/linode-cloud-controller-manager
Pull request #128 (2023-03-28 11:51:21)

Short circuit LB cleanup if no annotations present
### Rationale

If a service is not annotated, the two calls to new/old NBs will inevitably return the very same NB, so not NBs will get deleted in that method. We can thus short circuit this and not proceed to the API calls unless we have an annotation present within the service.

### 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 5617616..8d7436a 100644
--- a/cloud/linode/loadbalancers.go
+++ b/cloud/linode/loadbalancers.go
@@ -140,6 +140,12 @@ func (l *loadbalancers) getNodeBalancerByStatus(ctx context.Context, service *v1
 // LoadBalancer status; if they are different (because of an updated NodeBalancerID
 // annotation), the old one is deleted.
 func (l *loadbalancers) cleanupOldNodeBalancer(ctx context.Context, service *v1.Service) error {
+	// unless there's an annotation, we can never get a past and current NB to differ,
+	// because they're looked up the same way
+	if _, ok := getServiceAnnotation(service, annLinodeNodeBalancerID); !ok {
+		return nil
+	}
+
 	previousNB, err := l.getNodeBalancerByStatus(ctx, service)
 	switch err.(type) {
 	case nil:


Test patch:
diff --git a/cloud/linode/fake_linode_test.go b/cloud/linode/fake_linode_test.go
index f04aee3..cf3e7d4 100644
--- a/cloud/linode/fake_linode_test.go
+++ b/cloud/linode/fake_linode_test.go
@@ -42,6 +42,10 @@ func newFake(t *testing.T) *fakeAPI {
 	}
 }
 
+func (f *fakeAPI) ResetRequests() {
+	f.requests = make(map[fakeRequest]struct{})
+}
+
 func (f *fakeAPI) recordRequest(r *http.Request) {
 	bodyBytes, _ := ioutil.ReadAll(r.Body)
 	r.Body.Close()
diff --git a/cloud/linode/loadbalancers_test.go b/cloud/linode/loadbalancers_test.go
index 1166586..68c3645 100644
--- a/cloud/linode/loadbalancers_test.go
+++ b/cloud/linode/loadbalancers_test.go
@@ -166,6 +166,10 @@ func TestCCMLoadBalancers(t *testing.T) {
 			name: "makeLoadBalancerStatus",
 			f:    testMakeLoadBalancerStatus,
 		},
+		{
+			name: "Cleanup does not call the API unless Service annotated",
+			f:    testCleanupDoesntCall,
+		},
 	}
 
 	for _, tc := range testCases {
@@ -1444,6 +1448,54 @@ func testMakeLoadBalancerStatus(t *testing.T, client *linodego.Client, _ *fakeAP
 	}
 }
 
+func testCleanupDoesntCall(t *testing.T, client *linodego.Client, fakeAPI *fakeAPI) {
+	region := "us-west"
+	nb1, err := client.CreateNodeBalancer(context.TODO(), linodego.NodeBalancerCreateOptions{Region: region})
+	if err != nil {
+		t.Fatal(err)
+	}
+	nb2, err := client.CreateNodeBalancer(context.TODO(), linodego.NodeBalancerCreateOptions{Region: region})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	svc := &v1.Service{ObjectMeta: metav1.ObjectMeta{Name: "test"}}
+	svcAnn := &v1.Service{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:        "test",
+			Annotations: map[string]string{annLinodeNodeBalancerID: strconv.Itoa(nb2.ID)},
+		},
+	}
+	svc.Status.LoadBalancer = *makeLoadBalancerStatus(svc, nb1)
+	svcAnn.Status.LoadBalancer = *makeLoadBalancerStatus(svcAnn, nb1)
+	lb := &loadbalancers{client, region, nil}
+
+	fakeAPI.ResetRequests()
+	t.Run("non-annotated service shouldn't call the API during cleanup", func(t *testing.T) {
+		if err := lb.cleanupOldNodeBalancer(context.TODO(), svc); err != nil {
+			t.Fatal(err)
+		}
+		if len(fakeAPI.requests) != 0 {
+			t.Fatalf("unexpected API calls: %v", fakeAPI.requests)
+		}
+	})
+
+	fakeAPI.ResetRequests()
+	t.Run("annotated service calls the API to load said NB", func(t *testing.T) {
+		if err := lb.cleanupOldNodeBalancer(context.TODO(), svcAnn); err != nil {
+			t.Fatal(err)
+		}
+		expectedRequests := map[fakeRequest]struct{}{
+			{Path: "/nodebalancers", Body: "", Method: "GET"}:                            {},
+			{Path: fmt.Sprintf("/nodebalancers/%v", nb2.ID), Body: "", Method: "GET"}:    {},
+			{Path: fmt.Sprintf("/nodebalancers/%v", nb1.ID), Body: "", Method: "DELETE"}: {},
+		}
+		if !reflect.DeepEqual(fakeAPI.requests, expectedRequests) {
+			t.Fatalf("expected requests %#v, got %#v instead", expectedRequests, fakeAPI.requests)
+		}
+	})
+}
+
 func testGetNodeBalancerForServiceIDDoesNotExist(t *testing.T, client *linodego.Client, _ *fakeAPI) {
 	lb := &loadbalancers{client, "us-west", nil}
 	bogusNodeBalancerID := "123456"