Add E2E tests for Azure internal loadbalancer support, fix an issue for public IP resource deletion.

pkg/cloudprovider/providers/azure/azure_loadbalancer.go

@@ -24,7 +24,6 @@ import (
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/kubernetes/pkg/api/v1"
 	serviceapi "k8s.io/kubernetes/pkg/api/v1/service"
-	"k8s.io/kubernetes/pkg/cloudprovider"
 
 	"github.com/Azure/azure-sdk-for-go/arm/network"
 	"github.com/Azure/go-autorest/autorest/to"
@@ -63,7 +62,7 @@ func (az *Cloud) GetLoadBalancer(clusterName string, service *v1.Service) (statu
 		}
 	} else {
 		// TODO: Consider also read address from lb's FrontendIPConfigurations
-		pipName, err := az.getPublicIPName(clusterName, service)
+		pipName, err := az.determinePublicIPName(clusterName, service)
 		if err != nil {
 			return nil, false, err
 		}
@@ -86,10 +85,10 @@ func (az *Cloud) GetLoadBalancer(clusterName string, service *v1.Service) (statu
 	}, true, nil
 }
 
-func (az *Cloud) getPublicIPName(clusterName string, service *v1.Service) (string, error) {
+func (az *Cloud) determinePublicIPName(clusterName string, service *v1.Service) (string, error) {
 	loadBalancerIP := service.Spec.LoadBalancerIP
 	if len(loadBalancerIP) == 0 {
-		return fmt.Sprintf("%s-%s", clusterName, cloudprovider.GetLoadBalancerName(service)), nil
+		return getPublicIPName(clusterName, service), nil
 	}
 
 	list, err := az.PublicIPAddressesClient.List(az.ResourceGroup)
@@ -124,14 +123,6 @@ func (az *Cloud) EnsureLoadBalancer(clusterName string, service *v1.Service, nod
 		return nil, err
 	}
 
-	// Also clean up public ip resource, since service might be switched from public load balancer type.
-	if isInternal {
-		err = az.cleanupPublicIP(clusterName, service)
-		if err != nil {
-			return nil, err
-		}
-	}
-
 	serviceName := getServiceName(service)
 	glog.V(5).Infof("ensure(%s): START clusterName=%q lbName=%q", serviceName, clusterName, lbName)
 
@@ -198,7 +189,7 @@ func (az *Cloud) EnsureLoadBalancer(clusterName string, service *v1.Service, nod
 
 		fipConfigurationProperties = &configProperties
 	} else {
-		pipName, err := az.getPublicIPName(clusterName, service)
+		pipName, err := az.determinePublicIPName(clusterName, service)
 		if err != nil {
 			return nil, err
 		}
@@ -288,13 +279,6 @@ func (az *Cloud) EnsureLoadBalancerDeleted(clusterName string, service *v1.Servi
 		return err
 	}
 
-	if !isInternal {
-		err = az.cleanupPublicIP(clusterName, service)
-		if err != nil {
-			return err
-		}
-	}
-
 	sg, existsSg, err := az.getSecurityGroup()
 	if err != nil {
 		return err
@@ -332,6 +316,22 @@ func (az *Cloud) cleanupLoadBalancer(clusterName string, service *v1.Service, is
 		return err
 	}
 	if existsLb {
+		var publicIPToCleanup *string
+
+		if !isInternalLb {
+			// Find public ip resource to clean up from IP configuration
+			lbFrontendIPConfigName := getFrontendIPConfigName(service)
+			for _, config := range *lb.FrontendIPConfigurations {
+				if strings.EqualFold(*config.Name, lbFrontendIPConfigName) {
+					if config.PublicIPAddress != nil {
+						// Only ID property is available
+						publicIPToCleanup = config.PublicIPAddress.ID
+					}
+					break
+				}
+			}
+		}
+
 		lb, lbNeedsUpdate, reconcileErr := az.reconcileLoadBalancer(lb, nil, clusterName, service, []*v1.Node{})
 		if reconcileErr != nil {
 			return reconcileErr
@@ -352,23 +352,23 @@ func (az *Cloud) cleanupLoadBalancer(clusterName string, service *v1.Service, is
 				}
 			}
 		}
-	}
-
-	return nil
-}
-
-func (az *Cloud) cleanupPublicIP(clusterName string, service *v1.Service) error {
-	serviceName := getServiceName(service)
 
-	// Only delete an IP address if we created it.
-	if service.Spec.LoadBalancerIP == "" {
-		pipName, err := az.getPublicIPName(clusterName, service)
-		if err != nil {
-			return err
-		}
-		err = az.ensurePublicIPDeleted(serviceName, pipName)
-		if err != nil {
-			return err
+		// Public IP can be deleted after frontend ip configuration rule deleted.
+		if publicIPToCleanup != nil {
+			// Only delete an IP address if we created it, deducing by name.
+			if index := strings.LastIndex(*publicIPToCleanup, "/"); index != -1 {
+				managedPipName := getPublicIPName(clusterName, service)
+				pipName := (*publicIPToCleanup)[index+1:]
+				if strings.EqualFold(managedPipName, pipName) {
+					glog.V(5).Infof("Deleting public IP resource %q.", pipName)
+					err = az.ensurePublicIPDeleted(serviceName, pipName)
+					if err != nil {
+						return err
+					}
+				} else {
+					glog.V(5).Infof("Public IP resource %q found, but it does not match managed name %q, skip deleting.", pipName, managedPipName)
+				}
+			}
 		}
 	}
 

pkg/cloudprovider/providers/azure/azure_util.go

@@ -163,6 +163,7 @@ func getPrimaryIPConfig(nic network.Interface) (*network.InterfaceIPConfiguratio
 // For a load balancer, all frontend ip should reference either a subnet or publicIpAddress.
 // Thus Azure do not allow mixed type (public and internal) load balancer.
 // So we'd have a separate name for internal load balancer.
+// This would be the name for Azure LoadBalancer resource.
 func getLoadBalancerName(clusterName string, isInternal bool) string {
 	if isInternal {
 		return fmt.Sprintf("%s-internal", clusterName)
@@ -190,6 +191,10 @@ func getRulePrefix(service *v1.Service) string {
 	return cloudprovider.GetLoadBalancerName(service)
 }
 
+func getPublicIPName(clusterName string, service *v1.Service) string {
+	return fmt.Sprintf("%s-%s", clusterName, cloudprovider.GetLoadBalancerName(service))
+}
+
 func serviceOwnsRule(service *v1.Service, rule string) bool {
 	prefix := getRulePrefix(service)
 	return strings.HasPrefix(strings.ToUpper(rule), strings.ToUpper(prefix))

test/e2e/service.go

@@ -1241,6 +1241,77 @@ var _ = framework.KubeDescribe("Services", func() {
 		framework.CheckReachabilityFromPod(true, normalReachabilityTimeout, namespace, acceptPodName, svcIP)
 		framework.CheckReachabilityFromPod(true, normalReachabilityTimeout, namespace, dropPodName, svcIP)
 	})
+
+	It("should be able to create an internal type load balancer on Azure [Slow]", func() {
+		framework.SkipUnlessProviderIs("azure")
+
+		createTimeout := framework.LoadBalancerCreateTimeoutDefault
+		pollInterval := framework.Poll * 10
+
+		serviceAnnotationLoadBalancerInternal := "service.beta.kubernetes.io/azure-load-balancer-internal"
+		namespace := f.Namespace.Name
+		serviceName := "lb-internal"
+		jig := framework.NewServiceTestJig(cs, serviceName)
+
+		isInternalEndpoint := func(lbIngress *v1.LoadBalancerIngress) bool {
+			ingressEndpoint := framework.GetIngressPoint(lbIngress)
+			// Needs update for providers using hostname as endpoint.
+			return strings.HasPrefix(ingressEndpoint, "10.")
+		}
+
+		By("creating a service with type LoadBalancer and LoadBalancerInternal annotation set to true")
+		svc := jig.CreateTCPServiceOrFail(namespace, func(svc *v1.Service) {
+			svc.Spec.Type = v1.ServiceTypeLoadBalancer
+			svc.ObjectMeta.Annotations = map[string]string{
+				serviceAnnotationLoadBalancerInternal: "true",
+			}
+		})
+		svc = jig.WaitForLoadBalancerOrFail(namespace, serviceName, createTimeout)
+		jig.SanityCheckService(svc, v1.ServiceTypeLoadBalancer)
+		lbIngress := &svc.Status.LoadBalancer.Ingress[0]
+		// should have an internal IP.
+		Expect(isInternalEndpoint(lbIngress)).To(BeTrue())
+
+		By("switiching to external type LoadBalancer")
+		svc = jig.UpdateServiceOrFail(namespace, serviceName, func(svc *v1.Service) {
+			svc.ObjectMeta.Annotations[serviceAnnotationLoadBalancerInternal] = "false"
+		})
+		framework.Logf("Waiting up to %v for service %q to have an external LoadBalancer", createTimeout, serviceName)
+		if pollErr := wait.PollImmediate(pollInterval, createTimeout, func() (bool, error) {
+			svc, err := jig.Client.Core().Services(namespace).Get(serviceName, metav1.GetOptions{})
+			if err != nil {
+				return false, err
+			}
+			lbIngress = &svc.Status.LoadBalancer.Ingress[0]
+			return !isInternalEndpoint(lbIngress), nil
+		}); pollErr != nil {
+			framework.Failf("Loadbalancer IP not changed to external.")
+		}
+		// should have an external IP.
+		jig.SanityCheckService(svc, v1.ServiceTypeLoadBalancer)
+		Expect(isInternalEndpoint(lbIngress)).To(BeFalse())
+
+		By("switiching back to interal type LoadBalancer, with static IP specified.")
+		internalStaticIP := "10.240.11.11"
+		svc = jig.UpdateServiceOrFail(namespace, serviceName, func(svc *v1.Service) {
+			svc.Spec.LoadBalancerIP = internalStaticIP
+			svc.ObjectMeta.Annotations[serviceAnnotationLoadBalancerInternal] = "true"
+		})
+		framework.Logf("Waiting up to %v for service %q to have an internal LoadBalancer", createTimeout, serviceName)
+		if pollErr := wait.PollImmediate(pollInterval, createTimeout, func() (bool, error) {
+			svc, err := jig.Client.Core().Services(namespace).Get(serviceName, metav1.GetOptions{})
+			if err != nil {
+				return false, err
+			}
+			lbIngress = &svc.Status.LoadBalancer.Ingress[0]
+			return isInternalEndpoint(lbIngress), nil
+		}); pollErr != nil {
+			framework.Failf("Loadbalancer IP not changed to internal.")
+		}
+		// should have the given static internal IP.
+		jig.SanityCheckService(svc, v1.ServiceTypeLoadBalancer)
+		Expect(framework.GetIngressPoint(lbIngress)).To(Equal(internalStaticIP))
+	})
 })
 
 var _ = framework.KubeDescribe("ESIPP [Slow]", func() {