From 659d5e2218f76d15e75cf1c00447e5d4fa75e81e Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 6 Nov 2025 19:57:44 -0500 Subject: [PATCH 1/3] remove the use of reflect.DeepEqual for states the use of reflect.DeepEqual fails for states, because they contain value which cannot be directly compared for equality. The Equal method is not used much, except to aid in backend migration, so the failure there was rarely noticed. The failure of ManagedResourcesEqual would show up in the CLI after Terraform reported there were no changes, by exiting with a non-zero code because the resource states incorrectly reported as being changed. --- internal/states/checks.go | 41 ++++++++++++ internal/states/instance_object_src.go | 47 ++++++++++++++ internal/states/output_value.go | 16 +++++ internal/states/resource.go | 61 ++++++++++++++++++ internal/states/state_equal.go | 88 ++++++++++++++------------ 5 files changed, 213 insertions(+), 40 deletions(-) diff --git a/internal/states/checks.go b/internal/states/checks.go index f5718e49f2a9..9d8fa9c6218b 100644 --- a/internal/states/checks.go +++ b/internal/states/checks.go @@ -4,6 +4,8 @@ package states import ( + "slices" + "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/checks" ) @@ -169,6 +171,45 @@ func (r *CheckResults) DeepCopy() *CheckResults { return ret } +func (r *CheckResults) Equal(other *CheckResults) bool { + if r == other { + return true + } + if r == nil || other == nil { + return false + } + + if r.ConfigResults.Len() != other.ConfigResults.Len() { + return false + } + for key, elem := range r.ConfigResults.Iter() { + otherElem := other.ConfigResults.Get(key) + if otherElem == nil { + return false + } + if elem.Status != otherElem.Status { + return false + } + if elem.ObjectResults.Len() != otherElem.ObjectResults.Len() { + return false + } + for key, res := range elem.ObjectResults.Iter() { + otherRes := otherElem.ObjectResults.Get(key) + if otherRes == nil { + return false + } + if res.Status != otherRes.Status { + return false + } + if !slices.Equal(res.FailureMessages, otherRes.FailureMessages) { + return false + } + } + } + + return true +} + // ObjectAddrsKnown determines whether the set of objects recorded in this // aggregate is accurate (true) or if it's incomplete as a result of the // run being interrupted before instance expansion. diff --git a/internal/states/instance_object_src.go b/internal/states/instance_object_src.go index 63b7963c2249..fd7d445759b2 100644 --- a/internal/states/instance_object_src.go +++ b/internal/states/instance_object_src.go @@ -4,7 +4,9 @@ package states import ( + "bytes" "fmt" + "reflect" "github.com/zclconf/go-cty/cty" ctyjson "github.com/zclconf/go-cty/cty/json" @@ -168,3 +170,48 @@ func (os *ResourceInstanceObjectSrc) CompleteIdentityUpgrade(newAttrs cty.Value, new.IdentitySchemaVersion = uint64(schema.IdentityVersion) return new, nil } + +// Equal compares two ResourceInstanceObjectSrc objects for equality, skipping +// any internal fields which are not stored to the final serialized state. +func (os *ResourceInstanceObjectSrc) Equal(other *ResourceInstanceObjectSrc) bool { + if os == other { + return true + } + if os == nil || other == nil { + return false + } + + if os.SchemaVersion != other.SchemaVersion { + return false + } + if os.IdentitySchemaVersion != other.IdentitySchemaVersion { + return false + } + if os.Status != other.Status { + return false + } + if os.CreateBeforeDestroy != other.CreateBeforeDestroy { + return false + } + + if !bytes.Equal(os.AttrsJSON, other.AttrsJSON) { + return false + } + if !bytes.Equal(os.IdentityJSON, other.IdentityJSON) { + return false + } + if !bytes.Equal(os.Private, other.Private) { + return false + } + + // Compare legacy AttrsFlat maps. We shouldn't see this ever being used, but + // deal with in just in case until we remove it entirely. These are all + // simple maps of strings, so DeepEqual is perfectly fine here. + if !reflect.DeepEqual(os.AttrsFlat, other.AttrsFlat) { + return false + } + + // We skip fields that have no functional impact on resource state. + + return true +} diff --git a/internal/states/output_value.go b/internal/states/output_value.go index 98a3606c2283..4da33f82dc80 100644 --- a/internal/states/output_value.go +++ b/internal/states/output_value.go @@ -17,3 +17,19 @@ type OutputValue struct { Value cty.Value Sensitive bool } + +func (o *OutputValue) Equal(other *OutputValue) bool { + if o == other { + return true + } + if o == nil || other == nil { + return false + } + if !o.Addr.Equal(other.Addr) { + return false + } + if o.Sensitive != other.Sensitive { + return false + } + return o.Value.RawEquals(other.Value) +} diff --git a/internal/states/resource.go b/internal/states/resource.go index 25eed13c5ee5..70e290c69b9b 100644 --- a/internal/states/resource.go +++ b/internal/states/resource.go @@ -177,3 +177,64 @@ func NewDeposedKey() DeposedKey { func ParseDeposedKey(raw string) (DeposedKey, error) { return addrs.ParseDeposedKey(raw) } + +// Equal compares Resource objects for equality, comparing all the current and +// deposed instances. +func (rs *Resource) Equal(other *Resource) bool { + if rs == other { + return true + } + if rs == nil || other == nil { + return false + } + + if !rs.Addr.Equal(other.Addr) { + return false + } + if !rs.ProviderConfig.Equal(other.ProviderConfig) { + return false + } + + if len(rs.Instances) != len(other.Instances) { + return false + } + for k, inst := range rs.Instances { + otherInst, ok := other.Instances[k] + if !ok { + return false + } + if !inst.Equal(otherInst) { + return false + } + } + + return true +} + +func (i *ResourceInstance) Equal(other *ResourceInstance) bool { + if i == other { + return true + } + if i == nil || other == nil { + return false + } + + if !i.Current.Equal(other.Current) { + return false + } + + if len(i.Deposed) != len(other.Deposed) { + return false + } + for k, dep := range i.Deposed { + otherObj, ok := other.Deposed[k] + if !ok { + return false + } + if !dep.Equal(otherObj) { + return false + } + } + + return true +} diff --git a/internal/states/state_equal.go b/internal/states/state_equal.go index f771a714b2d0..61e343ff30f1 100644 --- a/internal/states/state_equal.go +++ b/internal/states/state_equal.go @@ -3,12 +3,6 @@ package states -import ( - "reflect" - - "github.com/hashicorp/terraform/internal/addrs" -) - // Equal returns true if the receiver is functionally equivalent to other, // including any ephemeral portions of the state that would not be included // if the state were saved to files. @@ -16,10 +10,22 @@ import ( // To test only the persistent portions of two states for equality, instead // use statefile.StatesMarshalEqual. func (s *State) Equal(other *State) bool { - // For the moment this is sufficient, but we may need to do something - // more elaborate in future if we have any portions of state that require - // more sophisticated comparisons. - return reflect.DeepEqual(s, other) + if s == other { + return true + } + if s == nil || other == nil { + return false + } + + if !s.RootOutputValuesEqual(other) { + return false + } + + if !s.CheckResults.Equal(other.CheckResults) { + return false + } + + return s.ManagedResourcesEqual(other) } // ManagedResourcesEqual returns true if all of the managed resources tracked @@ -33,9 +39,14 @@ func (s *State) ManagedResourcesEqual(other *State) bool { // First, some accommodations for situations where one of the objects is // nil, for robustness since we sometimes use a nil state to represent // a prior state being entirely absent. - if s == nil && other == nil { + if s == other { + // covers both states being nil, or both states being the exact same + // object. return true } + + // Managed resources are technically equal if one state is nil while the + // other has no resources. if s == nil { return !other.HasManagedResourceInstanceObjects() } @@ -45,29 +56,37 @@ func (s *State) ManagedResourcesEqual(other *State) bool { // If we get here then both states are non-nil. - // sameManagedResources tests that its second argument has all the - // resources that the first one does, so we'll call it twice with the - // arguments inverted to ensure that we'll also catch situations where - // the second has resources that the first does not. - return sameManagedResources(s, other) && sameManagedResources(other, s) -} + if len(s.Modules) != len(other.Modules) { + return false + } + + for key, sMod := range s.Modules { + otherMod, ok := other.Modules[key] + if !ok { + return false + } + // Something else is wrong if the addresses don't match, but they are + // definitely not equal + if !sMod.Addr.Equal(otherMod.Addr) { + return false + } + + if len(sMod.Resources) != len(otherMod.Resources) { + return false + } -func sameManagedResources(s1, s2 *State) bool { - for _, ms := range s1.Modules { - for _, rs := range ms.Resources { - addr := rs.Addr - if addr.Resource.Mode != addrs.ManagedResourceMode { - continue + for key, sRes := range sMod.Resources { + otherRes, ok := otherMod.Resources[key] + if !ok { + return false } - otherRS := s2.Resource(addr) - if !reflect.DeepEqual(rs, otherRS) { + if !sRes.Equal(otherRes) { return false } } } return true - } // RootOutputValuesEqual returns true if the root output values tracked in the @@ -77,25 +96,14 @@ func (s *State) RootOutputValuesEqual(s2 *State) bool { if s == nil && s2 == nil { return true } - if s == nil { - return !s2.HasRootOutputValues() - } - if s2 == nil { - return !s.HasRootOutputValues() - } - - return sameRootOutputValues(s, s2) -} -// sameRootOutputValues returns true if the two states have the same root output values. -func sameRootOutputValues(s1, s2 *State) bool { - if len(s1.RootOutputValues) != len(s2.RootOutputValues) { + if len(s.RootOutputValues) != len(s2.RootOutputValues) { return false } - for k, v1 := range s1.RootOutputValues { + for k, v1 := range s2.RootOutputValues { v2, ok := s2.RootOutputValues[k] - if !ok || !reflect.DeepEqual(v1, v2) { + if !ok || !v1.Equal(v2) { return false } } From 55e340c9d288cd5943d1da37ed0582964d7183a9 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 7 Nov 2025 14:43:29 -0500 Subject: [PATCH 2/3] more equality testing --- internal/states/state_test.go | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/internal/states/state_test.go b/internal/states/state_test.go index 0ce3f74c806d..4d6915978a84 100644 --- a/internal/states/state_test.go +++ b/internal/states/state_test.go @@ -203,6 +203,13 @@ func TestStateDeepCopy(t *testing.T) { Private: []byte("private data"), Dependencies: []addrs.ConfigResource{}, CreateBeforeDestroy: true, + + // these may or may not be copied, but should not affect equality of + // the resources. + decodeValueCache: cty.ObjectVal(map[string]cty.Value{ + "woozles": cty.StringVal("confuzles"), + }), + decodeIdentityCache: cty.DynamicVal, }, addrs.AbsProviderConfig{ Provider: addrs.NewDefaultProvider("test"), @@ -242,11 +249,34 @@ func TestStateDeepCopy(t *testing.T) { ) state.EnsureModule(addrs.RootModuleInstance.Child("child", addrs.NoKey)) - stateCopy := state.DeepCopy() if !state.Equal(stateCopy) { t.Fatalf("\nexpected:\n%q\ngot:\n%q\n", state, stateCopy) } + + // this is implied by the above, but has previously used a different + // codepath for comparison. + if !state.ManagedResourcesEqual(stateCopy) { + t.Fatalf("\nexpected managed resources to be equal:\n%q\ngot:\n%q\n", state, stateCopy) + } + + // remove the cached values and ensure equality still holds + for _, mod := range stateCopy.Modules { + for _, res := range mod.Resources { + for _, inst := range res.Instances { + inst.Current.decodeValueCache = cty.NilVal + inst.Current.decodeIdentityCache = cty.NilVal + } + } + } + + if !state.Equal(stateCopy) { + t.Fatalf("\nexpected:\n%q\ngot:\n%q\n", state, stateCopy) + } + + if !state.ManagedResourcesEqual(stateCopy) { + t.Fatalf("\nexpected managed resources to be equal:\n%q\ngot:\n%q\n", state, stateCopy) + } } func TestStateHasResourceInstanceObjects(t *testing.T) { From f78c64e528f1c92ce51fd25747276601aa2e2f80 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 10 Nov 2025 12:09:40 -0500 Subject: [PATCH 3/3] CHANGELOG --- .changes/v1.15/BUG FIXES-20251110-120921.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changes/v1.15/BUG FIXES-20251110-120921.yaml diff --git a/.changes/v1.15/BUG FIXES-20251110-120921.yaml b/.changes/v1.15/BUG FIXES-20251110-120921.yaml new file mode 100644 index 000000000000..cc5ae0920bb8 --- /dev/null +++ b/.changes/v1.15/BUG FIXES-20251110-120921.yaml @@ -0,0 +1,5 @@ +kind: BUG FIXES +body: A refresh-only plan could result in a non-zero exit code with no changes +time: 2025-11-10T12:09:21.029489-05:00 +custom: + Issue: "37406"