Merge pull request #128 from okokes-akamai/cleanup_short_circuit

Short circuit LB cleanup if no annotations present

Author: okokes-akamai

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()

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:

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"