Skip to content

Commit 0ea8b4a

Browse files
taruntej-ataruntej-a
authored
Merge pull request #47867 from hashicorp/b_aws_dynamodb_table-gsi-hash_key-type-no-diff
r/aws_dynamodb_table: Fix plan/apply does not detect `hash_key` type changes in DynamoDB GSI
2 parents 73008cd + 2271771 commit 0ea8b4a

3 files changed

Lines changed: 175 additions & 15 deletions

File tree

‎.changelog/47867.txt‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
```release-note:bug
2+
resource/aws_dynamodb_table: Ensure diffs are shown for GSI hash key type changes
3+
```

‎internal/service/dynamodb/table.go‎

Lines changed: 35 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,7 @@ func resourceTable() *schema.Resource {
169169
},
170170
},
171171
},
172-
Set: sdkv2.SimpleSchemaSetFunc(names.AttrName),
172+
Set: sdkv2.SimpleSchemaSetFunc(names.AttrName, names.AttrType),
173173
},
174174
"billing_mode": {
175175
Type: schema.TypeString,
@@ -1179,11 +1179,23 @@ func resourceTableUpdate(ctx context.Context, d *schema.ResourceData, meta any)
11791179
// will signal the problems.
11801180
var gsiUpdates []awstypes.GlobalSecondaryIndexUpdate
11811181

1182-
if d.HasChange("global_secondary_index") {
1183-
var err error
1184-
o, n := d.GetChange("global_secondary_index")
1185-
gsiUpdates, err = updateDiffGSI(o.(*schema.Set).List(), n.(*schema.Set).List(), newBillingMode)
1182+
if d.HasChanges("global_secondary_index", "attribute") {
1183+
oldGSI, newGSI := d.GetChange("global_secondary_index")
1184+
oldAttr, newAttr := d.GetChange("attribute")
1185+
1186+
oldAttrTypes := map[string]string{}
1187+
for _, a := range oldAttr.(*schema.Set).List() {
1188+
attr := a.(map[string]any)
1189+
oldAttrTypes[attr[names.AttrName].(string)] = attr[names.AttrType].(string)
1190+
}
1191+
newAttrTypes := map[string]string{}
1192+
for _, a := range newAttr.(*schema.Set).List() {
1193+
attr := a.(map[string]any)
1194+
newAttrTypes[attr[names.AttrName].(string)] = attr[names.AttrType].(string)
1195+
}
11861196

1197+
var err error
1198+
gsiUpdates, err = updateDiffGSI(oldGSI.(*schema.Set).List(), newGSI.(*schema.Set).List(), newBillingMode, oldAttrTypes, newAttrTypes)
11871199
if err != nil {
11881200
return create.AppendDiagError(diags, names.DynamoDB, create.ErrActionUpdating, resNameTable, d.Id(), fmt.Errorf("computing GSI difference: %w", err))
11891201
}
@@ -2093,7 +2105,7 @@ func updateWarmThroughput(ctx context.Context, conn *dynamodb.Client, warmList [
20932105
return nil
20942106
}
20952107

2096-
func updateDiffGSI(oldGsi, newGsi []any, billingMode awstypes.BillingMode) ([]awstypes.GlobalSecondaryIndexUpdate, error) {
2108+
func updateDiffGSI(oldGsi, newGsi []any, billingMode awstypes.BillingMode, oldAttrTypes, newAttrTypes map[string]string) ([]awstypes.GlobalSecondaryIndexUpdate, error) {
20972109
// Transform slices into maps
20982110
oldGsis := make(map[string]any)
20992111
for _, gsidata := range oldGsi {
@@ -2198,7 +2210,7 @@ func updateDiffGSI(oldGsi, newGsi []any, billingMode awstypes.BillingMode) ([]aw
21982210
// ordinal of elements in its equality (which we actually don't care about)
21992211
nonKeyAttributesChanged := checkIfNonKeyAttributesChanged(oldMap, newMap)
22002212

2201-
recreateAttributesChanged := checkIfGSIRecreateAttributesChanged(oldMap, newMap)
2213+
recreateAttributesChanged := checkIfGSIRecreateAttributesChanged(oldMap, newMap, oldAttrTypes, newAttrTypes)
22022214

22032215
gsiNeedsRecreate := nonKeyAttributesChanged || recreateAttributesChanged || warmThroughPutDecreased
22042216

@@ -2286,7 +2298,7 @@ func checkIfNonKeyAttributesChanged(oldMap, newMap map[string]any) bool {
22862298
return oldNkaExists != newNkaExists
22872299
}
22882300

2289-
func checkIfGSIRecreateAttributesChanged(oldMap, newMap map[string]any) bool {
2301+
func checkIfGSIRecreateAttributesChanged(oldMap, newMap map[string]any, oldAttrTypes, newAttrTypes map[string]string) bool {
22902302
oldAttributes := stripGSIUpdatableAttributes(oldMap)
22912303

22922304
newAttributes := stripGSIUpdatableAttributes(newMap)
@@ -2313,7 +2325,21 @@ func checkIfGSIRecreateAttributesChanged(oldMap, newMap map[string]any) bool {
23132325
delete(newAttributes, "range_key")
23142326
}
23152327

2316-
return !reflect.DeepEqual(oldAttributes, newAttributes)
2328+
if !reflect.DeepEqual(oldAttributes, newAttributes) {
2329+
return true
2330+
}
2331+
2332+
// Check if the type of any key attribute used by this GSI changed.
2333+
for _, keyAttr := range []string{oldMap["hash_key"].(string), oldMap["range_key"].(string)} {
2334+
if keyAttr == "" {
2335+
continue
2336+
}
2337+
if oldAttrTypes[keyAttr] != newAttrTypes[keyAttr] {
2338+
return true
2339+
}
2340+
}
2341+
2342+
return false
23172343
}
23182344

23192345
func deleteTable(ctx context.Context, conn *dynamodb.Client, tableName string) error {

‎internal/service/dynamodb/table_test.go‎

Lines changed: 137 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -861,7 +861,7 @@ func TestUpdateDiffGSI_Provisioned(t *testing.T) {
861861
tc.New[i] = normalizeGSIMapValue(v.(map[string]any))
862862
}
863863

864-
ops, err := tfdynamodb.UpdateDiffGSI(tc.Old, tc.New, awstypes.BillingModeProvisioned)
864+
ops, err := tfdynamodb.UpdateDiffGSI(tc.Old, tc.New, awstypes.BillingModeProvisioned, nil, nil)
865865
if err != nil {
866866
t.Fatal(err)
867867
}
@@ -1756,7 +1756,7 @@ func TestUpdateDiffGSI_OnDemand(t *testing.T) {
17561756
tc.New[i] = normalizeGSIMapValue(v.(map[string]any))
17571757
}
17581758

1759-
ops, err := tfdynamodb.UpdateDiffGSI(tc.Old, tc.New, awstypes.BillingModePayPerRequest)
1759+
ops, err := tfdynamodb.UpdateDiffGSI(tc.Old, tc.New, awstypes.BillingModePayPerRequest, nil, nil)
17601760
if err != nil {
17611761
t.Fatal(err)
17621762
}
@@ -1800,9 +1800,11 @@ func TestCheckIfGSIRecreateAttributesChanged(t *testing.T) {
18001800
t.Parallel()
18011801

18021802
testCases := map[string]struct {
1803-
Old map[string]any
1804-
New map[string]any
1805-
Expected bool
1803+
Old map[string]any
1804+
New map[string]any
1805+
OldAttrTypes map[string]string
1806+
NewAttrTypes map[string]string
1807+
Expected bool
18061808
}{
18071809
"key_schema syntax unchanged": {
18081810
// State has hash_key/range_key, config only has key_schema
@@ -1888,13 +1890,70 @@ func TestCheckIfGSIRecreateAttributesChanged(t *testing.T) {
18881890
},
18891891
Expected: true,
18901892
},
1893+
"hash_key attribute type changed S to N": {
1894+
Old: map[string]any{
1895+
names.AttrName: "gsi1",
1896+
"hash_key": "alternate_id",
1897+
"range_key": "",
1898+
"projection_type": "ALL",
1899+
"key_schema": nil,
1900+
},
1901+
New: map[string]any{
1902+
names.AttrName: "gsi1",
1903+
"hash_key": "alternate_id",
1904+
"range_key": "",
1905+
"projection_type": "ALL",
1906+
"key_schema": nil,
1907+
},
1908+
OldAttrTypes: map[string]string{"alternate_id": "S"},
1909+
NewAttrTypes: map[string]string{"alternate_id": "N"},
1910+
Expected: true,
1911+
},
1912+
"hash_key attribute type unchanged": {
1913+
Old: map[string]any{
1914+
names.AttrName: "gsi1",
1915+
"hash_key": "alternate_id",
1916+
"range_key": "",
1917+
"projection_type": "ALL",
1918+
"key_schema": nil,
1919+
},
1920+
New: map[string]any{
1921+
names.AttrName: "gsi1",
1922+
"hash_key": "alternate_id",
1923+
"range_key": "",
1924+
"projection_type": "ALL",
1925+
"key_schema": nil,
1926+
},
1927+
OldAttrTypes: map[string]string{"alternate_id": "S"},
1928+
NewAttrTypes: map[string]string{"alternate_id": "S"},
1929+
Expected: false,
1930+
},
1931+
"range_key attribute type changed S to N": {
1932+
Old: map[string]any{
1933+
names.AttrName: "gsi1",
1934+
"hash_key": "pk",
1935+
"range_key": "sk",
1936+
"projection_type": "ALL",
1937+
"key_schema": nil,
1938+
},
1939+
New: map[string]any{
1940+
names.AttrName: "gsi1",
1941+
"hash_key": "pk",
1942+
"range_key": "sk",
1943+
"projection_type": "ALL",
1944+
"key_schema": nil,
1945+
},
1946+
OldAttrTypes: map[string]string{"pk": "S", "sk": "S"},
1947+
NewAttrTypes: map[string]string{"pk": "S", "sk": "N"},
1948+
Expected: true,
1949+
},
18911950
}
18921951

18931952
for name, tc := range testCases {
18941953
t.Run(name, func(t *testing.T) {
18951954
t.Parallel()
18961955

1897-
got := tfdynamodb.CheckIfGSIRecreateAttributesChanged(tc.Old, tc.New)
1956+
got := tfdynamodb.CheckIfGSIRecreateAttributesChanged(tc.Old, tc.New, tc.OldAttrTypes, tc.NewAttrTypes)
18981957
if got != tc.Expected {
18991958
t.Errorf("expected %v, got %v", tc.Expected, got)
19001959
}
@@ -4810,6 +4869,23 @@ func TestAccDynamoDBTable_attributeUpdate(t *testing.T) {
48104869
Check: resource.ComposeAggregateTestCheckFunc(
48114870
testAccCheckInitialTableExists(ctx, t, resourceName, &conf),
48124871
),
4872+
ConfigPlanChecks: resource.ConfigPlanChecks{
4873+
PreApply: []plancheck.PlanCheck{
4874+
plancheck.ExpectResourceAction(resourceName, plancheck.ResourceActionCreate),
4875+
},
4876+
},
4877+
ConfigStateChecks: []statecheck.StateCheck{
4878+
statecheck.ExpectKnownValue(resourceName, tfjsonpath.New("attribute"), knownvalue.SetExact([]knownvalue.Check{
4879+
knownvalue.ObjectExact(map[string]knownvalue.Check{
4880+
names.AttrName: knownvalue.StringExact("staticHashKey"),
4881+
names.AttrType: knownvalue.StringExact("S"),
4882+
}),
4883+
knownvalue.ObjectExact(map[string]knownvalue.Check{
4884+
names.AttrName: knownvalue.StringExact("firstKey"),
4885+
names.AttrType: knownvalue.StringExact("S"),
4886+
}),
4887+
})),
4888+
},
48134889
},
48144890
{
48154891
ResourceName: resourceName,
@@ -4821,18 +4897,73 @@ func TestAccDynamoDBTable_attributeUpdate(t *testing.T) {
48214897
Check: resource.ComposeAggregateTestCheckFunc(
48224898
testAccCheckInitialTableExists(ctx, t, resourceName, &conf),
48234899
),
4900+
ConfigPlanChecks: resource.ConfigPlanChecks{
4901+
PreApply: []plancheck.PlanCheck{
4902+
plancheck.ExpectResourceAction(resourceName, plancheck.ResourceActionUpdate),
4903+
},
4904+
},
4905+
ConfigStateChecks: []statecheck.StateCheck{
4906+
statecheck.ExpectKnownValue(resourceName, tfjsonpath.New("attribute"), knownvalue.SetExact([]knownvalue.Check{
4907+
knownvalue.ObjectExact(map[string]knownvalue.Check{
4908+
names.AttrName: knownvalue.StringExact("staticHashKey"),
4909+
names.AttrType: knownvalue.StringExact("S"),
4910+
}),
4911+
knownvalue.ObjectExact(map[string]knownvalue.Check{
4912+
names.AttrName: knownvalue.StringExact("firstKey"),
4913+
names.AttrType: knownvalue.StringExact("N"),
4914+
}),
4915+
})),
4916+
},
48244917
},
48254918
{ // New attribute addition (index update)
48264919
Config: testAccTableConfig_twoAttributes(rName, "firstKey", "secondKey", "firstKey", "N", "secondKey", "S"),
48274920
Check: resource.ComposeAggregateTestCheckFunc(
48284921
testAccCheckInitialTableExists(ctx, t, resourceName, &conf),
48294922
),
4923+
ConfigPlanChecks: resource.ConfigPlanChecks{
4924+
PreApply: []plancheck.PlanCheck{
4925+
plancheck.ExpectResourceAction(resourceName, plancheck.ResourceActionUpdate),
4926+
},
4927+
},
4928+
ConfigStateChecks: []statecheck.StateCheck{
4929+
statecheck.ExpectKnownValue(resourceName, tfjsonpath.New("attribute"), knownvalue.SetExact([]knownvalue.Check{
4930+
knownvalue.ObjectExact(map[string]knownvalue.Check{
4931+
names.AttrName: knownvalue.StringExact("staticHashKey"),
4932+
names.AttrType: knownvalue.StringExact("S"),
4933+
}),
4934+
knownvalue.ObjectExact(map[string]knownvalue.Check{
4935+
names.AttrName: knownvalue.StringExact("firstKey"),
4936+
names.AttrType: knownvalue.StringExact("N"),
4937+
}),
4938+
knownvalue.ObjectExact(map[string]knownvalue.Check{
4939+
names.AttrName: knownvalue.StringExact("secondKey"),
4940+
names.AttrType: knownvalue.StringExact("S"),
4941+
}),
4942+
})),
4943+
},
48304944
},
48314945
{ // Attribute removal (index update)
48324946
Config: testAccTableConfig_oneAttribute(rName, "firstKey", "firstKey", "S"),
48334947
Check: resource.ComposeAggregateTestCheckFunc(
48344948
testAccCheckInitialTableExists(ctx, t, resourceName, &conf),
48354949
),
4950+
ConfigPlanChecks: resource.ConfigPlanChecks{
4951+
PreApply: []plancheck.PlanCheck{
4952+
plancheck.ExpectResourceAction(resourceName, plancheck.ResourceActionUpdate),
4953+
},
4954+
},
4955+
ConfigStateChecks: []statecheck.StateCheck{
4956+
statecheck.ExpectKnownValue(resourceName, tfjsonpath.New("attribute"), knownvalue.SetExact([]knownvalue.Check{
4957+
knownvalue.ObjectExact(map[string]knownvalue.Check{
4958+
names.AttrName: knownvalue.StringExact("staticHashKey"),
4959+
names.AttrType: knownvalue.StringExact("S"),
4960+
}),
4961+
knownvalue.ObjectExact(map[string]knownvalue.Check{
4962+
names.AttrName: knownvalue.StringExact("firstKey"),
4963+
names.AttrType: knownvalue.StringExact("S"),
4964+
}),
4965+
})),
4966+
},
48364967
},
48374968
},
48384969
})

0 commit comments

Comments
 (0)