do not tag user created public IPs

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
+		// ensure that the service tag is good for managed pips
-		changed, err := bindServicesToPIP(&pip, []string{serviceName}, false)
+		owns, isUserAssignedPIP := serviceOwnsPublicIP(service, &pip, clusterName)
-		if err != nil {
+		if owns && !isUserAssignedPIP {
-			return nil, err
+			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 {
+// serviceOwnsPublicIP checks if the service owns the pip and if the pip is user-created.
-	if pip != nil && pip.Tags != nil {
+// 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
-			// Backward compatible for clusters upgraded from old releases.
+		if to.String(serviceTag) == "" {
-			// In such case, only "service" tag is set.
+			return strings.EqualFold(to.String(pip.IPAddress), service.Spec.LoadBalancerIP), true
-			if clusterTag == nil {
+		}
-				return true
-			}
 
-			// If cluster name tag is set, then return true if it matches.
+		if serviceTag != nil {
-			if *clusterTag == clusterName {
+			// if there is service tag on the pip, it is system-created pip
-				return true
+			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 {

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
+		desc                    string
-		pip         *network.PublicIPAddress
+		pip                     *network.PublicIPAddress
-		clusterName string
+		clusterName             string
-		serviceName string
+		serviceName             string
-		expected    bool
+		serviceLBIP             string
+		expectedOwns            bool
+		expectedUserAssignedPIP bool
 	}{
 		{
-			desc:        "false should be returned when pip is nil",
+			desc:         "false should be returned when pip is nil",
-			clusterName: "kubernetes",
+			clusterName:  "kubernetes",
-			serviceName: "nginx",
+			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",
+			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",
+			clusterName:  "kubernetes",
-			serviceName: "nginx",
+			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",
+			clusterName:  "k8s",
-			serviceName: "nginx",
+			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",
+			clusterName:  "kubernetes",
-			serviceName: "nginx",
+			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",
+			clusterName:  "kubernetes",
-			serviceName: "nginx",
+			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",
+			clusterName:             "kubernetes",
-			serviceName: "nginx",
+			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",
+			clusterName:  "kubernetes",
-			serviceName: "nginx1",
+			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",
+			clusterName:  "kubernetes",
-			serviceName: "nginx3",
+			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)
+		t.Run(c.desc, func(t *testing.T) {
-		assert.Equal(t, c.expected, actual, "TestCase[%d]: %s", i, c.desc)
+			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
+		isUserAssignedPIP     bool
-		ipTagRequest          serviceIPTagRequest
 		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 {
-
+		if c.tags != nil {
-		actualShouldRelease := shouldReleaseExistingOwnedPublicIP(&c.existingPip, c.lbShouldExist, c.lbIsInternal, c.desiredPipName, "default/test1", c.ipTagRequest)
+			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)
+		t.Run(test.desc, func(t *testing.T) {
-		savedPips := make(map[string]network.PublicIPAddress)
+			deletedPips := make(map[string]bool)
-		createOrUpdateCount := 0
+			savedPips := make(map[string]network.PublicIPAddress)
-		var m sync.Mutex
+			createOrUpdateCount := 0
-		az := GetTestCloud(ctrl)
+			var m sync.Mutex
-		mockPIPsClient := az.PublicIPAddressesClient.(*mockpublicipclient.MockInterface)
+			az := GetTestCloud(ctrl)
-		creator := mockPIPsClient.EXPECT().CreateOrUpdate(gomock.Any(), "rg", gomock.Any(), gomock.Any()).AnyTimes()
+			mockPIPsClient := az.PublicIPAddressesClient.(*mockpublicipclient.MockInterface)
-		creator.DoAndReturn(func(ctx context.Context, resourceGroupName string, publicIPAddressName string, parameters network.PublicIPAddress) *retry.Error {
+			creator := mockPIPsClient.EXPECT().CreateOrUpdate(gomock.Any(), "rg", gomock.Any(), gomock.Any()).AnyTimes()
-			m.Lock()
+			creator.DoAndReturn(func(ctx context.Context, resourceGroupName string, publicIPAddressName string, parameters network.PublicIPAddress) *retry.Error {
-			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) {
 				m.Lock()
-				deletedValue, deletedContains := deletedPips[publicIPAddressName]
+				deletedPips[publicIPAddressName] = false
-				savedPipValue, savedPipContains := savedPips[publicIPAddressName]
+				savedPips[publicIPAddressName] = parameters
-				m.Unlock()
+				createOrUpdateCount++
-
-				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
 				m.Unlock()
 				return nil
 			})
 
-			err := az.PublicIPAddressesClient.CreateOrUpdate(context.TODO(), "rg", to.String(pip.Name), pip)
+			mockPIPsClient.EXPECT().List(gomock.Any(), "rg").Return(test.existingPIPs, nil).AnyTimes()
-			if err != nil {
+			if i == 2 {
-				t.Fatalf("TestCase[%d] meets unexpected error: %v", i, err)
+				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
+					if (!deletedContains || !deletedValue) && savedPipContains {
-			createOrUpdateCount = 0
+						return savedPipValue, nil
-		}
+					}
-		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 test.expectedPIP.PublicIPAddressPropertiesFormat != nil {
+					return network.PublicIPAddress{}, &retry.Error{HTTPStatusCode: http.StatusNotFound}
-				sortIPTags(test.expectedPIP.PublicIPAddressPropertiesFormat.IPTags)
+
+				})
+				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
 			}
-
+			pip, err := az.reconcilePublicIP("testCluster", &service, "", test.wantLb)
-			if pip.PublicIPAddressPropertiesFormat != nil {
+			if !test.expectedError {
-				sortIPTags(pip.PublicIPAddressPropertiesFormat.IPTags)
+				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)
+				}
 
-		}
+				if pip.PublicIPAddressPropertiesFormat != nil {
-		assert.Equal(t, test.expectedCreateOrUpdateCount, createOrUpdateCount, "TestCase[%d]: %s", i, test.desc)
+					sortIPTags(pip.PublicIPAddressPropertiesFormat.IPTags)
-		assert.Equal(t, test.expectedError, err != nil, "TestCase[%d]: %s", i, test.desc)
+				}
+
+				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.expectedCreateOrUpdateCount, createOrUpdateCount, "TestCase[%d]: %s", i, test.desc)
-		assert.Equal(t, test.expectedDeleteCount, deletedCount, "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)
+		})
 	}
 }