diff --git a/.changelog/37991.txt b/.changelog/37991.txt new file mode 100644 index 00000000000..9b338f4e5f3 --- /dev/null +++ b/.changelog/37991.txt @@ -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. +``` diff --git a/internal/service/dynamodb/table.go b/internal/service/dynamodb/table.go index f43c6044f75..c7e238e5727 100644 --- a/internal/service/dynamodb/table.go +++ b/internal/service/dynamodb/table.go @@ -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" @@ -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, ), @@ -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, @@ -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", + )) + } + } +} diff --git a/internal/service/dynamodb/table_test.go b/internal/service/dynamodb/table_test.go index ffa84f497c9..9a73db6451a 100644 --- a/internal/service/dynamodb/table_test.go +++ b/internal/service/dynamodb/table_test.go @@ -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" @@ -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, @@ -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, + }, }, }) } @@ -1393,12 +1413,60 @@ 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, @@ -1406,12 +1474,44 @@ func TestAccDynamoDBTable_TTL_disabled(t *testing.T) { 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`)), }, }, }) @@ -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" @@ -3510,7 +3647,7 @@ resource "aws_dynamodb_table" "test" { } ttl { - attribute_name = %[2]t ? "TestTTL" : "" + attribute_name = "" enabled = %[2]t } } diff --git a/website/docs/r/dynamodb_table.html.markdown b/website/docs/r/dynamodb_table.html.markdown index a708e401e50..ab4672b0dbe 100644 --- a/website/docs/r/dynamodb_table.html.markdown +++ b/website/docs/r/dynamodb_table.html.markdown @@ -55,7 +55,7 @@ resource "aws_dynamodb_table" "basic-dynamodb-table" { ttl { attribute_name = "TimeToExist" - enabled = false + enabled = true } global_secondary_index { @@ -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