Skip to content

Commit 024ffc1

Browse files
turkenfturkenf
committed
Fix update loop for table resource by adding CustomDiff
Signed-off-by: Fatih Türken <turkenf@gmail.com>
1 parent 19f072e commit 024ffc1

File tree

2 files changed

+228
-0
lines changed

2 files changed

+228
-0
lines changed

‎config/cluster/dynamodb/config.go‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,14 @@
55
package dynamodb
66

77
import (
8+
"bytes"
9+
"fmt"
10+
"sort"
11+
"strings"
12+
813
"github.com/crossplane/upjet/v2/pkg/config"
14+
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
15+
"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
916

1017
"github.com/upbound/provider-aws/v2/config/cluster/common"
1118
)
@@ -53,5 +60,112 @@ func Configure(p *config.Provider) { //nolint:gocyclo
5360
TerraformName: "aws_kms_key",
5461
Extractor: common.PathARNExtractor,
5562
}
63+
64+
// global_secondary_index is a TypeSet. The Terraform default set hash
65+
// is computed from ALL fields of a set element, including computed-only
66+
// fields like "warm_throughput" and "key_schema". AWS populates these
67+
// computed fields after creation, so the state hash (which includes
68+
// warm_throughput/key_schema values) diverges from the config hash
69+
// (which does not have them). This causes a perpetual diff where
70+
// Terraform sees the GSI as "delete old hash + add new hash" on every
71+
// reconcile, even though the actual user-specified values are identical.
72+
//
73+
// To fix this, we define a custom hash function that only uses the
74+
// user-configurable fields (name, hash_key, range_key, projection_type,
75+
// non_key_attributes, read_capacity, write_capacity) and compare old vs
76+
// new sets using that hash. If the sets are equal under the custom hash,
77+
// we suppress all global_secondary_index diffs.
78+
r.TerraformCustomDiff = func(diff *terraform.InstanceDiff, state *terraform.InstanceState, _ *terraform.ResourceConfig) (*terraform.InstanceDiff, error) {
79+
if state == nil || state.Empty() || diff == nil || diff.Empty() || diff.Destroy {
80+
return diff, nil
81+
}
82+
83+
// --- GSI TypeSet hash suppression ---
84+
// (see comment block above for full explanation)
85+
resourceData, err := schema.InternalMap(r.TerraformResource.Schema).Data(state, diff)
86+
if err == nil && resourceData.HasChange("global_secondary_index") {
87+
gsiUserFieldsHashFunc := func(v interface{}) int {
88+
var buf bytes.Buffer
89+
tfMap, ok := v.(map[string]interface{})
90+
if !ok {
91+
return 0
92+
}
93+
if name, ok := tfMap["name"].(string); ok {
94+
buf.WriteString(fmt.Sprintf("%s-", name)) //nolint:staticcheck
95+
}
96+
if hashKey, ok := tfMap["hash_key"].(string); ok {
97+
buf.WriteString(fmt.Sprintf("%s-", hashKey)) //nolint:staticcheck
98+
}
99+
if rangeKey, ok := tfMap["range_key"].(string); ok {
100+
buf.WriteString(fmt.Sprintf("%s-", rangeKey)) //nolint:staticcheck
101+
}
102+
if projType, ok := tfMap["projection_type"].(string); ok {
103+
buf.WriteString(fmt.Sprintf("%s-", projType)) //nolint:staticcheck
104+
}
105+
// read_capacity and write_capacity are "number" type in the
106+
// Terraform schema, which maps to float64 in Go (not int).
107+
if readCap, ok := tfMap["read_capacity"].(float64); ok {
108+
buf.WriteString(fmt.Sprintf("%g-", readCap))
109+
}
110+
if writeCap, ok := tfMap["write_capacity"].(float64); ok {
111+
buf.WriteString(fmt.Sprintf("%g-", writeCap))
112+
}
113+
if nka, ok := tfMap["non_key_attributes"]; ok {
114+
if nkaSet, ok := nka.(*schema.Set); ok {
115+
nkaList := make([]string, 0, nkaSet.Len())
116+
for _, v := range nkaSet.List() {
117+
if s, ok := v.(string); ok {
118+
nkaList = append(nkaList, s)
119+
}
120+
}
121+
sort.Strings(nkaList)
122+
for _, s := range nkaList {
123+
buf.WriteString(fmt.Sprintf("%s-", s)) //nolint:staticcheck
124+
}
125+
}
126+
}
127+
return schema.HashString(buf.String())
128+
}
129+
130+
oRaw, nRaw := resourceData.GetChange("global_secondary_index")
131+
oldGSIs := oRaw.(*schema.Set)
132+
newGSIs := nRaw.(*schema.Set)
133+
134+
oldGSIsCustomHash := schema.NewSet(gsiUserFieldsHashFunc, oldGSIs.List())
135+
newGSIsCustomHash := schema.NewSet(gsiUserFieldsHashFunc, newGSIs.List())
136+
137+
if oldGSIsCustomHash.HashEqual(newGSIsCustomHash) {
138+
for dk := range diff.Attributes {
139+
if strings.HasPrefix(dk, "global_secondary_index") {
140+
delete(diff.Attributes, dk)
141+
}
142+
}
143+
}
144+
}
145+
146+
// --- TTL and SSE spurious diff suppression ---
147+
// After late-initialization, the observed state contains
148+
// ttl (attribute_name, enabled) and server_side_encryption
149+
// (enabled) values populated by AWS. However, these values
150+
// may not be properly written back to the Terraform config
151+
// during late-init in v1beta1, causing a diff where the
152+
// config appears to "remove" these fields on every reconcile:
153+
// ttl.0.attribute_name: Old:"TimeToExist" → New:"" (NewRemoved)
154+
// ttl.0.enabled: Old:"true" → New:"false"
155+
// server_side_encryption.0.enabled: Old:"true" → New:"false" (NewRemoved)
156+
// This triggers an update that fails (empty attributeName
157+
// violates AWS API constraints) and loops indefinitely.
158+
// We suppress these diffs when the state has values but the
159+
// config is trying to remove them.
160+
if d, ok := diff.Attributes["ttl.0.attribute_name"]; ok && d.Old != "" && d.NewRemoved {
161+
delete(diff.Attributes, "ttl.0.attribute_name")
162+
delete(diff.Attributes, "ttl.0.enabled")
163+
}
164+
if d, ok := diff.Attributes["server_side_encryption.0.enabled"]; ok && d.Old == "true" && d.NewRemoved {
165+
delete(diff.Attributes, "server_side_encryption.0.enabled")
166+
}
167+
168+
return diff, nil
169+
}
56170
})
57171
}

