Fix DBParameterGroup deadlock and add e2e test cases

Requeue after a create DBParameterGroup API call

The DBParameterGroup API call doesn't allow users to set parameters, to
set those users will have to call `ModifyDBParameterGroup`, however the
delta function was failing to detect differences between two DBPGs
mainly because `sdkCreate` makes a deep copy of the desired state and
returns that to the main reconcilliation logic.

This patch fixes this issue by requeueing right after a successfull
create call. This allows the controller to properly read the latest
state and compute the delta between latest and desired.

Author: a-hilaly

apis/v1alpha1/ack-generate-metadata.yaml

@@ -1,13 +1,13 @@
 ack_generate_info:
-  build_date: "2022-08-22T20:21:30Z"
-  build_hash: fe61d04673fd4d9848d5f726b01e0689a16d3733
-  go_version: go1.18.2
-  version: v0.19.3-1-gfe61d04
+  build_date: "2022-09-02T16:27:09Z"
+  build_hash: cdb0d99c7c1d3d00e71623eb56e650960dcef0d5
+  go_version: go1.18.3
+  version: v0.19.3-2-gcdb0d99
 api_directory_checksum: 83bde1f77a49ebd8acd36a1c5e231138b67aa883
 api_version: v1alpha1
 aws_sdk_go_version: v1.44.27
 generator_config_info:
-  file_checksum: 5d126ead4715f729c2fda976c422eb14a948f3da
+  file_checksum: 868688590b240be6d557dd31f0583bad2d309cb8
   original_file_name: generator.yaml
 last_modification:
   reason: API generation

apis/v1alpha1/generator.yaml

@@ -319,6 +319,8 @@ resources:
         template_path: hooks/db_parameter_group/sdk_read_many_post_set_output.go.tpl
       delta_pre_compare:
         template_path: hooks/db_parameter_group/delta_pre_compare.go.tpl
+      sdk_create_post_set_output:
+        template_path: hooks/db_parameter_group/sdk_create_post_set_output.go.tpl
     fields:
       Name:
         is_primary_key: true

generator.yaml

@@ -319,6 +319,8 @@ resources:
         template_path: hooks/db_parameter_group/sdk_read_many_post_set_output.go.tpl
       delta_pre_compare:
         template_path: hooks/db_parameter_group/delta_pre_compare.go.tpl
+      sdk_create_post_set_output:
+        template_path: hooks/db_parameter_group/sdk_create_post_set_output.go.tpl
     fields:
       Name:
         is_primary_key: true

pkg/resource/db_parameter_group/hooks.go

@@ -17,15 +17,16 @@ import (
 	"context"
 	"fmt"
 	"sync"
+	"time"
 
 	ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare"
 	ackerr "github.com/aws-controllers-k8s/runtime/pkg/errors"
+	ackrequeue "github.com/aws-controllers-k8s/runtime/pkg/requeue"
 	ackrtlog "github.com/aws-controllers-k8s/runtime/pkg/runtime/log"
-
-	svcapitypes "github.com/aws-controllers-k8s/rds-controller/apis/v1alpha1"
 	"github.com/aws/aws-sdk-go/aws"
 	svcsdk "github.com/aws/aws-sdk-go/service/rds"
 
+	svcapitypes "github.com/aws-controllers-k8s/rds-controller/apis/v1alpha1"
 	"github.com/aws-controllers-k8s/rds-controller/pkg/util"
 )
 
