Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cherry pick of #87246: Fix the bug PIP's DNS is deleted if no DNS label service annotation isn't set. #87311

Merged
merged 1 commit into from Jan 31, 2020
Merged

cherry pick of #87246: Fix the bug PIP's DNS is deleted if no DNS label service annotation isn't set. #87311

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 15 additions & 13 deletions staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go
View file
Open in desktop
Expand Up @@ -139,11 +139,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
Expand Down Expand Up @@ -515,7 +515,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 {
Expand Down Expand Up @@ -555,11 +555,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,
}
}
}

Expand Down Expand Up @@ -814,8 +816,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
}
Expand Down Expand Up @@ -1518,8 +1520,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
Expand Down
66 changes: 55 additions & 11 deletions staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go
View file
Open in desktop
Expand Up @@ -1930,21 +1930,22 @@ 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)
}
}

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",
Expand All @@ -1961,7 +1962,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{
Expand All @@ -1980,7 +1983,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"),
},
},
},
},
}

Expand All @@ -1993,7 +2037,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 {
Expand Down