Merge branch 'master' of github.com:kubernetes-sigs/cloud-provider-az…

…ure into support-privateendpoint
kubernetes-sigs · Dec 23, 2022 · e423f17 · e423f17
2 parents 0434a17 + de80029
commit e423f17
Show file tree

Hide file tree

Showing 11 changed files with 204 additions and 104 deletions.
diff --git a/pkg/provider/azure_controller_common.go b/pkg/provider/azure_controller_common.go
@@ -100,6 +100,8 @@ type controllerCommon struct {
 	detachDiskMap sync.Map
 	// attach/detach disk rate limiter
 	diskOpRateLimiter flowcontrol.RateLimiter
+	// DisableUpdateCache whether disable update cache in disk attach/detach
+	DisableUpdateCache bool
 }
 
 // AttachDiskOptions attach disk options
@@ -432,16 +434,11 @@ func (c *controllerCommon) UpdateVM(ctx context.Context, nodeName types.NodeName
 	defer c.lockMap.UnlockEntry(node)
 
 	defer func() {
-		// invalidate the cache if there is an error with UpdateVM operation
-		if err != nil {
-			_ = vmset.DeleteCacheForNode(string(nodeName))
-		}
+		_ = vmset.DeleteCacheForNode(string(nodeName))
 	}()
 
 	klog.V(2).Infof("azureDisk - update: vm(%s)", nodeName)
-	err = vmset.UpdateVM(ctx, nodeName)
-	klog.V(2).Infof("azureDisk - update: vm(%s) returned with %v", err)
-	return err
+	return vmset.UpdateVM(ctx, nodeName)
 }
 
 func (c *controllerCommon) insertDetachDiskRequest(diskName, diskURI, nodeName string) error {

diff --git a/pkg/provider/azure_controller_standard.go b/pkg/provider/azure_controller_standard.go
@@ -265,6 +265,9 @@ func (as *availabilitySet) UpdateVMAsync(ctx context.Context, nodeName types.Nod
 }
 
 func (as *availabilitySet) updateCache(nodeName string, vm *compute.VirtualMachine) {
+	if as.common.DisableUpdateCache {
+		return
+	}
 	as.cloud.vmCache.Update(nodeName, vm)
 	klog.V(2).Infof("updateCache(%s) successfully", nodeName)
 }

diff --git a/pkg/provider/azure_controller_vmssflex.go b/pkg/provider/azure_controller_vmssflex.go
@@ -264,6 +264,9 @@ func (fs *FlexScaleSet) UpdateVMAsync(ctx context.Context, nodeName types.NodeNa
 }
 
 func (fs *FlexScaleSet) updateCache(nodeName string, vm *compute.VirtualMachine) error {
+	if fs.common.DisableUpdateCache {
+		return nil
+	}
 	if vm == nil {
 		return fmt.Errorf("vm is nil")
 	}
@@ -290,7 +293,6 @@ func (fs *FlexScaleSet) updateCache(nodeName string, vm *compute.VirtualMachine)
 	}
 	vmMap := cached.(*sync.Map)
 	vmMap.Store(nodeName, vm)
-	fs.vmssFlexVMCache.Update(vmssFlexID, vmMap)
 
 	fs.vmssFlexVMNameToVmssID.Store(strings.ToLower(*vm.OsProfile.ComputerName), vmssFlexID)
 	fs.vmssFlexVMNameToNodeName.Store(*vm.Name, strings.ToLower(*vm.OsProfile.ComputerName))

diff --git a/pkg/provider/azure_controller_vmssflex_test.go b/pkg/provider/azure_controller_vmssflex_test.go
@@ -344,18 +344,19 @@ func TestGetDataDisksWithVmssFlex(t *testing.T) {
 	}
 }
 
