Modify tags related functions and some test files to fix flaky assertion errors (#51)

Issue #, if available:
MemoryDB Controller has some flaky tests that randomly show assertion error.
Root causes:
1. Snapshot test files didn't clean resources which were created from previous failed tests, so eventually test account reached node limit.
2. Delta compare considers order matters, so it considers two tag arrays are different even though they have same elements with different orders. Function of updating tags didn't include this corner case, and it caused infinite update.
3. When creating resources, some resources take a while to finish creation. Function of getting tags got error because it couldn't find resource, and this error covered previous ResourceNotFound error. Hence, reconciler didn't call requeue, because it didn't recognize the new error.
4. Cluster update validation test file has a step to update and validate Snapshot Window (daily time range to auto snapshot). If this step is executing within the time range of new Snapshot Window, update would succeed (ResourceSynced returns true) because two resources match, but cluster starts snapshotting after it's Snapshot Window is updated to the current time. Hence, for next step to update other fields of cluster, new fields couldn't be updated on time, because the status of cluster is snapshotting and cluster cannot be updated.

Description of changes:
1. Fix updateTags function and it covers situation of equal tag arrays. (It still wastes time to call sdkUpdate over and over. It is better to modify delta comparison of tags in code-generator. New PR is created (aws-controllers-k8s/community#1658).
2. Change updateTags functions from parameter group and subnet group to match the updateTags functions in other resources.
3. Add a status check for getTags. It is only called when status of resource (cluster, snapshot, acl, user) is active.
4. Increase wait time for the next update step after Snapshot Window update. The new wait time is enough to cover snapshotting process.
5. Use same method to create cluster for snapshot for snapshot_validate_tags.yaml test file.
6. Correct user names and delete unused resources in test files.
7. Modify some test IDs, step IDs, and descriptions which are not appropriate that makes debugging really hard.
8. Fix import order of hook files.
9. Modify equalStrings function.

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

Author: kyriechen96

apis/v1alpha1/ack-generate-metadata.yaml

@@ -1,8 +1,8 @@
 ack_generate_info:
-  build_date: "2023-01-10T21:58:28Z"
-  build_hash: 1b20baf45a0b73a11b296050322a384c705fa897
-  go_version: go1.17.5
-  version: v0.22.0
+  build_date: "2023-02-03T00:39:12Z"
+  build_hash: c6651c200ba136e5c7f50ad8be751fae060a38e6
+  go_version: go1.19
+  version: v0.22.0-1-gc6651c2
 api_directory_checksum: ee32acc4d4a0ba7e2823dd20fdbe2c4ef1d9e0f4
 api_version: v1alpha1
 aws_sdk_go_version: v1.44.93

apis/v1alpha1/subnet_group.go

@@ -34,8 +34,8 @@ spec:
             type: object
           spec:
             description: "SubnetGroupSpec defines the desired state of SubnetGroup.
-              \n Represents the output of one of the following operations: \n * CreateSubnetGroup
-              \n * UpdateSubnetGroup \n A subnet group is a collection of subnets
+              \n Represents the output of one of the following operations: \n - CreateSubnetGroup
+              \n - UpdateSubnetGroup \n A subnet group is a collection of subnets
               (typically private) that you can designate for your clusters running
               in an Amazon Virtual Private Cloud (VPC) environment."
             properties:

apis/v1alpha1/types.go

@@ -20,9 +20,13 @@ import (
 	"github.com/aws-controllers-k8s/runtime/pkg/requeue"
 	ackrtlog "github.com/aws-controllers-k8s/runtime/pkg/runtime/log"
 	ackutil "github.com/aws-controllers-k8s/runtime/pkg/util"
+	svcsdk "github.com/aws/aws-sdk-go/service/memorydb"
 
 	svcapitypes "github.com/aws-controllers-k8s/memorydb-controller/apis/v1alpha1"
-	svcsdk "github.com/aws/aws-sdk-go/service/memorydb"
+)
+
+var (
+	resourceStatusActive string = "active"
 )
 
 // validateACLNeedsUpdate this function's purpose is to requeue if the resource is currently unavailable
@@ -32,7 +36,7 @@ func (rm *resourceManager) validateACLNeedsUpdate(
 
 	// requeue if necessary
 	latestStatus := latest.ko.Status.Status
-	if latestStatus == nil || *latestStatus != "active" {
+	if latestStatus == nil || *latestStatus != resourceStatusActive {
 		return requeue.NeededAfter(
 			errors.New("ACL cannot be modified as its status is not 'active'."),
 			requeue.DefaultRequeueAfterDuration)
@@ -41,6 +45,14 @@ func (rm *resourceManager) validateACLNeedsUpdate(
 	return nil
 }
 
+// aclActive returns true when the status of the given ACL is set to `active`
+func (rm *resourceManager) aclActive(
+	latest *resource,
+) bool {
+	latestStatus := latest.ko.Status.Status
+	return latestStatus != nil && *latestStatus == resourceStatusActive
+}
+
 // getTags gets tags from given ParameterGroup.
 func (rm *resourceManager) getTags(
 	ctx context.Context,
@@ -114,17 +126,23 @@ func computeTagsDelta(
 	latest []*svcapitypes.Tag,
 ) (addedOrUpdated []*svcapitypes.Tag, removed []*string) {
 	var visitedIndexes []string
+	var hasSameKey bool
 
 	for _, latestElement := range latest {
+		hasSameKey = false
 		visitedIndexes = append(visitedIndexes, *latestElement.Key)
 		for _, desiredElement := range desired {
 			if equalStrings(latestElement.Key, desiredElement.Key) {
+				hasSameKey = true
 				if !equalStrings(latestElement.Value, desiredElement.Value) {
 					addedOrUpdated = append(addedOrUpdated, desiredElement)
 				}
-				continue
+				break
 			}
 		}
+		if hasSameKey {
+			continue
+		}
 		removed = append(removed, latestElement.Key)
 	}
 	for _, desiredElement := range desired {
@@ -163,7 +181,9 @@ func resourceTagsFromSDKTags(
 
 func equalStrings(a, b *string) bool {
 	if a == nil {
-		return b == nil || *b == ""
+		return b == nil
+	} else if b == nil {
+		return false
 	}
-	return (*a == "" && b == nil) || *a == *b
+	return *a == *b
 }

helm/crds/memorydb.services.k8s.aws_subnetgroups.yaml

@@ -24,14 +24,15 @@ import (
 	ackrequeue "github.com/aws-controllers-k8s/runtime/pkg/requeue"
 	ackrtlog "github.com/aws-controllers-k8s/runtime/pkg/runtime/log"
 	ackutil "github.com/aws-controllers-k8s/runtime/pkg/util"
+	svcsdk "github.com/aws/aws-sdk-go/service/memorydb"
 
 	svcapitypes "github.com/aws-controllers-k8s/memorydb-controller/apis/v1alpha1"
-	svcsdk "github.com/aws/aws-sdk-go/service/memorydb"
 )
 
 var (
-	condMsgCurrentlyDeleting     = "cluster currently being deleted"
-	condMsgNoDeleteWhileUpdating = "cluster is being updated. cannot delete"
+	condMsgCurrentlyDeleting            = "cluster currently being deleted"
+	condMsgNoDeleteWhileUpdating        = "cluster is being updated. cannot delete"
+	resourceStatusActive         string = "active"
 )
 
 var (
@@ -264,6 +265,14 @@ func (rm *resourceManager) newMemoryDBClusterUploadPayload(
 	return res
 }
 
+// clusterActive returns true when the status of the given Cluster is set to `active`
+func (rm *resourceManager) clusterActive(
+	latest *resource,
+) bool {
+	latestStatus := latest.ko.Status.Status
+	return latestStatus != nil && *latestStatus == resourceStatusActive
+}
+
 // getTags gets tags from given ParameterGroup.
 func (rm *resourceManager) getTags(
 	ctx context.Context,
@@ -337,17 +346,23 @@ func computeTagsDelta(
 	latest []*svcapitypes.Tag,
 ) (addedOrUpdated []*svcapitypes.Tag, removed []*string) {
 	var visitedIndexes []string
+	var hasSameKey bool
 
 	for _, latestElement := range latest {
+		hasSameKey = false
 		visitedIndexes = append(visitedIndexes, *latestElement.Key)
 		for _, desiredElement := range desired {
 			if equalStrings(latestElement.Key, desiredElement.Key) {
+				hasSameKey = true
 				if !equalStrings(latestElement.Value, desiredElement.Value) {
 					addedOrUpdated = append(addedOrUpdated, desiredElement)
 				}
-				continue
+				break
 			}
 		}
+		if hasSameKey {
+			continue
+		}
 		removed = append(removed, latestElement.Key)
 	}
 	for _, desiredElement := range desired {
@@ -386,7 +401,9 @@ func resourceTagsFromSDKTags(
 
 func equalStrings(a, b *string) bool {
 	if a == nil {
-		return b == nil || *b == ""
+		return b == nil
+	} else if b == nil {
+		return false
 	}
-	return (*a == "" && b == nil) || *a == *b
+	return *a == *b
 }

pkg/resource/acl/hooks.go

@@ -19,10 +19,10 @@ import (
 	ackv1alpha1 "github.com/aws-controllers-k8s/runtime/apis/core/v1alpha1"
 	ackerr "github.com/aws-controllers-k8s/runtime/pkg/errors"
 	ackrtlog "github.com/aws-controllers-k8s/runtime/pkg/runtime/log"
+	ackutil "github.com/aws-controllers-k8s/runtime/pkg/util"
+	svcsdk "github.com/aws/aws-sdk-go/service/memorydb"
 
 	svcapitypes "github.com/aws-controllers-k8s/memorydb-controller/apis/v1alpha1"
-
-	svcsdk "github.com/aws/aws-sdk-go/service/memorydb"
 )
 
 func (rm *resourceManager) setParameters(
@@ -173,13 +173,7 @@ func (rm *resourceManager) getTags(
 	if err != nil {
 		return nil, err
 	}
-	tags := make([]*svcapitypes.Tag, 0, len(resp.TagList))
-	for _, tag := range resp.TagList {
-		tags = append(tags, &svcapitypes.Tag{
-			Key:   tag.Key,
-			Value: tag.Value,
-		})
-	}
+	tags := resourceTagsFromSDKTags(resp.TagList)
 	return tags, nil
 }
 
@@ -235,34 +229,33 @@ func (rm *resourceManager) updateTags(
 func computeTagsDelta(
 	desired []*svcapitypes.Tag,
 	latest []*svcapitypes.Tag,
-) (added []*svcapitypes.Tag, removed []*string) {
-	toDelete := []*string{}
-	toAdd := []*svcapitypes.Tag{}
+) (addedOrUpdated []*svcapitypes.Tag, removed []*string) {
+	var visitedIndexes []string
+	var hasSameKey bool
 
-	desiredTags := map[string]string{}
-	key := ""
-	value := ""
-	for _, tag := range desired {
-		if tag.Key != nil {
-			key = *tag.Key
-			value = ""
-			if tag.Value != nil {
-				value = *tag.Value
+	for _, latestElement := range latest {
+		hasSameKey = false
+		visitedIndexes = append(visitedIndexes, *latestElement.Key)
+		for _, desiredElement := range desired {
+			if equalStrings(latestElement.Key, desiredElement.Key) {
+				hasSameKey = true
+				if !equalStrings(latestElement.Value, desiredElement.Value) {
+					addedOrUpdated = append(addedOrUpdated, desiredElement)
+				}
+				break
 			}
-			desiredTags[key] = value
 		}
+		if hasSameKey {
+			continue
+		}
+		removed = append(removed, latestElement.Key)
 	}
-
-	for _, tag := range desired {
-		toAdd = append(toAdd, tag)
-	}
-	for _, tag := range latest {
-		_, ok := desiredTags[*tag.Key]
-		if !ok {
-			toDelete = append(toDelete, tag.Key)
+	for _, desiredElement := range desired {
+		if !ackutil.InStrings(*desiredElement.Key, visitedIndexes) {
+			addedOrUpdated = append(addedOrUpdated, desiredElement)
 		}
 	}
-	return toAdd, toDelete
+	return addedOrUpdated, removed
 }
 
 func sdkTagsFromResourceTags(
@@ -277,3 +270,25 @@ func sdkTagsFromResourceTags(
 	}
 	return tags
 }
+
+func resourceTagsFromSDKTags(
+	sdkTags []*svcsdk.Tag,
+) []*svcapitypes.Tag {
+	tags := make([]*svcapitypes.Tag, len(sdkTags))
+	for i := range sdkTags {
+		tags[i] = &svcapitypes.Tag{
+			Key:   sdkTags[i].Key,
+			Value: sdkTags[i].Value,
+		}
+	}
+	return tags
+}
+
+func equalStrings(a, b *string) bool {
+	if a == nil {
+		return b == nil
+	} else if b == nil {
+		return false
+	}
+	return *a == *b
+}