From 1a55d0f7b0938cbd1b5d5be03be80e5d3ff25dd5 Mon Sep 17 00:00:00 2001 From: Anders Liu Date: Fri, 13 Dec 2019 23:27:57 -0800 Subject: [PATCH] fix: should truncate long subnet name on lb rules --- .../azure/azure_loadbalancer.go | 6 +- .../azure/azure_loadbalancer_test.go | 2 +- .../azure/azure_standard.go | 28 ++++-- .../azure/azure_standard_test.go | 85 +++++++++++++++++++ .../azure/azure_test.go | 4 +- 5 files changed, 113 insertions(+), 12 deletions(-) diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go index e315086d24b..401fefab828 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go @@ -405,7 +405,7 @@ func (az *Cloud) getServiceLoadBalancerStatus(service *v1.Service, lb *network.L return nil, nil } isInternal := requiresInternalLoadBalancer(service) - lbFrontendIPConfigName := az.getFrontendIPConfigName(service, subnet(service)) + lbFrontendIPConfigName := az.getFrontendIPConfigName(service) serviceName := getServiceName(service) for _, ipConfiguration := range *lb.FrontendIPConfigurations { if lbFrontendIPConfigName == *ipConfiguration.Name { @@ -693,7 +693,7 @@ func (az *Cloud) reconcileLoadBalancer(clusterName string, service *v1.Service, } lbName := *lb.Name klog.V(2).Infof("reconcileLoadBalancer for service(%s): lb(%s) wantLb(%t) resolved load balancer name", serviceName, lbName, wantLb) - lbFrontendIPConfigName := az.getFrontendIPConfigName(service, subnet(service)) + lbFrontendIPConfigName := az.getFrontendIPConfigName(service) lbFrontendIPConfigID := az.getFrontendIPConfigID(lbName, lbFrontendIPConfigName) lbBackendPoolName := getBackendPoolName(az.ipv6DualStackEnabled, clusterName, service) lbBackendPoolID := az.getBackendPoolID(lbName, lbBackendPoolName) @@ -1026,7 +1026,7 @@ func (az *Cloud) reconcileLoadBalancerRule( } for _, protocol := range protocols { - lbRuleName := az.getLoadBalancerRuleName(service, protocol, port.Port, subnet(service)) + lbRuleName := az.getLoadBalancerRuleName(service, protocol, port.Port) klog.V(2).Infof("reconcileLoadBalancerRule lb name (%s) rule name (%s)", lbName, lbRuleName) transportProto, _, probeProto, err := getProtocolsFromKubernetesProtocol(protocol) diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go index fc65aefb712..c4eec3507d7 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go @@ -1627,7 +1627,7 @@ func TestGetServiceLoadBalancerStatus(t *testing.T) { }, { desc: "getServiceLoadBalancerStatus shall return nil if lb.FrontendIPConfigurations.name != " + - "az.getFrontendIPConfigName(service, subnet(service))", + "az.getFrontendIPConfigName(service)", service: &internalService, lb: &lb3, }, diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_standard.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_standard.go index 216a2dc77ea..57c24954797 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_standard.go +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_standard.go @@ -273,12 +273,21 @@ func getBackendPoolName(ipv6DualStackEnabled bool, clusterName string, service * return clusterName } -func (az *Cloud) getLoadBalancerRuleName(service *v1.Service, protocol v1.Protocol, port int32, subnetName *string) string { +func (az *Cloud) getLoadBalancerRuleName(service *v1.Service, protocol v1.Protocol, port int32) string { prefix := az.getRulePrefix(service) - if subnetName == nil { - return fmt.Sprintf("%s-%s-%d", prefix, protocol, port) + ruleName := fmt.Sprintf("%s-%s-%d", prefix, protocol, port) + subnet := subnet(service) + if subnet == nil { + return ruleName } - return fmt.Sprintf("%s-%s-%s-%d", prefix, *subnetName, protocol, port) + + // Load balancer rule name must be less or equal to 80 charactors, so excluding the hyphen two segments cannot exceed 79 + subnetSegment := *subnet + if len(ruleName) + len(subnetSegment) > 79 { + subnetSegment = subnetSegment[:79 - len(ruleName)] + } + + return fmt.Sprintf("%s-%s-%s-%d", prefix, subnetSegment, protocol, port) } func (az *Cloud) getSecurityRuleName(service *v1.Service, port v1.ServicePort, sourceAddrPrefix string) string { @@ -316,10 +325,17 @@ func (az *Cloud) serviceOwnsFrontendIP(fip network.FrontendIPConfiguration, serv return strings.HasPrefix(*fip.Name, baseName) } -func (az *Cloud) getFrontendIPConfigName(service *v1.Service, subnetName *string) string { +func (az *Cloud) getFrontendIPConfigName(service *v1.Service) string { baseName := az.GetLoadBalancerName(context.TODO(), "", service) + subnetName := subnet(service) if subnetName != nil { - return fmt.Sprintf("%s-%s", baseName, *subnetName) + ipcName := fmt.Sprintf("%s-%s", baseName, *subnetName) + + // Azure lb front end configuration name must not exceed 80 charactors + if len(ipcName) > 80 { + ipcName = ipcName[:80] + } + return ipcName } return baseName } diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_standard_test.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_standard_test.go index 014854cffe8..40a74edf72b 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_standard_test.go +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_standard_test.go @@ -20,6 +20,7 @@ package azure import ( "testing" + "strconv" "github.com/stretchr/testify/assert" @@ -253,3 +254,87 @@ func TestGetAzureLoadBalancerName(t *testing.T) { assert.Equal(t, c.expected, loadbalancerName, c.description) } } + +func TestGetLoadBalancingRuleName(t *testing.T) { + az := getTestCloud() + az.PrimaryAvailabilitySetName = "primary" + + svc := &v1.Service{ + ObjectMeta: meta.ObjectMeta{ + Annotations: map[string]string{ + ServiceAnnotationLoadBalancerInternalSubnet: "subnet", + ServiceAnnotationLoadBalancerInternal: "true", + }, + UID: "257b9655-5137-4ad2-b091-ef3f07043ad3", + }, + } + + cases := []struct { + description string + subnetName string + isInternal bool + useStandardLB bool + protocol v1.Protocol + port int32 + expected string + }{ + { + description: "internal lb should have subnet name on the rule name", + subnetName: "shortsubnet", + isInternal: true, + useStandardLB: true, + protocol: v1.ProtocolTCP, + port: 9000, + expected: "a257b965551374ad2b091ef3f07043ad-shortsubnet-TCP-9000", + }, + { + description: "internal standard lb should have subnet name on the rule name but truncated to 80 charactors", + subnetName: "averylonnnngggnnnnnnnnnnnnnnnnnnnnnngggggggggggggggggggggggggggggggggggggsubet", + isInternal: true, + useStandardLB: true, + protocol: v1.ProtocolTCP, + port: 9000, + expected: "a257b965551374ad2b091ef3f07043ad-averylonnnngggnnnnnnnnnnnnnnnnnnnnnngg-TCP-9000", + }, + { + description: "internal basic lb should have subnet name on the rule name but truncated to 80 charactors", + subnetName: "averylonnnngggnnnnnnnnnnnnnnnnnnnnnngggggggggggggggggggggggggggggggggggggsubet", + isInternal: true, + useStandardLB: false, + protocol: v1.ProtocolTCP, + port: 9000, + expected: "a257b965551374ad2b091ef3f07043ad-averylonnnngggnnnnnnnnnnnnnnnnnnnnnngg-TCP-9000", + }, + { + description: "external standard lb should not have subnet name on the rule name", + subnetName: "shortsubnet", + isInternal: false, + useStandardLB: true, + protocol: v1.ProtocolTCP, + port: 9000, + expected: "a257b965551374ad2b091ef3f07043ad-TCP-9000", + }, + { + description: "external basic lb should not have subnet name on the rule name", + subnetName: "shortsubnet", + isInternal: false, + useStandardLB: false, + protocol: v1.ProtocolTCP, + port: 9000, + expected: "a257b965551374ad2b091ef3f07043ad-TCP-9000", + }, + } + + for _, c := range cases { + if c.useStandardLB { + az.Config.LoadBalancerSku = loadBalancerSkuStandard + } else { + az.Config.LoadBalancerSku = loadBalancerSkuBasic + } + svc.Annotations[ServiceAnnotationLoadBalancerInternalSubnet] = c.subnetName + svc.Annotations[ServiceAnnotationLoadBalancerInternal] = strconv.FormatBool(c.isInternal) + + loadbalancerName := az.getLoadBalancerRuleName(svc, c.protocol, c.port) + assert.Equal(t, c.expected, loadbalancerName, c.description) + } +} diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_test.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_test.go index 12026fe57b2..15857b2b995 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_test.go +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_test.go @@ -1238,14 +1238,14 @@ func validateLoadBalancer(t *testing.T, loadBalancer *network.LoadBalancer, serv if len(svc.Spec.Ports) > 0 { expectedFrontendIPCount++ expectedFrontendIP := ExpectedFrontendIPInfo{ - Name: az.getFrontendIPConfigName(&svc, subnet(&svc)), + Name: az.getFrontendIPConfigName(&svc), Subnet: subnet(&svc), } expectedFrontendIPs = append(expectedFrontendIPs, expectedFrontendIP) } for _, wantedRule := range svc.Spec.Ports { expectedRuleCount++ - wantedRuleName := az.getLoadBalancerRuleName(&svc, wantedRule.Protocol, wantedRule.Port, subnet(&svc)) + wantedRuleName := az.getLoadBalancerRuleName(&svc, wantedRule.Protocol, wantedRule.Port) foundRule := false for _, actualRule := range *loadBalancer.LoadBalancingRules { if strings.EqualFold(*actualRule.Name, wantedRuleName) &&