Merge pull request #30247 from hashicorp/td-aws_rds_cluster-pending-engine-version

Handle pending RDS engine version updates

Author: jar-b

.changelog/30247.txt

@@ -0,0 +1,16 @@
+```release-note:bug
+resource/aws_rds_cluster: Fix inconsistent final plan errors when `engine_version` updates are not applied immediately
+```
+
+```release-note:bug
+resource/aws_rds_cluster_instance: Fix inconsistent final plan errors when `engine_version` updates are not applied immediately
+```
+
+```release-note:bug
+resource/aws_rds_instance: Fix inconsistent final plan errors when `engine_version` updates are not applied immediately
+```
+
+```release-note:bug
+resource/aws_rds_cluster: Send `db_instance_parameter_group_name` on all modify requests when set 
+```
+

internal/service/rds/cluster.go

@@ -1237,8 +1237,11 @@ func resourceClusterUpdate(ctx context.Context, d *schema.ResourceData, meta int
 			input.DBClusterParameterGroupName = aws.String(d.Get("db_cluster_parameter_group_name").(string))
 		}
 
-		if d.HasChange("db_instance_parameter_group_name") {
-			input.DBInstanceParameterGroupName = aws.String(d.Get("db_instance_parameter_group_name").(string))
+		// DB instance parameter group name is not currently returned from the
+		// DescribeDBClusters API. This means there is no drift detection, so when
+		// set, the configured attribute should always be sent on modify.
+		if v, ok := d.GetOk("db_instance_parameter_group_name"); ok || d.HasChange("db_instance_parameter_group_name") {
+			input.DBInstanceParameterGroupName = aws.String(v.(string))
 		}
 
 		if d.HasChange("deletion_protection") {
@@ -1268,6 +1271,13 @@ func resourceClusterUpdate(ctx context.Context, d *schema.ResourceData, meta int
 			input.EngineVersion = aws.String(d.Get("engine_version").(string))
 		}
 
+		// This can happen when updates are deferred (apply_immediately = false), and
+		// multiple applies occur before the maintenance window. In this case,
+		// continue sending the desired engine_version as part of the modify request.
+		if d.Get("engine_version").(string) != d.Get("engine_version_actual").(string) {
+			input.EngineVersion = aws.String(d.Get("engine_version").(string))
+		}
+
 		if d.HasChange("iam_database_authentication_enabled") {
 			input.EnableIAMDatabaseAuthentication = aws.Bool(d.Get("iam_database_authentication_enabled").(bool))
 		}
@@ -1562,7 +1572,11 @@ func removeIAMRoleFromCluster(ctx context.Context, conn *rds.RDS, clusterID, rol
 func clusterSetResourceDataEngineVersionFromCluster(d *schema.ResourceData, c *rds.DBCluster) {
 	oldVersion := d.Get("engine_version").(string)
 	newVersion := aws.StringValue(c.EngineVersion)
-	compareActualEngineVersion(d, oldVersion, newVersion)
+	var pendingVersion string
+	if c.PendingModifiedValues != nil && c.PendingModifiedValues.EngineVersion != nil {
+		pendingVersion = aws.StringValue(c.PendingModifiedValues.EngineVersion)
+	}
+	compareActualEngineVersion(d, oldVersion, newVersion, pendingVersion)
 }
 
 func FindDBClusterByID(ctx context.Context, conn *rds.RDS, id string) (*rds.DBCluster, error) {

internal/service/rds/cluster_instance.go

@@ -563,5 +563,9 @@ func resourceClusterInstanceDelete(ctx context.Context, d *schema.ResourceData,
 func clusterSetResourceDataEngineVersionFromClusterInstance(d *schema.ResourceData, c *rds.DBInstance) {
 	oldVersion := d.Get("engine_version").(string)
 	newVersion := aws.StringValue(c.EngineVersion)
-	compareActualEngineVersion(d, oldVersion, newVersion)
+	var pendingVersion string
+	if c.PendingModifiedValues != nil && c.PendingModifiedValues.EngineVersion != nil {
+		pendingVersion = aws.StringValue(c.PendingModifiedValues.EngineVersion)
+	}
+	compareActualEngineVersion(d, oldVersion, newVersion, pendingVersion)
 }

internal/service/rds/cluster_test.go

@@ -217,7 +217,7 @@ func TestAccRDSCluster_allowMajorVersionUpgrade(t *testing.T) {
 		CheckDestroy:             testAccCheckClusterDestroy(ctx),
 		Steps: []resource.TestStep{
 			{
-				Config: testAccClusterConfig_allowMajorVersionUpgrade(rName, true, engine, engineVersion1),
+				Config: testAccClusterConfig_allowMajorVersionUpgrade(rName, true, engine, engineVersion1, true),
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckClusterExists(ctx, resourceName, &dbCluster1),
 					resource.TestCheckResourceAttr(resourceName, "allow_major_version_upgrade", "true"),
@@ -240,7 +240,65 @@ func TestAccRDSCluster_allowMajorVersionUpgrade(t *testing.T) {
 				},
 			},
 			{
-				Config: testAccClusterConfig_allowMajorVersionUpgrade(rName, true, engine, engineVersion2),
+				Config: testAccClusterConfig_allowMajorVersionUpgrade(rName, true, engine, engineVersion2, true),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckClusterExists(ctx, resourceName, &dbCluster2),
+					resource.TestCheckResourceAttr(resourceName, "allow_major_version_upgrade", "true"),
+					resource.TestCheckResourceAttr(resourceName, "engine", engine),
+					resource.TestCheckResourceAttr(resourceName, "engine_version", engineVersion2),
+				),
+			},
+		},
+	})
+}
+
+func TestAccRDSCluster_allowMajorVersionUpgradeNoApplyImmediately(t *testing.T) {
+	ctx := acctest.Context(t)
+	if testing.Short() {
+		t.Skip("skipping long-running test in short mode")
+	}
+
+	var dbCluster1, dbCluster2 rds.DBCluster
+	rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
+	resourceName := "aws_rds_cluster.test"
+	// If these hardcoded versions become a maintenance burden, use DescribeDBEngineVersions
+	// either by having a new data source created or implementing the testing similar
+	// to TestAccDMSReplicationInstance_engineVersion
+	engine := "aurora-postgresql"
+	engineVersion1 := "12.9"
+	engineVersion2 := "13.5"
+
+	resource.ParallelTest(t, resource.TestCase{
+		PreCheck:                 func() { acctest.PreCheck(ctx, t) },
+		ErrorCheck:               acctest.ErrorCheck(t, rds.EndpointsID),
+		ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
+		CheckDestroy:             testAccCheckClusterDestroy(ctx),
+		Steps: []resource.TestStep{
+			{
+				Config: testAccClusterConfig_allowMajorVersionUpgrade(rName, true, engine, engineVersion1, false),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckClusterExists(ctx, resourceName, &dbCluster1),
+					resource.TestCheckResourceAttr(resourceName, "allow_major_version_upgrade", "true"),
+					resource.TestCheckResourceAttr(resourceName, "engine", engine),
+					resource.TestCheckResourceAttr(resourceName, "engine_version", engineVersion1),
+				),
+			},
+			{
+				ResourceName:      resourceName,
+				ImportState:       true,
+				ImportStateVerify: true,
+				ImportStateVerifyIgnore: []string{
+					"allow_major_version_upgrade",
+					"apply_immediately",
+					"cluster_identifier_prefix",
+					"db_instance_parameter_group_name",
+					"enable_global_write_forwarding",
+					"master_password",
+					"skip_final_snapshot",
+				},
+			},
+			{
+				Config: testAccClusterConfig_allowMajorVersionUpgrade(rName, true, engine, engineVersion2, false),
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckClusterExists(ctx, resourceName, &dbCluster2),
 					resource.TestCheckResourceAttr(resourceName, "allow_major_version_upgrade", "true"),
@@ -2599,11 +2657,11 @@ resource "aws_rds_cluster" "test" {
 `, identifierPrefix)
 }
 
-func testAccClusterConfig_allowMajorVersionUpgrade(rName string, allowMajorVersionUpgrade bool, engine string, engineVersion string) string {
+func testAccClusterConfig_allowMajorVersionUpgrade(rName string, allowMajorVersionUpgrade bool, engine string, engineVersion string, applyImmediately bool) string {
 	return fmt.Sprintf(`
 resource "aws_rds_cluster" "test" {
   allow_major_version_upgrade = %[1]t
-  apply_immediately           = true
+  apply_immediately           = %[5]t
   cluster_identifier          = %[2]q
   engine                      = %[3]q
   engine_version              = %[4]q
@@ -2620,6 +2678,7 @@ data "aws_rds_orderable_db_instance" "test" {
 
 # Upgrading requires a healthy primary instance
 resource "aws_rds_cluster_instance" "test" {
+  apply_immediately  = %[5]t
   cluster_identifier = aws_rds_cluster.test.id
   engine             = data.aws_rds_orderable_db_instance.test.engine
   engine_version     = data.aws_rds_orderable_db_instance.test.engine_version
@@ -2630,7 +2689,7 @@ resource "aws_rds_cluster_instance" "test" {
     ignore_changes = [engine_version]
   }
 }
-`, allowMajorVersionUpgrade, rName, engine, engineVersion)
+`, allowMajorVersionUpgrade, rName, engine, engineVersion, applyImmediately)
 }
 
 func testAccClusterConfig_allowMajorVersionUpgradeCustomParameters(rName string, allowMajorVersionUpgrade bool, engine string, engineVersion string, applyImmediate bool) string {

internal/service/rds/engine_version_test.go

@@ -10,10 +10,17 @@ func TestCompareActualEngineVersion(t *testing.T) {
 	type testCase struct {
 		configuredVersion           string
 		actualVersion               string
+		pendingVersion              string
 		expectedEngineVersion       string
 		expectedEngineVersionActual string
 	}
 	tests := map[string]testCase{
+		"import": {
+			configuredVersion:           "", // no "old" value on import
+			actualVersion:               "8.1",
+			expectedEngineVersion:       "8.1",
+			expectedEngineVersionActual: "8.1",
+		},
 		"point version upgrade": {
 			configuredVersion:           "8.0",
 			actualVersion:               "8.0.27",
@@ -32,6 +39,20 @@ func TestCompareActualEngineVersion(t *testing.T) {
 			expectedEngineVersion:       "9.0.0",
 			expectedEngineVersionActual: "9.0.0",
 		},
+		"pending minor version upgrade": {
+			configuredVersion:           "8.1.1",
+			actualVersion:               "8.0",
+			pendingVersion:              "8.1.1",
+			expectedEngineVersion:       "8.1.1",
+			expectedEngineVersionActual: "8.0",
+		},
+		"pending major version upgrade": {
+			configuredVersion:           "9.0.0",
+			actualVersion:               "8.1",
+			pendingVersion:              "9.0.0",
+			expectedEngineVersion:       "9.0.0",
+			expectedEngineVersionActual: "8.1",
+		},
 		"aurora upgrade": {
 			configuredVersion:           "5.7.mysql_aurora.2.07",
 			actualVersion:               "5.7.serverless_mysql_aurora.2.08.3",
@@ -66,7 +87,7 @@ func TestCompareActualEngineVersion(t *testing.T) {
 			r := ResourceCluster()
 			d := r.Data(nil)
 			d.Set("engine_version", test.configuredVersion)
-			compareActualEngineVersion(d, test.configuredVersion, test.actualVersion)
+			compareActualEngineVersion(d, test.configuredVersion, test.actualVersion, test.pendingVersion)
 
 			if want, got := test.expectedEngineVersion, d.Get("engine_version"); got != want {
 				t.Errorf("unexpected engine_version; want: %q, got: %q", want, got)

internal/service/rds/instance.go

@@ -2339,7 +2339,11 @@ func isStorageTypeGP3BelowAllocatedStorageThreshold(d *schema.ResourceData) bool
 func dbSetResourceDataEngineVersionFromInstance(d *schema.ResourceData, c *rds.DBInstance) {
 	oldVersion := d.Get("engine_version").(string)
 	newVersion := aws.StringValue(c.EngineVersion)
-	compareActualEngineVersion(d, oldVersion, newVersion)
+	var pendingVersion string
+	if c.PendingModifiedValues != nil && c.PendingModifiedValues.EngineVersion != nil {
+		pendingVersion = aws.StringValue(c.PendingModifiedValues.EngineVersion)
+	}
+	compareActualEngineVersion(d, oldVersion, newVersion, pendingVersion)
 }
 
 type dbInstanceARN struct {

internal/service/rds/verify.go

@@ -4,16 +4,30 @@ import (
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 )
 
-func compareActualEngineVersion(d *schema.ResourceData, oldVersion string, newVersion string) {
-	newVersionSubstr := newVersion
+// compareActualEngineVersion sets engine version related attributes
+//
+// `engine_version_actual` is always set to newVersion
+//
+// `engine_version` is set to newVersion unless:
+//   - old and pending versions are equal (ie. the update is awaiting a
+//     maintenance window)
+//   - old and new versions are not exactly equal, but match after accounting
+//     for an omitted patch value in the configuration (ie. old="1.3",
+//     new="1.3.27" will not trigger a set)
+func compareActualEngineVersion(d *schema.ResourceData, oldVersion, newVersion, pendingVersion string) {
+	d.Set("engine_version_actual", newVersion)
+
+	if oldVersion != "" && oldVersion == pendingVersion {
+		return
+	}
 
+	newVersionSubstr := newVersion
 	if len(newVersion) > len(oldVersion) {
 		newVersionSubstr = string([]byte(newVersion)[0 : len(oldVersion)+1])
 	}
-
-	if oldVersion != newVersion && string(append([]byte(oldVersion), []byte(".")...)) != newVersionSubstr {
-		d.Set("engine_version", newVersion)
+	if oldVersion != newVersion && oldVersion+"." == newVersionSubstr {
+		return
 	}
 
-	d.Set("engine_version_actual", newVersion)
+	d.Set("engine_version", newVersion)
 }