From 8b9fa6d05fd45273c2f145988097d59c13417ccb Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 28 Mar 2019 16:21:58 -0400 Subject: [PATCH 1/5] add test provider coverage around unknown vals --- builtin/providers/test/resource.go | 14 ++ builtin/providers/test/resource_list.go | 20 +++ builtin/providers/test/resource_list_test.go | 129 +++++++++++++++++++ builtin/providers/test/resource_test.go | 126 ++++++++++++++++++ 4 files changed, 289 insertions(+) diff --git a/builtin/providers/test/resource.go b/builtin/providers/test/resource.go index c8556a2b71..32f1049c70 100644 --- a/builtin/providers/test/resource.go +++ b/builtin/providers/test/resource.go @@ -18,6 +18,13 @@ func testResource() *schema.Resource { State: schema.ImportStatePassthrough, }, + CustomizeDiff: func(d *schema.ResourceDiff, _ interface{}) error { + if d.HasChange("required") { + d.SetNewComputed("planned_computed") + } + return nil + }, + Schema: map[string]*schema.Schema{ "required": { Type: schema.TypeString, @@ -129,6 +136,11 @@ func testResource() *schema.Resource { Optional: true, Description: "return and error during apply", }, + "planned_computed": { + Type: schema.TypeString, + Computed: true, + Description: "copied the required field during apply, and plans computed when changed", + }, }, } } @@ -164,6 +176,8 @@ func testResourceRead(d *schema.ResourceData, meta interface{}) error { d.Set("computed_list", []string{"listval1", "listval2"}) d.Set("computed_set", []string{"setval1", "setval2"}) + d.Set("planned_computed", d.Get("required")) + // if there is no "set" value, erroneously set it to an empty set. This // might change a null value to an empty set, but we should be able to // ignore that. diff --git a/builtin/providers/test/resource_list.go b/builtin/providers/test/resource_list.go index a3d01b79ec..36db61993d 100644 --- a/builtin/providers/test/resource_list.go +++ b/builtin/providers/test/resource_list.go @@ -11,6 +11,13 @@ func testResourceList() *schema.Resource { Update: testResourceListUpdate, Delete: testResourceListDelete, + CustomizeDiff: func(d *schema.ResourceDiff, _ interface{}) error { + if d.HasChange("dependent_list") { + d.SetNewComputed("computed_list") + } + return nil + }, + Schema: map[string]*schema.Schema{ "list_block": { Type: schema.TypeList, @@ -55,6 +62,19 @@ func testResourceList() *schema.Resource { }, }, }, + "sublist_block_optional": { + Type: schema.TypeList, + Optional: true, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "list": { + Type: schema.TypeList, + Optional: true, + Elem: &schema.Schema{Type: schema.TypeString}, + }, + }, + }, + }, }, }, }, diff --git a/builtin/providers/test/resource_list_test.go b/builtin/providers/test/resource_list_test.go index 87d1dde123..aeb56e989f 100644 --- a/builtin/providers/test/resource_list_test.go +++ b/builtin/providers/test/resource_list_test.go @@ -318,3 +318,132 @@ resource "test_resource_list" "foo" { }, }) } + +func TestResourceList_planUnknownInterpolation(t *testing.T) { + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_list" "foo" { + list_block { + string = "x" + } +} +resource "test_resource_list" "bar" { + list_block { + sublist = [ + test_resource_list.foo.list_block[0].string, + ] + } +} + `), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr( + "test_resource_list.bar", "list_block.0.sublist.0", "x", + ), + ), + }, + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_list" "foo" { + list_block { + string = "x" + } + dependent_list { + val = "y" + } +} +resource "test_resource_list" "bar" { + list_block { + sublist = [ + test_resource_list.foo.computed_list[0], + ] + } +} + `), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr( + "test_resource_list.bar", "list_block.0.sublist.0", "y", + ), + ), + }, + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_list" "foo" { + list_block { + string = "x" + } + dependent_list { + val = "z" + } +} +resource "test_resource_list" "bar" { + list_block { + sublist = [ + test_resource_list.foo.computed_list[0], + ] + } +} + `), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr( + "test_resource_list.bar", "list_block.0.sublist.0", "z", + ), + ), + }, + }, + }) +} + +func TestResourceList_planUnknownInterpolationList(t *testing.T) { + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_list" "foo" { + dependent_list { + val = "y" + } +} +resource "test_resource_list" "bar" { + list_block { + sublist_block_optional { + list = test_resource_list.foo.computed_list + } + } +} + `), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr( + "test_resource_list.bar", "list_block.0.sublist_block_optional.0.list.0", "y", + ), + ), + }, + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_list" "foo" { + dependent_list { + val = "z" + } +} +resource "test_resource_list" "bar" { + list_block { + sublist_block_optional { + list = test_resource_list.foo.computed_list + } + } +} + `), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr( + "test_resource_list.bar", "list_block.0.sublist_block_optional.0.list.0", "z", + ), + ), + }, + }, + }) +} diff --git a/builtin/providers/test/resource_test.go b/builtin/providers/test/resource_test.go index 350a76841a..3c64dc5945 100644 --- a/builtin/providers/test/resource_test.go +++ b/builtin/providers/test/resource_test.go @@ -790,3 +790,129 @@ resource "test_resource" "foo" { }, }) } + +func TestResource_optionalComputedMap(t *testing.T) { + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource" "foo" { + required = "yep" + required_map = { + key = "value" + } + optional_computed_map = { + foo = "bar" + baz = "" + } +} + `), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr( + "test_resource.foo", "optional_computed_map.foo", "bar", + ), + resource.TestCheckResourceAttr( + "test_resource.foo", "optional_computed_map.baz", "", + ), + ), + }, + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource" "foo" { + required = "yep" + required_map = { + key = "value" + } + optional_computed_map = {} +} + `), + // removing the map from the config should still leave an empty computed map + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr( + "test_resource.foo", "optional_computed_map.%", "0", + ), + ), + }, + }, + }) +} + +func TestResource_plannedComputed(t *testing.T) { + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource" "foo" { + required = "ok" + required_map = { + key = "value" + } +} + `), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr( + "test_resource.foo", "planned_computed", "ok", + ), + ), + }, + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource" "foo" { + required = "changed" + required_map = { + key = "value" + } +} + `), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr( + "test_resource.foo", "planned_computed", "changed", + ), + ), + }, + }, + }) +} + +func TestDiffApply_map(t *testing.T) { + resSchema := map[string]*schema.Schema{ + "map": { + Type: schema.TypeMap, + Optional: true, + Computed: true, + Elem: &schema.Schema{Type: schema.TypeString}, + }, + } + + priorAttrs := map[string]string{ + "id": "ok", + "map.%": "2", + "map.foo": "bar", + "map.bar": "", + } + + diff := &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "map.foo": &terraform.ResourceAttrDiff{Old: "bar", New: "", NewRemoved: true}, + "map.bar": &terraform.ResourceAttrDiff{Old: "", New: "", NewRemoved: true}, + }, + } + + newAttrs, err := diff.Apply(priorAttrs, (&schema.Resource{Schema: resSchema}).CoreConfigSchema()) + if err != nil { + t.Fatal(err) + } + + expect := map[string]string{ + "id": "ok", + "map.%": "0", + } + + if !reflect.DeepEqual(newAttrs, expect) { + t.Fatalf("expected:%#v got:%#v", expect, newAttrs) + } +} From 009df443f7088f418fc13a5624200976043a9a5b Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 29 Mar 2019 09:36:27 -0400 Subject: [PATCH 2/5] restore lost unknowns during a planned update. Because schema.ResourceDiff can't differentiate between unknown values and new computed values, unknowns can be lost during an update. If a planned value converted an unknown to a null, restore the unknown so that it can be correctly replaced in the final plan. --- helper/plugin/grpc_provider.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/helper/plugin/grpc_provider.go b/helper/plugin/grpc_provider.go index c2299b24f2..b268795d8f 100644 --- a/helper/plugin/grpc_provider.go +++ b/helper/plugin/grpc_provider.go @@ -1144,6 +1144,14 @@ func normalizeNullValues(dst, src cty.Value, preferDst bool) cty.Value { ty := dst.Type() if !src.IsNull() && !src.IsKnown() { + // While this seems backwards to return src when preferDst is set, it + // means this might be a plan scenario, and it must retain unknown + // interpolated placeholders, which could be lost if we're only updating + // a resource. If this is a read scenario, then there shouldn't be any + // unknowns all. + if dst.IsNull() && preferDst { + return src + } return dst } From 86e30add9837c3b4fb7682233496d7a4c2d1a923 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 29 Mar 2019 11:24:14 -0400 Subject: [PATCH 3/5] fix unknowns added to maps by schemaMap The legacy diff process inserts unknown values into an optional+computed map. Fix these up in post-plan normalization process, by looking for known strings that were changed to unknown. --- helper/plugin/grpc_provider.go | 25 ++++++++++++++++++++----- helper/plugin/grpc_provider_test.go | 24 +++++++++++++++++++++++- 2 files changed, 43 insertions(+), 6 deletions(-) diff --git a/helper/plugin/grpc_provider.go b/helper/plugin/grpc_provider.go index b268795d8f..9705e269c8 100644 --- a/helper/plugin/grpc_provider.go +++ b/helper/plugin/grpc_provider.go @@ -1184,11 +1184,8 @@ func normalizeNullValues(dst, src cty.Value, preferDst bool) cty.Value { dstMap = map[string]cty.Value{} } - ei := src.ElementIterator() - for ei.Next() { - k, v := ei.Element() - key := k.AsString() - + srcMap := src.AsValueMap() + for key, v := range srcMap { dstVal := dstMap[key] if dstVal == cty.NilVal { if preferDst && ty.IsMapType() { @@ -1211,6 +1208,24 @@ func normalizeNullValues(dst, src cty.Value, preferDst bool) cty.Value { } if ty.IsMapType() { + // helper/schema will populate an optional+computed map with + // unknowns which we have to fixup here. + // It would be preferable to simply prevent any known value from + // becoming unknown, but concessions have to be made to retain the + // broken legacy behavior when possible. + for k, srcVal := range srcMap { + if !srcVal.IsNull() && srcVal.IsKnown() { + dstVal, ok := dstMap[k] + if !ok { + continue + } + + if !dstVal.IsNull() && !dstVal.IsKnown() { + dstMap[k] = srcVal + } + } + } + return cty.MapVal(dstMap) } diff --git a/helper/plugin/grpc_provider_test.go b/helper/plugin/grpc_provider_test.go index a31fe57c50..fb33e4b26b 100644 --- a/helper/plugin/grpc_provider_test.go +++ b/helper/plugin/grpc_provider_test.go @@ -909,7 +909,7 @@ func TestNormalizeNullValues(t *testing.T) { }), }), }, - // the empty list should be transferred, but the new unknown show not be overridden + // the empty list should be transferred, but the new unknown should not be overridden { Src: cty.ObjectVal(map[string]cty.Value{ "network_interface": cty.ListVal([]cty.Value{ @@ -942,6 +942,28 @@ func TestNormalizeNullValues(t *testing.T) { }), Plan: true, }, + { + // fix unknowns added to a map + Src: cty.ObjectVal(map[string]cty.Value{ + "map": cty.MapVal(map[string]cty.Value{ + "a": cty.StringVal("a"), + "b": cty.StringVal(""), + }), + }), + Dst: cty.ObjectVal(map[string]cty.Value{ + "map": cty.MapVal(map[string]cty.Value{ + "a": cty.StringVal("a"), + "b": cty.UnknownVal(cty.String), + }), + }), + Expect: cty.ObjectVal(map[string]cty.Value{ + "map": cty.MapVal(map[string]cty.Value{ + "a": cty.StringVal("a"), + "b": cty.StringVal(""), + }), + }), + Plan: true, + }, } { t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { got := normalizeNullValues(tc.Dst, tc.Src, tc.Plan) From 6868ddd085023b4d96a27dab92326f793d9f21cf Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 29 Mar 2019 13:27:44 -0400 Subject: [PATCH 4/5] don't set NewRemoved values as empty strings NewRemoved diff values were somtimes left in maps and lists. --- terraform/diff.go | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/terraform/diff.go b/terraform/diff.go index a5d70e2642..00b0052e9b 100644 --- a/terraform/diff.go +++ b/terraform/diff.go @@ -699,14 +699,6 @@ func (d *InstanceDiff) applySingleAttrDiff(path []string, attrs map[string]strin return result, nil } - if diff.Old == diff.New && diff.New == "" { - // this can only be a valid empty string - if attrSchema.Type == cty.String { - result[attr] = "" - } - return result, nil - } - // check for missmatched diff values if exists && old != diff.Old && @@ -715,13 +707,21 @@ func (d *InstanceDiff) applySingleAttrDiff(path []string, attrs map[string]strin return result, fmt.Errorf("diff apply conflict for %s: diff expects %q, but prior value has %q", attr, diff.Old, old) } - if attrSchema.Computed && diff.NewComputed { - result[attr] = config.UnknownVariableValue + if diff.NewRemoved { + // don't set anything in the new value + return map[string]string{}, nil + } + + if diff.Old == diff.New && diff.New == "" { + // this can only be a valid empty string + if attrSchema.Type == cty.String { + result[attr] = "" + } return result, nil } - if diff.NewRemoved { - // don't set anything in the new value + if attrSchema.Computed && diff.NewComputed { + result[attr] = config.UnknownVariableValue return result, nil } @@ -863,8 +863,10 @@ func (d *InstanceDiff) applyCollectionDiff(path []string, attrs map[string]strin } } - // Fill in the count value if it was missing for some reason: - if result[countKey] == "" { + // Fill in the count value if it wasn't present in the diff for some reason, + // or if there is no count at all. + _, countDiff := d.Attributes[countKey] + if result[countKey] == "" || (!countDiff && len(keys) != len(result)) { result[countKey] = countFlatmapContainerValues(countKey, result) } From 64c76be8042324a0723ab7d0c6dbc23b22d4f968 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 29 Mar 2019 14:54:54 -0400 Subject: [PATCH 5/5] fixup lost collections containing unknowns It turns out that collections containing only unknowns could be lost, meaning there wasn't a direct correlation between the unknown and null value which would have otherwise been restored. --- helper/plugin/grpc_provider.go | 19 ++++++++++++++- helper/plugin/grpc_provider_test.go | 37 +++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/helper/plugin/grpc_provider.go b/helper/plugin/grpc_provider.go index 9705e269c8..ca122af588 100644 --- a/helper/plugin/grpc_provider.go +++ b/helper/plugin/grpc_provider.go @@ -1240,12 +1240,29 @@ func normalizeNullValues(dst, src cty.Value, preferDst bool) cty.Value { } case ty.IsListType(), ty.IsTupleType(): - // If the dst is nil, and the src is known, then we lost an empty value + // If the dst is null, and the src is known, then we lost an empty value // so take the original. if dst.IsNull() { if src.IsWhollyKnown() && src.LengthInt() == 0 && !preferDst { return src } + + // if dst is null and src only contains unknown values, then we lost + // those during a plan (which is when preferDst is set, there would + // be no unknowns during read). + if preferDst && !src.IsNull() { + allUnknown := true + for _, v := range src.AsValueSlice() { + if v.IsKnown() { + allUnknown = false + break + } + } + if allUnknown { + return src + } + } + return dst } diff --git a/helper/plugin/grpc_provider_test.go b/helper/plugin/grpc_provider_test.go index fb33e4b26b..ce31cb51c8 100644 --- a/helper/plugin/grpc_provider_test.go +++ b/helper/plugin/grpc_provider_test.go @@ -964,6 +964,43 @@ func TestNormalizeNullValues(t *testing.T) { }), Plan: true, }, + { + // fix unknowns lost from a list + Src: cty.ObjectVal(map[string]cty.Value{ + "top": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "list": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "values": cty.ListVal([]cty.Value{cty.UnknownVal(cty.String)}), + }), + }), + }), + }), + }), + Dst: cty.ObjectVal(map[string]cty.Value{ + "top": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "list": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "values": cty.NullVal(cty.List(cty.String)), + }), + }), + }), + }), + }), + Expect: cty.ObjectVal(map[string]cty.Value{ + "top": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "list": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "values": cty.ListVal([]cty.Value{cty.UnknownVal(cty.String)}), + }), + }), + }), + }), + }), + Plan: true, + }, } { t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { got := normalizeNullValues(tc.Dst, tc.Src, tc.Plan)