Fix: unable to set some parameters in the DbClusterParameterGroup

.gitignore

@@ -4,4 +4,6 @@
 .idea
 /docs/site
 bin
-build
+build
+ack.log
+test.txt

pkg/resource/db_cluster_parameter_group/hooks.go

@@ -220,6 +220,12 @@ func (rm *resourceManager) syncParameters(
 	groupName := desired.ko.Spec.Name
 	family := desired.ko.Spec.Family
 
+	// If there are no parameter overrides in the desired state,
+	// consider this a valid state and return success
+	if len(desired.ko.Spec.ParameterOverrides) == 0 {
+		return nil
+	}
+
 	desiredOverrides := desired.ko.Spec.ParameterOverrides
 	latestOverrides := util.Parameters{}
 	// In the create code paths, we pass a nil latest...

pkg/util/parameter_cache.go

@@ -42,6 +42,13 @@ type ParamMetaCache struct {
 	Cache  map[string]map[string]ParamMeta
 }
 
+// ClearFamily removes the cached parameter information for a specific family
+func (c *ParamMetaCache) ClearFamily(family string) {
+	c.Lock()
+	defer c.Unlock()
+	delete(c.Cache, family)
+}
+
 // Get retrieves the metadata for a named parameter group family and parameter
 // name.
 func (c *ParamMetaCache) Get(
@@ -70,6 +77,9 @@ func (c *ParamMetaCache) Get(
 	}
 	meta, found = metas[name]
 	if !found {
+		// Clear the cache for this family when a parameter is not found
+		// This ensures the next reconciliation will fetch fresh metadata
+		c.ClearFamily(family)
 		return nil, NewErrUnknownParameter(name)
 	}
 	c.Hits++

pkg/util/parameters.go

@@ -15,9 +15,6 @@ package util
 
 import (
 	"fmt"
-
-	ackerr "github.com/aws-controllers-k8s/runtime/pkg/errors"
-	"github.com/samber/lo"
 )
 
 var (
@@ -29,26 +26,20 @@ var (
 // or a DB Cluster Parameter Group
 type Parameters map[string]*string
 
-// NewErrUnknownParameter generates an ACK terminal error about
+// NewErrUnknownParameter generates an ACK error about
 // an unknown parameter
 func NewErrUnknownParameter(name string) error {
-	// This is a terminal error because unless the user removes this parameter
-	// from their list of parameter overrides, we will not be able to get the
-	// resource into a synced state.
-	return ackerr.NewTerminalError(
-		fmt.Errorf("%w: %s", ErrUnknownParameter, name),
-	)
+	// Changed from Terminal to regular error since it should be
+	// recoverable when the parameter is removed from the spec
+	return fmt.Errorf("%w: %s", ErrUnknownParameter, name)
 }
 
-// NewErrUnmodifiableParameter generates an ACK terminal error about
+// NewErrUnmodifiableParameter generates an ACK error about
 // a parameter that may not be modified
 func NewErrUnmodifiableParameter(name string) error {
-	// This is a terminal error because unless the user removes this parameter
-	// from their list of parameter overrides, we will not be able to get the
-	// resource into a synced state.
-	return ackerr.NewTerminalError(
-		fmt.Errorf("%w: %s", ErrUnmodifiableParameter, name),
-	)
+	// Changed from Terminal to regular error since it should be
+	// recoverable when the parameter is removed from the spec
+	return fmt.Errorf("%w: %s", ErrUnmodifiableParameter, name)
 }
 
 // GetParametersDifference compares two Parameters maps and returns the
@@ -57,16 +48,62 @@ func NewErrUnmodifiableParameter(name string) error {
 func GetParametersDifference(
 	to, from Parameters,
 ) (added, unchanged, removed Parameters) {
-	// we need to convert the tag tuples to a comparable interface type
-	fromPairs := lo.ToPairs(from)
-	toPairs := lo.ToPairs(to)
+	added = Parameters{}
+	unchanged = Parameters{}
+	removed = Parameters{}
+
+	// Handle nil maps
+	if to == nil {
+		to = Parameters{}
+	}
+	if from == nil {
+		from = Parameters{}
+	}
 
-	left, right := lo.Difference(fromPairs, toPairs)
-	middle := lo.Intersect(fromPairs, toPairs)
+	// If both maps are empty, return early
+	if len(to) == 0 && len(from) == 0 {
+		return added, unchanged, removed
+	}
+
+	// If 'from' is empty, all 'to' parameters are additions
+	if len(from) == 0 {
+		return to, unchanged, removed
+	}
 
-	removed = lo.FromPairs(left)
-	added = lo.FromPairs(right)
-	unchanged = lo.FromPairs(middle)
+	// If 'to' is empty, all 'from' parameters are removals
+	if len(to) == 0 {
+		return added, unchanged, from
+	}
+
+	// Find added and unchanged parameters
+	for toKey, toVal := range to {
+		if fromVal, exists := from[toKey]; exists {
+			// Parameter exists in both maps
+			if toVal == nil && fromVal == nil {
+				// Both values are nil, consider unchanged
+				unchanged[toKey] = nil
+			} else if toVal == nil || fromVal == nil {
+				// One value is nil, the other isn't - consider it a modification
+				added[toKey] = toVal
+			} else if *toVal == *fromVal {
+				// Both values are non-nil and equal
+				unchanged[toKey] = toVal
+			} else {
+				// Both values are non-nil but different
+				added[toKey] = toVal
+			}
+		} else {
+			// Not in 'from' = new parameter
+			added[toKey] = toVal
+		}
+	}
+
+	// Find removed parameters
+	for fromKey, fromVal := range from {
+		if _, exists := to[fromKey]; !exists {
+			removed[fromKey] = fromVal
+		}
+	}
 
 	return added, unchanged, removed
 }

test/e2e/.gitignore

@@ -1,3 +1,5 @@
 __pycache__/
 *.py[cod]
-**/bootstrap.pkl
+**/bootstrap.pkl
+ack.log
+test.txt

test/e2e/conftest.py

@@ -14,15 +14,19 @@
 import os
 import pytest
 import boto3
+import logging
 
 from acktest import k8s
+from e2e.resource_cleanup import cleanup_old_resources
 
-
-def pytest_addoption(parser):
-    parser.addoption("--runslow", action="store_true", default=False, help="run slow tests")
-
-
+# Increase default timeouts to handle AWS API latency
 def pytest_configure(config):
+    # Set longer timeouts for tests
+    os.environ['PYTEST_TIMEOUT'] = '1800'  # 30 minutes (increased from 15)
+
+    # Configure longer shutdown timeout to allow proper cleanup
+    config.option.shutdown_timeout = 300  # 5 minutes to allow AWS resources to clean up
+
     config.addinivalue_line(
         "markers", "canary: mark test to also run in canary tests"
     )
@@ -32,6 +36,24 @@ def pytest_configure(config):
     config.addinivalue_line(
         "markers", "slow: mark test as slow to run"
     )
+
+    # Configure logging
+    logging.basicConfig(
+        level=logging.INFO,
+        format='%(asctime)s - %(name)s - %(levelname)s - %(message)s'
+    )
+
+    # Clean up any old test resources that might be lingering
+    try:
+        logging.info("Running pre-test cleanup of stale AWS resources...")
+        cleanup_old_resources()
+    except Exception as e:
+        logging.warning(f"Error during pre-test resource cleanup: {str(e)}")
+
+
+def pytest_addoption(parser):
+    parser.addoption("--runslow", action="store_true", default=False, help="run slow tests")
+
 
 def pytest_collection_modifyitems(config, items):
     if config.getoption("--runslow"):
@@ -56,4 +78,23 @@ def rds_resource():
 
 @pytest.fixture(scope='module')
 def sts_client():
-    return boto3.client('sts')
+    return boto3.client('sts')
+
+def pytest_sessionfinish(session, exitstatus):
+    """Run at the end of the test session to ensure proper cleanup.
+
+    This function will run regardless of whether tests passed or failed,
+    ensuring that AWS resources are cleaned up properly.
+    """
+    logging.info(f"Test session finished with exit status: {exitstatus}")
+
+    if exitstatus != 0:
+        # Tests failed, perform additional cleanup to prevent resources from being orphaned
+        try:
+            from e2e.resource_cleanup import force_cleanup_test_resources
+            logging.info("Running forced cleanup due to test failures...")
+            force_cleanup_test_resources()
+        except Exception as e:
+            logging.warning(f"Error during forced cleanup: {str(e)}")
+
+    logging.info("Test session cleanup completed")