Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure required values are set when changing storage_type. #28847

Merged
merged 15 commits into from
Nov 22, 2024
Merged

Ensure required values are set when changing storage_type. #28847

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
eb95c0a
Ensure required values are set when changing `storage_type`.
fatmcgav Jan 12, 2023
8d4edab
Merge main
YakDriver Nov 22, 2024
597e802
Fixes after merge, sdkv2
YakDriver Nov 22, 2024
d9491e3
Update tests after merge, new ways
YakDriver Nov 22, 2024
0aa73d9
Add changelog
YakDriver Nov 22, 2024
fd9e4bb
Add retroactive changelog
YakDriver Nov 22, 2024
6d466f5
Fix misplacing of logic during merge
YakDriver Nov 22, 2024
af22811
Clean up tests after merge
YakDriver Nov 22, 2024
b3fb095
Remove git merge artifact
YakDriver Nov 22, 2024
a223c87
Fix merge artifact
YakDriver Nov 22, 2024
c312e22
Remove unused
YakDriver Nov 22, 2024
2d10619
Fix naming
YakDriver Nov 22, 2024
8348019
MySQL/Postgres don't support allocated storage was trying
YakDriver Nov 22, 2024
95e2ed1
Update .changelog/28847.txt
YakDriver Nov 22, 2024
85a8e92
Update .changelog/37257.txt
YakDriver Nov 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .changelog/28847.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
resource/aws_db_instance: When changing to gp3 larger than allocated storage threshold, 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`
YakDriver marked this conversation as resolved.
Show resolved Hide resolved
```
3 changes: 3 additions & 0 deletions .changelog/37257.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot on a previous PR.

resource/aws_db_instance: When changing from io1/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`
YakDriver marked this conversation as resolved.
Show resolved Hide resolved
```
8 changes: 7 additions & 1 deletion internal/service/rds/instance.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)))
}
}

Expand Down
176 changes: 88 additions & 88 deletions internal/service/rds/instance_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3727,79 +3727,6 @@ func TestAccRDSInstance_MonitoringRoleARN_removedToEnabled(t *testing.T) {
})
}

// Regression test for https://github.com/hashicorp/terraform/issues/3760 .
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are just moved to be near the other storage-related tests

// 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() {
Expand Down Expand Up @@ -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"),
Expand All @@ -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"),
Expand Down Expand Up @@ -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"),
Expand All @@ -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"),
Expand All @@ -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")
Expand All @@ -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"),
Expand All @@ -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"),
),
Expand Down Expand Up @@ -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")
Expand All @@ -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"),
Expand All @@ -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"),
Expand All @@ -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")
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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(`
Expand Down Expand Up @@ -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
Expand Down
Loading