Skip to content

Commit

Permalink
Merge pull request #37991 from hashicorp/b-dynamo-ttl-disabled
Browse files Browse the repository at this point in the history 
resource/aws_dynamodb_table: Fixes perpetual diff when `ttl.attribute_name` is set when not enabled
  • Loading branch information
@gdavison
gdavison authored Jun 17, 2024
2 parents 8b7cf75 + f6a9d33 commit 46774b5
Show file tree
Hide file tree
Showing 4 changed files with 226 additions and 17 deletions.
7 changes: 7 additions & 0 deletions .changelog/37991.txt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
```release-note:bug
resource/aws_dynamodb_table: Fixes perpetual diff when `ttl.attribute_name` is set when `ttl.enabled` is not set.
```

```release-note:enhancement
resource/aws_dynamodb_table: Adds validation for `ttl` values.
```
65 changes: 64 additions & 1 deletion internal/service/dynamodb/table.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"github.com/aws/aws-sdk-go-v2/service/dynamodb"
awstypes "github.com/aws/aws-sdk-go-v2/service/dynamodb/types"
"github.com/hashicorp/aws-sdk-go-base/v2/tfawserr"
"github.com/hashicorp/go-cty/cty"
"github.com/hashicorp/go-multierror"
"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/customdiff"
Expand Down Expand Up @@ -116,6 +117,7 @@ func resourceTable() *schema.Resource {
// https://github.com/hashicorp/terraform-provider-aws/issues/25214
return old.(string) != new.(string) && new.(string) != ""
}),
validateTTLCustomDiff,
verify.SetTagsDiff,
),

