Compare tags using common tag type (#399)

Implements aws-controllers-k8s/community#1658

Description of changes:
Instead of comparing tag fields using the standard `reflect.DeepEquals`, this PR updates the code-generator to convert the tags to the common `acktags.Tag` type and then compare those as maps. This ensures that changes in ordering do not result in false positive deltas.

Controllers that rely on custom tag delta functions also typically use a function to determine which tags are being added and which are being removed. Using the same trick as this PR, a method can be added to the common runtime to deduce those - resulting in far less duplicated and verbose custom hook code. Those changes are not included in this PR, though.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Author: RedbackThomson

pkg/generate/code/compare.go

@@ -99,6 +99,11 @@ func CompareResource(
 
 	resConfig := cfg.GetResourceConfig(r.Names.Camel)
 
+	tagField, err := r.GetTagField()
+	if err != nil {
+		panic(err)
+	}
+
 	// We need a deterministic order to traverse our top-level fields...
 	specFieldNames := []string{}
 	for fieldName := range r.SpecFields {
@@ -147,10 +152,18 @@ func CompareResource(
 			out += fmt.Sprintf("%s}\n", indent)
 			continue
 		}
+
+		// Use a special comparison model for tags, since they need to be
+		// converted into the common ACK tag type before doing a map delta
+		if tagField != nil && specField == tagField {
+			out += compareTags(deltaVarName, firstResAdaptedVarName, secondResAdaptedVarName, fieldPath, indentLevel)
+			continue
+		}
+
 		memberShapeRef := specField.ShapeRef
 		memberShape := memberShapeRef.Shape
 
-		// if ackcompare.HasNilDifference(a.ko.Spec.Name, b.ko.Spec.Name == nil) {
+		// if ackcompare.HasNilDifference(a.ko.Spec.Name, b.ko.Spec.Name) {
 		//   delta.Add("Spec.Name", a.ko.Spec.Name, b.ko.Spec.Name)
 		// }
 		nilCode := compareNil(
@@ -520,6 +533,46 @@ func compareSlice(
 	return out
 }
 
+// compareTags outputs Go code that compares two slices of tags from two
+// resource fields by first converting them to the common ACK tag type and then
+// using a map comparison. If there is a difference, adds the difference to a
+// variable representing an `ackcompare.Delta`.
+//
+// Output code will look something like this:
+//
+//	if !ackcompare.MapStringStringEqual(ToACKTags(a.ko.Spec.Tags), ToACKTags(b.ko.Spec.Tags)) {
+//	  delta.Add("Spec.Tags", a.ko.Spec.Tags, b.ko.Spec.Tags)
+//	}
+func compareTags(
+	// String representing the name of the variable that is of type
+	// `*ackcompare.Delta`. We will generate Go code that calls the `Add()`
+	// method of this variable when differences between fields are detected.
+	deltaVarName string,
+	// String representing the name of the variable that represents the first
+	// CR under comparison. This will typically be something like
+	// "a.ko.Spec.Name". See `templates/pkg/resource/delta.go.tpl`.
+	firstResVarName string,
+	// String representing the name of the variable that represents the second
+	// CR under comparison. This will typically be something like
+	// "b.ko.Spec.Name". See `templates/pkg/resource/delta.go.tpl`.
+	secondResVarName string,
+	// String indicating the current field path being evaluated, e.g.
+	// "Author.Name". This does not include the top-level Spec or Status
+	// struct.
+	fieldPath string,
+	// Number of levels of indentation to use
+	indentLevel int,
+) string {
+	out := ""
+	indent := strings.Repeat("\t", indentLevel)
+
+	out += fmt.Sprintf("%sif !ackcompare.MapStringStringEqual(ToACKTags(%s), ToACKTags(%s)) {\n", indent, firstResVarName, secondResVarName)
+	out += fmt.Sprintf("%s\t%s.Add(\"%s\", %s, %s)\n", indent, deltaVarName, fieldPath, firstResVarName, secondResVarName)
+	out += fmt.Sprintf("%s}\n", indent)
+
+	return out
+}
+
 // CompareStruct outputs Go code that compares two struct values from two
 // resource fields and, if there is a difference, adds the difference to a
 // variable representing an `ackcompare.Delta`.
@@ -551,6 +604,11 @@ func CompareStruct(
 ) string {
 	out := ""
 
+	tagField, err := r.GetTagField()
+	if err != nil {
+		panic(err)
+	}
+
 	fieldConfigs := cfg.GetFieldConfigs(r.Names.Original)
 
 	for _, memberName := range shape.MemberNames() {
@@ -572,7 +630,9 @@ func CompareStruct(
 		// memberFieldPath contains the field path along with the prefix cfg.PrefixConfig.SpecField + "." hence we
 		// would need to substring to exclude cfg.PrefixConfig.SpecField + "." to get correct field config.
 		specFieldLen := len(strings.TrimPrefix(cfg.PrefixConfig.SpecField, "."))
-		fieldConfig := fieldConfigs[memberFieldPath[specFieldLen+1:len(memberFieldPath)]]
+		trimmedFieldPath := memberFieldPath[specFieldLen+1:]
+
+		fieldConfig := fieldConfigs[trimmedFieldPath]
 		if fieldConfig != nil {
 			compareConfig = fieldConfig.Compare
 		}
@@ -583,6 +643,13 @@ func CompareStruct(
 
 		memberShape := memberShapeRef.Shape
 
+		// Use a special comparison model for tags, since they need to be
+		// converted into the common ACK tag type before doing a map delta
+		if tagField != nil && tagField.Path == trimmedFieldPath {
+			out += compareTags(deltaVarName, firstResAdaptedVarName, secondResAdaptedVarName, fieldPath, indentLevel)
+			continue
+		}
+
 		// if ackcompare.HasNilDifference(a.ko.Spec.Name, b.ko.Spec.Name == nil) {
 		//   delta.Add("Spec.Name", a.ko.Spec.Name, b.ko.Spec.Name)
 		// }
@@ -604,6 +671,7 @@ func CompareStruct(
 			)
 			indentLevel++
 		}
+
 		switch memberShape.Type {
 		case "structure":
 			// Recurse through all the struct's fields and subfields, building

pkg/generate/code/compare_test.go

@@ -120,6 +120,13 @@ func TestCompareResource_S3_Bucket(t *testing.T) {
 			delta.Add("Spec.ObjectLockEnabledForBucket", a.ko.Spec.ObjectLockEnabledForBucket, b.ko.Spec.ObjectLockEnabledForBucket)
 		}
 	}
+	if ackcompare.HasNilDifference(a.ko.Spec.Tagging, b.ko.Spec.Tagging) {
+		delta.Add("Spec.Tagging", a.ko.Spec.Tagging, b.ko.Spec.Tagging)
+	} else if a.ko.Spec.Tagging != nil && b.ko.Spec.Tagging != nil {
+		if !ackcompare.MapStringStringEqual(ToACKTags(a.ko.Spec.Tagging.TagSet), ToACKTags(b.ko.Spec.Tagging.TagSet)) {
+			delta.Add("Spec.Tagging", a.ko.Spec.Tagging.TagSet, b.ko.Spec.Tagging.TagSet)
+		}
+	}
 `
 	assert.Equal(
 		expected,
@@ -333,12 +340,8 @@ func TestCompareResource_Lambda_Function(t *testing.T) {
 			delta.Add("Spec.Runtime", a.ko.Spec.Runtime, b.ko.Spec.Runtime)
 		}
 	}
-	if ackcompare.HasNilDifference(a.ko.Spec.Tags, b.ko.Spec.Tags) {
+	if !ackcompare.MapStringStringEqual(ToACKTags(a.ko.Spec.Tags), ToACKTags(b.ko.Spec.Tags)) {
 		delta.Add("Spec.Tags", a.ko.Spec.Tags, b.ko.Spec.Tags)
-	} else if a.ko.Spec.Tags != nil && b.ko.Spec.Tags != nil {
-		if !ackcompare.MapStringStringPEqual(a.ko.Spec.Tags, b.ko.Spec.Tags) {
-			delta.Add("Spec.Tags", a.ko.Spec.Tags, b.ko.Spec.Tags)
-		}
 	}
 	if ackcompare.HasNilDifference(a.ko.Spec.Timeout, b.ko.Spec.Timeout) {
 		delta.Add("Spec.Timeout", a.ko.Spec.Timeout, b.ko.Spec.Timeout)
@@ -490,7 +493,7 @@ func TestCompareResource_IAM_OIDC_URL(t *testing.T) {
 	if !ackcompare.SliceStringPEqual(a.ko.Spec.ClientIDList, b.ko.Spec.ClientIDList) {
 		delta.Add("Spec.ClientIDList", a.ko.Spec.ClientIDList, b.ko.Spec.ClientIDList)
 	}
-	if !reflect.DeepEqual(a.ko.Spec.Tags, b.ko.Spec.Tags) {
+	if !ackcompare.MapStringStringEqual(ToACKTags(a.ko.Spec.Tags), ToACKTags(b.ko.Spec.Tags)) {
 		delta.Add("Spec.Tags", a.ko.Spec.Tags, b.ko.Spec.Tags)
 	}
 	if !ackcompare.SliceStringPEqual(a.ko.Spec.ThumbprintList, b.ko.Spec.ThumbprintList) {
@@ -539,7 +542,7 @@ func TestCompareResource_MemoryDB_User(t *testing.T) {
 			delta.Add("Spec.Name", a.ko.Spec.Name, b.ko.Spec.Name)
 		}
 	}
-	if !reflect.DeepEqual(a.ko.Spec.Tags, b.ko.Spec.Tags) {
+	if !ackcompare.MapStringStringEqual(ToACKTags(a.ko.Spec.Tags), ToACKTags(b.ko.Spec.Tags)) {
 		delta.Add("Spec.Tags", a.ko.Spec.Tags, b.ko.Spec.Tags)
 	}
 `

pkg/model/model_s3_test.go

@@ -82,6 +82,7 @@ func TestS3_Bucket(t *testing.T) {
 		// ListBuckets API call...
 		"Name",
 		"ObjectLockEnabledForBucket",
+		"Tagging",
 	}
 	assert.Equal(expSpecFieldCamel, attrCamelNames(specFields))
 

pkg/testdata/models/apis/lambda/0000-00-00/generator.yaml

@@ -25,3 +25,6 @@ resources:
           in:
             - 1
             - 2
+  CodeSigningConfig:
+    tags:
+      ignore: true

pkg/testdata/models/apis/s3/0000-00-00/generator.yaml

@@ -18,6 +18,8 @@ resources:
     list_operation:
       match_fields:
         - Name
+    tags:
+      path: Tagging.TagSet
     fields:
       ACL:
         # This is to test the ackcompare field ignore functionality. This
@@ -27,4 +29,8 @@ resources:
       Logging:
         from:
           operation: PutBucketLogging
-          path: BucketLoggingStatus
+          path: BucketLoggingStatus
+      Tagging:
+        from:
+          operation: PutBucketTagging
+          path: Tagging

templates/pkg/resource/delta.go.tpl

@@ -7,12 +7,14 @@ import (
 	"reflect"
 
 	ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare"
+	acktags "github.com/aws-controllers-k8s/runtime/pkg/tags"
 )
 
 // Hack to avoid import errors during build...
 var (
 	_ = &bytes.Buffer{}
 	_ = &reflect.Method{}
+	_ = &acktags.Tags{}
 )
 
 // newResourceDelta returns a new `ackcompare.Delta` used to compare two