From bba2b9331eb14bb4bcabe9a413f0e1fcaf4b6d9c Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Wed, 25 Jan 2023 21:05:01 -0500 Subject: [PATCH 01/17] dynamodb/table_replica: Fix creation error when KMS --- internal/service/dynamodb/table.go | 1 + internal/service/dynamodb/table_replica.go | 23 ++-- .../service/dynamodb/table_replica_test.go | 103 ++++++++++++++++++ internal/service/dynamodb/table_test.go | 77 ++++++++++++- website/docs/r/dynamodb_table.html.markdown | 2 +- .../r/dynamodb_table_replica.html.markdown | 2 +- 6 files changed, 191 insertions(+), 17 deletions(-) diff --git a/internal/service/dynamodb/table.go b/internal/service/dynamodb/table.go index 591fbb442939..d4720b3b2504 100644 --- a/internal/service/dynamodb/table.go +++ b/internal/service/dynamodb/table.go @@ -265,6 +265,7 @@ func ResourceTable() *schema.Resource { Optional: true, Computed: true, ValidateFunc: verify.ValidARN, + ForceNew: true, }, "point_in_time_recovery": { Type: schema.TypeBool, diff --git a/internal/service/dynamodb/table_replica.go b/internal/service/dynamodb/table_replica.go index 4e7606d5cf9b..2a055b00a4c1 100644 --- a/internal/service/dynamodb/table_replica.go +++ b/internal/service/dynamodb/table_replica.go @@ -68,6 +68,7 @@ func ResourceTableReplica() *schema.Resource { Optional: true, Computed: true, ValidateFunc: verify.ValidARN, + ForceNew: true, }, "point_in_time_recovery": { // direct to replica Type: schema.TypeBool, @@ -128,11 +129,9 @@ func resourceTableReplicaCreate(ctx context.Context, d *schema.ResourceData, met input := &dynamodb.UpdateTableInput{ TableName: aws.String(tableName), - ReplicaUpdates: []*dynamodb.ReplicationGroupUpdate{ - { - Create: replicaInput, - }, - }, + ReplicaUpdates: []*dynamodb.ReplicationGroupUpdate{{ + Create: replicaInput, + }}, } err = resource.RetryContext(ctx, maxDuration(replicaUpdateTimeout, d.Timeout(schema.TimeoutCreate)), func() *resource.RetryError { @@ -369,21 +368,19 @@ func resourceTableReplicaUpdate(ctx context.Context, d *schema.ResourceData, met viaMainChanges := false viaMainInput := &dynamodb.UpdateReplicationGroupMemberAction{ - RegionName: aws.String(replicaRegion), + RegionName: aws.String(replicaRegion), + KMSMasterKeyId: aws.String(d.Get("kms_key_arn").(string)), } - if d.HasChange("kms_key_arn") { + if d.HasChange("kms_key_arn") && !d.IsNewResource() { viaMainChanges = true - viaMainInput.KMSMasterKeyId = aws.String(d.Get("kms_key_arn").(string)) } if viaMainChanges { input := &dynamodb.UpdateTableInput{ - ReplicaUpdates: []*dynamodb.ReplicationGroupUpdate{ - { - Update: viaMainInput, - }, - }, + ReplicaUpdates: []*dynamodb.ReplicationGroupUpdate{{ + Update: viaMainInput, + }}, TableName: aws.String(tableName), } diff --git a/internal/service/dynamodb/table_replica_test.go b/internal/service/dynamodb/table_replica_test.go index 77c08eff24c7..5197ee848666 100644 --- a/internal/service/dynamodb/table_replica_test.go +++ b/internal/service/dynamodb/table_replica_test.go @@ -209,6 +209,44 @@ func TestAccDynamoDBTableReplica_tableClass(t *testing.T) { }) } +func TestAccDynamoDBTableReplica_keys(t *testing.T) { + ctx := acctest.Context(t) + if testing.Short() { + t.Skip("skipping long-running test in short mode") + } + + resourceName := "aws_dynamodb_table_replica.test" + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(t); acctest.PreCheckMultipleRegion(t, 2) }, + ErrorCheck: acctest.ErrorCheck(t, dynamodb.EndpointsID), + ProtoV5ProviderFactories: acctest.ProtoV5FactoriesMultipleRegions(t, 2), + CheckDestroy: testAccCheckTableReplicaDestroy(ctx), + Steps: []resource.TestStep{ + { + Config: testAccTableReplicaConfig_keys(rName, "test1"), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckTableReplicaExists(ctx, resourceName), + resource.TestCheckResourceAttrPair(resourceName, "kms_key_arn", "aws_kms_key.test1", "arn"), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + { + Config: testAccTableReplicaConfig_keys(rName, "test2"), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckTableReplicaExists(ctx, resourceName), + resource.TestCheckResourceAttrPair(resourceName, "kms_key_arn", "aws_kms_key.test2", "arn"), + ), + }, + }, + }) +} + func testAccCheckTableReplicaDestroy(ctx context.Context) resource.TestCheckFunc { return func(s *terraform.State) error { conn := acctest.Provider.Meta().(*conns.AWSClient).DynamoDBConn() @@ -516,3 +554,68 @@ resource "aws_dynamodb_table_replica" "test" { } `, rName, class)) } + +func testAccTableReplicaConfig_keys(rName, key string) string { + return acctest.ConfigCompose( + acctest.ConfigMultipleRegionProvider(2), + fmt.Sprintf(` +resource "aws_kms_key" "alternate" { + provider = awsalternate + description = "Julie test KMS key A" + multi_region = false + deletion_window_in_days = 7 +} + +resource "aws_kms_key" "test1" { + description = "Julie test KMS key Z" + multi_region = false + deletion_window_in_days = 7 +} + +resource "aws_kms_key" "test2" { + description = "Julie test KMS key Z" + multi_region = false + deletion_window_in_days = 7 +} + +resource "aws_dynamodb_table" "test" { + provider = awsalternate + name = %[1]q + hash_key = "ParticipantId" + range_key = "SubscriptionId" + billing_mode = "PAY_PER_REQUEST" + stream_enabled = true + stream_view_type = "NEW_AND_OLD_IMAGES" + table_class = "STANDARD" + + point_in_time_recovery { + enabled = true + } + + server_side_encryption { + enabled = true + kms_key_arn = aws_kms_key.alternate.arn + } + + attribute { + name = "ParticipantId" + type = "S" + } + + attribute { + name = "SubscriptionId" + type = "S" + } + + lifecycle { + ignore_changes = [replica] + } +} + +resource "aws_dynamodb_table_replica" "test" { + global_table_arn = aws_dynamodb_table.test.arn + kms_key_arn = aws_kms_key.%[2]s.arn + point_in_time_recovery = true +} +`, rName, key)) +} diff --git a/internal/service/dynamodb/table_test.go b/internal/service/dynamodb/table_test.go index 6bc4d1aca784..584ed5684353 100644 --- a/internal/service/dynamodb/table_test.go +++ b/internal/service/dynamodb/table_test.go @@ -1662,6 +1662,7 @@ func TestAccDynamoDBTable_Replica_singleAddCMK(t *testing.T) { kmsKeyResourceName := "aws_kms_key.test" kmsAliasDatasourceName := "data.aws_kms_alias.dynamodb" kmsKeyReplicaResourceName := "aws_kms_key.replica" + kmsKeyReplica2ResourceName := "aws_kms_key.replica2" kmsAliasReplicaDatasourceName := "data.aws_kms_alias.replica" rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) @@ -1686,7 +1687,7 @@ func TestAccDynamoDBTable_Replica_singleAddCMK(t *testing.T) { ExpectNonEmptyPlan: true, // Because of https://github.com/hashicorp/terraform-provider-aws/issues/27850 }, { - Config: testAccTableConfig_replicaCMK(rName), + Config: testAccTableConfig_replicaCMKUpdate(rName, "replica"), Check: resource.ComposeAggregateTestCheckFunc( testAccCheckInitialTableExists(ctx, resourceName, &conf), resource.TestCheckResourceAttr(resourceName, "replica.#", "1"), @@ -1696,7 +1697,17 @@ func TestAccDynamoDBTable_Replica_singleAddCMK(t *testing.T) { ), }, { - Config: testAccTableConfig_replicaCMK(rName), + Config: testAccTableConfig_replicaCMKUpdate(rName, "replica2"), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckInitialTableExists(ctx, resourceName, &conf), + resource.TestCheckResourceAttr(resourceName, "replica.#", "1"), + resource.TestCheckResourceAttrPair(resourceName, "replica.0.kms_key_arn", kmsKeyReplica2ResourceName, "arn"), + resource.TestCheckResourceAttr(resourceName, "server_side_encryption.0.enabled", "true"), + resource.TestCheckResourceAttrPair(resourceName, "server_side_encryption.0.kms_key_arn", kmsKeyResourceName, "arn"), + ), + }, + { + Config: testAccTableConfig_replicaCMKUpdate(rName, "replica2"), ResourceName: resourceName, ImportState: true, ImportStateVerify: true, @@ -3189,6 +3200,12 @@ resource "aws_kms_key" "replica" { deletion_window_in_days = 7 } +resource "aws_kms_key" "replica2" { + provider = "awsalternate" + description = "%[1]s-3" + deletion_window_in_days = 7 +} + resource "aws_dynamodb_table" "test" { name = %[1]q hash_key = "TestTableHashKey" @@ -3220,6 +3237,62 @@ resource "aws_dynamodb_table" "test" { `, rName)) } +func testAccTableConfig_replicaCMKUpdate(rName, key string) string { + return acctest.ConfigCompose( + acctest.ConfigMultipleRegionProvider(3), // Prevent "Provider configuration not present" errors + fmt.Sprintf(` +data "aws_region" "alternate" { + provider = "awsalternate" +} + +resource "aws_kms_key" "test" { + description = %[1]q + deletion_window_in_days = 7 +} + +resource "aws_kms_key" "replica" { + provider = "awsalternate" + description = "%[1]s-2" + deletion_window_in_days = 7 +} + +resource "aws_kms_key" "replica2" { + provider = "awsalternate" + description = "%[1]s-3" + deletion_window_in_days = 7 +} + +resource "aws_dynamodb_table" "test" { + name = %[1]q + hash_key = "TestTableHashKey" + billing_mode = "PAY_PER_REQUEST" + stream_enabled = true + stream_view_type = "NEW_AND_OLD_IMAGES" + + attribute { + name = "TestTableHashKey" + type = "S" + } + + replica { + region_name = data.aws_region.alternate.name + kms_key_arn = aws_kms_key.%[2]s.arn + } + + server_side_encryption { + enabled = true + kms_key_arn = aws_kms_key.test.arn + } + + timeouts { + create = "20m" + update = "20m" + delete = "20m" + } +} +`, rName, key)) +} + func testAccTableConfig_replicaAmazonManagedKey(rName string) string { return acctest.ConfigCompose( acctest.ConfigMultipleRegionProvider(3), // Prevent "Provider configuration not present" errors diff --git a/website/docs/r/dynamodb_table.html.markdown b/website/docs/r/dynamodb_table.html.markdown index 46bc49c3df5a..99406f544680 100644 --- a/website/docs/r/dynamodb_table.html.markdown +++ b/website/docs/r/dynamodb_table.html.markdown @@ -221,7 +221,7 @@ Optional arguments: ### `replica` -* `kms_key_arn` - (Optional) ARN of the CMK that should be used for the AWS KMS encryption. +* `kms_key_arn` - (Optional, Forces new resource) ARN of the CMK that should be used for the AWS KMS encryption. * `point_in_time_recovery` - (Optional) Whether to enable Point In Time Recovery for the replica. Default is `false`. * `propagate_tags` - (Optional) Whether to propagate the global table's tags to a replica. Default is `false`. Changes to tags only move in one direction: from global (source) to replica. In other words, tag drift on a replica will not trigger an update. Tag or replica changes on the global table, whether from drift or configuration changes, are propagated to replicas. Changing from `true` to `false` on a subsequent `apply` means replica tags are left as they were, unmanaged, not deleted. * `region_name` - (Required) Region name of the replica. diff --git a/website/docs/r/dynamodb_table_replica.html.markdown b/website/docs/r/dynamodb_table_replica.html.markdown index 1b3f6150b193..c05182e55cd3 100644 --- a/website/docs/r/dynamodb_table_replica.html.markdown +++ b/website/docs/r/dynamodb_table_replica.html.markdown @@ -66,7 +66,7 @@ Required arguments: Optional arguments: -* `kms_key_arn` - (Optional) ARN of the CMK that should be used for the AWS KMS encryption. +* `kms_key_arn` - (Optional, Forces new resource) ARN of the CMK that should be used for the AWS KMS encryption. * `point_in_time_recovery` - (Optional) Whether to enable Point In Time Recovery for the replica. Default is `false`. * `table_class_override` - (Optional, Forces new resource) Storage class of the table replica. Valid values are `STANDARD` and `STANDARD_INFREQUENT_ACCESS`. If not used, the table replica will use the same class as the global table. * `tags` - (Optional) Map of tags to populate on the created table. If configured with a provider [`default_tags` configuration block](https://registry.terraform.io/providers/hashicorp/aws/latest/docs#default_tags-configuration-block) present, tags with matching keys will overwrite those defined at the provider-level. From e5b8e7dfd30da17ad23ddd8e6e9552c2a9d752e5 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Wed, 25 Jan 2023 23:42:05 -0500 Subject: [PATCH 02/17] Add changelog --- .changelog/29102.txt | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 .changelog/29102.txt diff --git a/.changelog/29102.txt b/.changelog/29102.txt new file mode 100644 index 000000000000..d39248fd202a --- /dev/null +++ b/.changelog/29102.txt @@ -0,0 +1,23 @@ +```release-note:bug +resource/aws_dynamodb_table: Fix to allow updating of `replica.*.point_in_time_recovery` when a `replica` has `kms_key_arn` set +``` + +```release-note:bug +resource/aws_dynamodb_table: Fix to allow updating of `replica.*.kms_key_arn` +``` + +```release-note:note +resource/aws_dynamodb_table: Updating `replica.*.kms_key_arn` or `replica.*.point_in_time_recovery`, when the `replica`'s `kms_key_arn` is set, requires recreating the replica. +``` + +```release-note:note +resource/aws_dynamodb_table_replica: Updating `kms_key_arn` forces replacement of the replica now as required to re-encrypt the replica +``` + +```release-note:bug +resource/aws_dynamodb_table_replica: Fix to allow updating of `kms_key_arn` +``` + +```release-note:bug +resource/aws_dynamodb_table_replica: Fix to allow creation of the replica without errors when `kms_key_arn` is set +``` From ac8d20682670cb8167c63c5f28aad63f1fc4f750 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Wed, 25 Jan 2023 23:43:31 -0500 Subject: [PATCH 03/17] Fix bugs to allow updates of CMK and PITR when CMK set --- internal/service/dynamodb/table.go | 35 ++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/internal/service/dynamodb/table.go b/internal/service/dynamodb/table.go index d4720b3b2504..18245bb5b10c 100644 --- a/internal/service/dynamodb/table.go +++ b/internal/service/dynamodb/table.go @@ -265,7 +265,7 @@ func ResourceTable() *schema.Resource { Optional: true, Computed: true, ValidateFunc: verify.ValidARN, - ForceNew: true, + // update is equivalent of force a new *replica* }, "point_in_time_recovery": { Type: schema.TypeBool, @@ -280,6 +280,7 @@ func ResourceTable() *schema.Resource { "region_name": { Type: schema.TypeString, Required: true, + // update is equivalent of force a new *replica* }, }, }, @@ -1274,22 +1275,28 @@ func updateReplica(ctx context.Context, d *schema.ResourceData, conn *dynamodb.D oRaw, nRaw := d.GetChange("replica") o := oRaw.(*schema.Set) n := nRaw.(*schema.Set) - newRegions := replicaRegions(n) - oldRegions := replicaRegions(o) - // If it is a change in KMS keys causing the diff no updates needed - if reflect.DeepEqual(oldRegions, newRegions) { - return nil - } removed := o.Difference(n).List() added := n.Difference(o).List() + // 1. changing replica kms keys requires recreation of the replica, like ForceNew, but we don't want to ForceNew the *table* + // 2. also, in order to update PITR if a replica is encrypted (has KMS key), it requires recreation (how'd u restore from a backup encrypted with a different key?) + + var removeFirst []interface{} // replicas to delete before recreating (~ForceNew without recreating table) + // For true updates, don't remove and add, just update (i.e., keep in added // but remove from removed) for _, a := range added { for j, r := range removed { ma := a.(map[string]interface{}) mr := r.(map[string]interface{}) + + if ma["region_name"].(string) == mr["region_name"].(string) && (ma["kms_key_arn"].(string) != "" || mr["kms_key_arn"].(string) != "") { + removeFirst = append(removeFirst, removed[j]) + removed = append(removed[:j], removed[j+1:]...) + continue + } + if ma["region_name"].(string) == mr["region_name"].(string) { removed = append(removed[:j], removed[j+1:]...) continue @@ -1297,6 +1304,12 @@ func updateReplica(ctx context.Context, d *schema.ResourceData, conn *dynamodb.D } } + if len(removeFirst) > 0 { // like ForceNew but doesn't recreate the table + if err := deleteReplicas(ctx, conn, d.Id(), removeFirst, d.Timeout(schema.TimeoutUpdate)); err != nil { + return fmt.Errorf("updating replicas, while deleting: %w", err) + } + } + if len(added) > 0 { if err := createReplicas(ctx, conn, d.Id(), added, tfVersion, true, d.Timeout(schema.TimeoutUpdate)); err != nil { return fmt.Errorf("updating replicas, while creating: %w", err) @@ -1529,9 +1542,8 @@ func deleteReplicas(ctx context.Context, conn *dynamodb.DynamoDB, tableName stri } func replicaPITR(ctx context.Context, conn *dynamodb.DynamoDB, tableName string, region string, tfVersion string) (bool, error) { - // At a future time, replicas should probably have a separate resource because, - // to manage them, you need connections from the different regions. However, they - // have to be created from the starting/main region... + // To manage replicas you need connections from the different regions. However, they + // have to be created from the starting/main region. session, err := conns.NewSessionForRegion(&conn.Config, region, tfVersion) if err != nil { return false, fmt.Errorf("new session for replica (%s) PITR: %w", region, err) @@ -1565,8 +1577,7 @@ func replicaPITR(ctx context.Context, conn *dynamodb.DynamoDB, tableName string, func addReplicaPITRs(ctx context.Context, conn *dynamodb.DynamoDB, tableName string, tfVersion string, replicas []interface{}) ([]interface{}, error) { // This non-standard approach is needed because PITR info for a replica - // must come from a region-specific connection. A future table_replica - // resource may improve this. + // must come from a region-specific connection. for i, replicaRaw := range replicas { replica := replicaRaw.(map[string]interface{}) From e8d6f0951b537fa755970b699a54b7b2b05ffcd7 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Wed, 25 Jan 2023 23:44:06 -0500 Subject: [PATCH 04/17] Update tests to cover what was broken --- .../service/dynamodb/table_replica_test.go | 94 ++++++++++++++ internal/service/dynamodb/table_test.go | 122 ++++++++++++++++++ 2 files changed, 216 insertions(+) diff --git a/internal/service/dynamodb/table_replica_test.go b/internal/service/dynamodb/table_replica_test.go index 5197ee848666..a853685ec7b6 100644 --- a/internal/service/dynamodb/table_replica_test.go +++ b/internal/service/dynamodb/table_replica_test.go @@ -109,6 +109,54 @@ func TestAccDynamoDBTableReplica_pitr(t *testing.T) { }) } +func TestAccDynamoDBTableReplica_pitrKMS(t *testing.T) { + ctx := acctest.Context(t) + if testing.Short() { + t.Skip("skipping long-running test in short mode") + } + + resourceName := "aws_dynamodb_table_replica.test" + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(t); acctest.PreCheckMultipleRegion(t, 2) }, + ErrorCheck: acctest.ErrorCheck(t, dynamodb.EndpointsID), + ProtoV5ProviderFactories: acctest.ProtoV5FactoriesMultipleRegions(t, 3), + CheckDestroy: testAccCheckTableReplicaDestroy(ctx), + Steps: []resource.TestStep{ + { + Config: testAccTableReplicaConfig_pitrKMS(rName, false), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckTableReplicaExists(ctx, resourceName), + resource.TestCheckResourceAttr(resourceName, "point_in_time_recovery", "false"), + resource.TestCheckResourceAttrPair(resourceName, "kms_key_arn", "aws_kms_key.alternate", "arn"), + ), + }, + { + Config: testAccTableReplicaConfig_pitrKMS(rName, true), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckTableReplicaExists(ctx, resourceName), + resource.TestCheckResourceAttr(resourceName, "point_in_time_recovery", "true"), + resource.TestCheckResourceAttrPair(resourceName, "kms_key_arn", "aws_kms_key.alternate", "arn"), + ), + }, + { + Config: testAccTableReplicaConfig_pitrKMS(rName, false), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckTableReplicaExists(ctx, resourceName), + resource.TestCheckResourceAttr(resourceName, "point_in_time_recovery", "false"), + resource.TestCheckResourceAttrPair(resourceName, "kms_key_arn", "aws_kms_key.alternate", "arn"), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + func TestAccDynamoDBTableReplica_tags(t *testing.T) { ctx := acctest.Context(t) if testing.Short() { @@ -405,6 +453,52 @@ resource "aws_dynamodb_table_replica" "test" { `, rName)) } +func testAccTableReplicaConfig_pitrKMS(rName string, pitr bool) string { + return acctest.ConfigCompose( + acctest.ConfigMultipleRegionProvider(3), + fmt.Sprintf(` +resource "aws_kms_key" "test" { + description = %[1]q + deletion_window_in_days = 7 +} + +resource "aws_dynamodb_table" "test" { + name = %[1]q + hash_key = "TestTableHashKey" + billing_mode = "PAY_PER_REQUEST" + stream_enabled = true + stream_view_type = "NEW_AND_OLD_IMAGES" + + attribute { + name = "TestTableHashKey" + type = "S" + } + + server_side_encryption { + enabled = true + kms_key_arn = aws_kms_key.test.arn + } + + lifecycle { + ignore_changes = [replica] + } +} + +resource "aws_kms_key" "alternate" { + provider = awsalternate + description = %[1]q + deletion_window_in_days = 7 +} + +resource "aws_dynamodb_table_replica" "test" { + provider = awsalternate + global_table_arn = aws_dynamodb_table.test.arn + point_in_time_recovery = %[2]t + kms_key_arn = aws_kms_key.alternate.arn +} +`, rName, pitr)) +} + func testAccTableReplicaConfig_tags1(rName string) string { return acctest.ConfigCompose( acctest.ConfigMultipleRegionProvider(3), diff --git a/internal/service/dynamodb/table_test.go b/internal/service/dynamodb/table_test.go index 584ed5684353..8456f73ecac3 100644 --- a/internal/service/dynamodb/table_test.go +++ b/internal/service/dynamodb/table_test.go @@ -1773,6 +1773,63 @@ func TestAccDynamoDBTable_Replica_pitr(t *testing.T) { }) } +func TestAccDynamoDBTable_Replica_pitrKMS(t *testing.T) { + ctx := acctest.Context(t) + if testing.Short() { + t.Skip("skipping long-running test in short mode") + } + + var conf dynamodb.TableDescription + resourceName := "aws_dynamodb_table.test" + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { + acctest.PreCheck(t) + acctest.PreCheckMultipleRegion(t, 2) + }, + ErrorCheck: acctest.ErrorCheck(t, dynamodb.EndpointsID), + ProtoV5ProviderFactories: acctest.ProtoV5FactoriesMultipleRegions(t, 3), // 3 due to shared test configuration + CheckDestroy: testAccCheckTableDestroy(ctx), + Steps: []resource.TestStep{ + { + Config: testAccTableConfig_replicaPITRKMS(rName, false, true, false), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckInitialTableExists(ctx, resourceName, &conf), + resource.TestCheckResourceAttr(resourceName, "replica.#", "2"), + resource.TestCheckTypeSetElemNestedAttrs(resourceName, "replica.*", map[string]string{ + "point_in_time_recovery": "true", + "region_name": acctest.AlternateRegion(), + }), + resource.TestCheckTypeSetElemNestedAttrs(resourceName, "replica.*", map[string]string{ + "point_in_time_recovery": "false", + "region_name": acctest.ThirdRegion(), + }), + resource.TestCheckResourceAttr(resourceName, "point_in_time_recovery.#", "1"), + resource.TestCheckResourceAttr(resourceName, "point_in_time_recovery.0.enabled", "false"), + ), + }, + { + Config: testAccTableConfig_replicaPITRKMS(rName, true, false, true), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckInitialTableExists(ctx, resourceName, &conf), + resource.TestCheckResourceAttr(resourceName, "replica.#", "2"), + resource.TestCheckTypeSetElemNestedAttrs(resourceName, "replica.*", map[string]string{ + "point_in_time_recovery": "false", + "region_name": acctest.AlternateRegion(), + }), + resource.TestCheckTypeSetElemNestedAttrs(resourceName, "replica.*", map[string]string{ + "point_in_time_recovery": "true", + "region_name": acctest.ThirdRegion(), + }), + resource.TestCheckResourceAttr(resourceName, "point_in_time_recovery.#", "1"), + resource.TestCheckResourceAttr(resourceName, "point_in_time_recovery.0.enabled", "true"), + ), + }, + }, + }) +} + func TestAccDynamoDBTable_Replica_tagsOneOfTwo(t *testing.T) { ctx := acctest.Context(t) if testing.Short() { @@ -3380,6 +3437,71 @@ resource "aws_dynamodb_table" "test" { `, rName, mainPITR, replica1, replica2)) } +func testAccTableConfig_replicaPITRKMS(rName string, mainPITR, replica1, replica2 bool) string { + return acctest.ConfigCompose( + acctest.ConfigMultipleRegionProvider(3), + fmt.Sprintf(` +data "aws_region" "alternate" { + provider = awsalternate +} + +data "aws_region" "third" { + provider = awsthird +} + +resource "aws_kms_key" "test" { + description = %[1]q + deletion_window_in_days = 7 +} + +resource "aws_kms_key" "alternate" { + provider = awsalternate + description = %[1]q + deletion_window_in_days = 7 +} + +resource "aws_kms_key" "third" { + provider = awsthird + description = %[1]q + deletion_window_in_days = 7 +} + +resource "aws_dynamodb_table" "test" { + name = %[1]q + hash_key = "TestTableHashKey" + billing_mode = "PAY_PER_REQUEST" + stream_enabled = true + stream_view_type = "NEW_AND_OLD_IMAGES" + + attribute { + name = "TestTableHashKey" + type = "S" + } + + point_in_time_recovery { + enabled = %[2]t + } + + server_side_encryption { + enabled = true + kms_key_arn = aws_kms_key.test.arn + } + + replica { + region_name = data.aws_region.alternate.name + point_in_time_recovery = %[3]t + kms_key_arn = aws_kms_key.alternate.arn + } + + replica { + region_name = data.aws_region.third.name + point_in_time_recovery = %[4]t + kms_key_arn = aws_kms_key.third.arn + } +} +`, rName, mainPITR, replica1, replica2)) +} + func testAccTableConfig_replicaTags(rName, key, value string, propagate1, propagate2 bool) string { return acctest.ConfigCompose( acctest.ConfigMultipleRegionProvider(3), From d3f402db7e0603efd6b391fe383f5a8dbc17e4d2 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 26 Jan 2023 00:11:47 -0500 Subject: [PATCH 05/17] Remove unnecessary change --- internal/service/dynamodb/table_replica.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/service/dynamodb/table_replica.go b/internal/service/dynamodb/table_replica.go index 2a055b00a4c1..320db9dff92a 100644 --- a/internal/service/dynamodb/table_replica.go +++ b/internal/service/dynamodb/table_replica.go @@ -368,12 +368,12 @@ func resourceTableReplicaUpdate(ctx context.Context, d *schema.ResourceData, met viaMainChanges := false viaMainInput := &dynamodb.UpdateReplicationGroupMemberAction{ - RegionName: aws.String(replicaRegion), - KMSMasterKeyId: aws.String(d.Get("kms_key_arn").(string)), + RegionName: aws.String(replicaRegion), } - if d.HasChange("kms_key_arn") && !d.IsNewResource() { + if d.HasChange("kms_key_arn") && !d.IsNewResource() { // create ends with update and sets kms_key_arn causing change that is not viaMainChanges = true + viaMainInput.KMSMasterKeyId = aws.String(d.Get("kms_key_arn").(string)) } if viaMainChanges { From b5de36def67dc5ef5c54014c564ad6ae2f44b81d Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 26 Jan 2023 00:14:37 -0500 Subject: [PATCH 06/17] Remove unused --- internal/service/dynamodb/table.go | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/internal/service/dynamodb/table.go b/internal/service/dynamodb/table.go index 18245bb5b10c..d6ff37b32ec6 100644 --- a/internal/service/dynamodb/table.go +++ b/internal/service/dynamodb/table.go @@ -1325,18 +1325,6 @@ func updateReplica(ctx context.Context, d *schema.ResourceData, conn *dynamodb.D return nil } -func replicaRegions(r *schema.Set) []string { - var regions []string - for _, tfMapRaw := range r.List() { - tfMap, ok := tfMapRaw.(map[string]interface{}) - if !ok { - continue - } - regions = append(regions, tfMap["region_name"].(string)) - } - return regions -} - func UpdateDiffGSI(oldGsi, newGsi []interface{}, billingMode string) (ops []*dynamodb.GlobalSecondaryIndexUpdate, e error) { // Transform slices into maps oldGsis := make(map[string]interface{}) From 5fafa157f3b4fc9489ee2cbee4ee8d4f579c83bd Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 26 Jan 2023 22:21:22 -0500 Subject: [PATCH 07/17] kms: Add default key by region finder --- internal/service/kms/find.go | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/internal/service/kms/find.go b/internal/service/kms/find.go index 6c8419595060..614edbe5b091 100644 --- a/internal/service/kms/find.go +++ b/internal/service/kms/find.go @@ -2,11 +2,13 @@ package kms import ( "context" + "fmt" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/kms" "github.com/hashicorp/aws-sdk-go-base/v2/awsv1shim/v2/tfawserr" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" + "github.com/hashicorp/terraform-provider-aws/internal/conns" "github.com/hashicorp/terraform-provider-aws/internal/tfresource" ) @@ -96,6 +98,26 @@ func FindKeyByID(ctx context.Context, conn *kms.KMS, id string) (*kms.KeyMetadat return keyMetadata, nil } +func FindDefaultKey(ctx context.Context, service, region string, meta interface{}) (string, error) { + conn := meta.(*conns.AWSClient).KMSConn() + + if aws.StringValue(conn.Config.Region) != region { + session, err := conns.NewSessionForRegion(&conn.Config, region, meta.(*conns.AWSClient).TerraformVersion) + if err != nil { + return "", fmt.Errorf("finding default key, getting connection for %s: %w", region, err) + } + + conn = kms.New(session) + } + + k, err := FindKeyByID(ctx, conn, fmt.Sprintf("alias/aws/%s", service)) //default key + if err != nil { + return "", fmt.Errorf("finding default key: %s", err) + } + + return aws.StringValue(k.Arn), nil +} + func FindKeyPolicyByKeyIDAndPolicyName(ctx context.Context, conn *kms.KMS, keyID, policyName string) (*string, error) { input := &kms.GetKeyPolicyInput{ KeyId: aws.String(keyID), From 05729bc73610b7f5ac9ecbe88d96c1407e169162 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 26 Jan 2023 22:22:11 -0500 Subject: [PATCH 08/17] dcos/dynamodb_table: Clarify kms_key_arn --- website/docs/r/dynamodb_table.html.markdown | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/website/docs/r/dynamodb_table.html.markdown b/website/docs/r/dynamodb_table.html.markdown index 99406f544680..ba42ce96e70d 100644 --- a/website/docs/r/dynamodb_table.html.markdown +++ b/website/docs/r/dynamodb_table.html.markdown @@ -221,15 +221,15 @@ Optional arguments: ### `replica` -* `kms_key_arn` - (Optional, Forces new resource) ARN of the CMK that should be used for the AWS KMS encryption. +* `kms_key_arn` - (Optional, Forces new resource) ARN of the CMK that should be used for the AWS KMS encryption. This argument should only be used if the key is different from the default KMS-managed DynamoDB key, `alias/aws/dynamodb`. **Note:** This attribute will _not_ be populated with the ARN of _default_ keys. * `point_in_time_recovery` - (Optional) Whether to enable Point In Time Recovery for the replica. Default is `false`. * `propagate_tags` - (Optional) Whether to propagate the global table's tags to a replica. Default is `false`. Changes to tags only move in one direction: from global (source) to replica. In other words, tag drift on a replica will not trigger an update. Tag or replica changes on the global table, whether from drift or configuration changes, are propagated to replicas. Changing from `true` to `false` on a subsequent `apply` means replica tags are left as they were, unmanaged, not deleted. * `region_name` - (Required) Region name of the replica. ### `server_side_encryption` -* `enabled` - (Required) Whether or not to enable encryption at rest using an AWS managed KMS customer master key (CMK). If `enabled` is `false` then server-side encryption is set to AWS owned CMK (shown as `DEFAULT` in the AWS console). If `enabled` is `true` and no `kms_key_arn` is specified then server-side encryption is set to AWS managed CMK (shown as `KMS` in the AWS console). The [AWS KMS documentation](https://docs.aws.amazon.com/kms/latest/developerguide/concepts.html) explains the difference between AWS owned and AWS managed CMKs. -* `kms_key_arn` - (Optional) ARN of the CMK that should be used for the AWS KMS encryption. This attribute should only be specified if the key is different from the default DynamoDB CMK, `alias/aws/dynamodb`. +* `enabled` - (Required) Whether or not to enable encryption at rest using an AWS managed KMS customer master key (CMK). If `enabled` is `false` then server-side encryption is set to AWS-_owned_ key (shown as `DEFAULT` in the AWS console). Potentially confusingly, if `enabled` is `true` and no `kms_key_arn` is specified then server-side encryption is set to the _default_ KMS-_managed_ key (shown as `KMS` in the AWS console). The [AWS KMS documentation](https://docs.aws.amazon.com/kms/latest/developerguide/concepts.html) explains the difference between AWS-_owned_ and KMS-_managed_ keys. +* `kms_key_arn` - (Optional) ARN of the CMK that should be used for the AWS KMS encryption. This argument should only be used if the key is different from the default KMS-managed DynamoDB key, `alias/aws/dynamodb`. **Note:** This attribute will _not_ be populated with the ARN of _default_ keys. ### `ttl` From fef72753e05a1199d41505444d0e12cd68b810f6 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 26 Jan 2023 22:22:54 -0500 Subject: [PATCH 09/17] docs/dynamodb_table_replica: Clarify kms_key_arn with defaults --- website/docs/r/dynamodb_table_replica.html.markdown | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/docs/r/dynamodb_table_replica.html.markdown b/website/docs/r/dynamodb_table_replica.html.markdown index c05182e55cd3..c5b67bb9d6ec 100644 --- a/website/docs/r/dynamodb_table_replica.html.markdown +++ b/website/docs/r/dynamodb_table_replica.html.markdown @@ -66,7 +66,7 @@ Required arguments: Optional arguments: -* `kms_key_arn` - (Optional, Forces new resource) ARN of the CMK that should be used for the AWS KMS encryption. +* `kms_key_arn` - (Optional, Forces new resource) ARN of the CMK that should be used for the AWS KMS encryption. This argument should only be used if the key is different from the default KMS-managed DynamoDB key, `alias/aws/dynamodb`. **Note:** This attribute will _not_ be populated with the ARN of _default_ keys. * `point_in_time_recovery` - (Optional) Whether to enable Point In Time Recovery for the replica. Default is `false`. * `table_class_override` - (Optional, Forces new resource) Storage class of the table replica. Valid values are `STANDARD` and `STANDARD_INFREQUENT_ACCESS`. If not used, the table replica will use the same class as the global table. * `tags` - (Optional) Map of tags to populate on the created table. If configured with a provider [`default_tags` configuration block](https://registry.terraform.io/providers/hashicorp/aws/latest/docs#default_tags-configuration-block) present, tags with matching keys will overwrite those defined at the provider-level. From 9725b66c16fb519b8bffe647c04522332a481a88 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 26 Jan 2023 22:23:48 -0500 Subject: [PATCH 10/17] dynamodb/table_replica: Add tests for default keys --- .../service/dynamodb/table_replica_test.go | 81 +++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/internal/service/dynamodb/table_replica_test.go b/internal/service/dynamodb/table_replica_test.go index a853685ec7b6..abf62d3ebbd0 100644 --- a/internal/service/dynamodb/table_replica_test.go +++ b/internal/service/dynamodb/table_replica_test.go @@ -157,6 +157,54 @@ func TestAccDynamoDBTableReplica_pitrKMS(t *testing.T) { }) } +func TestAccDynamoDBTableReplica_pitrDefault(t *testing.T) { + ctx := acctest.Context(t) + if testing.Short() { + t.Skip("skipping long-running test in short mode") + } + + resourceName := "aws_dynamodb_table_replica.test" + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(t); acctest.PreCheckMultipleRegion(t, 2) }, + ErrorCheck: acctest.ErrorCheck(t, dynamodb.EndpointsID), + ProtoV5ProviderFactories: acctest.ProtoV5FactoriesMultipleRegions(t, 3), + CheckDestroy: testAccCheckTableReplicaDestroy(ctx), + Steps: []resource.TestStep{ + { + Config: testAccTableReplicaConfig_pitrDefault(rName, false), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckTableReplicaExists(ctx, resourceName), + resource.TestCheckResourceAttr(resourceName, "point_in_time_recovery", "false"), + resource.TestCheckResourceAttr(resourceName, "kms_key_arn", ""), + ), + }, + { + Config: testAccTableReplicaConfig_pitrDefault(rName, true), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckTableReplicaExists(ctx, resourceName), + resource.TestCheckResourceAttr(resourceName, "point_in_time_recovery", "true"), + resource.TestCheckResourceAttr(resourceName, "kms_key_arn", ""), + ), + }, + { + Config: testAccTableReplicaConfig_pitrDefault(rName, false), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckTableReplicaExists(ctx, resourceName), + resource.TestCheckResourceAttr(resourceName, "point_in_time_recovery", "false"), + resource.TestCheckResourceAttr(resourceName, "kms_key_arn", ""), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + func TestAccDynamoDBTableReplica_tags(t *testing.T) { ctx := acctest.Context(t) if testing.Short() { @@ -499,6 +547,39 @@ resource "aws_dynamodb_table_replica" "test" { `, rName, pitr)) } +func testAccTableReplicaConfig_pitrDefault(rName string, pitr bool) string { + return acctest.ConfigCompose( + acctest.ConfigMultipleRegionProvider(3), + fmt.Sprintf(` +resource "aws_dynamodb_table" "test" { + name = %[1]q + hash_key = "TestTableHashKey" + billing_mode = "PAY_PER_REQUEST" + stream_enabled = true + stream_view_type = "NEW_AND_OLD_IMAGES" + + attribute { + name = "TestTableHashKey" + type = "S" + } + + server_side_encryption { + enabled = true + } + + lifecycle { + ignore_changes = [replica] + } +} + +resource "aws_dynamodb_table_replica" "test" { + provider = awsalternate + global_table_arn = aws_dynamodb_table.test.arn + point_in_time_recovery = %[2]t +} +`, rName, pitr)) +} + func testAccTableReplicaConfig_tags1(rName string) string { return acctest.ConfigCompose( acctest.ConfigMultipleRegionProvider(3), From 34b76a7473677f73bd93f26210f7f1b84a1c6ce1 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 26 Jan 2023 22:24:39 -0500 Subject: [PATCH 11/17] dynamodb/table: Add tests for default KMS keys, diffs --- internal/service/dynamodb/table_test.go | 89 ++++++++++++++++++++++--- 1 file changed, 81 insertions(+), 8 deletions(-) diff --git a/internal/service/dynamodb/table_test.go b/internal/service/dynamodb/table_test.go index 8456f73ecac3..c1a35d6187dd 100644 --- a/internal/service/dynamodb/table_test.go +++ b/internal/service/dynamodb/table_test.go @@ -1470,7 +1470,7 @@ func TestAccDynamoDBTable_encryption(t *testing.T) { resourceName := "aws_dynamodb_table.test" rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) kmsKeyResourceName := "aws_kms_key.test" - kmsAliasDatasourceName := "data.aws_kms_alias.dynamodb" + //kmsAliasDatasourceName := "data.aws_kms_alias.dynamodb" resource.ParallelTest(t, resource.TestCase{ PreCheck: func() { acctest.PreCheck(t) }, @@ -1507,7 +1507,7 @@ func TestAccDynamoDBTable_encryption(t *testing.T) { testAccCheckTableNotRecreated(&confEncEnabled, &confEncDisabled), resource.TestCheckResourceAttr(resourceName, "server_side_encryption.#", "1"), resource.TestCheckResourceAttr(resourceName, "server_side_encryption.0.enabled", "true"), - resource.TestCheckResourceAttrPair(resourceName, "server_side_encryption.0.kms_key_arn", kmsAliasDatasourceName, "target_key_arn"), + resource.TestCheckResourceAttr(resourceName, "server_side_encryption.0.kms_key_arn", ""), ), }, }, @@ -1614,7 +1614,50 @@ func TestAccDynamoDBTable_Replica_single(t *testing.T) { }) } -func TestAccDynamoDBTable_Replica_singleWithCMK(t *testing.T) { +func TestAccDynamoDBTable_Replica_singleDefaultKeyEncrypted(t *testing.T) { + ctx := acctest.Context(t) + if testing.Short() { + t.Skip("skipping long-running test in short mode") + } + + var conf dynamodb.TableDescription + resourceName := "aws_dynamodb_table.test" + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { + acctest.PreCheck(t) + acctest.PreCheckMultipleRegion(t, 2) + }, + ErrorCheck: acctest.ErrorCheck(t, dynamodb.EndpointsID), + ProtoV5ProviderFactories: acctest.ProtoV5FactoriesMultipleRegions(t, 3), // 3 due to shared test configuration + CheckDestroy: testAccCheckTableDestroy(ctx), + Steps: []resource.TestStep{ + { + Config: testAccTableConfig_replicaEncryptedDefault(rName), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckInitialTableExists(ctx, resourceName, &conf), + resource.TestCheckResourceAttr(resourceName, "replica.#", "1"), + resource.TestCheckResourceAttr(resourceName, "server_side_encryption.0.enabled", "true"), + ), + }, + { + Config: testAccTableConfig_replicaEncryptedDefault(rName), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckInitialTableExists(ctx, resourceName, &conf), + resource.TestCheckResourceAttr(resourceName, "replica.#", "1"), + resource.TestCheckResourceAttr(resourceName, "server_side_encryption.0.enabled", "true"), + ), + }, + { + Config: testAccTableConfig_replicaEncryptedDefault(rName), + PlanOnly: true, + }, + }, + }) +} + +func TestAccDynamoDBTable_Replica_singleCMK(t *testing.T) { ctx := acctest.Context(t) if testing.Short() { t.Skip("skipping long-running test in short mode") @@ -1660,10 +1703,10 @@ func TestAccDynamoDBTable_Replica_singleAddCMK(t *testing.T) { var conf dynamodb.TableDescription resourceName := "aws_dynamodb_table.test" kmsKeyResourceName := "aws_kms_key.test" - kmsAliasDatasourceName := "data.aws_kms_alias.dynamodb" + //kmsAliasDatasourceName := "data.aws_kms_alias.dynamodb" kmsKeyReplicaResourceName := "aws_kms_key.replica" kmsKeyReplica2ResourceName := "aws_kms_key.replica2" - kmsAliasReplicaDatasourceName := "data.aws_kms_alias.replica" + //kmsAliasReplicaDatasourceName := "data.aws_kms_alias.replica" rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) resource.ParallelTest(t, resource.TestCase{ @@ -1681,10 +1724,9 @@ func TestAccDynamoDBTable_Replica_singleAddCMK(t *testing.T) { testAccCheckInitialTableExists(ctx, resourceName, &conf), resource.TestCheckResourceAttr(resourceName, "replica.#", "1"), resource.TestCheckResourceAttr(resourceName, "server_side_encryption.0.enabled", "true"), - resource.TestCheckResourceAttrPair(resourceName, "replica.0.kms_key_arn", kmsAliasReplicaDatasourceName, "target_key_arn"), - resource.TestCheckResourceAttrPair(resourceName, "server_side_encryption.0.kms_key_arn", kmsAliasDatasourceName, "target_key_arn"), + resource.TestCheckResourceAttr(resourceName, "replica.0.kms_key_arn", ""), + resource.TestCheckResourceAttr(resourceName, "server_side_encryption.0.kms_key_arn", ""), ), - ExpectNonEmptyPlan: true, // Because of https://github.com/hashicorp/terraform-provider-aws/issues/27850 }, { Config: testAccTableConfig_replicaCMKUpdate(rName, "replica"), @@ -3238,6 +3280,37 @@ resource "aws_dynamodb_table" "test" { `, rName)) } +func testAccTableConfig_replicaEncryptedDefault(rName string) string { + return acctest.ConfigCompose( + acctest.ConfigMultipleRegionProvider(3), // Prevent "Provider configuration not present" errors + fmt.Sprintf(` +data "aws_region" "alternate" { + provider = "awsalternate" +} + +resource "aws_dynamodb_table" "test" { + name = %[1]q + hash_key = "TestTableHashKey" + billing_mode = "PAY_PER_REQUEST" + stream_enabled = true + stream_view_type = "NEW_AND_OLD_IMAGES" + + attribute { + name = "TestTableHashKey" + type = "S" + } + + server_side_encryption { + enabled = true + } + + replica { + region_name = data.aws_region.alternate.name + } +} +`, rName)) +} + func testAccTableConfig_replicaCMK(rName string) string { return acctest.ConfigCompose( acctest.ConfigMultipleRegionProvider(3), // Prevent "Provider configuration not present" errors From 4b76251fdd42162c8e059bc71e70122a456c89f9 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 26 Jan 2023 22:25:24 -0500 Subject: [PATCH 12/17] dynamodb/table: Fix perpetual diffs with default keys --- internal/service/dynamodb/table.go | 75 +++++++++++++++++++++++++++--- 1 file changed, 68 insertions(+), 7 deletions(-) diff --git a/internal/service/dynamodb/table.go b/internal/service/dynamodb/table.go index d6ff37b32ec6..1ed6c2baf6a7 100644 --- a/internal/service/dynamodb/table.go +++ b/internal/service/dynamodb/table.go @@ -22,6 +22,7 @@ import ( "github.com/hashicorp/terraform-provider-aws/internal/create" "github.com/hashicorp/terraform-provider-aws/internal/errs/sdkdiag" "github.com/hashicorp/terraform-provider-aws/internal/flex" + "github.com/hashicorp/terraform-provider-aws/internal/service/kms" tftags "github.com/hashicorp/terraform-provider-aws/internal/tags" "github.com/hashicorp/terraform-provider-aws/internal/tfresource" "github.com/hashicorp/terraform-provider-aws/internal/verify" @@ -51,7 +52,6 @@ func ResourceTable() *schema.Resource { Update: schema.DefaultTimeout(updateTableTimeoutTotal), }, - //TODO: Add a custom diff if it is just the kms keys changing maybe check the replica update function? CustomizeDiff: customdiff.All( func(_ context.Context, diff *schema.ResourceDiff, meta interface{}) error { return validStreamSpec(diff) @@ -265,7 +265,7 @@ func ResourceTable() *schema.Resource { Optional: true, Computed: true, ValidateFunc: verify.ValidARN, - // update is equivalent of force a new *replica* + // update is equivalent of force a new *replica*, not table }, "point_in_time_recovery": { Type: schema.TypeBool, @@ -280,7 +280,7 @@ func ResourceTable() *schema.Resource { "region_name": { Type: schema.TypeString, Required: true, - // update is equivalent of force a new *replica* + // update is equivalent of force a new *replica*, not table }, }, }, @@ -653,7 +653,10 @@ func resourceTableRead(ctx context.Context, d *schema.ResourceData, meta interfa d.Set("stream_arn", table.LatestStreamArn) d.Set("stream_label", table.LatestStreamLabel) - if err := d.Set("server_side_encryption", flattenTableServerSideEncryption(table.SSEDescription)); err != nil { + sse := flattenTableServerSideEncryption(table.SSEDescription) + sse = clearSSEDefaultKey(ctx, sse, meta) + + if err := d.Set("server_side_encryption", sse); err != nil { return create.DiagSettingError(names.DynamoDB, ResNameTable, d.Id(), "server_side_encryption", err) } @@ -664,6 +667,7 @@ func resourceTableRead(ctx context.Context, d *schema.ResourceData, meta interfa } replicas = addReplicaTagPropagates(d.Get("replica").(*schema.Set), replicas) + replicas = clearReplicaDefaultKeys(ctx, replicas, meta) if err := d.Set("replica", replicas); err != nil { return create.DiagSettingError(names.DynamoDB, ResNameTable, d.Id(), "replica", err) @@ -1125,6 +1129,9 @@ func createReplicas(ctx context.Context, conn *dynamodb.DynamoDB, tableName stri if tfawserr.ErrMessageContains(err, dynamodb.ErrCodeLimitExceededException, "can be created, updated, or deleted simultaneously") { return resource.RetryableError(err) } + if tfawserr.ErrMessageContains(err, "ValidationException", "Replica specified in the Replica Update or Replica Delete action of the request was not found") { + return resource.RetryableError(err) + } if tfawserr.ErrCodeEquals(err, dynamodb.ErrCodeResourceInUseException) { return resource.RetryableError(err) } @@ -1280,9 +1287,9 @@ func updateReplica(ctx context.Context, d *schema.ResourceData, conn *dynamodb.D added := n.Difference(o).List() // 1. changing replica kms keys requires recreation of the replica, like ForceNew, but we don't want to ForceNew the *table* - // 2. also, in order to update PITR if a replica is encrypted (has KMS key), it requires recreation (how'd u restore from a backup encrypted with a different key?) + // 2. also, in order to update PITR if a replica is encrypted (has KMS key), it requires recreation (how'd u recover from a backup encrypted with a different key?) - var removeFirst []interface{} // replicas to delete before recreating (~ForceNew without recreating table) + var removeFirst []interface{} // replicas to delete before recreating (like ForceNew without recreating table) // For true updates, don't remove and add, just update (i.e., keep in added // but remove from removed) @@ -1566,7 +1573,6 @@ func replicaPITR(ctx context.Context, conn *dynamodb.DynamoDB, tableName string, func addReplicaPITRs(ctx context.Context, conn *dynamodb.DynamoDB, tableName string, tfVersion string, replicas []interface{}) ([]interface{}, error) { // This non-standard approach is needed because PITR info for a replica // must come from a region-specific connection. - for i, replicaRaw := range replicas { replica := replicaRaw.(map[string]interface{}) @@ -1613,6 +1619,61 @@ func addReplicaTagPropagates(configReplicas *schema.Set, replicas []interface{}) return replicas } +// clearSSEDefaultKey sets the kms_key_arn to "" if it is the default key alias/aws/dynamodb. +// Not clearing the key causes diff problems and sends the key to AWS when it should not be. +func clearSSEDefaultKey(ctx context.Context, sseList []interface{}, meta interface{}) []interface{} { + if len(sseList) == 0 { + return sseList + } + + sse := sseList[0].(map[string]interface{}) + + dk, err := kms.FindDefaultKey(ctx, "dynamodb", meta.(*conns.AWSClient).Region, meta) + if err != nil { + return sseList + } + + if sse["kms_key_arn"].(string) == dk { + sse["kms_key_arn"] = "" + return []interface{}{sse} + } + + return sseList +} + +// clearReplicaDefaultKeys sets a replica's kms_key_arn to "" if it is the default key alias/aws/dynamodb for +// the replica's region. Not clearing the key causes diff problems and sends the key to AWS when it should not be. +func clearReplicaDefaultKeys(ctx context.Context, replicas []interface{}, meta interface{}) []interface{} { + if len(replicas) == 0 { + return replicas + } + + for i, replicaRaw := range replicas { + replica := replicaRaw.(map[string]interface{}) + + if v, ok := replica["kms_key_arn"].(string); !ok || v == "" { + continue + } + + if v, ok := replica["region_name"].(string); !ok || v == "" { + continue + } + + dk, err := kms.FindDefaultKey(ctx, "dynamodb", replica["region_name"].(string), meta) + if err != nil { + continue + } + + if replica["kms_key_arn"].(string) == dk { + replica["kms_key_arn"] = "" + } + + replicas[i] = replica + } + + return replicas +} + // flatteners, expanders func flattenTableAttributeDefinitions(definitions []*dynamodb.AttributeDefinition) []interface{} { From ce215f9f01ed46550cbd4a1b1b80940dca81039f Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 26 Jan 2023 22:26:10 -0500 Subject: [PATCH 13/17] dynamodb/table_replica: Improve default key handling --- internal/service/dynamodb/table_replica.go | 23 +++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/internal/service/dynamodb/table_replica.go b/internal/service/dynamodb/table_replica.go index 320db9dff92a..a82e48d06eca 100644 --- a/internal/service/dynamodb/table_replica.go +++ b/internal/service/dynamodb/table_replica.go @@ -19,6 +19,7 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" "github.com/hashicorp/terraform-provider-aws/internal/conns" "github.com/hashicorp/terraform-provider-aws/internal/create" + "github.com/hashicorp/terraform-provider-aws/internal/service/kms" tftags "github.com/hashicorp/terraform-provider-aws/internal/tags" "github.com/hashicorp/terraform-provider-aws/internal/tfresource" "github.com/hashicorp/terraform-provider-aws/internal/verify" @@ -249,7 +250,16 @@ func resourceTableReplicaRead(ctx context.Context, d *schema.ResourceData, meta return create.DiagError(names.DynamoDB, create.ErrActionReading, ResNameTableReplica, d.Id(), err) } - d.Set("kms_key_arn", replica.KMSMasterKeyId) + dk, err := kms.FindDefaultKey(ctx, "dynamodb", replicaRegion, meta) + if err != nil { + return create.DiagError(names.DynamoDB, create.ErrActionReading, ResNameTableReplica, d.Id(), err) + } + + if replica.KMSMasterKeyId == nil || aws.StringValue(replica.KMSMasterKeyId) == dk { + d.Set("kms_key_arn", nil) + } else { + d.Set("kms_key_arn", replica.KMSMasterKeyId) + } if replica.ReplicaTableClassSummary != nil { d.Set("table_class_override", replica.ReplicaTableClassSummary.TableClass) @@ -372,8 +382,15 @@ func resourceTableReplicaUpdate(ctx context.Context, d *schema.ResourceData, met } if d.HasChange("kms_key_arn") && !d.IsNewResource() { // create ends with update and sets kms_key_arn causing change that is not - viaMainChanges = true - viaMainInput.KMSMasterKeyId = aws.String(d.Get("kms_key_arn").(string)) + dk, err := kms.FindDefaultKey(ctx, "dynamodb", replicaRegion, meta) + if err != nil { + return create.DiagError(names.DynamoDB, create.ErrActionUpdating, ResNameTableReplica, d.Id(), fmt.Errorf("region %s: %w", replicaRegion, err)) + } + + if d.Get("kms_key_arn").(string) != dk { + viaMainChanges = true + viaMainInput.KMSMasterKeyId = aws.String(d.Get("kms_key_arn").(string)) + } } if viaMainChanges { From b727762a69e7445666a0dcd8fdd385d98ccbb7b1 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 26 Jan 2023 22:30:05 -0500 Subject: [PATCH 14/17] Update changelog --- .changelog/29102.txt | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.changelog/29102.txt b/.changelog/29102.txt index d39248fd202a..4c78b9e49d2a 100644 --- a/.changelog/29102.txt +++ b/.changelog/29102.txt @@ -6,6 +6,10 @@ resource/aws_dynamodb_table: Fix to allow updating of `replica.*.point_in_time_r resource/aws_dynamodb_table: Fix to allow updating of `replica.*.kms_key_arn` ``` +```release-note:bug +resource/aws_dynamodb_table: Fix perpetual diffs when using default AWS-managed keys +``` + ```release-note:note resource/aws_dynamodb_table: Updating `replica.*.kms_key_arn` or `replica.*.point_in_time_recovery`, when the `replica`'s `kms_key_arn` is set, requires recreating the replica. ``` From 39c0c2e4eb21a542adb14d72cd2a3c9923c4c4c5 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 26 Jan 2023 22:40:46 -0500 Subject: [PATCH 15/17] dynamodb/table: Check diffs for #25812 --- internal/service/dynamodb/table_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/service/dynamodb/table_test.go b/internal/service/dynamodb/table_test.go index c1a35d6187dd..9b165543c2b8 100644 --- a/internal/service/dynamodb/table_test.go +++ b/internal/service/dynamodb/table_test.go @@ -1610,6 +1610,10 @@ func TestAccDynamoDBTable_Replica_single(t *testing.T) { resource.TestCheckResourceAttr(resourceName, "replica.#", "1"), ), }, + { + Config: testAccTableConfig_replica1(rName), + PlanOnly: true, + }, }, }) } From fa65b515225a0c887e2304d334ea9a05910f82d8 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 26 Jan 2023 22:45:12 -0500 Subject: [PATCH 16/17] Update changelog --- .changelog/29102.txt | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/.changelog/29102.txt b/.changelog/29102.txt index 4c78b9e49d2a..dfe70fa5b515 100644 --- a/.changelog/29102.txt +++ b/.changelog/29102.txt @@ -18,6 +18,18 @@ resource/aws_dynamodb_table: Updating `replica.*.kms_key_arn` or `replica.*.poin resource/aws_dynamodb_table_replica: Updating `kms_key_arn` forces replacement of the replica now as required to re-encrypt the replica ``` +```release-note:note +resource/aws_dynamodb_table: In the past, in certain situations, `server_side_encryption.0.kms_key_arn` or `replica.*.kms_key_arn` could be populated with the default DynamoDB key `alias/aws/dynamodb`. This was an error because it would then be sent back to AWS and should not be. +``` + +```release-note:note +resource/aws_dynamodb_table: In the past, in certain situations, `kms_key_arn` could be populated with the default DynamoDB key `alias/aws/dynamodb`. This was an error because it would then be sent back to AWS and should not be. +``` + +```release-note:note +resource/aws_dynamodb_table_replica: Updating `kms_key_arn` forces replacement of the replica now as required to re-encrypt the replica +``` + ```release-note:bug resource/aws_dynamodb_table_replica: Fix to allow updating of `kms_key_arn` ``` From 4a4154b197b0578e468efb3045ff384ce5d26dc2 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 26 Jan 2023 22:49:03 -0500 Subject: [PATCH 17/17] Lint --- internal/service/dynamodb/table_replica_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/service/dynamodb/table_replica_test.go b/internal/service/dynamodb/table_replica_test.go index abf62d3ebbd0..cc851840f39b 100644 --- a/internal/service/dynamodb/table_replica_test.go +++ b/internal/service/dynamodb/table_replica_test.go @@ -564,7 +564,7 @@ resource "aws_dynamodb_table" "test" { } server_side_encryption { - enabled = true + enabled = true } lifecycle {