From afd31035c219529edce87d4a946176ca42c4a477 Mon Sep 17 00:00:00 2001 From: t-qini Date: Wed, 15 Jan 2020 22:52:03 +0800 Subject: [PATCH] Fix the bug PIP's DNS is deleted if no DNS label service annotation is set. --- .../azure/azure_loadbalancer.go | 28 ++++---- .../azure/azure_loadbalancer_test.go | 66 +++++++++++++++---- 2 files changed, 70 insertions(+), 24 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 995a4000c9b..d3f70a0de3d 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 @@ -137,11 +137,11 @@ func (az *Cloud) GetLoadBalancer(ctx context.Context, clusterName string, servic return status, true, nil } -func getPublicIPDomainNameLabel(service *v1.Service) string { +func getPublicIPDomainNameLabel(service *v1.Service) (string, bool) { if labelName, found := service.Annotations[ServiceAnnotationDNSLabelName]; found { - return labelName + return labelName, found } - return "" + return "", false } // EnsureLoadBalancer creates a new load balancer 'name', or updates the existing one. Returns the status of the balancer @@ -498,7 +498,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, clusterName string, shouldPIPExisted bool) (*network.PublicIPAddress, error) { +func (az *Cloud) ensurePublicIPExists(service *v1.Service, pipName string, domainNameLabel, clusterName string, shouldPIPExisted, foundDNSLabelAnnotation bool) (*network.PublicIPAddress, error) { pipResourceGroup := az.getPublicIPAddressResourceGroup(service) pip, existsPip, err := az.getPublicIPAddress(pipResourceGroup, pipName) if err != nil { @@ -538,11 +538,13 @@ func (az *Cloud) ensurePublicIPExists(service *v1.Service, pipName string, domai } klog.V(2).Infof("ensurePublicIPExists for service(%s): pip(%s) - creating", serviceName, *pip.Name) } - if len(domainNameLabel) == 0 { - pip.PublicIPAddressPropertiesFormat.DNSSettings = nil - } else { - pip.PublicIPAddressPropertiesFormat.DNSSettings = &network.PublicIPAddressDNSSettings{ - DomainNameLabel: &domainNameLabel, + if foundDNSLabelAnnotation { + if len(domainNameLabel) == 0 { + pip.PublicIPAddressPropertiesFormat.DNSSettings = nil + } else { + pip.PublicIPAddressPropertiesFormat.DNSSettings = &network.PublicIPAddressDNSSettings{ + DomainNameLabel: &domainNameLabel, + } } } @@ -807,8 +809,8 @@ func (az *Cloud) reconcileLoadBalancer(clusterName string, service *v1.Service, if err != nil { return nil, err } - domainNameLabel := getPublicIPDomainNameLabel(service) - pip, err := az.ensurePublicIPExists(service, pipName, domainNameLabel, clusterName, shouldPIPExisted) + domainNameLabel, found := getPublicIPDomainNameLabel(service) + pip, err := az.ensurePublicIPExists(service, pipName, domainNameLabel, clusterName, shouldPIPExisted, found) if err != nil { return nil, err } @@ -1515,8 +1517,8 @@ func (az *Cloud) reconcilePublicIP(clusterName string, service *v1.Service, lbNa if !isInternal && wantLb { // Confirm desired public ip resource exists var pip *network.PublicIPAddress - domainNameLabel := getPublicIPDomainNameLabel(service) - if pip, err = az.ensurePublicIPExists(service, desiredPipName, domainNameLabel, clusterName, shouldPIPExisted); err != nil { + domainNameLabel, found := getPublicIPDomainNameLabel(service) + if pip, err = az.ensurePublicIPExists(service, desiredPipName, domainNameLabel, clusterName, shouldPIPExisted, found); err != nil { return nil, err } return pip, nil 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 3da7bfe14c2..8b971d2d17e 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 @@ -1983,8 +1983,8 @@ func TestReconcilePublicIP(t *testing.T) { pip, err := az.reconcilePublicIP("testCluster", &service, "", test.wantLb) if test.expectedID != "" { assert.Equal(t, test.expectedID, to.String(pip.ID), "TestCase[%d]: %s", i, test.desc) - } else { - assert.Equal(t, test.expectedPIP, pip, "TestCase[%d]: %s", i, test.desc) + } else if test.expectedPIP != nil && test.expectedPIP.Name != nil { + assert.Equal(t, *test.expectedPIP.Name, *pip.Name, "TestCase[%d]: %s", i, test.desc) } assert.Equal(t, test.expectedError, err != nil, "TestCase[%d]: %s", i, test.desc) } @@ -1992,12 +1992,13 @@ func TestReconcilePublicIP(t *testing.T) { func TestEnsurePublicIPExists(t *testing.T) { testCases := []struct { - desc string - existingPIPs []network.PublicIPAddress - expectedPIP *network.PublicIPAddress - expectedID string - expectedDNS string - expectedError bool + desc string + existingPIPs []network.PublicIPAddress + inputDNSLabel string + foundDNSLabelAnnotation bool + expectedPIP *network.PublicIPAddress + expectedID string + expectedError bool }{ { desc: "ensurePublicIPExists shall return existed PIP if there is any", @@ -2014,7 +2015,9 @@ func TestEnsurePublicIPExists(t *testing.T) { "Microsoft.Network/publicIPAddresses/pip1", }, { - desc: "ensurePublicIPExists shall update existed PIP's dns label", + desc: "ensurePublicIPExists shall update existed PIP's dns label", + inputDNSLabel: "newdns", + foundDNSLabelAnnotation: true, existingPIPs: []network.PublicIPAddress{{ Name: to.StringPtr("pip1"), PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{ @@ -2033,7 +2036,48 @@ func TestEnsurePublicIPExists(t *testing.T) { }, }, }, - expectedDNS: "newdns", + }, + { + desc: "ensurePublicIPExists shall delete DNS from PIP if DNS label is set empty", + foundDNSLabelAnnotation: true, + existingPIPs: []network.PublicIPAddress{{ + Name: to.StringPtr("pip1"), + PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{ + DNSSettings: &network.PublicIPAddressDNSSettings{ + DomainNameLabel: to.StringPtr("previousdns"), + }, + }, + }}, + expectedPIP: &network.PublicIPAddress{ + Name: to.StringPtr("pip1"), + ID: to.StringPtr("/subscriptions/subscription/resourceGroups/rg" + + "/providers/Microsoft.Network/publicIPAddresses/pip1"), + PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{ + DNSSettings: nil, + }, + }, + }, + { + desc: "ensurePublicIPExists shall not delete DNS from PIP if DNS label annotation is not set", + foundDNSLabelAnnotation: false, + existingPIPs: []network.PublicIPAddress{{ + Name: to.StringPtr("pip1"), + PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{ + DNSSettings: &network.PublicIPAddressDNSSettings{ + DomainNameLabel: to.StringPtr("previousdns"), + }, + }, + }}, + expectedPIP: &network.PublicIPAddress{ + Name: to.StringPtr("pip1"), + ID: to.StringPtr("/subscriptions/subscription/resourceGroups/rg" + + "/providers/Microsoft.Network/publicIPAddresses/pip1"), + PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{ + DNSSettings: &network.PublicIPAddressDNSSettings{ + DomainNameLabel: to.StringPtr("previousdns"), + }, + }, + }, }, } @@ -2046,7 +2090,7 @@ func TestEnsurePublicIPExists(t *testing.T) { t.Fatalf("TestCase[%d] meets unexpected error: %v", i, err) } } - pip, err := az.ensurePublicIPExists(&service, "pip1", test.expectedDNS, "", false) + pip, err := az.ensurePublicIPExists(&service, "pip1", test.inputDNSLabel, "", false, test.foundDNSLabelAnnotation) if test.expectedID != "" { assert.Equal(t, test.expectedID, to.String(pip.ID), "TestCase[%d]: %s", i, test.desc) } else {