Repository: kubernetes-sigs/cloud-provider-azure
Pull request #558 (2021-03-15 08:31:29)

feat: support system tag
<!--  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 feature

**What this PR does / why we need it**:

| Tags | SystemTags | existing tags on resources | new tags on resources |
| ----- | ------------ | ----- | ----- |
| "a=b,c=d" | "" | {} | {"a": "b", "c": "d"} |
| "a=b,c=d" | "" | {"a": "x", "c": "y"} | {"a": "b", "c": "d"} |
| "a=b,c=d" | "" | {"e": "f"} | {"a": "b", "c": "d", "e": "f"} /* won't delete `e` because the SystemTags is empty */ |
| "c=d" | "a" | {"a": "b"} | {"a": "b", "c": "d"} /* won't delete `a` because it's in the SystemTags */ |
| "c=d" | "x" | {"a": "b"} | {"c": "d"} /* will delete `a` because it's not in Tags or SystemTags */ |

**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 https://github.com/kubernetes-sigs/cloud-provider-azure/issues/499

**Special notes for your reviewer**:


**Release note**:
```
feat: support system tag
```


k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/cloud-provider-azure/pull/558#" title="Author self-approved">nilo19</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fcloud-provider-azure).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubernetes-sigs/cloud-provider-azure/blob/master/OWNERS)~~ [nilo19]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
nilo19: would enrich the doc after this one is merged

Patch:
diff --git a/pkg/consts/consts.go b/pkg/consts/consts.go
index d84e966cc..ea54fd297 100644
--- a/pkg/consts/consts.go
+++ b/pkg/consts/consts.go
@@ -115,6 +115,11 @@ const (
 	VMSetCIDRIPV4TagKey = "kubernetesNodeCIDRMaskIPV4"
 	// VMSetCIDRIPV6TagKey specifies the node ipv6 CIDR mask of the instances on the VMSS or VMAS
 	VMSetCIDRIPV6TagKey = "kubernetesNodeCIDRMaskIPV6"
+
+	// TagsDelimiter is the delimiter of tags
+	TagsDelimiter = ","
+	// TagKeyValueDelimiter is the delimiter between keys and values in tagas
+	TagKeyValueDelimiter = "="
 )
 
 // azure cloud config