‎config/namespaced/dynamodb/config.go‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,14 @@
55
package dynamodb
66

77
import (
8+
"bytes"
9+
"fmt"
10+
"sort"
11+
"strings"
12+
813
"github.com/crossplane/upjet/v2/pkg/config"
14+
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
15+
"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
916

1017
"github.com/upbound/provider-aws/v2/config/namespaced/common"
1118
)
@@ -53,5 +60,112 @@ func Configure(p *config.Provider) { //nolint:gocyclo
5360
TerraformName: "aws_kms_key",
5461
Extractor: common.PathARNExtractor,
5562
}
63+
64+
// global_secondary_index is a TypeSet. The Terraform default set hash
65+
// is computed from ALL fields of a set element, including computed-only
66+
// fields like "warm_throughput" and "key_schema". AWS populates these
67+
// computed fields after creation, so the state hash (which includes
68+
// warm_throughput/key_schema values) diverges from the config hash
69+
// (which does not have them). This causes a perpetual diff where
70+
// Terraform sees the GSI as "delete old hash + add new hash" on every
71+
// reconcile, even though the actual user-specified values are identical.
72+
//
73+
// To fix this, we define a custom hash function that only uses the
74+
// user-configurable fields (name, hash_key, range_key, projection_type,
75+
// non_key_attributes, read_capacity, write_capacity) and compare old vs
76+
// new sets using that hash. If the sets are equal under the custom hash,
77+
// we suppress all global_secondary_index diffs.
78+
r.TerraformCustomDiff = func(diff *terraform.InstanceDiff, state *terraform.InstanceState, _ *terraform.ResourceConfig) (*terraform.InstanceDiff, error) {
79+
if state == nil || state.Empty() || diff == nil || diff.Empty() || diff.Destroy {
80+
return diff, nil
81+
}
82+
83+
// --- GSI TypeSet hash suppression ---
84+
// (see comment block above for full explanation)
85+
resourceData, err := schema.InternalMap(r.TerraformResource.Schema).Data(state, diff)
86+
if err == nil && resourceData.HasChange("global_secondary_index") {
87+
gsiUserFieldsHashFunc := func(v interface{}) int {
88+
var buf bytes.Buffer
89+
tfMap, ok := v.(map[string]interface{})
90+
if !ok {
91+
return 0
92+
}
93+
if name, ok := tfMap["name"].(string); ok {
94+
buf.WriteString(fmt.Sprintf("%s-", name)) //nolint:staticcheck
95+
}
96+
if hashKey, ok := tfMap["hash_key"].(string); ok {
97+
buf.WriteString(fmt.Sprintf("%s-", hashKey)) //nolint:staticcheck
98+
}
99+
if rangeKey, ok := tfMap["range_key"].(string); ok {
100+
buf.WriteString(fmt.Sprintf("%s-", rangeKey)) //nolint:staticcheck
101+
}
102+
if projType, ok := tfMap["projection_type"].(string); ok {
103+
buf.WriteString(fmt.Sprintf("%s-", projType)) //nolint:staticcheck
104+
}
105+
// read_capacity and write_capacity are "number" type in the
106+
// Terraform schema, which maps to float64 in Go (not int).
107+
if readCap, ok := tfMap["read_capacity"].(float64); ok {
108+
buf.WriteString(fmt.Sprintf("%g-", readCap))
109+
}
110+
if writeCap, ok := tfMap["write_capacity"].(float64); ok {
111+
buf.WriteString(fmt.Sprintf("%g-", writeCap))
112+
}
113+
if nka, ok := tfMap["non_key_attributes"]; ok {
114+
if nkaSet, ok := nka.(*schema.Set); ok {
115+
nkaList := make([]string, 0, nkaSet.Len())
116+
for _, v := range nkaSet.List() {
117+
if s, ok := v.(string); ok {
118+
nkaList = append(nkaList, s)
119+
}
120+
}
121+
sort.Strings(nkaList)
122+
for _, s := range nkaList {
123+
buf.WriteString(fmt.Sprintf("%s-", s)) //nolint:staticcheck
124+
}
125+
}
126+
}
127+
return schema.HashString(buf.String())
128+
}
129+
130+
oRaw, nRaw := resourceData.GetChange("global_secondary_index")
131+
oldGSIs := oRaw.(*schema.Set)
132+
newGSIs := nRaw.(*schema.Set)
133+
134+
oldGSIsCustomHash := schema.NewSet(gsiUserFieldsHashFunc, oldGSIs.List())
135+
newGSIsCustomHash := schema.NewSet(gsiUserFieldsHashFunc, newGSIs.List())
136+
137+
if oldGSIsCustomHash.HashEqual(newGSIsCustomHash) {
138+
for dk := range diff.Attributes {
139+
if strings.HasPrefix(dk, "global_secondary_index") {
140+
delete(diff.Attributes, dk)
141+
}
142+
}
143+
}
144+
}
145+
146+
// --- TTL and SSE spurious diff suppression ---
147+
// After late-initialization, the observed state contains
148+
// ttl (attribute_name, enabled) and server_side_encryption
149+
// (enabled) values populated by AWS. However, these values
150+
// may not be properly written back to the Terraform config
151+
// during late-init in v1beta1, causing a diff where the
152+
// config appears to "remove" these fields on every reconcile:
153+
// ttl.0.attribute_name: Old:"TimeToExist" → New:"" (NewRemoved)
154+
// ttl.0.enabled: Old:"true" → New:"false"
155+
// server_side_encryption.0.enabled: Old:"true" → New:"false" (NewRemoved)
156+
// This triggers an update that fails (empty attributeName
157+
// violates AWS API constraints) and loops indefinitely.
158+
// We suppress these diffs when the state has values but the
159+
// config is trying to remove them.
160+
if d, ok := diff.Attributes["ttl.0.attribute_name"]; ok && d.Old != "" && d.NewRemoved {
161+
delete(diff.Attributes, "ttl.0.attribute_name")
162+
delete(diff.Attributes, "ttl.0.enabled")
163+
}
164+
if d, ok := diff.Attributes["server_side_encryption.0.enabled"]; ok && d.Old == "true" && d.NewRemoved {
165+
delete(diff.Attributes, "server_side_encryption.0.enabled")
166+
}
167+
168+
return diff, nil
169+
}
56170
})
57171
}

0 commit comments

Comments
 (0)