From 0520f143a221696b88086f75a219fda727fff124 Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Thu, 24 Sep 2020 16:29:30 -0400 Subject: [PATCH 1/3] Add diff coverage for maps Considers wholly marked maps as well as map keys --- command/format/diff.go | 35 ++++++-- command/format/diff_test.go | 170 +++++++++++++++++++++++++++++++++--- 2 files changed, 184 insertions(+), 21 deletions(-) diff --git a/command/format/diff.go b/command/format/diff.go index 66367dc84..b274f5a0d 100644 --- a/command/format/diff.go +++ b/command/format/diff.go @@ -777,12 +777,9 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa // However, these specialized implementations can apply only if both // values are known and non-null. if old.IsKnown() && new.IsKnown() && !old.IsNull() && !new.IsNull() && typesEqual { - // Create unmarked values for comparisons - unmarkedOld, oldMarks := old.UnmarkDeep() - unmarkedNew, newMarks := new.UnmarkDeep() switch { case ty == cty.Bool || ty == cty.Number: - if len(oldMarks) > 0 || len(newMarks) > 0 { + if old.IsMarked() || new.IsMarked() { p.buf.WriteString("(sensitive)") return } @@ -793,7 +790,7 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa // For single-line strings that don't parse as JSON we just fall // out of this switch block and do the default old -> new rendering. - if len(oldMarks) > 0 || len(newMarks) > 0 { + if old.IsMarked() || new.IsMarked() { p.buf.WriteString("(sensitive)") return } @@ -1079,6 +1076,18 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa } p.buf.WriteString("\n") + // If the whole map is marked, we need to show our + // sensitive block warning rather than attempt to iterate + // through a marked map + if old.IsMarked() || new.IsMarked() { + p.buf.WriteString(strings.Repeat(" ", indent+2)) + p.buf.WriteString("# This map is (or was) sensitive and will not be displayed") + p.buf.WriteString("\n") + p.buf.WriteString(strings.Repeat(" ", indent)) + p.buf.WriteString("}") + return + } + var allKeys []string keyLen := 0 for it := old.ElementIterator(); it.Next(); { @@ -1114,10 +1123,18 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa action = plans.Create } else if new.HasIndex(kV).False() { action = plans.Delete - } else if eqV := unmarkedOld.Index(kV).Equals(unmarkedNew.Index(kV)); eqV.IsKnown() && eqV.True() { - action = plans.NoOp - } else { - action = plans.Update + } + + // 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() { + action = plans.NoOp + } else { + action = plans.Update + } } if action == plans.NoOp && p.concise { diff --git a/command/format/diff_test.go b/command/format/diff_test.go index 26111a9d8..ffb21e644 100644 --- a/command/format/diff_test.go +++ b/command/format/diff_test.go @@ -3631,6 +3631,14 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { After: cty.ObjectVal(map[string]cty.Value{ "id": cty.StringVal("i-02ae66f368e8518a9"), "ami": cty.StringVal("ami-123"), + "map_key": cty.MapVal(map[string]cty.Value{ + "breakfast": cty.NumberIntVal(800), + "dinner": cty.NumberIntVal(2000), + }), + "map_whole": cty.MapVal(map[string]cty.Value{ + "breakfast": cty.StringVal("pizza"), + "dinner": cty.StringVal("pizza"), + }), "list_field": cty.ListVal([]cty.Value{ cty.StringVal("hello"), cty.StringVal("friends"), @@ -3646,6 +3654,14 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { Path: cty.Path{cty.GetAttrStep{Name: "list_field"}, cty.IndexStep{Key: cty.NumberIntVal(1)}}, Marks: cty.NewValueMarks("sensitive"), }, + { + Path: cty.Path{cty.GetAttrStep{Name: "map_whole"}}, + Marks: cty.NewValueMarks("sensitive"), + }, + { + Path: cty.Path{cty.GetAttrStep{Name: "map_key"}, cty.IndexStep{Key: cty.StringVal("dinner")}}, + Marks: cty.NewValueMarks("sensitive"), + }, }, RequiredReplace: cty.NewPathSet(), Tainted: false, @@ -3653,6 +3669,8 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { Attributes: map[string]*configschema.Attribute{ "id": {Type: cty.String, Optional: true, Computed: true}, "ami": {Type: cty.String, Optional: true}, + "map_whole": {Type: cty.Map(cty.String), Optional: true}, + "map_key": {Type: cty.Map(cty.Number), Optional: true}, "list_field": {Type: cty.List(cty.String), Optional: true}, }, }, @@ -3665,6 +3683,11 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { + (sensitive), + "!", ] + + map_key = { + + "breakfast" = 800 + + "dinner" = (sensitive) + } + + map_whole = (sensitive) } `, }, @@ -3681,6 +3704,14 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { 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"), + }), }), After: cty.ObjectVal(map[string]cty.Value{ "id": cty.StringVal("i-02ae66f368e8518a9"), @@ -3692,6 +3723,14 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { cty.StringVal("friends"), cty.StringVal("."), }), + "map_key": cty.MapVal(map[string]cty.Value{ + "breakfast": cty.NumberIntVal(800), + "dinner": cty.NumberIntVal(1900), + }), + "map_whole": cty.MapVal(map[string]cty.Value{ + "breakfast": cty.StringVal("cereal"), + "dinner": cty.StringVal("pizza"), + }), }), BeforeValMarks: []cty.PathValueMarks{ { @@ -3710,6 +3749,14 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { 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"), + }, }, RequiredReplace: cty.NewPathSet(), Tainted: false, @@ -3720,6 +3767,8 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { "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}, }, }, ExpectedOutput: ` # test_instance.example will be updated in-place @@ -3734,6 +3783,17 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { - (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 = { + # This map is (or was) sensitive and will not be displayed + } # Warning: this attribute value will no longer be marked as sensitive # after applying this change ~ some_number = (sensitive) @@ -3748,25 +3808,33 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { Mode: addrs.ManagedResourceMode, Before: cty.ObjectVal(map[string]cty.Value{ "id": cty.StringVal("i-02ae66f368e8518a9"), - "tags": cty.MapVal(map[string]cty.Value{ - "name": cty.StringVal("anna a"), - "address": cty.StringVal("123 Main St"), - }), "list_field": cty.ListVal([]cty.Value{ cty.StringVal("hello"), cty.StringVal("friends"), }), + "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"), + }), }), After: cty.ObjectVal(map[string]cty.Value{ "id": cty.StringVal("i-02ae66f368e8518a9"), - "tags": cty.MapVal(map[string]cty.Value{ - "name": cty.StringVal("anna b"), - "address": cty.StringVal("123 Main Ave"), - }), "list_field": cty.ListVal([]cty.Value{ cty.StringVal("goodbye"), cty.StringVal("friends"), }), + "map_key": cty.MapVal(map[string]cty.Value{ + "breakfast": cty.NumberIntVal(700), + "dinner": cty.NumberIntVal(2100), // sensitive key + }), + "map_whole": cty.MapVal(map[string]cty.Value{ + "breakfast": cty.StringVal("cereal"), + "dinner": cty.StringVal("pizza"), + }), }), AfterValMarks: []cty.PathValueMarks{ { @@ -3777,14 +3845,23 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { Path: cty.Path{cty.GetAttrStep{Name: "list_field"}, cty.IndexStep{Key: cty.NumberIntVal(0)}}, 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"), + }, }, RequiredReplace: cty.NewPathSet(), Tainted: false, Schema: &configschema.Block{ Attributes: map[string]*configschema.Attribute{ "id": {Type: cty.String, Optional: true, Computed: true}, - "tags": {Type: cty.Map(cty.String), Optional: true}, "list_field": {Type: cty.List(cty.String), Optional: true}, + "map_key": {Type: cty.Map(cty.Number), Optional: true}, + "map_whole": {Type: cty.Map(cty.String), Optional: true}, }, }, ExpectedOutput: ` # test_instance.example will be updated in-place @@ -3795,11 +3872,16 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { + (sensitive), "friends", ] - ~ tags = { + ~ map_key = { + ~ "breakfast" = 800 -> 700 # Warning: this attribute value will be marked as sensitive and will # not display in UI output after applying this change - ~ "address" = (sensitive) - ~ "name" = "anna a" -> "anna b" + ~ "dinner" = (sensitive) + } + # Warning: this attribute value will be marked as sensitive and will + # not display in UI output after applying this change + ~ map_whole = { + # This map is (or was) sensitive and will not be displayed } } `, @@ -3814,6 +3896,14 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { cty.StringVal("hello"), cty.StringVal("friends"), }), + "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"), + }), }), After: cty.ObjectVal(map[string]cty.Value{ "id": cty.StringVal("i-02ae66f368e8518a9"), @@ -3822,6 +3912,14 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { cty.StringVal("goodbye"), cty.StringVal("friends"), }), + "map_key": cty.MapVal(map[string]cty.Value{ + "breakfast": cty.NumberIntVal(800), + "dinner": cty.NumberIntVal(1800), // sensitive key + }), + "map_whole": cty.MapVal(map[string]cty.Value{ + "breakfast": cty.StringVal("cereal"), + "dinner": cty.StringVal("pizza"), + }), }), BeforeValMarks: []cty.PathValueMarks{ { @@ -3832,6 +3930,14 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { Path: cty.Path{cty.GetAttrStep{Name: "list_field"}, cty.IndexStep{Key: cty.NumberIntVal(0)}}, 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"), + }, }, AfterValMarks: []cty.PathValueMarks{ { @@ -3842,6 +3948,14 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { Path: cty.Path{cty.GetAttrStep{Name: "list_field"}, cty.IndexStep{Key: cty.NumberIntVal(0)}}, 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"), + }, }, RequiredReplace: cty.NewPathSet(), Tainted: false, @@ -3850,6 +3964,8 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { "id": {Type: cty.String, Optional: true, Computed: true}, "ami": {Type: cty.String, Optional: true}, "list_field": {Type: cty.List(cty.String), Optional: true}, + "map_key": {Type: cty.Map(cty.Number), Optional: true}, + "map_whole": {Type: cty.Map(cty.String), Optional: true}, }, }, ExpectedOutput: ` # test_instance.example will be updated in-place @@ -3861,6 +3977,13 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { + (sensitive), "friends", ] + ~ map_key = { + ~ "dinner" = (sensitive) + # (1 unchanged element hidden) + } + ~ map_whole = { + # This map is (or was) sensitive and will not be displayed + } } `, }, @@ -3874,6 +3997,14 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { cty.StringVal("hello"), cty.StringVal("friends"), }), + "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"), + }), }), After: cty.NullVal(cty.EmptyObject), BeforeValMarks: []cty.PathValueMarks{ @@ -3885,6 +4016,14 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { Path: cty.Path{cty.GetAttrStep{Name: "list_field"}, cty.IndexStep{Key: cty.NumberIntVal(1)}}, 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"), + }, }, RequiredReplace: cty.NewPathSet(), Tainted: false, @@ -3893,6 +4032,8 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { "id": {Type: cty.String, Optional: true, Computed: true}, "ami": {Type: cty.String, Optional: true}, "list_field": {Type: cty.List(cty.String), Optional: true}, + "map_key": {Type: cty.Map(cty.Number), Optional: true}, + "map_whole": {Type: cty.Map(cty.String), Optional: true}, }, }, ExpectedOutput: ` # test_instance.example will be destroyed @@ -3903,6 +4044,11 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { - "hello", - (sensitive), ] -> null + - map_key = { + - "breakfast" = 800 + - "dinner" = (sensitive) + } -> null + - map_whole = (sensitive) -> null } `, }, From 178035163686b41ed83fbd43e76db0a2676adfab Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Thu, 1 Oct 2020 14:30:33 -0400 Subject: [PATCH 2/3] If the whole map is marked, have the same sensitivity behavior as a single value --- command/format/diff.go | 17 +++++------------ command/format/diff_test.go | 12 +++--------- 2 files changed, 8 insertions(+), 21 deletions(-) diff --git a/command/format/diff.go b/command/format/diff.go index b274f5a0d..b7b5b6ae2 100644 --- a/command/format/diff.go +++ b/command/format/diff.go @@ -1070,24 +1070,17 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa return case ty.IsMapType(): + if old.IsMarked() || new.IsMarked() { + p.buf.WriteString("(sensitive)") + return + } + p.buf.WriteString("{") if p.pathForcesNewResource(path) { p.buf.WriteString(p.color.Color(forcesNewResourceCaption)) } p.buf.WriteString("\n") - // If the whole map is marked, we need to show our - // sensitive block warning rather than attempt to iterate - // through a marked map - if old.IsMarked() || new.IsMarked() { - p.buf.WriteString(strings.Repeat(" ", indent+2)) - p.buf.WriteString("# This map is (or was) sensitive and will not be displayed") - p.buf.WriteString("\n") - p.buf.WriteString(strings.Repeat(" ", indent)) - p.buf.WriteString("}") - return - } - var allKeys []string keyLen := 0 for it := old.ElementIterator(); it.Next(); { diff --git a/command/format/diff_test.go b/command/format/diff_test.go index ffb21e644..541770200 100644 --- a/command/format/diff_test.go +++ b/command/format/diff_test.go @@ -3791,9 +3791,7 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { } # Warning: this attribute value will no longer be marked as sensitive # after applying this change - ~ map_whole = { - # This map is (or was) sensitive and will not be displayed - } + ~ map_whole = (sensitive) # Warning: this attribute value will no longer be marked as sensitive # after applying this change ~ some_number = (sensitive) @@ -3880,9 +3878,7 @@ 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 = { - # This map is (or was) sensitive and will not be displayed - } + ~ map_whole = (sensitive) } `, }, @@ -3981,9 +3977,7 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { ~ "dinner" = (sensitive) # (1 unchanged element hidden) } - ~ map_whole = { - # This map is (or was) sensitive and will not be displayed - } + ~ map_whole = (sensitive) } `, }, From 52b6f7f53e68737a76752aca3d66b0135a84807e Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Thu, 1 Oct 2020 14:34:44 -0400 Subject: [PATCH 3/3] Move common IsMarked checks above switch statement --- command/format/diff.go | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/command/format/diff.go b/command/format/diff.go index b7b5b6ae2..e15d1590a 100644 --- a/command/format/diff.go +++ b/command/format/diff.go @@ -777,23 +777,18 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa // However, these specialized implementations can apply only if both // values are known and non-null. if old.IsKnown() && new.IsKnown() && !old.IsNull() && !new.IsNull() && typesEqual { + if old.IsMarked() || new.IsMarked() { + p.buf.WriteString("(sensitive)") + return + } + switch { - case ty == cty.Bool || ty == cty.Number: - if old.IsMarked() || new.IsMarked() { - p.buf.WriteString("(sensitive)") - return - } case ty == cty.String: // We have special behavior for both multi-line strings in general // and for strings that can parse as JSON. For the JSON handling // to apply, both old and new must be valid JSON. // For single-line strings that don't parse as JSON we just fall // out of this switch block and do the default old -> new rendering. - - if old.IsMarked() || new.IsMarked() { - p.buf.WriteString("(sensitive)") - return - } oldS := old.AsString() newS := new.AsString() @@ -1070,11 +1065,6 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa return case ty.IsMapType(): - if old.IsMarked() || new.IsMarked() { - p.buf.WriteString("(sensitive)") - return - } - p.buf.WriteString("{") if p.pathForcesNewResource(path) { p.buf.WriteString(p.color.Color(forcesNewResourceCaption))