diff --git a/pkg/provider/azure.go b/pkg/provider/azure.go
index 98f122697..34289ae08 100644
--- a/pkg/provider/azure.go
+++ b/pkg/provider/azure.go
@@ -138,6 +138,10 @@ type Config struct {
 	// includes load balancer, security group and route table. The supported format is `a=b,c=d,...`. After updated
 	// this config, the old tags would be replaced by the new ones.
 	Tags string `json:"tags,omitempty" yaml:"tags,omitempty"`
+	// SystemTags determines the tag keys managed by cloud provider. If it is not set, no tags would be deleted if
+	// the `Tags` is changed. However, the old tags would be deleted if they are neither included in `Tags` nor
+	// in `SystemTags` after the update of `Tags`.
+	SystemTags string `json:"systemTags,omitempty" yaml:"systemTags,omitempty"`
 	// Sku of Load Balancer and Public IP. Candidate values are: basic and standard.
 	// If not set, it will be default to basic.
 	LoadBalancerSku string `json:"loadBalancerSku,omitempty" yaml:"loadBalancerSku,omitempty"`
diff --git a/pkg/provider/azure_loadbalancer.go b/pkg/provider/azure_loadbalancer.go
index afccd1597..df4d486a8 100644
--- a/pkg/provider/azure_loadbalancer.go
+++ b/pkg/provider/azure_loadbalancer.go
@@ -2264,9 +2264,8 @@ func shouldReleaseExistingOwnedPublicIP(existingPip *network.PublicIPAddress, lb
 		(ipTagRequest.IPTagsRequestedByAnnotation && !areIPTagsEquivalent(currentIPTags, ipTagRequest.IPTags))
 }
 
-//  ensurePIPTagged ensures the public IP of the service is tagged as configured
+// ensurePIPTagged ensures the public IP of the service is tagged as configured
 func (az *Cloud) ensurePIPTagged(service *v1.Service, pip *network.PublicIPAddress) bool {
-	changed := false
 	configTags := parseTags(az.Tags)
 	annotationTags := make(map[string]*string)
 	if _, ok := service.Annotations[consts.ServiceAnnotationAzurePIPTags]; ok {
@@ -2275,6 +2274,7 @@ func (az *Cloud) ensurePIPTagged(service *v1.Service, pip *network.PublicIPAddre
 	for k, v := range annotationTags {
 		configTags[k] = v
 	}
+
 	// include the cluster name and service names tags when comparing
 	var clusterName, serviceNames *string
 	if v, ok := pip.Tags[consts.ClusterNameKey]; ok {
@@ -2289,12 +2289,10 @@ func (az *Cloud) ensurePIPTagged(service *v1.Service, pip *network.PublicIPAddre
 	if serviceNames != nil {
 		configTags[consts.ServiceTagKey] = serviceNames
 	}
-	for k, v := range configTags {
-		if vv, ok := pip.Tags[k]; !ok || !strings.EqualFold(to.String(v), to.String(vv)) {
-			pip.Tags[k] = v
-			changed = true
-		}
-	}
+
+	tags, changed := az.reconcileTags(pip.Tags, configTags)
+	pip.Tags = tags
+
 	return changed
 }
 
@@ -2821,7 +2819,6 @@ func unbindServiceFromPIP(pip *network.PublicIPAddress, serviceName string) erro
 
 // ensureLoadBalancerTagged ensures every load balancer in the resource group is tagged as configured
 func (az *Cloud) ensureLoadBalancerTagged(lb *network.LoadBalancer) bool {
-	changed := false
 	if az.Tags == "" {
 		return false
 	}
@@ -2829,18 +2826,15 @@ func (az *Cloud) ensureLoadBalancerTagged(lb *network.LoadBalancer) bool {
 	if lb.Tags == nil {
 		lb.Tags = make(map[string]*string)
 	}
-	for k, v := range tags {
-		if vv, ok := lb.Tags[k]; !ok || !strings.EqualFold(to.String(v), to.String(vv)) {
-			lb.Tags[k] = v
-			changed = true
-		}
-	}
+
+	tags, changed := az.reconcileTags(lb.Tags, tags)
+	lb.Tags = tags
+
 	return changed
 }
 
 // ensureSecurityGroupTagged ensures the security group is tagged as configured
 func (az *Cloud) ensureSecurityGroupTagged(sg *network.SecurityGroup) bool {
-	changed := false
 	if az.Tags == "" {
 		return false
 	}
@@ -2848,11 +2842,9 @@ func (az *Cloud) ensureSecurityGroupTagged(sg *network.SecurityGroup) bool {
 	if sg.Tags == nil {
 		sg.Tags = make(map[string]*string)
 	}
-	for k, v := range tags {
-		if vv, ok := sg.Tags[k]; !ok || !strings.EqualFold(to.String(v), to.String(vv)) {
-			sg.Tags[k] = v
-			changed = true
-		}
-	}
+
+	tags, changed := az.reconcileTags(sg.Tags, tags)
+	sg.Tags = tags
+
 	return changed
 }
diff --git a/pkg/provider/azure_routes.go b/pkg/provider/azure_routes.go
index 0965d2c3c..e72ab7b57 100644
--- a/pkg/provider/azure_routes.go
+++ b/pkg/provider/azure_routes.go
@@ -505,21 +505,18 @@ func cidrtoRfc1035(cidr string) string {
 	return cidr
 }
 
-//  ensureRouteTableTagged ensures the route table is tagged as configured
+// ensureRouteTableTagged ensures the route table is tagged as configured
 func (az *Cloud) ensureRouteTableTagged(rt *network.RouteTable) (map[string]*string, bool) {
 	if az.Tags == "" {
 		return nil, false
 	}
-	changed := false
 	tags := parseTags(az.Tags)
 	if rt.Tags == nil {
 		rt.Tags = make(map[string]*string)
 	}
-	for k, v := range tags {
-		if vv, ok := rt.Tags[k]; !ok || !strings.EqualFold(to.String(v), to.String(vv)) {
-			rt.Tags[k] = v
-			changed = true
-		}
-	}
+
+	tags, changed := az.reconcileTags(rt.Tags, tags)
+	rt.Tags = tags
+
 	return rt.Tags, changed
 }
diff --git a/pkg/provider/azure_utils.go b/pkg/provider/azure_utils.go
index cc8daf4d4..58ac42cbc 100644
--- a/pkg/provider/azure_utils.go
+++ b/pkg/provider/azure_utils.go
@@ -18,18 +18,16 @@ package provider
 
 import (
 	"context"
-	"fmt"
 	"strings"
 	"sync"
 
+	"k8s.io/apimachinery/pkg/util/sets"
+
 	"github.com/Azure/go-autorest/autorest/to"
 
 	"k8s.io/klog/v2"
-)
 
-const (
-	tagsDelimiter        = ","
-	tagKeyValueDelimiter = "="
+	"sigs.k8s.io/cloud-provider-azure/pkg/consts"
 )
 
 // lockMap used to lock on entries
@@ -84,31 +82,6 @@ func getContextWithCancel() (context.Context, context.CancelFunc) {
 	return context.WithCancel(context.Background())
 }
 
-// ConvertTagsToMap convert the tags from string to map
-// the valid tags format is "key1=value1,key2=value2", which could be converted to
-// {"key1": "value1", "key2": "value2"}
-func ConvertTagsToMap(tags string) (map[string]string, error) {
-	m := make(map[string]string)
-	if tags == "" {
-		return m, nil
-	}
-	s := strings.Split(tags, tagsDelimiter)
-	for _, tag := range s {
-		kv := strings.Split(tag, tagKeyValueDelimiter)
-		if len(kv) != 2 {
-			return nil, fmt.Errorf("Tags '%s' are invalid, the format should like: 'key1=value1,key2=value2'", tags)
-		}
-		key := strings.TrimSpace(kv[0])
-		if key == "" {
-			return nil, fmt.Errorf("Tags '%s' are invalid, the format should like: 'key1=value1,key2=value2'", tags)
-		}
-		value := strings.TrimSpace(kv[1])
-		m[key] = value
-	}
-
-	return m, nil
-}
-
 func convertMapToMapPointer(origin map[string]string) map[string]*string {
 	newly := make(map[string]*string)
 	for k, v := range origin {
@@ -119,10 +92,10 @@ func convertMapToMapPointer(origin map[string]string) map[string]*string {
 }
 
 func parseTags(tags string) map[string]*string {
-	kvs := strings.Split(tags, ",")
+	kvs := strings.Split(tags, consts.TagsDelimiter)
 	formatted := make(map[string]*string)
 	for _, kv := range kvs {
-		res := strings.Split(kv, "=")
+		res := strings.Split(kv, consts.TagKeyValueDelimiter)
 		if len(res) != 2 {
 			klog.Warningf("parseTags: error when parsing key-value pair %s, would ignore this one", kv)
 			continue
@@ -136,3 +109,36 @@ func parseTags(tags string) map[string]*string {
 	}
 	return formatted
 }
+
+func (az *Cloud) reconcileTags(currentTagsOnResource, newTags map[string]*string) (reconciledTags map[string]*string, changed bool) {
+	var systemTags []string
+	var systemTagsMap sets.String
+
+	if az.SystemTags != "" {
+		systemTags = strings.Split(az.SystemTags, consts.TagsDelimiter)
+		for i := 0; i < len(systemTags); i++ {
+			systemTags[i] = strings.TrimSpace(systemTags[i])
+		}
+		systemTagsMap = sets.NewString(systemTags...)
+	}
+
+	// if the systemTags is not set, just add/update new currentTagsOnResource and not delete old currentTagsOnResource
+	for k, v := range newTags {
+		if vv, ok := currentTagsOnResource[k]; !ok || !strings.EqualFold(to.String(v), to.String(vv)) {
+			currentTagsOnResource[k] = v
+			changed = true
+		}
+	}
+
+	// if the systemTags is set, delete the old currentTagsOnResource
+	if len(systemTagsMap) > 0 {
+		for k := range currentTagsOnResource {
+			if _, ok := newTags[k]; !ok && !systemTagsMap.Has(k) {
+				delete(currentTagsOnResource, k)
+				changed = true
+			}
+		}
+	}
+
+	return currentTagsOnResource, changed
+}
diff --git a/site/content/en/development/design-docs/ipam.md b/site/content/en/development/design-docs/ipam.md
new file mode 100644
index 000000000..fd20ce645
--- /dev/null
+++ b/site/content/en/development/design-docs/ipam.md
@@ -0,0 +1,53 @@
+---
+title: "Out-of-tree Node IPAM Controller"
+linkTitle: "node IPAM controller"
+type: docs
+weight: 10
+description: "Support Node IPAM allocator in the out-of-tree provider Azure."
+---
+
+## Background
+
+The in-tree [Node IPAM controller](https://github.com/kubernetes/kubernetes/tree/master/pkg/controller/nodeipam) only supports a fixed node CIDR mask size for all nodes, while in multiple node pool (VMSS) scenarios, different mask sizes are required for different node pools. There is a GCE-specific cloud CIDR allocator for a similar scenario, but that is not exposed in cloud provider API and it is planned to be moved out-of-tree. 
+
+Hence this docs proposes an out-of-tree node IPAM controller. Specifically, allocate different pod CIDRs based on different CIDR mask size for different node pools (VMSS or VMAS).
+
+## User experience
+
+There are two kinds of CIDR allocator in the node IPAM controller, which are `RangeAllocator` and `CloudAllocator`.
+The `RangeAllocator` is the default one which allocates the pod CIDR for every node in the range of the cluster CIDR.
+The `CloudAllocator` allocates the pod CIDR for every node in the range of the CIDR on the corresponding VMSS or VMAS.
+
+The CIDR on the VMSS or VMAS is set by a specific tag `{"kubernetesNodeCIDRMaskSize": "24"}` and every node would get a 
+pod CIDR within the range. Note that the mask size in the VMSS or VMAS must be within the cluster CIDR, or an error would be 
+thrown.
+
+When the above tag doesn't exist on VMSS/VMAS, the values from `--node-cidr-mask-size` would be used as default mask size.
+
+## Configurations
+
+### kube-controller-manager
+
+kube-controller-manager would be configured with option `--allocate-node-cidrs=false` to indicate it won't allocate Node CIDRs.
+
+### cloud-controller-manager
+
+The following configurations from cloud-controller-manager would be used as default options:
+
+| name | type | default | description |
+| ----- | -----| ----- | ----- |
+| allocate-node-cidrs | bool | true | Should CIDRs for Pods be allocated and set on the cloud provider. |
+| cluster-cidr | string | "10.244.0.0/16" | CIDR Range for Pods in cluster. Requires --allocate-node-cidrs to be true |
+| service-cluster-ip-range | string | "" | CIDR Range for Services in cluster. Requires --allocate-node-cidrs to be true |
+| node-cidr-mask-size | int | 24 | Mask size for node cidr in cluster. Default is 24 for IPv4 and 64 for IPv6. |
+| node-cidr-mask-size-ipv4 | int | 24 | Mask size for IPv4 node cidr in dual-stack cluster. Default is 24. |
+| node-cidr-mask-size-ipv6 | int | 64 | Mask size for IPv6 node cidr in dual-stack cluster. Default is 64. |
+| cidr-allocator-type | string | "RangeAllocator" | The CIDR allocator type. "RangeAllocator" or "CloudAllocator" |
+
+For each node pool (VMSS or VMAS), tag `{"kubernetesNodeCIDRMaskSize": "24"}` could be added by demand and its values would be used for new node's CIDR allocation.
+
+## Open Questions
+
+Q: what would happen if the tag value gets changed after some time?
+A: In this case, existing node's nodeCIDR won't be changed, while new nodes' CIDR would be allocated with a new mask size.
+
diff --git a/site/content/en/topics/ipam.md b/site/content/en/topics/ipam.md
deleted file mode 100644
index 03b1e0cdc..000000000
--- a/site/content/en/topics/ipam.md
+++ /dev/null
@@ -1,71 +0,0 @@
----
-title: "Node IPAM controller"
-linkTitle: "Node IPAM controller"
-type: docs
-weight: 10
-description: "Usage of out-of-tree Node IPAM allocator."
----
-
-> This feature is supported since v1.21.0.
-
-## Background
-
-The in-tree [Node IPAM controller](https://github.com/kubernetes/kubernetes/tree/master/pkg/controller/nodeipam) only 
-supports a fixed node CIDR mask size for all nodes, while in multiple node pool (VMSS) scenarios, different mask sizes 
-are required for different node pools. There is a GCE-specific cloud CIDR allocator for a similar scenario, but that is 
-not exposed in cloud provider API and it is planned to be moved out-of-tree. 
-
-Hence this docs proposes an out-of-tree node IPAM controller. Specifically, allocate different pod CIDRs based on 
-different CIDR mask size for different node pools (VMSS or VMAS).
-
-## Usage
-
-There are two kinds of CIDR allocator in the node IPAM controller, which are `RangeAllocator` and `CloudAllocator`.
-The `RangeAllocator` is the default one which allocates the pod CIDR for every node in the range of the cluster CIDR.
-The `CloudAllocator` allocates the pod CIDR for every node in the range of the CIDR on the corresponding VMSS or VMAS.
-
-The pod CIDR mask size of each node that belongs to a specific VMSS or VMAS is set by a specific tag 
-`{"kubernetesNodeCIDRMaskIPV4": "24"}` or `{"kubernetesNodeCIDRMaskIPV6": "64"}`. Note that the mask size tagging on 
-the VMSS or VMAS must be within the cluster CIDR, or an error would be thrown.
-
-When the above tag doesn't exist on VMSS/VMAS, the default mask size (24 for ipv4 and 64 for ipv6) would be used.
-
-To turn on the out-of-tree node IPAM controller:
-1. Disable the in-tree node IPAM controller by setting `--allocate-node-cidrs=false` in kube-controller-manager.
-1. Enable the out-of-tree counterpart by setting `--allocate-node-cidrs=true` in cloud-controller-manager.
-1. To use `RangeAllocator`:
-    * configure the `--cluster-cidr`, `--service-cluster-ip-range` and `--node-cidr-mask-size`;
-    * if you enable the ipv6 dualstack, setting `--node-cidr-mask-size-ipv4` and `--node-cidr-mask-size-ipv6` instead of 
-      `--node-cidr-mask-size`.
-1. To use `CloudAllocator`:
-    * set the `--cidr-allocator-type=CloudAllocator`;
-    * configure mask sizes of each VMSS/VMAS by tagging `{"kubernetesNodeCIDRMaskIPV4": "custom-mask-size"}` and
-      `{"kubernetesNodeCIDRMaskIPV4": "custom-mask-size"}` if necessary.
-
-## Configurations
-
-### kube-controller-manager
-
-kube-controller-manager would be configured with option `--allocate-node-cidrs=false` to disable the in-tree node IPAM controller.
-
-### cloud-controller-manager
-
-The following configurations from cloud-controller-manager would be used as default options:
-
-| name | type | default | description |
-| ----- | -----| ----- | ----- |
-| allocate-node-cidrs | bool | true | Should CIDRs for Pods be allocated and set on the cloud provider. |
-| cluster-cidr | string | "10.244.0.0/16" | CIDR Range for Pods in cluster. Requires --allocate-node-cidrs to be true. |
-| service-cluster-ip-range | string | "" | CIDR Range for Services in cluster, this would get excluded from the allocatable range. Requires --allocate-node-cidrs to be true. |
-| node-cidr-mask-size | int | 24 | Mask size for node cidr in cluster. Default is 24 for IPv4 and 64 for IPv6. |
-| node-cidr-mask-size-ipv4 | int | 24 | Mask size for IPv4 node cidr in dual-stack cluster. Default is 24. |
-| node-cidr-mask-size-ipv6 | int | 64 | Mask size for IPv6 node cidr in dual-stack cluster. Default is 64. |
-| cidr-allocator-type | string | "RangeAllocator" | The CIDR allocator type. "RangeAllocator" or "CloudAllocator". |
-
-## Limitations
-
-1. We plan to integrate out-of-tree node ipam controller with aks-engine to provider a better experience. Before that, 
-the manual configuration is required.
-1. It is not supported to change the custom mask size value on the tag once it is set.
-1. For now, there is no e2e test covering this feature, so there can be potential bugs. It is not recommended enabling
-it in the production environment.


Test patch:
diff --git a/pkg/provider/azure_loadbalancer_test.go b/pkg/provider/azure_loadbalancer_test.go
index 4c4e72bca..c77163022 100644
--- a/pkg/provider/azure_loadbalancer_test.go
+++ b/pkg/provider/azure_loadbalancer_test.go
@@ -3767,27 +3767,48 @@ func buildDefaultTestLB(name string, backendIPConfigs []string) network.LoadBala
 }
 
 func TestEnsurePIPTagged(t *testing.T) {
-	t.Run("ensurePIPTagged should ensure the pip is tagged as configured", func(t *testing.T) {
-		ctrl := gomock.NewController(t)
-		defer ctrl.Finish()
-		cloud := GetTestCloud(ctrl)
-		cloud.Tags = "a=x,y=z"
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
 
-		service := v1.Service{
-			ObjectMeta: metav1.ObjectMeta{
-				Annotations: map[string]string{
-					consts.ServiceAnnotationAzurePIPTags: "a=b,c=d,e=,=f,ghi",
-				},
+	cloud := GetTestCloud(ctrl)
+	cloud.Tags = "a=x,y=z"
+
+	service := v1.Service{
+		ObjectMeta: metav1.ObjectMeta{
+			Annotations: map[string]string{
+				consts.ServiceAnnotationAzurePIPTags: "a=b,c=d,e=,=f,ghi",
 			},
-		}
-		pip := network.PublicIPAddress{
+		},
+	}
+	pip := network.PublicIPAddress{
+		Tags: map[string]*string{
+			consts.ClusterNameKey: to.StringPtr("testCluster"),
+			consts.ServiceTagKey:  to.StringPtr("default/svc1,default/svc2"),
+			"foo":                 to.StringPtr("bar"),
+			"a":                   to.StringPtr("j"),
+			"m":                   to.StringPtr("n"),
+		},
+	}
+
+	t.Run("ensurePIPTagged should ensure the pip is tagged as configured", func(t *testing.T) {
+		expectedPIP := network.PublicIPAddress{
 			Tags: map[string]*string{
 				consts.ClusterNameKey: to.StringPtr("testCluster"),
 				consts.ServiceTagKey:  to.StringPtr("default/svc1,default/svc2"),
 				"foo":                 to.StringPtr("bar"),
-				"a":                   to.StringPtr("j"),
+				"a":                   to.StringPtr("b"),
+				"c":                   to.StringPtr("d"),
+				"y":                   to.StringPtr("z"),
+				"m":                   to.StringPtr("n"),
 			},
 		}
+		changed := cloud.ensurePIPTagged(&service, &pip)
+		assert.True(t, changed)
+		assert.Equal(t, expectedPIP, pip)
+	})
+
+	t.Run("ensurePIPTagged should delete the old tags if tht SystemTags is set", func(t *testing.T) {
+		cloud.SystemTags = "a,foo"
 		expectedPIP := network.PublicIPAddress{
 			Tags: map[string]*string{
 				consts.ClusterNameKey: to.StringPtr("testCluster"),
@@ -3804,6 +3825,45 @@ func TestEnsurePIPTagged(t *testing.T) {
 	})
 }
 
+func TestEnsureLoadBalancerTagged(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	for _, tc := range []struct {
+		description               string
+		existedTags, expectedTags map[string]*string
+		newTags, systemTags       string
+		expectedChanged           bool
+	}{
+		{
+			description:     "ensureLoadBalancerTagged should not delete the old tags if SystemTags is not specified",
+			existedTags:     map[string]*string{"a": to.StringPtr("b")},
+			newTags:         "c=d",
+			expectedTags:    map[string]*string{"a": to.StringPtr("b"), "c": to.StringPtr("d")},
+			expectedChanged: true,
+		},
+		{
+			description:     "ensureLoadBalancerTagged should delete the old tags if SystemTags is specified",
+			existedTags:     map[string]*string{"a": to.StringPtr("b"), "c": to.StringPtr("d"), "h": to.StringPtr("i")},
+			newTags:         "c=e,f=g",
+			systemTags:      "a,x,y,z",
+			expectedTags:    map[string]*string{"a": to.StringPtr("b"), "c": to.StringPtr("e"), "f": to.StringPtr("g")},
+			expectedChanged: true,
+		},
+	} {
+		t.Run(tc.description, func(t *testing.T) {
+			cloud := GetTestCloud(ctrl)
+			cloud.Tags = tc.newTags
+			cloud.SystemTags = tc.systemTags
+			lb := &network.LoadBalancer{Tags: tc.existedTags}
+
+			changed := cloud.ensureLoadBalancerTagged(lb)
+			assert.Equal(t, tc.expectedChanged, changed)
+			assert.Equal(t, tc.expectedTags, lb.Tags)
+		})
+	}
+}
+
 func TestShouldChangeLoadBalancer(t *testing.T) {
 	t.Run("shouldChangeLoadBalancer should return true if the mode is different from the current vm set", func(t *testing.T) {
 		ctrl := gomock.NewController(t)
diff --git a/pkg/provider/azure_utils_test.go b/pkg/provider/azure_utils_test.go
index 203959383..f42f4552c 100644
--- a/pkg/provider/azure_utils_test.go
+++ b/pkg/provider/azure_utils_test.go
@@ -17,11 +17,8 @@ limitations under the License.
 package provider
 
 import (
-	"reflect"
 	"testing"
 	"time"
-
-	"github.com/stretchr/testify/assert"
 )
 
 func TestSimpleLockEntry(t *testing.T) {
@@ -84,69 +81,3 @@ func ensureNoCallback(t *testing.T, callbackChan <-chan interface{}) bool {
 		return true
 	}
 }
-
-func TestConvertTagsToMap(t *testing.T) {
-	testCases := []struct {
-		desc           string
-		tags           string
-		expectedOutput map[string]string
-		expectedError  bool
-	}{
-		{
-			desc:           "should return empty map when tag is empty",
-			tags:           "",
-			expectedOutput: map[string]string{},
-			expectedError:  false,
-		},
-		{
-			desc: "sing valid tag should be converted",
-			tags: "key=value",
-			expectedOutput: map[string]string{
-				"key": "value",
-			},
-			expectedError: false,
-		},
-		{
-			desc: "multiple valid tags should be converted",
-			tags: "key1=value1,key2=value2",
-			expectedOutput: map[string]string{
-				"key1": "value1",
-				"key2": "value2",
-			},
-			expectedError: false,
-		},
-		{
-			desc: "whitespaces should be trimmed",
-			tags: "key1=value1, key2=value2",
-			expectedOutput: map[string]string{
-				"key1": "value1",
-				"key2": "value2",
-			},
-			expectedError: false,
-		},
-		{
-			desc:           "should return error for invalid format",
-			tags:           "foo,bar",
-			expectedOutput: nil,
-			expectedError:  true,
-		},
-		{
-			desc:           "should return error for when key is missed",
-			tags:           "key1=value1,=bar",
-			expectedOutput: nil,
-			expectedError:  true,
-		},
-	}
-
-	for i, c := range testCases {
-		m, err := ConvertTagsToMap(c.tags)
-		if c.expectedError {
-			assert.NotNil(t, err, "TestCase[%d]: %s", i, c.desc)
-		} else {
-			assert.Nil(t, err, "TestCase[%d]: %s", i, c.desc)
-			if !reflect.DeepEqual(m, c.expectedOutput) {
-				t.Errorf("got: %v, expected: %v, desc: %v", m, c.expectedOutput, c.desc)
-			}
-		}
-	}
-}
diff --git a/tests/e2e/network/standard_lb.go b/tests/e2e/network/standard_lb.go
index f3ec7cda7..205cc3924 100644
--- a/tests/e2e/network/standard_lb.go
+++ b/tests/e2e/network/standard_lb.go
@@ -97,17 +97,14 @@ var _ = Describe("[StandardLoadBalancer] Standard load balancer", func() {
 
 		lbBackendAddressPoolsIDMap := make(map[string]bool)
 		for _, backendAddressPool := range *lb.BackendAddressPools {
-			backendAddressPoolID := strings.ToLower(to.String(backendAddressPool.ID))
-			lbBackendAddressPoolsIDMap[backendAddressPoolID] = true
-			utils.Logf("found backend pool %s", backendAddressPoolID)
+			utils.Logf("found backend pool %s", to.String(backendAddressPool.ID))
+			lbBackendAddressPoolsIDMap[*backendAddressPool.ID] = true
 		}
-		utils.Logf("got lbBackendAddressPoolsIDMap: %v", lbBackendAddressPoolsIDMap)
 
 		NICList, err := utils.ListNICs(tc, rgName)
 		Expect(err).NotTo(HaveOccurred())
 		var found bool
 		for _, nic := range *NICList {
-			found = false
 			if strings.Split(*nic.Name, "-")[1] == "master" {
 				continue
 			}
@@ -117,9 +114,8 @@ var _ = Describe("[StandardLoadBalancer] Standard load balancer", func() {
 				}
 				utils.Logf("found ip config %s", to.String(ipConfig.Name))
 				for _, backendAddressPool := range *ipConfig.LoadBalancerBackendAddressPools {
-					backendAddressPoolID := strings.ToLower(to.String(backendAddressPool.ID))
-					utils.Logf("found backend pool on nic %s", backendAddressPoolID)
-					if lbBackendAddressPoolsIDMap[backendAddressPoolID] {
+					utils.Logf("found backend pool on nic %s", to.String(backendAddressPool.ID))
+					if lbBackendAddressPoolsIDMap[*backendAddressPool.ID] {
 						found = true
 						break
 					}