-func TestUpdateCache(t *testing.T) {
+func TestVMSSFlexUpdateCache(t *testing.T) {
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()
 
 	fs, err := NewTestFlexScaleSet(ctrl)
 	assert.NoError(t, err, "unexpected error when creating test FlexScaleSet")
 
 	testCases := []struct {
-		description string
-		nodeName    string
-		vm          *compute.VirtualMachine
-		expectedErr error
+		description        string
+		nodeName           string
+		vm                 *compute.VirtualMachine
+		disableUpdateCache bool
+		expectedErr        error
 	}{
 		{
 			description: "vm is nil",
@@ -388,11 +389,16 @@ func TestUpdateCache(t *testing.T) {
 			},
 			expectedErr: fmt.Errorf("vm.OsProfile.ComputerName is nil"),
 		},
+		{
+			description:        "disableUpdateCache is set",
+			disableUpdateCache: true,
+			expectedErr:        nil,
+		},
 	}
 
 	for _, test := range testCases {
+		fs.DisableUpdateCache = test.disableUpdateCache
 		err = fs.updateCache(test.nodeName, test.vm)
 		assert.Equal(t, test.expectedErr, err, test.description)
 	}
-
 }
diff --git a/pkg/provider/azure_storageaccount.go b/pkg/provider/azure_storageaccount.go
@@ -40,8 +40,6 @@ const GroupIDFile = "file"
 const GroupIDBlob = "blob"
 const privateDNSZoneNameFmt = "privatelink.%s.%s"
 
-var privateDNSZoneName string
-
 type StorageType string
 
 const (
@@ -172,6 +170,7 @@ func (az *Cloud) EnsureStorageAccount(ctx context.Context, accountOptions *Accou
 		subsID = accountOptions.SubscriptionID
 	}
 
+	var privateDNSZoneName string
 	if accountOptions.CreatePrivateEndpoint {
 		if accountOptions.StorageType == "" {
 			klog.V(2).Info("set StorageType as file when not specified")
@@ -232,7 +231,7 @@ func (az *Cloud) EnsureStorageAccount(ctx context.Context, accountOptions *Accou
 		if _, err := az.privatednsclient.Get(ctx, vnetResourceGroup, privateDNSZoneName); err != nil {
 			klog.V(2).Infof("get private dns zone %s returned with %v", privateDNSZoneName, err.Error())
 			// Create DNS zone first, this could make sure driver has write permission on vnetResourceGroup
-			if err := az.createPrivateDNSZone(ctx, vnetResourceGroup); err != nil {
+			if err := az.createPrivateDNSZone(ctx, vnetResourceGroup, privateDNSZoneName); err != nil {
 				return "", "", fmt.Errorf("create private DNS zone(%s) in resourceGroup(%s): %w", privateDNSZoneName, vnetResourceGroup, err)
 			}
 		}
@@ -241,7 +240,7 @@ func (az *Cloud) EnsureStorageAccount(ctx context.Context, accountOptions *Accou
 		vNetLinkName := accountName + "-vnetlink"
 		if _, err := az.virtualNetworkLinksClient.Get(ctx, vnetResourceGroup, privateDNSZoneName, vNetLinkName); err != nil {
 			klog.V(2).Infof("get virtual link for vnet(%s) and DNS Zone(%s) returned with %v", vnetName, privateDNSZoneName, err.Error())
-			if err := az.createVNetLink(ctx, vNetLinkName, vnetResourceGroup, vnetName); err != nil {
+			if err := az.createVNetLink(ctx, vNetLinkName, vnetResourceGroup, vnetName, privateDNSZoneName); err != nil {
 				return "", "", fmt.Errorf("create virtual link for vnet(%s) and DNS Zone(%s) in resourceGroup(%s): %w", vnetName, privateDNSZoneName, vnetResourceGroup, err)
 			}
 		}
@@ -391,7 +390,7 @@ func (az *Cloud) EnsureStorageAccount(ctx context.Context, accountOptions *Accou
 
 			// Create dns zone group
 			dnsZoneGroupName := accountName + "-dnszonegroup"
-			if err := az.createPrivateDNSZoneGroup(ctx, dnsZoneGroupName, privateEndpointName, vnetResourceGroup, vnetName); err != nil {
+			if err := az.createPrivateDNSZoneGroup(ctx, dnsZoneGroupName, privateEndpointName, vnetResourceGroup, vnetName, privateDNSZoneName); err != nil {
 				return "", "", fmt.Errorf("create private DNS zone group - privateEndpoint(%s), vNetName(%s), resourceGroup(%s): %w", privateEndpointName, vnetName, vnetResourceGroup, err)
 			}
 		}
@@ -441,7 +440,7 @@ func (az *Cloud) createPrivateEndpoint(ctx context.Context, accountName string,
 	return az.privateendpointclient.CreateOrUpdate(ctx, vnetResourceGroup, privateEndpointName, privateEndpoint, "", true).Error()
 }
 
-func (az *Cloud) createPrivateDNSZone(ctx context.Context, vnetResourceGroup string) error {
+func (az *Cloud) createPrivateDNSZone(ctx context.Context, vnetResourceGroup, privateDNSZoneName string) error {
 	klog.V(2).Infof("Creating private dns zone(%s) in resourceGroup (%s)", privateDNSZoneName, vnetResourceGroup)
 	location := LocationGlobal
 	privateDNSZone := privatedns.PrivateZone{Location: &location}
@@ -455,7 +454,7 @@ func (az *Cloud) createPrivateDNSZone(ctx context.Context, vnetResourceGroup str
 	return nil
 }
 
-func (az *Cloud) createVNetLink(ctx context.Context, vNetLinkName, vnetResourceGroup, vnetName string) error {
+func (az *Cloud) createVNetLink(ctx context.Context, vNetLinkName, vnetResourceGroup, vnetName, privateDNSZoneName string) error {
 	klog.V(2).Infof("Creating virtual link for vnet(%s) and DNS Zone(%s) in resourceGroup(%s)", vNetLinkName, privateDNSZoneName, vnetResourceGroup)
 	location := LocationGlobal
 	vnetID := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Network/virtualNetworks/%s", az.SubscriptionID, vnetResourceGroup, vnetName)
@@ -468,12 +467,11 @@ func (az *Cloud) createVNetLink(ctx context.Context, vNetLinkName, vnetResourceG
 	return az.virtualNetworkLinksClient.CreateOrUpdate(ctx, vnetResourceGroup, privateDNSZoneName, vNetLinkName, parameters, "", false).Error()
 }
 
-func (az *Cloud) createPrivateDNSZoneGroup(ctx context.Context, dnsZoneGroupName, privateEndpointName, vnetResourceGroup, vnetName string) error {
+func (az *Cloud) createPrivateDNSZoneGroup(ctx context.Context, dnsZoneGroupName, privateEndpointName, vnetResourceGroup, vnetName, privateDNSZoneName string) error {
 	klog.V(2).Infof("Creating private DNS zone group(%s) with privateEndpoint(%s), vNetName(%s), resourceGroup(%s)", dnsZoneGroupName, privateEndpointName, vnetName, vnetResourceGroup)
 	privateDNSZoneID := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Network/privateDnsZones/%s", az.SubscriptionID, vnetResourceGroup, privateDNSZoneName)
-	dnsZoneName := privateDNSZoneName
 	privateDNSZoneConfig := network.PrivateDNSZoneConfig{
-		Name: &dnsZoneName,
+		Name: &privateDNSZoneName,
 		PrivateDNSZonePropertiesFormat: &network.PrivateDNSZonePropertiesFormat{
 			PrivateDNSZoneID: &privateDNSZoneID},
 	}

diff --git a/pkg/provider/azure_utils.go b/pkg/provider/azure_utils.go
@@ -328,3 +328,17 @@ func isNodeInVMSSVMCache(nodeName string, vmssVMCache *azcache.TimedCache) bool
 
 	return isInCache
 }
+
+func extractVmssVMName(name string) (string, string, error) {
+	split := strings.SplitAfter(name, consts.VMSSNameSeparator)
+	if len(split) < 2 {
+		klog.V(3).Infof("Failed to extract vmssVMName %q", name)
+		return "", "", ErrorNotVmssInstance
+	}
+
+	ssName := strings.Join(split[0:len(split)-1], "")
+	// removing the trailing `vmssNameSeparator` since we used SplitAfter
+	ssName = ssName[:len(ssName)-1]
+	instanceID := split[len(split)-1]
+	return ssName, instanceID, nil
+}
diff --git a/pkg/provider/azure_utils_test.go b/pkg/provider/azure_utils_test.go
@@ -404,3 +404,47 @@ func TestIsNodeInVMSSVMCache(t *testing.T) {
 		assert.Equal(t, test.expectedResult, result, test.description)
 	}
 }
+
+func TestExtractVmssVMName(t *testing.T) {
+	cases := []struct {
+		description        string
+		vmName             string
+		expectError        bool
+		expectedScaleSet   string
+		expectedInstanceID string
+	}{
+		{
+			description: "wrong vmss VM name should report error",
+			vmName:      "vm1234",
+			expectError: true,
+		},
+		{
+			description: "wrong VM name separator should report error",
+			vmName:      "vm-1234",
+			expectError: true,
+		},
+		{
+			description:        "correct vmss VM name should return correct ScaleSet and instanceID",
+			vmName:             "vm_1234",
+			expectedScaleSet:   "vm",
+			expectedInstanceID: "1234",
+		},
+		{
+			description:        "correct vmss VM name with Extra Separator should return correct ScaleSet and instanceID",
+			vmName:             "vm_test_1234",
+			expectedScaleSet:   "vm_test",
+			expectedInstanceID: "1234",
+		},
+	}
+
+	for _, c := range cases {
+		ssName, instanceID, err := extractVmssVMName(c.vmName)
+		if c.expectError {
+			assert.Error(t, err, c.description)
+			continue
+		}
+
+		assert.Equal(t, c.expectedScaleSet, ssName, c.description)
+		assert.Equal(t, c.expectedInstanceID, instanceID, c.description)
+	}
+}
diff --git a/pkg/provider/azure_vmss.go b/pkg/provider/azure_vmss.go
@@ -47,11 +47,13 @@ var (
 	ErrorNotVmssInstance = errors.New("not a vmss instance")
 	ErrScaleSetNotFound  = errors.New("scale set not found")
 
-	scaleSetNameRE         = regexp.MustCompile(`.*/subscriptions/(?:.*)/Microsoft.Compute/virtualMachineScaleSets/(.+)/virtualMachines(?:.*)`)
-	resourceGroupRE        = regexp.MustCompile(`.*/subscriptions/(?:.*)/resourceGroups/(.+)/providers/Microsoft.Compute/virtualMachineScaleSets/(?:.*)/virtualMachines(?:.*)`)
-	vmssIPConfigurationRE  = regexp.MustCompile(`.*/subscriptions/(?:.*)/resourceGroups/(.+)/providers/Microsoft.Compute/virtualMachineScaleSets/(.+)/virtualMachines/(.+)/networkInterfaces(?:.*)`)
-	vmssPIPConfigurationRE = regexp.MustCompile(`.*/subscriptions/(?:.*)/resourceGroups/(.+)/providers/Microsoft.Compute/virtualMachineScaleSets/(.+)/virtualMachines/(.+)/networkInterfaces/(.+)/ipConfigurations/(.+)/publicIPAddresses/(.+)`)
-	vmssVMProviderIDRE     = regexp.MustCompile(`azure:///subscriptions/(?:.*)/resourceGroups/(.+)/providers/Microsoft.Compute/virtualMachineScaleSets/(.+)/virtualMachines/(?:\d+)`)
+	scaleSetNameRE           = regexp.MustCompile(`.*/subscriptions/(?:.*)/Microsoft.Compute/virtualMachineScaleSets/(.+)/virtualMachines(?:.*)`)
+	resourceGroupRE          = regexp.MustCompile(`.*/subscriptions/(?:.*)/resourceGroups/(.+)/providers/Microsoft.Compute/virtualMachineScaleSets/(?:.*)/virtualMachines(?:.*)`)
+	vmssIPConfigurationRE    = regexp.MustCompile(`.*/subscriptions/(?:.*)/resourceGroups/(.+)/providers/Microsoft.Compute/virtualMachineScaleSets/(.+)/virtualMachines/(.+)/networkInterfaces(?:.*)`)
+	vmssPIPConfigurationRE   = regexp.MustCompile(`.*/subscriptions/(?:.*)/resourceGroups/(.+)/providers/Microsoft.Compute/virtualMachineScaleSets/(.+)/virtualMachines/(.+)/networkInterfaces/(.+)/ipConfigurations/(.+)/publicIPAddresses/(.+)`)
+	vmssVMResourceIDTemplate = `/subscriptions/(?:.*)/resourceGroups/(.+)/providers/Microsoft.Compute/virtualMachineScaleSets/(.+)/virtualMachines/(?:\d+)`
+	vmssVMResourceIDRE       = regexp.MustCompile(vmssVMResourceIDTemplate)
+	vmssVMProviderIDRE       = regexp.MustCompile(fmt.Sprintf("%s%s", "azure://", vmssVMResourceIDTemplate))
 )
 
 // vmssMetaInfo contains the metadata for a VMSS.
@@ -1231,6 +1233,14 @@ func getVmssAndResourceGroupNameByVMProviderID(providerID string) (string, strin
 	return matches[1], matches[2], nil
 }
 
+func getVmssAndResourceGroupNameByVMID(id string) (string, string, error) {
+	matches := vmssVMResourceIDRE.FindStringSubmatch(id)
+	if len(matches) != 3 {
+		return "", "", ErrorNotVmssInstance
+	}
+	return matches[1], matches[2], nil
+}
+
 func (ss *ScaleSet) ensureVMSSInPool(service *v1.Service, nodes []*v1.Node, backendPoolID string, vmSetNameOfLB string) error {
 	klog.V(2).Infof("ensureVMSSInPool: ensuring VMSS with backendPoolID %s", backendPoolID)
 	vmssNamesMap := make(map[string]bool)
@@ -1254,10 +1264,25 @@ func (ss *ScaleSet) ensureVMSSInPool(service *v1.Service, nodes []*v1.Node, back
 			}
 
 			// in this scenario the vmSetName is an empty string and the name of vmss should be obtained from the provider IDs of nodes
-			resourceGroupName, vmssName, err := getVmssAndResourceGroupNameByVMProviderID(node.Spec.ProviderID)
-			if err != nil {
-				klog.V(4).Infof("ensureVMSSInPool: found VMSS node %s, will skip checking and continue", node.Name)
-				continue
+			var resourceGroupName, vmssName string
+			if node.Spec.ProviderID != "" {
+				resourceGroupName, vmssName, err = getVmssAndResourceGroupNameByVMProviderID(node.Spec.ProviderID)
+				if err != nil {
+					klog.V(4).Infof("ensureVMSSInPool: the provider ID %s of node %s is not the format of VMSS VM, will skip checking and continue", node.Spec.ProviderID, node.Name)
+					continue
+				}
+			} else {
+				klog.V(4).Infof("ensureVMSSInPool: the provider ID of node %s is empty, will check the VM ID", node.Name)
+				instanceID, err := ss.InstanceID(context.TODO(), types.NodeName(node.Name))
+				if err != nil {
+					klog.Errorf("ensureVMSSInPool: Failed to get instance ID for node %q: %v", node.Name, err)
+					return err
+				}
+				resourceGroupName, vmssName, err = getVmssAndResourceGroupNameByVMID(instanceID)
+				if err != nil {
+					klog.V(4).Infof("ensureVMSSInPool: the instance ID %s of node %s is not the format of VMSS VM, will skip checking and continue", node.Spec.ProviderID, node.Name)
+					continue
+				}
 			}
 			// only vmsses in the resource group same as it's in azure config are included
 			if strings.EqualFold(resourceGroupName, ss.ResourceGroup) {

diff --git a/pkg/provider/azure_vmss_cache.go b/pkg/provider/azure_vmss_cache.go
@@ -122,20 +122,6 @@ func (ss *ScaleSet) newVMSSCache(ctx context.Context) (*azcache.TimedCache, erro
 	return azcache.NewTimedcache(time.Duration(ss.Config.VmssCacheTTLInSeconds)*time.Second, getter)
 }
 
-func extractVmssVMName(name string) (string, string, error) {
-	split := strings.SplitAfter(name, consts.VMSSNameSeparator)
-	if len(split) < 2 {
-		klog.V(3).Infof("Failed to extract vmssVMName %q", name)
-		return "", "", ErrorNotVmssInstance
-	}
-
-	ssName := strings.Join(split[0:len(split)-1], "")
-	// removing the trailing `vmssNameSeparator` since we used SplitAfter
-	ssName = ssName[:len(ssName)-1]
-	instanceID := split[len(split)-1]
-	return ssName, instanceID, nil
-}
-
 func (ss *ScaleSet) getVMSSVMsFromCache(resourceGroup, vmssName string, crt azcache.AzureCacheReadType) (*sync.Map, error) {
 	cacheKey := getVMSSVMCacheKey(resourceGroup, vmssName)
 	entry, err := ss.vmssVMCache.Get(cacheKey, crt)
@@ -158,7 +144,6 @@ func (ss *ScaleSet) newVMSSVirtualMachinesCache() (*azcache.TimedCache, error) {
 
 	getter := func(cacheKey string) (interface{}, error) {
 		localCache := &sync.Map{} // [nodeName]*VMSSVirtualMachineEntry
-
 		oldCache := make(map[string]*VMSSVirtualMachineEntry)
 
 		entry, exists, err := ss.vmssVMCache.Store.GetByKey(cacheKey)
@@ -292,6 +277,9 @@ func (ss *ScaleSet) DeleteCacheForNode(nodeName string) error {
 }
 
 func (ss *ScaleSet) updateCache(nodeName, resourceGroupName, vmssName, instanceID string, updatedVM *compute.VirtualMachineScaleSetVM) error {
+	if ss.common.DisableUpdateCache {
+		return nil
+	}
 	// lock the VMSS entry to ensure a consistent view of the VM map when there are concurrent updates.
 	cacheKey := getVMSSVMCacheKey(resourceGroupName, vmssName)
 	ss.lockMap.LockEntry(cacheKey)