Modify tags related functions and some test files to fix flaky assert…

…ion errors (aws-controllers-k8s#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.

apis/v1alpha1/ack-generate-metadata.yaml

@@ -1,13 +1,13 @@
 ack_generate_info:
-  build_date: "2023-01-10T21:58:28Z"
-  build_hash: 1b20baf45a0b73a11b296050322a384c705fa897
-  go_version: go1.17.5
-  version: v0.22.0
-api_directory_checksum: ee32acc4d4a0ba7e2823dd20fdbe2c4ef1d9e0f4
+  build_date: "2023-02-06T23:57:24Z"
+  build_hash: 9c9c7c2e841a27e56ea46dc962bf026ea6857912
+  go_version: go1.19
+  version: v0.23.1-2-g9c9c7c2
+api_directory_checksum: 585f390bd5f6e7f305c0202cc5982c5b63a14947
 api_version: v1alpha1
 aws_sdk_go_version: v1.44.93
 generator_config_info:
-  file_checksum: d7ad13c5bc8d9e9e2171c92dc3ac51c2b5e3b769
+  file_checksum: 48977f3ee0ba02781e608ede837865bbe0ef4ce4
   original_file_name: generator.yaml
 last_modification:
   reason: API generation

apis/v1alpha1/generator.yaml

@@ -236,6 +236,11 @@ resources:
         DescribeSnapshots:
           input_fields:
             SnapshotName: Name
+    synced:
+      when:
+        - path: Status.Status
+          in:
+            - available
   ParameterGroup:
     exceptions:
       errors:

config/controller/deployment.yaml

@@ -1,8 +1,6 @@
 apiVersion: v1
 kind: Namespace
 metadata:
-  labels:
-    control-plane: controller
   name: ack-system
 ---
 apiVersion: apps/v1
@@ -11,16 +9,17 @@ metadata:
   name: ack-memorydb-controller
   namespace: ack-system
   labels:
-    control-plane: controller
+    app.kubernetes.io/name: ack-memorydb-controller
+    app.kubernetes.io/part-of: ack-system
 spec:
   selector:
     matchLabels:
-      control-plane: controller
+      app.kubernetes.io/name: ack-memorydb-controller
   replicas: 1
   template:
     metadata:
       labels:
-        control-plane: controller
+        app.kubernetes.io/name: ack-memorydb-controller
     spec:
       containers:
       - command:

config/controller/service.yaml

@@ -5,7 +5,7 @@ metadata:
   namespace: ack-system
 spec:
   selector:
-    control-plane: controller
+    app.kubernetes.io/name: ack-memorydb-controller
   ports:
     - name: metricsport
       port: 8080

config/crd/bases/memorydb.services.k8s.aws_snapshots.yaml

@@ -68,6 +68,7 @@ spec:
                   type: object
                 type: array
             required:
+            - clusterName
             - name
             type: object
           status:

generator.yaml

@@ -236,6 +236,11 @@ resources:
         DescribeSnapshots:
           input_fields:
             SnapshotName: Name
+    synced:
+      when:
+        - path: Status.Status
+          in:
+            - available
   ParameterGroup:
     exceptions:
       errors:

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

@@ -68,6 +68,7 @@ spec:
                   type: object
                 type: array
             required:
+            - clusterName
             - name
             type: object
           status:

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

@@ -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:

helm/templates/deployment.yaml

@@ -10,7 +10,6 @@ metadata:
     app.kubernetes.io/version: {{ .Chart.AppVersion | quote }}
     k8s-app: {{ include "app.name" . }}
     helm.sh/chart: {{ include "chart.name-version" . }}
-    control-plane: controller
 spec:
   replicas: 1
   selector:

helm/templates/metrics-service.yaml

@@ -11,7 +11,6 @@ metadata:
     app.kubernetes.io/version: {{ .Chart.AppVersion | quote }}
     k8s-app: {{ include "app.name" . }}
     helm.sh/chart: {{ include "chart.name-version" . }}
-    control-plane: controller
 spec:
   selector:
     app.kubernetes.io/name: {{ include "app.name" . }}

pkg/resource/acl/hooks.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
 }
 
+// isAclActive returns true when the status of the given ACL is set to `active`
+func (rm *resourceManager) isAclActive(
+	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
 }

pkg/resource/cluster/hooks.go

@@ -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
 }
 
+// isClusterActive returns true when the status of the given Cluster is set to `active`
+func (rm *resourceManager) isClusterActive(
+	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
 }