From 1336858b8ce39272361ee89d693513b8999a4673 Mon Sep 17 00:00:00 2001
From: Bryce Soghigian <49734722+Bryce-Soghigian@users.noreply.github.com>
Date: Fri, 31 Jan 2025 13:28:32 -0800
Subject: [PATCH] refactor: network interface garbage collection followup
 (#670)

* refactor: nicListQuery and vmListQuery vars should be scoped to the provider
* style: breaking test.Interface calls into multiple lines
---
 .../nodeclaim/garbagecollection/suite_test.go | 23 ++++++++++++++-----
 pkg/providers/instance/instance.go            | 16 ++++++-------
 2 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/pkg/controllers/nodeclaim/garbagecollection/suite_test.go b/pkg/controllers/nodeclaim/garbagecollection/suite_test.go
index 3197e5c94..15594abe6 100644
--- a/pkg/controllers/nodeclaim/garbagecollection/suite_test.go
+++ b/pkg/controllers/nodeclaim/garbagecollection/suite_test.go
@@ -324,7 +324,10 @@ var _ = Describe("NetworkInterface Garbage Collection", func() {
 		ExpectApplied(ctx, env.Client, nodeClaim)
 
 		// Create a managed NIC
-		nic := test.Interface(test.InterfaceOptions{Name: instance.GenerateResourceName(nodeClaim.Name), NodepoolName: nodePool.Name})
+		nic := test.Interface(test.InterfaceOptions{
+			Name:         instance.GenerateResourceName(nodeClaim.Name),
+			NodepoolName: nodePool.Name,
+		})
 		azureEnv.NetworkInterfacesAPI.NetworkInterfaces.Store(lo.FromPtr(nic.ID), *nic)
 
 		nicsBeforeGC, err := azureEnv.InstanceProvider.ListNics(ctx)
@@ -340,8 +343,12 @@ var _ = Describe("NetworkInterface Garbage Collection", func() {
 		Expect(len(nicsAfterGC)).To(Equal(1))
 	})
 	It("should delete a NIC if there is no associated VM", func() {
-		nic := test.Interface(test.InterfaceOptions{NodepoolName: nodePool.Name})
-		nic2 := test.Interface(test.InterfaceOptions{NodepoolName: nodePool.Name})
+		nic := test.Interface(test.InterfaceOptions{
+			NodepoolName: nodePool.Name,
+		})
+		nic2 := test.Interface(test.InterfaceOptions{
+			NodepoolName: nodePool.Name,
+		})
 		azureEnv.NetworkInterfacesAPI.NetworkInterfaces.Store(lo.FromPtr(nic.ID), *nic)
 		azureEnv.NetworkInterfacesAPI.NetworkInterfaces.Store(lo.FromPtr(nic2.ID), *nic2)
 		nicsBeforeGC, err := azureEnv.InstanceProvider.ListNics(ctx)
@@ -355,7 +362,9 @@ var _ = Describe("NetworkInterface Garbage Collection", func() {
 		Expect(len(nicsAfterGC)).To(Equal(0))
 	})
 	It("should not delete a NIC if there is an associated VM", func() {
-		managedNic := test.Interface(test.InterfaceOptions{NodepoolName: nodePool.Name})
+		managedNic := test.Interface(test.InterfaceOptions{
+			NodepoolName: nodePool.Name,
+		})
 		azureEnv.NetworkInterfacesAPI.NetworkInterfaces.Store(lo.FromPtr(managedNic.ID), *managedNic)
 		managedVM := test.VirtualMachine(test.VirtualMachineOptions{Name: lo.FromPtr(managedNic.Name), NodepoolName: nodePool.Name})
 		azureEnv.VirtualMachinesAPI.VirtualMachinesBehavior.Instances.Store(lo.FromPtr(managedVM.ID), *managedVM)
@@ -366,8 +375,10 @@ var _ = Describe("NetworkInterface Garbage Collection", func() {
 		Expect(len(nicsAfterGC)).To(Equal(1))
 
 	})
-	It("the vm gc controller should remove the nic if there is an associated vm", func() {
-		managedNic := test.Interface(test.InterfaceOptions{NodepoolName: nodePool.Name})
+	It("the vm gc controller should handle deletion of network interfaces if a nic is associated with a vm", func() {
+		managedNic := test.Interface(test.InterfaceOptions{
+			NodepoolName: nodePool.Name,
+		})
 		azureEnv.NetworkInterfacesAPI.NetworkInterfaces.Store(lo.FromPtr(managedNic.ID), *managedNic)
 		managedVM := test.VirtualMachine(test.VirtualMachineOptions{
 			Name:         lo.FromPtr(managedNic.Name),
diff --git a/pkg/providers/instance/instance.go b/pkg/providers/instance/instance.go
index b8c2fb77f..08504fa16 100644
--- a/pkg/providers/instance/instance.go
+++ b/pkg/providers/instance/instance.go
@@ -53,11 +53,6 @@ import (
 	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/network/armnetwork"
 )
 
-var (
-	vmListQuery  string
-	nicListQuery string
-)
-
 var (
 	NodePoolTagKey = strings.ReplaceAll(karpv1.NodePoolLabelKey, "/", "_")
 
@@ -106,6 +101,8 @@ type DefaultProvider struct {
 	subscriptionID         string
 	unavailableOfferings   *cache.UnavailableOfferings
 	provisionMode          string
+
+	vmListQuery, nicListQuery string
 }
 
 func NewDefaultProvider(
@@ -119,8 +116,6 @@ func NewDefaultProvider(
 	subscriptionID string,
 	provisionMode string,
 ) *DefaultProvider {
-	vmListQuery = GetVMListQueryBuilder(resourceGroup).String()
-	nicListQuery = GetNICListQueryBuilder(resourceGroup).String()
 	return &DefaultProvider{
 		azClient:               azClient,
 		instanceTypeProvider:   instanceTypeProvider,
@@ -131,6 +126,9 @@ func NewDefaultProvider(
 		subscriptionID:         subscriptionID,
 		unavailableOfferings:   offeringsCache,
 		provisionMode:          provisionMode,
+
+		vmListQuery:  GetVMListQueryBuilder(resourceGroup).String(),
+		nicListQuery: GetNICListQueryBuilder(resourceGroup).String(),
 	}
 }
 
@@ -180,7 +178,7 @@ func (p *DefaultProvider) Get(ctx context.Context, vmName string) (*armcompute.V
 }
 
 func (p *DefaultProvider) List(ctx context.Context) ([]*armcompute.VirtualMachine, error) {
-	req := NewQueryRequest(&(p.subscriptionID), vmListQuery)
+	req := NewQueryRequest(&(p.subscriptionID), p.vmListQuery)
 	client := p.azClient.azureResourceGraphClient
 	data, err := GetResourceData(ctx, client, *req)
 	if err != nil {
@@ -212,7 +210,7 @@ func (p *DefaultProvider) GetNic(ctx context.Context, rg, nicName string) (*armn
 
 // ListNics returns all network interfaces in the resource group that have the nodepool tag
 func (p *DefaultProvider) ListNics(ctx context.Context) ([]*armnetwork.Interface, error) {
-	req := NewQueryRequest(&(p.subscriptionID), nicListQuery)
+	req := NewQueryRequest(&(p.subscriptionID), p.nicListQuery)
 	client := p.azClient.azureResourceGraphClient
 	data, err := GetResourceData(ctx, client, *req)
 	if err != nil {