Merge pull request #6105 from nilo19/fix/multi-slb-internal-svc

fix: Only check internal lbs for internal services when using multi-slb
kubernetes-sigs · May 7, 2024 · f3af02b · f3af02b
2 parents 689e97b + 9e0b139
commit f3af02b
Show file tree

Hide file tree

Showing 3 changed files with 20 additions and 9 deletions.
diff --git a/pkg/provider/azure_loadbalancer.go b/pkg/provider/azure_loadbalancer.go
@@ -3899,7 +3899,7 @@ func (az *Cloud) getAzureLoadBalancerName(
 		}
 
 		currentLBName := az.getServiceCurrentLoadBalancerName(service)
-		lbNamePrefix = getMostEligibleLBForService(currentLBName, eligibleLBs, existingLBs)
+		lbNamePrefix = getMostEligibleLBForService(currentLBName, eligibleLBs, existingLBs, requiresInternalLoadBalancer(service))
 	}
 
 	if isInternal {
@@ -3912,6 +3912,7 @@ func getMostEligibleLBForService(
 	currentLBName string,
 	eligibleLBs []string,
 	existingLBs *[]network.LoadBalancer,
+	isInternal bool,
 ) string {
 	// 1. If the LB is eligible and being used, choose it.
 	if StringInSlice(currentLBName, eligibleLBs) {
@@ -3923,8 +3924,10 @@ func getMostEligibleLBForService(
 	for _, eligibleLB := range eligibleLBs {
 		var found bool
 		if existingLBs != nil {
-			for _, existingLB := range *existingLBs {
-				if strings.EqualFold(trimSuffixIgnoreCase(pointer.StringDeref(existingLB.Name, ""), consts.InternalLoadBalancerNameSuffix), eligibleLB) {
+			for i := range *existingLBs {
+				existingLB := (*existingLBs)[i]
+				if strings.EqualFold(trimSuffixIgnoreCase(pointer.StringDeref(existingLB.Name, ""), consts.InternalLoadBalancerNameSuffix), eligibleLB) &&
+					isInternalLoadBalancer(&existingLB) == isInternal {
 					found = true
 					break
 				}
@@ -3940,8 +3943,10 @@ func getMostEligibleLBForService(
 	var expectedLBName string
 	ruleCount := 301
 	if existingLBs != nil {
-		for _, existingLB := range *existingLBs {
-			if StringInSlice(trimSuffixIgnoreCase(pointer.StringDeref(existingLB.Name, ""), consts.InternalLoadBalancerNameSuffix), eligibleLBs) {
+		for i := range *existingLBs {
+			existingLB := (*existingLBs)[i]
+			if StringInSlice(trimSuffixIgnoreCase(pointer.StringDeref(existingLB.Name, ""), consts.InternalLoadBalancerNameSuffix), eligibleLBs) &&
+				isInternalLoadBalancer(&existingLB) == isInternal {
 				if existingLB.LoadBalancerPropertiesFormat != nil &&
 					existingLB.LoadBalancingRules != nil {
 					if len(*existingLB.LoadBalancingRules) < ruleCount {
@@ -3996,7 +4001,8 @@ func (az *Cloud) getEligibleLoadBalancersForService(service *v1.Service) ([]stri
 	lbsFromAnnotation := consts.GetLoadBalancerConfigurationsNames(service)
 	if len(lbsFromAnnotation) > 0 {
 		lbNamesSet := utilsets.NewString(lbsFromAnnotation...)
-		for _, multiSLBConfig := range az.MultipleStandardLoadBalancerConfigurations {
+		for i := range az.MultipleStandardLoadBalancerConfigurations {
+			multiSLBConfig := az.MultipleStandardLoadBalancerConfigurations[i]
 			if lbNamesSet.Has(multiSLBConfig.Name) {
 				logger.V(4).Info("selects the load balancer by annotation",
 					"load balancer configuration name", multiSLBConfig.Name)
@@ -4121,7 +4127,8 @@ func (az *Cloud) getEligibleLoadBalancersForService(service *v1.Service) ([]stri
 		logger.V(4).Info("no load balancer that has label/namespace selector matches the service, so the service can be placed on the load balancers that do not have label/namespace selector")
 	}
 
-	for _, eligibleLB := range eligibleLBs {
+	for i := range eligibleLBs {
+		eligibleLB := eligibleLBs[i]
 		eligibleLBNames = append(eligibleLBNames, eligibleLB.Name)
 	}
 

diff --git a/pkg/provider/azure_loadbalancer_backendpool.go b/pkg/provider/azure_loadbalancer_backendpool.go
@@ -452,7 +452,9 @@ func (bi *backendPoolTypeNodeIP) EnsureHostsInPool(service *v1.Service, nodes []
 			}
 
 			privateIP := getNodePrivateIPAddress(node, isIPv6)
-			nodePrivateIPsSet.Insert(privateIP)
+			if privateIP != "" {
+				nodePrivateIPsSet.Insert(privateIP)
+			}
 
 			if bi.useMultipleStandardLoadBalancers() {
 				if !activeNodes.Has(node.Name) {

diff --git a/pkg/provider/azure_loadbalancer_test.go b/pkg/provider/azure_loadbalancer_test.go
@@ -7400,6 +7400,7 @@ func TestGetMostEligibleLBName(t *testing.T) {
 		currentLBName  string
 		eligibleLBs    []string
 		existingLBs    *[]network.LoadBalancer
+		isInternal     bool
 		expectedLBName string
 	}{
 		{
@@ -7487,10 +7488,11 @@ func TestGetMostEligibleLBName(t *testing.T) {
 				},
 			},
 			expectedLBName: "lb2",
+			isInternal:     true,
 		},
 	} {
 		t.Run(tc.description, func(t *testing.T) {
-			lbName := getMostEligibleLBForService(tc.currentLBName, tc.eligibleLBs, tc.existingLBs)
+			lbName := getMostEligibleLBForService(tc.currentLBName, tc.eligibleLBs, tc.existingLBs, tc.isInternal)
 			assert.Equal(t, tc.expectedLBName, lbName)
 		})
 	}