Expand Down Expand Up @@ -450,7 +452,7 @@ func resourceTable() *schema.Resource {
Schema: map[string]*schema.Schema{
"attribute_name": {
Type: schema.TypeString,
Required: true,
Optional: true,
},
names.AttrEnabled: {
Type: schema.TypeBool,
Expand Down Expand Up @@ -2389,3 +2391,64 @@ func validateProvisionedThroughputField(diff *schema.ResourceDiff, key string) e
}
return nil
}

func validateTTLCustomDiff(ctx context.Context, d *schema.ResourceDiff, meta any) error {
var diags diag.Diagnostics

configRaw := d.GetRawConfig()
if !configRaw.IsKnown() || configRaw.IsNull() {
return nil
}

ttlPath := cty.GetAttrPath("ttl")
ttl := configRaw.GetAttr("ttl")
if ttl.IsKnown() && !ttl.IsNull() {
if ttl.LengthInt() == 1 {
idx := cty.NumberIntVal(0)
ttl := ttl.Index(idx)
ttlPath := ttlPath.Index(idx)
ttlPlantimeValidate(ttlPath, ttl, &diags)
}
}

return sdkdiag.DiagnosticsError(diags)
}

func ttlPlantimeValidate(ttlPath cty.Path, ttl cty.Value, diags *diag.Diagnostics) {
attribute := ttl.GetAttr("attribute_name")
if !attribute.IsKnown() {
return
}

enabled := ttl.GetAttr(names.AttrEnabled)
if !enabled.IsKnown() {
return
}
if enabled.IsNull() {
return
}

if enabled.True() {
if attribute.IsNull() {
*diags = append(*diags, errs.NewAttributeRequiredWhenError(
ttlPath.GetAttr("attribute_name"),
ttlPath.GetAttr(names.AttrEnabled),
"true",
))
} else if attribute.AsString() == "" {
*diags = append(*diags, errs.NewInvalidValueAttributeErrorf(
ttlPath.GetAttr("attribute_name"),
"Attribute %q cannot have an empty value",
errs.PathString(ttlPath.GetAttr("attribute_name")),
))
}
} else {
if !(attribute.IsNull() || attribute.AsString() == "") {
*diags = append(*diags, errs.NewAttributeConflictsWhenError(
ttlPath.GetAttr("attribute_name"),
ttlPath.GetAttr(names.AttrEnabled),
"false",
))
}
}
}
163 changes: 150 additions & 13 deletions internal/service/dynamodb/table_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@ import (
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
sdkacctest "github.com/hashicorp/terraform-plugin-testing/helper/acctest"
"github.com/hashicorp/terraform-plugin-testing/helper/resource"
"github.com/hashicorp/terraform-plugin-testing/knownvalue"
"github.com/hashicorp/terraform-plugin-testing/statecheck"
"github.com/hashicorp/terraform-plugin-testing/terraform"
"github.com/hashicorp/terraform-plugin-testing/tfjsonpath"
"github.com/hashicorp/terraform-provider-aws/internal/acctest"
"github.com/hashicorp/terraform-provider-aws/internal/conns"
"github.com/hashicorp/terraform-provider-aws/internal/create"
Expand Down Expand Up @@ -372,10 +375,16 @@ func TestAccDynamoDBTable_basic(t *testing.T) {
resource.TestCheckResourceAttr(resourceName, "table_class", "STANDARD"),
resource.TestCheckResourceAttr(resourceName, acctest.CtTagsPercent, acctest.Ct0),
resource.TestCheckResourceAttr(resourceName, acctest.CtTagsAllPercent, acctest.Ct0),
resource.TestCheckResourceAttr(resourceName, "ttl.#", acctest.Ct1),
resource.TestCheckResourceAttr(resourceName, "ttl.0.enabled", acctest.CtFalse),
resource.TestCheckResourceAttr(resourceName, "write_capacity", acctest.Ct1),
),
ConfigStateChecks: []statecheck.StateCheck{
statecheck.ExpectKnownValue(resourceName, tfjsonpath.New("ttl"), knownvalue.ListExact([]knownvalue.Check{
knownvalue.ObjectExact(map[string]knownvalue.Check{
"attribute_name": knownvalue.StringExact(""),
names.AttrEnabled: knownvalue.Bool(false),
}),
})),
},
},
{
ResourceName: resourceName,
Expand Down Expand Up @@ -1362,18 +1371,29 @@ func TestAccDynamoDBTable_TTL_enabled(t *testing.T) {
CheckDestroy: testAccCheckTableDestroy(ctx),
Steps: []resource.TestStep{
{
Config: testAccTableConfig_timeToLive(rName, true),
Config: testAccTableConfig_timeToLive(rName, rName, true),
Check: resource.ComposeAggregateTestCheckFunc(
testAccCheckInitialTableExists(ctx, resourceName, &table),
resource.TestCheckResourceAttr(resourceName, "ttl.#", acctest.Ct1),
resource.TestCheckResourceAttr(resourceName, "ttl.0.enabled", acctest.CtTrue),
),
ConfigStateChecks: []statecheck.StateCheck{
statecheck.ExpectKnownValue(resourceName, tfjsonpath.New("ttl"), knownvalue.ListExact([]knownvalue.Check{
knownvalue.ObjectExact(map[string]knownvalue.Check{
"attribute_name": knownvalue.StringExact(rName),
names.AttrEnabled: knownvalue.Bool(true),
}),
})),
},
},
{
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
},
{
Config: testAccTableConfig_timeToLive_unset(rName),
PlanOnly: true,
ExpectNonEmptyPlan: false,
},
},
})
}
Expand All @@ -1393,25 +1413,105 @@ func TestAccDynamoDBTable_TTL_disabled(t *testing.T) {
CheckDestroy: testAccCheckTableDestroy(ctx),
Steps: []resource.TestStep{
{
Config: testAccTableConfig_timeToLive(rName, false),
Config: testAccTableConfig_timeToLive(rName, "", false),
Check: resource.ComposeAggregateTestCheckFunc(
testAccCheckInitialTableExists(ctx, resourceName, &table),
),
ConfigStateChecks: []statecheck.StateCheck{
statecheck.ExpectKnownValue(resourceName, tfjsonpath.New("ttl"), knownvalue.ListExact([]knownvalue.Check{
knownvalue.ObjectExact(map[string]knownvalue.Check{
"attribute_name": knownvalue.StringExact(""),
names.AttrEnabled: knownvalue.Bool(false),
}),
})),
},
},
{
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
},
{
Config: testAccTableConfig_timeToLive_unset(rName),
PlanOnly: true,
ExpectNonEmptyPlan: false,
},
},
})
}

