From c1f4a25e64b366bacdaa63287ed03435b4e2d3d8 Mon Sep 17 00:00:00 2001 From: Qi Ni Date: Wed, 31 Mar 2021 11:17:14 +0800 Subject: [PATCH] do not tag user created public IPs --- .../azure/azure_loadbalancer.go | 83 +++- .../azure/azure_loadbalancer_test.go | 401 ++++++++++++------ 2 files changed, 341 insertions(+), 143 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 629da8c2669..44bddbb46bb 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 @@ -650,11 +650,19 @@ 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) - if err != nil { - return nil, err + // 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 @@ -2084,7 +2092,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 @@ -2207,9 +2220,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 { @@ -2221,7 +2235,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) @@ -2542,26 +2556,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) { - // Backward compatible for clusters upgraded from old releases. - // In such case, only "service" tag is set. - if clusterTag == nil { - return true - } + // 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 cluster name tag is set, then return true if it matches. - if *clusterTag == clusterName { - return 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, false + } + + // If cluster name tag is set, then return true if it matches. + if *clusterTag == clusterName { + 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 { 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 c8857639f0d..41d269dbada 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 @@ -447,74 +447,91 @@ func TestEnsureLoadBalancerDeleted(t *testing.T) { func TestServiceOwnsPublicIP(t *testing.T) { tests := []struct { - desc string - pip *network.PublicIPAddress - clusterName string - serviceName string - expected bool + desc string + pip *network.PublicIPAddress + clusterName string + serviceName string + serviceLBIP string + expectedOwns bool + expectedUserAssignedPIP bool }{ { - desc: "false should be returned when pip is nil", - clusterName: "kubernetes", - serviceName: "nginx", - expected: false, + desc: "false should be returned when pip is nil", + clusterName: "kubernetes", + serviceName: "nginx", + 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, + serviceName: "web", + 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, + clusterName: "kubernetes", + serviceName: "nginx", + 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, + clusterName: "k8s", + serviceName: "nginx", + 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, + clusterName: "kubernetes", + serviceName: "nginx", + 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, + clusterName: "kubernetes", + serviceName: "nginx", + 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, + clusterName: "kubernetes", + serviceName: "nginx", + 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, + clusterName: "kubernetes", + serviceName: "nginx1", + 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, + clusterName: "kubernetes", + serviceName: "nginx3", + 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) } } @@ -2515,11 +2624,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 @@ -2548,6 +2657,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/" + @@ -2581,14 +2693,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{ @@ -2596,8 +2717,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, + PublicIPAddressVersion: network.IPv4, + IPAddress: to.StringPtr("1.2.3.4"), }, }, expectedCreateOrUpdateCount: 1, @@ -2611,14 +2732,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{ @@ -2626,8 +2756,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, + PublicIPAddressVersion: network.IPv4, + IPAddress: to.StringPtr("1.2.3.4"), }, }, expectedCreateOrUpdateCount: 1, @@ -2644,14 +2774,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{ @@ -2692,6 +2831,7 @@ func TestReconcilePublicIP(t *testing.T) { Tag: to.StringPtr("tag1value"), }, }, + IPAddress: to.StringPtr("1.2.3.4"), }, }, }, @@ -2708,6 +2848,7 @@ func TestReconcilePublicIP(t *testing.T) { Tag: to.StringPtr("tag1value"), }, }, + IPAddress: to.StringPtr("1.2.3.4"), }, }, expectedCreateOrUpdateCount: 1, @@ -2720,21 +2861,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, + PublicIPAddressVersion: network.IPv4, + IPAddress: to.StringPtr("1.2.3.4"), }, }, expectedCreateOrUpdateCount: 1, @@ -2747,6 +2897,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, @@ -2754,91 +2907,93 @@ func TestReconcilePublicIP(t *testing.T) { } for i, test := range testCases { - deletedPips := make(map[string]bool) - savedPips := make(map[string]network.PublicIPAddress) - createOrUpdateCount := 0 - var m sync.Mutex - az := GetTestCloud(ctrl) - mockPIPsClient := az.PublicIPAddressesClient.(*mockpublicipclient.MockInterface) - creator := mockPIPsClient.EXPECT().CreateOrUpdate(gomock.Any(), "rg", gomock.Any(), gomock.Any()).AnyTimes() - creator.DoAndReturn(func(ctx context.Context, resourceGroupName string, publicIPAddressName string, parameters network.PublicIPAddress) *retry.Error { - m.Lock() - deletedPips[publicIPAddressName] = false - savedPips[publicIPAddressName] = parameters - createOrUpdateCount++ - m.Unlock() - return nil - }) - - mockPIPsClient.EXPECT().List(gomock.Any(), "rg").Return(test.existingPIPs, nil).AnyTimes() - if i == 2 { - mockPIPsClient.EXPECT().Get(gomock.Any(), "rg", "testCluster-atest1", gomock.Any()).Return(network.PublicIPAddress{}, &retry.Error{HTTPStatusCode: http.StatusNotFound}).Times(1) - mockPIPsClient.EXPECT().Get(gomock.Any(), "rg", "testCluster-atest1", gomock.Any()).Return(network.PublicIPAddress{ID: to.StringPtr("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/publicIPAddresses/testCluster-atest1")}, nil).Times(1) - } - service := getTestService("test1", v1.ProtocolTCP, nil, false, 80) - service.Annotations = test.annotations - for _, pip := range test.existingPIPs { - savedPips[*pip.Name] = pip - getter := mockPIPsClient.EXPECT().Get(gomock.Any(), "rg", *pip.Name, gomock.Any()).AnyTimes() - getter.DoAndReturn(func(ctx context.Context, resourceGroupName string, publicIPAddressName string, expand string) (result network.PublicIPAddress, rerr *retry.Error) { + t.Run(test.desc, func(t *testing.T) { + deletedPips := make(map[string]bool) + savedPips := make(map[string]network.PublicIPAddress) + createOrUpdateCount := 0 + var m sync.Mutex + az := GetTestCloud(ctrl) + mockPIPsClient := az.PublicIPAddressesClient.(*mockpublicipclient.MockInterface) + creator := mockPIPsClient.EXPECT().CreateOrUpdate(gomock.Any(), "rg", gomock.Any(), gomock.Any()).AnyTimes() + creator.DoAndReturn(func(ctx context.Context, resourceGroupName string, publicIPAddressName string, parameters network.PublicIPAddress) *retry.Error { m.Lock() - deletedValue, deletedContains := deletedPips[publicIPAddressName] - savedPipValue, savedPipContains := savedPips[publicIPAddressName] - m.Unlock() - - if (!deletedContains || !deletedValue) && savedPipContains { - return savedPipValue, nil - } - - return network.PublicIPAddress{}, &retry.Error{HTTPStatusCode: http.StatusNotFound} - - }) - deleter := mockPIPsClient.EXPECT().Delete(gomock.Any(), "rg", *pip.Name).Return(nil).AnyTimes() - deleter.Do(func(ctx context.Context, resourceGroupName string, publicIPAddressName string) *retry.Error { - m.Lock() - deletedPips[publicIPAddressName] = true + deletedPips[publicIPAddressName] = false + savedPips[publicIPAddressName] = parameters + createOrUpdateCount++ m.Unlock() return nil }) - err := az.PublicIPAddressesClient.CreateOrUpdate(context.TODO(), "rg", to.String(pip.Name), pip) - if err != nil { - t.Fatalf("TestCase[%d] meets unexpected error: %v", i, err) + mockPIPsClient.EXPECT().List(gomock.Any(), "rg").Return(test.existingPIPs, nil).AnyTimes() + if i == 2 { + mockPIPsClient.EXPECT().Get(gomock.Any(), "rg", "testCluster-atest1", gomock.Any()).Return(network.PublicIPAddress{}, &retry.Error{HTTPStatusCode: http.StatusNotFound}).Times(1) + mockPIPsClient.EXPECT().Get(gomock.Any(), "rg", "testCluster-atest1", gomock.Any()).Return(network.PublicIPAddress{ID: to.StringPtr("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/publicIPAddresses/testCluster-atest1")}, nil).Times(1) } + service := getTestService("test1", v1.ProtocolTCP, nil, false, 80) + service.Annotations = test.annotations + for _, pip := range test.existingPIPs { + savedPips[*pip.Name] = pip + getter := mockPIPsClient.EXPECT().Get(gomock.Any(), "rg", *pip.Name, gomock.Any()).AnyTimes() + getter.DoAndReturn(func(ctx context.Context, resourceGroupName string, publicIPAddressName string, expand string) (result network.PublicIPAddress, rerr *retry.Error) { + m.Lock() + deletedValue, deletedContains := deletedPips[publicIPAddressName] + savedPipValue, savedPipContains := savedPips[publicIPAddressName] + m.Unlock() - // Clear create or update count to prepare for main execution - createOrUpdateCount = 0 - } - pip, err := az.reconcilePublicIP("testCluster", &service, "", test.wantLb) - if !test.expectedError { - assert.Equal(t, nil, err, "TestCase[%d]: %s", i, test.desc) - } - if test.expectedID != "" { - assert.Equal(t, test.expectedID, to.String(pip.ID), "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) + if (!deletedContains || !deletedValue) && savedPipContains { + return savedPipValue, nil + } - if test.expectedPIP.PublicIPAddressPropertiesFormat != nil { - sortIPTags(test.expectedPIP.PublicIPAddressPropertiesFormat.IPTags) + return network.PublicIPAddress{}, &retry.Error{HTTPStatusCode: http.StatusNotFound} + + }) + deleter := mockPIPsClient.EXPECT().Delete(gomock.Any(), "rg", *pip.Name).Return(nil).AnyTimes() + deleter.Do(func(ctx context.Context, resourceGroupName string, publicIPAddressName string) *retry.Error { + m.Lock() + deletedPips[publicIPAddressName] = true + m.Unlock() + return nil + }) + + err := az.PublicIPAddressesClient.CreateOrUpdate(context.TODO(), "rg", to.String(pip.Name), pip) + if err != nil { + t.Fatalf("TestCase[%d] meets unexpected error: %v", i, err) + } + + // Clear create or update count to prepare for main execution + createOrUpdateCount = 0 } - - if pip.PublicIPAddressPropertiesFormat != nil { - sortIPTags(pip.PublicIPAddressPropertiesFormat.IPTags) + pip, err := az.reconcilePublicIP("testCluster", &service, "", test.wantLb) + if !test.expectedError { + assert.Equal(t, nil, err, "TestCase[%d]: %s", i, test.desc) } + if test.expectedID != "" { + assert.Equal(t, test.expectedID, to.String(pip.ID), "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.expectedPIP.PublicIPAddressPropertiesFormat, pip.PublicIPAddressPropertiesFormat, "TestCase[%d]: %s", i, test.desc) + if test.expectedPIP.PublicIPAddressPropertiesFormat != nil { + sortIPTags(test.expectedPIP.PublicIPAddressPropertiesFormat.IPTags) + } - } - assert.Equal(t, test.expectedCreateOrUpdateCount, createOrUpdateCount, "TestCase[%d]: %s", i, test.desc) - assert.Equal(t, test.expectedError, err != nil, "TestCase[%d]: %s", i, test.desc) + if pip.PublicIPAddressPropertiesFormat != nil { + sortIPTags(pip.PublicIPAddressPropertiesFormat.IPTags) + } + + assert.Equal(t, test.expectedPIP.PublicIPAddressPropertiesFormat, pip.PublicIPAddressPropertiesFormat, "TestCase[%d]: %s", i, test.desc) - deletedCount := 0 - for _, deleted := range deletedPips { - if deleted { - deletedCount++ } - } - assert.Equal(t, test.expectedDeleteCount, deletedCount, "TestCase[%d]: %s", i, test.desc) + assert.Equal(t, test.expectedCreateOrUpdateCount, createOrUpdateCount, "TestCase[%d]: %s", i, test.desc) + assert.Equal(t, test.expectedError, err != nil, "TestCase[%d]: %s", i, test.desc) + + deletedCount := 0 + for _, deleted := range deletedPips { + if deleted { + deletedCount++ + } + } + assert.Equal(t, test.expectedDeleteCount, deletedCount, "TestCase[%d]: %s", i, test.desc) + }) } }