From 73b1d8b0d176390591da9fc98886899150f7640d Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Tue, 29 Sep 2020 12:59:30 -0400 Subject: [PATCH] Add special diff for nested blocks When a value in a nested block is marked as sensitive, it will result in the behavior of every member of that block being sensitive. As such, show a specialized diff that reduces noise of (sensitive) for many attributes that we won't show. Also update the warning to differentiate between showing a warning for an attribute or warning for blocks. --- command/format/diff.go | 66 +++++++++++++++--- command/format/diff_test.go | 129 ++++++++++++++++++++++++++++++------ 2 files changed, 163 insertions(+), 32 deletions(-) diff --git a/command/format/diff.go b/command/format/diff.go index ad6886811..86f03cf5f 100644 --- a/command/format/diff.go +++ b/command/format/diff.go @@ -338,7 +338,7 @@ func (p *blockBodyDiffPrinter) writeAttrDiff(name string, attrS *configschema.At p.buf.WriteString("\n") - p.writeSensitivityWarning(old, new, indent, action) + p.writeSensitivityWarning(old, new, indent, action, false) p.buf.WriteString(strings.Repeat(" ", indent)) p.writeActionSymbol(action) @@ -377,6 +377,49 @@ func (p *blockBodyDiffPrinter) writeNestedBlockDiffs(name string, blockS *config return skippedBlocks } + // If either the old or the new value is marked, + // Display a special diff because it is irrelevant + // to list all obfuscated attributes as (sensitive) + if old.IsMarked() || new.IsMarked() { + unmarkedOld, _ := old.Unmark() + unmarkedNew, _ := new.Unmark() + eqV := unmarkedNew.Equals(unmarkedOld) + var action plans.Action + switch { + case old.IsNull(): + action = plans.Create + case new.IsNull(): + action = plans.Delete + case !new.IsWhollyKnown() || !old.IsWhollyKnown(): + // "old" should actually always be known due to our contract + // that old values must never be unknown, but we'll allow it + // anyway to be robust. + action = plans.Update + case !eqV.IsKnown() || !eqV.True(): + action = plans.Update + } + + if blankBefore { + p.buf.WriteRune('\n') + } + + // New line before warning printing + p.buf.WriteRune('\n') + p.writeSensitivityWarning(old, new, indent, action, true) + p.buf.WriteString(strings.Repeat(" ", indent)) + p.writeActionSymbol(action) + fmt.Fprintf(p.buf, "%s {", name) + p.buf.WriteRune('\n') + p.buf.WriteString(strings.Repeat(" ", indent+4)) + p.buf.WriteString("# At least one attribute in this block is (or was) sensitive,\n") + p.buf.WriteString(strings.Repeat(" ", indent+4)) + p.buf.WriteString("# so its contents will not be displayed.") + p.buf.WriteRune('\n') + p.buf.WriteString(strings.Repeat(" ", indent+2)) + p.buf.WriteString("}") + return 0 + } + // Where old/new are collections representing a nesting mode other than // NestingSingle, we assume the collection value can never be unknown // since we always produce the container for the nested objects, even if @@ -1129,7 +1172,7 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa oldV := old.Index(kV) newV := new.Index(kV) - p.writeSensitivityWarning(oldV, newV, indent+2, action) + p.writeSensitivityWarning(oldV, newV, indent+2, action, false) p.buf.WriteString(strings.Repeat(" ", indent+2)) p.writeActionSymbol(action) @@ -1307,15 +1350,21 @@ func (p *blockBodyDiffPrinter) writeActionSymbol(action plans.Action) { } } -func (p *blockBodyDiffPrinter) writeSensitivityWarning(old, new cty.Value, indent int, action plans.Action) { +func (p *blockBodyDiffPrinter) writeSensitivityWarning(old, new cty.Value, indent int, action plans.Action, isBlock bool) { // Dont' show this warning for create or delete if action == plans.Create || action == plans.Delete { return } + // Customize the warning based on if it is an attribute or block + diffType := "attribute value" + if isBlock { + diffType = "block" + } + if new.IsMarked() && !old.IsMarked() { p.buf.WriteString(strings.Repeat(" ", indent)) - p.buf.WriteString(p.color.Color("# [yellow]Warning:[reset] this attribute value will be marked as sensitive and will\n")) + p.buf.WriteString(p.color.Color(fmt.Sprintf("# [yellow]Warning:[reset] this %s will be marked as sensitive and will\n", diffType))) p.buf.WriteString(strings.Repeat(" ", indent)) p.buf.WriteString(p.color.Color("# not display in UI output after applying this change\n")) } @@ -1323,7 +1372,7 @@ func (p *blockBodyDiffPrinter) writeSensitivityWarning(old, new cty.Value, inden // Note if changing this attribute will change its sensitivity if old.IsMarked() && !new.IsMarked() { p.buf.WriteString(strings.Repeat(" ", indent)) - p.buf.WriteString(p.color.Color("# [yellow]Warning:[reset] this attribute value will no longer be marked as sensitive\n")) + p.buf.WriteString(p.color.Color(fmt.Sprintf("# [yellow]Warning:[reset] this %s will no longer be marked as sensitive\n", diffType))) p.buf.WriteString(strings.Repeat(" ", indent)) p.buf.WriteString(p.color.Color("# after applying this change\n")) } @@ -1373,15 +1422,10 @@ func ctyCollectionValues(val cty.Value) []cty.Value { return nil } - // Sets with marks mark the whole set as marked, - // so when we unmark it, apply those marks to all members - // of the set - val, marks := val.Unmark() - ret := make([]cty.Value, 0, val.LengthInt()) for it := val.ElementIterator(); it.Next(); { _, value := it.Element() - ret = append(ret, value.WithMarks(marks)) + ret = append(ret, value) } return ret } diff --git a/command/format/diff_test.go b/command/format/diff_test.go index 2be3ac2a3..12891bc0e 100644 --- a/command/format/diff_test.go +++ b/command/format/diff_test.go @@ -3644,7 +3644,7 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { cty.StringVal("friends"), cty.StringVal("!"), }), - "nested_block": cty.ListVal([]cty.Value{ + "nested_block_list": cty.ListVal([]cty.Value{ cty.ObjectVal(map[string]cty.Value{ "an_attr": cty.StringVal("secretval"), "another": cty.StringVal("not secret"), @@ -3670,7 +3670,7 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { }, { // Nested blocks/sets will mark the whole set/block as sensitive - Path: cty.Path{cty.GetAttrStep{Name: "nested_block"}}, + Path: cty.Path{cty.GetAttrStep{Name: "nested_block_list"}}, Marks: cty.NewValueMarks("sensitive"), }, }, @@ -3685,7 +3685,7 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { "list_field": {Type: cty.List(cty.String), Optional: true}, }, BlockTypes: map[string]*configschema.NestedBlock{ - "nested_block": { + "nested_block_list": { Block: configschema.Block{ Attributes: map[string]*configschema.Attribute{ "an_attr": {Type: cty.String, Optional: true}, @@ -3711,9 +3711,9 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { } + map_whole = (sensitive) - + nested_block { - + an_attr = (sensitive) - + another = (sensitive) + + nested_block_list { + # At least one attribute in this block is (or was) sensitive, + # so its contents will not be displayed. } } `, @@ -3742,7 +3742,11 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { "nested_block": cty.ListVal([]cty.Value{ cty.ObjectVal(map[string]cty.Value{ "an_attr": cty.StringVal("secretval"), - "another": cty.StringVal("not secret"), + }), + }), + "nested_block_set": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "an_attr": cty.StringVal("secretval"), }), }), }), @@ -3767,7 +3771,11 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { "nested_block": cty.ListVal([]cty.Value{ cty.ObjectVal(map[string]cty.Value{ "an_attr": cty.StringVal("changed"), - "another": cty.StringVal("not secret"), + }), + }), + "nested_block_set": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "an_attr": cty.StringVal("changed"), }), }), }), @@ -3800,6 +3808,10 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { 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, @@ -3814,14 +3826,13 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { "map_whole": {Type: cty.Map(cty.String), Optional: true}, }, BlockTypes: map[string]*configschema.NestedBlock{ - "nested_block": { + "nested_block_set": { Block: configschema.Block{ Attributes: map[string]*configschema.Attribute{ "an_attr": {Type: cty.String, Optional: true}, - "another": {Type: cty.String, Optional: true}, }, }, - Nesting: configschema.NestingList, + Nesting: configschema.NestingSet, }, }, }, @@ -3853,11 +3864,11 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { # after applying this change ~ special = (sensitive) - ~ nested_block { - # Warning: this attribute value will no longer be marked as sensitive - # after applying this change - ~ an_attr = (sensitive) - # (1 unchanged attribute hidden) + # 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. } } `, @@ -3879,6 +3890,9 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { "breakfast": cty.StringVal("pizza"), "dinner": cty.StringVal("pizza"), }), + "nested_block_single": cty.ObjectVal(map[string]cty.Value{ + "an_attr": cty.StringVal("original"), + }), }), After: cty.ObjectVal(map[string]cty.Value{ "id": cty.StringVal("i-02ae66f368e8518a9"), @@ -3894,6 +3908,9 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { "breakfast": cty.StringVal("cereal"), "dinner": cty.StringVal("pizza"), }), + "nested_block_single": cty.ObjectVal(map[string]cty.Value{ + "an_attr": cty.StringVal("changed"), + }), }), AfterValMarks: []cty.PathValueMarks{ { @@ -3912,6 +3929,13 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { Path: cty.Path{cty.GetAttrStep{Name: "map_whole"}}, Marks: cty.NewValueMarks("sensitive"), }, + { + Path: cty.Path{cty.GetAttrStep{Name: "nested_block_single"}}, + Marks: cty.NewValueMarks("sensitive"), + }, + { + Marks: cty.NewValueMarks("sensitive"), + }, }, RequiredReplace: cty.NewPathSet(), Tainted: false, @@ -3922,6 +3946,16 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { "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_single": { + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "an_attr": {Type: cty.String, Optional: true}, + }, + }, + Nesting: configschema.NestingSingle, + }, + }, }, ExpectedOutput: ` # test_instance.example will be updated in-place ~ resource "test_instance" "example" { @@ -3940,6 +3974,13 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { # Warning: this attribute value will be marked as sensitive and will # not display in UI output after applying this change ~ map_whole = (sensitive) + + # Warning: this block will be marked as sensitive and will + # not display in UI output after applying this change + ~ nested_block_single { + # At least one attribute in this block is (or was) sensitive, + # so its contents will not be displayed. + } } `, }, @@ -3961,6 +4002,11 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { "breakfast": cty.StringVal("pizza"), "dinner": cty.StringVal("pizza"), }), + "nested_block_map": cty.MapVal(map[string]cty.Value{ + "foo": cty.ObjectVal(map[string]cty.Value{ + "an_attr": cty.StringVal("original"), + }), + }), }), After: cty.ObjectVal(map[string]cty.Value{ "id": cty.StringVal("i-02ae66f368e8518a9"), @@ -3977,6 +4023,11 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { "breakfast": cty.StringVal("cereal"), "dinner": cty.StringVal("pizza"), }), + "nested_block_map": cty.MapVal(map[string]cty.Value{ + "foo": cty.ObjectVal(map[string]cty.Value{ + "an_attr": cty.StringVal("changed"), + }), + }), }), BeforeValMarks: []cty.PathValueMarks{ { @@ -3995,6 +4046,13 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { Path: cty.Path{cty.GetAttrStep{Name: "map_whole"}}, Marks: cty.NewValueMarks("sensitive"), }, + { + Path: cty.Path{cty.GetAttrStep{Name: "nested_block_map"}}, + Marks: cty.NewValueMarks("sensitive"), + }, + { + Marks: cty.NewValueMarks("sensitive"), + }, }, AfterValMarks: []cty.PathValueMarks{ { @@ -4013,6 +4071,10 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { Path: cty.Path{cty.GetAttrStep{Name: "map_whole"}}, Marks: cty.NewValueMarks("sensitive"), }, + { + Path: cty.Path{cty.GetAttrStep{Name: "nested_block_map"}}, + Marks: cty.NewValueMarks("sensitive"), + }, }, RequiredReplace: cty.NewPathSet(), Tainted: false, @@ -4024,6 +4086,16 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { "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_map": { + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "an_attr": {Type: cty.String, Optional: true}, + }, + }, + Nesting: configschema.NestingMap, + }, + }, }, ExpectedOutput: ` # test_instance.example will be updated in-place ~ resource "test_instance" "example" { @@ -4039,6 +4111,11 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { # (1 unchanged element hidden) } ~ map_whole = (sensitive) + + ~ nested_block_map { + # At least one attribute in this block is (or was) sensitive, + # so its contents will not be displayed. + } } `, }, @@ -4066,6 +4143,12 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { "another": cty.StringVal("not secret"), }), }), + "nested_block_set": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "an_attr": cty.StringVal("secret"), + "another": cty.StringVal("not secret"), + }), + }), }), After: cty.NullVal(cty.EmptyObject), BeforeValMarks: []cty.PathValueMarks{ @@ -4089,6 +4172,10 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { 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, @@ -4101,14 +4188,14 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { "map_whole": {Type: cty.Map(cty.String), Optional: true}, }, BlockTypes: map[string]*configschema.NestedBlock{ - "nested_block": { + "nested_block_set": { Block: configschema.Block{ Attributes: map[string]*configschema.Attribute{ "an_attr": {Type: cty.String, Optional: true}, "another": {Type: cty.String, Optional: true}, }, }, - Nesting: configschema.NestingList, + Nesting: configschema.NestingSet, }, }, }, @@ -4126,9 +4213,9 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { } -> null - map_whole = (sensitive) -> null - - nested_block { - - an_attr = (sensitive) -> null - - another = (sensitive) -> null + - nested_block_set { + # At least one attribute in this block is (or was) sensitive, + # so its contents will not be displayed. } } `,