Fixes issues noted in review

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,21 +576,23 @@ 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 {
+		if isInternal {
 			for i := len(newConfigs) - 1; i >= 0; i-- {
 				config := newConfigs[i]
-			if serviceOwnsFrontEndIP(config, service) && !strings.EqualFold(*config.Name, lbFrontendIPConfigName) {
+				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) {
@@ -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 requiresInternalLoadBalancer(service) {
 		if l, ok := service.Annotations[ServiceAnnotationLoadBalancerInternalSubnet]; ok {
 			return &l
 		}
+	}
+
 	return nil
 }

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.
+// Test addition of a new service on an internal LB with a subnet.
-func TestReconcileLoadBalancerAddPortOnInternalSubnet(t *testing.T) {
+func TestReconcileLoadBalancerAddServiceOnInternalSubnet(t *testing.T) {
 	az := getTestCloud()
-	svc := getTestService("servicea", v1.ProtocolTCP, 80)
+	svc := getInternalTestService("servicea", 80)
-	addTestSubnet(&svc)
+	addTestSubnet(t, &svc)
-	configProperties := getTestPublicFipConfigurationProperties()
+	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)
+	svc2 := getInternalTestService("service2", 8081)
-	addTestSubnet(&svc2)
+	addTestSubnet(t, &svc2)
-	configProperties := getTestPublicFipConfigurationProperties()
+	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)
+	svc := getInternalTestService("service1", 8081)
-	addTestSubnet(&svc)
+	addTestSubnet(t, &svc)
-	configProperties := getTestPublicFipConfigurationProperties()
+	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) {
+func addTestSubnet(t *testing.T, svc *v1.Service) {
-	svc.Annotations[ServiceAnnotationLoadBalancerInternal] = "true"
+	if svc.Annotations[ServiceAnnotationLoadBalancerInternal] != "true" {
+		t.Error("Subnet added to non-internal service")
+	}
 	svc.Annotations[ServiceAnnotationLoadBalancerInternalSubnet] = "TestSubnet"
 }

pkg/cloudprovider/providers/azure/azure_util.go

@@ -202,13 +202,10 @@ 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)
-}
 
 // This returns a human-readable version of the Service used to tag some resources.
 // This is only used for human-readable convenience, and not to filter.
@@ -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)
 }