Repository: kubernetes-sigs/cloud-provider-azure
Pull request #3892 (2023-05-17 13:52:52)

[release-1.24] fix: the pip without tags should be user-assigned
<!--  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/first-contribution.md#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/sig-release/release.md#issuepr-kind-label
3. Ensure you have added or ran the appropriate tests for your PR: https://git.k8s.io/community/contributors/devel/sig-testing/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. 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?

<!--
Add one of the following kinds:
/kind bug
/kind cleanup
/kind documentation
/kind feature
/kind design

Optionally add one or more of the following kinds if applicable:
/kind api-change
/kind deprecation
/kind failing-test
/kind flake
/kind regression
-->

/kind bug

#### What this PR does / why we need it:

1. PIPs without tags should be treated as user-assigned resources. We should not tag user-assigned pip without tags.
2. When the svc changes its IP from one to another, the cached pip can be outdated. Need to refresh the cache when necessary.
3. We should not tag user-assigned pip for DNS labels.

#### 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 #

#### Special notes for your reviewer:

#### Does this PR introduce a user-facing change?
<!--
If no, just write "NONE" in the release-note block below.
If yes, a release note is required:
Enter your extended release note in the block below. If the PR requires additional action from users switching to the new release, include the string "action required".

For more information on release notes see: https://git.k8s.io/community/contributors/guide/release-notes.md
-->
```release-note
fix: the pip without tags should be user-assigned
fix: refresh the pip cache when necessary
fix: do not tag user-assigned pip with `kubernetes-dns-label-service: <svcName>`
```

#### Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

<!--
This section can be blank if this pull request does not require a release note.

When adding links which point to resources within git repositories, like
KEPs or supporting documentation, please reference a specific commit and avoid
linking directly to the master branch. This ensures that links reference a
specific point in time, rather than a document that may change over time.

See here for guidance on getting permanent links to files: https://help.github.com/en/articles/getting-permanent-links-to-files

Please use the following format for linking documentation:
- [KEP]: <link>
- [Usage]: <link>
- [Other doc]: <link>
-->
```docs

```



