fix: remove firewall device by device ID not entity ID (#214)

* fix: remove firewall device by device ID not entity ID

* update tests

---------

Co-authored-by: Ryan Lonergan <[email protected]>

Author: rl0nergan

cloud/linode/firewall/firewalls.go

@@ -260,11 +260,17 @@ func (l *LinodeClient) UpdateNodeBalancerFirewall(
 		klog.Errorf("Found more than one firewall attached to nodebalancer: %d, firewall IDs: %v", nb.ID, firewalls)
 		return ErrTooManyNBFirewalls
 	}
-
-	err = l.Client.DeleteFirewallDevice(ctx, firewalls[0].ID, nb.ID)
+	deviceID, deviceExists, err := l.getNodeBalancerDeviceID(ctx, firewalls[0].ID, nb.ID)
 	if err != nil {
 		return err
 	}
+	if deviceExists {
+		err = l.Client.DeleteFirewallDevice(ctx, firewalls[0].ID, deviceID)
+		if err != nil {
+			return err
+		}
+	}
+
 	// once we delete the device, we should see if there's anything attached to that firewall
 	devices, err := l.Client.ListFirewallDevices(ctx, firewalls[0].ID, &linodego.ListOptions{})
 	if err != nil {

cloud/linode/loadbalancers_test.go

@@ -188,7 +188,11 @@ func TestCCMLoadBalancers(t *testing.T) {
 		},
 		{
 			name: "Update Load Balancer - Delete Firewall ID",
-			f:    testUpdateLoadBalancerDeleteFirewall,
+			f:    testUpdateLoadBalancerDeleteFirewallRemoveID,
+		},
+		{
+			name: "Update Load Balancer - Delete Firewall ACL",
+			f:    testUpdateLoadBalancerDeleteFirewallRemoveACL,
 		},
 		{
 			name: "Update Load Balancer - Update Firewall ACL",
@@ -1290,6 +1294,99 @@ func testUpdateLoadBalancerAddNewFirewallACL(t *testing.T, client *linodego.Clie
 	}
 }
 
+func testUpdateLoadBalancerDeleteFirewallRemoveACL(t *testing.T, client *linodego.Client, fakeAPI *fakeAPI) {
+	svc := &v1.Service{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: randString(),
+			UID:  "foobar123",
+		},
+		Spec: v1.ServiceSpec{
+			Ports: []v1.ServicePort{
+				{
+					Name:     randString(),
+					Protocol: "TCP",
+					Port:     int32(80),
+					NodePort: int32(30000),
+				},
+			},
+		},
+	}
+
+	nodes := []*v1.Node{
+		{
+			Status: v1.NodeStatus{
+				Addresses: []v1.NodeAddress{
+					{
+						Type:    v1.NodeInternalIP,
+						Address: "127.0.0.1",
+					},
+				},
+			},
+		},
+	}
+
+	lb := newLoadbalancers(client, "us-west").(*loadbalancers)
+	fakeClientset := fake.NewSimpleClientset()
+	lb.kubeClient = fakeClientset
+
+	svc.ObjectMeta.SetAnnotations(map[string]string{
+		annotations.AnnLinodeCloudFirewallACL: `{
+			"allowList": {
+				"ipv4": ["2.2.2.2"]
+			}
+		}`,
+	})
+
+	defer func() {
+		_ = lb.EnsureLoadBalancerDeleted(context.TODO(), "linodelb", svc)
+	}()
+	lbStatus, err := lb.EnsureLoadBalancer(context.TODO(), "linodelb", svc, nodes)
+	if err != nil {
+		t.Errorf("EnsureLoadBalancer returned an error: %s", err)
+	}
+	svc.Status.LoadBalancer = *lbStatus
+	stubService(fakeClientset, svc)
+
+	nb, err := lb.getNodeBalancerByStatus(context.TODO(), svc)
+	if err != nil {
+		t.Fatalf("failed to get NodeBalancer via status: %s", err)
+	}
+
+	firewalls, err := lb.client.ListNodeBalancerFirewalls(context.TODO(), nb.ID, &linodego.ListOptions{})
+	if err != nil {
+		t.Fatalf("Failed to list nodeBalancer firewalls %s", err)
+	}
+
+	if len(firewalls) == 0 {
+		t.Fatalf("No firewalls attached")
+	}
+
+	if firewalls[0].Rules.InboundPolicy != "DROP" {
+		t.Errorf("expected DROP inbound policy, got %s", firewalls[0].Rules.InboundPolicy)
+	}
+
+	fwIPs := firewalls[0].Rules.Inbound[0].Addresses.IPv4
+	if fwIPs == nil {
+		t.Errorf("expected IP, got %v", fwIPs)
+	}
+
+	svc.ObjectMeta.SetAnnotations(map[string]string{})
+
+	err = lb.UpdateLoadBalancer(context.TODO(), "linodelb", svc, nodes)
+	if err != nil {
+		t.Errorf("UpdateLoadBalancer returned an error: %s", err)
+	}
+
+	firewallsNew, err := lb.client.ListNodeBalancerFirewalls(context.TODO(), nb.ID, &linodego.ListOptions{})
+	if err != nil {
+		t.Fatalf("failed to List Firewalls %s", err)
+	}
+
+	if len(firewallsNew) != 0 {
+		t.Fatalf("firewall's %d still attached", firewallsNew[0].ID)
+	}
+}
+
 func testUpdateLoadBalancerUpdateFirewallRemoveACLaddID(t *testing.T, client *linodego.Client, fakeAPI *fakeAPI) {
 	svc := &v1.Service{
 		ObjectMeta: metav1.ObjectMeta{
@@ -1810,7 +1907,7 @@ func testUpdateLoadBalancerUpdateFirewall(t *testing.T, client *linodego.Client,
 	}
 }
 
-func testUpdateLoadBalancerDeleteFirewall(t *testing.T, client *linodego.Client, fakeAPI *fakeAPI) {
+func testUpdateLoadBalancerDeleteFirewallRemoveID(t *testing.T, client *linodego.Client, fakeAPI *fakeAPI) {
 	firewallCreateOpts := linodego.FirewallCreateOptions{
 		Label: "test",
 		Rules: linodego.FirewallRuleSet{Inbound: []linodego.FirewallRule{{