Repository: kubernetes-sigs/cloud-provider-azure (go)

Issue:
User created pip is deleted unexpectedly if the binded service is deleted
<!-- Please use this template while reporting a bug and provide as much info as possible. Not doing so may result in your bug not being addressed in a timely manner. Thanks!
-->


**What happened**:
If we create a service and set the `service.Spec.LoadBalancerIP` to bind an existing pip, the pip would be deleted if the service is deleted, which is not expected.

**What you expected to happen**:

**How to reproduce it**:

**Anything else we need to know?**:

**Environment**:
- Kubernetes version (use `kubectl version`):
- OS (e.g. from /etc/os-release):
- Kernel (e.g. `uname -a`):
- Install tools:
- Others:


Pull request:
fix: not delete existing pip when service is deleted
<!--  Thanks for sending a pull request!  Here are some tips for you:

1. If this is your first time, please read our contributor guidelines: https://git.k8s.io/community/contributors/guide#your-first-contribution and developer guide https://git.k8s.io/community/contributors/devel/development.md#development-guide
2. Please label this pull request according to what type of issue you are addressing, especially if this is a release targeted pull request. For reference on required PR/issue labels, read here:
https://git.k8s.io/community/contributors/devel/release.md#issue-kind-label
3. Ensure you have added or ran the appropriate tests for your PR: https://git.k8s.io/community/contributors/devel/testing.md
4. If you want *faster* PR reviews, read how: https://git.k8s.io/community/contributors/guide/pull-requests.md#best-practices-for-faster-reviews
5. Follow the instructions for writing a release note: https://git.k8s.io/community/contributors/guide/release-notes.md
6. If the PR is unfinished, see how to mark it: https://git.k8s.io/community/contributors/guide/pull-requests.md#marking-unfinished-pull-requests
-->

**What type of PR is this?**
> Uncomment only one ` /kind <>` line, hit enter to put that in a new line, and remove leading whitespaces from that line:
>
> /kind api-change
> /kind bug
> /kind cleanup
> /kind design
> /kind documentation
> /kind failing-test
> /kind feature
> /kind flake
/kind bug

**What this PR does / why we need it**:
The user-created services could be deleted accidentally because we delete the pip by checking if the service tags on it are empty. This fix solves the issue by not tagging the user-created pips.

**Which issue(s) this PR fixes**:
<!-- 
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Fixes #573

**Special notes for your reviewer**:


**Release note**:
```
fix: not delete existing pip when service is deleted
```