Patch:
diff --git a/pkg/provider/azure_loadbalancer.go b/pkg/provider/azure_loadbalancer.go
index 854329b4b..b7fa89c78 100644
--- a/pkg/provider/azure_loadbalancer.go
+++ b/pkg/provider/azure_loadbalancer.go
@@ -186,7 +186,7 @@ func (az *Cloud) reconcileService(ctx context.Context, clusterName string, servi
 // EnsureLoadBalancer creates a new load balancer 'name', or updates the existing one. Returns the status of the balancer
 func (az *Cloud) EnsureLoadBalancer(ctx context.Context, clusterName string, service *v1.Service, nodes []*v1.Node) (*v1.LoadBalancerStatus, error) {
 	// When a client updates the internal load balancer annotation,
-	// the service may be switched from an internal LB to a public one, or vise versa.
+	// the service may be switched from an internal LB to a public one, or vice versa.
 	// Here we'll firstly ensure service do not lie in the opposite LB.
 
 	// Serialize service reconcile process
@@ -543,7 +543,7 @@ func (az *Cloud) safeDeleteLoadBalancer(lb network.LoadBalancer, clusterName, vm
 // 1. Using multiple slbs and the vmSet is supposed to share the primary slb.
 // 2. When migrating from multiple slbs to single slb mode.
 // It also ensures those vmSets are joint the backend pools of the primary SLBs.
-// It runs only once everytime the cloud controller manager restarts.
+// It runs only once every time the cloud controller manager restarts.
 func (az *Cloud) reconcileSharedLoadBalancer(service *v1.Service, clusterName string, nodes []*v1.Node) ([]network.LoadBalancer, error) {
 	var (
 		existingLBs []network.LoadBalancer
@@ -912,19 +912,36 @@ func (az *Cloud) determinePublicIPName(clusterName string, service *v1.Service)
 }
 
 func (az *Cloud) findMatchedPIPByLoadBalancerIP(service *v1.Service, loadBalancerIP, pipResourceGroup string) (*network.PublicIPAddress, error) {
-	pips, err := az.listPIP(pipResourceGroup)
+	pips, err := az.listPIP(pipResourceGroup, azcache.CacheReadTypeDefault)
 	if err != nil {
 		return nil, fmt.Errorf("findMatchedPIPByLoadBalancerIP: failed to listPIP: %w", err)
 	}
+
+	pip, err := getExpectedPIPFromListByIPAddress(pips, loadBalancerIP)
+	if err != nil {
+		pips, err = az.listPIP(pipResourceGroup, azcache.CacheReadTypeForceRefresh)
+		if err != nil {
+			return nil, fmt.Errorf("findMatchedPIPByLoadBalancerIP: failed to listPIP force refresh: %w", err)
+		}
+
+		pip, err = getExpectedPIPFromListByIPAddress(pips, loadBalancerIP)
+		if err != nil {
+			return nil, fmt.Errorf("findMatchedPIPByLoadBalancerIP: cannot find public IP with IP address %s in resource group %s", loadBalancerIP, pipResourceGroup)
+		}
+	}
+
+	return pip, nil
+}
+
+func getExpectedPIPFromListByIPAddress(pips []network.PublicIPAddress, ip string) (*network.PublicIPAddress, error) {
 	for _, pip := range pips {
-		pip := pip
 		if pip.PublicIPAddressPropertiesFormat.IPAddress != nil &&
-			*pip.PublicIPAddressPropertiesFormat.IPAddress == loadBalancerIP {
+			*pip.PublicIPAddressPropertiesFormat.IPAddress == ip {
 			return &pip, nil
 		}
 	}
 
-	return nil, fmt.Errorf("findMatchedPIPByLoadBalancerIP: cannot find public IP with IP address %s in resource group %s", loadBalancerIP, pipResourceGroup)
+	return nil, fmt.Errorf("getExpectedPIPFromListByIPAddress: cannot find public IP with IP address %s", ip)
 }
 
 func flipServiceInternalAnnotation(service *v1.Service) *v1.Service {
@@ -1088,7 +1105,7 @@ func (az *Cloud) ensurePublicIPExists(service *v1.Service, pipName string, domai
 	}
 
 	if foundDNSLabelAnnotation {
-		updatedDNSSettings, err := reconcileDNSSettings(&pip, domainNameLabel, serviceName, pipName)
+		updatedDNSSettings, err := reconcileDNSSettings(&pip, domainNameLabel, serviceName, pipName, isUserAssignedPIP)
 		if err != nil {
 			return nil, fmt.Errorf("ensurePublicIPExists for service(%s): failed to reconcileDNSSettings: %w", serviceName, err)
 		}
@@ -1158,7 +1175,11 @@ func (az *Cloud) reconcileIPSettings(pip *network.PublicIPAddress, service *v1.S
 	return changed
 }
 
-func reconcileDNSSettings(pip *network.PublicIPAddress, domainNameLabel, serviceName, pipName string) (bool, error) {
+func reconcileDNSSettings(
+	pip *network.PublicIPAddress,
+	domainNameLabel, serviceName, pipName string,
+	isUserAssignedPIP bool,
+) (bool, error) {
 	var changed bool
 
 	if existingServiceName := getServiceFromPIPDNSTags(pip.Tags); existingServiceName != "" && !strings.EqualFold(existingServiceName, serviceName) {
@@ -1187,8 +1208,10 @@ func reconcileDNSSettings(pip *network.PublicIPAddress, domainNameLabel, service
 		}
 
 		if svc := getServiceFromPIPDNSTags(pip.Tags); svc == "" || !strings.EqualFold(svc, serviceName) {
-			pip.Tags[consts.ServiceUsingDNSKey] = &serviceName
-			changed = true
+			if !isUserAssignedPIP {
+				pip.Tags[consts.ServiceUsingDNSKey] = &serviceName
+				changed = true
+			}
 		}
 	}
 
@@ -3028,7 +3051,7 @@ func (az *Cloud) reconcilePublicIP(clusterName string, service *v1.Service, lbNa
 
 	pipResourceGroup := az.getPublicIPAddressResourceGroup(service)
 
-	pips, err := az.listPIP(pipResourceGroup)
+	pips, err := az.listPIP(pipResourceGroup, azcache.CacheReadTypeDefault)
 	if err != nil {
 		return nil, err
 	}
@@ -3122,7 +3145,7 @@ func (az *Cloud) getPublicIPUpdates(
 		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)
 				if err = unbindServiceFromPIP(&pip, service, serviceName, clusterName, isUserAssignedPIP); err != nil {
 					return false, nil, false, nil, err
@@ -3167,65 +3190,77 @@ func (az *Cloud) getPublicIPUpdates(
 func (az *Cloud) safeDeletePublicIP(service *v1.Service, pipResourceGroup string, pip *network.PublicIPAddress, lb *network.LoadBalancer) error {
 	// Remove references if pip.IPConfiguration is not nil.
 	if pip.PublicIPAddressPropertiesFormat != nil &&
-		pip.PublicIPAddressPropertiesFormat.IPConfiguration != nil &&
-		lb != nil && lb.LoadBalancerPropertiesFormat != nil &&
-		lb.LoadBalancerPropertiesFormat.FrontendIPConfigurations != nil {
-		referencedLBRules := []network.SubResource{}
-		frontendIPConfigUpdated := false
-		loadBalancerRuleUpdated := false
-
-		// Check whether there are still frontend IP configurations referring to it.
-		ipConfigurationID := pointer.StringDeref(pip.PublicIPAddressPropertiesFormat.IPConfiguration.ID, "")
-		if ipConfigurationID != "" {
-			lbFrontendIPConfigs := *lb.LoadBalancerPropertiesFormat.FrontendIPConfigurations
-			for i := len(lbFrontendIPConfigs) - 1; i >= 0; i-- {
-				config := lbFrontendIPConfigs[i]
-				if strings.EqualFold(ipConfigurationID, pointer.StringDeref(config.ID, "")) {
-					if config.FrontendIPConfigurationPropertiesFormat != nil &&
-						config.FrontendIPConfigurationPropertiesFormat.LoadBalancingRules != nil {
-						referencedLBRules = *config.FrontendIPConfigurationPropertiesFormat.LoadBalancingRules
-					}
+		pip.PublicIPAddressPropertiesFormat.IPConfiguration != nil {
+		// Fetch latest pip to check if the pip in the cache is stale.
+		// In some cases the public IP to be deleted is still referencing
+		// the frontend IP config on the LB. This is because the pip is
+		// stored in the cache and is not up-to-date.
+		latestPIP, ok, err := az.getPublicIPAddress(pipResourceGroup, *pip.Name, azcache.CacheReadTypeForceRefresh)
+		if err != nil {
+			klog.Errorf("safeDeletePublicIP: failed to get latest public IP %s/%s: %s", pipResourceGroup, *pip.Name, err.Error())
+			return err
+		}
+		if ok && latestPIP.PublicIPAddressPropertiesFormat != nil &&
+			latestPIP.PublicIPAddressPropertiesFormat.IPConfiguration != nil &&
+			lb != nil && lb.LoadBalancerPropertiesFormat != nil &&
+			lb.LoadBalancerPropertiesFormat.FrontendIPConfigurations != nil {
+			referencedLBRules := []network.SubResource{}
+			frontendIPConfigUpdated := false
+			loadBalancerRuleUpdated := false
+
+			// Check whether there are still frontend IP configurations referring to it.
+			ipConfigurationID := pointer.StringDeref(pip.PublicIPAddressPropertiesFormat.IPConfiguration.ID, "")
+			if ipConfigurationID != "" {
+				lbFrontendIPConfigs := *lb.LoadBalancerPropertiesFormat.FrontendIPConfigurations
+				for i := len(lbFrontendIPConfigs) - 1; i >= 0; i-- {
+					config := lbFrontendIPConfigs[i]
+					if strings.EqualFold(ipConfigurationID, pointer.StringDeref(config.ID, "")) {
+						if config.FrontendIPConfigurationPropertiesFormat != nil &&
+							config.FrontendIPConfigurationPropertiesFormat.LoadBalancingRules != nil {
+							referencedLBRules = *config.FrontendIPConfigurationPropertiesFormat.LoadBalancingRules
+						}
 
-					frontendIPConfigUpdated = true
-					lbFrontendIPConfigs = append(lbFrontendIPConfigs[:i], lbFrontendIPConfigs[i+1:]...)
-					break
+						frontendIPConfigUpdated = true
+						lbFrontendIPConfigs = append(lbFrontendIPConfigs[:i], lbFrontendIPConfigs[i+1:]...)
+						break
+					}
 				}
-			}
 
-			if frontendIPConfigUpdated {
-				lb.LoadBalancerPropertiesFormat.FrontendIPConfigurations = &lbFrontendIPConfigs
+				if frontendIPConfigUpdated {
+					lb.LoadBalancerPropertiesFormat.FrontendIPConfigurations = &lbFrontendIPConfigs
+				}
 			}
-		}
 
-		// Check whether there are still load balancer rules referring to it.
-		if len(referencedLBRules) > 0 {
-			referencedLBRuleIDs := sets.NewString()
-			for _, refer := range referencedLBRules {
-				referencedLBRuleIDs.Insert(pointer.StringDeref(refer.ID, ""))
-			}
+			// Check whether there are still load balancer rules referring to it.
+			if len(referencedLBRules) > 0 {
+				referencedLBRuleIDs := sets.NewString()
+				for _, refer := range referencedLBRules {
+					referencedLBRuleIDs.Insert(pointer.StringDeref(refer.ID, ""))
+				}
 
-			if lb.LoadBalancerPropertiesFormat.LoadBalancingRules != nil {
-				lbRules := *lb.LoadBalancerPropertiesFormat.LoadBalancingRules
-				for i := len(lbRules) - 1; i >= 0; i-- {
-					ruleID := pointer.StringDeref(lbRules[i].ID, "")
-					if ruleID != "" && referencedLBRuleIDs.Has(ruleID) {
-						loadBalancerRuleUpdated = true
-						lbRules = append(lbRules[:i], lbRules[i+1:]...)
+				if lb.LoadBalancerPropertiesFormat.LoadBalancingRules != nil {
+					lbRules := *lb.LoadBalancerPropertiesFormat.LoadBalancingRules
+					for i := len(lbRules) - 1; i >= 0; i-- {
+						ruleID := pointer.StringDeref(lbRules[i].ID, "")
+						if ruleID != "" && referencedLBRuleIDs.Has(ruleID) {
+							loadBalancerRuleUpdated = true
+							lbRules = append(lbRules[:i], lbRules[i+1:]...)
+						}
 					}
-				}
 
-				if loadBalancerRuleUpdated {
-					lb.LoadBalancerPropertiesFormat.LoadBalancingRules = &lbRules
+					if loadBalancerRuleUpdated {
+						lb.LoadBalancerPropertiesFormat.LoadBalancingRules = &lbRules
+					}
 				}
 			}
-		}
 
-		// Update load balancer when frontendIPConfigUpdated or loadBalancerRuleUpdated.
-		if frontendIPConfigUpdated || loadBalancerRuleUpdated {
-			err := az.CreateOrUpdateLB(service, *lb)
-			if err != nil {
-				klog.Errorf("safeDeletePublicIP for service(%s) failed with error: %v", getServiceName(service), err)
-				return err
+			// Update load balancer when frontendIPConfigUpdated or loadBalancerRuleUpdated.
+			if frontendIPConfigUpdated || loadBalancerRuleUpdated {
+				err := az.CreateOrUpdateLB(service, *lb)
+				if err != nil {
+					klog.Errorf("safeDeletePublicIP for service(%s) failed with error: %v", getServiceName(service), err)
+					return err
+				}
 			}
 		}
 	}
@@ -3490,7 +3525,7 @@ func serviceOwnsPublicIP(service *v1.Service, pip *network.PublicIPAddress, clus
 
 		// if there is no service tag on the pip, it is user-created pip
 		if serviceTag == "" {
-			return strings.EqualFold(pointer.StringDeref(pip.IPAddress, ""), getServiceLoadBalancerIP(service)), true
+			return isServiceLoadBalancerIPMatchesPIP(service, pip), true
 		}
 
 		// if there is service tag on the pip, it is system-created pip
@@ -3502,17 +3537,20 @@ func serviceOwnsPublicIP(service *v1.Service, pip *network.PublicIPAddress, clus
 			}
 
 			// 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(pointer.StringDeref(pip.IPAddress, ""), getServiceLoadBalancerIP(service)), false
+			return strings.EqualFold(clusterTag, clusterName), false
 		}
+
+		// if the service is not included in the tags of the system-created pip, check the ip address
+		// this could happen for secondary services
+		return isServiceLoadBalancerIPMatchesPIP(service, pip), false
 	}
 
-	return false, false
+	// if the pip has no tags, it should be user-created
+	return isServiceLoadBalancerIPMatchesPIP(service, pip), true
+}
+
+func isServiceLoadBalancerIPMatchesPIP(service *v1.Service, pip *network.PublicIPAddress) bool {
+	return strings.EqualFold(pointer.StringDeref(pip.IPAddress, ""), getServiceLoadBalancerIP(service))
 }
 
 func isSVCNameInPIPTag(tag, svcName string) bool {
diff --git a/pkg/provider/azure_wrap.go b/pkg/provider/azure_wrap.go
index 9982eb4b4..c6239417b 100644
--- a/pkg/provider/azure_wrap.go
+++ b/pkg/provider/azure_wrap.go
@@ -125,8 +125,8 @@ func (az *Cloud) getPublicIPAddress(pipResourceGroup string, pipName string, crt
 	return *(deepcopy.Copy(pip).(*network.PublicIPAddress)), true, nil
 }
 
-func (az *Cloud) listPIP(pipResourceGroup string) ([]network.PublicIPAddress, error) {
-	cached, err := az.pipCache.Get(pipResourceGroup, azcache.CacheReadTypeDefault)
+func (az *Cloud) listPIP(pipResourceGroup string, crt azcache.AzureCacheReadType) ([]network.PublicIPAddress, error) {
+	cached, err := az.pipCache.Get(pipResourceGroup, crt)
 	if err != nil {
 		return nil, err
 	}
diff --git a/site/content/en/topics/shared-ip.md b/site/content/en/topics/shared-ip.md
index ab5fb264c..8e61eaf26 100644
--- a/site/content/en/topics/shared-ip.md
+++ b/site/content/en/topics/shared-ip.md
@@ -35,7 +35,8 @@ kind: Service
 metadata:
   name: https
   namespace: default
-  service.beta.kubernetes.io/azure-load-balancer-ipv4: 1.2.3.4 # the IP address could be the same as it is of `nginx` service
+  annotations:
+    service.beta.kubernetes.io/azure-load-balancer-ipv4: 1.2.3.4 # the IP address could be the same as it is of `nginx` service
 spec:
   ports:
     - port: 443
@@ -50,9 +51,9 @@ Note that if you specify the annotations `service.beta.kubernetes.io/azure-load-
 
 ## DNS
 
-Even if multiple services can refer to one public IP, the DNS label cannot be re-used. The public IP would have the label `kubernetes-dns-label-service: <svcName>` to indicate which service is binding to the DNS label. In this case if there is another service sharing this specific IP address trying to refer to the DNS label, an error would be reported.
+Even if multiple services can refer to one public IP, the DNS label cannot be re-used. The public IP would have the label `kubernetes-dns-label-service: <svcName>` to indicate which service is binding to the DNS label. In this case if there is another service sharing this specific IP address trying to refer to the DNS label, an error would be reported. For managed public IPs, this label will be added automatically by the cloud provider. For static public IPs, this label should be added manually.
 
-> The DNS name on the public IP won't be deleted after the service with the DNS annotation being deleted, because the cloud provider don't know if the DNS was set by the user or not.
+```yaml
 
 ## Restrictions
 


Test patch:
diff --git a/pkg/provider/azure_loadbalancer_test.go b/pkg/provider/azure_loadbalancer_test.go
index 250cd1c99..c6ab5f094 100644
--- a/pkg/provider/azure_loadbalancer_test.go
+++ b/pkg/provider/azure_loadbalancer_test.go
@@ -959,6 +959,17 @@ func TestServiceOwnsPublicIP(t *testing.T) {
 			serviceLBIP:  "1.1.1.1",
 			expectedOwns: true,
 		},
+		{
+			desc: "should be user-assigned pip if it has no tags",
+			pip: &network.PublicIPAddress{
+				PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
+					IPAddress: pointer.String("1.2.3.4"),
+				},
+			},
+			serviceLBIP:             "1.2.3.4",
+			expectedOwns:            true,
+			expectedUserAssignedPIP: true,
+		},
 	}
 
 	for i, c := range tests {
@@ -2059,10 +2070,11 @@ func TestFindMatchedPIPByLoadBalancerIP(t *testing.T) {
 		},
 	}
 	testCases := []struct {
-		desc          string
-		pips          []network.PublicIPAddress
-		expectedPIP   *network.PublicIPAddress
-		expectedError bool
+		desc               string
+		pips               []network.PublicIPAddress
+		shouldRefreshCache bool
+		expectedPIP        *network.PublicIPAddress
+		expectedError      bool
 	}{
 		{
 			desc:        "findMatchedPIPByLoadBalancerIP shall return the matched ip",
@@ -2070,9 +2082,16 @@ func TestFindMatchedPIPByLoadBalancerIP(t *testing.T) {
 			expectedPIP: &testPIP,
 		},
 		{
-			desc:          "findMatchedPIPByLoadBalancerIP shall return error if ip is not found",
-			pips:          []network.PublicIPAddress{},
-			expectedError: true,
+			desc:               "findMatchedPIPByLoadBalancerIP shall return error if ip is not found",
+			pips:               []network.PublicIPAddress{},
+			shouldRefreshCache: true,
+			expectedError:      true,
+		},
+		{
+			desc:               "findMatchedPIPByLoadBalancerIP should refresh cache if no matched ip is found",
+			pips:               []network.PublicIPAddress{testPIP},
+			shouldRefreshCache: true,
+			expectedPIP:        &testPIP,
 		},
 	}
 	for _, test := range testCases {
@@ -2082,6 +2101,9 @@ func TestFindMatchedPIPByLoadBalancerIP(t *testing.T) {
 			setServiceLoadBalancerIP(&service, "1.2.3.4")
 
 			mockPIPsClient := az.PublicIPAddressesClient.(*mockpublicipclient.MockInterface)
+			if test.shouldRefreshCache {
+				mockPIPsClient.EXPECT().List(gomock.Any(), "rg").Return([]network.PublicIPAddress{}, nil)
+			}
 			mockPIPsClient.EXPECT().List(gomock.Any(), "rg").Return(test.pips, nil)
 			pip, err := az.findMatchedPIPByLoadBalancerIP(&service, "1.2.3.4", "rg")
 			assert.Equal(t, test.expectedPIP, pip)
@@ -2137,7 +2159,7 @@ func TestDeterminePublicIPName(t *testing.T) {
 			setServiceLoadBalancerIP(&service, test.loadBalancerIP)
 
 			mockPIPsClient := az.PublicIPAddressesClient.(*mockpublicipclient.MockInterface)
-			mockPIPsClient.EXPECT().List(gomock.Any(), "rg").Return(test.existingPIPs, nil).MaxTimes(1)
+			mockPIPsClient.EXPECT().List(gomock.Any(), "rg").Return(test.existingPIPs, nil).MaxTimes(2)
 			mockPIPsClient.EXPECT().CreateOrUpdate(gomock.Any(), "rg", gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
 			for _, existingPIP := range test.existingPIPs {
 				mockPIPsClient.EXPECT().Get(gomock.Any(), "rg", *existingPIP.Name, gomock.Any()).Return(existingPIP, nil).AnyTimes()
@@ -3859,7 +3881,8 @@ func TestSafeDeletePublicIP(t *testing.T) {
 		desc          string
 		pip           *network.PublicIPAddress
 		lb            *network.LoadBalancer
-		expectedError bool
+		listError     *retry.Error
+		expectedError error
 	}{
 		{
 			desc: "safeDeletePublicIP shall delete corresponding ip configurations and lb rules",
@@ -3886,12 +3909,30 @@ func TestSafeDeletePublicIP(t *testing.T) {
 				},
 			},
 		},
+		{
+			desc: "safeDeletePublicIP should return error if failed to list pip",
+			pip: &network.PublicIPAddress{
+				Name: pointer.String("pip1"),
+				PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
+					IPConfiguration: &network.IPConfiguration{
+						ID: pointer.String("id1"),
+					},
+				},
+			},
+			listError:     retry.NewError(false, errors.New("error")),
+			expectedError: retry.NewError(false, errors.New("error")).Error(),
+		},
 	}
 
 	for _, test := range testCases {
 		t.Run(test.desc, func(t *testing.T) {
 			az := GetTestCloud(ctrl)
 			mockPIPsClient := az.PublicIPAddressesClient.(*mockpublicipclient.MockInterface)
+			if test.pip != nil &&
+				test.pip.PublicIPAddressPropertiesFormat != nil &&
+				test.pip.IPConfiguration != nil {
+				mockPIPsClient.EXPECT().List(gomock.Any(), gomock.Any()).Return([]network.PublicIPAddress{*test.pip}, test.listError)
+			}
 			mockPIPsClient.EXPECT().CreateOrUpdate(gomock.Any(), "rg", "pip1", gomock.Any()).Return(nil).AnyTimes()
 			mockPIPsClient.EXPECT().Delete(gomock.Any(), "rg", "pip1").Return(nil).AnyTimes()
 			err := az.PublicIPAddressesClient.CreateOrUpdate(context.TODO(), "rg", "pip1", network.PublicIPAddress{
@@ -3904,13 +3945,19 @@ func TestSafeDeletePublicIP(t *testing.T) {
 			})
 			assert.NoError(t, err.Error())
 			service := getTestService("test1", v1.ProtocolTCP, nil, false, 80)
-			mockLBsClient := mockloadbalancerclient.NewMockInterface(ctrl)
-			mockLBsClient.EXPECT().CreateOrUpdate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
-			az.LoadBalancerClient = mockLBsClient
+			if test.listError == nil {
+				mockLBsClient := mockloadbalancerclient.NewMockInterface(ctrl)
+				mockLBsClient.EXPECT().CreateOrUpdate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
+				az.LoadBalancerClient = mockLBsClient
+			}
 			rerr := az.safeDeletePublicIP(&service, "rg", test.pip, test.lb)
-			assert.Equal(t, 0, len(*test.lb.FrontendIPConfigurations))
-			assert.Equal(t, 0, len(*test.lb.LoadBalancingRules))
-			assert.Equal(t, test.expectedError, rerr != nil)
+			if test.expectedError == nil {
+				assert.Equal(t, 0, len(*test.lb.FrontendIPConfigurations))
+				assert.Equal(t, 0, len(*test.lb.LoadBalancingRules))
+				assert.NoError(t, rerr)
+			} else {
+				assert.Equal(t, rerr.Error(), test.listError.Error().Error())
+			}
 		})
 	}
 }
diff --git a/pkg/provider/azure_standard_test.go b/pkg/provider/azure_standard_test.go
index cd291c449..ae68aecf9 100644
--- a/pkg/provider/azure_standard_test.go
+++ b/pkg/provider/azure_standard_test.go
@@ -1809,7 +1809,7 @@ func TestServiceOwnsFrontendIP(t *testing.T) {
 			cloud := GetTestCloud(ctrl)
 			if test.existingPIPs != nil {
 				mockPIPsClient := cloud.PublicIPAddressesClient.(*mockpublicipclient.MockInterface)
-				mockPIPsClient.EXPECT().List(gomock.Any(), "rg").Return(test.existingPIPs, nil)
+				mockPIPsClient.EXPECT().List(gomock.Any(), "rg").Return(test.existingPIPs, nil).MaxTimes(2)
 			}
 			isOwned, isPrimary, err := cloud.serviceOwnsFrontendIP(test.fip, test.service)
 			assert.Equal(t, test.expectedErr, err)
diff --git a/pkg/provider/azure_wrap_test.go b/pkg/provider/azure_wrap_test.go
index f8ca17cce..da7241648 100644
--- a/pkg/provider/azure_wrap_test.go
+++ b/pkg/provider/azure_wrap_test.go
@@ -396,7 +396,7 @@ func TestListPIP(t *testing.T) {
 			if test.expectPIPList {
 				mockPIPsClient.EXPECT().List(gomock.Any(), az.ResourceGroup).Return(test.existingPIPs, nil).MaxTimes(2)
 			}
-			pips, err := az.listPIP(az.ResourceGroup)
+			pips, err := az.listPIP(az.ResourceGroup, azcache.CacheReadTypeDefault)
 			if test.expectPIPList {
 				assert.ElementsMatch(t, test.existingPIPs, pips)
 			} else {
diff --git a/tests/e2e/network/service_annotations.go b/tests/e2e/network/service_annotations.go
index 1ed013b49..43bd33473 100644
--- a/tests/e2e/network/service_annotations.go
+++ b/tests/e2e/network/service_annotations.go
@@ -191,7 +191,7 @@ var _ = Describe("Service with annotation", Label(utils.TestSuiteLabelServiceAnn
 		err = utils.DeleteService(cs, ns.Name, oldServiceName)
 		Expect(err).NotTo(HaveOccurred())
 
-		By("Check if PIP DNS label is deleted")
+		By("Check if PIP DNS label is not tagged onto the user-assigned pip")
 		deleted, err := ifPIPDNSLabelDeleted(tc, pipName)
 		Expect(err).NotTo(HaveOccurred())
 		Expect(deleted).To(BeTrue())