@@ -36,8 +37,14 @@ const (
 )
 
 var (
-	errUnknownParameter      = fmt.Errorf("unknown parameter")
-	errUnmodifiableParameter = fmt.Errorf("parameter is not modifiable")
+	errUnknownParameter         = fmt.Errorf("unknown parameter")
+	errUnmodifiableParameter    = fmt.Errorf("parameter is not modifiable")
+	errParamterGroupJustCreated = fmt.Errorf("parameter group just got created")
+
+	requeueWaitWhileCreating = ackrequeue.NeededAfter(
+		errParamterGroupJustCreated,
+		100*time.Millisecond,
+	)
 )
 
 func newErrUnknownParameter(name string) error {
@@ -277,7 +284,7 @@ func (rm *resourceManager) getParameters(
 	err error,
 ) {
 	var marker *string
-
+	params = make(map[string]*string)
 	for {
 		resp, err := rm.sdkapi.DescribeDBParametersWithContext(
 			ctx,
@@ -375,7 +382,7 @@ func (rm *resourceManager) modifyParameters(
 	toModify map[string]*string,
 ) (err error) {
 	rlog := ackrtlog.FromContext(ctx)
-	exit := rlog.Trace("rm.resetParameters")
+	exit := rlog.Trace("rm.modifyParameters")
 	defer func() { exit(err) }()
 
 	var pMeta *paramMeta
@@ -563,15 +570,17 @@ func (c *paramMetaCache) get(
 	name string,
 	fetcher metaFetcher,
 ) (*paramMeta, error) {
-	c.RLock()
-	defer c.RUnlock()
-
 	var err error
 	var found bool
 	var metas map[string]paramMeta
 	var meta paramMeta
 
+	// We need to release the lock right after the read operation, because
+	// loadFamilly will might call a writeLock at L619
+	c.RLock()
 	metas, found = c.cache[family]
+	c.RUnlock()
+
 	if !found {
 		c.misses++
 		metas, err = c.loadFamily(ctx, family, fetcher)

pkg/resource/db_parameter_group/sdk.go

@@ -0,0 +1,4 @@
+    // 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

templates/hooks/db_parameter_group/sdk_create_post_set_output.go.tpl

@@ -32,6 +32,19 @@ def get(db_parameter_group_name):
     except c.exceptions.DBParameterGroupNotFoundFault:
         return None
 
+def get_parameters(db_parameter_group_name):
+    """Returns a dict containing the paramters of a given parameter group
+
+    If no such DB parameter group exists, returns None.
+    """
+    c = boto3.client('rds')
+    try:
+        resp = c.describe_db_parameters(
+            DBParameterGroupName=db_parameter_group_name,
+        )
+        return resp['Parameters']
+    except c.exceptions.DBParameterGroupNotFoundFault:
+        return None
 
 def get_tags(db_parameter_group_arn):
     """Returns a dict containing the DB parameter group's tag records from the

test/e2e/db_parameter_group.py

@@ -6,6 +6,9 @@ spec:
   name: $DB_PARAMETER_GROUP_NAME
   description: $DB_PARAMETER_GROUP_DESC
   family: postgres13
+  parameterOverrides:
+    array_nulls: "1"
+    authentication_timeout: "50"
   tags:
     - key: environment
       value: dev

test/e2e/resources/db_parameter_group_postgres13_standard.yaml

@@ -28,11 +28,9 @@
 
 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
-
 
 @service_marker
 @pytest.mark.canary
@@ -57,17 +55,25 @@ def test_create_delete_postgres13_standard(self):
             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)
 
         assert cr is not None
         assert k8s.get_resource_exists(ref)
-        condition.assert_synced(ref)
 
         # Let's check that the DB parameter group appears in RDS
         latest = db_parameter_group.get(resource_name)
         assert latest is not None
         assert latest['Description'] == resource_desc
 
+        params = db_parameter_group.get_parameters(resource_name)
+        test_params = list(filter(lambda x: x["ParameterName"] in ["array_nulls", "authentication_timeout"], params))
+        assert len(test_params) == 2
+        assert test_params[0]["ParameterName"] == "array_nulls"
+        assert test_params[0]["ParameterValue"] == "1"
+        assert test_params[1]["ParameterName"] == "authentication_timeout"
+        assert test_params[1]["ParameterValue"] == "50"
+
         arn = latest['DBParameterGroupArn']
         expect_tags = [
             {"Key": "environment", "Value": "dev"}
@@ -83,8 +89,16 @@ def test_create_delete_postgres13_standard(self):
                 "value": "prod",
             }
         ]
+        new_params = {
+            "array_nulls": "1",
+            "authentication_timeout": "60",
+        }
+        
         updates = {
-            "spec": {"tags": new_tags},
+            "spec": {
+                "tags": new_tags,
+                "parameterOverrides": new_params,
+            },
         }
         k8s.patch_custom_resource(ref, updates)
         time.sleep(MODIFY_WAIT_AFTER_SECONDS)
@@ -97,6 +111,11 @@ def test_create_delete_postgres13_standard(self):
             }
         ]
         assert latest_tags == after_update_expected_tags
+        params = db_parameter_group.get_parameters(resource_name)
+        test_params = list(filter(lambda x: x["ParameterName"] in ["array_nulls", "authentication_timeout"], params))
+        assert len(test_params) == 2
+        assert test_params[1]["ParameterName"] == "authentication_timeout"
+        assert test_params[1]["ParameterValue"] == "60"
 
         # Delete the k8s resource on teardown of the module
         k8s.delete_custom_resource(ref)