From 79a31f627b1b5229e91bace4afd199dc38d58db9 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 17 Dec 2020 12:03:28 -0500 Subject: [PATCH] compare unordered sets of PathMarkValues When comparing marks for values during plan and apply, we need to ensure the order of the marked paths is consistent. --- terraform/marks.go | 39 +++++++ terraform/marks_test.go | 104 +++++++++++++++++++ terraform/node_resource_abstract_instance.go | 5 +- 3 files changed, 145 insertions(+), 3 deletions(-) create mode 100644 terraform/marks.go create mode 100644 terraform/marks_test.go diff --git a/terraform/marks.go b/terraform/marks.go new file mode 100644 index 0000000000..8e2a326072 --- /dev/null +++ b/terraform/marks.go @@ -0,0 +1,39 @@ +package terraform + +import ( + "fmt" + "sort" + + "github.com/zclconf/go-cty/cty" +) + +// marksEqual compares 2 unordered sets of PathValue marks for equality, with +// the comparison using the cty.PathValueMarks.Equal method. +func marksEqual(a, b []cty.PathValueMarks) bool { + if len(a) == 0 && len(b) == 0 { + return true + } + + if len(a) != len(b) { + return false + } + + less := func(s []cty.PathValueMarks) func(i, j int) bool { + return func(i, j int) bool { + // the sort only needs to be consistent, so use the GoString format + // to get a comparable value + return fmt.Sprintf("%#v", s[i]) < fmt.Sprintf("%#v", s[j]) + } + } + + sort.Slice(a, less(a)) + sort.Slice(b, less(b)) + + for i := 0; i < len(a); i++ { + if !a[i].Equal(b[i]) { + return false + } + } + + return true +} diff --git a/terraform/marks_test.go b/terraform/marks_test.go new file mode 100644 index 0000000000..efb3b7e9b8 --- /dev/null +++ b/terraform/marks_test.go @@ -0,0 +1,104 @@ +package terraform + +import ( + "fmt" + "testing" + + "github.com/zclconf/go-cty/cty" +) + +func TestMarksEqual(t *testing.T) { + for i, tc := range []struct { + a, b []cty.PathValueMarks + equal bool + }{ + { + []cty.PathValueMarks{ + cty.PathValueMarks{Path: cty.Path{cty.GetAttrStep{Name: "a"}}, Marks: cty.NewValueMarks("sensitive")}, + }, + []cty.PathValueMarks{ + cty.PathValueMarks{Path: cty.Path{cty.GetAttrStep{Name: "a"}}, Marks: cty.NewValueMarks("sensitive")}, + }, + true, + }, + { + []cty.PathValueMarks{ + cty.PathValueMarks{Path: cty.Path{cty.GetAttrStep{Name: "a"}}, Marks: cty.NewValueMarks("sensitive")}, + }, + []cty.PathValueMarks{ + cty.PathValueMarks{Path: cty.Path{cty.GetAttrStep{Name: "A"}}, Marks: cty.NewValueMarks("sensitive")}, + }, + false, + }, + { + []cty.PathValueMarks{ + cty.PathValueMarks{Path: cty.Path{cty.GetAttrStep{Name: "a"}}, Marks: cty.NewValueMarks("sensitive")}, + cty.PathValueMarks{Path: cty.Path{cty.GetAttrStep{Name: "b"}}, Marks: cty.NewValueMarks("sensitive")}, + cty.PathValueMarks{Path: cty.Path{cty.GetAttrStep{Name: "c"}}, Marks: cty.NewValueMarks("sensitive")}, + }, + []cty.PathValueMarks{ + cty.PathValueMarks{Path: cty.Path{cty.GetAttrStep{Name: "b"}}, Marks: cty.NewValueMarks("sensitive")}, + cty.PathValueMarks{Path: cty.Path{cty.GetAttrStep{Name: "c"}}, Marks: cty.NewValueMarks("sensitive")}, + cty.PathValueMarks{Path: cty.Path{cty.GetAttrStep{Name: "a"}}, Marks: cty.NewValueMarks("sensitive")}, + }, + true, + }, + { + []cty.PathValueMarks{ + cty.PathValueMarks{ + Path: cty.Path{cty.GetAttrStep{Name: "a"}, cty.GetAttrStep{Name: "b"}}, + Marks: cty.NewValueMarks("sensitive"), + }, + cty.PathValueMarks{ + Path: cty.Path{cty.GetAttrStep{Name: "a"}, cty.GetAttrStep{Name: "c"}}, + Marks: cty.NewValueMarks("sensitive"), + }, + }, + []cty.PathValueMarks{ + cty.PathValueMarks{ + Path: cty.Path{cty.GetAttrStep{Name: "a"}, cty.GetAttrStep{Name: "c"}}, + Marks: cty.NewValueMarks("sensitive"), + }, + cty.PathValueMarks{ + Path: cty.Path{cty.GetAttrStep{Name: "a"}, cty.GetAttrStep{Name: "b"}}, + Marks: cty.NewValueMarks("sensitive"), + }, + }, + true, + }, + { + []cty.PathValueMarks{ + cty.PathValueMarks{Path: cty.Path{cty.GetAttrStep{Name: "a"}}, Marks: cty.NewValueMarks("sensitive")}, + }, + []cty.PathValueMarks{ + cty.PathValueMarks{Path: cty.Path{cty.GetAttrStep{Name: "b"}}, Marks: cty.NewValueMarks("sensitive")}, + }, + false, + }, + { + nil, + nil, + true, + }, + { + []cty.PathValueMarks{ + cty.PathValueMarks{Path: cty.Path{cty.GetAttrStep{Name: "a"}}, Marks: cty.NewValueMarks("sensitive")}, + }, + nil, + false, + }, + { + nil, + []cty.PathValueMarks{ + cty.PathValueMarks{Path: cty.Path{cty.GetAttrStep{Name: "a"}}, Marks: cty.NewValueMarks("sensitive")}, + }, + false, + }, + } { + t.Run(fmt.Sprint(i), func(t *testing.T) { + if marksEqual(tc.a, tc.b) != tc.equal { + t.Fatalf("marksEqual(\n%#v,\n%#v,\n) != %t\n", tc.a, tc.b, tc.equal) + } + }) + } +} diff --git a/terraform/node_resource_abstract_instance.go b/terraform/node_resource_abstract_instance.go index d4df3f0805..18846047a5 100644 --- a/terraform/node_resource_abstract_instance.go +++ b/terraform/node_resource_abstract_instance.go @@ -3,7 +3,6 @@ package terraform import ( "fmt" "log" - "reflect" "strings" multierror "github.com/hashicorp/go-multierror" @@ -920,7 +919,7 @@ func (n *NodeAbstractResourceInstance) plan( // If we plan to write or delete sensitive paths from state, // this is an Update action - if action == plans.NoOp && !reflect.DeepEqual(priorPaths, unmarkedPaths) { + if action == plans.NoOp && !marksEqual(unmarkedPaths, priorPaths) { action = plans.Update } @@ -1863,7 +1862,7 @@ func (n *NodeAbstractResourceInstance) apply( // persisted. eqV := unmarkedBefore.Equals(unmarkedAfter) eq := eqV.IsKnown() && eqV.True() - if change.Action == plans.Update && eq && !reflect.DeepEqual(beforePaths, afterPaths) { + if change.Action == plans.Update && eq && !marksEqual(beforePaths, afterPaths) { // Copy the previous state, changing only the value newState = &states.ResourceInstanceObject{ CreateBeforeDestroy: state.CreateBeforeDestroy,