From 1ea5a692fea037e23053b000e143669d12637d44 Mon Sep 17 00:00:00 2001 From: Pengfei Ni Date: Wed, 8 May 2019 19:31:36 +0800 Subject: [PATCH] Add support of shared resource group for Azure public IP To support this, a new tag "kubernetes-cluster-name" is added to public IP which indicates the kubernetes cluster name (set in kube-controller-manager). --- .../azure/azure_loadbalancer.go | 41 ++++++++-- .../azure/azure_loadbalancer_test.go | 79 +++++++++++++++++++ .../azure/azure_test.go | 17 +++- 3 files changed, 126 insertions(+), 11 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 58f3bb637ae..03601209734 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 @@ -83,6 +83,11 @@ const ( // ServiceAnnotationLoadBalancerMixedProtocols is the annotation used on the service // to create both TCP and UDP protocols when creating load balancer rules. ServiceAnnotationLoadBalancerMixedProtocols = "service.beta.kubernetes.io/azure-load-balancer-mixed-protocols" + + // serviceTagKey is the service key applied for public IP tags. + serviceTagKey = "service" + // clusterNameKey is the cluster name key applied for public IP tags. + clusterNameKey = "kubernetes-cluster-name" ) var ( @@ -465,7 +470,7 @@ func (az *Cloud) findServiceIPAddress(ctx context.Context, clusterName string, s return lbStatus.Ingress[0].IP, nil } -func (az *Cloud) ensurePublicIPExists(service *v1.Service, pipName string, domainNameLabel string) (*network.PublicIPAddress, error) { +func (az *Cloud) ensurePublicIPExists(service *v1.Service, pipName string, domainNameLabel, clusterName string) (*network.PublicIPAddress, error) { pipResourceGroup := az.getPublicIPAddressResourceGroup(service) pip, existsPip, err := az.getPublicIPAddress(pipResourceGroup, pipName) if err != nil { @@ -486,7 +491,10 @@ func (az *Cloud) ensurePublicIPExists(service *v1.Service, pipName string, domai DomainNameLabel: &domainNameLabel, } } - pip.Tags = map[string]*string{"service": &serviceName} + pip.Tags = map[string]*string{ + serviceTagKey: &serviceName, + clusterNameKey: &clusterName, + } if az.useStandardLoadBalancer() { pip.Sku = &network.PublicIPAddressSku{ Name: network.PublicIPAddressSkuNameStandard, @@ -711,7 +719,7 @@ func (az *Cloud) reconcileLoadBalancer(clusterName string, service *v1.Service, return nil, err } domainNameLabel := getPublicIPDomainNameLabel(service) - pip, err := az.ensurePublicIPExists(service, pipName, domainNameLabel) + pip, err := az.ensurePublicIPExists(service, pipName, domainNameLabel, clusterName) if err != nil { return nil, err } @@ -1344,9 +1352,7 @@ func (az *Cloud) reconcilePublicIP(clusterName string, service *v1.Service, lb * for i := range pips { pip := pips[i] - if pip.Tags != nil && - (pip.Tags)["service"] != nil && - *(pip.Tags)["service"] == serviceName { + if serviceOwnsPublicIP(&pip, clusterName, serviceName) { // We need to process for pips belong to this service pipName := *pip.Name if wantLb && !isInternal && pipName == desiredPipName { @@ -1369,7 +1375,7 @@ func (az *Cloud) reconcilePublicIP(clusterName string, service *v1.Service, lb * // Confirm desired public ip resource exists var pip *network.PublicIPAddress domainNameLabel := getPublicIPDomainNameLabel(service) - if pip, err = az.ensurePublicIPExists(service, desiredPipName, domainNameLabel); err != nil { + if pip, err = az.ensurePublicIPExists(service, desiredPipName, domainNameLabel, clusterName); err != nil { return nil, err } return pip, nil @@ -1612,3 +1618,24 @@ func getServiceTags(service *v1.Service) ([]string, error) { return nil, nil } + +func serviceOwnsPublicIP(pip *network.PublicIPAddress, clusterName, serviceName string) bool { + if pip != nil && pip.Tags != nil { + serviceTag := pip.Tags[serviceTagKey] + clusterTag := pip.Tags[clusterNameKey] + + if serviceTag != nil && *serviceTag == serviceName { + // Backward compatible for clusters upgraded from old releases. + // In such case, only "service" tag is set. + if clusterTag == nil { + return true + } + + // If cluster name tag is set, then return true if it matches. + if *clusterTag == clusterName { + return true + } + } + } + return false +} 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 06c26291c22..0efb7ee2974 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 @@ -368,3 +368,82 @@ func TestEnsureLoadBalancerDeleted(t *testing.T) { assert.Equal(t, len(result), 0, "TestCase[%d]: %s", i, c.desc) } } + +func TestServiceOwnsPublicIP(t *testing.T) { + tests := []struct { + desc string + pip *network.PublicIPAddress + clusterName string + serviceName string + expected bool + }{ + { + desc: "false should be returned when pip is nil", + clusterName: "kubernetes", + serviceName: "nginx", + expected: false, + }, + { + desc: "false should be returned when service name tag doesn't match", + pip: &network.PublicIPAddress{ + Tags: map[string]*string{ + serviceTagKey: to.StringPtr("nginx"), + }, + }, + serviceName: "web", + expected: false, + }, + { + desc: "true should be returned when service name tag matches and cluster name tag is not set", + pip: &network.PublicIPAddress{ + Tags: map[string]*string{ + serviceTagKey: to.StringPtr("nginx"), + }, + }, + clusterName: "kubernetes", + serviceName: "nginx", + expected: true, + }, + { + desc: "false should be returned when cluster name doesn't match", + pip: &network.PublicIPAddress{ + Tags: map[string]*string{ + serviceTagKey: to.StringPtr("nginx"), + clusterNameKey: to.StringPtr("kubernetes"), + }, + }, + clusterName: "k8s", + serviceName: "nginx", + expected: false, + }, + { + desc: "false should be returned when cluster name matches while service name doesn't match", + pip: &network.PublicIPAddress{ + Tags: map[string]*string{ + serviceTagKey: to.StringPtr("web"), + clusterNameKey: to.StringPtr("kubernetes"), + }, + }, + clusterName: "kubernetes", + serviceName: "nginx", + expected: false, + }, + { + desc: "true should be returned when both service name tag and cluster name match", + pip: &network.PublicIPAddress{ + Tags: map[string]*string{ + serviceTagKey: to.StringPtr("nginx"), + clusterNameKey: to.StringPtr("kubernetes"), + }, + }, + clusterName: "kubernetes", + serviceName: "nginx", + expected: true, + }, + } + + for i, c := range tests { + owns := serviceOwnsPublicIP(c.pip, c.clusterName, c.serviceName) + assert.Equal(t, owns, c.expected, "TestCase[%d]: %s", i, c.desc) + } +} 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 efcfbad5602..9a8ec28b1eb 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 @@ -1367,14 +1367,23 @@ func validatePublicIP(t *testing.T, publicIP *network.PublicIPAddress, service * t.Errorf("Expected publicIP resource exists, when it is not an internal service") } - if publicIP.Tags == nil || publicIP.Tags["service"] == nil { - t.Errorf("Expected publicIP resource has tags[service]") + if publicIP.Tags == nil || publicIP.Tags[serviceTagKey] == nil { + t.Errorf("Expected publicIP resource has tags[%s]", serviceTagKey) } serviceName := getServiceName(service) - if serviceName != *(publicIP.Tags["service"]) { - t.Errorf("Expected publicIP resource has matching tags[service]") + if serviceName != *(publicIP.Tags[serviceTagKey]) { + t.Errorf("Expected publicIP resource has matching tags[%s]", serviceTagKey) } + + if publicIP.Tags[clusterNameKey] == nil { + t.Errorf("Expected publicIP resource has tags[%s]", clusterNameKey) + } + + if *(publicIP.Tags[clusterNameKey]) != testClusterName { + t.Errorf("Expected publicIP resource has matching tags[%s]", clusterNameKey) + } + // We cannot use service.Spec.LoadBalancerIP to compare with // Public IP's IPAddress // Because service properties are updated outside of cloudprovider code