From 3e7be13dff63a0708f119ceb7eb20131efd79a94 Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Thu, 24 Sep 2020 12:41:50 -0400 Subject: [PATCH 1/7] Update ordering for marking/unmarking and asserting plan valid Update when we unmark objects so we can assert the plan is valid, and process UnknownAsNull on the unmarked value --- command/format/diff.go | 7 ++++++- command/format/diff_test.go | 27 +++++++++++++++++++++++++++ states/instance_object.go | 15 +++++++-------- terraform/eval_diff.go | 15 ++++++++------- 4 files changed, 48 insertions(+), 16 deletions(-) diff --git a/command/format/diff.go b/command/format/diff.go index e15d1590a..ad6886811 100644 --- a/command/format/diff.go +++ b/command/format/diff.go @@ -1373,10 +1373,15 @@ 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) + ret = append(ret, value.WithMarks(marks)) } return ret } diff --git a/command/format/diff_test.go b/command/format/diff_test.go index 541770200..61159b266 100644 --- a/command/format/diff_test.go +++ b/command/format/diff_test.go @@ -3644,6 +3644,12 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { cty.StringVal("friends"), cty.StringVal("!"), }), + "nested_block": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "an_attr": cty.StringVal("secretval"), + "another": cty.StringVal("not secret"), + }), + }), }), AfterValMarks: []cty.PathValueMarks{ { @@ -3662,6 +3668,11 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { Path: cty.Path{cty.GetAttrStep{Name: "map_key"}, cty.IndexStep{Key: cty.StringVal("dinner")}}, Marks: cty.NewValueMarks("sensitive"), }, + { + // Nested blocks/sets will mark the whole set/block as sensitive + Path: cty.Path{cty.GetAttrStep{Name: "nested_block"}}, + Marks: cty.NewValueMarks("sensitive"), + }, }, RequiredReplace: cty.NewPathSet(), Tainted: false, @@ -3673,6 +3684,17 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { "map_key": {Type: cty.Map(cty.Number), Optional: true}, "list_field": {Type: cty.List(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}, + "another": {Type: cty.String, Optional: true}, + }, + }, + Nesting: configschema.NestingList, + }, + }, }, ExpectedOutput: ` # test_instance.example will be created + resource "test_instance" "example" { @@ -3688,6 +3710,11 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { + "dinner" = (sensitive) } + map_whole = (sensitive) + + + nested_block { + + an_attr = (sensitive) + + another = (sensitive) + } } `, }, diff --git a/states/instance_object.go b/states/instance_object.go index a9ebf4804..e92ffdc14 100644 --- a/states/instance_object.go +++ b/states/instance_object.go @@ -87,6 +87,11 @@ const ( // so the caller must not mutate the receiver any further once once this // method is called. func (o *ResourceInstanceObject) Encode(ty cty.Type, schemaVersion uint64) (*ResourceInstanceObjectSrc, error) { + // If it contains marks, remove these marks before traversing the + // structure with UnknownAsNull, and save the PathValueMarks + // so we can save them in state. + val, pvm := o.Value.UnmarkDeepWithPaths() + // Our state serialization can't represent unknown values, so we convert // them to nulls here. This is lossy, but nobody should be writing unknown // values here and expecting to get them out again later. @@ -96,15 +101,9 @@ func (o *ResourceInstanceObject) Encode(ty cty.Type, schemaVersion uint64) (*Res // for expression evaluation. The apply step should never produce unknown // values, but if it does it's the responsibility of the caller to detect // and raise an error about that. - val := cty.UnknownAsNull(o.Value) + val = cty.UnknownAsNull(val) - // If it contains marks, save these in state - unmarked := val - var pvm []cty.PathValueMarks - if val.ContainsMarked() { - unmarked, pvm = val.UnmarkDeepWithPaths() - } - src, err := ctyjson.Marshal(unmarked, ty) + src, err := ctyjson.Marshal(val, ty) if err != nil { return nil, err } diff --git a/terraform/eval_diff.go b/terraform/eval_diff.go index 49be3be1c..5818e1ea8 100644 --- a/terraform/eval_diff.go +++ b/terraform/eval_diff.go @@ -279,11 +279,6 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { panic(fmt.Sprintf("PlanResourceChange of %s produced nil value", absAddr.String())) } - // Add the marks back to the planned new value - if len(unmarkedPaths) > 0 { - plannedNewVal = plannedNewVal.MarkWithPaths(unmarkedPaths) - } - // We allow the planned new value to disagree with configuration _values_ // here, since that allows the provider to do special logic like a // DiffSuppressFunc, but we still require that the provider produces @@ -302,7 +297,7 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { return nil, diags.Err() } - if errs := objchange.AssertPlanValid(schema, priorVal, configValIgnored, plannedNewVal); len(errs) > 0 { + if errs := objchange.AssertPlanValid(schema, unmarkedPriorVal, unmarkedConfigVal, plannedNewVal); len(errs) > 0 { if resp.LegacyTypeSystem { // The shimming of the old type system in the legacy SDK is not precise // enough to pass this consistency check, so we'll give it a pass here, @@ -333,6 +328,13 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { } } + // Add the marks back to the planned new value -- this must happen after ignore changes + // have been processed + unmarkedPlannedNewVal := plannedNewVal + if len(unmarkedPaths) > 0 { + plannedNewVal = plannedNewVal.MarkWithPaths(unmarkedPaths) + } + // The provider produces a list of paths to attributes whose changes mean // that we must replace rather than update an existing remote object. // However, we only need to do that if the identified attributes _have_ @@ -395,7 +397,6 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { // Unmark for this test for equality. If only sensitivity has changed, // this does not require an Update or Replace - unmarkedPlannedNewVal, _ := plannedNewVal.UnmarkDeep() eqV := unmarkedPlannedNewVal.Equals(unmarkedPriorVal) eq := eqV.IsKnown() && eqV.True() From 6617c2729cc6c44f7c93f0258e37334571ed1d44 Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Fri, 25 Sep 2020 13:58:33 -0400 Subject: [PATCH 2/7] Test additions for nested blocks Add some coverage for in-place on nested block Add nested block to deletion test --- command/format/diff_test.go | 60 +++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/command/format/diff_test.go b/command/format/diff_test.go index 61159b266..2be3ac2a3 100644 --- a/command/format/diff_test.go +++ b/command/format/diff_test.go @@ -3739,6 +3739,12 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { "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"), + "another": cty.StringVal("not secret"), + }), + }), }), After: cty.ObjectVal(map[string]cty.Value{ "id": cty.StringVal("i-02ae66f368e8518a9"), @@ -3758,6 +3764,12 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { "breakfast": cty.StringVal("cereal"), "dinner": cty.StringVal("pizza"), }), + "nested_block": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "an_attr": cty.StringVal("changed"), + "another": cty.StringVal("not secret"), + }), + }), }), BeforeValMarks: []cty.PathValueMarks{ { @@ -3784,6 +3796,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"}}, + Marks: cty.NewValueMarks("sensitive"), + }, }, RequiredReplace: cty.NewPathSet(), Tainted: false, @@ -3797,6 +3813,17 @@ 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": { + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "an_attr": {Type: cty.String, Optional: true}, + "another": {Type: cty.String, Optional: true}, + }, + }, + Nesting: configschema.NestingList, + }, + }, }, ExpectedOutput: ` # test_instance.example will be updated in-place ~ resource "test_instance" "example" { @@ -3825,6 +3852,13 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { # Warning: this attribute value will no longer be marked as sensitive # 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) + } } `, }, @@ -4026,6 +4060,12 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { "breakfast": cty.StringVal("pizza"), "dinner": cty.StringVal("pizza"), }), + "nested_block": 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{ @@ -4045,6 +4085,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"}}, + Marks: cty.NewValueMarks("sensitive"), + }, }, RequiredReplace: cty.NewPathSet(), Tainted: false, @@ -4056,6 +4100,17 @@ 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": { + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "an_attr": {Type: cty.String, Optional: true}, + "another": {Type: cty.String, Optional: true}, + }, + }, + Nesting: configschema.NestingList, + }, + }, }, ExpectedOutput: ` # test_instance.example will be destroyed - resource "test_instance" "example" { @@ -4070,6 +4125,11 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { - "dinner" = (sensitive) } -> null - map_whole = (sensitive) -> null + + - nested_block { + - an_attr = (sensitive) -> null + - another = (sensitive) -> null + } } `, }, 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 3/7] 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. } } `, From f35b53083786e8837b64948c80fcbac79843f7c4 Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Tue, 29 Sep 2020 13:48:22 -0400 Subject: [PATCH 4/7] Update compatibility checks for blocks to not use marks Remove marks for object compatibility tests to allow apply to continue. Adds a block to the test provider to use in testing, and extends the sensitivity apply test to include a block --- plans/objchange/compatible.go | 5 +++-- terraform/context_apply_test.go | 11 ++++++++++- terraform/context_test.go | 11 +++++++++++ 3 files changed, 24 insertions(+), 3 deletions(-) diff --git a/plans/objchange/compatible.go b/plans/objchange/compatible.go index 181a044f6..5536ad41c 100644 --- a/plans/objchange/compatible.go +++ b/plans/objchange/compatible.go @@ -73,8 +73,9 @@ func assertObjectCompatible(schema *configschema.Block, planned, actual cty.Valu } } for name, blockS := range schema.BlockTypes { - plannedV := planned.GetAttr(name) - actualV := actual.GetAttr(name) + // Unmark values before testing compatibility + plannedV, _ := planned.GetAttr(name).UnmarkDeep() + actualV, _ := actual.GetAttr(name).UnmarkDeep() // As a special case, if there were any blocks whose leaf attributes // are all unknown then we assume (possibly incorrectly) that the diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 4596ae09d..e45a20c26 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -11939,8 +11939,17 @@ variable "sensitive_var" { sensitive = true } +variable "sensitive_id" { + default = "secret id" + sensitive = true +} + resource "test_resource" "foo" { value = var.sensitive_var + + network_interface { + network_interface_id = var.sensitive_id + } }`, }) @@ -11982,7 +11991,7 @@ resource "test_resource" "foo" { t.Fatalf("apply errors: %s", diags.Err()) } - // Now change the variable value + // Now change the variable value for sensitive_var ctx = testContext2(t, &ContextOpts{ Config: m, Providers: map[addrs.Provider]providers.Factory{ diff --git a/terraform/context_test.go b/terraform/context_test.go index a07e275e2..c0e33d7fd 100644 --- a/terraform/context_test.go +++ b/terraform/context_test.go @@ -636,6 +636,17 @@ func testProviderSchema(name string) *ProviderSchema { Optional: true, }, }, + BlockTypes: map[string]*configschema.NestedBlock{ + "network_interface": { + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "network_interface_id": {Type: cty.String, Optional: true}, + "device_index": {Type: cty.Number, Optional: true}, + }, + }, + Nesting: configschema.NestingSet, + }, + }, }, name + "_ami_list": { Attributes: map[string]*configschema.Attribute{ From 2ec95f1abc46b5f9ddadce9ed806b7225ae583f3 Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Tue, 29 Sep 2020 14:02:43 -0400 Subject: [PATCH 5/7] Make an after val unknown to exercise the known check --- command/format/diff_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/format/diff_test.go b/command/format/diff_test.go index 12891bc0e..09b66c373 100644 --- a/command/format/diff_test.go +++ b/command/format/diff_test.go @@ -4025,7 +4025,7 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { }), "nested_block_map": cty.MapVal(map[string]cty.Value{ "foo": cty.ObjectVal(map[string]cty.Value{ - "an_attr": cty.StringVal("changed"), + "an_attr": cty.UnknownVal(cty.String), }), }), }), From 55c96da27eafede07abf26227da50a060c918540 Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Fri, 2 Oct 2020 14:56:50 -0400 Subject: [PATCH 6/7] Move nested block printing to own method for readability --- command/format/diff.go | 77 ++++++++++++++++++++++-------------------- 1 file changed, 41 insertions(+), 36 deletions(-) diff --git a/command/format/diff.go b/command/format/diff.go index 86f03cf5f..f8d7c5656 100644 --- a/command/format/diff.go +++ b/command/format/diff.go @@ -381,42 +381,7 @@ func (p *blockBodyDiffPrinter) writeNestedBlockDiffs(name string, blockS *config // 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("}") + p.writeSensitiveNestedBlockDiff(name, old, new, indent, blankBefore) return 0 } @@ -624,6 +589,46 @@ func (p *blockBodyDiffPrinter) writeNestedBlockDiffs(name string, blockS *config return skippedBlocks } +func (p *blockBodyDiffPrinter) writeSensitiveNestedBlockDiff(name string, old, new cty.Value, indent int, blankBefore bool) { + 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 +} + func (p *blockBodyDiffPrinter) writeNestedBlockDiff(name string, label *string, blockS *configschema.Block, action plans.Action, old, new cty.Value, indent int, path cty.Path) bool { if action == plans.NoOp && p.concise { return true From 111aadd0f0d89e3d577cf122a86828faeac28118 Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Fri, 2 Oct 2020 15:01:17 -0400 Subject: [PATCH 7/7] Extend tests further --- command/format/diff_test.go | 45 ++++++++++++++++++++++++++++++++----- 1 file changed, 39 insertions(+), 6 deletions(-) diff --git a/command/format/diff_test.go b/command/format/diff_test.go index 09b66c373..81225a39f 100644 --- a/command/format/diff_test.go +++ b/command/format/diff_test.go @@ -3650,6 +3650,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("secretval"), + "another": cty.StringVal("not secret"), + }), + }), }), AfterValMarks: []cty.PathValueMarks{ { @@ -3673,6 +3679,10 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { Path: cty.Path{cty.GetAttrStep{Name: "nested_block_list"}}, Marks: cty.NewValueMarks("sensitive"), }, + { + Path: cty.Path{cty.GetAttrStep{Name: "nested_block_set"}}, + Marks: cty.NewValueMarks("sensitive"), + }, }, RequiredReplace: cty.NewPathSet(), Tainted: false, @@ -3694,6 +3704,15 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { }, Nesting: configschema.NestingList, }, + "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.NestingSet, + }, }, }, ExpectedOutput: ` # test_instance.example will be created @@ -3715,6 +3734,11 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { # At least one attribute in this block is (or was) sensitive, # so its contents will not be displayed. } + + + nested_block_set { + # At least one attribute in this block is (or was) sensitive, + # so its contents will not be displayed. + } } `, }, @@ -3826,6 +3850,14 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { "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{ @@ -3864,6 +3896,13 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { # 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 { @@ -3933,9 +3972,6 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { Path: cty.Path{cty.GetAttrStep{Name: "nested_block_single"}}, Marks: cty.NewValueMarks("sensitive"), }, - { - Marks: cty.NewValueMarks("sensitive"), - }, }, RequiredReplace: cty.NewPathSet(), Tainted: false, @@ -4050,9 +4086,6 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { Path: cty.Path{cty.GetAttrStep{Name: "nested_block_map"}}, Marks: cty.NewValueMarks("sensitive"), }, - { - Marks: cty.NewValueMarks("sensitive"), - }, }, AfterValMarks: []cty.PathValueMarks{ {