Merge pull request #28847 from fatmcgav/fix_28589

Ensure required values are set when changing `storage_type`.
hashicorp · Nov 22, 2024 · 042a38f · 042a38f
2 parents 998cfca + 85a8e92
commit 042a38f
Show file tree

Hide file tree

Showing 4 changed files with 101 additions and 89 deletions.
diff --git a/.changelog/28847.txt b/.changelog/28847.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+resource/aws_db_instance: When changing a `gp3` volume's `allocated_storage` to a value larger than the [threshold value for `engine`](https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/CHAP_Storage.html#gp3-storage), fix bug causing error `InvalidParameterCombination: You must specify both the storage size and iops when modifying the storage size or iops on a DB instance that has iops`
+```
diff --git a/.changelog/37257.txt b/.changelog/37257.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+resource/aws_db_instance: When changing `storage_type` from `io1` or `io2` to `gp3`, fix bug causing error `InvalidParameterCombination: You must specify both the storage size and iops when modifying the storage size or iops on a DB instance that has iops`
+```
diff --git a/internal/service/rds/instance.go b/internal/service/rds/instance.go
@@ -2587,9 +2587,15 @@ func dbInstancePopulateModify(input *rds.ModifyDBInstanceInput, d *schema.Resour
 		needsModify = true
 		input.StorageType = aws.String(d.Get(names.AttrStorageType).(string))
 
-		if slices.Contains([]string{storageTypeIO1, storageTypeIO2}, aws.ToString(input.StorageType)) {
+		// Need to send the iops and allocated_size if migrating to a gp3 volume that's larger than the threshold.
+		if aws.ToString(input.StorageType) == storageTypeGP3 && !isStorageTypeGP3BelowAllocatedStorageThreshold(d) {
+			input.AllocatedStorage = aws.Int32(int32(d.Get(names.AttrAllocatedStorage).(int)))
 			input.Iops = aws.Int32(int32(d.Get(names.AttrIOPS).(int)))
+		}
+
+		if slices.Contains([]string{storageTypeIO1, storageTypeIO2}, aws.ToString(input.StorageType)) {
 			input.AllocatedStorage = aws.Int32(int32(d.Get(names.AttrAllocatedStorage).(int)))
+			input.Iops = aws.Int32(int32(d.Get(names.AttrIOPS).(int)))
 		}
 	}
 

diff --git a/internal/service/rds/instance_test.go b/internal/service/rds/instance_test.go
@@ -3727,79 +3727,6 @@ func TestAccRDSInstance_MonitoringRoleARN_removedToEnabled(t *testing.T) {
 	})
 }
 