Patch:
diff --git a/pkg/provider/azure_loadbalancer.go b/pkg/provider/azure_loadbalancer.go
index a50a75d38..e19351792 100644
--- a/pkg/provider/azure_loadbalancer.go
+++ b/pkg/provider/azure_loadbalancer.go
@@ -18,7 +18,6 @@ package provider
 
 import (
 	"context"
-	"encoding/json"
 	"fmt"
 	"math"
 	"reflect"
@@ -734,11 +733,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
@@ -815,28 +822,9 @@ 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 foundDNSLabelAnnotation {
-		if existingServiceName, ok := pip.Tags[consts.ServiceUsingDNSKey]; ok {
-			if !strings.EqualFold(to.String(existingServiceName), serviceName) {
-				return nil, fmt.Errorf("ensurePublicIPExists for service(%s): pip(%s) - there is an existing service %s consuming the DNS label on the public IP, so the service cannot set the DNS label annotation with this value", serviceName, pipName, *existingServiceName)
-			}
-		}
-
-		if len(domainNameLabel) == 0 {
-			pip.PublicIPAddressPropertiesFormat.DNSSettings = nil
-		} else {
-			if pip.PublicIPAddressPropertiesFormat.DNSSettings == nil ||
-				pip.PublicIPAddressPropertiesFormat.DNSSettings.DomainNameLabel == nil {
-				klog.V(6).Infof("ensurePublicIPExists for service(%s): pip(%s) - no existing DNS label on the public IP, create one", serviceName, pipName)
-				pip.PublicIPAddressPropertiesFormat.DNSSettings = &network.PublicIPAddressDNSSettings{
-					DomainNameLabel: &domainNameLabel,
-				}
-			} else {
-				existingDNSLabel := pip.PublicIPAddressPropertiesFormat.DNSSettings.DomainNameLabel
-				if !strings.EqualFold(to.String(existingDNSLabel), domainNameLabel) {
-					return nil, fmt.Errorf("ensurePublicIPExists for service(%s): pip(%s) - there is an existing DNS label %s on the public IP", serviceName, pipName, *existingDNSLabel)
-				}
-			}
-			pip.Tags[consts.ServiceUsingDNSKey] = &serviceName
+		err = reconcileDNSSettings(&pip, domainNameLabel, serviceName, pipName)
+		if err != nil {
+			return nil, fmt.Errorf("ensurePublicIPExists for service(%s): failed to reconcileDNSSettings: %w", serviceName, err)
 		}
 	}
 
@@ -874,6 +862,34 @@ func (az *Cloud) ensurePublicIPExists(service *v1.Service, pipName string, domai
 	return &pip, nil
 }
 
+func reconcileDNSSettings(pip *network.PublicIPAddress, domainNameLabel, serviceName, pipName string) error {
+	if existingServiceName, ok := pip.Tags[consts.ServiceUsingDNSKey]; ok {
+		if !strings.EqualFold(to.String(existingServiceName), serviceName) {
+			return fmt.Errorf("ensurePublicIPExists for service(%s): pip(%s) - there is an existing service %s consuming the DNS label on the public IP, so the service cannot set the DNS label annotation with this value", serviceName, pipName, *existingServiceName)
+		}
+	}
+
+	if len(domainNameLabel) == 0 {
+		pip.PublicIPAddressPropertiesFormat.DNSSettings = nil
+	} else {
+		if pip.PublicIPAddressPropertiesFormat.DNSSettings == nil ||
+			pip.PublicIPAddressPropertiesFormat.DNSSettings.DomainNameLabel == nil {
+			klog.V(6).Infof("ensurePublicIPExists for service(%s): pip(%s) - no existing DNS label on the public IP, create one", serviceName, pipName)
+			pip.PublicIPAddressPropertiesFormat.DNSSettings = &network.PublicIPAddressDNSSettings{
+				DomainNameLabel: &domainNameLabel,
+			}
+		} else {
+			existingDNSLabel := pip.PublicIPAddressPropertiesFormat.DNSSettings.DomainNameLabel
+			if !strings.EqualFold(to.String(existingDNSLabel), domainNameLabel) {
+				return fmt.Errorf("ensurePublicIPExists for service(%s): pip(%s) - there is an existing DNS label %s on the public IP", serviceName, pipName, *existingDNSLabel)
+			}
+		}
+		pip.Tags[consts.ServiceUsingDNSKey] = &serviceName
+	}
+
+	return nil
+}
+
 type serviceIPTagRequest struct {
 	IPTagsRequestedByAnnotation bool
 	IPTags                      *[]network.IPTag
@@ -1330,7 +1346,7 @@ func (az *Cloud) reconcileLBProbes(lb *network.LoadBalancer, service *v1.Service
 			}
 			if !keepProbe {
 				updatedProbes = append(updatedProbes[:i], updatedProbes[i+1:]...)
-				klog.V(2).Infof("reconcileLoadBalancer for service (%s)(%t): lb probe(%s) - dropping", serviceName, wantLb, *existingProbe.Name)
+				klog.V(10).Infof("reconcileLoadBalancer for service (%s)(%t): lb probe(%s) - dropping", serviceName, wantLb, *existingProbe.Name)
 				dirtyProbes = true
 			}
 		}
@@ -1349,8 +1365,6 @@ func (az *Cloud) reconcileLBProbes(lb *network.LoadBalancer, service *v1.Service
 		}
 	}
 	if dirtyProbes {
-		probesJSON, _ := json.MarshalIndent(expectedProbes, "", "  ")
-		klog.V(2).Infof("reconcileLoadBalancer for service (%s)(%t): lb probes updated: %s", serviceName, wantLb, string(probesJSON))
 		lb.Probes = &updatedProbes
 	}
 	return dirtyProbes
@@ -1395,8 +1409,6 @@ func (az *Cloud) reconcileLBRules(lb *network.LoadBalancer, service *v1.Service,
 		}
 	}
 	if dirtyRules {
-		ruleJSON, _ := json.MarshalIndent(expectedRules, "", "  ")
-		klog.V(2).Infof("reconcileLoadBalancer for service (%s)(%t): lb rules updated: %s", serviceName, wantLb, string(ruleJSON))
 		lb.LoadBalancingRules = &updatedRules
 	}
 	return dirtyRules
