fix: VM name should be obtained from NIC.VirtualMachine.ID instead of…

… NIC name.
kubernetes-sigs · Oct 26, 2023 · a254c06 · a254c06
1 parent 5cae02a
commit a254c06
Show file tree

Hide file tree

Showing 5 changed files with 113 additions and 35 deletions.
diff --git a/pkg/provider/azure_utils.go b/pkg/provider/azure_utils.go
@@ -604,3 +604,17 @@ func getServiceIPFamily(service *v1.Service) string {
 	}
 	return consts.IPVersionIPv4String
 }
+
+func getResourceGroupAndNameFromNICID(ipConfigurationID string) (string, string, error) {
+	matches := nicIDRE.FindStringSubmatch(ipConfigurationID)
+	if len(matches) != 3 {
+		klog.V(4).Infof("Can not extract nic name from ipConfigurationID (%s)", ipConfigurationID)
+		return "", "", fmt.Errorf("invalid ip config ID %s", ipConfigurationID)
+	}
+
+	nicResourceGroup, nicName := matches[1], matches[2]
+	if nicResourceGroup == "" || nicName == "" {
+		return "", "", fmt.Errorf("invalid ip config ID %s", ipConfigurationID)
+	}
+	return nicResourceGroup, nicName, nil
+}
diff --git a/pkg/provider/azure_vmss_cache.go b/pkg/provider/azure_vmss_cache.go
@@ -484,22 +484,47 @@ func (ss *ScaleSet) getVMManagementTypeByIPConfigurationID(ipConfigurationID str
 		return ManagedByUnknownVMSet, err
 	}
 
-	matches := nicIDRE.FindStringSubmatch(ipConfigurationID)
-	if len(matches) != 3 {
+	nicResourceGroup, nicName, err := getResourceGroupAndNameFromNICID(ipConfigurationID)
+	if err != nil {
 		return ManagedByUnknownVMSet, fmt.Errorf("can not extract nic name from ipConfigurationID (%s)", ipConfigurationID)
 	}
 
-	nicResourceGroup, nicName := matches[1], matches[2]
-	if nicResourceGroup == "" || nicName == "" {
-		return ManagedByUnknownVMSet, fmt.Errorf("invalid ip config ID %s", ipConfigurationID)
-	}
-
 	vmName := strings.Replace(nicName, "-nic", "", 1)
 
 	cachedAvSetVMs := cached.(NonVmssUniformNodesEntry).AvSetVMNodeNames
 
 	if cachedAvSetVMs.Has(vmName) {
 		return ManagedByAvSet, nil
 	}
+
+	// Get the vmName by nic.VirtualMachine.ID if the vmName is not in the format
+	// of `vmName-nic`. This introduces an extra ARM call.
+	vmName, err = ss.GetVMNameByIPConfigurationName(nicResourceGroup, nicName)
+	if err != nil {
+		return ManagedByUnknownVMSet, fmt.Errorf("failed to get vm name by ip config ID %s: %w", ipConfigurationID, err)
+	}
+	if cachedAvSetVMs.Has(vmName) {
+		return ManagedByAvSet, nil
+	}
+
 	return ManagedByVmssFlex, nil
 }
+
+func (az *Cloud) GetVMNameByIPConfigurationName(nicResourceGroup, nicName string) (string, error) {
+	ctx, cancel := getContextWithCancel()
+	defer cancel()
+	nic, rerr := az.InterfacesClient.Get(ctx, nicResourceGroup, nicName, "")
+	if rerr != nil {
+		return "", fmt.Errorf("failed to get interface of name %s: %w", nicName, rerr.Error())
+	}
+	if nic.InterfacePropertiesFormat == nil || nic.InterfacePropertiesFormat.VirtualMachine == nil || nic.InterfacePropertiesFormat.VirtualMachine.ID == nil {
+		return "", fmt.Errorf("failed to get vm ID of nic %s", pointer.StringDeref(nic.Name, ""))
+	}
+	vmID := pointer.StringDeref(nic.InterfacePropertiesFormat.VirtualMachine.ID, "")
+	matches := vmIDRE.FindStringSubmatch(vmID)
+	if len(matches) != 2 {
+		return "", fmt.Errorf("invalid virtual machine ID %s", vmID)
+	}
+	vmName := matches[1]
+	return vmName, nil
+}
diff --git a/pkg/provider/azure_vmss_cache_test.go b/pkg/provider/azure_vmss_cache_test.go
@@ -18,17 +18,20 @@ package provider
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net/http"
 	"testing"
 
 	"github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2022-08-01/compute"
+	"github.com/Azure/azure-sdk-for-go/services/network/mgmt/2022-07-01/network"
 	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/assert"
 
 	cloudprovider "k8s.io/cloud-provider"
 	"k8s.io/utils/pointer"
 
