From 9e0b13960a9b44abdbd3b3de349a55751301acca Mon Sep 17 00:00:00 2001 From: Qi Ni Date: Sun, 5 May 2024 16:27:55 +0800 Subject: [PATCH] fix: Only check internal lbs for internal services when using multi-slb --- pkg/provider/azure_loadbalancer.go | 21 ++++++++++++------- .../azure_loadbalancer_backendpool.go | 4 +++- pkg/provider/azure_loadbalancer_test.go | 4 +++- 3 files changed, 20 insertions(+), 9 deletions(-) diff --git a/pkg/provider/azure_loadbalancer.go b/pkg/provider/azure_loadbalancer.go index 5d25ce124e..93b2728176 100644 --- a/pkg/provider/azure_loadbalancer.go +++ b/pkg/provider/azure_loadbalancer.go @@ -3767,7 +3767,7 @@ func (az *Cloud) getAzureLoadBalancerName( } currentLBName := az.getServiceCurrentLoadBalancerName(service) - lbNamePrefix = getMostEligibleLBForService(currentLBName, eligibleLBs, existingLBs) + lbNamePrefix = getMostEligibleLBForService(currentLBName, eligibleLBs, existingLBs, requiresInternalLoadBalancer(service)) } if isInternal { @@ -3780,6 +3780,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) { @@ -3791,8 +3792,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 } @@ -3808,8 +3811,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 { @@ -3864,7 +3869,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) @@ -3989,7 +3995,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 index b33ee2e71c..bd63ed3958 100644 --- 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 index b55f4b098b..cff7b18843 100644 --- 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) }) }