From 9b449bec995ba28f445f80c8dba1172d0b4a4661 Mon Sep 17 00:00:00 2001 From: Nick Fagerlund Date: Mon, 20 Dec 2021 21:46:39 -0800 Subject: [PATCH 1/3] Sort dependencies when encoding `ResourceInstanceObject` Resource dependencies are by nature an unordered collection, but they're persisted to state as a JSON array (in random order). This makes a mess for `terraform apply -refresh-only`, which sees the new random order as a change that requires the user to approve a state update. (As an additional problem on top of that, the user interface for refresh-only runs doesn't expect to see that as a type of change, so it says "no changes! would you like to update to reflect these detected changes?") This commit changes `ResourceInstanceObject.Encode()` to sort the in-memory slice of dependencies (lexically, by address) before passing it on to be compared and persisted. This appears to fix the observed UI issues with a minimum of logic changes. --- internal/states/instance_object.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/internal/states/instance_object.go b/internal/states/instance_object.go index 85ca5287829c..7452b4174d64 100644 --- a/internal/states/instance_object.go +++ b/internal/states/instance_object.go @@ -1,6 +1,8 @@ package states import ( + "sort" + "github.com/zclconf/go-cty/cty" ctyjson "github.com/zclconf/go-cty/cty/json" @@ -108,6 +110,13 @@ func (o *ResourceInstanceObject) Encode(ty cty.Type, schemaVersion uint64) (*Res return nil, err } + // Dependencies are collected and merged in an unordered format (using map + // keys as a set), then later changed to a slice (in random ordering) to be + // stored in state as an array. To avoid pointless thrashing of state in + // refresh-only runs, we can either override comparison of dependency lists + // (more desirable, but tricky for Reasons) or just sort when encoding. + sort.Slice(o.Dependencies, func(i, j int) bool { return o.Dependencies[i].String() < o.Dependencies[j].String() }) + return &ResourceInstanceObjectSrc{ SchemaVersion: schemaVersion, AttrsJSON: src, From df36a03be10a460eb84477e7bccb576c0869d196 Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Wed, 5 Jan 2022 12:29:20 -0500 Subject: [PATCH 2/3] states: Add failing test for ordered dependencies --- internal/states/instance_object_test.go | 37 +++++++++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 internal/states/instance_object_test.go diff --git a/internal/states/instance_object_test.go b/internal/states/instance_object_test.go new file mode 100644 index 000000000000..f8be9743ed83 --- /dev/null +++ b/internal/states/instance_object_test.go @@ -0,0 +1,37 @@ +package states + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/hashicorp/terraform/internal/addrs" + "github.com/zclconf/go-cty/cty" +) + +func TestResourceInstanceObject_encode(t *testing.T) { + value := cty.ObjectVal(map[string]cty.Value{ + "foo": cty.True, + }) + deps := []addrs.ConfigResource{ + addrs.RootModule.Resource(addrs.ManagedResourceMode, "test", "honk"), + addrs.RootModule.Child("child").Resource(addrs.ManagedResourceMode, "test", "flub"), + addrs.RootModule.Resource(addrs.ManagedResourceMode, "test", "boop"), + } + wantDeps := []addrs.ConfigResource{ + addrs.RootModule.Child("child").Resource(addrs.ManagedResourceMode, "test", "flub"), + addrs.RootModule.Resource(addrs.ManagedResourceMode, "test", "boop"), + addrs.RootModule.Resource(addrs.ManagedResourceMode, "test", "honk"), + } + rio := &ResourceInstanceObject{ + Value: value, + Status: ObjectPlanned, + Dependencies: deps, + } + rios, err := rio.Encode(value.Type(), 0) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + if diff := cmp.Diff(wantDeps, rios.Dependencies); diff != "" { + t.Errorf("wrong result for deps\n%s", diff) + } +} From 05d0febf7f23ef0846c336de7bfdf0236cf93576 Mon Sep 17 00:00:00 2001 From: Nick Fagerlund Date: Wed, 5 Jan 2022 14:38:53 -0800 Subject: [PATCH 3/3] Relax test to focus on the behavior we care about (encoded == encoded) The specific output order is meaningless, but it should always be the same after two Encode() calls with identical (ignoring in-memory order) dependency sets. --- internal/states/instance_object_test.go | 27 ++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/internal/states/instance_object_test.go b/internal/states/instance_object_test.go index f8be9743ed83..8f961330df0a 100644 --- a/internal/states/instance_object_test.go +++ b/internal/states/instance_object_test.go @@ -12,26 +12,39 @@ func TestResourceInstanceObject_encode(t *testing.T) { value := cty.ObjectVal(map[string]cty.Value{ "foo": cty.True, }) - deps := []addrs.ConfigResource{ + // The in-memory order of resource dependencies is random, since they're an + // unordered set. + depsOne := []addrs.ConfigResource{ addrs.RootModule.Resource(addrs.ManagedResourceMode, "test", "honk"), addrs.RootModule.Child("child").Resource(addrs.ManagedResourceMode, "test", "flub"), addrs.RootModule.Resource(addrs.ManagedResourceMode, "test", "boop"), } - wantDeps := []addrs.ConfigResource{ + depsTwo := []addrs.ConfigResource{ addrs.RootModule.Child("child").Resource(addrs.ManagedResourceMode, "test", "flub"), addrs.RootModule.Resource(addrs.ManagedResourceMode, "test", "boop"), addrs.RootModule.Resource(addrs.ManagedResourceMode, "test", "honk"), } - rio := &ResourceInstanceObject{ + rioOne := &ResourceInstanceObject{ Value: value, Status: ObjectPlanned, - Dependencies: deps, + Dependencies: depsOne, } - rios, err := rio.Encode(value.Type(), 0) + rioTwo := &ResourceInstanceObject{ + Value: value, + Status: ObjectPlanned, + Dependencies: depsTwo, + } + riosOne, err := rioOne.Encode(value.Type(), 0) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + riosTwo, err := rioTwo.Encode(value.Type(), 0) if err != nil { t.Fatalf("unexpected error: %s", err) } - if diff := cmp.Diff(wantDeps, rios.Dependencies); diff != "" { - t.Errorf("wrong result for deps\n%s", diff) + // However, identical sets of dependencies should always be written to state + // in an identical order, so we don't do meaningless state updates on refresh. + if diff := cmp.Diff(riosOne.Dependencies, riosTwo.Dependencies); diff != "" { + t.Errorf("identical dependencies got encoded in different orders:\n%s", diff) } }