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

Issue:
Support special characters in tags
<!-- Feature requests are unlikely to make progress as an issue.

Instead, please suggest enhancements by engaging with SIGs on slack and mailing lists.

A proposal that works through the design along with the implications of the change can be opened as a KEP:
https://git.k8s.io/enhancements/keps#kubernetes-enhancement-proposals-keps
-->

#### What would you like to be added:

Refer https://kubernetes-sigs.github.io/cloud-provider-azure/topics/tagging-resources/, because of the configuration format  `"tags": "a=b,c=d",`, special characters `=` and `,` couldn't be used directly the tags configuration.

To support random characters in the tags, key-value pairs should be allowed in the configuration `tags` and `systemTags`.

#### Why is this needed:

Azure tags support special characters in both key and value, hence cloud provider should provide consistent behavior with that.

Pull request:
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:

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, 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.

#### 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 #887

#### 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 8377103d0..4a7be2818 100644
--- a/pkg/provider/azure.go
+++ b/pkg/provider/azure.go
@@ -145,7 +145,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 1d49c3895..e75f14d49 100644
--- a/pkg/provider/azure_loadbalancer.go
+++ b/pkg/provider/azure_loadbalancer.go
@@ -2712,10 +2712,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 {
@@ -3306,7 +3306,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)
 	}
@@ -3322,7 +3322,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 4bc9823e5..f0dd91c6f 100644
--- a/pkg/provider/azure_routes.go
+++ b/pkg/provider/azure_routes.go
@@ -566,7 +566,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 ebc025bb1..1b5dd1e80 100644
--- a/pkg/provider/azure_utils.go
+++ b/pkg/provider/azure_utils.go
@@ -99,26 +99,42 @@ func convertMapToMapPointer(origin map[string]string) map[string]*string {
 	return newly
 }
 
-func parseTags(tags string) map[string]*string {
+func parseTags(tags string, tagsMap map[string]string) map[string]*string {
 	formatted := make(map[string]*string)
-	if len(tags) == 0 {
-		return formatted
-	}
 
-	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
+	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 7edac5ce8..e1ebfdec9 100644
--- a/site/content/en/install/configs.md
+++ b/site/content/en/install/configs.md
@@ -107,6 +107,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 b21a90ed0..1b3b6a43d 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.
 
 > The annotation `service.beta.kubernetes.io/azure-pip-tags` only works for managed public IPs. For BYO public IPs, the cloud provider would not apply any tags to them.
 
@@ -38,3 +38,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 2834fc3ac..1752c68e0 100644
--- a/pkg/provider/azure_loadbalancer_test.go
+++ b/pkg/provider/azure_loadbalancer_test.go
@@ -4225,6 +4225,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)
@@ -4242,6 +4243,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 c900b1e24..fbbf56635 100644
--- a/tests/e2e/network/service_annotations.go
+++ b/tests/e2e/network/service_annotations.go
@@ -339,6 +339,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())
@@ -363,6 +364,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))
diff --git a/tests/kubemark/templates/hollow-node.yaml b/tests/kubemark/templates/hollow-node.yaml
index 2166cc10c..acd564a24 100644
--- a/tests/kubemark/templates/hollow-node.yaml
+++ b/tests/kubemark/templates/hollow-node.yaml
@@ -44,13 +44,13 @@ spec:
                   fieldPath: metadata.name
           command:
             [
-              "/go-runner",
-              "-log-file=/var/log/kubelet-$(NODE_NAME).log",
               "/kubemark",
               "--morph=kubelet",
               "--name=$(NODE_NAME)",
               "--kubeconfig=/kubeconfig/kubelet.kubeconfig",
               "$(CONTENT_TYPE)",
+              "--log-file=/var/log/kubelet-$(NODE_NAME).log",
+              "--logtostderr=false",
               "--v=2",
             ]
           volumeMounts:
@@ -82,14 +82,14 @@ spec:
                   fieldPath: metadata.name
           command:
             [
-              "/go-runner",
-              "-log-file=/var/log/kubeproxy-$(NODE_NAME).log",
               "/kubemark",
               "--morph=proxy",
               "--name=$(NODE_NAME)",
               "--use-real-proxier=false",
               "--kubeconfig=/kubeconfig/kubeproxy.kubeconfig",
               "$(CONTENT_TYPE)",
+              "--log-file=/var/log/kubeproxy-$(NODE_NAME).log",
+              "--logtostderr=false",
               "--v=10",
             ]
           volumeMounts: