From 79a31f627b1b5229e91bace4afd199dc38d58db9 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 17 Dec 2020 12:03:28 -0500 Subject: [PATCH 1/2] 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 000000000..8e2a32607 --- /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 000000000..efb3b7e9b --- /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 d4df3f080..18846047a 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, From 1309b36b83524aa132c1f646498b1e2cc6f38bf5 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 17 Dec 2020 12:41:34 -0500 Subject: [PATCH 2/2] plan context test for mysterious changes This plan would occasionally show changes when there weren't any due to the sensitive marks being compared incorrectly. --- terraform/context_plan_test.go | 57 ++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index 60f84550c..816525129 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -6668,3 +6668,60 @@ resource "test_instance" "a" { t.Fatal(diags.Err()) } } + +func TestContext2Plan_noSensitivityChange(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ` +variable "sensitive_var" { + default = "hello" + sensitive = true +} + +resource "test_resource" "foo" { + value = var.sensitive_var + sensitive_value = var.sensitive_var +}`, + }) + + p := testProvider("test") + p.ApplyResourceChangeFn = testApplyFn + p.PlanResourceChangeFn = testDiffFn + + ctx := testContext2(t, &ContextOpts{ + Config: m, + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + State: states.BuildState(func(s *states.SyncState) { + s.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_resource", + Name: "foo", + }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"foo", "value":"hello", "sensitive_value":"hello", "network_interface":[]}`), + AttrSensitivePaths: []cty.PathValueMarks{ + cty.PathValueMarks{Path: cty.Path{cty.GetAttrStep{Name: "value"}}, Marks: cty.NewValueMarks("sensitive")}, + cty.PathValueMarks{Path: cty.Path{cty.GetAttrStep{Name: "sensitive_value"}}, Marks: cty.NewValueMarks("sensitive")}, + }, + }, + addrs.AbsProviderConfig{ + Provider: addrs.NewDefaultProvider("test"), + Module: addrs.RootModule, + }, + ) + }), + }) + plan, diags := ctx.Plan() + if diags.HasErrors() { + t.Fatal(diags.Err()) + } + + for _, c := range plan.Changes.Resources { + if c.Action != plans.NoOp { + t.Fatalf("expected no changes, got %s for %q", c.Action, c.Addr) + } + } +}