Skip to content

Commit

Permalink
b/aws_redshiftserverless_workgroup: Fix base_capacity, max_capacity o…
Browse files Browse the repository at this point in the history 
…rdered updates and max_capacity limit removal
  • Loading branch information
Marcin Belczewski committed Feb 29, 2024
1 parent 31abe30 commit e024709
Show file tree
Hide file tree
Showing 3 changed files with 122 additions and 28 deletions.
3 changes: 3 additions & 0 deletions .changelog/36032.txt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
resource/aws_redshiftserverless_workgroup: Fix base_capacity, max_capacity ordered updates and max_capacity limit removal
```
86 changes: 66 additions & 20 deletions internal/service/redshiftserverless/workgroup.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/redshiftserverless"
"github.com/google/go-cmp/cmp"
"github.com/hashicorp/aws-sdk-go-base/v2/awsv1shim/v2/tfawserr"
"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/retry"
Expand Down Expand Up @@ -157,7 +158,6 @@ func ResourceWorkgroup() *schema.Resource {
"max_capacity": {
Type: schema.TypeInt,
Optional: true,
Computed: true,
},
"namespace_name": {
Type: schema.TypeString,
Expand Down Expand Up @@ -306,15 +306,72 @@ func resourceWorkgroupUpdate(ctx context.Context, d *schema.ResourceData, meta i
var diags diag.Diagnostics
conn := meta.(*conns.AWSClient).RedshiftServerlessConn(ctx)

// You can't update multiple parameters in one request.

if d.HasChange("base_capacity") {
input := &redshiftserverless.UpdateWorkgroupInput{
BaseCapacity: aws.Int64(int64(d.Get("base_capacity").(int))),
WorkgroupName: aws.String(d.Id()),
checkCapacityChange := func(key string) (hasCapacityChange bool, oldCapacity int, newCapacity int) {
o, n := d.GetChange(key)
oldCapacity, newCapacity = o.(int), n.(int)
hasCapacityChange = !cmp.Equal(newCapacity, oldCapacity)
return
}

// You can't update multiple workgroup parameters in one request.
// This is particularly important when adjusting base_capacity and max_capacity due to their interdependencies:
// - base_capacity cannot be increased to a value greater than the current max_capacity.
// - max_capacity cannot be decreased to a value smaller than the current base_capacity.
// The value 0 of max_capacity in the state signifies "not set".
// Sending max_capacity value of -1 to AWS API removes max_capacity limit, but -1 cannot be used as max_capacity in the state,
// because AWS API never returns -1 as the value of unset max_capacity. There would be a diff on subsequent apply,
// resulting in errors due to the lack of AWS API idempotency.
// Some validations, such as increasing base_capacity beyond an unchanged max_capacity, are deferred to the AWS API.

hasBaseCapacityChange, _, newBaseCapacity := checkCapacityChange("base_capacity")
hasMaxCapacityChange, oldMaxCapacity, newMaxCapacity := checkCapacityChange("max_capacity")

switch {
case hasMaxCapacityChange && newMaxCapacity == 0:
if err :=
updateWorkgroup(ctx, conn,
&redshiftserverless.UpdateWorkgroupInput{MaxCapacity: aws.Int64(-1), WorkgroupName: aws.String(d.Id())},
d.Timeout(schema.TimeoutUpdate)); err != nil {
return sdkdiag.AppendFromErr(diags, err)
}

if err := updateWorkgroup(ctx, conn, input, d.Timeout(schema.TimeoutUpdate)); err != nil {
case hasBaseCapacityChange && hasMaxCapacityChange && (oldMaxCapacity == 0 || newBaseCapacity <= oldMaxCapacity):
if err :=
updateWorkgroup(ctx, conn,
&redshiftserverless.UpdateWorkgroupInput{BaseCapacity: aws.Int64(int64(newBaseCapacity)), WorkgroupName: aws.String(d.Id())},
d.Timeout(schema.TimeoutUpdate)); err != nil {
return sdkdiag.AppendFromErr(diags, err)
}
if err :=
updateWorkgroup(ctx, conn,
&redshiftserverless.UpdateWorkgroupInput{MaxCapacity: aws.Int64(int64(newMaxCapacity)), WorkgroupName: aws.String(d.Id())},
d.Timeout(schema.TimeoutUpdate)); err != nil {
return sdkdiag.AppendFromErr(diags, err)
}
case hasBaseCapacityChange && hasMaxCapacityChange && newBaseCapacity > oldMaxCapacity:
if err :=
updateWorkgroup(ctx, conn,
&redshiftserverless.UpdateWorkgroupInput{MaxCapacity: aws.Int64(int64(newMaxCapacity)), WorkgroupName: aws.String(d.Id())},
d.Timeout(schema.TimeoutUpdate)); err != nil {
return sdkdiag.AppendFromErr(diags, err)
}
if err :=
updateWorkgroup(ctx, conn,
&redshiftserverless.UpdateWorkgroupInput{BaseCapacity: aws.Int64(int64(newBaseCapacity)), WorkgroupName: aws.String(d.Id())},
d.Timeout(schema.TimeoutUpdate)); err != nil {
return sdkdiag.AppendFromErr(diags, err)
}
case hasBaseCapacityChange:
if err :=
updateWorkgroup(ctx, conn,
&redshiftserverless.UpdateWorkgroupInput{BaseCapacity: aws.Int64(int64(newBaseCapacity)), WorkgroupName: aws.String(d.Id())},
d.Timeout(schema.TimeoutUpdate)); err != nil {
return sdkdiag.AppendFromErr(diags, err)
}
case hasMaxCapacityChange:
if err :=
updateWorkgroup(ctx, conn,
&redshiftserverless.UpdateWorkgroupInput{MaxCapacity: aws.Int64(int64(newMaxCapacity)), WorkgroupName: aws.String(d.Id())},
d.Timeout(schema.TimeoutUpdate)); err != nil {
return sdkdiag.AppendFromErr(diags, err)
}
}
Expand All @@ -341,17 +398,6 @@ func resourceWorkgroupUpdate(ctx context.Context, d *schema.ResourceData, meta i
}
}

if d.HasChange("max_capacity") {
input := &redshiftserverless.UpdateWorkgroupInput{
MaxCapacity: aws.Int64(int64(d.Get("max_capacity").(int))),
WorkgroupName: aws.String(d.Id()),
}

if err := updateWorkgroup(ctx, conn, input, d.Timeout(schema.TimeoutUpdate)); err != nil {
return sdkdiag.AppendFromErr(diags, err)
}
}

if d.HasChange("port") {
input := &redshiftserverless.UpdateWorkgroupInput{
Port: aws.Int64(int64(d.Get("port").(int))),
Expand Down
61 changes: 53 additions & 8 deletions internal/service/redshiftserverless/workgroup_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ func TestAccRedshiftServerlessWorkgroup_basic(t *testing.T) {
})
}

// Tests the complex logic involved in updating 'base_capacity' and 'max_capacity'.
// The order of updates is crucial and is determined by their current state values.
func TestAccRedshiftServerlessWorkgroup_baseAndMaxCapacityAndPubliclyAccessible(t *testing.T) {
ctx := acctest.Context(t)
resourceName := "aws_redshiftserverless_workgroup.test"
Expand All @@ -62,28 +64,57 @@ func TestAccRedshiftServerlessWorkgroup_baseAndMaxCapacityAndPubliclyAccessible(
CheckDestroy: testAccCheckWorkgroupDestroy(ctx),
Steps: []resource.TestStep{
{
Config: testAccWorkgroupConfig_baseAndMaxCapacityAndPubliclyAccessible(rName, 64, 128, true),
Config: testAccWorkgroupConfig_baseCapacity(rName, 128),
Check: resource.ComposeTestCheckFunc(
testAccCheckWorkgroupExists(ctx, resourceName),
resource.TestCheckResourceAttr(resourceName, "base_capacity", "128"),
resource.TestCheckResourceAttr(resourceName, "max_capacity", "0"),
),
},
{
Config: testAccWorkgroupConfig_baseCapacity(rName, 256),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr(resourceName, "base_capacity", "256"),
resource.TestCheckResourceAttr(resourceName, "max_capacity", "0"),
),
},
{
Config: testAccWorkgroupConfig_baseAndMaxCapacityAndPubliclyAccessible(rName, 64, 128, false),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr(resourceName, "base_capacity", "64"),
resource.TestCheckResourceAttr(resourceName, "max_capacity", "128"),
resource.TestCheckResourceAttr(resourceName, "publicly_accessible", "true"),
),
},
{
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
Config: testAccWorkgroupConfig_baseAndMaxCapacityAndPubliclyAccessible(rName, 128, 256, false),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr(resourceName, "base_capacity", "128"),
resource.TestCheckResourceAttr(resourceName, "max_capacity", "256"),
),
},
{
Config: testAccWorkgroupConfig_baseAndMaxCapacityAndPubliclyAccessible(rName, 128, 5632, false),
Config: testAccWorkgroupConfig_baseAndMaxCapacityAndPubliclyAccessible(rName, 512, 5632, false),
Check: resource.ComposeTestCheckFunc(
testAccCheckWorkgroupExists(ctx, resourceName),
resource.TestCheckResourceAttr(resourceName, "base_capacity", "128"),
resource.TestCheckResourceAttr(resourceName, "base_capacity", "512"),
resource.TestCheckResourceAttr(resourceName, "max_capacity", "5632"),
resource.TestCheckResourceAttr(resourceName, "publicly_accessible", "false"),
),
},
{
Config: testAccWorkgroupConfig_baseAndMaxCapacityAndPubliclyAccessible(rName, 128, 256, true),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr(resourceName, "base_capacity", "128"),
resource.TestCheckResourceAttr(resourceName, "max_capacity", "256"),
resource.TestCheckResourceAttr(resourceName, "publicly_accessible", "true"),
),
},
{
Config: testAccWorkgroupConfig_baseCapacity(rName, 128),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr(resourceName, "base_capacity", "128"),
resource.TestCheckResourceAttr(resourceName, "max_capacity", "0"),
),
},
},
})
}
Expand Down Expand Up @@ -361,6 +392,20 @@ resource "aws_redshiftserverless_workgroup" "test" {
`, rName, baseCapacity, maxCapacity, publiclyAccessible)
}

func testAccWorkgroupConfig_baseCapacity(rName string, baseCapacity int) string {
return fmt.Sprintf(`
resource "aws_redshiftserverless_namespace" "test" {
namespace_name = %[1]q
}
resource "aws_redshiftserverless_workgroup" "test" {
namespace_name = aws_redshiftserverless_namespace.test.namespace_name
workgroup_name = %[1]q
base_capacity = %[2]d
}
`, rName, baseCapacity)
}

func testAccWorkgroupConfig_configParameters(rName, maxQueryExecutionTime string) string {
return fmt.Sprintf(`
resource "aws_redshiftserverless_namespace" "test" {
Expand Down

0 comments on commit e024709

Please sign in to comment.