+	"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/interfaceclient/mockinterfaceclient"
 	"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/vmclient/mockvmclient"
 	"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/vmssclient/mockvmssclient"
 	"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/vmssvmclient/mockvmssvmclient"
@@ -323,6 +326,17 @@ func TestGetVMManagementTypeByProviderID(t *testing.T) {
 	}
 }
 
+func buildTestNICWithVMName(vmName string) network.Interface {
+	return network.Interface{
+		Name: &vmName,
+		InterfacePropertiesFormat: &network.InterfacePropertiesFormat{
+			VirtualMachine: &network.SubResource{
+				ID: pointer.String(fmt.Sprintf("/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Compute/virtualMachines/%s", vmName)),
+			},
+		},
+	}
+}
+
 func TestGetVMManagementTypeByIPConfigurationID(t *testing.T) {
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()
@@ -337,28 +351,53 @@ func TestGetVMManagementTypeByIPConfigurationID(t *testing.T) {
 		testVM2,
 	}
 
+	testVM1NIC := buildTestNICWithVMName("testvm1")
+	testVM2NIC := buildTestNICWithVMName("testvm2")
+	testVM3NIC := buildTestNICWithVMName("testvm3")
+	testVM3NIC.VirtualMachine = nil
+
 	testCases := []struct {
 		description                 string
 		ipConfigurationID           string
 		DisableAvailabilitySetNodes bool
 		EnableVmssFlexNodes         bool
 		vmListErr                   *retry.Error
+		nicGetErr                   *retry.Error
+		expectedNIC                 string
 		expectedVMManagementType    VMManagementType
 		expectedErr                 error
 	}{
 		{
 			description:              "getVMManagementTypeByIPConfigurationID should return ManagedByVmssFlex for vmss flex node",
 			ipConfigurationID:        "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Network/networkInterfaces/testvm1-nic/ipConfigurations/pipConfig",
-			vmListErr:                nil,
+			expectedNIC:              "testvm1",
 			expectedVMManagementType: ManagedByVmssFlex,
-			expectedErr:              nil,
 		},
 		{
 			description:              "getVMManagementTypeByIPConfigurationID should return ManagedByAvSet for availabilityset node",
 			ipConfigurationID:        "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Network/networkInterfaces/testvm2-nic/ipConfigurations/pipConfig",
-			vmListErr:                nil,
 			expectedVMManagementType: ManagedByAvSet,
-			expectedErr:              nil,
+		},
+		{
+			description:              "getVMManagementTypeByIPConfigurationID should return ManagedByAvSet for availabilityset node from nic.VirtualMachine.ID",
+			ipConfigurationID:        "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Network/networkInterfaces/testvm2-interface/ipConfigurations/pipConfig",
+			expectedNIC:              "testvm2",
+			expectedVMManagementType: ManagedByAvSet,
+		},
+		{
+			description:              "getVMManagementTypeByIPConfigurationID should return an error if nic.VirtualMachine.ID is empty",
+			ipConfigurationID:        "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Network/networkInterfaces/testvm3-interface/ipConfigurations/pipConfig",
+			expectedNIC:              "testvm3",
+			expectedVMManagementType: ManagedByUnknownVMSet,
+			expectedErr:              fmt.Errorf("failed to get vm name by ip config ID /subscriptions/sub/resourceGroups/rg/providers/Microsoft.Network/networkInterfaces/testvm3-interface/ipConfigurations/pipConfig: %w", errors.New("failed to get vm ID of nic testvm3")),
+		},
+		{
+			description:              "getVMManagementTypeByIPConfigurationID should return an error if failed to get nic",
+			ipConfigurationID:        "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Network/networkInterfaces/testvm1-nic/ipConfigurations/pipConfig",
+			expectedNIC:              "testvm1",
+			nicGetErr:                &retry.Error{RawError: fmt.Errorf("failed to get nic")},
+			expectedVMManagementType: ManagedByUnknownVMSet,
+			expectedErr:              fmt.Errorf("failed to get vm name by ip config ID /subscriptions/sub/resourceGroups/rg/providers/Microsoft.Network/networkInterfaces/testvm1-nic/ipConfigurations/pipConfig: %w", errors.New("failed to get interface of name testvm1-nic: Retriable: false, RetryAfter: 0s, HTTPStatusCode: 0, RawError: failed to get nic")),
 		},
 		{
 			description:              "getVMManagementTypeByIPConfigurationID should return ManagedByVmssUniform for vmss uniform node",
@@ -395,6 +434,22 @@ func TestGetVMManagementTypeByIPConfigurationID(t *testing.T) {
 		mockVMClient := ss.cloud.VirtualMachinesClient.(*mockvmclient.MockInterface)
 		mockVMClient.EXPECT().List(gomock.Any(), gomock.Any()).Return(testVMList, tc.vmListErr).AnyTimes()
 
+		if tc.expectedNIC != "" {
+			mockNICClient := ss.InterfacesClient.(*mockinterfaceclient.MockInterface)
+			mockNICClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(ctx context.Context, resourceGroupName string, nicName string, expand string) (network.Interface, *retry.Error) {
+				switch tc.expectedNIC {
+				case "testvm1":
+					return testVM1NIC, tc.nicGetErr
+				case "testvm2":
+					return testVM2NIC, tc.nicGetErr
+				case "testvm3":
+					return testVM3NIC, tc.nicGetErr
+				default:
+					return network.Interface{}, retry.NewError(false, errors.New("failed to get nic"))
+				}
+			})
+		}
+
 		vmManagementType, err := ss.getVMManagementTypeByIPConfigurationID(tc.ipConfigurationID, azcache.CacheReadTypeDefault)
 		assert.Equal(t, tc.expectedVMManagementType, vmManagementType, tc.description)
 		if tc.expectedErr != nil {

diff --git a/pkg/provider/azure_vmssflex.go b/pkg/provider/azure_vmssflex.go
@@ -384,33 +384,16 @@ func (fs *FlexScaleSet) GetNodeNameByIPConfigurationID(ipConfigurationID string)
 }
 
 func (fs *FlexScaleSet) getNodeInformationByIPConfigurationID(ipConfigurationID string) (string, string, string, error) {
-	matches := nicIDRE.FindStringSubmatch(ipConfigurationID)
-	if len(matches) != 3 {
-		klog.V(4).Infof("Can not extract nic name from ipConfigurationID (%s)", ipConfigurationID)
-		return "", "", "", fmt.Errorf("invalid ip config ID %s", ipConfigurationID)
-	}
-
-	nicResourceGroup, nicName := matches[1], matches[2]
-	if nicResourceGroup == "" || nicName == "" {
-		return "", "", "", fmt.Errorf("invalid ip config ID %s", ipConfigurationID)
+	nicResourceGroup, nicName, err := getResourceGroupAndNameFromNICID(ipConfigurationID)
+	if err != nil {
+		return "", "", "", fmt.Errorf("failed to get resource group and name from ip config ID %s: %w", ipConfigurationID, err)
 	}
 
 	// get vmName by nic name
-	ctx, cancel := getContextWithCancel()
-	defer cancel()
-	nic, rerr := fs.InterfacesClient.Get(ctx, nicResourceGroup, nicName, "")
-	if rerr != nil {
-		return "", "", "", fmt.Errorf("getNodeInformationByIPConfigurationID(%s): failed to get interface of name %s: %w", ipConfigurationID, nicName, rerr.Error())
-	}
-	if nic.InterfacePropertiesFormat == nil || nic.InterfacePropertiesFormat.VirtualMachine == nil || nic.InterfacePropertiesFormat.VirtualMachine.ID == nil {
-		return "", "", "", fmt.Errorf("failed to get vm ID of ip config ID %s", ipConfigurationID)
-	}
-	vmID := pointer.StringDeref(nic.InterfacePropertiesFormat.VirtualMachine.ID, "")
-	matches = vmIDRE.FindStringSubmatch(vmID)
-	if len(matches) != 2 {
-		return "", "", "", fmt.Errorf("invalid virtual machine ID %s", vmID)
+	vmName, err := fs.GetVMNameByIPConfigurationName(nicResourceGroup, nicName)
+	if err != nil {
+		return "", "", "", fmt.Errorf("failed to get vm name of ip config ID %s", ipConfigurationID)
 	}
-	vmName := matches[1]
 
 	nodeName, err := fs.getNodeNameByVMName(vmName)
 	if err != nil {

diff --git a/pkg/provider/azure_vmssflex_test.go b/pkg/provider/azure_vmssflex_test.go
@@ -17,6 +17,7 @@ limitations under the License.
 package provider
 
 import (
+	"errors"
 	"fmt"
 	"testing"
 
@@ -808,7 +809,7 @@ func TestGetNodeNameByIPConfigurationIDVmssFlex(t *testing.T) {
 			vmListErr:                      nil,
 			expectedNodeName:               "",
 			expectedVMSetName:              "",
-			expectedErr:                    fmt.Errorf("invalid ip config ID /subscriptions/sub/resourceGroups/rg/providers/Microsoft.Network/networkInterfaces//ipConfigurations/pipConfig"),
+			expectedErr:                    fmt.Errorf("failed to get resource group and name from ip config ID /subscriptions/sub/resourceGroups/rg/providers/Microsoft.Network/networkInterfaces//ipConfigurations/pipConfig: %w", errors.New("invalid ip config ID /subscriptions/sub/resourceGroups/rg/providers/Microsoft.Network/networkInterfaces//ipConfigurations/pipConfig")),
 		},
 	}