Repository: kubernetes-sigs/cloud-provider-azure
Pull request #895 (2021-11-09 13:30:51)

feat: support json style tags
<!--  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 feature

#### What this PR does / why we need it:

Cherry pick from #892: feat: support json sytle tags

#### 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
feat: support json style tags
```

#### 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.go b/pkg/provider/azure.go
index 6bb8da415..d598c94fc 100644
--- a/pkg/provider/azure.go
+++ b/pkg/provider/azure.go
@@ -140,7 +140,11 @@ type Config struct {
 	// Tags determines what tags shall be applied to the shared resources managed by controller manager, which
 	// 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.
+	// Because special characters are not supported in "tags" configuration, "tags" support would be removed in a future release,
+	// please consider migrating the config to "tagsMap".
 	Tags string `json:"tags,omitempty" yaml:"tags,omitempty"`
+	// TagsMap is similar to Tags but holds tags with special characters such as `=` and `,`.
+	TagsMap map[string]string `json:"tagsMap,omitempty" yaml:"tagsMap,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`.
diff --git a/pkg/provider/azure_loadbalancer.go b/pkg/provider/azure_loadbalancer.go
index a39d4c039..0ce56fda5 100644
--- a/pkg/provider/azure_loadbalancer.go
+++ b/pkg/provider/azure_loadbalancer.go
@@ -2626,10 +2626,10 @@ func shouldReleaseExistingOwnedPublicIP(existingPip *network.PublicIPAddress, lb
 
 // ensurePIPTagged ensures the public IP of the service is tagged as configured
 func (az *Cloud) ensurePIPTagged(service *v1.Service, pip *network.PublicIPAddress) bool {
-	configTags := parseTags(az.Tags)
+	configTags := parseTags(az.Tags, az.TagsMap)
 	annotationTags := make(map[string]*string)
 	if _, ok := service.Annotations[consts.ServiceAnnotationAzurePIPTags]; ok {
-		annotationTags = parseTags(service.Annotations[consts.ServiceAnnotationAzurePIPTags])
+		annotationTags = parseTags(service.Annotations[consts.ServiceAnnotationAzurePIPTags], map[string]string{})
 	}
 
 	for k, v := range annotationTags {
@@ -3224,7 +3224,7 @@ func (az *Cloud) ensureLoadBalancerTagged(lb *network.LoadBalancer) bool {
 	if az.Tags == "" {
 		return false
 	}
-	tags := parseTags(az.Tags)
+	tags := parseTags(az.Tags, az.TagsMap)
 	if lb.Tags == nil {
 		lb.Tags = make(map[string]*string)
 	}
@@ -3240,7 +3240,7 @@ func (az *Cloud) ensureSecurityGroupTagged(sg *network.SecurityGroup) bool {
 	if az.Tags == "" {
 		return false
 	}
-	tags := parseTags(az.Tags)
+	tags := parseTags(az.Tags, az.TagsMap)
 	if sg.Tags == nil {
 		sg.Tags = make(map[string]*string)
 	}
diff --git a/pkg/provider/azure_routes.go b/pkg/provider/azure_routes.go
index c8ed1fabf..4cf22d282 100644
--- a/pkg/provider/azure_routes.go
+++ b/pkg/provider/azure_routes.go
@@ -542,7 +542,7 @@ func (az *Cloud) ensureRouteTableTagged(rt *network.RouteTable) (map[string]*str
 	if az.Tags == "" {
 		return nil, false
 	}
-	tags := parseTags(az.Tags)
+	tags := parseTags(az.Tags, az.TagsMap)
 	if rt.Tags == nil {
 		rt.Tags = make(map[string]*string)
 	}
diff --git a/pkg/provider/azure_utils.go b/pkg/provider/azure_utils.go
index d8a748635..ebb002d29 100644
--- a/pkg/provider/azure_utils.go
+++ b/pkg/provider/azure_utils.go
@@ -94,22 +94,42 @@ func convertMapToMapPointer(origin map[string]string) map[string]*string {
 	return newly
 }
 
-func parseTags(tags string) map[string]*string {
-	kvs := strings.Split(tags, consts.TagsDelimiter)
+func parseTags(tags string, tagsMap map[string]string) map[string]*string {
 	formatted := make(map[string]*string)
-	for _, kv := range kvs {
-		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
+
+	if tags != "" {
+		kvs := strings.Split(tags, consts.TagsDelimiter)
+		for _, kv := range kvs {
+			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
+			}
+			k, v := strings.TrimSpace(res[0]), strings.TrimSpace(res[1])
+			if k == "" {
+				klog.Warning("parseTags: empty key, ignoring this key-value pair")
+				continue
+			}
+			formatted[k] = to.StringPtr(v)
 		}
-		k, v := strings.TrimSpace(res[0]), strings.TrimSpace(res[1])
-		if k == "" || v == "" {
-			klog.Warningf("parseTags: error when parsing key-value pair %s-%s, would ignore this one", k, v)
-			continue
+	}
+
+	if len(tagsMap) > 0 {
+		for key, value := range tagsMap {
+			key, value := strings.TrimSpace(key), strings.TrimSpace(value)
+			if key == "" {
+				klog.Warningf("parseTags: empty key, ignoring this key-value pair")
+				continue
+			}
+
+			if found, k := findKeyInMapCaseInsensitive(formatted, key); found && k != key {
+				klog.V(4).Infof("parseTags: found identical keys: %s from tags and %s from tagsMap (case-insensitive), %s will replace %s", k, key, key, k)
+				delete(formatted, k)
+			}
+			formatted[key] = to.StringPtr(value)
 		}
-		formatted[k] = to.StringPtr(v)
 	}
+
 	return formatted
 }
 
diff --git a/site/content/en/install/configs.md b/site/content/en/install/configs.md
index 9fb138e5e..f351629c4 100644
--- a/site/content/en/install/configs.md
+++ b/site/content/en/install/configs.md
@@ -105,6 +105,7 @@ If more than one value is set, the order is `Managed Identity` > `Service Princi
 |routeTableCacheTTLInSeconds|Cache TTL in seconds for route table|Since v1.18.0, default is 120|
 | disableAzureStackCloud | DisableAzureStackCloud disables AzureStackCloud support. It should be used when setting Cloud with "AZURESTACKCLOUD" to customize ARM endpoints while the cluster is not running on AzureStack. Default is false. |Optional. Supported since v1.20.0 in out-of-tree cloud provider Azure. |
 | tags | tags that would be tagged onto the cloud provider managed resources, including lb, public IP, network security group and route table. | Optional. Supported since v1.20.0. |
+| tagsMap | JSON-style tags, will be merged with `tags` | Optional. Supported since v1.23.0. |
 | systemTags | tag keys that should not be deleted when being updated. | Optional. Supported since v1.21.0. |
 
 ### primaryAvailabilitySetName
diff --git a/site/content/en/topics/tagging-resources.md b/site/content/en/topics/tagging-resources.md
index c6bf0373b..118fc1cac 100644
--- a/site/content/en/topics/tagging-resources.md
+++ b/site/content/en/topics/tagging-resources.md
@@ -13,13 +13,13 @@ Specifically, the shared resources (load balancer, route table and security grou
 
 ```json
 {
-  "tags": "a=b,c=d",
+  "tags": "a=b,c=d"
 }
 ```
 
 the controller manager would parse this configuration and tag the shared resources once restarted.
 
-The non-shared resource (public IP) could be tagged by setting `tags` in `azure.json` or service annotation `service.beta.kubernetes.io/azure-pip-tags`. The format of the two is similiar and the tags in the annotation would be considered first when there are conflicts between the configuration file and the annotation.
+The non-shared resource (public IP) could be tagged by setting `tags` in `azure.json` or service annotation `service.beta.kubernetes.io/azure-pip-tags`. The format of the two is similar and the tags in the annotation would be considered first when there are conflicts between the configuration file and the annotation.
 
 When the configuration, file or annotation, is updated, the old ones would be updated if there are conflicts. For example, after updating `{"tags": "a=b,c=d"}` to `{"tags": "a=c,e=f"}`, the new tags would be `a=c,c=d,e=f`.
 
@@ -36,3 +36,20 @@ Normally the controller manager don't delete the existing tags even if they are
 | "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 */ |
+
+> Please consider migrating existing "tags" to "tagsMap", the support of "tags" configuration would be removed in a future release.
+
+## Including special characters in tags
+
+> This feature is supported since v1.23.0.
+
+Normally we don't support special characters such as `=` or `,` in key-value pairs. These characters will be treated as separator and will not be included in the key/value literal. To solve this problem, `tagsMap` is introduced since v1.23.0, in which a JSON-style tag is acceptable.
+
+```json
+{
+  "tags": "a=b,c=d",
+  "tagsMap": {"e": "f", "g=h": "i,j"}
+}
+```
+
+`tags` and `tagsMap` will be merged, and similarly, they are case-insensitive.


Test patch:
diff --git a/pkg/provider/azure_loadbalancer_test.go b/pkg/provider/azure_loadbalancer_test.go
index daee898b2..5919cffa3 100644
--- a/pkg/provider/azure_loadbalancer_test.go
+++ b/pkg/provider/azure_loadbalancer_test.go
@@ -4133,6 +4133,7 @@ func TestEnsurePIPTagged(t *testing.T) {
 				"c":                   to.StringPtr("d"),
 				"y":                   to.StringPtr("z"),
 				"m":                   to.StringPtr("n"),
+				"e":                   to.StringPtr(""),
 			},
 		}
 		changed := cloud.ensurePIPTagged(&service, &pip)
@@ -4150,6 +4151,26 @@ func TestEnsurePIPTagged(t *testing.T) {
 				"a":                   to.StringPtr("b"),
 				"c":                   to.StringPtr("d"),
 				"y":                   to.StringPtr("z"),
+				"e":                   to.StringPtr(""),
+			},
+		}
+		changed := cloud.ensurePIPTagged(&service, &pip)
+		assert.True(t, changed)
+		assert.Equal(t, expectedPIP, pip)
+	})
+
+	t.Run("ensurePIPTagged should support TagsMap", func(t *testing.T) {
+		cloud.SystemTags = "a,foo"
+		cloud.TagsMap = map[string]string{"a": "c", "a=b": "c=d", "Y": "zz"}
+		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("b"),
+				"c":                   to.StringPtr("d"),
+				"a=b":                 to.StringPtr("c=d"),
+				"e":                   to.StringPtr(""),
 			},
 		}
 		changed := cloud.ensurePIPTagged(&service, &pip)
diff --git a/tests/e2e/network/service_annotations.go b/tests/e2e/network/service_annotations.go
index 6089ba627..6be4818ea 100644
--- a/tests/e2e/network/service_annotations.go
+++ b/tests/e2e/network/service_annotations.go
@@ -346,6 +346,7 @@ var _ = Describe("Service with annotation", func() {
 		expectedTags := map[string]*string{
 			"a": to.StringPtr("b"),
 			"c": to.StringPtr("d"),
+			"e": to.StringPtr(""),
 		}
 		pips, err := tc.ListPublicIPs(tc.GetResourceGroup())
 		Expect(err).NotTo(HaveOccurred())
@@ -370,6 +371,7 @@ var _ = Describe("Service with annotation", func() {
 		expectedTags = map[string]*string{
 			"a": to.StringPtr("c"),
 			"c": to.StringPtr("d"),
+			"e": to.StringPtr(""),
 			"x": to.StringPtr("y"),
 		}
 		err = waitComparePIPTags(tc, expectedTags, to.String(targetPIP.Name))