-// Regression test for https://github.com/hashicorp/terraform/issues/3760 .
-// We apply a plan, then change just the iops. If the apply succeeds, we
-// consider this a pass, as before in 3760 the request would fail
-func TestAccRDSInstance_Storage_separateIOPSUpdate_Io1(t *testing.T) {
-	ctx := acctest.Context(t)
-	if testing.Short() {
-		t.Skip("skipping long-running test in short mode")
-	}
-
-	var v types.DBInstance
-	resourceName := "aws_db_instance.test"
-	rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
-
-	resource.ParallelTest(t, resource.TestCase{
-		PreCheck:                 func() { acctest.PreCheck(ctx, t) },
-		ErrorCheck:               acctest.ErrorCheck(t, names.RDSServiceID),
-		ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
-		CheckDestroy:             testAccCheckDBInstanceDestroy(ctx),
-		Steps: []resource.TestStep{
-			{
-				Config: testAccInstanceConfig_iopsUpdate(rName, "io1", 1000),
-				Check: resource.ComposeAggregateTestCheckFunc(
-					testAccCheckDBInstanceExists(ctx, resourceName, &v),
-					testAccCheckInstanceAttributes(&v),
-				),
-			},
-
-			{
-				Config: testAccInstanceConfig_iopsUpdate(rName, "io1", 2000),
-				Check: resource.ComposeAggregateTestCheckFunc(
-					testAccCheckDBInstanceExists(ctx, resourceName, &v),
-					testAccCheckInstanceAttributes(&v),
-				),
-			},
-		},
-	})
-}
-
-func TestAccRDSInstance_Storage_separateIOPSUpdate_Io2(t *testing.T) {
-	ctx := acctest.Context(t)
-	if testing.Short() {
-		t.Skip("skipping long-running test in short mode")
-	}
-
-	var v types.DBInstance
-	resourceName := "aws_db_instance.test"
-	rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
-
-	resource.ParallelTest(t, resource.TestCase{
-		PreCheck:                 func() { acctest.PreCheck(ctx, t) },
-		ErrorCheck:               acctest.ErrorCheck(t, names.RDSServiceID),
-		ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
-		CheckDestroy:             testAccCheckDBInstanceDestroy(ctx),
-		Steps: []resource.TestStep{
-			{
-				Config: testAccInstanceConfig_iopsUpdate(rName, "io2", 1000),
-				Check: resource.ComposeAggregateTestCheckFunc(
-					testAccCheckDBInstanceExists(ctx, resourceName, &v),
-					testAccCheckInstanceAttributes(&v),
-				),
-			},
-
-			{
-				Config: testAccInstanceConfig_iopsUpdate(rName, "io2", 2000),
-				Check: resource.ComposeAggregateTestCheckFunc(
-					testAccCheckDBInstanceExists(ctx, resourceName, &v),
-					testAccCheckInstanceAttributes(&v),
-				),
-			},
-		},
-	})
-}
-
 func TestAccRDSInstance_portUpdate(t *testing.T) {
 	ctx := acctest.Context(t)
 	if testing.Short() {
@@ -6126,7 +6053,7 @@ func TestAccRDSInstance_Storage_changeThroughput(t *testing.T) {
 		CheckDestroy:             testAccCheckDBInstanceDestroy(ctx),
 		Steps: []resource.TestStep{
 			{
-				Config: testAccInstanceConfig_Storage_throughput(rName, 12000, 500),
+				Config: testAccInstanceConfig_Storage_iopsThroughputMySQLGP3(rName, 12000, 500),
 				Check: resource.ComposeAggregateTestCheckFunc(
 					testAccCheckDBInstanceExists(ctx, resourceName, &v),
 					resource.TestCheckResourceAttr(resourceName, names.AttrIOPS, "12000"),
@@ -6135,7 +6062,7 @@ func TestAccRDSInstance_Storage_changeThroughput(t *testing.T) {
 				),
 			},
 			{
-				Config: testAccInstanceConfig_Storage_throughput(rName, 12000, 600),
+				Config: testAccInstanceConfig_Storage_iopsThroughputMySQLGP3(rName, 12000, 600),
 				Check: resource.ComposeAggregateTestCheckFunc(
 					testAccCheckDBInstanceExists(ctx, resourceName, &v),
 					resource.TestCheckResourceAttr(resourceName, names.AttrIOPS, "12000"),
@@ -6165,7 +6092,7 @@ func TestAccRDSInstance_Storage_changeIOPSThroughput(t *testing.T) {
 		CheckDestroy:             testAccCheckDBInstanceDestroy(ctx),
 		Steps: []resource.TestStep{
 			{
-				Config: testAccInstanceConfig_Storage_throughput(rName, 12000, 500),
+				Config: testAccInstanceConfig_Storage_iopsThroughputMySQLGP3(rName, 12000, 500),
 				Check: resource.ComposeAggregateTestCheckFunc(
 					testAccCheckDBInstanceExists(ctx, resourceName, &v),
 					resource.TestCheckResourceAttr(resourceName, names.AttrIOPS, "12000"),
@@ -6174,7 +6101,7 @@ func TestAccRDSInstance_Storage_changeIOPSThroughput(t *testing.T) {
 				),
 			},
 			{
-				Config: testAccInstanceConfig_Storage_throughput(rName, 13000, 600),
+				Config: testAccInstanceConfig_Storage_iopsThroughputMySQLGP3(rName, 13000, 600),
 				Check: resource.ComposeAggregateTestCheckFunc(
 					testAccCheckDBInstanceExists(ctx, resourceName, &v),
 					resource.TestCheckResourceAttr(resourceName, names.AttrIOPS, "13000"),
@@ -6187,7 +6114,7 @@ func TestAccRDSInstance_Storage_changeIOPSThroughput(t *testing.T) {
 }
 
 // https://github.com/hashicorp/terraform-provider-aws/issues/33512
-func TestAccRDSInstance_Storage_changeIOPS(t *testing.T) {
+func TestAccRDSInstance_Storage_changeIOPSGP3(t *testing.T) {
 	ctx := acctest.Context(t)
 	if testing.Short() {
 		t.Skip("skipping long-running test in short mode")
@@ -6204,7 +6131,7 @@ func TestAccRDSInstance_Storage_changeIOPS(t *testing.T) {
 		CheckDestroy:             testAccCheckDBInstanceDestroy(ctx),
 		Steps: []resource.TestStep{
 			{
-				Config: testAccInstanceConfig_Storage_throughput(rName, 12000, 500),
+				Config: testAccInstanceConfig_Storage_iopsThroughputMySQLGP3(rName, 12000, 500),
 				Check: resource.ComposeAggregateTestCheckFunc(
 					testAccCheckDBInstanceExists(ctx, resourceName, &v),
 					resource.TestCheckResourceAttr(resourceName, names.AttrIOPS, "12000"),
@@ -6213,10 +6140,10 @@ func TestAccRDSInstance_Storage_changeIOPS(t *testing.T) {
 				),
 			},
 			{
-				Config: testAccInstanceConfig_Storage_throughput(rName, 13000, 500),
+				Config: testAccInstanceConfig_Storage_iopsThroughputMySQLGP3(rName, 14000, 500),
 				Check: resource.ComposeAggregateTestCheckFunc(
 					testAccCheckDBInstanceExists(ctx, resourceName, &v),
-					resource.TestCheckResourceAttr(resourceName, names.AttrIOPS, "13000"),
+					resource.TestCheckResourceAttr(resourceName, names.AttrIOPS, "14000"),
 					resource.TestCheckResourceAttr(resourceName, "storage_throughput", "500"),
 					resource.TestCheckResourceAttr(resourceName, names.AttrStorageType, "gp3"),
 				),
@@ -6264,7 +6191,7 @@ func TestAccRDSInstance_Storage_throughputSSE(t *testing.T) {
 	})
 }
 
-func TestAccRDSInstance_Storage_typePostgres(t *testing.T) {
+func TestAccRDSInstance_Storage_postgres(t *testing.T) {
 	ctx := acctest.Context(t)
 	if testing.Short() {
 		t.Skip("skipping long-running test in short mode")
@@ -6281,7 +6208,7 @@ func TestAccRDSInstance_Storage_typePostgres(t *testing.T) {
 		CheckDestroy:             testAccCheckDBInstanceDestroy(ctx),
 		Steps: []resource.TestStep{
 			{
-				Config: testAccInstanceConfig_Storage_typePostgres(rName, "gp2", 200),
+				Config: testAccInstanceConfig_Storage_postgres(rName, "gp2", 200),
 				Check: resource.ComposeAggregateTestCheckFunc(
 					testAccCheckDBInstanceExists(ctx, resourceName, &v),
 					resource.TestCheckResourceAttr(resourceName, names.AttrAllocatedStorage, "200"),
@@ -6304,7 +6231,7 @@ func TestAccRDSInstance_Storage_typePostgres(t *testing.T) {
 				},
 			},
 			{
-				Config: testAccInstanceConfig_Storage_typePostgres(rName, "gp3", 300),
+				Config: testAccInstanceConfig_Storage_postgres(rName, "gp3", 300),
 				Check: resource.ComposeAggregateTestCheckFunc(
 					testAccCheckDBInstanceExists(ctx, resourceName, &v),
 					resource.TestCheckResourceAttr(resourceName, names.AttrAllocatedStorage, "300"),
@@ -6317,7 +6244,80 @@ func TestAccRDSInstance_Storage_typePostgres(t *testing.T) {
 	})
 }
 
-func TestAccRDSInstance_newIdentifier_Pending(t *testing.T) {
+// Regression test for https://github.com/hashicorp/terraform/issues/3760 .
+// We apply a plan, then change just the iops. If the apply succeeds, we
+// consider this a pass, as before in 3760 the request would fail
+func TestAccRDSInstance_Storage_changeIOPSio1(t *testing.T) {
+	ctx := acctest.Context(t)
+	if testing.Short() {
+		t.Skip("skipping long-running test in short mode")
+	}
+
+	var v types.DBInstance
+	resourceName := "aws_db_instance.test"
+	rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
+
+	resource.ParallelTest(t, resource.TestCase{
+		PreCheck:                 func() { acctest.PreCheck(ctx, t) },
+		ErrorCheck:               acctest.ErrorCheck(t, names.RDSServiceID),
+		ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
+		CheckDestroy:             testAccCheckDBInstanceDestroy(ctx),
+		Steps: []resource.TestStep{
+			{
+				Config: testAccInstanceConfig_iopsUpdate(rName, "io1", 1000),
+				Check: resource.ComposeAggregateTestCheckFunc(
+					testAccCheckDBInstanceExists(ctx, resourceName, &v),
+					testAccCheckInstanceAttributes(&v),
+				),
+			},
+
+			{
+				Config: testAccInstanceConfig_iopsUpdate(rName, "io1", 2000),
+				Check: resource.ComposeAggregateTestCheckFunc(
+					testAccCheckDBInstanceExists(ctx, resourceName, &v),
+					testAccCheckInstanceAttributes(&v),
+				),
+			},
+		},
+	})
+}
+
+func TestAccRDSInstance_Storage_changeIOPSio2(t *testing.T) {
+	ctx := acctest.Context(t)
+	if testing.Short() {
+		t.Skip("skipping long-running test in short mode")
+	}
+
+	var v types.DBInstance
+	resourceName := "aws_db_instance.test"
+	rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
+
+	resource.ParallelTest(t, resource.TestCase{
+		PreCheck:                 func() { acctest.PreCheck(ctx, t) },
+		ErrorCheck:               acctest.ErrorCheck(t, names.RDSServiceID),
+		ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
+		CheckDestroy:             testAccCheckDBInstanceDestroy(ctx),
+		Steps: []resource.TestStep{
+			{
+				Config: testAccInstanceConfig_iopsUpdate(rName, "io2", 1000),
+				Check: resource.ComposeAggregateTestCheckFunc(
+					testAccCheckDBInstanceExists(ctx, resourceName, &v),
+					testAccCheckInstanceAttributes(&v),
+				),
+			},
+
+			{
+				Config: testAccInstanceConfig_iopsUpdate(rName, "io2", 2000),
+				Check: resource.ComposeAggregateTestCheckFunc(
+					testAccCheckDBInstanceExists(ctx, resourceName, &v),
+					testAccCheckInstanceAttributes(&v),
+				),
+			},
+		},
+	})
+}
+
+func TestAccRDSInstance_NewIdentifier_pending(t *testing.T) {
 	ctx := acctest.Context(t)
 	if testing.Short() {
 		t.Skip("skipping long-running test in short mode")
@@ -6363,7 +6363,7 @@ func TestAccRDSInstance_newIdentifier_Pending(t *testing.T) {
 	})
 }
 
-func TestAccRDSInstance_newIdentifier_Immediately(t *testing.T) {
+func TestAccRDSInstance_NewIdentifier_immediately(t *testing.T) {
 	ctx := acctest.Context(t)
 	if testing.Short() {
 		t.Skip("skipping long-running test in short mode")
@@ -12480,7 +12480,7 @@ resource "aws_db_instance" "test" {
 `, rName, tfrds.InstanceEngineSQLServerExpress, allocatedStorage))
 }
 
-func testAccInstanceConfig_Storage_throughput(rName string, iops, throughput int) string {
+func testAccInstanceConfig_Storage_iopsThroughputMySQLGP3(rName string, iops, throughput int) string {
 	return acctest.ConfigCompose(
 		testAccInstanceConfig_orderableClassMySQLGP3(),
 		fmt.Sprintf(`
@@ -12540,7 +12540,7 @@ resource "aws_db_instance" "test" {
 `, tfrds.InstanceEngineSQLServerStandard, mainInstanceClasses, rName, iops, throughput)
 }
 
-func testAccInstanceConfig_Storage_typePostgres(rName string, storageType string, allocatedStorage int) string {
+func testAccInstanceConfig_Storage_postgres(rName string, storageType string, allocatedStorage int) string {
 	return fmt.Sprintf(`
 data "aws_rds_engine_version" "default" {
   engine = %[1]q