From f8ae27db57b32b32b4a755c8c0ec19377ab5f423 Mon Sep 17 00:00:00 2001 From: Dong Liu Date: Mon, 8 May 2017 11:42:40 +0800 Subject: [PATCH] Add E2E tests for Azure internal loadbalancer support, fix an issue for public IP resource deletion. --- .../providers/azure/azure_loadbalancer.go | 72 +++++++++---------- .../providers/azure/azure_util.go | 5 ++ test/e2e/service.go | 71 ++++++++++++++++++ 3 files changed, 112 insertions(+), 36 deletions(-) diff --git a/pkg/cloudprovider/providers/azure/azure_loadbalancer.go b/pkg/cloudprovider/providers/azure/azure_loadbalancer.go index 42c6eeb2d36..55d91f4683f 100644 --- a/pkg/cloudprovider/providers/azure/azure_loadbalancer.go +++ b/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) + } + } } } diff --git a/pkg/cloudprovider/providers/azure/azure_util.go b/pkg/cloudprovider/providers/azure/azure_util.go index 0054e74d23f..7bfa37db285 100644 --- a/pkg/cloudprovider/providers/azure/azure_util.go +++ b/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)) diff --git a/test/e2e/service.go b/test/e2e/service.go index a68305d2690..0b6432f69e1 100644 --- a/test/e2e/service.go +++ b/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() {