ensure all resources sync for RDS (#143)

jaypipes · web-flow · commit 4ac9aa8dcfdc · 2023-03-30T14:31:14.000Z
This patch fixes both the DB Parameter Group and the DB Cluster Parameter Group resources failure to properly get to ACK.ResourceSynced=True quickly and reliably. I removed the manual requeueWhileCreating condition for both Param Group and Cluster Param Group resources, replacing that sdk_create_post_set_output hook with a call to `syncParameters`, supplying a `nil` `latest` parameter. This allowed the parameter overrides to be immediately set after the parameter group was created. Added a lengthy e2e test for RDS resource references based on the YAML definitions present in the related issue (aws-controllers-k8s/community#1727) with a DBCluster resource that references a DB Cluster Parameter Group, a DBInstance resource that references a DB Parameter Group (by AWSResourceReference) and the DB Cluster (by DBClusterIdentifier). Fixes Issue aws-controllers-k8s/community#1727 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
diff --git a/apis/v1alpha1/ack-generate-metadata.yaml b/apis/v1alpha1/ack-generate-metadata.yaml
@@ -1,7 +1,7 @@
 ack_generate_info:
-  build_date: "2023-03-22T21:52:47Z"
+  build_date: "2023-03-24T19:44:43Z"
   build_hash: fa24753ea8b657d8815ae3eac7accd0958f5f9fb
-  go_version: go1.19
+  go_version: go1.19.4
   version: v0.25.0
 api_directory_checksum: 249bb23477dbb69a88064ea191a078f41f4966d0
 api_version: v1alpha1
diff --git a/pkg/resource/db_cluster_parameter_group/hooks.go b/pkg/resource/db_cluster_parameter_group/hooks.go
@@ -216,12 +216,18 @@ func (rm *resourceManager) syncParameters(
 	exit := rlog.Trace("rm.syncParameters")
 	defer func() { exit(err) }()
 
-	groupName := latest.ko.Spec.Name
-	family := latest.ko.Spec.Family
+	groupName := desired.ko.Spec.Name
+	family := desired.ko.Spec.Family
+
+	desiredOverrides := desired.ko.Spec.ParameterOverrides
+	latestOverrides := util.Parameters{}
+	// In the create code paths, we pass a nil latest...
+	if latest != nil {
+		latestOverrides = latest.ko.Spec.ParameterOverrides
+	}
 
 	toModify, _, toDelete := util.GetParametersDifference(
-		desired.ko.Spec.ParameterOverrides,
-		latest.ko.Spec.ParameterOverrides,
+		desiredOverrides, latestOverrides,
 	)
 
 	if len(toDelete) > 0 {
diff --git a/pkg/resource/db_cluster_parameter_group/sdk.go b/pkg/resource/db_cluster_parameter_group/sdk.go
diff --git a/pkg/resource/db_parameter_group/hooks.go b/pkg/resource/db_parameter_group/hooks.go
@@ -215,12 +215,18 @@ func (rm *resourceManager) syncParameters(
 	exit := rlog.Trace("rm.syncParameters")
 	defer func() { exit(err) }()
 
-	groupName := latest.ko.Spec.Name
-	family := latest.ko.Spec.Family
+	groupName := desired.ko.Spec.Name
+	family := desired.ko.Spec.Family
+
+	desiredOverrides := desired.ko.Spec.ParameterOverrides
+	latestOverrides := util.Parameters{}
+	// In the create code paths, we pass a nil latest...
+	if latest != nil {
+		latestOverrides = latest.ko.Spec.ParameterOverrides
+	}
 
 	toModify, _, toDelete := util.GetParametersDifference(
-		desired.ko.Spec.ParameterOverrides,
-		latest.ko.Spec.ParameterOverrides,
+		desiredOverrides, latestOverrides,
 	)
 
 	// NOTE(jaypipes): ResetDBParameterGroup and ModifyDBParameterGroup only
diff --git a/pkg/resource/db_parameter_group/sdk.go b/pkg/resource/db_parameter_group/sdk.go
diff --git a/templates/hooks/db_cluster_parameter_group/sdk_create_post_set_output.go.tpl b/templates/hooks/db_cluster_parameter_group/sdk_create_post_set_output.go.tpl
@@ -1,4 +1,3 @@
-    // We need to instantly requeue after a create operation, otherwise the controller
-    // will override the latest resource with the desired resource overrideParameters
-    // and cause the controller to not properly compare latest and desired resources.
-    return &resource{ko}, requeueWaitWhileCreating
+	if err = rm.syncParameters(ctx, desired, nil); err != nil {
+		return nil, err
+	}
diff --git a/templates/hooks/db_parameter_group/sdk_create_post_set_output.go.tpl b/templates/hooks/db_parameter_group/sdk_create_post_set_output.go.tpl
@@ -1,4 +1,3 @@
-    // We need to instantly requeue after a create operation, otherwise the controller
-    // will override the latest resource with the desired resource overrideParameters
-    // and cause the controller to not properly compare latest and desired resources.
-    return &resource{ko}, requeueWaitWhileCreating
+	if err = rm.syncParameters(ctx, desired, nil); err != nil {
+		return nil, err
+	}
diff --git a/test/e2e/db_cluster.py b/test/e2e/db_cluster.py
@@ -35,7 +35,8 @@ def __init__(self, status):
         self.match_on = status
 
     def __call__(self, record: dict) -> bool:
-        return 'Status' in record and record['Status'] == self.match_on
+        return (record is not None and 'Status' in record
+                and record['Status'] == self.match_on)
 
 
 def status_matches(status: str) -> ClusterMatchFunc:
diff --git a/test/e2e/db_instance.py b/test/e2e/db_instance.py
@@ -22,7 +22,7 @@
 
 DEFAULT_WAIT_UNTIL_TIMEOUT_SECONDS = 60*30
 DEFAULT_WAIT_UNTIL_INTERVAL_SECONDS = 15
-DEFAULT_WAIT_UNTIL_DELETED_TIMEOUT_SECONDS = 60*10
+DEFAULT_WAIT_UNTIL_DELETED_TIMEOUT_SECONDS = 60*20
 DEFAULT_WAIT_UNTIL_DELETED_INTERVAL_SECONDS = 15
 
 InstanceMatchFunc = typing.NewType(
@@ -35,7 +35,7 @@ def __init__(self, status):
         self.match_on = status
 
     def __call__(self, record: dict) -> bool:
-        return ('DBInstanceStatus' in record
+        return (record is not None and 'DBInstanceStatus' in record
                 and record['DBInstanceStatus'] == self.match_on)
 
 
diff --git a/test/e2e/resources/db_cluster_parameter_group_aurora_postgresql14.yaml b/test/e2e/resources/db_cluster_parameter_group_aurora_postgresql14.yaml
@@ -0,0 +1,8 @@
+apiVersion: rds.services.k8s.aws/v1alpha1
+kind: DBClusterParameterGroup
+metadata:
+  name: $DB_CLUSTER_PARAMETER_GROUP_NAME
+spec:
+  name: $DB_CLUSTER_PARAMETER_GROUP_NAME
+  description: $DB_CLUSTER_PARAMETER_GROUP_DESC
+  family: aurora-postgresql14
diff --git a/test/e2e/resources/db_cluster_ref.yaml b/test/e2e/resources/db_cluster_ref.yaml
@@ -0,0 +1,19 @@
+apiVersion: rds.services.k8s.aws/v1alpha1
+kind: DBCluster
+metadata:
+  name: $DB_CLUSTER_ID
+spec:
+  copyTagsToSnapshot: false
+  dbClusterIdentifier: $DB_CLUSTER_ID
+  databaseName: $DB_NAME
+  dbClusterParameterGroupRef:
+    from:
+      name: $DB_CLUSTER_PARAMETER_GROUP_NAME
+  engine: aurora-postgresql
+  engineMode: provisioned
+  engineVersion: "14.6"
+  masterUsername: root
+  masterUserPassword:
+    namespace: $MASTER_USER_PASS_SECRET_NAMESPACE
+    name: $MASTER_USER_PASS_SECRET_NAME
+    key: $MASTER_USER_PASS_SECRET_KEY
diff --git a/test/e2e/resources/db_instance_ref.yaml b/test/e2e/resources/db_instance_ref.yaml
@@ -0,0 +1,14 @@
+apiVersion: rds.services.k8s.aws/v1alpha1
+kind: DBInstance
+metadata:
+  name: $DB_INSTANCE_ID
+spec:
+  # NOTE(jaypipes): This needs to be db.t3.medium to support Aurora PostgreSQL
+  # 14.6+. Smaller sizes will result in an InvalidParameterCombination.
+  dbInstanceClass: db.t3.medium
+  dbInstanceIdentifier: $DB_INSTANCE_ID
+  dbClusterIdentifier: $DB_CLUSTER_ID
+  dbParameterGroupRef:
+    from:
+      name: $DB_PARAMETER_GROUP_NAME
+  engine: aurora-postgresql
diff --git a/test/e2e/resources/db_parameter_group_aurora_postgresql14.yaml b/test/e2e/resources/db_parameter_group_aurora_postgresql14.yaml
@@ -0,0 +1,8 @@
+apiVersion: rds.services.k8s.aws/v1alpha1
+kind: DBParameterGroup
+metadata:
+  name: $DB_PARAMETER_GROUP_NAME
+spec:
+  name: $DB_PARAMETER_GROUP_NAME
+  description: $DB_PARAMETER_GROUP_DESC
+  family: aurora-postgresql14
diff --git a/test/e2e/tests/test_db_cluster_parameter_group.py b/test/e2e/tests/test_db_cluster_parameter_group.py
@@ -29,6 +29,7 @@
 
 RESOURCE_PLURAL = 'dbclusterparametergroups'
 
+CREATE_WAIT_AFTER_SECONDS = 10
 DELETE_WAIT_AFTER_SECONDS = 10
 # NOTE(jaypipes): According to the RDS API documentation, updating tags can
 # take several minutes before the new tag values are available due to caching.
@@ -58,6 +59,7 @@ def aurora_mysql57_cluster_param_group():
     )
     k8s.create_custom_resource(ref, resource_data)
     cr = k8s.wait_resource_consumed_by_controller(ref)
+    time.sleep(CREATE_WAIT_AFTER_SECONDS)
 
     assert cr is not None
     assert k8s.get_resource_exists(ref)
diff --git a/test/e2e/tests/test_db_parameter_group.py b/test/e2e/tests/test_db_parameter_group.py
@@ -30,8 +30,10 @@
 RESOURCE_PLURAL = 'dbparametergroups'
 
 CREATE_WAIT_AFTER_SECONDS = 10
-MODIFY_WAIT_AFTER_SECONDS = 10
 DELETE_WAIT_AFTER_SECONDS = 10
+# NOTE(jaypipes): According to the RDS API documentation, updating tags can
+# take several minutes before the new tag values are available due to caching.
+MODIFY_WAIT_AFTER_SECONDS = 180
 
 RESOURCE_DESC_PG13 = "Parameters for PostgreSQL 13"
 
@@ -56,8 +58,8 @@ def pg13_param_group():
         resource_name, namespace="default",
     )
     k8s.create_custom_resource(ref, resource_data)
-    time.sleep(CREATE_WAIT_AFTER_SECONDS)
     cr = k8s.wait_resource_consumed_by_controller(ref)
+    time.sleep(CREATE_WAIT_AFTER_SECONDS)
 
     assert cr is not None
     assert k8s.get_resource_exists(ref)
diff --git a/test/e2e/tests/test_references.py b/test/e2e/tests/test_references.py
