Merge pull request #669 from nilo19/fix/tag-case

fix: make tags case-insensitive for both keys and values
kubernetes-sigs · Jun 21, 2021 · cc2d89d · cc2d89d
2 parents cd92520 + 5e9c1c4
commit cc2d89d
Show file tree

Hide file tree

Showing 2 changed files with 124 additions and 6 deletions.
diff --git a/pkg/provider/azure_utils.go b/pkg/provider/azure_utils.go
@@ -115,32 +115,52 @@ 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 (az *Cloud) reconcileTags(currentTagsOnResource, newTags map[string]*string) (reconciledTags map[string]*string, changed bool) {
 	var systemTags []string
-	var systemTagsMap sets.String
+	systemTagsMap := make(map[string]*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...)
+
+		for _, systemTag := range systemTags {
+			systemTagsMap[systemTag] = to.StringPtr("")
+		}
 	}
 
 	// 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)) {
+		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
 		}
 	}
 
 	// 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
+			if _, ok := newTags[k]; !ok {
+				if found, _ := findKeyInMapCaseInsensitive(systemTagsMap, k); !found {
+					delete(currentTagsOnResource, k)
+					changed = true
+				}
 			}
 		}
 	}

diff --git a/pkg/provider/azure_utils_test.go b/pkg/provider/azure_utils_test.go
@@ -19,6 +19,9 @@ package provider
 import (
 	"testing"
 	"time"
+
+	"github.com/Azure/go-autorest/autorest/to"
+	"github.com/stretchr/testify/assert"
 )
 
 func TestSimpleLockEntry(t *testing.T) {
@@ -81,3 +84,98 @@ func ensureNoCallback(t *testing.T, callbackChan <-chan interface{}) bool {
 		return true
 	}
 }
+
+func TestReconcileTags(t *testing.T) {
+	for _, testCase := range []struct {
+		description, systemTags                      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 remove the tags that are not included in systemTags",
+			currentTagsOnResource: map[string]*string{
+				"a": to.StringPtr("b"),
+				"c": to.StringPtr("d"),
+			},
+			newTags: map[string]*string{
+				"a": to.StringPtr("c"),
+			},
+			systemTags: "a, b",
+			expectedTags: map[string]*string{
+				"a": to.StringPtr("c"),
+			},
+			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"),
+			},
+		},
+		{
+			description: "reconcileTags should ignore the case of keys when checking systemTags",
+			currentTagsOnResource: map[string]*string{
+				"a": to.StringPtr("b"),
+				"c": to.StringPtr("d"),
+			},
+			newTags: map[string]*string{
+				"a": to.StringPtr("c"),
+			},
+			systemTags: "A, b",
+			expectedTags: map[string]*string{
+				"a": to.StringPtr("c"),
+			},
+			expectedChanged: true,
+		},
+	} {
+		t.Run(testCase.description, func(t *testing.T) {
+			cloud := &Cloud{}
+			if testCase.systemTags != "" {
+				cloud.SystemTags = testCase.systemTags
+			}
+
+			tags, changed := cloud.reconcileTags(testCase.currentTagsOnResource, testCase.newTags)
+			assert.Equal(t, testCase.expectedChanged, changed)
+			assert.Equal(t, testCase.expectedTags, tags)
+		})
+	}
+}