@@ -2286,7 +2298,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
 
@@ -2450,11 +2467,12 @@ func (az *Cloud) getPublicIPUpdates(clusterName string, service *v1.Service, pip
 
 		// 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)
+				err = unbindServiceFromPIP(&pip, service, serviceName, clusterName)
 				if err != nil {
 					return false, nil, false, nil, err
 				}
@@ -2464,7 +2482,7 @@ func (az *Cloud) getPublicIPUpdates(clusterName string, service *v1.Service, pip
 			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)
 
@@ -2740,26 +2758,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[consts.ServiceTagKey]
 		clusterTag := pip.Tags[consts.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 {
@@ -2843,11 +2890,12 @@ func bindServicesToPIP(pip *network.PublicIPAddress, incomingServiceNames []stri
 	return addedNew, nil
 }
 
-func unbindServiceFromPIP(pip *network.PublicIPAddress, serviceName string) error {
+func unbindServiceFromPIP(pip *network.PublicIPAddress, service *v1.Service, serviceName, clusterName string) error {
 	if pip == nil || pip.Tags == nil {
 		return fmt.Errorf("nil public IP or tags")
 	}
 
+	// skip removing tags for user assigned pips
 	serviceTagValue := pip.Tags[consts.ServiceTagKey]
 	existingServiceNames := parsePIPServiceTag(serviceTagValue)
 	var found bool


Test patch:
diff --git a/pkg/provider/azure_loadbalancer_test.go b/pkg/provider/azure_loadbalancer_test.go
index 058fbcbff..67f51b143 100644
--- a/pkg/provider/azure_loadbalancer_test.go
+++ b/pkg/provider/azure_loadbalancer_test.go
@@ -450,74 +450,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{
-					consts.ServiceTagKey: to.StringPtr("nginx"),
+					consts.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{
-					consts.ServiceTagKey: to.StringPtr("nginx"),
+					consts.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{
-					consts.ServiceTagKey:  to.StringPtr("nginx"),
+					consts.ServiceTagKey:  to.StringPtr("default/nginx"),
 					consts.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{
-					consts.ServiceTagKey:  to.StringPtr("web"),
+					consts.ServiceTagKey:  to.StringPtr("default/web"),
 					consts.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{
-					consts.ServiceTagKey:  to.StringPtr("nginx"),
+					consts.ServiceTagKey:  to.StringPtr("default/nginx"),
 					consts.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",
@@ -526,22 +543,29 @@ func TestServiceOwnsPublicIP(t *testing.T) {
 					consts.ServiceTagKey:  to.StringPtr(""),
 					consts.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{
-					consts.ServiceTagKey:  to.StringPtr("nginx1,nginx2"),
+					consts.ServiceTagKey:  to.StringPtr("default/nginx1,default/nginx2"),
 					consts.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",
@@ -550,16 +574,59 @@ func TestServiceOwnsPublicIP(t *testing.T) {
 					consts.ServiceTagKey:  to.StringPtr("default/nginx1,default/nginx2"),
 					consts.ClusterNameKey: to.StringPtr("kubernetes"),
 				},
+				PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
+					IPAddress: to.StringPtr("1.2.3.4"),
+				},
+			},
+			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{
+					consts.ServiceTagKey:  to.StringPtr(""),
+					consts.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{
+					consts.ServiceTagKey:  to.StringPtr("default/nginx1,default/nginx2"),
+					consts.ClusterNameKey: to.StringPtr("kubernetes"),
+				},
+				PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
+					IPAddress: to.StringPtr("1.2.3.4"),
+				},
 			},
-			clusterName: "kubernetes",
-			serviceName: "nginx3",
-			expected:    false,
+			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)
+		})
 	}
 }
 
@@ -625,8 +692,10 @@ func TestShouldReleaseExistingOwnedPublicIP(t *testing.T) {
 		desiredPipName        string
 		existingPip           network.PublicIPAddress
 		ipTagRequest          serviceIPTagRequest
+		tags                  map[string]*string
 		lbShouldExist         bool
 		lbIsInternal          bool
+		isUserAssignedPIP     bool
 		expectedShouldRelease bool
 	}{
 		{
@@ -718,11 +787,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{consts.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{consts.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{consts.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)
 	}
 }
@@ -2715,6 +2824,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/" +
@@ -2748,14 +2860,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{
@@ -2763,8 +2884,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,
@@ -2778,14 +2899,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{
@@ -2793,8 +2923,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,
@@ -2811,14 +2941,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{
@@ -2859,6 +2998,7 @@ func TestReconcilePublicIP(t *testing.T) {
 								Tag:       to.StringPtr("tag1value"),
 							},
 						},
+						IPAddress: to.StringPtr("1.2.3.4"),
 					},
 				},
 			},
@@ -2875,6 +3015,7 @@ func TestReconcilePublicIP(t *testing.T) {
 							Tag:       to.StringPtr("tag1value"),
 						},
 					},
+					IPAddress: to.StringPtr("1.2.3.4"),
 				},
 			},
 			expectedCreateOrUpdateCount: 1,
@@ -2887,21 +3028,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,
@@ -2914,6 +3064,9 @@ func TestReconcilePublicIP(t *testing.T) {
 				{
 					Name: to.StringPtr("pip1"),
 					Tags: map[string]*string{consts.ServiceTagKey: to.StringPtr("default/test1,default/test2")},
+					PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
+						IPAddress: to.StringPtr("1.2.3.4"),
+					},
 				},
 			},
 			expectedCreateOrUpdateCount: 1,
@@ -2921,91 +3074,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) {
-				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 {
+		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()
-				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()
+
+					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)
+				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 !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 {
-				sortIPTags(test.expectedPIP.PublicIPAddressPropertiesFormat.IPTags)
+				// 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)
+		})
 	}
 }
 
