[release-1.30] Multi-slb related bug fixes

pkg/provider/azure_loadbalancer.go

@@ -182,7 +182,7 @@ func (az *Cloud) reconcileService(ctx context.Context, clusterName string, servi
 		az.localServiceNameToServiceInfoMap.Store(key, newServiceInfo(getServiceIPFamily(service), lbName))
 		// There are chances that the endpointslice changes after EnsureHostsInPool, so
 		// need to check endpointslice for a second time.
-		if err := az.checkAndApplyLocalServiceBackendPoolUpdates(ctx, *lb, service); err != nil {
+		if err := az.checkAndApplyLocalServiceBackendPoolUpdates(*lb, service); err != nil {
 			logger.Error(err, "Failed to checkAndApplyLocalServiceBackendPoolUpdates")
 			return nil, err
 		}

pkg/provider/azure_loadbalancer_backendpool.go

@@ -203,7 +203,7 @@ func (bc *backendPoolTypeNodeIPConfig) ReconcileBackendPools(
 			if bp.BackendAddressPoolPropertiesFormat != nil &&
 				bp.LoadBalancerBackendAddresses != nil &&
 				len(*bp.LoadBalancerBackendAddresses) > 0 {
-				if removeNodeIPAddressesFromBackendPool(bp, []string{}, true, false) {
+				if removeNodeIPAddressesFromBackendPool(bp, []string{}, true, false, false) {
 					isMigration = true
 					bp.VirtualNetwork = nil
 					if err := bc.CreateOrUpdateLBBackendPool(ctx, lbName, bp); err != nil {
@@ -411,7 +411,6 @@ func (bi *backendPoolTypeNodeIP) EnsureHostsInPool(ctx context.Context, service
 		changed               bool
 		numOfAdd, numOfDelete int
 		activeNodes           *utilsets.IgnoreCaseSet
-		err                   error
 	)
 	if bi.useMultipleStandardLoadBalancers() {
 		if !isLocalService(service) {
@@ -426,10 +425,12 @@ func (bi *backendPoolTypeNodeIP) EnsureHostsInPool(ctx context.Context, service
 					"current load balancer", si.lbName)
 				return nil
 			}
-			activeNodes, err = bi.getLocalServiceEndpointsNodeNames(ctx, service)
-			if err != nil {
-				return err
-			}
+			activeNodes = bi.getLocalServiceEndpointsNodeNames(service)
+		}
+
+		if isNICPool(backendPool) {
+			klog.V(4).InfoS("EnsureHostsInPool: skipping NIC-based backend pool", "backendPoolName", ptr.Deref(backendPool.Name, ""))
+			return nil
 		}
 	}
 
@@ -464,7 +465,7 @@ func (bi *backendPoolTypeNodeIP) EnsureHostsInPool(ctx context.Context, service
 			}
 
 			if bi.useMultipleStandardLoadBalancers() {
-				if !activeNodes.Has(node.Name) {
+				if activeNodes != nil && !activeNodes.Has(node.Name) {
 					klog.V(4).Infof("bi.EnsureHostsInPool: node %s should not be in load balancer %q", node.Name, lbName)
 					continue
 				}
@@ -501,7 +502,7 @@ func (bi *backendPoolTypeNodeIP) EnsureHostsInPool(ctx context.Context, service
 				}
 			}
 		}
-		removeNodeIPAddressesFromBackendPool(backendPool, nodeIPsToBeDeleted, false, bi.useMultipleStandardLoadBalancers())
+		removeNodeIPAddressesFromBackendPool(backendPool, nodeIPsToBeDeleted, false, bi.useMultipleStandardLoadBalancers(), true)
 	}
 	if changed {
 		klog.V(2).Infof("bi.EnsureHostsInPool: updating backend pool %s of load balancer %s to add %d nodes and remove %d nodes", lbBackendPoolName, lbName, numOfAdd, numOfDelete)
@@ -634,7 +635,7 @@ func (bi *backendPoolTypeNodeIP) ReconcileBackendPools(ctx context.Context, clus
 		if isMigration && bi.EnableMigrateToIPBasedBackendPoolAPI {
 			var backendPoolNames []string
 			for _, id := range lbBackendPoolIDsSlice {
-				name, err := getLBNameFromBackendPoolID(id)
+				name, err := getBackendPoolNameFromBackendPoolID(id)
 				if err != nil {
 					klog.Errorf("bi.ReconcileBackendPools for service (%s): failed to get LB name from backend pool ID: %s", serviceName, err.Error())
 					return false, false, nil, err
@@ -675,7 +676,7 @@ func (bi *backendPoolTypeNodeIP) ReconcileBackendPools(ctx context.Context, clus
 				}
 			}
 			if len(nodeIPAddressesToBeDeleted) > 0 {
-				if removeNodeIPAddressesFromBackendPool(bp, nodeIPAddressesToBeDeleted, false, false) {
+				if removeNodeIPAddressesFromBackendPool(bp, nodeIPAddressesToBeDeleted, false, false, true) {
 					updated = true
 				}
 			}
@@ -872,11 +873,13 @@ func hasIPAddressInBackendPool(backendPool *network.BackendAddressPool, ipAddres
 func removeNodeIPAddressesFromBackendPool(
 	backendPool network.BackendAddressPool,
 	nodeIPAddresses []string,
-	removeAll, useMultipleStandardLoadBalancers bool,
+	removeAll, useMultipleStandardLoadBalancers, isNodeIP bool,
 ) bool {
 	changed := false
 	nodeIPsSet := utilsets.NewString(nodeIPAddresses...)
 
+	logger := klog.Background().WithName("removeNodeIPAddressFromBackendPool")
+
 	if backendPool.BackendAddressPoolPropertiesFormat == nil ||
 		backendPool.LoadBalancerBackendAddresses == nil {
 		return false
@@ -887,7 +890,13 @@ func removeNodeIPAddressesFromBackendPool(
 		if addresses[i].LoadBalancerBackendAddressPropertiesFormat != nil {
 			ipAddress := pointer.StringDeref((*backendPool.LoadBalancerBackendAddresses)[i].IPAddress, "")
 			if ipAddress == "" {
-				klog.V(4).Infof("removeNodeIPAddressFromBackendPool: LoadBalancerBackendAddress %s is not IP-based, skipping", pointer.StringDeref(addresses[i].Name, ""))
+				if isNodeIP {
+					logger.V(4).Info("LoadBalancerBackendAddress is not IP-based, removing", "LoadBalancerBackendAddress", ptr.Deref(addresses[i].Name, ""))
+					addresses = append(addresses[:i], addresses[i+1:]...)
+					changed = true
+				} else {
+					logger.V(4).Info("LoadBalancerBackendAddress is not IP-based, skipping", "LoadBalancerBackendAddress", ptr.Deref(addresses[i].Name, ""))
+				}
 				continue
 			}
 			if removeAll || nodeIPsSet.Has(ipAddress) {

pkg/provider/azure_loadbalancer_backendpool_test.go

@@ -33,6 +33,7 @@ import (
 	"k8s.io/client-go/kubernetes/fake"
 	cloudprovider "k8s.io/cloud-provider"
 	"k8s.io/utils/pointer"
+	"k8s.io/utils/ptr"
 
 	"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/loadbalancerclient/mockloadbalancerclient"
 	"sigs.k8s.io/cloud-provider-azure/pkg/consts"
@@ -201,19 +202,14 @@ func TestEnsureHostsInPoolNodeIP(t *testing.T) {
 			},
 		},
 		{
-			desc: "should add correct nodes to the pool and remove unwanted ones when using multi-slb",
+			desc: "should skip NIC-based backend pool when using multi-slb",
 			backendPool: network.BackendAddressPool{
-				Name: pointer.String("kubernetes"),
+				Name: ptr.To("kubernetes"),
 				BackendAddressPoolPropertiesFormat: &network.BackendAddressPoolPropertiesFormat{
 					LoadBalancerBackendAddresses: &[]network.LoadBalancerBackendAddress{
 						{
 							LoadBalancerBackendAddressPropertiesFormat: &network.LoadBalancerBackendAddressPropertiesFormat{
-								IPAddress: pointer.String("10.0.0.1"),
-							},
-						},
-						{
-							LoadBalancerBackendAddressPropertiesFormat: &network.LoadBalancerBackendAddressPropertiesFormat{
-								IPAddress: pointer.String("10.0.0.3"),
+								IPAddress: ptr.To(""),
 							},
 						},
 					},
@@ -228,49 +224,55 @@ func TestEnsureHostsInPoolNodeIP(t *testing.T) {
 				},
 			},
 			expectedBackendPool: network.BackendAddressPool{
-				Name: pointer.String("kubernetes"),
+				Name: ptr.To("kubernetes"),
 				BackendAddressPoolPropertiesFormat: &network.BackendAddressPoolPropertiesFormat{
-					VirtualNetwork: &network.SubResource{ID: pointer.String("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/virtualNetworks/vnet")},
 					LoadBalancerBackendAddresses: &[]network.LoadBalancerBackendAddress{
 						{
-							Name: pointer.String("vmss-2"),
 							LoadBalancerBackendAddressPropertiesFormat: &network.LoadBalancerBackendAddressPropertiesFormat{
-								IPAddress: pointer.String("10.0.0.4"),
+								IPAddress: ptr.To(""),
 							},
 						},
 					},
 				},
 			},
+			skip: true,
 		},
 		{
-			desc:  "should add ips to the local service dedicated backend pool",
-			local: true,
+			desc: "should add correct nodes to the pool and remove unwanted ones when using multi-slb",
 			backendPool: network.BackendAddressPool{
-				Name: pointer.String("default-svc-1"),
+				Name: pointer.String("kubernetes"),
 				BackendAddressPoolPropertiesFormat: &network.BackendAddressPoolPropertiesFormat{
-					LoadBalancerBackendAddresses: &[]network.LoadBalancerBackendAddress{},
+					LoadBalancerBackendAddresses: &[]network.LoadBalancerBackendAddress{
+						{
+							LoadBalancerBackendAddressPropertiesFormat: &network.LoadBalancerBackendAddressPropertiesFormat{
+								IPAddress: pointer.String("10.0.0.1"),
+							},
+						},
+						{
+							LoadBalancerBackendAddressPropertiesFormat: &network.LoadBalancerBackendAddressPropertiesFormat{
+								IPAddress: pointer.String("10.0.0.3"),
+							},
+						},
+					},
 				},
 			},
 			multiSLBConfigs: []MultipleStandardLoadBalancerConfiguration{
 				{
 					Name: "kubernetes",
+					MultipleStandardLoadBalancerConfigurationStatus: MultipleStandardLoadBalancerConfigurationStatus{
+						ActiveNodes: utilsets.NewString("vmss-2"),
+					},
 				},
 			},
 			expectedBackendPool: network.BackendAddressPool{
-				Name: pointer.String("default-svc-1"),
+				Name: pointer.String("kubernetes"),
 				BackendAddressPoolPropertiesFormat: &network.BackendAddressPoolPropertiesFormat{
 					VirtualNetwork: &network.SubResource{ID: pointer.String("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/virtualNetworks/vnet")},
 					LoadBalancerBackendAddresses: &[]network.LoadBalancerBackendAddress{
 						{
-							Name: pointer.String("vmss-0"),
-							LoadBalancerBackendAddressPropertiesFormat: &network.LoadBalancerBackendAddressPropertiesFormat{
-								IPAddress: pointer.String("10.0.0.2"),
-							},
-						},
-						{
-							Name: pointer.String("vmss-1"),
+							Name: pointer.String("vmss-2"),
 							LoadBalancerBackendAddressPropertiesFormat: &network.LoadBalancerBackendAddressPropertiesFormat{
-								IPAddress: pointer.String("10.0.0.1"),
+								IPAddress: pointer.String("10.0.0.4"),
 							},
 						},
 					},
@@ -934,20 +936,22 @@ func TestReconcileBackendPoolsNodeIPConfigToIPWithMigrationAPI(t *testing.T) {
 	mockVMSet.EXPECT().GetPrimaryVMSetName().Return("k8s-agentpool1-00000000").AnyTimes()
 
 	mockLBClient := mockloadbalancerclient.NewMockInterface(ctrl)
-	mockLBClient.EXPECT().MigrateToIPBasedBackendPool(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(retry.NewError(false, errors.New("error")))
+	mockLBClient.EXPECT().MigrateToIPBasedBackendPool(gomock.Any(), gomock.Any(), "testCluster", []string{"testCluster"}).Return(retry.NewError(false, errors.New("error")))
 
 	az := GetTestCloud(ctrl)
 	az.VMSet = mockVMSet
 	az.LoadBalancerClient = mockLBClient
 	az.EnableMigrateToIPBasedBackendPoolAPI = true
+	az.LoadBalancerSku = "standard"
+	az.MultipleStandardLoadBalancerConfigurations = []MultipleStandardLoadBalancerConfiguration{{Name: "kubernetes"}}
 
 	bi := newBackendPoolTypeNodeIP(az)
 	svc := getTestService("test", v1.ProtocolTCP, nil, false, 80)
 	_, _, _, err := bi.ReconcileBackendPools(context.TODO(), testClusterName, &svc, &lb)
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "error")
 
-	mockLBClient.EXPECT().MigrateToIPBasedBackendPool(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
+	mockLBClient.EXPECT().MigrateToIPBasedBackendPool(gomock.Any(), gomock.Any(), "testCluster", []string{"testCluster"}).Return(nil)
 	bps := buildLBWithVMIPs(testClusterName, []string{"1.2.3.4", "2.3.4.5"}).BackendAddressPools
 	mockLBClient.EXPECT().GetLBBackendPool(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return((*bps)[0], nil)
 	mockLBClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(network.LoadBalancer{}, nil)
@@ -980,6 +984,7 @@ func TestRemoveNodeIPAddressFromBackendPool(t *testing.T) {
 		description                           string
 		removeAll, useMultiSLB                bool
 		unwantedIPs, existingIPs, expectedIPs []string
+		isNodeIP                              bool
 	}{
 		{
 			description: "removeNodeIPAddressFromBackendPool should remove the unwanted IP addresses from the backend pool",
@@ -1007,12 +1012,26 @@ func TestRemoveNodeIPAddressFromBackendPool(t *testing.T) {
 			existingIPs: []string{"1.2.3.4", "4.3.2.1", ""},
 			expectedIPs: []string{""},
 		},
+		{
+			description: "removeNodeIPAddressFromBackendPool should skip non-IP based backend addresses when isNodeIP is false",
+			unwantedIPs: []string{"1.2.3.4"},
+			existingIPs: []string{"1.2.3.4", ""},
+			expectedIPs: []string{""},
+		},
+		{
+			description: "removeNodeIPAddressFromBackendPool should remove non-IP based backend addresses when isNodeIP is true",
+			unwantedIPs: []string{"1.2.3.4"},
+			existingIPs: []string{"1.2.3.4", ""},
+			expectedIPs: []string{},
+			useMultiSLB: true,
+			isNodeIP:    true,
+		},
 	} {
 		t.Run(tc.description, func(t *testing.T) {
 			backendPool := buildTestLoadBalancerBackendPoolWithIPs("kubernetes", tc.existingIPs)
 			expectedBackendPool := buildTestLoadBalancerBackendPoolWithIPs("kubernetes", tc.expectedIPs)
 
-			removeNodeIPAddressesFromBackendPool(backendPool, tc.unwantedIPs, tc.removeAll, tc.useMultiSLB)
+			removeNodeIPAddressesFromBackendPool(backendPool, tc.unwantedIPs, tc.removeAll, tc.useMultiSLB, tc.isNodeIP)
 			assert.Equal(t, expectedBackendPool, backendPool)
 		})
 	}

pkg/provider/azure_loadbalancer_repo.go

@@ -298,7 +298,6 @@ func (az *Cloud) MigrateToIPBasedBackendPoolAndWaitForCompletion(
 		}
 		return true, nil
 	})
-
 	if err != nil {
 		if errors.Is(err, wait.ErrWaitTimeout) {
 			klog.Warningf("MigrateToIPBasedBackendPoolAndWaitForCompletion: Timeout waiting for migration to IP based backend pool for lb %s, backend pool %s", lbName, strings.Join(backendPoolNames, ","))
@@ -352,15 +351,15 @@ func (az *Cloud) getAzureLoadBalancer(ctx context.Context, name string, crt azca
 // If not same, the lbName for existingBackendPools would also be returned.
 func isBackendPoolOnSameLB(newBackendPoolID string, existingBackendPools []string) (bool, string, error) {
 	matches := backendPoolIDRE.FindStringSubmatch(newBackendPoolID)
-	if len(matches) != 2 {
+	if len(matches) != 3 {
 		return false, "", fmt.Errorf("new backendPoolID %q is in wrong format", newBackendPoolID)
 	}
 
 	newLBName := matches[1]
 	newLBNameTrimmed := trimSuffixIgnoreCase(newLBName, consts.InternalLoadBalancerNameSuffix)
 	for _, backendPool := range existingBackendPools {
 		matches := backendPoolIDRE.FindStringSubmatch(backendPool)
-		if len(matches) != 2 {
+		if len(matches) != 3 {
 			return false, "", fmt.Errorf("existing backendPoolID %q is in wrong format", backendPool)
 		}
 
@@ -381,3 +380,18 @@ func (az *Cloud) serviceOwnsRule(service *v1.Service, rule string) bool {
 	prefix := az.getRulePrefix(service)
 	return strings.HasPrefix(strings.ToUpper(rule), strings.ToUpper(prefix))
 }
+
+func isNICPool(bp network.BackendAddressPool) bool {
+	logger := klog.Background().WithName("isNICPool").WithValues("backendPoolName", ptr.Deref(bp.Name, ""))
+	if bp.BackendAddressPoolPropertiesFormat != nil &&
+		bp.LoadBalancerBackendAddresses != nil {
+		for _, addr := range *bp.LoadBalancerBackendAddresses {
+			if ptr.Deref(addr.IPAddress, "") == "" {
+				logger.V(4).Info("The load balancer backend address has empty ip address, assuming it is a NIC pool",
+					"loadBalancerBackendAddress", ptr.Deref(addr.Name, ""))
+				return true
+			}
+		}
+	}
+	return false
+}