From 37c42b10dddbb01992abf27d8d78be09d67cd167 Mon Sep 17 00:00:00 2001 From: Ivan Towlson Date: Wed, 30 Aug 2017 16:00:21 +1200 Subject: [PATCH 1/2] Azure: expose services on non-default subnets --- .../providers/azure/azure_loadbalancer.go | 36 +++++- .../providers/azure/azure_test.go | 111 +++++++++++++++++- .../providers/azure/azure_util.go | 27 ++++- 3 files changed, 158 insertions(+), 16 deletions(-) diff --git a/pkg/cloudprovider/providers/azure/azure_loadbalancer.go b/pkg/cloudprovider/providers/azure/azure_loadbalancer.go index bd3fcf28ac..5987fb409e 100644 --- a/pkg/cloudprovider/providers/azure/azure_loadbalancer.go +++ b/pkg/cloudprovider/providers/azure/azure_loadbalancer.go @@ -35,6 +35,10 @@ import ( // ServiceAnnotationLoadBalancerInternal is the annotation used on the service const ServiceAnnotationLoadBalancerInternal = "service.beta.kubernetes.io/azure-load-balancer-internal" +// ServiceAnnotationLoadBalancerInternalSubnet is the annotation used on the service +// to specify what subnet it is exposed on +const ServiceAnnotationLoadBalancerInternalSubnet = "service.beta.kubernetes.io/azure-load-balancer-internal-subnet" + // GetLoadBalancer returns whether the specified load balancer exists, and // if so, what its status is. func (az *Cloud) GetLoadBalancer(clusterName string, service *v1.Service) (status *v1.LoadBalancerStatus, exists bool, err error) { @@ -54,7 +58,7 @@ func (az *Cloud) GetLoadBalancer(clusterName string, service *v1.Service) (statu var lbIP *string if isInternal { - lbFrontendIPConfigName := getFrontendIPConfigName(service) + lbFrontendIPConfigName := getFrontendIPConfigName(service, subnet(service)) for _, ipConfiguration := range *lb.FrontendIPConfigurations { if lbFrontendIPConfigName == *ipConfiguration.Name { lbIP = ipConfiguration.PrivateIPAddress @@ -182,7 +186,11 @@ func (az *Cloud) EnsureLoadBalancer(clusterName string, service *v1.Service, nod var fipConfigurationProperties *network.FrontendIPConfigurationPropertiesFormat if isInternal { - subnet, existsSubnet, err := az.getSubnet(az.VnetName, az.SubnetName) + subnetName := subnet(service) + if subnetName == nil { + subnetName = &az.SubnetName + } + subnet, existsSubnet, err := az.getSubnet(az.VnetName, *subnetName) if err != nil { return nil, err } @@ -366,7 +374,7 @@ func (az *Cloud) cleanupLoadBalancer(clusterName string, service *v1.Service, is if !isInternalLb { // Find public ip resource to clean up from IP configuration - lbFrontendIPConfigName := getFrontendIPConfigName(service) + lbFrontendIPConfigName := getFrontendIPConfigName(service, subnet(service)) for _, config := range *lb.FrontendIPConfigurations { if strings.EqualFold(*config.Name, lbFrontendIPConfigName) { if config.PublicIPAddress != nil { @@ -523,7 +531,7 @@ func (az *Cloud) reconcileLoadBalancer(lb network.LoadBalancer, fipConfiguration isInternal := requiresInternalLoadBalancer(service) lbName := getLoadBalancerName(clusterName, isInternal) serviceName := getServiceName(service) - lbFrontendIPConfigName := getFrontendIPConfigName(service) + lbFrontendIPConfigName := getFrontendIPConfigName(service, subnet(service)) lbFrontendIPConfigID := az.getFrontendIPConfigID(lbName, lbFrontendIPConfigName) lbBackendPoolName := getBackendPoolName(clusterName) lbBackendPoolID := az.getBackendPoolID(lbName, lbBackendPoolName) @@ -575,6 +583,14 @@ func (az *Cloud) reconcileLoadBalancer(lb network.LoadBalancer, fipConfiguration } } } else { + for i := len(newConfigs) - 1; i >= 0; i-- { + config := newConfigs[i] + if serviceOwnsFrontEndIP(config, service) && !strings.EqualFold(*config.Name, lbFrontendIPConfigName) { + glog.V(3).Infof("reconcile(%s)(%t): lb frontendconfig(%s) - dropping", serviceName, wantLb, *config.Name) + newConfigs = append(newConfigs[:i], newConfigs[i+1:]...) + dirtyConfigs = true + } + } foundConfig := false for _, config := range newConfigs { if strings.EqualFold(*config.Name, lbFrontendIPConfigName) { @@ -608,7 +624,7 @@ func (az *Cloud) reconcileLoadBalancer(lb network.LoadBalancer, fipConfiguration var expectedProbes []network.Probe var expectedRules []network.LoadBalancingRule for _, port := range ports { - lbRuleName := getLoadBalancerRuleName(service, port) + lbRuleName := getLoadBalancerRuleName(service, port, subnet(service)) transportProto, _, probeProto, err := getProtocolsFromKubernetesProtocol(port.Protocol) if err != nil { @@ -652,6 +668,7 @@ func (az *Cloud) reconcileLoadBalancer(lb network.LoadBalancer, fipConfiguration if service.Spec.SessionAffinity == v1.ServiceAffinityClientIP { loadDistribution = network.SourceIP } + expectedRule := network.LoadBalancingRule{ Name: &lbRuleName, LoadBalancingRulePropertiesFormat: &network.LoadBalancingRulePropertiesFormat{ @@ -797,7 +814,7 @@ func (az *Cloud) reconcileSecurityGroup(sg network.SecurityGroup, clusterName st } for j := range sourceAddressPrefixes { ix := i*len(sourceAddressPrefixes) + j - securityRuleName := getSecurityRuleName(service, port, sourceAddressPrefixes[j]) + securityRuleName := getSecurityRuleName(service, port, subnet(service), sourceAddressPrefixes[j]) expectedSecurityRules[ix] = network.SecurityRule{ Name: to.StringPtr(securityRuleName), SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{ @@ -994,3 +1011,10 @@ func requiresInternalLoadBalancer(service *v1.Service) bool { return false } + +func subnet(service *v1.Service) *string { + if l, ok := service.Annotations[ServiceAnnotationLoadBalancerInternalSubnet]; ok { + return &l + } + return nil +} diff --git a/pkg/cloudprovider/providers/azure/azure_test.go b/pkg/cloudprovider/providers/azure/azure_test.go index 32ea4c68e0..e8e1525a50 100644 --- a/pkg/cloudprovider/providers/azure/azure_test.go +++ b/pkg/cloudprovider/providers/azure/azure_test.go @@ -67,6 +67,104 @@ func TestReconcileLoadBalancerAddPort(t *testing.T) { validateLoadBalancer(t, lb, svc) } +// Test additional of a new service/port on an internal LB with a subnet. +func TestReconcileLoadBalancerAddPortOnInternalSubnet(t *testing.T) { + az := getTestCloud() + svc := getTestService("servicea", v1.ProtocolTCP, 80) + addTestSubnet(&svc) + configProperties := getTestPublicFipConfigurationProperties() + lb := getTestLoadBalancer() + nodes := []*v1.Node{} + + svc.Spec.Ports = append(svc.Spec.Ports, v1.ServicePort{ + Name: fmt.Sprintf("port-udp-%d", 1234), + Protocol: v1.ProtocolUDP, + Port: 1234, + NodePort: getBackendPort(1234), + }) + + lb, updated, err := az.reconcileLoadBalancer(lb, &configProperties, testClusterName, &svc, nodes) + if err != nil { + t.Errorf("Unexpected error: %q", err) + } + + if !updated { + t.Error("Expected the loadbalancer to need an update") + } + + // ensure we got a frontend ip configuration + if len(*lb.FrontendIPConfigurations) != 1 { + t.Error("Expected the loadbalancer to have a frontend ip configuration") + } + + validateLoadBalancer(t, lb, svc) +} + +// Test addition of services on an internal LB using both default and explicit subnets. +func TestReconcileLoadBalancerAddServicesOnMultipleSubnets(t *testing.T) { + az := getTestCloud() + svc1 := getTestService("service1", v1.ProtocolTCP, 8081) + svc2 := getTestService("service2", v1.ProtocolTCP, 8081) + addTestSubnet(&svc2) + configProperties := getTestPublicFipConfigurationProperties() + lb := getTestLoadBalancer() + nodes := []*v1.Node{} + + lb, updated, err := az.reconcileLoadBalancer(lb, &configProperties, testClusterName, &svc1, nodes) + if err != nil { + t.Errorf("Unexpected error reconciling svc1: %q", err) + } + + lb, updated, err = az.reconcileLoadBalancer(lb, &configProperties, testClusterName, &svc2, nodes) + if err != nil { + t.Errorf("Unexpected error reconciling svc2: %q", err) + } + + if !updated { + t.Error("Expected the loadbalancer to need an update") + } + + // ensure we got a frontend ip configuration for each service + if len(*lb.FrontendIPConfigurations) != 2 { + t.Error("Expected the loadbalancer to have 2 frontend ip configurations") + } + + validateLoadBalancer(t, lb, svc1, svc2) +} + +// Test moving a service exposure from one subnet to another. +func TestReconcileLoadBalancerEditServiceSubnet(t *testing.T) { + az := getTestCloud() + svc := getTestService("service1", v1.ProtocolTCP, 8081) + addTestSubnet(&svc) + configProperties := getTestPublicFipConfigurationProperties() + lb := getTestLoadBalancer() + nodes := []*v1.Node{} + + lb, updated, err := az.reconcileLoadBalancer(lb, &configProperties, testClusterName, &svc, nodes) + if err != nil { + t.Errorf("Unexpected error reconciling initial svc: %q", err) + } + + svc.Annotations[ServiceAnnotationLoadBalancerInternalSubnet] = "NewSubnet" + + lb, updated, err = az.reconcileLoadBalancer(lb, &configProperties, testClusterName, &svc, nodes) + if err != nil { + t.Errorf("Unexpected error reconciling edits to svc: %q", err) + } + + if !updated { + t.Error("Expected the loadbalancer to need an update") + } + + // ensure we got a frontend ip configuration for the service + if len(*lb.FrontendIPConfigurations) != 1 { + t.Error("Expected the loadbalancer to have 1 frontend ip configuration") + } + + validateLoadBalancer(t, lb, svc) +} + func TestReconcileLoadBalancerNodeHealth(t *testing.T) { az := getTestCloud() svc := getTestService("servicea", v1.ProtocolTCP, 80) @@ -337,7 +435,7 @@ func getTestLoadBalancer(services ...v1.Service) network.LoadBalancer { for _, service := range services { for _, port := range service.Spec.Ports { - ruleName := getLoadBalancerRuleName(&service, port) + ruleName := getLoadBalancerRuleName(&service, port, nil) rules = append(rules, network.LoadBalancingRule{ Name: to.StringPtr(ruleName), LoadBalancingRulePropertiesFormat: &network.LoadBalancingRulePropertiesFormat{ @@ -381,7 +479,7 @@ func getTestSecurityGroup(services ...v1.Service) network.SecurityGroup { for _, port := range service.Spec.Ports { sources := getServiceSourceRanges(&service) for _, src := range sources { - ruleName := getSecurityRuleName(&service, port, src) + ruleName := getSecurityRuleName(&service, port, nil, src) rules = append(rules, network.SecurityRule{ Name: to.StringPtr(ruleName), SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{ @@ -412,7 +510,7 @@ func validateLoadBalancer(t *testing.T, loadBalancer network.LoadBalancer, servi } for _, wantedRule := range svc.Spec.Ports { expectedRuleCount++ - wantedRuleName := getLoadBalancerRuleName(&svc, wantedRule) + wantedRuleName := getLoadBalancerRuleName(&svc, wantedRule, subnet(&svc)) foundRule := false for _, actualRule := range *loadBalancer.LoadBalancingRules { if strings.EqualFold(*actualRule.Name, wantedRuleName) && @@ -484,7 +582,7 @@ func validateSecurityGroup(t *testing.T, securityGroup network.SecurityGroup, se for _, wantedRule := range svc.Spec.Ports { sources := getServiceSourceRanges(&svc) for _, source := range sources { - wantedRuleName := getSecurityRuleName(&svc, wantedRule, source) + wantedRuleName := getSecurityRuleName(&svc, wantedRule, nil, source) expectedRuleCount++ foundRule := false for _, actualRule := range *securityGroup.SecurityRules { @@ -887,3 +985,8 @@ func TestMetadataParsing(t *testing.T) { t.Errorf("Unexpected inequality:\n%#v\nvs\n%#v", network, networkJSON) } } + +func addTestSubnet(svc *v1.Service) { + svc.Annotations[ServiceAnnotationLoadBalancerInternal] = "true" + svc.Annotations[ServiceAnnotationLoadBalancerInternalSubnet] = "TestSubnet" +} diff --git a/pkg/cloudprovider/providers/azure/azure_util.go b/pkg/cloudprovider/providers/azure/azure_util.go index e6b8c4922a..7a6b48e8d9 100644 --- a/pkg/cloudprovider/providers/azure/azure_util.go +++ b/pkg/cloudprovider/providers/azure/azure_util.go @@ -195,13 +195,19 @@ func getBackendPoolName(clusterName string) string { return clusterName } -func getLoadBalancerRuleName(service *v1.Service, port v1.ServicePort) string { - return fmt.Sprintf("%s-%s-%d", getRulePrefix(service), port.Protocol, port.Port) +func getLoadBalancerRuleName(service *v1.Service, port v1.ServicePort, subnetName *string) string { + if subnetName == nil { + return fmt.Sprintf("%s-%s-%d", getRulePrefix(service), port.Protocol, port.Port) + } + return fmt.Sprintf("%s-%s-%s-%d", getRulePrefix(service), *subnetName, port.Protocol, port.Port) } -func getSecurityRuleName(service *v1.Service, port v1.ServicePort, sourceAddrPrefix string) string { +func getSecurityRuleName(service *v1.Service, port v1.ServicePort, subnetName *string, sourceAddrPrefix string) string { safePrefix := strings.Replace(sourceAddrPrefix, "/", "_", -1) - return fmt.Sprintf("%s-%s-%d-%s", getRulePrefix(service), port.Protocol, port.Port, safePrefix) + if subnetName == nil { + return fmt.Sprintf("%s-%s-%d-%s", getRulePrefix(service), port.Protocol, port.Port, safePrefix) + } + return fmt.Sprintf("%s-%s-%s-%d-%s", getRulePrefix(service), *subnetName, port.Protocol, port.Port, safePrefix) } // This returns a human-readable version of the Service used to tag some resources. @@ -224,8 +230,17 @@ func serviceOwnsRule(service *v1.Service, rule string) bool { return strings.HasPrefix(strings.ToUpper(rule), strings.ToUpper(prefix)) } -func getFrontendIPConfigName(service *v1.Service) string { - return cloudprovider.GetLoadBalancerName(service) +func serviceOwnsFrontEndIP(fip network.FrontendIPConfiguration, service *v1.Service) bool { + baseName := cloudprovider.GetLoadBalancerName(service) + return strings.HasPrefix(*fip.Name, baseName) +} + +func getFrontendIPConfigName(service *v1.Service, subnetName *string) string { + baseName := cloudprovider.GetLoadBalancerName(service) + if subnetName != nil { + return fmt.Sprintf("%s-%s", baseName, *subnetName) + } + return baseName } // This returns the next available rule priority level for a given set of security rules. From 125a05479059bde504c948f774d171b7991bcaa8 Mon Sep 17 00:00:00 2001 From: Unknown Date: Wed, 6 Sep 2017 16:51:58 +1200 Subject: [PATCH 2/2] Fixes issues noted in review --- .../providers/azure/azure_loadbalancer.go | 27 +++-- .../providers/azure/azure_test.go | 109 ++++++++++++++---- .../providers/azure/azure_util.go | 9 +- 3 files changed, 104 insertions(+), 41 deletions(-) diff --git a/pkg/cloudprovider/providers/azure/azure_loadbalancer.go b/pkg/cloudprovider/providers/azure/azure_loadbalancer.go index 5987fb409e..264067c808 100644 --- a/pkg/cloudprovider/providers/azure/azure_loadbalancer.go +++ b/pkg/cloudprovider/providers/azure/azure_loadbalancer.go @@ -374,7 +374,7 @@ func (az *Cloud) cleanupLoadBalancer(clusterName string, service *v1.Service, is if !isInternalLb { // Find public ip resource to clean up from IP configuration - lbFrontendIPConfigName := getFrontendIPConfigName(service, subnet(service)) + lbFrontendIPConfigName := getFrontendIPConfigName(service, nil) for _, config := range *lb.FrontendIPConfigurations { if strings.EqualFold(*config.Name, lbFrontendIPConfigName) { if config.PublicIPAddress != nil { @@ -576,19 +576,21 @@ func (az *Cloud) reconcileLoadBalancer(lb network.LoadBalancer, fipConfiguration if !wantLb { for i := len(newConfigs) - 1; i >= 0; i-- { config := newConfigs[i] - if strings.EqualFold(*config.Name, lbFrontendIPConfigName) { + if serviceOwnsFrontendIP(config, service) { glog.V(3).Infof("reconcile(%s)(%t): lb frontendconfig(%s) - dropping", serviceName, wantLb, lbFrontendIPConfigName) newConfigs = append(newConfigs[:i], newConfigs[i+1:]...) dirtyConfigs = true } } } else { - for i := len(newConfigs) - 1; i >= 0; i-- { - config := newConfigs[i] - if serviceOwnsFrontEndIP(config, service) && !strings.EqualFold(*config.Name, lbFrontendIPConfigName) { - glog.V(3).Infof("reconcile(%s)(%t): lb frontendconfig(%s) - dropping", serviceName, wantLb, *config.Name) - newConfigs = append(newConfigs[:i], newConfigs[i+1:]...) - dirtyConfigs = true + if isInternal { + for i := len(newConfigs) - 1; i >= 0; i-- { + config := newConfigs[i] + if serviceOwnsFrontendIP(config, service) && !strings.EqualFold(*config.Name, lbFrontendIPConfigName) { + glog.V(3).Infof("reconcile(%s)(%t): lb frontendconfig(%s) - dropping", serviceName, wantLb, *config.Name) + newConfigs = append(newConfigs[:i], newConfigs[i+1:]...) + dirtyConfigs = true + } } } foundConfig := false @@ -814,7 +816,7 @@ func (az *Cloud) reconcileSecurityGroup(sg network.SecurityGroup, clusterName st } for j := range sourceAddressPrefixes { ix := i*len(sourceAddressPrefixes) + j - securityRuleName := getSecurityRuleName(service, port, subnet(service), sourceAddressPrefixes[j]) + securityRuleName := getSecurityRuleName(service, port, sourceAddressPrefixes[j]) expectedSecurityRules[ix] = network.SecurityRule{ Name: to.StringPtr(securityRuleName), SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{ @@ -1013,8 +1015,11 @@ func requiresInternalLoadBalancer(service *v1.Service) bool { } func subnet(service *v1.Service) *string { - if l, ok := service.Annotations[ServiceAnnotationLoadBalancerInternalSubnet]; ok { - return &l + if requiresInternalLoadBalancer(service) { + if l, ok := service.Annotations[ServiceAnnotationLoadBalancerInternalSubnet]; ok { + return &l + } } + return nil } diff --git a/pkg/cloudprovider/providers/azure/azure_test.go b/pkg/cloudprovider/providers/azure/azure_test.go index e8e1525a50..d7a560cb8b 100644 --- a/pkg/cloudprovider/providers/azure/azure_test.go +++ b/pkg/cloudprovider/providers/azure/azure_test.go @@ -67,22 +67,15 @@ func TestReconcileLoadBalancerAddPort(t *testing.T) { validateLoadBalancer(t, lb, svc) } -// Test additional of a new service/port on an internal LB with a subnet. -func TestReconcileLoadBalancerAddPortOnInternalSubnet(t *testing.T) { +// Test addition of a new service on an internal LB with a subnet. +func TestReconcileLoadBalancerAddServiceOnInternalSubnet(t *testing.T) { az := getTestCloud() - svc := getTestService("servicea", v1.ProtocolTCP, 80) - addTestSubnet(&svc) - configProperties := getTestPublicFipConfigurationProperties() + svc := getInternalTestService("servicea", 80) + addTestSubnet(t, &svc) + configProperties := getTestInternalFipConfigurationProperties(to.StringPtr("TestSubnet")) lb := getTestLoadBalancer() nodes := []*v1.Node{} - svc.Spec.Ports = append(svc.Spec.Ports, v1.ServicePort{ - Name: fmt.Sprintf("port-udp-%d", 1234), - Protocol: v1.ProtocolUDP, - Port: 1234, - NodePort: getBackendPort(1234), - }) - lb, updated, err := az.reconcileLoadBalancer(lb, &configProperties, testClusterName, &svc, nodes) if err != nil { t.Errorf("Unexpected error: %q", err) @@ -104,18 +97,19 @@ func TestReconcileLoadBalancerAddPortOnInternalSubnet(t *testing.T) { func TestReconcileLoadBalancerAddServicesOnMultipleSubnets(t *testing.T) { az := getTestCloud() svc1 := getTestService("service1", v1.ProtocolTCP, 8081) - svc2 := getTestService("service2", v1.ProtocolTCP, 8081) - addTestSubnet(&svc2) - configProperties := getTestPublicFipConfigurationProperties() + svc2 := getInternalTestService("service2", 8081) + addTestSubnet(t, &svc2) + configProperties1 := getTestPublicFipConfigurationProperties() + configProperties2 := getTestInternalFipConfigurationProperties(to.StringPtr("TestSubnet")) lb := getTestLoadBalancer() nodes := []*v1.Node{} - lb, updated, err := az.reconcileLoadBalancer(lb, &configProperties, testClusterName, &svc1, nodes) + lb, updated, err := az.reconcileLoadBalancer(lb, &configProperties1, testClusterName, &svc1, nodes) if err != nil { t.Errorf("Unexpected error reconciling svc1: %q", err) } - lb, updated, err = az.reconcileLoadBalancer(lb, &configProperties, testClusterName, &svc2, nodes) + lb, updated, err = az.reconcileLoadBalancer(lb, &configProperties2, testClusterName, &svc2, nodes) if err != nil { t.Errorf("Unexpected error reconciling svc2: %q", err) } @@ -135,9 +129,9 @@ func TestReconcileLoadBalancerAddServicesOnMultipleSubnets(t *testing.T) { // Test moving a service exposure from one subnet to another. func TestReconcileLoadBalancerEditServiceSubnet(t *testing.T) { az := getTestCloud() - svc := getTestService("service1", v1.ProtocolTCP, 8081) - addTestSubnet(&svc) - configProperties := getTestPublicFipConfigurationProperties() + svc := getInternalTestService("service1", 8081) + addTestSubnet(t, &svc) + configProperties := getTestInternalFipConfigurationProperties(to.StringPtr("TestSubnet")) lb := getTestLoadBalancer() nodes := []*v1.Node{} @@ -146,7 +140,10 @@ func TestReconcileLoadBalancerEditServiceSubnet(t *testing.T) { t.Errorf("Unexpected error reconciling initial svc: %q", err) } + validateLoadBalancer(t, lb, svc) + svc.Annotations[ServiceAnnotationLoadBalancerInternalSubnet] = "NewSubnet" + configProperties = getTestInternalFipConfigurationProperties(to.StringPtr("NewSubnet")) lb, updated, err = az.reconcileLoadBalancer(lb, &configProperties, testClusterName, &svc, nodes) if err != nil { @@ -397,6 +394,17 @@ func getTestPublicFipConfigurationProperties() network.FrontendIPConfigurationPr } } +func getTestInternalFipConfigurationProperties(expectedSubnetName *string) network.FrontendIPConfigurationPropertiesFormat { + var expectedSubnet *network.Subnet + if expectedSubnetName != nil { + expectedSubnet = &network.Subnet{Name: expectedSubnetName} + } + return network.FrontendIPConfigurationPropertiesFormat{ + PublicIPAddress: &network.PublicIPAddress{ID: to.StringPtr("/this/is/a/public/ip/address/id")}, + Subnet: expectedSubnet, + } +} + func getTestService(identifier string, proto v1.Protocol, requestedPorts ...int32) v1.Service { ports := []v1.ServicePort{} for _, port := range requestedPorts { @@ -479,7 +487,7 @@ func getTestSecurityGroup(services ...v1.Service) network.SecurityGroup { for _, port := range service.Spec.Ports { sources := getServiceSourceRanges(&service) for _, src := range sources { - ruleName := getSecurityRuleName(&service, port, nil, src) + ruleName := getSecurityRuleName(&service, port, src) rules = append(rules, network.SecurityRule{ Name: to.StringPtr(ruleName), SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{ @@ -504,9 +512,15 @@ func validateLoadBalancer(t *testing.T, loadBalancer network.LoadBalancer, servi expectedRuleCount := 0 expectedFrontendIPCount := 0 expectedProbeCount := 0 + expectedFrontendIPs := []ExpectedFrontendIPInfo{} for _, svc := range services { if len(svc.Spec.Ports) > 0 { expectedFrontendIPCount++ + expectedFrontendIP := ExpectedFrontendIPInfo{ + Name: getFrontendIPConfigName(&svc, subnet(&svc)), + Subnet: subnet(&svc), + } + expectedFrontendIPs = append(expectedFrontendIPs, expectedFrontendIP) } for _, wantedRule := range svc.Spec.Ports { expectedRuleCount++ @@ -565,6 +579,13 @@ func validateLoadBalancer(t *testing.T, loadBalancer network.LoadBalancer, servi t.Errorf("Expected the loadbalancer to have %d frontend IPs. Found %d.\n%v", expectedFrontendIPCount, frontendIPCount, loadBalancer.FrontendIPConfigurations) } + frontendIPs := *loadBalancer.FrontendIPConfigurations + for _, expectedFrontendIP := range expectedFrontendIPs { + if !expectedFrontendIP.existsIn(frontendIPs) { + t.Errorf("Expected the loadbalancer to have frontend IP %s/%s. Found %s", expectedFrontendIP.Name, to.String(expectedFrontendIP.Subnet), describeFIPs(frontendIPs)) + } + } + lenRules := len(*loadBalancer.LoadBalancingRules) if lenRules != expectedRuleCount { t.Errorf("Expected the loadbalancer to have %d rules. Found %d.\n%v", expectedRuleCount, lenRules, loadBalancer.LoadBalancingRules) @@ -576,13 +597,51 @@ func validateLoadBalancer(t *testing.T, loadBalancer network.LoadBalancer, servi } } +type ExpectedFrontendIPInfo struct { + Name string + Subnet *string +} + +func (expected ExpectedFrontendIPInfo) matches(frontendIP network.FrontendIPConfiguration) bool { + return strings.EqualFold(expected.Name, to.String(frontendIP.Name)) && strings.EqualFold(to.String(expected.Subnet), to.String(subnetName(frontendIP))) +} + +func (expected ExpectedFrontendIPInfo) existsIn(frontendIPs []network.FrontendIPConfiguration) bool { + for _, fip := range frontendIPs { + if expected.matches(fip) { + return true + } + } + return false +} + +func subnetName(frontendIP network.FrontendIPConfiguration) *string { + if frontendIP.Subnet != nil { + return frontendIP.Subnet.Name + } + return nil +} + +func describeFIPs(frontendIPs []network.FrontendIPConfiguration) string { + description := "" + for _, actualFIP := range frontendIPs { + actualSubnetName := "" + if actualFIP.Subnet != nil { + actualSubnetName = to.String(actualFIP.Subnet.Name) + } + actualFIPText := fmt.Sprintf("%s/%s ", to.String(actualFIP.Name), actualSubnetName) + description = description + actualFIPText + } + return description +} + func validateSecurityGroup(t *testing.T, securityGroup network.SecurityGroup, services ...v1.Service) { expectedRuleCount := 0 for _, svc := range services { for _, wantedRule := range svc.Spec.Ports { sources := getServiceSourceRanges(&svc) for _, source := range sources { - wantedRuleName := getSecurityRuleName(&svc, wantedRule, nil, source) + wantedRuleName := getSecurityRuleName(&svc, wantedRule, source) expectedRuleCount++ foundRule := false for _, actualRule := range *securityGroup.SecurityRules { @@ -986,7 +1045,9 @@ func TestMetadataParsing(t *testing.T) { } } -func addTestSubnet(svc *v1.Service) { - svc.Annotations[ServiceAnnotationLoadBalancerInternal] = "true" +func addTestSubnet(t *testing.T, svc *v1.Service) { + if svc.Annotations[ServiceAnnotationLoadBalancerInternal] != "true" { + t.Error("Subnet added to non-internal service") + } svc.Annotations[ServiceAnnotationLoadBalancerInternalSubnet] = "TestSubnet" } diff --git a/pkg/cloudprovider/providers/azure/azure_util.go b/pkg/cloudprovider/providers/azure/azure_util.go index 7a6b48e8d9..2f46d46aba 100644 --- a/pkg/cloudprovider/providers/azure/azure_util.go +++ b/pkg/cloudprovider/providers/azure/azure_util.go @@ -202,12 +202,9 @@ func getLoadBalancerRuleName(service *v1.Service, port v1.ServicePort, subnetNam return fmt.Sprintf("%s-%s-%s-%d", getRulePrefix(service), *subnetName, port.Protocol, port.Port) } -func getSecurityRuleName(service *v1.Service, port v1.ServicePort, subnetName *string, sourceAddrPrefix string) string { +func getSecurityRuleName(service *v1.Service, port v1.ServicePort, sourceAddrPrefix string) string { safePrefix := strings.Replace(sourceAddrPrefix, "/", "_", -1) - if subnetName == nil { - return fmt.Sprintf("%s-%s-%d-%s", getRulePrefix(service), port.Protocol, port.Port, safePrefix) - } - return fmt.Sprintf("%s-%s-%s-%d-%s", getRulePrefix(service), *subnetName, port.Protocol, port.Port, safePrefix) + return fmt.Sprintf("%s-%s-%d-%s", getRulePrefix(service), port.Protocol, port.Port, safePrefix) } // This returns a human-readable version of the Service used to tag some resources. @@ -230,7 +227,7 @@ func serviceOwnsRule(service *v1.Service, rule string) bool { return strings.HasPrefix(strings.ToUpper(rule), strings.ToUpper(prefix)) } -func serviceOwnsFrontEndIP(fip network.FrontendIPConfiguration, service *v1.Service) bool { +func serviceOwnsFrontendIP(fip network.FrontendIPConfiguration, service *v1.Service) bool { baseName := cloudprovider.GetLoadBalancerName(service) return strings.HasPrefix(*fip.Name, baseName) }