@@ -3446,6 +3601,8 @@ func TestUnbindServiceFromPIP(t *testing.T) {
 		{Tags: map[string]*string{consts.ServiceTagKey: to.StringPtr("ns1/svc1,ns2/svc2")}},
 	}
 	serviceName := "ns2/svc2"
+	service := getTestService(serviceName, v1.ProtocolTCP, nil, false, 80)
+	service.Spec.LoadBalancerIP = "1.2.3.4"
 	expectedTags := []map[string]*string{
 		nil,
 		{consts.ServiceTagKey: to.StringPtr("")},
@@ -3454,7 +3611,7 @@ func TestUnbindServiceFromPIP(t *testing.T) {
 	}
 
 	for i, pip := range pips {
-		_ = unbindServiceFromPIP(pip, serviceName)
+		_ = unbindServiceFromPIP(pip, &service, serviceName, "")
 		assert.Equal(t, expectedTags[i], pip.Tags)
 	}
 }
diff --git a/tests/e2e/network/ensureloadbalancer.go b/tests/e2e/network/ensureloadbalancer.go
index 26c5b4b11..52230c373 100644
--- a/tests/e2e/network/ensureloadbalancer.go
+++ b/tests/e2e/network/ensureloadbalancer.go
@@ -260,7 +260,7 @@ var _ = Describe("Ensure LoadBalancer", func() {
 		Expect(err).To(Equal(wait.ErrWaitTimeout))
 	})
 
-	FIt("should support multiple external services sharing one preset public IP address", func() {
+	It("should support multiple external services sharing one preset public IP address", func() {
 		ipName := basename + "-public-remain" + string(uuid.NewUUID())[0:4]
 		pip, err := utils.WaitCreatePIP(tc, ipName, tc.GetResourceGroup(), defaultPublicIPAddress(ipName))
 		Expect(err).NotTo(HaveOccurred())