// TTL tests must be split since it can only be updated once per hour
// ValidationException: Time to live has been modified multiple times within a fixed interval
func TestAccDynamoDBTable_TTL_update(t *testing.T) {
ctx := acctest.Context(t)
var table awstypes.TableDescription
resourceName := "aws_dynamodb_table.test"
rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { acctest.PreCheck(ctx, t) },
ErrorCheck: acctest.ErrorCheck(t, names.DynamoDBServiceID),
ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
CheckDestroy: testAccCheckTableDestroy(ctx),
Steps: []resource.TestStep{
{
Config: testAccTableConfig_timeToLive(rName, "", false),
Check: resource.ComposeAggregateTestCheckFunc(
testAccCheckInitialTableExists(ctx, resourceName, &table),
resource.TestCheckResourceAttr(resourceName, "ttl.#", acctest.Ct1),
resource.TestCheckResourceAttr(resourceName, "ttl.0.enabled", acctest.CtFalse),
),
ConfigStateChecks: []statecheck.StateCheck{
statecheck.ExpectKnownValue(resourceName, tfjsonpath.New("ttl"), knownvalue.ListExact([]knownvalue.Check{
knownvalue.ObjectExact(map[string]knownvalue.Check{
"attribute_name": knownvalue.StringExact(""),
names.AttrEnabled: knownvalue.Bool(false),
}),
})),
},
},
{
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
},
{
Config: testAccTableConfig_timeToLive(rName, true),
Config: testAccTableConfig_timeToLive(rName, rName, true),
Check: resource.ComposeAggregateTestCheckFunc(
testAccCheckInitialTableExists(ctx, resourceName, &table),
resource.TestCheckResourceAttr(resourceName, "ttl.#", acctest.Ct1),
resource.TestCheckResourceAttr(resourceName, "ttl.0.enabled", acctest.CtTrue),
),
ConfigStateChecks: []statecheck.StateCheck{
statecheck.ExpectKnownValue(resourceName, tfjsonpath.New("ttl"), knownvalue.ListExact([]knownvalue.Check{
knownvalue.ObjectExact(map[string]knownvalue.Check{
"attribute_name": knownvalue.StringExact(rName),
names.AttrEnabled: knownvalue.Bool(true),
}),
})),
},
},
},
})
}

func TestAccDynamoDBTable_TTL_validate(t *testing.T) {
ctx := acctest.Context(t)
rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { acctest.PreCheck(ctx, t) },
ErrorCheck: acctest.ErrorCheck(t, names.DynamoDBServiceID),
ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
CheckDestroy: testAccCheckTableDestroy(ctx),
Steps: []resource.TestStep{
{
Config: testAccTableConfig_timeToLive(rName, "TestTTL", false),
ExpectError: regexache.MustCompile(regexp.QuoteMeta(`Attribute "ttl[0].attribute_name" cannot be specified when "ttl[0].enabled" is "false"`)),
},
{
Config: testAccTableConfig_timeToLive(rName, "", true),
ExpectError: regexache.MustCompile(regexp.QuoteMeta(`Attribute "ttl[0].attribute_name" cannot have an empty value`)),
},
{
Config: testAccTableConfig_TTL_missingAttributeName(rName, true),
ExpectError: regexache.MustCompile(regexp.QuoteMeta(`Attribute "ttl[0].attribute_name" cannot have an empty value`)),
},
},
})
Expand Down Expand Up @@ -3496,7 +3596,44 @@ resource "aws_dynamodb_table" "test" {
`, rName)
}

func testAccTableConfig_timeToLive(rName string, ttlEnabled bool) string {
func testAccTableConfig_timeToLive(rName, ttlAttribute string, ttlEnabled bool) string {
return fmt.Sprintf(`
resource "aws_dynamodb_table" "test" {
hash_key = "TestTableHashKey"
name = %[1]q
read_capacity = 1
write_capacity = 1
attribute {
name = "TestTableHashKey"
type = "S"
}
ttl {
attribute_name = %[2]q
enabled = %[3]t
}
}
`, rName, ttlAttribute, ttlEnabled)
}

func testAccTableConfig_timeToLive_unset(rName string) string {
return fmt.Sprintf(`
resource "aws_dynamodb_table" "test" {
hash_key = "TestTableHashKey"
name = %[1]q
read_capacity = 1
write_capacity = 1
attribute {
name = "TestTableHashKey"
type = "S"
}
}
`, rName)
}

func testAccTableConfig_TTL_missingAttributeName(rName string, ttlEnabled bool) string {
return fmt.Sprintf(`
resource "aws_dynamodb_table" "test" {
hash_key = "TestTableHashKey"
Expand All @@ -3510,7 +3647,7 @@ resource "aws_dynamodb_table" "test" {
}
ttl {
attribute_name = %[2]t ? "TestTTL" : ""
attribute_name = ""
enabled = %[2]t
}
}
Expand Down
8 changes: 5 additions & 3 deletions website/docs/r/dynamodb_table.html.markdown
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ resource "aws_dynamodb_table" "basic-dynamodb-table" {
ttl {
attribute_name = "TimeToExist"
enabled = false
enabled = true
}
global_secondary_index {
Expand Down Expand Up @@ -261,8 +261,10 @@ Optional arguments:

### `ttl`

* `enabled` - (Required) Whether TTL is enabled.
* `attribute_name` - (Required) Name of the table attribute to store the TTL timestamp in.
* `attribute_name` - (Optional) Name of the table attribute to store the TTL timestamp in.
Required if `enabled` is `true`, must not be set otherwise.
* `enabled` - (Optional) Whether TTL is enabled.
Default value is `false`.

## Attribute Reference

Expand Down

0 comments on commit 46774b5

Please sign in to comment.