From c798dc98db3f9b8fc6b07395168081b6a9e9a88e Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Tue, 13 Oct 2020 13:55:16 -0400 Subject: [PATCH] command: Show diffs when only sensitivity changes When an attribute's sensitivity changes, but its value remains the same, we consider this an update operation for the plan. This commit updates the diff renderer to match this, detecting and displaying the change in sensitivity. Previously, the renderer would detect no changes to the value of the attribute, and consider it a no-op action. This resulted in suppression of the attribute when the plan is in concise mode. This is achieved with a new helper function, ctyEqualValueAndMarks. We call this function whenever we want to check that two values are equal in order to determine whether the action is update or no-op. --- command/format/diff.go | 33 ++++--- command/format/diff_test.go | 170 ++++++++++++++++++++++++++++++++++++ 2 files changed, 186 insertions(+), 17 deletions(-) diff --git a/command/format/diff.go b/command/format/diff.go index 126d5ddbb..7a3df5f09 100644 --- a/command/format/diff.go +++ b/command/format/diff.go @@ -307,12 +307,6 @@ func (p *blockBodyDiffPrinter) writeBlockBodyDiff(schema *configschema.Block, ol func getPlanActionAndShow(old cty.Value, new cty.Value) (plans.Action, bool) { var action plans.Action showJustNew := false - if old.ContainsMarked() { - old, _ = old.UnmarkDeep() - } - if new.ContainsMarked() { - new, _ = new.UnmarkDeep() - } switch { case old.IsNull(): action = plans.Create @@ -590,9 +584,6 @@ func (p *blockBodyDiffPrinter) writeNestedBlockDiffs(name string, blockS *config } func (p *blockBodyDiffPrinter) writeSensitiveNestedBlockDiff(name string, old, new cty.Value, indent int, blankBefore bool, path cty.Path) { - unmarkedOld, _ := old.Unmark() - unmarkedNew, _ := new.Unmark() - eqV := unmarkedNew.Equals(unmarkedOld) var action plans.Action switch { case old.IsNull(): @@ -604,7 +595,7 @@ func (p *blockBodyDiffPrinter) writeSensitiveNestedBlockDiff(name string, old, n // that old values must never be unknown, but we'll allow it // anyway to be robust. action = plans.Update - case !eqV.IsKnown() || !eqV.True(): + case !ctyEqualValueAndMarks(old, new): action = plans.Update } @@ -1162,12 +1153,8 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa action = plans.Delete } - // Use unmarked values for equality testing if old.HasIndex(kV).True() && new.HasIndex(kV).True() { - unmarkedOld, _ := old.Index(kV).Unmark() - unmarkedNew, _ := new.Index(kV).Unmark() - eqV := unmarkedOld.Equals(unmarkedNew) - if eqV.IsKnown() && eqV.True() { + if ctyEqualValueAndMarks(old.Index(kV), new.Index(kV)) { action = plans.NoOp } else { action = plans.Update @@ -1269,7 +1256,7 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa action = plans.Create } else if !new.Type().HasAttribute(kV) { action = plans.Delete - } else if eqV := old.GetAttr(kV).Equals(new.GetAttr(kV)); eqV.IsKnown() && eqV.True() { + } else if ctyEqualValueAndMarks(old.GetAttr(kV), new.GetAttr(kV)) { action = plans.NoOp } else { action = plans.Update @@ -1493,11 +1480,23 @@ func ctySequenceDiff(old, new []cty.Value) []*plans.Change { return ret } +// ctyEqualValueAndMarks checks equality of two possibly-marked values, +// considering partially-unknown values and equal values with different marks +// as inequal func ctyEqualWithUnknown(old, new cty.Value) bool { if !old.IsWhollyKnown() || !new.IsWhollyKnown() { return false } - return old.Equals(new).True() + return ctyEqualValueAndMarks(old, new) +} + +// ctyEqualValueAndMarks checks equality of two possibly-marked values, +// considering equal values with different marks as inequal +func ctyEqualValueAndMarks(old, new cty.Value) bool { + oldUnmarked, oldMarks := old.UnmarkDeep() + newUnmarked, newMarks := new.UnmarkDeep() + sameValue := oldUnmarked.Equals(newUnmarked) + return sameValue.IsKnown() && sameValue.True() && oldMarks.Equal(newMarks) } // ctyTypesEqual checks equality of two types more loosely diff --git a/command/format/diff_test.go b/command/format/diff_test.go index b3ba9af76..076b1a9a2 100644 --- a/command/format/diff_test.go +++ b/command/format/diff_test.go @@ -4150,6 +4150,176 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { # so its contents will not be displayed. } } +`, + }, + "in-place update - value unchanged, sensitivity changes": { + Action: plans.Update, + Mode: addrs.ManagedResourceMode, + Before: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "ami": cty.StringVal("ami-BEFORE"), + "special": cty.BoolVal(true), + "some_number": cty.NumberIntVal(1), + "list_field": cty.ListVal([]cty.Value{ + cty.StringVal("hello"), + cty.StringVal("friends"), + cty.StringVal("!"), + }), + "map_key": cty.MapVal(map[string]cty.Value{ + "breakfast": cty.NumberIntVal(800), + "dinner": cty.NumberIntVal(2000), // sensitive key + }), + "map_whole": cty.MapVal(map[string]cty.Value{ + "breakfast": cty.StringVal("pizza"), + "dinner": cty.StringVal("pizza"), + }), + "nested_block": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "an_attr": cty.StringVal("secretval"), + }), + }), + "nested_block_set": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "an_attr": cty.StringVal("secretval"), + }), + }), + }), + After: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "ami": cty.StringVal("ami-BEFORE"), + "special": cty.BoolVal(true), + "some_number": cty.NumberIntVal(1), + "list_field": cty.ListVal([]cty.Value{ + cty.StringVal("hello"), + cty.StringVal("friends"), + cty.StringVal("!"), + }), + "map_key": cty.MapVal(map[string]cty.Value{ + "breakfast": cty.NumberIntVal(800), + "dinner": cty.NumberIntVal(2000), // sensitive key + }), + "map_whole": cty.MapVal(map[string]cty.Value{ + "breakfast": cty.StringVal("pizza"), + "dinner": cty.StringVal("pizza"), + }), + "nested_block": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "an_attr": cty.StringVal("secretval"), + }), + }), + "nested_block_set": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "an_attr": cty.StringVal("secretval"), + }), + }), + }), + BeforeValMarks: []cty.PathValueMarks{ + { + Path: cty.Path{cty.GetAttrStep{Name: "ami"}}, + Marks: cty.NewValueMarks("sensitive"), + }, + { + Path: cty.Path{cty.GetAttrStep{Name: "special"}}, + Marks: cty.NewValueMarks("sensitive"), + }, + { + Path: cty.Path{cty.GetAttrStep{Name: "some_number"}}, + Marks: cty.NewValueMarks("sensitive"), + }, + { + Path: cty.Path{cty.GetAttrStep{Name: "list_field"}, cty.IndexStep{Key: cty.NumberIntVal(2)}}, + Marks: cty.NewValueMarks("sensitive"), + }, + { + Path: cty.Path{cty.GetAttrStep{Name: "map_key"}, cty.IndexStep{Key: cty.StringVal("dinner")}}, + Marks: cty.NewValueMarks("sensitive"), + }, + { + Path: cty.Path{cty.GetAttrStep{Name: "map_whole"}}, + Marks: cty.NewValueMarks("sensitive"), + }, + { + Path: cty.Path{cty.GetAttrStep{Name: "nested_block"}}, + Marks: cty.NewValueMarks("sensitive"), + }, + { + Path: cty.Path{cty.GetAttrStep{Name: "nested_block_set"}}, + Marks: cty.NewValueMarks("sensitive"), + }, + }, + RequiredReplace: cty.NewPathSet(), + Tainted: false, + Schema: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "id": {Type: cty.String, Optional: true, Computed: true}, + "ami": {Type: cty.String, Optional: true}, + "list_field": {Type: cty.List(cty.String), Optional: true}, + "special": {Type: cty.Bool, Optional: true}, + "some_number": {Type: cty.Number, Optional: true}, + "map_key": {Type: cty.Map(cty.Number), Optional: true}, + "map_whole": {Type: cty.Map(cty.String), Optional: true}, + }, + BlockTypes: map[string]*configschema.NestedBlock{ + "nested_block": { + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "an_attr": {Type: cty.String, Optional: true}, + }, + }, + Nesting: configschema.NestingList, + }, + "nested_block_set": { + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "an_attr": {Type: cty.String, Optional: true}, + }, + }, + Nesting: configschema.NestingSet, + }, + }, + }, + ExpectedOutput: ` # test_instance.example will be updated in-place + ~ resource "test_instance" "example" { + # Warning: this attribute value will no longer be marked as sensitive + # after applying this change + ~ ami = (sensitive) + id = "i-02ae66f368e8518a9" + ~ list_field = [ + # (1 unchanged element hidden) + "friends", + - (sensitive), + + "!", + ] + ~ map_key = { + # Warning: this attribute value will no longer be marked as sensitive + # after applying this change + ~ "dinner" = (sensitive) + # (1 unchanged element hidden) + } + # Warning: this attribute value will no longer be marked as sensitive + # after applying this change + ~ map_whole = (sensitive) + # Warning: this attribute value will no longer be marked as sensitive + # after applying this change + ~ some_number = (sensitive) + # Warning: this attribute value will no longer be marked as sensitive + # after applying this change + ~ special = (sensitive) + + # Warning: this block will no longer be marked as sensitive + # after applying this change + ~ nested_block { + # At least one attribute in this block is (or was) sensitive, + # so its contents will not be displayed. + } + + # Warning: this block will no longer be marked as sensitive + # after applying this change + ~ nested_block_set { + # At least one attribute in this block is (or was) sensitive, + # so its contents will not be displayed. + } + } `, }, "deletion": {