From 9f8ceb05edb187b472cf64f9a9ca55eb79e3cf36 Mon Sep 17 00:00:00 2001 From: Pengfei Ni Date: Wed, 17 Apr 2019 11:42:29 +0800 Subject: [PATCH] Fix Azure SLB support for multiple backend pools Azure VM and vmssVM support multiple backend pools for the same SLB, but not for different LBs. --- .../providers/azure/azure_standard.go | 19 +++-- .../providers/azure/azure_vmss.go | 19 +++-- .../providers/azure/azure_wrap.go | 26 +++++++ .../providers/azure/azure_wrap_test.go | 75 +++++++++++++++++++ 4 files changed, 123 insertions(+), 16 deletions(-) diff --git a/pkg/cloudprovider/providers/azure/azure_standard.go b/pkg/cloudprovider/providers/azure/azure_standard.go index 4746e0fe1d..c2b22e1e6d 100644 --- a/pkg/cloudprovider/providers/azure/azure_standard.go +++ b/pkg/cloudprovider/providers/azure/azure_standard.go @@ -676,17 +676,20 @@ func (as *availabilitySet) ensureHostInPool(service *v1.Service, nodeName types. // sets, the same network interface couldn't be added to more than one load balancer of // the same type. Omit those nodes (e.g. masters) so Azure ARM won't complain // about this. + newBackendPoolsIDs := make([]string, 0, len(newBackendPools)) for _, pool := range newBackendPools { - backendPool := *pool.ID - matches := backendPoolIDRE.FindStringSubmatch(backendPool) - if len(matches) == 2 { - lbName := matches[1] - if strings.HasSuffix(lbName, InternalLoadBalancerNameSuffix) == isInternal { - klog.V(4).Infof("Node %q has already been added to LB %q, omit adding it to a new one", nodeName, lbName) - return nil - } + if pool.ID != nil { + newBackendPoolsIDs = append(newBackendPoolsIDs, *pool.ID) } } + isSameLB, oldLBName, err := isBackendPoolOnSameLB(backendPoolID, newBackendPoolsIDs) + if err != nil { + return err + } + if !isSameLB { + klog.V(4).Infof("Node %q has already been added to LB %q, omit adding it to a new one", nodeName, oldLBName) + return nil + } } newBackendPools = append(newBackendPools, diff --git a/pkg/cloudprovider/providers/azure/azure_vmss.go b/pkg/cloudprovider/providers/azure/azure_vmss.go index a2ef73b3a9..b0e20b8ce0 100644 --- a/pkg/cloudprovider/providers/azure/azure_vmss.go +++ b/pkg/cloudprovider/providers/azure/azure_vmss.go @@ -785,17 +785,20 @@ func (ss *scaleSet) ensureHostsInVMSetPool(service *v1.Service, backendPoolID st // the same network interface couldn't be added to more than one load balancer of // the same type. Omit those nodes (e.g. masters) so Azure ARM won't complain // about this. + newBackendPoolsIDs := make([]string, 0, len(newBackendPools)) for _, pool := range newBackendPools { - backendPool := *pool.ID - matches := backendPoolIDRE.FindStringSubmatch(backendPool) - if len(matches) == 2 { - lbName := matches[1] - if strings.HasSuffix(lbName, InternalLoadBalancerNameSuffix) == isInternal { - klog.V(4).Infof("vmss %q has already been added to LB %q, omit adding it to a new one", vmSetName, lbName) - return nil - } + if pool.ID != nil { + newBackendPoolsIDs = append(newBackendPoolsIDs, *pool.ID) } } + isSameLB, oldLBName, err := isBackendPoolOnSameLB(backendPoolID, newBackendPoolsIDs) + if err != nil { + return err + } + if !isSameLB { + klog.V(4).Infof("VMSS %q has already been added to LB %q, omit adding it to a new one", vmSetName, oldLBName) + return nil + } } newBackendPools = append(newBackendPools, diff --git a/pkg/cloudprovider/providers/azure/azure_wrap.go b/pkg/cloudprovider/providers/azure/azure_wrap.go index 47b2f54fc6..15cdf4534f 100644 --- a/pkg/cloudprovider/providers/azure/azure_wrap.go +++ b/pkg/cloudprovider/providers/azure/azure_wrap.go @@ -336,3 +336,29 @@ func convertResourceGroupNameToLower(resourceID string) (string, error) { resourceGroup := matches[1] return strings.Replace(resourceID, resourceGroup, strings.ToLower(resourceGroup), 1), nil } + +// isBackendPoolOnSameLB checks whether newBackendPoolID is on the same load balancer as existingBackendPools. +// Since both public and internal LBs are supported, lbName and lbName-internal are treated as same. +// 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 { + return false, "", fmt.Errorf("new backendPoolID %q is in wrong format", newBackendPoolID) + } + + newLBName := matches[1] + newLBNameTrimmed := strings.TrimRight(newLBName, InternalLoadBalancerNameSuffix) + for _, backendPool := range existingBackendPools { + matches := backendPoolIDRE.FindStringSubmatch(backendPool) + if len(matches) != 2 { + return false, "", fmt.Errorf("existing backendPoolID %q is in wrong format", backendPool) + } + + lbName := matches[1] + if !strings.EqualFold(strings.TrimRight(lbName, InternalLoadBalancerNameSuffix), newLBNameTrimmed) { + return false, lbName, nil + } + } + + return true, "", nil +} diff --git a/pkg/cloudprovider/providers/azure/azure_wrap_test.go b/pkg/cloudprovider/providers/azure/azure_wrap_test.go index 84367b547c..417f46cee8 100644 --- a/pkg/cloudprovider/providers/azure/azure_wrap_test.go +++ b/pkg/cloudprovider/providers/azure/azure_wrap_test.go @@ -198,3 +198,78 @@ func TestConvertResourceGroupNameToLower(t *testing.T) { assert.Equal(t, test.expected, real, test.desc) } } + +func TestIsBackendPoolOnSameLB(t *testing.T) { + tests := []struct { + backendPoolID string + existingBackendPools []string + expected bool + expectedLBName string + expectError bool + }{ + { + backendPoolID: "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Network/loadBalancers/lb1/backendAddressPools/pool1", + existingBackendPools: []string{ + "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Network/loadBalancers/lb1/backendAddressPools/pool2", + }, + expected: true, + expectedLBName: "", + }, + { + backendPoolID: "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Network/loadBalancers/lb1-internal/backendAddressPools/pool1", + existingBackendPools: []string{ + "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Network/loadBalancers/lb1/backendAddressPools/pool2", + }, + expected: true, + expectedLBName: "", + }, + { + backendPoolID: "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Network/loadBalancers/lb1/backendAddressPools/pool1", + existingBackendPools: []string{ + "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Network/loadBalancers/lb1-internal/backendAddressPools/pool2", + }, + expected: true, + expectedLBName: "", + }, + { + backendPoolID: "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Network/loadBalancers/lb1/backendAddressPools/pool1", + existingBackendPools: []string{ + "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Network/loadBalancers/lb2/backendAddressPools/pool2", + }, + expected: false, + expectedLBName: "lb2", + }, + { + backendPoolID: "wrong-backendpool-id", + existingBackendPools: []string{ + "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Network/loadBalancers/lb1/backendAddressPools/pool2", + }, + expectError: true, + }, + { + backendPoolID: "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Network/loadBalancers/lb1/backendAddressPools/pool1", + existingBackendPools: []string{ + "wrong-existing-backendpool-id", + }, + expectError: true, + }, + { + backendPoolID: "wrong-backendpool-id", + existingBackendPools: []string{ + "wrong-existing-backendpool-id", + }, + expectError: true, + }, + } + + for _, test := range tests { + isSameLB, lbName, err := isBackendPoolOnSameLB(test.backendPoolID, test.existingBackendPools) + if test.expectError { + assert.Error(t, err) + continue + } + + assert.Equal(t, test.expected, isSameLB) + assert.Equal(t, test.expectedLBName, lbName) + } +}