From bee8f31ff973f36a9677cfab39c59881d73e9fc6 Mon Sep 17 00:00:00 2001 From: Stanley Halka Date: Wed, 4 Sep 2019 11:21:34 -0700 Subject: [PATCH 1/2] fix(diffDynamoDbGSI): ignore ordering of non_key_attributes in equality check to stop forced reconstruction of GSI [fixes ##3828] --- aws/resource_aws_dynamodb_table.go | 2 +- aws/resource_aws_dynamodb_table_test.go | 25 +++++++++- aws/structure.go | 63 ++++++++++++++++++++++++- 3 files changed, 87 insertions(+), 3 deletions(-) diff --git a/aws/resource_aws_dynamodb_table.go b/aws/resource_aws_dynamodb_table.go index f34d571e6e32..95b4158fb53d 100644 --- a/aws/resource_aws_dynamodb_table.go +++ b/aws/resource_aws_dynamodb_table.go @@ -208,7 +208,7 @@ func resourceAwsDynamoDbTable() *schema.Resource { Required: true, }, "non_key_attributes": { - Type: schema.TypeList, + Type: schema.TypeSet, Optional: true, Elem: &schema.Schema{Type: schema.TypeString}, }, diff --git a/aws/resource_aws_dynamodb_table_test.go b/aws/resource_aws_dynamodb_table_test.go index 6f1992a900e0..c1459d26f641 100644 --- a/aws/resource_aws_dynamodb_table_test.go +++ b/aws/resource_aws_dynamodb_table_test.go @@ -58,7 +58,7 @@ func TestDiffDynamoDbGSI(t *testing.T) { New []interface{} ExpectedUpdates []*dynamodb.GlobalSecondaryIndexUpdate }{ - { // No-op + { // No-op => no changes Old: []interface{}{ map[string]interface{}{ "name": "att1-index", @@ -79,6 +79,29 @@ func TestDiffDynamoDbGSI(t *testing.T) { }, ExpectedUpdates: []*dynamodb.GlobalSecondaryIndexUpdate{}, }, + { // No-op => ignore ordering of non_key_attributes + Old: []interface{}{ + map[string]interface{}{ + "name": "att1-index", + "hash_key": "att1", + "write_capacity": 10, + "read_capacity": 10, + "projection_type": "INCLUDE", + "non_key_attributes": []interface{}{"attr3", "attr1", "attr2"}, + }, + }, + New: []interface{}{ + map[string]interface{}{ + "name": "att1-index", + "hash_key": "att1", + "write_capacity": 10, + "read_capacity": 10, + "projection_type": "INCLUDE", + "non_key_attributes": []interface{}{"attr1", "attr2", "attr3"}, + }, + }, + ExpectedUpdates: []*dynamodb.GlobalSecondaryIndexUpdate{}, + }, { // Creation Old: []interface{}{ diff --git a/aws/structure.go b/aws/structure.go index 52eaa1dce597..26b66f89932a 100644 --- a/aws/structure.go +++ b/aws/structure.go @@ -4165,17 +4165,31 @@ func diffDynamoDbGSI(oldGsi, newGsi []interface{}, billingMode string) (ops []*d newWriteCapacity, newReadCapacity := newMap["write_capacity"].(int), newMap["read_capacity"].(int) capacityChanged := (oldWriteCapacity != newWriteCapacity || oldReadCapacity != newReadCapacity) + // pluck non_key_attributes from oldAttributes and newAttributes as reflect.DeepEquals will compare + // ordinal of elements in its equality (which we actually don't care about) + nonKeyAttributesChanged := checkIfNonKeyAttributesChanged(oldMap, newMap) + oldAttributes, err := stripCapacityAttributes(oldMap) if err != nil { e = err return } + oldAttributes, err = stripNonKeyAttributes(oldAttributes) + if err != nil { + e = err + return + } newAttributes, err := stripCapacityAttributes(newMap) if err != nil { e = err return } - otherAttributesChanged := !reflect.DeepEqual(oldAttributes, newAttributes) + newAttributes, err = stripNonKeyAttributes(newAttributes) + if err != nil { + e = err + return + } + otherAttributesChanged := nonKeyAttributesChanged || !reflect.DeepEqual(oldAttributes, newAttributes) if capacityChanged && !otherAttributesChanged { update := &dynamodb.GlobalSecondaryIndexUpdate{ @@ -4214,6 +4228,49 @@ func diffDynamoDbGSI(oldGsi, newGsi []interface{}, billingMode string) (ops []*d return } +func stripNonKeyAttributes(in map[string]interface{}) (map[string]interface{}, error) { + mapCopy, err := copystructure.Copy(in) + if err != nil { + return nil, err + } + + m := mapCopy.(map[string]interface{}) + + delete(m, "non_key_attributes") + + return m, nil +} + +// checkIfNonKeyAttributesChanged returns true if non_key_attributes between old map and new map are different +func checkIfNonKeyAttributesChanged(oldMap, newMap map[string]interface{}) bool { + oldNonKeyAttributes, oldNkaExists := oldMap["non_key_attributes"].([]string) + newNonKeyAttributes, newNkaExists := newMap["non_key_attributes"].([]string) + if oldNkaExists != newNkaExists { + return true + } + + o := map[string]bool{} + for _, oldNonKeyAttribute := range oldNonKeyAttributes { + o[oldNonKeyAttribute] = true + } + n := map[string]bool{} + for _, newNonKeyAttribute := range newNonKeyAttributes { + n[newNonKeyAttribute] = true + } + + // perform this check after populating map so we ignore duplicated fields in non_key_attributes + if len(o) != len(n) { + return true + } + + for k, v := range n { + if oVal, oExists := o[k]; !oExists || v != oVal { + return true + } + } + return false +} + func stripCapacityAttributes(in map[string]interface{}) (map[string]interface{}, error) { mapCopy, err := copystructure.Copy(in) if err != nil { @@ -4474,6 +4531,10 @@ func expandDynamoDbProjection(data map[string]interface{}) *dynamodb.Projection projection.NonKeyAttributes = expandStringList(v) } + if v, ok := data["non_key_attributes"].(*schema.Set); ok && v.Len() > 0 { + projection.NonKeyAttributes = expandStringList(v.List()) + } + return projection } From 32a1e66b2e943dfe6b630dcce06322637b4bdb44 Mon Sep 17 00:00:00 2001 From: Angie Pinilla Date: Tue, 1 Sep 2020 12:16:28 -0400 Subject: [PATCH 2/2] refactor non-key attributes difference method and acctests --- aws/resource_aws_dynamodb_table_test.go | 506 ++++++++++-------------- aws/structure.go | 32 +- 2 files changed, 206 insertions(+), 332 deletions(-) diff --git a/aws/resource_aws_dynamodb_table_test.go b/aws/resource_aws_dynamodb_table_test.go index 8d4399fbe475..129dae441fd5 100644 --- a/aws/resource_aws_dynamodb_table_test.go +++ b/aws/resource_aws_dynamodb_table_test.go @@ -5,7 +5,6 @@ import ( "log" "regexp" "testing" - "time" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/awserr" @@ -89,7 +88,7 @@ func TestDiffDynamoDbGSI(t *testing.T) { "write_capacity": 10, "read_capacity": 10, "projection_type": "INCLUDE", - "non_key_attributes": []interface{}{"attr3", "attr1", "attr2"}, + "non_key_attributes": schema.NewSet(schema.HashString, []interface{}{"attr3", "attr1", "attr2"}), }, }, New: []interface{}{ @@ -99,7 +98,7 @@ func TestDiffDynamoDbGSI(t *testing.T) { "write_capacity": 10, "read_capacity": 10, "projection_type": "INCLUDE", - "non_key_attributes": []interface{}{"attr1", "attr2", "attr3"}, + "non_key_attributes": schema.NewSet(schema.HashString, []interface{}{"attr1", "attr2", "attr3"}), }, }, ExpectedUpdates: []*dynamodb.GlobalSecondaryIndexUpdate{}, @@ -238,7 +237,7 @@ func TestDiffDynamoDbGSI(t *testing.T) { "write_capacity": 10, "read_capacity": 10, "projection_type": "KEYS_ONLY", - "non_key_attributes": []interface{}{"RandomAttribute"}, + "non_key_attributes": schema.NewSet(schema.HashString, []interface{}{"RandomAttribute"}), }, }, ExpectedUpdates: []*dynamodb.GlobalSecondaryIndexUpdate{ @@ -291,7 +290,7 @@ func TestDiffDynamoDbGSI(t *testing.T) { "write_capacity": 12, "read_capacity": 12, "projection_type": "INCLUDE", - "non_key_attributes": []interface{}{"RandomAttribute"}, + "non_key_attributes": schema.NewSet(schema.HashString, []interface{}{"RandomAttribute"}), }, }, ExpectedUpdates: []*dynamodb.GlobalSecondaryIndexUpdate{ @@ -392,7 +391,7 @@ func TestAccAWSDynamoDbTable_disappears(t *testing.T) { Config: testAccAWSDynamoDbConfig_basic(rName), Check: resource.ComposeTestCheckFunc( testAccCheckInitialAWSDynamoDbTableExists(resourceName, &table1), - testAccCheckAWSDynamoDbTableDisappears(&table1), + testAccCheckResourceDisappears(testAccProvider, resourceAwsDynamoDbTable(), resourceName), ), ExpectNonEmptyPlan: true, }, @@ -414,7 +413,7 @@ func TestAccAWSDynamoDbTable_disappears_PayPerRequestWithGSI(t *testing.T) { Config: testAccAWSDynamoDbBilling_PayPerRequestWithGSI(rName), Check: resource.ComposeTestCheckFunc( testAccCheckInitialAWSDynamoDbTableExists(resourceName, &table1), - testAccCheckAWSDynamoDbTableDisappears(&table1), + testAccCheckResourceDisappears(testAccProvider, resourceAwsDynamoDbTable(), resourceName), ), ExpectNonEmptyPlan: true, }, @@ -458,7 +457,48 @@ func TestAccAWSDynamoDbTable_extended(t *testing.T) { { Config: testAccAWSDynamoDbConfigAddSecondaryGSI(rName), Check: resource.ComposeTestCheckFunc( - testAccCheckDynamoDbTableWasUpdated(resourceName), + testAccCheckInitialAWSDynamoDbTableExists(resourceName, &conf), + resource.TestCheckResourceAttr(resourceName, "name", rName), + resource.TestCheckResourceAttr(resourceName, "hash_key", "TestTableHashKey"), + resource.TestCheckResourceAttr(resourceName, "range_key", "TestTableRangeKey"), + resource.TestCheckResourceAttr(resourceName, "billing_mode", dynamodb.BillingModeProvisioned), + resource.TestCheckResourceAttr(resourceName, "write_capacity", "2"), + resource.TestCheckResourceAttr(resourceName, "read_capacity", "2"), + resource.TestCheckResourceAttr(resourceName, "server_side_encryption.#", "0"), + resource.TestCheckResourceAttr(resourceName, "attribute.#", "4"), + resource.TestCheckResourceAttr(resourceName, "global_secondary_index.#", "1"), + resource.TestCheckResourceAttr(resourceName, "local_secondary_index.#", "1"), + tfawsresource.TestCheckTypeSetElemNestedAttrs(resourceName, "attribute.*", map[string]string{ + "name": "TestTableHashKey", + "type": "S", + }), + tfawsresource.TestCheckTypeSetElemNestedAttrs(resourceName, "attribute.*", map[string]string{ + "name": "TestTableRangeKey", + "type": "S", + }), + tfawsresource.TestCheckTypeSetElemNestedAttrs(resourceName, "attribute.*", map[string]string{ + "name": "TestLSIRangeKey", + "type": "N", + }), + tfawsresource.TestCheckTypeSetElemNestedAttrs(resourceName, "attribute.*", map[string]string{ + "name": "ReplacementGSIRangeKey", + "type": "N", + }), + tfawsresource.TestCheckTypeSetElemNestedAttrs(resourceName, "global_secondary_index.*", map[string]string{ + "name": "ReplacementTestTableGSI", + "hash_key": "TestTableHashKey", + "range_key": "ReplacementGSIRangeKey", + "write_capacity": "5", + "read_capacity": "5", + "projection_type": "INCLUDE", + "non_key_attributes.#": "1", + }), + tfawsresource.TestCheckTypeSetElemAttr(resourceName, "global_secondary_index.*.non_key_attributes.*", "TestNonKeyAttribute"), + tfawsresource.TestCheckTypeSetElemNestedAttrs(resourceName, "local_secondary_index.*", map[string]string{ + "name": "TestTableLSI", + "range_key": "TestLSIRangeKey", + "projection_type": "ALL", + }), ), }, }, @@ -490,7 +530,7 @@ func TestAccAWSDynamoDbTable_enablePitr(t *testing.T) { { Config: testAccAWSDynamoDbConfig_backup(rName), Check: resource.ComposeTestCheckFunc( - testAccCheckDynamoDbTableHasPointInTimeRecoveryEnabled(resourceName), + testAccCheckInitialAWSDynamoDbTableExists(resourceName, &conf), resource.TestCheckResourceAttr(resourceName, "point_in_time_recovery.#", "1"), resource.TestCheckResourceAttr(resourceName, "point_in_time_recovery.0.enabled", "true"), ), @@ -500,6 +540,7 @@ func TestAccAWSDynamoDbTable_enablePitr(t *testing.T) { } func TestAccAWSDynamoDbTable_BillingMode_PayPerRequestToProvisioned(t *testing.T) { + var conf dynamodb.DescribeTableOutput resourceName := "aws_dynamodb_table.test" rName := acctest.RandomWithPrefix("TerraformTestTable-") @@ -511,7 +552,8 @@ func TestAccAWSDynamoDbTable_BillingMode_PayPerRequestToProvisioned(t *testing.T { Config: testAccAWSDynamoDbBilling_PayPerRequest(rName), Check: resource.ComposeTestCheckFunc( - testAccCheckDynamoDbTableHasBilling_PayPerRequest(resourceName), + testAccCheckInitialAWSDynamoDbTableExists(resourceName, &conf), + resource.TestCheckResourceAttr(resourceName, "billing_mode", dynamodb.BillingModePayPerRequest), ), }, { @@ -522,7 +564,8 @@ func TestAccAWSDynamoDbTable_BillingMode_PayPerRequestToProvisioned(t *testing.T { Config: testAccAWSDynamoDbBilling_Provisioned(rName), Check: resource.ComposeTestCheckFunc( - testAccCheckDynamoDbTableHasBilling_Provisioned(resourceName), + testAccCheckInitialAWSDynamoDbTableExists(resourceName, &conf), + resource.TestCheckResourceAttr(resourceName, "billing_mode", dynamodb.BillingModeProvisioned), ), }, }, @@ -530,6 +573,7 @@ func TestAccAWSDynamoDbTable_BillingMode_PayPerRequestToProvisioned(t *testing.T } func TestAccAWSDynamoDbTable_BillingMode_ProvisionedToPayPerRequest(t *testing.T) { + var conf dynamodb.DescribeTableOutput resourceName := "aws_dynamodb_table.test" rName := acctest.RandomWithPrefix("TerraformTestTable-") @@ -541,7 +585,8 @@ func TestAccAWSDynamoDbTable_BillingMode_ProvisionedToPayPerRequest(t *testing.T { Config: testAccAWSDynamoDbBilling_Provisioned(rName), Check: resource.ComposeTestCheckFunc( - testAccCheckDynamoDbTableHasBilling_Provisioned(resourceName), + testAccCheckInitialAWSDynamoDbTableExists(resourceName, &conf), + resource.TestCheckResourceAttr(resourceName, "billing_mode", dynamodb.BillingModeProvisioned), ), }, { @@ -552,7 +597,8 @@ func TestAccAWSDynamoDbTable_BillingMode_ProvisionedToPayPerRequest(t *testing.T { Config: testAccAWSDynamoDbBilling_PayPerRequest(rName), Check: resource.ComposeTestCheckFunc( - testAccCheckDynamoDbTableHasBilling_PayPerRequest(resourceName), + testAccCheckInitialAWSDynamoDbTableExists(resourceName, &conf), + resource.TestCheckResourceAttr(resourceName, "billing_mode", dynamodb.BillingModePayPerRequest), ), }, }, @@ -560,6 +606,7 @@ func TestAccAWSDynamoDbTable_BillingMode_ProvisionedToPayPerRequest(t *testing.T } func TestAccAWSDynamoDbTable_BillingMode_GSI_PayPerRequestToProvisioned(t *testing.T) { + var conf dynamodb.DescribeTableOutput resourceName := "aws_dynamodb_table.test" rName := acctest.RandomWithPrefix("TerraformTestTable-") @@ -571,7 +618,8 @@ func TestAccAWSDynamoDbTable_BillingMode_GSI_PayPerRequestToProvisioned(t *testi { Config: testAccAWSDynamoDbBilling_PayPerRequestWithGSI(rName), Check: resource.ComposeTestCheckFunc( - testAccCheckDynamoDbTableHasBilling_PayPerRequest(resourceName), + testAccCheckInitialAWSDynamoDbTableExists(resourceName, &conf), + resource.TestCheckResourceAttr(resourceName, "billing_mode", dynamodb.BillingModePayPerRequest), ), }, { @@ -582,7 +630,8 @@ func TestAccAWSDynamoDbTable_BillingMode_GSI_PayPerRequestToProvisioned(t *testi { Config: testAccAWSDynamoDbBilling_ProvisionedWithGSI(rName), Check: resource.ComposeTestCheckFunc( - testAccCheckDynamoDbTableHasBilling_Provisioned(resourceName), + testAccCheckInitialAWSDynamoDbTableExists(resourceName, &conf), + resource.TestCheckResourceAttr(resourceName, "billing_mode", dynamodb.BillingModeProvisioned), ), }, }, @@ -590,6 +639,7 @@ func TestAccAWSDynamoDbTable_BillingMode_GSI_PayPerRequestToProvisioned(t *testi } func TestAccAWSDynamoDbTable_BillingMode_GSI_ProvisionedToPayPerRequest(t *testing.T) { + var conf dynamodb.DescribeTableOutput resourceName := "aws_dynamodb_table.test" rName := acctest.RandomWithPrefix("TerraformTestTable-") @@ -601,7 +651,8 @@ func TestAccAWSDynamoDbTable_BillingMode_GSI_ProvisionedToPayPerRequest(t *testi { Config: testAccAWSDynamoDbBilling_ProvisionedWithGSI(rName), Check: resource.ComposeTestCheckFunc( - testAccCheckDynamoDbTableHasBilling_Provisioned(resourceName), + testAccCheckInitialAWSDynamoDbTableExists(resourceName, &conf), + resource.TestCheckResourceAttr(resourceName, "billing_mode", dynamodb.BillingModeProvisioned), ), }, { @@ -612,7 +663,8 @@ func TestAccAWSDynamoDbTable_BillingMode_GSI_ProvisionedToPayPerRequest(t *testi { Config: testAccAWSDynamoDbBilling_PayPerRequestWithGSI(rName), Check: resource.ComposeTestCheckFunc( - testAccCheckDynamoDbTableHasBilling_PayPerRequest(resourceName), + testAccCheckInitialAWSDynamoDbTableExists(resourceName, &conf), + resource.TestCheckResourceAttr(resourceName, "billing_mode", dynamodb.BillingModePayPerRequest), ), }, }, @@ -635,7 +687,7 @@ func TestAccAWSDynamoDbTable_streamSpecification(t *testing.T) { testAccCheckInitialAWSDynamoDbTableExists(resourceName, &conf), resource.TestCheckResourceAttr(resourceName, "stream_enabled", "true"), resource.TestCheckResourceAttr(resourceName, "stream_view_type", "KEYS_ONLY"), - resource.TestCheckResourceAttrSet(resourceName, "stream_arn"), + testAccMatchResourceAttrRegionalARN(resourceName, "stream_arn", "dynamodb", regexp.MustCompile(fmt.Sprintf("table/%s/stream", tableName))), resource.TestCheckResourceAttrSet(resourceName, "stream_label"), ), }, @@ -650,7 +702,7 @@ func TestAccAWSDynamoDbTable_streamSpecification(t *testing.T) { testAccCheckInitialAWSDynamoDbTableExists(resourceName, &conf), resource.TestCheckResourceAttr(resourceName, "stream_enabled", "false"), resource.TestCheckResourceAttr(resourceName, "stream_view_type", ""), - resource.TestCheckResourceAttrSet(resourceName, "stream_arn"), + testAccMatchResourceAttrRegionalARN(resourceName, "stream_arn", "dynamodb", regexp.MustCompile(fmt.Sprintf("table/%s/stream", tableName))), resource.TestCheckResourceAttrSet(resourceName, "stream_label"), ), }, @@ -686,8 +738,7 @@ func TestAccAWSDynamoDbTable_tags(t *testing.T) { Check: resource.ComposeTestCheckFunc( testAccCheckInitialAWSDynamoDbTableExists(resourceName, &conf), testAccCheckInitialAWSDynamoDbTableConf(resourceName), - resource.TestCheckResourceAttr( - resourceName, "tags.%", "3"), + resource.TestCheckResourceAttr(resourceName, "tags.%", "3"), ), }, { @@ -830,12 +881,12 @@ func TestAccAWSDynamoDbTable_gsiUpdateOtherAttributes(t *testing.T) { "hash_key": "att3", "name": "att3-index", "non_key_attributes.#": "1", - "non_key_attributes.0": "RandomAttribute", "projection_type": "INCLUDE", "range_key": "att4", "read_capacity": "1", "write_capacity": "1", }), + tfawsresource.TestCheckTypeSetElemAttr(resourceName, "global_secondary_index.*.non_key_attributes.*", "RandomAttribute"), tfawsresource.TestCheckTypeSetElemNestedAttrs(resourceName, "global_secondary_index.*", map[string]string{ "hash_key": "att1", "name": "att1-index", @@ -880,12 +931,12 @@ func TestAccAWSDynamoDbTable_gsiUpdateNonKeyAttributes(t *testing.T) { "hash_key": "att3", "name": "att3-index", "non_key_attributes.#": "1", - "non_key_attributes.0": "RandomAttribute", "projection_type": "INCLUDE", "range_key": "att4", "read_capacity": "1", "write_capacity": "1", }), + tfawsresource.TestCheckTypeSetElemAttr(resourceName, "global_secondary_index.*.non_key_attributes.*", "RandomAttribute"), tfawsresource.TestCheckTypeSetElemNestedAttrs(resourceName, "global_secondary_index.*", map[string]string{ "hash_key": "att1", "name": "att1-index", @@ -919,13 +970,13 @@ func TestAccAWSDynamoDbTable_gsiUpdateNonKeyAttributes(t *testing.T) { "hash_key": "att3", "name": "att3-index", "non_key_attributes.#": "2", - "non_key_attributes.0": "RandomAttribute", - "non_key_attributes.1": "AnotherAttribute", "projection_type": "INCLUDE", "range_key": "att4", "read_capacity": "1", "write_capacity": "1", }), + tfawsresource.TestCheckTypeSetElemAttr(resourceName, "global_secondary_index.*.non_key_attributes.*", "RandomAttribute"), + tfawsresource.TestCheckTypeSetElemAttr(resourceName, "global_secondary_index.*.non_key_attributes.*", "AnotherAttribute"), tfawsresource.TestCheckTypeSetElemNestedAttrs(resourceName, "global_secondary_index.*", map[string]string{ "hash_key": "att1", "name": "att1-index", @@ -941,6 +992,50 @@ func TestAccAWSDynamoDbTable_gsiUpdateNonKeyAttributes(t *testing.T) { }) } +// https://github.com/terraform-providers/terraform-provider-aws/issues/671 +func TestAccAWSDynamoDbTable_gsiUpdateNonKeyAttributes_emptyPlan(t *testing.T) { + var conf dynamodb.DescribeTableOutput + resourceName := "aws_dynamodb_table.test" + name := acctest.RandString(10) + attributes := fmt.Sprintf("%q, %q", "AnotherAttribute", "RandomAttribute") + reorderedAttributes := fmt.Sprintf("%q, %q", "RandomAttribute", "AnotherAttribute") + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSDynamoDbTableDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAWSDynamoDbConfigGsiMultipleNonKeyAttributes(name, attributes), + Check: resource.ComposeTestCheckFunc( + testAccCheckInitialAWSDynamoDbTableExists(resourceName, &conf), + tfawsresource.TestCheckTypeSetElemNestedAttrs(resourceName, "global_secondary_index.*", map[string]string{ + "hash_key": "att1", + "name": "att1-index", + "non_key_attributes.#": "2", + "projection_type": "INCLUDE", + "range_key": "att2", + "read_capacity": "1", + "write_capacity": "1", + }), + tfawsresource.TestCheckTypeSetElemAttr(resourceName, "global_secondary_index.*.non_key_attributes.*", "AnotherAttribute"), + tfawsresource.TestCheckTypeSetElemAttr(resourceName, "global_secondary_index.*.non_key_attributes.*", "RandomAttribute"), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + { + Config: testAccAWSDynamoDbConfigGsiMultipleNonKeyAttributes(name, reorderedAttributes), + 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 TestAccAWSDynamoDbTable_Ttl_Enabled(t *testing.T) { @@ -1188,266 +1283,47 @@ func testAccCheckInitialAWSDynamoDbTableExists(n string, table *dynamodb.Describ } } -func testAccCheckInitialAWSDynamoDbTableConf(n string) resource.TestCheckFunc { - return func(s *terraform.State) error { - log.Printf("[DEBUG] Trying to create initial table state!") - rs, ok := s.RootModule().Resources[n] - if !ok { - return fmt.Errorf("Not found: %s", n) - } - - if rs.Primary.ID == "" { - return fmt.Errorf("No DynamoDB table name specified!") - } - - conn := testAccProvider.Meta().(*AWSClient).dynamodbconn - - params := &dynamodb.DescribeTableInput{ - TableName: aws.String(rs.Primary.ID), - } - - resp, err := conn.DescribeTable(params) - - if err != nil { - return fmt.Errorf("Problem describing table '%s': %s", rs.Primary.ID, err) - } - - table := resp.Table - - log.Printf("[DEBUG] Checking on table %s", rs.Primary.ID) - - if table.BillingModeSummary != nil && aws.StringValue(table.BillingModeSummary.BillingMode) != dynamodb.BillingModeProvisioned { - return fmt.Errorf("Billing Mode was %s, not %s!", aws.StringValue(table.BillingModeSummary.BillingMode), dynamodb.BillingModeProvisioned) - } - - if *table.ProvisionedThroughput.WriteCapacityUnits != 2 { - return fmt.Errorf("Provisioned write capacity was %d, not 2!", table.ProvisionedThroughput.WriteCapacityUnits) - } - - if *table.ProvisionedThroughput.ReadCapacityUnits != 1 { - return fmt.Errorf("Provisioned read capacity was %d, not 1!", table.ProvisionedThroughput.ReadCapacityUnits) - } - - if table.SSEDescription != nil && *table.SSEDescription.Status != dynamodb.SSEStatusDisabled { - return fmt.Errorf("SSE status was %s, not %s", *table.SSEDescription.Status, dynamodb.SSEStatusDisabled) - } - - attrCount := len(table.AttributeDefinitions) - gsiCount := len(table.GlobalSecondaryIndexes) - lsiCount := len(table.LocalSecondaryIndexes) - - if attrCount != 4 { - return fmt.Errorf("There were %d attributes, not 4 like there should have been!", attrCount) - } - - if gsiCount != 1 { - return fmt.Errorf("There were %d GSIs, not 1 like there should have been!", gsiCount) - } - - if lsiCount != 1 { - return fmt.Errorf("There were %d LSIs, not 1 like there should have been!", lsiCount) - } - - attrmap := dynamoDbAttributesToMap(&table.AttributeDefinitions) - if attrmap["TestTableHashKey"] != "S" { - return fmt.Errorf("Test table hash key was of type %s instead of S!", attrmap["TestTableHashKey"]) - } - if attrmap["TestTableRangeKey"] != "S" { - return fmt.Errorf("Test table range key was of type %s instead of S!", attrmap["TestTableRangeKey"]) - } - if attrmap["TestLSIRangeKey"] != "N" { - return fmt.Errorf("Test table LSI range key was of type %s instead of N!", attrmap["TestLSIRangeKey"]) - } - if attrmap["TestGSIRangeKey"] != "S" { - return fmt.Errorf("Test table GSI range key was of type %s instead of S!", attrmap["TestGSIRangeKey"]) - } - - return nil - } -} - -func testAccCheckDynamoDbTableHasPointInTimeRecoveryEnabled(n string) resource.TestCheckFunc { - return func(s *terraform.State) error { - rs, ok := s.RootModule().Resources[n] - if !ok { - return fmt.Errorf("Not found: %s", n) - } - - if rs.Primary.ID == "" { - return fmt.Errorf("No DynamoDB table name specified!") - } - - conn := testAccProvider.Meta().(*AWSClient).dynamodbconn - - resp, err := conn.DescribeContinuousBackups(&dynamodb.DescribeContinuousBackupsInput{ - TableName: aws.String(rs.Primary.ID), - }) - - if err != nil { - return err - } - - pitr := resp.ContinuousBackupsDescription.PointInTimeRecoveryDescription - status := *pitr.PointInTimeRecoveryStatus - if status != dynamodb.PointInTimeRecoveryStatusEnabled { - return fmt.Errorf("Point in time backup had a status of %s rather than enabled", status) - } - - return nil - } -} - -func testAccCheckDynamoDbTableHasBilling_PayPerRequest(n string) resource.TestCheckFunc { - return func(s *terraform.State) error { - rs, ok := s.RootModule().Resources[n] - if !ok { - return fmt.Errorf("Not found: %s", n) - } - - if rs.Primary.ID == "" { - return fmt.Errorf("No DynamoDB table name specified!") - } - - conn := testAccProvider.Meta().(*AWSClient).dynamodbconn - params := &dynamodb.DescribeTableInput{ - TableName: aws.String(rs.Primary.ID), - } - resp, err := conn.DescribeTable(params) - - if err != nil { - return err - } - table := resp.Table - - if table.BillingModeSummary == nil { - return fmt.Errorf("Billing Mode summary was empty, expected summary to exist and contain billing mode %s", dynamodb.BillingModePayPerRequest) - } else if aws.StringValue(table.BillingModeSummary.BillingMode) != dynamodb.BillingModePayPerRequest { - return fmt.Errorf("Billing Mode was %s, not %s!", aws.StringValue(table.BillingModeSummary.BillingMode), dynamodb.BillingModePayPerRequest) - - } - - return nil - } -} - -func testAccCheckDynamoDbTableHasBilling_Provisioned(n string) resource.TestCheckFunc { - return func(s *terraform.State) error { - rs, ok := s.RootModule().Resources[n] - if !ok { - return fmt.Errorf("Not found: %s", n) - } - - if rs.Primary.ID == "" { - return fmt.Errorf("No DynamoDB table name specified!") - } - - conn := testAccProvider.Meta().(*AWSClient).dynamodbconn - params := &dynamodb.DescribeTableInput{ - TableName: aws.String(rs.Primary.ID), - } - resp, err := conn.DescribeTable(params) - - if err != nil { - return err - } - table := resp.Table - - // DynamoDB can omit BillingModeSummary for tables created as PROVISIONED - if table.BillingModeSummary != nil && aws.StringValue(table.BillingModeSummary.BillingMode) != dynamodb.BillingModeProvisioned { - return fmt.Errorf("Billing Mode was %s, not %s!", aws.StringValue(table.BillingModeSummary.BillingMode), dynamodb.BillingModeProvisioned) - - } - - return nil - } -} - -func testAccCheckDynamoDbTableWasUpdated(n string) resource.TestCheckFunc { - return func(s *terraform.State) error { - rs, ok := s.RootModule().Resources[n] - if !ok { - return fmt.Errorf("Not found: %s", n) - } - - if rs.Primary.ID == "" { - return fmt.Errorf("No DynamoDB table name specified!") - } - - conn := testAccProvider.Meta().(*AWSClient).dynamodbconn - - params := &dynamodb.DescribeTableInput{ - TableName: aws.String(rs.Primary.ID), - } - resp, err := conn.DescribeTable(params) - table := resp.Table - - if err != nil { - return err - } - - attrCount := len(table.AttributeDefinitions) - gsiCount := len(table.GlobalSecondaryIndexes) - lsiCount := len(table.LocalSecondaryIndexes) - - if attrCount != 4 { - return fmt.Errorf("There were %d attributes, not 4 like there should have been!", attrCount) - } - - if gsiCount != 1 { - return fmt.Errorf("There were %d GSIs, not 1 like there should have been!", gsiCount) - } - - if lsiCount != 1 { - return fmt.Errorf("There were %d LSIs, not 1 like there should have been!", lsiCount) - } - - if dynamoDbGetGSIIndex(&table.GlobalSecondaryIndexes, "ReplacementTestTableGSI") == -1 { - return fmt.Errorf("Could not find GSI named 'ReplacementTestTableGSI' in the table!") - } - - if dynamoDbGetGSIIndex(&table.GlobalSecondaryIndexes, "InitialTestTableGSI") != -1 { - return fmt.Errorf("Should have removed 'InitialTestTableGSI' but it still exists!") - } - - attrmap := dynamoDbAttributesToMap(&table.AttributeDefinitions) - if attrmap["TestTableHashKey"] != "S" { - return fmt.Errorf("Test table hash key was of type %s instead of S!", attrmap["TestTableHashKey"]) - } - if attrmap["TestTableRangeKey"] != "S" { - return fmt.Errorf("Test table range key was of type %s instead of S!", attrmap["TestTableRangeKey"]) - } - if attrmap["TestLSIRangeKey"] != "N" { - return fmt.Errorf("Test table LSI range key was of type %s instead of N!", attrmap["TestLSIRangeKey"]) - } - if attrmap["ReplacementGSIRangeKey"] != "N" { - return fmt.Errorf("Test table replacement GSI range key was of type %s instead of N!", attrmap["ReplacementGSIRangeKey"]) - } - - return nil - } -} - -func testAccCheckAWSDynamoDbTableDisappears(table *dynamodb.DescribeTableOutput) resource.TestCheckFunc { - return func(s *terraform.State) error { - conn := testAccProvider.Meta().(*AWSClient).dynamodbconn - tableName := aws.StringValue(table.Table.TableName) - - input := &dynamodb.DeleteTableInput{ - TableName: table.Table.TableName, - } - - _, err := conn.DeleteTable(input) - - if err != nil { - return fmt.Errorf("error deleting DynamoDB Table (%s): %s", tableName, err) - } - - if err := waitForDynamodbTableDeletion(conn, tableName, 10*time.Minute); err != nil { - return fmt.Errorf("error waiting for DynamoDB Table (%s) deletion: %s", tableName, err) - } - - return nil - } +func testAccCheckInitialAWSDynamoDbTableConf(resourceName string) resource.TestCheckFunc { + return resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr(resourceName, "hash_key", "TestTableHashKey"), + resource.TestCheckResourceAttr(resourceName, "range_key", "TestTableRangeKey"), + resource.TestCheckResourceAttr(resourceName, "billing_mode", dynamodb.BillingModeProvisioned), + resource.TestCheckResourceAttr(resourceName, "write_capacity", "2"), + resource.TestCheckResourceAttr(resourceName, "read_capacity", "1"), + resource.TestCheckResourceAttr(resourceName, "server_side_encryption.#", "0"), + resource.TestCheckResourceAttr(resourceName, "attribute.#", "4"), + resource.TestCheckResourceAttr(resourceName, "global_secondary_index.#", "1"), + resource.TestCheckResourceAttr(resourceName, "local_secondary_index.#", "1"), + tfawsresource.TestCheckTypeSetElemNestedAttrs(resourceName, "attribute.*", map[string]string{ + "name": "TestTableHashKey", + "type": "S", + }), + tfawsresource.TestCheckTypeSetElemNestedAttrs(resourceName, "attribute.*", map[string]string{ + "name": "TestTableRangeKey", + "type": "S", + }), + tfawsresource.TestCheckTypeSetElemNestedAttrs(resourceName, "attribute.*", map[string]string{ + "name": "TestLSIRangeKey", + "type": "N", + }), + tfawsresource.TestCheckTypeSetElemNestedAttrs(resourceName, "attribute.*", map[string]string{ + "name": "TestGSIRangeKey", + "type": "S", + }), + tfawsresource.TestCheckTypeSetElemNestedAttrs(resourceName, "global_secondary_index.*", map[string]string{ + "name": "InitialTestTableGSI", + "hash_key": "TestTableHashKey", + "range_key": "TestGSIRangeKey", + "write_capacity": "1", + "read_capacity": "1", + "projection_type": "KEYS_ONLY", + }), + tfawsresource.TestCheckTypeSetElemNestedAttrs(resourceName, "local_secondary_index.*", map[string]string{ + "name": "TestTableLSI", + "range_key": "TestLSIRangeKey", + "projection_type": "ALL", + }), + ) } func TestAccAWSDynamoDbTable_Replica_Multiple(t *testing.T) { @@ -1540,26 +1416,6 @@ func TestAccAWSDynamoDbTable_Replica_Single(t *testing.T) { }) } -func dynamoDbGetGSIIndex(gsiList *[]*dynamodb.GlobalSecondaryIndexDescription, target string) int { - for idx, gsiObject := range *gsiList { - if *gsiObject.IndexName == target { - return idx - } - } - - return -1 -} - -func dynamoDbAttributesToMap(attributes *[]*dynamodb.AttributeDefinition) map[string]string { - attrmap := make(map[string]string) - - for _, attrdef := range *attributes { - attrmap[*attrdef.AttributeName] = *attrdef.AttributeType - } - - return attrmap -} - func testAccAWSDynamoDbConfig_basic(rName string) string { return fmt.Sprintf(` resource "aws_dynamodb_table" "test" { @@ -2157,6 +2013,46 @@ resource "aws_dynamodb_table" "test" { `, name) } +func testAccAWSDynamoDbConfigGsiMultipleNonKeyAttributes(name, attributes string) string { + return fmt.Sprintf(` +variable "capacity" { + default = 1 +} + +resource "aws_dynamodb_table" "test" { + name = "tf-acc-test-%s" + read_capacity = var.capacity + write_capacity = var.capacity + hash_key = "id" + + attribute { + name = "id" + type = "S" + } + + attribute { + name = "att1" + type = "S" + } + + attribute { + name = "att2" + type = "S" + } + + global_secondary_index { + name = "att1-index" + hash_key = "att1" + range_key = "att2" + write_capacity = var.capacity + read_capacity = var.capacity + projection_type = "INCLUDE" + non_key_attributes = [%s] + } +} +`, name, attributes) +} + func testAccAWSDynamoDbConfigTimeToLive(rName string, ttlEnabled bool) string { return fmt.Sprintf(` resource "aws_dynamodb_table" "test" { diff --git a/aws/structure.go b/aws/structure.go index 7cb877eee2e8..c28ba23e1662 100644 --- a/aws/structure.go +++ b/aws/structure.go @@ -3957,32 +3957,14 @@ func stripNonKeyAttributes(in map[string]interface{}) (map[string]interface{}, e // checkIfNonKeyAttributesChanged returns true if non_key_attributes between old map and new map are different func checkIfNonKeyAttributesChanged(oldMap, newMap map[string]interface{}) bool { - oldNonKeyAttributes, oldNkaExists := oldMap["non_key_attributes"].([]string) - newNonKeyAttributes, newNkaExists := newMap["non_key_attributes"].([]string) - if oldNkaExists != newNkaExists { - return true - } + oldNonKeyAttributes, oldNkaExists := oldMap["non_key_attributes"].(*schema.Set) + newNonKeyAttributes, newNkaExists := newMap["non_key_attributes"].(*schema.Set) - o := map[string]bool{} - for _, oldNonKeyAttribute := range oldNonKeyAttributes { - o[oldNonKeyAttribute] = true - } - n := map[string]bool{} - for _, newNonKeyAttribute := range newNonKeyAttributes { - n[newNonKeyAttribute] = true + if oldNkaExists && newNkaExists { + return !oldNonKeyAttributes.Equal(newNonKeyAttributes) } - // perform this check after populating map so we ignore duplicated fields in non_key_attributes - if len(o) != len(n) { - return true - } - - for k, v := range n { - if oVal, oExists := o[k]; !oExists || v != oVal { - return true - } - } - return false + return oldNkaExists != newNkaExists } func stripCapacityAttributes(in map[string]interface{}) (map[string]interface{}, error) { @@ -4265,10 +4247,6 @@ func expandDynamoDbProjection(data map[string]interface{}) *dynamodb.Projection ProjectionType: aws.String(data["projection_type"].(string)), } - if v, ok := data["non_key_attributes"].([]interface{}); ok && len(v) > 0 { - projection.NonKeyAttributes = expandStringList(v) - } - if v, ok := data["non_key_attributes"].(*schema.Set); ok && v.Len() > 0 { projection.NonKeyAttributes = expandStringList(v.List()) }