From 86278ade513fd2e73cbec2a8204b1c45f4e04e84 Mon Sep 17 00:00:00 2001 From: Qi Ni Date: Wed, 25 Aug 2021 20:20:59 +0800 Subject: [PATCH] fix: ignore the case when updating tags --- .../azure/azure_loadbalancer.go | 33 ++++------ .../azure/azure_loadbalancer_test.go | 2 +- .../azure/azure_routes.go | 11 ++-- .../azure/azure_utils.go | 25 ++++++++ .../azure/azure_utils_test.go | 61 +++++++++++++++++++ 5 files changed, 103 insertions(+), 29 deletions(-) diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go index 3f3556639bb..4660c95646e 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go @@ -2131,7 +2131,6 @@ 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 { - changed := false configTags := parseTags(az.Tags) annotationTags := make(map[string]*string) if _, ok := service.Annotations[ServiceAnnotationAzurePIPTags]; ok { @@ -2154,12 +2153,10 @@ func (az *Cloud) ensurePIPTagged(service *v1.Service, pip *network.PublicIPAddre if serviceNames != nil { configTags[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 := reconcileTags(pip.Tags, configTags) + pip.Tags = tags + return changed } @@ -2720,7 +2717,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 } @@ -2728,18 +2724,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 := 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 } @@ -2747,11 +2740,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 := reconcileTags(sg.Tags, tags) + sg.Tags = tags + return changed } diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go index 54b984b15ea..4d21a8c0b0c 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go @@ -3784,7 +3784,7 @@ func TestEnsurePIPTagged(t *testing.T) { service := v1.Service{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - ServiceAnnotationAzurePIPTags: "a=b,c=d,e=,=f,ghi", + ServiceAnnotationAzurePIPTags: "A=b,c=d,e=,=f,ghi", }, }, } diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_routes.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_routes.go index 25bccad80ae..bb61f9c519d 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_routes.go +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_routes.go @@ -541,16 +541,13 @@ func (az *Cloud) ensureRouteTableTagged(rt *network.RouteTable) (map[string]*str 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 := reconcileTags(rt.Tags, tags) + rt.Tags = tags + return rt.Tags, changed } diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_utils.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_utils.go index c5d1b203cf4..4c8baac728a 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_utils.go +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_utils.go @@ -138,3 +138,28 @@ func parseTags(tags string) map[string]*string { } return formatted } + +func findKeyInMapCaseInsensitive(targetMap map[string]*string, key string) (bool, string) { + for k := range targetMap { + if strings.EqualFold(k, key) { + return true, k + } + } + + return false, "" +} + +func reconcileTags(currentTagsOnResource, newTags map[string]*string) (reconciledTags map[string]*string, changed bool) { + for k, v := range newTags { + found, key := findKeyInMapCaseInsensitive(currentTagsOnResource, k) + if !found { + currentTagsOnResource[k] = v + changed = true + } else if !strings.EqualFold(to.String(v), to.String(currentTagsOnResource[key])) { + currentTagsOnResource[key] = v + changed = true + } + } + + return currentTagsOnResource, changed +} diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_utils_test.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_utils_test.go index cf148ec4dbb..0a0f4087670 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_utils_test.go +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_utils_test.go @@ -23,6 +23,7 @@ import ( "testing" "time" + "github.com/Azure/go-autorest/autorest/to" "github.com/stretchr/testify/assert" ) @@ -152,3 +153,63 @@ func TestConvertTagsToMap(t *testing.T) { } } } + +func TestReconcileTags(t *testing.T) { + for _, testCase := range []struct { + description string + currentTagsOnResource, newTags, expectedTags map[string]*string + expectedChanged bool + }{ + { + description: "reconcileTags should add missing tags and update existing tags", + currentTagsOnResource: map[string]*string{ + "a": to.StringPtr("b"), + }, + newTags: map[string]*string{ + "a": to.StringPtr("c"), + "b": to.StringPtr("d"), + }, + expectedTags: map[string]*string{ + "a": to.StringPtr("c"), + "b": to.StringPtr("d"), + }, + expectedChanged: true, + }, + { + description: "reconcileTags should ignore the case of keys when comparing", + currentTagsOnResource: map[string]*string{ + "A": to.StringPtr("b"), + "c": to.StringPtr("d"), + }, + newTags: map[string]*string{ + "a": to.StringPtr("b"), + "C": to.StringPtr("d"), + }, + expectedTags: map[string]*string{ + "A": to.StringPtr("b"), + "c": to.StringPtr("d"), + }, + }, + { + description: "reconcileTags should ignore the case of values when comparing", + currentTagsOnResource: map[string]*string{ + "A": to.StringPtr("b"), + "c": to.StringPtr("d"), + }, + newTags: map[string]*string{ + "a": to.StringPtr("B"), + "C": to.StringPtr("D"), + }, + expectedTags: map[string]*string{ + "A": to.StringPtr("b"), + "c": to.StringPtr("d"), + }, + }, + } { + t.Run(testCase.description, func(t *testing.T) { + tags, changed := reconcileTags(testCase.currentTagsOnResource, testCase.newTags) + assert.Equal(t, testCase.expectedChanged, changed) + assert.Equal(t, testCase.expectedTags, tags) + }) + } +}