Merge pull request #100694 from nilo19/bug/cherry-pick-574

Cherry pick #574 from Cloud Provider Azure: do not tag user created public IPs
This commit is contained in:
Kubernetes Prow Robot 2021-04-09 05:20:20 -07:00 committed by GitHub
commit 0e4545de01
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 341 additions and 143 deletions

View File

@ -650,12 +650,20 @@ func (az *Cloud) ensurePublicIPExists(service *v1.Service, pipName string, domai
serviceName := getServiceName(service)
var changed bool
if existsPip {
// ensure that the service tag is good
changed, err := bindServicesToPIP(&pip, []string{serviceName}, false)
// ensure that the service tag is good for managed pips
owns, isUserAssignedPIP := serviceOwnsPublicIP(service, &pip, clusterName)
if owns && !isUserAssignedPIP {
changed, err = bindServicesToPIP(&pip, []string{serviceName}, false)
if err != nil {
return nil, err
}
}
if pip.Tags == nil {
pip.Tags = make(map[string]*string)
}
// return if pip exist and dns label is the same
if strings.EqualFold(getDomainNameLabel(&pip), domainNameLabel) {
@ -2091,7 +2099,12 @@ func deduplicate(collection *[]string) *[]string {
}
// Determine if we should release existing owned public IPs
func shouldReleaseExistingOwnedPublicIP(existingPip *network.PublicIPAddress, lbShouldExist, lbIsInternal bool, desiredPipName, svcName string, ipTagRequest serviceIPTagRequest) bool {
func shouldReleaseExistingOwnedPublicIP(existingPip *network.PublicIPAddress, lbShouldExist, lbIsInternal, isUserAssignedPIP bool, desiredPipName string, ipTagRequest serviceIPTagRequest) bool {
// skip deleting user created pip
if isUserAssignedPIP {
return false
}
// Latch some variables for readability purposes.
pipName := *(*existingPip).Name
@ -2214,9 +2227,10 @@ func (az *Cloud) reconcilePublicIP(clusterName string, service *v1.Service, lbNa
// Now, let's perform additional analysis to determine if we should release the public ips we have found.
// We can only let them go if (a) they are owned by this service and (b) they meet the criteria for deletion.
if serviceOwnsPublicIP(&pip, clusterName, serviceName) {
owns, isUserAssignedPIP := serviceOwnsPublicIP(service, &pip, clusterName)
if owns {
var dirtyPIP, toBeDeleted bool
if !wantLb {
if !wantLb && !isUserAssignedPIP {
klog.V(2).Infof("reconcilePublicIP for service(%s): unbinding the service from pip %s", serviceName, *pip.Name)
err = unbindServiceFromPIP(&pip, serviceName)
if err != nil {
@ -2228,7 +2242,7 @@ func (az *Cloud) reconcilePublicIP(clusterName string, service *v1.Service, lbNa
if changed {
dirtyPIP = true
}
if shouldReleaseExistingOwnedPublicIP(&pip, wantLb, isInternal, desiredPipName, serviceName, serviceIPTagRequest) {
if shouldReleaseExistingOwnedPublicIP(&pip, wantLb, isInternal, isUserAssignedPIP, desiredPipName, serviceIPTagRequest) {
// Then, release the public ip
pipsToBeDeleted = append(pipsToBeDeleted, &pip)
@ -2549,26 +2563,55 @@ func getServiceTags(service *v1.Service) []string {
return nil
}
func serviceOwnsPublicIP(pip *network.PublicIPAddress, clusterName, serviceName string) bool {
if pip != nil && pip.Tags != nil {
// serviceOwnsPublicIP checks if the service owns the pip and if the pip is user-created.
// The pip is user-created if and only if there is no service tags.
// The service owns the pip if:
// 1. The serviceName is included in the service tags of a system-created pip.
// 2. The service.Spec.LoadBalancerIP matches the IP address of a user-created pip.
func serviceOwnsPublicIP(service *v1.Service, pip *network.PublicIPAddress, clusterName string) (bool, bool) {
if service == nil || pip == nil {
klog.Warningf("serviceOwnsPublicIP: nil service or public IP")
return false, false
}
if pip.PublicIPAddressPropertiesFormat == nil || to.String(pip.IPAddress) == "" {
klog.Warningf("serviceOwnsPublicIP: empty pip.IPAddress")
return false, false
}
serviceName := getServiceName(service)
if pip.Tags != nil {
serviceTag := pip.Tags[serviceTagKey]
clusterTag := pip.Tags[clusterNameKey]
if serviceTag != nil && isSVCNameInPIPTag(*serviceTag, serviceName) {
// if there is no service tag on the pip, it is user-created pip
if to.String(serviceTag) == "" {
return strings.EqualFold(to.String(pip.IPAddress), service.Spec.LoadBalancerIP), true
}
if serviceTag != nil {
// if there is service tag on the pip, it is system-created pip
if isSVCNameInPIPTag(*serviceTag, serviceName) {
// Backward compatible for clusters upgraded from old releases.
// In such case, only "service" tag is set.
if clusterTag == nil {
return true
return true, false
}
// If cluster name tag is set, then return true if it matches.
if *clusterTag == clusterName {
return true
return true, false
}
} else {
// if the service is not included in te tags of the system-created pip, check the ip address
// this could happen for secondary services
return strings.EqualFold(to.String(pip.IPAddress), service.Spec.LoadBalancerIP), false
}
}
}
return false
return false, false
}
func isSVCNameInPIPTag(tag, svcName string) bool {

View File

@ -451,70 +451,87 @@ func TestServiceOwnsPublicIP(t *testing.T) {
pip *network.PublicIPAddress
clusterName string
serviceName string
expected bool
serviceLBIP string
expectedOwns bool
expectedUserAssignedPIP bool
}{
{
desc: "false should be returned when pip is nil",
clusterName: "kubernetes",
serviceName: "nginx",
expected: false,
expectedOwns: false,
},
{
desc: "false should be returned when service name tag doesn't match",
pip: &network.PublicIPAddress{
Tags: map[string]*string{
serviceTagKey: to.StringPtr("nginx"),
serviceTagKey: to.StringPtr("default/nginx"),
},
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
IPAddress: to.StringPtr("1.2.3.4"),
},
},
serviceName: "web",
expected: false,
expectedOwns: 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"),
serviceTagKey: to.StringPtr("default/nginx"),
},
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
IPAddress: to.StringPtr("1.2.3.4"),
},
},
clusterName: "kubernetes",
serviceName: "nginx",
expected: true,
expectedOwns: true,
},
{
desc: "false should be returned when cluster name doesn't match",
pip: &network.PublicIPAddress{
Tags: map[string]*string{
serviceTagKey: to.StringPtr("nginx"),
serviceTagKey: to.StringPtr("default/nginx"),
clusterNameKey: to.StringPtr("kubernetes"),
},
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
IPAddress: to.StringPtr("1.2.3.4"),
},
},
clusterName: "k8s",
serviceName: "nginx",
expected: false,
expectedOwns: 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"),
serviceTagKey: to.StringPtr("default/web"),
clusterNameKey: to.StringPtr("kubernetes"),
},
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
IPAddress: to.StringPtr("1.2.3.4"),
},
},
clusterName: "kubernetes",
serviceName: "nginx",
expected: false,
expectedOwns: 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"),
serviceTagKey: to.StringPtr("default/nginx"),
clusterNameKey: to.StringPtr("kubernetes"),
},
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
IPAddress: to.StringPtr("1.2.3.4"),
},
},
clusterName: "kubernetes",
serviceName: "nginx",
expected: true,
expectedOwns: true,
},
{
desc: "false should be returned when the tag is empty",
@ -523,22 +540,29 @@ func TestServiceOwnsPublicIP(t *testing.T) {
serviceTagKey: to.StringPtr(""),
clusterNameKey: to.StringPtr("kubernetes"),
},
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
IPAddress: to.StringPtr("1.2.3.4"),
},
},
clusterName: "kubernetes",
serviceName: "nginx",
expected: false,
expectedOwns: false,
expectedUserAssignedPIP: true,
},
{
desc: "true should be returned if there is a match among a multi-service tag",
pip: &network.PublicIPAddress{
Tags: map[string]*string{
serviceTagKey: to.StringPtr("nginx1,nginx2"),
serviceTagKey: to.StringPtr("default/nginx1,default/nginx2"),
clusterNameKey: to.StringPtr("kubernetes"),
},
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
IPAddress: to.StringPtr("1.2.3.4"),
},
},
clusterName: "kubernetes",
serviceName: "nginx1",
expected: true,
expectedOwns: true,
},
{
desc: "false should be returned if there is not a match among a multi-service tag",
@ -547,16 +571,59 @@ func TestServiceOwnsPublicIP(t *testing.T) {
serviceTagKey: to.StringPtr("default/nginx1,default/nginx2"),
clusterNameKey: to.StringPtr("kubernetes"),
},
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
IPAddress: to.StringPtr("1.2.3.4"),
},
},
clusterName: "kubernetes",
serviceName: "nginx3",
expected: false,
expectedOwns: false,
},
{
desc: "true should be returned if the load balancer IP is matched even if the svc name is not included in the tag",
pip: &network.PublicIPAddress{
Tags: map[string]*string{
serviceTagKey: to.StringPtr(""),
clusterNameKey: to.StringPtr("kubernetes"),
},
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
IPAddress: to.StringPtr("1.2.3.4"),
},
},
clusterName: "kubernetes",
serviceName: "nginx3",
serviceLBIP: "1.2.3.4",
expectedOwns: true,
expectedUserAssignedPIP: true,
},
{
desc: "true should be returned if the load balancer IP is not matched but the svc name is included in the tag",
pip: &network.PublicIPAddress{
Tags: map[string]*string{
serviceTagKey: to.StringPtr("default/nginx1,default/nginx2"),
clusterNameKey: to.StringPtr("kubernetes"),
},
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
IPAddress: to.StringPtr("1.2.3.4"),
},
},
clusterName: "kubernetes",
serviceName: "nginx1",
serviceLBIP: "1.1.1.1",
expectedOwns: true,
},
}
for i, c := range tests {
actual := serviceOwnsPublicIP(c.pip, c.clusterName, c.serviceName)
assert.Equal(t, c.expected, actual, "TestCase[%d]: %s", i, c.desc)
t.Run(c.desc, func(t *testing.T) {
service := getTestService(c.serviceName, v1.ProtocolTCP, nil, false, 80)
if c.serviceLBIP != "" {
service.Spec.LoadBalancerIP = c.serviceLBIP
}
owns, isUserAssignedPIP := serviceOwnsPublicIP(&service, c.pip, c.clusterName)
assert.Equal(t, c.expectedOwns, owns, "TestCase[%d]: %s", i, c.desc)
assert.Equal(t, c.expectedUserAssignedPIP, isUserAssignedPIP, "TestCase[%d]: %s", i, c.desc)
})
}
}
@ -619,11 +686,13 @@ func TestShouldReleaseExistingOwnedPublicIP(t *testing.T) {
tests := []struct {
desc string
desiredPipName string
existingPip network.PublicIPAddress
ipTagRequest serviceIPTagRequest
tags map[string]*string
lbShouldExist bool
lbIsInternal bool
desiredPipName string
ipTagRequest serviceIPTagRequest
isUserAssignedPIP bool
expectedShouldRelease bool
}{
{
@ -715,11 +784,51 @@ func TestShouldReleaseExistingOwnedPublicIP(t *testing.T) {
},
expectedShouldRelease: true,
},
{
desc: "should delete orphaned managed public IP",
existingPip: existingPipWithTag,
lbShouldExist: false,
lbIsInternal: false,
desiredPipName: *existingPipWithTag.Name,
tags: map[string]*string{serviceTagKey: to.StringPtr("")},
ipTagRequest: serviceIPTagRequest{
IPTagsRequestedByAnnotation: true,
IPTags: existingPipWithTag.PublicIPAddressPropertiesFormat.IPTags,
},
expectedShouldRelease: true,
},
{
desc: "should not delete managed public IP which has references",
existingPip: existingPipWithTag,
lbShouldExist: false,
lbIsInternal: false,
desiredPipName: *existingPipWithTag.Name,
tags: map[string]*string{serviceTagKey: to.StringPtr("svc1")},
ipTagRequest: serviceIPTagRequest{
IPTagsRequestedByAnnotation: true,
IPTags: existingPipWithTag.PublicIPAddressPropertiesFormat.IPTags,
},
},
{
desc: "should not delete orphaned unmanaged public IP",
existingPip: existingPipWithTag,
lbShouldExist: false,
lbIsInternal: false,
desiredPipName: *existingPipWithTag.Name,
tags: map[string]*string{serviceTagKey: to.StringPtr("")},
ipTagRequest: serviceIPTagRequest{
IPTagsRequestedByAnnotation: true,
IPTags: existingPipWithTag.PublicIPAddressPropertiesFormat.IPTags,
},
isUserAssignedPIP: true,
},
}
for i, c := range tests {
actualShouldRelease := shouldReleaseExistingOwnedPublicIP(&c.existingPip, c.lbShouldExist, c.lbIsInternal, c.desiredPipName, "default/test1", c.ipTagRequest)
if c.tags != nil {
c.existingPip.Tags = c.tags
}
actualShouldRelease := shouldReleaseExistingOwnedPublicIP(&c.existingPip, c.lbShouldExist, c.lbIsInternal, c.isUserAssignedPIP, c.desiredPipName, c.ipTagRequest)
assert.Equal(t, c.expectedShouldRelease, actualShouldRelease, "TestCase[%d]: %s", i, c.desc)
}
}
@ -2568,11 +2677,11 @@ func TestReconcilePublicIP(t *testing.T) {
testCases := []struct {
desc string
wantLb bool
expectedID string
annotations map[string]string
existingPIPs []network.PublicIPAddress
expectedID string
expectedPIP *network.PublicIPAddress
wantLb bool
expectedError bool
expectedCreateOrUpdateCount int
expectedDeleteCount int
@ -2601,6 +2710,9 @@ func TestReconcilePublicIP(t *testing.T) {
{
Name: to.StringPtr("pip1"),
Tags: map[string]*string{"service": to.StringPtr("default/test1")},
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
IPAddress: to.StringPtr("1.2.3.4"),
},
},
},
expectedID: "/subscriptions/subscription/resourceGroups/rg/providers/" +
@ -2634,14 +2746,23 @@ func TestReconcilePublicIP(t *testing.T) {
{
Name: to.StringPtr("pip1"),
Tags: map[string]*string{"service": to.StringPtr("default/test1")},
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
IPAddress: to.StringPtr("1.2.3.4"),
},
},
{
Name: to.StringPtr("pip2"),
Tags: map[string]*string{"service": to.StringPtr("default/test1")},
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
IPAddress: to.StringPtr("1.2.3.4"),
},
},
{
Name: to.StringPtr("testPIP"),
Tags: map[string]*string{"service": to.StringPtr("default/test1")},
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
IPAddress: to.StringPtr("1.2.3.4"),
},
},
},
expectedPIP: &network.PublicIPAddress{
@ -2649,8 +2770,8 @@ func TestReconcilePublicIP(t *testing.T) {
Name: to.StringPtr("testPIP"),
Tags: map[string]*string{"service": to.StringPtr("default/test1")},
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
PublicIPAllocationMethod: network.Static,
PublicIPAddressVersion: network.IPv4,
IPAddress: to.StringPtr("1.2.3.4"),
},
},
expectedCreateOrUpdateCount: 1,
@ -2664,14 +2785,23 @@ func TestReconcilePublicIP(t *testing.T) {
{
Name: to.StringPtr("pip1"),
Tags: map[string]*string{"service": to.StringPtr("default/test1")},
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
IPAddress: to.StringPtr("1.2.3.4"),
},
},
{
Name: to.StringPtr("pip2"),
Tags: map[string]*string{"service": to.StringPtr("default/test1,default/test2")},
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
IPAddress: to.StringPtr("1.2.3.4"),
},
},
{
Name: to.StringPtr("testPIP"),
Tags: map[string]*string{"service": to.StringPtr("default/test1")},
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
IPAddress: to.StringPtr("1.2.3.4"),
},
},
},
expectedPIP: &network.PublicIPAddress{
@ -2679,8 +2809,8 @@ func TestReconcilePublicIP(t *testing.T) {
Name: to.StringPtr("testPIP"),
Tags: map[string]*string{"service": to.StringPtr("default/test1")},
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
PublicIPAllocationMethod: network.Static,
PublicIPAddressVersion: network.IPv4,
IPAddress: to.StringPtr("1.2.3.4"),
},
},
expectedCreateOrUpdateCount: 1,
@ -2697,14 +2827,23 @@ func TestReconcilePublicIP(t *testing.T) {
{
Name: to.StringPtr("pip1"),
Tags: map[string]*string{"service": to.StringPtr("default/test1")},
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
IPAddress: to.StringPtr("1.2.3.4"),
},
},
{
Name: to.StringPtr("pip2"),
Tags: map[string]*string{"service": to.StringPtr("default/test1")},
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
IPAddress: to.StringPtr("1.2.3.4"),
},
},
{
Name: to.StringPtr("testPIP"),
Tags: map[string]*string{"service": to.StringPtr("default/test1")},
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
IPAddress: to.StringPtr("1.2.3.4"),
},
},
},
expectedPIP: &network.PublicIPAddress{
@ -2745,6 +2884,7 @@ func TestReconcilePublicIP(t *testing.T) {
Tag: to.StringPtr("tag1value"),
},
},
IPAddress: to.StringPtr("1.2.3.4"),
},
},
},
@ -2761,6 +2901,7 @@ func TestReconcilePublicIP(t *testing.T) {
Tag: to.StringPtr("tag1value"),
},
},
IPAddress: to.StringPtr("1.2.3.4"),
},
},
expectedCreateOrUpdateCount: 1,
@ -2773,21 +2914,30 @@ func TestReconcilePublicIP(t *testing.T) {
existingPIPs: []network.PublicIPAddress{
{
Name: to.StringPtr("pip1"),
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
IPAddress: to.StringPtr("1.2.3.4"),
},
},
{
Name: to.StringPtr("pip2"),
Tags: map[string]*string{"service": to.StringPtr("default/test1")},
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
IPAddress: to.StringPtr("1.2.3.4"),
},
},
{
Name: to.StringPtr("testPIP"),
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
IPAddress: to.StringPtr("1.2.3.4"),
},
},
},
expectedPIP: &network.PublicIPAddress{
ID: to.StringPtr("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/publicIPAddresses/testPIP"),
Name: to.StringPtr("testPIP"),
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
PublicIPAllocationMethod: network.Static,
PublicIPAddressVersion: network.IPv4,
IPAddress: to.StringPtr("1.2.3.4"),
},
},
expectedCreateOrUpdateCount: 1,
@ -2800,6 +2950,9 @@ func TestReconcilePublicIP(t *testing.T) {
{
Name: to.StringPtr("pip1"),
Tags: map[string]*string{serviceTagKey: to.StringPtr("default/test1,default/test2")},
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
IPAddress: to.StringPtr("1.2.3.4"),
},
},
},
expectedCreateOrUpdateCount: 1,
@ -2807,6 +2960,7 @@ func TestReconcilePublicIP(t *testing.T) {
}
for i, test := range testCases {
t.Run(test.desc, func(t *testing.T) {
deletedPips := make(map[string]bool)
savedPips := make(map[string]network.PublicIPAddress)
createOrUpdateCount := 0
@ -2892,6 +3046,7 @@ func TestReconcilePublicIP(t *testing.T) {
}
}
assert.Equal(t, test.expectedDeleteCount, deletedCount, "TestCase[%d]: %s", i, test.desc)
})
}
}