diff --git a/.changelog/30247.txt b/.changelog/30247.txt new file mode 100644 index 00000000000..436360c49ba --- /dev/null +++ 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 +``` + diff --git a/internal/service/rds/cluster.go b/internal/service/rds/cluster.go index 6897ec9f744..54b2d0d815d 100644 --- a/internal/service/rds/cluster.go +++ b/internal/service/rds/cluster.go @@ -1149,8 +1149,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") { @@ -1180,6 +1183,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)) } @@ -1464,7 +1474,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) { diff --git a/internal/service/rds/cluster_instance.go b/internal/service/rds/cluster_instance.go index 420f709a363..c9dbee5ff8c 100644 --- a/internal/service/rds/cluster_instance.go +++ b/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) } diff --git a/internal/service/rds/cluster_test.go b/internal/service/rds/cluster_test.go index 80af74131bf..66c6da78321 100644 --- a/internal/service/rds/cluster_test.go +++ b/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"), @@ -2370,11 +2428,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 @@ -2391,6 +2449,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 @@ -2401,7 +2460,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 { diff --git a/internal/service/rds/engine_version_test.go b/internal/service/rds/engine_version_test.go index 492fa68e88d..beed7faa38b 100644 --- a/internal/service/rds/engine_version_test.go +++ b/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) diff --git a/internal/service/rds/instance.go b/internal/service/rds/instance.go index c62ece13b48..dca07cccc61 100644 --- a/internal/service/rds/instance.go +++ b/internal/service/rds/instance.go @@ -2247,7 +2247,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 { diff --git a/internal/service/rds/verify.go b/internal/service/rds/verify.go index c7df67724dc..e98f2dbc296 100644 --- a/internal/service/rds/verify.go +++ b/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) }