From c794bf5bcc1abeeadb4689f6d37316ef73e4a19a Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 7 Feb 2019 14:01:39 -0800 Subject: [PATCH 01/12] plans/objchange: Don't presume unknown for values unset in config Previously we would construct a proposed new state with unknown values in place of any not-set-in-config computed attributes, trying to save the provider a little work in specifying that itself. Unfortunately that turns out to be problematic because it conflates two concerns: attributes can be explicitly set in configuration to an unknown value, in which case the final result of that unknown overrides any default value the provider might normally populate. In other words, this allows the provider to recognize in the proposed new state the difference between an Optional+Computed attribute being set to unknown in the config vs not being set in the config at all. The provider now has the responsibility to replace these proposed null values with unknown values during PlanResourceChange if it expects to select a value during the apply step. It may also populate a known value if the final result can be predicted at plan time, as is the case for constant defaults specified in the provider code. This change comes from a realization that from core's perspective the helper/schema ideas of zero values, explicit default values, and customizediff tweaks are all just examples of "defaults", and by allowing the provider to see during plan whether these attributes are being explicitly set in configuration and thus decide whether the default will be provided immediately during plan or deferred until apply. --- plans/objchange/all_null.go | 68 +++++++++++++++++++++++++++++++ plans/objchange/objchange.go | 9 ++-- plans/objchange/objchange_test.go | 64 ++++++++++++++++++++++++++++- 3 files changed, 135 insertions(+), 6 deletions(-) create mode 100644 plans/objchange/all_null.go diff --git a/plans/objchange/all_null.go b/plans/objchange/all_null.go new file mode 100644 index 000000000000..b1acb253afe4 --- /dev/null +++ b/plans/objchange/all_null.go @@ -0,0 +1,68 @@ +package objchange + +import ( + "fmt" + + "github.com/hashicorp/terraform/configs/configschema" + "github.com/zclconf/go-cty/cty" +) + +// AllAttributesNull constructs a non-null cty.Value of the object type implied +// by the given schema that has all of its leaf attributes set to null and all +// of its nested block collections set to zero-length. +// +// This simulates what would result from decoding an empty configuration block +// with the given schema, except that it does not produce errors +func AllAttributesNull(schema *configschema.Block) cty.Value { + vals := make(map[string]cty.Value) + ty := schema.ImpliedType() + + for name := range schema.Attributes { + aty := ty.AttributeType(name) + vals[name] = cty.NullVal(aty) + } + + for name, blockS := range schema.BlockTypes { + aty := ty.AttributeType(name) + + switch blockS.Nesting { + case configschema.NestingSingle: + // NestingSingle behaves like an object attribute, which decodes + // as null when it's not present in configuration. + vals[name] = cty.NullVal(aty) + default: + // All other nesting types decode as "empty" when not present, but + // empty values take different forms depending on the type. + switch { + case aty.IsListType(): + vals[name] = cty.ListValEmpty(aty.ElementType()) + case aty.IsSetType(): + vals[name] = cty.SetValEmpty(aty.ElementType()) + case aty.IsMapType(): + vals[name] = cty.MapValEmpty(aty.ElementType()) + case aty.Equals(cty.DynamicPseudoType): + // We use DynamicPseudoType in situations where there's a + // nested attribute of DynamicPseudoType, since the schema + // system cannot predict the final type until it knows exactly + // how many elements there will be. However, since we're + // trying to behave as if there are _no_ elements, we know + // we're producing either an empty tuple or empty object + // and just need to distinguish these two cases. + switch blockS.Nesting { + case configschema.NestingList: + vals[name] = cty.EmptyTupleVal + case configschema.NestingMap: + vals[name] = cty.EmptyObjectVal + } + } + } + + // By the time we get down here we should always have set a value. + // If not, that suggests a missing case in the above switches. + if _, ok := vals[name]; !ok { + panic(fmt.Sprintf("failed to create empty value for nested block %q", name)) + } + } + + return cty.ObjectVal(vals) +} diff --git a/plans/objchange/objchange.go b/plans/objchange/objchange.go index 33e82dd52ff5..1f59e6f416cd 100644 --- a/plans/objchange/objchange.go +++ b/plans/objchange/objchange.go @@ -26,10 +26,11 @@ import ( // block where _all_ attributes are computed. func ProposedNewObject(schema *configschema.Block, prior, config cty.Value) cty.Value { if prior.IsNull() { - // In this case, we will treat the prior value as unknown so that - // any computed attributes not overridden in config will show as - // unknown values, rather than null values. - prior = cty.UnknownVal(schema.ImpliedType()) + // In this case, we will construct a synthetic prior value that is + // similar to the result of decoding an empty configuration block, + // which simplifies our handling of the top-level attributes/blocks + // below by giving us one non-null level of object to pull values from. + prior = AllAttributesNull(schema) } if config.IsNull() || !config.IsKnown() { // This is a weird situation, but we'll allow it anyway to free diff --git a/plans/objchange/objchange_test.go b/plans/objchange/objchange_test.go index b2f6402544b9..91f9df1f21c7 100644 --- a/plans/objchange/objchange_test.go +++ b/plans/objchange/objchange_test.go @@ -44,6 +44,11 @@ func TestProposedNewObject(t *testing.T) { Optional: true, Computed: true, }, + "biz": { + Type: cty.String, + Optional: true, + Computed: true, + }, }, }, }, @@ -55,13 +60,26 @@ func TestProposedNewObject(t *testing.T) { "bar": cty.NullVal(cty.String), "baz": cty.ObjectVal(map[string]cty.Value{ "boz": cty.StringVal("world"), + + // An unknown in the config represents a situation where + // an argument is explicitly set to an expression result + // that is derived from an unknown value. This is distinct + // from leaving it null, which allows the provider itself + // to decide the value during PlanResourceChange. + "biz": cty.UnknownVal(cty.String), }), }), cty.ObjectVal(map[string]cty.Value{ "foo": cty.StringVal("hello"), - "bar": cty.UnknownVal(cty.String), + + // unset computed attributes are null in the proposal; provider + // usually changes them to "unknown" during PlanResourceChange, + // to indicate that the value will be decided during apply. + "bar": cty.NullVal(cty.String), + "baz": cty.ObjectVal(map[string]cty.Value{ "boz": cty.StringVal("world"), + "biz": cty.UnknownVal(cty.String), // explicit unknown preserved from config }), }), }, @@ -468,7 +486,49 @@ func TestProposedNewObject(t *testing.T) { }), cty.ObjectVal(map[string]cty.Value{ "bar": cty.StringVal("bosh"), - "baz": cty.UnknownVal(cty.String), + "baz": cty.NullVal(cty.String), + }), + }), + }), + }, + "sets differing only by unknown": { + &configschema.Block{ + BlockTypes: map[string]*configschema.NestedBlock{ + "multi": { + Nesting: configschema.NestingSet, + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "optional": { + Type: cty.String, + Optional: true, + Computed: true, + }, + }, + }, + }, + }, + }, + cty.NullVal(cty.DynamicPseudoType), + cty.ObjectVal(map[string]cty.Value{ + "multi": cty.SetVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "optional": cty.UnknownVal(cty.String), + }), + cty.ObjectVal(map[string]cty.Value{ + "optional": cty.UnknownVal(cty.String), + }), + }), + }), + cty.ObjectVal(map[string]cty.Value{ + "multi": cty.SetVal([]cty.Value{ + // These remain distinct because unknown values never + // compare equal. They may be consolidated together once + // the values become known, though. + cty.ObjectVal(map[string]cty.Value{ + "optional": cty.UnknownVal(cty.String), + }), + cty.ObjectVal(map[string]cty.Value{ + "optional": cty.UnknownVal(cty.String), }), }), }), From f8a6f66be436b454179fa2d679b5a5d0c927a2e8 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 7 Feb 2019 14:35:13 -0800 Subject: [PATCH 02/12] lang/funcs: Fix panic in "join" when an element is null It is now a proper error message. --- lang/funcs/string.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/lang/funcs/string.go b/lang/funcs/string.go index cb3d03cbd477..c9ddf19e368d 100644 --- a/lang/funcs/string.go +++ b/lang/funcs/string.go @@ -39,10 +39,18 @@ var JoinFunc = function.New(&function.Spec{ } items := make([]string, 0, l) - for _, list := range listVals { + for ai, list := range listVals { + ei := 0 for it := list.ElementIterator(); it.Next(); { _, val := it.Element() + if val.IsNull() { + if len(listVals) > 1 { + return cty.UnknownVal(cty.String), function.NewArgErrorf(ai+1, "element %d of list %d is null; cannot concatenate null values", ei, ai+1) + } + return cty.UnknownVal(cty.String), function.NewArgErrorf(ai+1, "element %d is null; cannot concatenate null values", ei) + } items = append(items, val.AsString()) + ei++ } } From be127725cc46c48173090407fe548c30f3068bff Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 7 Feb 2019 20:23:39 -0500 Subject: [PATCH 03/12] Additional tests with interpolated values --- builtin/providers/test/resource_list_test.go | 55 +++++++++++++++++++ .../test/resource_nested_set_test.go | 53 ++++++++++++++++++ 2 files changed, 108 insertions(+) diff --git a/builtin/providers/test/resource_list_test.go b/builtin/providers/test/resource_list_test.go index 76c82f15a80d..e39b575219cc 100644 --- a/builtin/providers/test/resource_list_test.go +++ b/builtin/providers/test/resource_list_test.go @@ -133,3 +133,58 @@ resource "test_resource_list" "foo" { }, }) } + +func TestResourceList_interpolationChanges(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 { + string = test_resource_list.foo.id + } +} + `), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr( + "test_resource_list.foo", "list_block.0.string", "x", + ), + resource.TestCheckResourceAttr( + "test_resource_list.bar", "list_block.0.string", "testId", + ), + ), + }, + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_list" "baz" { + list_block { + string = "x" + int = 1 + } +} +resource "test_resource_list" "bar" { + list_block { + string = test_resource_list.baz.id + int = 3 + } +} + `), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr( + "test_resource_list.baz", "list_block.0.string", "x", + ), + resource.TestCheckResourceAttr( + "test_resource_list.bar", "list_block.0.string", "testId", + ), + ), + }, + }, + }) +} diff --git a/builtin/providers/test/resource_nested_set_test.go b/builtin/providers/test/resource_nested_set_test.go index e47f1ac5883e..e782bb7fb5e1 100644 --- a/builtin/providers/test/resource_nested_set_test.go +++ b/builtin/providers/test/resource_nested_set_test.go @@ -527,3 +527,56 @@ resource "test_resource_nested_set" "c" { }, }) } + +func TestResourceNestedSet_interpolationChanges(t *testing.T) { + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_nested_set" "foo" { + single { + value = "x" + } +} +resource "test_resource_nested_set" "bar" { + single { + value = test_resource_nested_set.foo.id + } +} + `), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr( + "test_resource_nested_set.foo", "single.#", "1", + ), + resource.TestCheckResourceAttr( + "test_resource_nested_set.bar", "single.#", "1", + ), + ), + }, + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_nested_set" "baz" { + single { + value = "x" + } +} +resource "test_resource_nested_set" "bar" { + single { + value = test_resource_nested_set.baz.id + } +} + `), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr( + "test_resource_nested_set.baz", "single.#", "1", + ), + resource.TestCheckResourceAttr( + "test_resource_nested_set.bar", "single.#", "1", + ), + ), + }, + }, + }) +} From d17ba647a8945c534fda7d3239edb464cee132bb Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 7 Feb 2019 20:24:36 -0500 Subject: [PATCH 04/12] add SetUnknowns SetUnknown walks through a resource and changes any unset (null) values that are going computed in the schema to Unknown. --- helper/plugin/unknown.go | 89 +++++++++ helper/plugin/unknown_test.go | 353 ++++++++++++++++++++++++++++++++++ 2 files changed, 442 insertions(+) create mode 100644 helper/plugin/unknown.go create mode 100644 helper/plugin/unknown_test.go diff --git a/helper/plugin/unknown.go b/helper/plugin/unknown.go new file mode 100644 index 000000000000..cd4c2a6fe411 --- /dev/null +++ b/helper/plugin/unknown.go @@ -0,0 +1,89 @@ +package plugin + +import ( + "fmt" + + "github.com/hashicorp/terraform/configs/configschema" + "github.com/zclconf/go-cty/cty" +) + +// SetUnknowns takes a cty.Value, and compares it to the schema setting any null +// leaf values which are computed as unknown. +func SetUnknowns(val cty.Value, schema *configschema.Block) cty.Value { + if val.IsNull() || !val.IsKnown() { + return val + } + + valMap := val.AsValueMap() + newVals := make(map[string]cty.Value) + + for name, attr := range schema.Attributes { + v := valMap[name] + + if attr.Computed && v.IsNull() { + newVals[name] = cty.UnknownVal(attr.Type) + continue + } + + newVals[name] = v + } + + for name, blockS := range schema.BlockTypes { + blockVal := valMap[name] + if blockVal.IsNull() || !blockVal.IsKnown() { + newVals[name] = blockVal + continue + } + + blockType := blockS.Block.ImpliedType() + + switch blockS.Nesting { + case configschema.NestingSingle: + newVals[name] = SetUnknowns(blockVal, &blockS.Block) + case configschema.NestingSet, configschema.NestingList: + listVals := blockVal.AsValueSlice() + newListVals := make([]cty.Value, 0, len(listVals)) + + for _, v := range listVals { + newListVals = append(newListVals, SetUnknowns(v, &blockS.Block)) + } + + switch blockS.Nesting { + case configschema.NestingSet: + switch len(newListVals) { + case 0: + newVals[name] = cty.SetValEmpty(blockType) + default: + newVals[name] = cty.SetVal(newListVals) + } + case configschema.NestingList: + switch len(newListVals) { + case 0: + newVals[name] = cty.ListValEmpty(blockType) + default: + newVals[name] = cty.ListVal(newListVals) + } + } + + case configschema.NestingMap: + mapVals := blockVal.AsValueMap() + newMapVals := make(map[string]cty.Value) + + for k, v := range mapVals { + newMapVals[k] = SetUnknowns(v, &blockS.Block) + } + + switch len(newMapVals) { + case 0: + newVals[name] = cty.MapValEmpty(blockType) + default: + newVals[name] = cty.MapVal(newMapVals) + } + + default: + panic(fmt.Sprintf("failed to set unknown values for nested block %q", name)) + } + } + + return cty.ObjectVal(newVals) +} diff --git a/helper/plugin/unknown_test.go b/helper/plugin/unknown_test.go new file mode 100644 index 000000000000..df4ba42c4d50 --- /dev/null +++ b/helper/plugin/unknown_test.go @@ -0,0 +1,353 @@ +package plugin + +import ( + "testing" + + "github.com/hashicorp/terraform/configs/configschema" + "github.com/zclconf/go-cty/cty" +) + +func TestSetUnknowns(t *testing.T) { + for n, tc := range map[string]struct { + Schema *configschema.Block + Val cty.Value + Expected cty.Value + }{ + "empty": { + &configschema.Block{}, + cty.EmptyObjectVal, + cty.EmptyObjectVal, + }, + "no prior": { + &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "foo": { + Type: cty.String, + Optional: true, + }, + "bar": { + Type: cty.String, + Computed: true, + }, + }, + BlockTypes: map[string]*configschema.NestedBlock{ + "baz": { + Nesting: configschema.NestingSingle, + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "boz": { + Type: cty.String, + Optional: true, + Computed: true, + }, + "biz": { + Type: cty.String, + Optional: true, + Computed: true, + }, + }, + }, + }, + }, + }, + cty.ObjectVal(map[string]cty.Value{}), + cty.ObjectVal(map[string]cty.Value{ + "bar": cty.UnknownVal(cty.String), + }), + }, + "no prior with set": { + // the set value should remain null + &configschema.Block{ + BlockTypes: map[string]*configschema.NestedBlock{ + "baz": { + Nesting: configschema.NestingSet, + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "boz": { + Type: cty.String, + Optional: true, + Computed: true, + }, + }, + }, + }, + }, + }, + cty.ObjectVal(map[string]cty.Value{}), + cty.ObjectVal(map[string]cty.Value{}), + }, + "prior attributes": { + &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "foo": { + Type: cty.String, + Optional: true, + }, + "bar": { + Type: cty.String, + Computed: true, + }, + "baz": { + Type: cty.String, + Optional: true, + Computed: true, + }, + "boz": { + Type: cty.String, + Optional: true, + Computed: true, + }, + }, + }, + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.StringVal("bonjour"), + "bar": cty.StringVal("petit dejeuner"), + "baz": cty.StringVal("grande dejeuner"), + }), + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.StringVal("bonjour"), + "bar": cty.StringVal("petit dejeuner"), + "baz": cty.StringVal("grande dejeuner"), + "boz": cty.UnknownVal(cty.String), + }), + }, + "prior nested single": { + &configschema.Block{ + BlockTypes: map[string]*configschema.NestedBlock{ + "foo": { + Nesting: configschema.NestingSingle, + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "bar": { + Type: cty.String, + Optional: true, + Computed: true, + }, + "baz": { + Type: cty.String, + Optional: true, + Computed: true, + }, + }, + }, + }, + }, + }, + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("beep"), + }), + }), + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("beep"), + "baz": cty.UnknownVal(cty.String), + }), + }), + }, + "prior nested list": { + &configschema.Block{ + BlockTypes: map[string]*configschema.NestedBlock{ + "foo": { + Nesting: configschema.NestingList, + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "bar": { + Type: cty.String, + Optional: true, + Computed: true, + }, + "baz": { + Type: cty.String, + Optional: true, + Computed: true, + }, + }, + }, + }, + }, + }, + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("bap"), + }), + cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("blep"), + }), + }), + }), + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("bap"), + "baz": cty.UnknownVal(cty.String), + }), + cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("blep"), + "baz": cty.UnknownVal(cty.String), + }), + }), + }), + }, + "prior nested map": { + &configschema.Block{ + BlockTypes: map[string]*configschema.NestedBlock{ + "foo": { + Nesting: configschema.NestingMap, + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "bar": { + Type: cty.String, + Optional: true, + Computed: true, + }, + "baz": { + Type: cty.String, + Optional: true, + Computed: true, + }, + }, + }, + }, + }, + }, + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.MapVal(map[string]cty.Value{ + "a": cty.ObjectVal(map[string]cty.Value{ + "bar": cty.NullVal(cty.String), + "baz": cty.StringVal("boop"), + }), + "b": cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("blep"), + "baz": cty.NullVal(cty.String), + }), + }), + }), + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.MapVal(map[string]cty.Value{ + "a": cty.ObjectVal(map[string]cty.Value{ + "bar": cty.UnknownVal(cty.String), + "baz": cty.StringVal("boop"), + }), + "b": cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("blep"), + "baz": cty.UnknownVal(cty.String), + }), + }), + }), + }, + "prior nested set": { + &configschema.Block{ + BlockTypes: map[string]*configschema.NestedBlock{ + "foo": { + Nesting: configschema.NestingSet, + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "bar": { + Type: cty.String, + Optional: true, + }, + "baz": { + Type: cty.String, + Optional: true, + Computed: true, + }, + }, + }, + }, + }, + }, + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.SetVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("blep"), + "baz": cty.NullVal(cty.String), + }), + cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("boop"), + "baz": cty.NullVal(cty.String), + }), + }), + }), + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.SetVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("blep"), + "baz": cty.UnknownVal(cty.String), + }), + cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("boop"), + "baz": cty.UnknownVal(cty.String), + }), + }), + }), + }, + "sets differing only by unknown": { + &configschema.Block{ + BlockTypes: map[string]*configschema.NestedBlock{ + "foo": { + Nesting: configschema.NestingSet, + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "bar": { + Type: cty.String, + Optional: true, + }, + "baz": { + Type: cty.String, + Optional: true, + Computed: true, + }, + }, + }, + }, + }, + }, + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.SetVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("boop"), + "baz": cty.NullVal(cty.String), + }), + cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("boop"), + "baz": cty.UnknownVal(cty.String), + }), + }), + }), + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.SetVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("boop"), + "baz": cty.UnknownVal(cty.String), + }), + cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("boop"), + "baz": cty.UnknownVal(cty.String), + }), + }), + }), + }, + } { + t.Run(n, func(t *testing.T) { + // coerce the values because SetUnknowns expects the values to be + // complete, and so we can take shortcuts writing them in the + // test. + v, err := tc.Schema.CoerceValue(tc.Val) + if err != nil { + t.Fatal(err) + } + + expected, err := tc.Schema.CoerceValue(tc.Expected) + if err != nil { + t.Fatal(err) + } + + got := SetUnknowns(v, tc.Schema) + if !got.RawEquals(expected) { + t.Fatalf("\nexpected: %#v\ngot: %#v\n", expected, got) + } + }) + } +} From 32671241e04a30542557d6f1be9cab84dc93e2f3 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 7 Feb 2019 20:29:24 -0500 Subject: [PATCH 05/12] set unknowns during initial PlanResourceChange If ID is not set, make sure it's unknown. Use SetUnknowns to set the rest of the computed values to Unknown. --- helper/plugin/grpc_provider.go | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/helper/plugin/grpc_provider.go b/helper/plugin/grpc_provider.go index 8291445c83b8..709441b475f4 100644 --- a/helper/plugin/grpc_provider.go +++ b/helper/plugin/grpc_provider.go @@ -511,23 +511,30 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl // turn the proposed state into a legacy configuration cfg := terraform.NewResourceConfigShimmed(proposedNewStateVal, block) + diff, err := s.provider.SimpleDiff(info, priorState, cfg) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil } + // if this is a new instance, we need to make sure ID is going to be computed + if priorStateVal.IsNull() { + if diff == nil { + diff = terraform.NewInstanceDiff() + } + + diff.Attributes["id"] = &terraform.ResourceAttrDiff{ + NewComputed: true, + } + } + if diff == nil || len(diff.Attributes) == 0 { // schema.Provider.Diff returns nil if it ends up making a diff with no // changes, but our new interface wants us to return an actual change - // description that _shows_ there are no changes. This is usually the - // PriorSate, however if there was no prior state and no diff, then we - // use the ProposedNewState. - if !priorStateVal.IsNull() { - resp.PlannedState = req.PriorState - } else { - resp.PlannedState = req.ProposedNewState - } + // description that _shows_ there are no changes. This is always the + // prior state, because we force a diff above if this is a new instance. + resp.PlannedState = req.PriorState return resp, nil } @@ -561,7 +568,7 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl plannedStateVal = copyTimeoutValues(plannedStateVal, proposedNewStateVal) - // The old SDK code has some inprecisions that cause it to sometimes + // The old SDK code has some imprecisions that cause it to sometimes // generate differences that the SDK itself does not consider significant // but Terraform Core would. To avoid producing weird do-nothing diffs // in that case, we'll check if the provider as produced something we @@ -575,6 +582,12 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl forceNoChanges = true } + // if this was creating the resource, we need to set any remaining computed + // fields + if priorStateVal.IsNull() { + plannedStateVal = SetUnknowns(plannedStateVal, block) + } + plannedMP, err := msgpack.Marshal(plannedStateVal, block.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) From c3e7efec3519eff900a333e077786e81c132592b Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 7 Feb 2019 16:57:58 -0800 Subject: [PATCH 06/12] core: Reject unknown values after reading a data resource Data resources do not have a plan/apply distinction, so it is never valid for a data resource to produce unknown values in its result object. Unknown values in the data resource _config_ cause us to postpone the read altogether, so a data source never receives unknown values as input and therefore may never produce unknown values as output. --- terraform/eval_read_data.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/terraform/eval_read_data.go b/terraform/eval_read_data.go index e9096909eb0f..54d59eea4e84 100644 --- a/terraform/eval_read_data.go +++ b/terraform/eval_read_data.go @@ -227,6 +227,24 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { ), )) } + if !newVal.IsWhollyKnown() { + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Provider produced invalid object", + fmt.Sprintf( + "Provider %q produced a value for %s that is not wholly known.\n\nThis is a bug in the provider, which should be reported in the provider's own issue tracker.", + n.ProviderAddr.ProviderConfig.Type, absAddr, + ), + )) + + // We'll still save the object, but we need to eliminate any unknown + // values first because we can't serialize them in the state file. + // Note that this may cause set elements to be coalesced if they + // differed only by having unknown values, but we don't worry about + // that here because we're saving the value only for inspection + // purposes; the error we added above will halt the graph walk. + newVal = cty.UnknownAsNull(newVal) + } // Since we've completed the read, we actually have no change to make, but // we'll produce a NoOp one anyway to preserve the usual flow of the From 8882dcaf86fc8f24dbf362283b5991261eb834a5 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 7 Feb 2019 17:33:17 -0800 Subject: [PATCH 07/12] core: Fix TestContext2Plan_dataResourceBecomesComputed Now that ProposedNewState uses null to represent Computed attributes not set in the configuration, the provider must fill in the unknown value for "computed" in its plan result. It seems that this test was incorrectly updated during our bulk-fix after integrating the HCL2 work, but it didn't really matter because the ReadDataSource function isn't called in the happy path anyway. But to make the intent clearer here, we also now make ReadDataSource return an error if it is called, making it explicit that no call is expected. --- terraform/context_plan_test.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index f5eadd40b909..f95f59068cc7 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -1985,8 +1985,12 @@ func TestContext2Plan_dataResourceBecomesComputed(t *testing.T) { } p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse { + fooVal := req.ProposedNewState.GetAttr("foo") return providers.PlanResourceChangeResponse{ - PlannedState: req.ProposedNewState, + PlannedState: cty.ObjectVal(map[string]cty.Value{ + "foo": fooVal, + "computed": cty.UnknownVal(cty.String), + }), PlannedPrivate: req.PriorPrivate, } } @@ -1995,9 +1999,9 @@ func TestContext2Plan_dataResourceBecomesComputed(t *testing.T) { ty := schema.ImpliedType() p.ReadDataSourceResponse = providers.ReadDataSourceResponse{ - State: cty.ObjectVal(map[string]cty.Value{ - "foo": cty.UnknownVal(cty.String), - }), + // This should not be called, because the configuration for the + // data resource contains an unknown value for "foo". + Diagnostics: tfdiags.Diagnostics(nil).Append(fmt.Errorf("ReadDataSource called, but should not have been")), } ctx := testContext2(t, &ContextOpts{ From 312d798a89daa929a2c10a9ee84bb98dd4fe8e7f Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 7 Feb 2019 18:33:05 -0800 Subject: [PATCH 08/12] core: Restore our EvalReadData behavior In an earlier commit we changed objchange.ProposedNewObject so that the task of populating unknown values for attributes not known during apply is the responsibility of the provider's PlanResourceChange method, rather than being handled automatically. However, we were also using objchange.ProposedNewObject to construct the placeholder new object for a deferred data resource read, and so we inadvertently broke that deferral behavior. Here we restore the old behavior by introducing a new function objchange.PlannedDataResourceObject which is a specialized version of objchange.ProposedNewObject that includes the forced behavior of populating unknown values, because the provider gets no opportunity to customize a deferred read. TestContext2Plan_createBeforeDestroy_depends_datasource required some updates here because its implementation of PlanResourceChange was not handling the insertion of the unknown value for attribute "computed". The other changes here are just in an attempt to make the flow of this test more obvious, by clarifying that it is simulating a -refresh=false run, which effectively forces a deferred read since we skip the eager read that would normally happen in the refresh step. --- plans/objchange/objchange.go | 26 ++++++++++++++++++++ terraform/context_plan_test.go | 43 +++++++++++++++++++++++++++++++++- terraform/eval_read_data.go | 8 +++++-- 3 files changed, 74 insertions(+), 3 deletions(-) diff --git a/plans/objchange/objchange.go b/plans/objchange/objchange.go index 1f59e6f416cd..15565a400694 100644 --- a/plans/objchange/objchange.go +++ b/plans/objchange/objchange.go @@ -32,6 +32,32 @@ func ProposedNewObject(schema *configschema.Block, prior, config cty.Value) cty. // below by giving us one non-null level of object to pull values from. prior = AllAttributesNull(schema) } + return proposedNewObject(schema, prior, config) +} + +// PlannedDataResourceObject is similar to ProposedNewObject but tailored for +// planning data resources in particular. Specifically, it replaces the values +// of any Computed attributes not set in the configuration with an unknown +// value, which serves as a placeholder for a value to be filled in by the +// provider when the data resource is finally read. +// +// Data resources are different because the planning of them is handled +// entirely within Terraform Core and not subject to customization by the +// provider. This function is, in effect, producing an equivalent result to +// passing the ProposedNewObject result into a provider's PlanResourceChange +// function, assuming a fixed implementation of PlanResourceChange that just +// fills in unknown values as needed. +func PlannedDataResourceObject(schema *configschema.Block, config cty.Value) cty.Value { + // Our trick here is to run the ProposedNewObject logic with an + // entirely-unknown prior value. Because of cty's unknown short-circuit + // behavior, any operation on prior returns another unknown, and so + // unknown values propagate into all of the parts of the resulting value + // that would normally be filled in by preserving the prior state. + prior := cty.UnknownVal(schema.ImpliedType()) + return proposedNewObject(schema, prior, config) +} + +func proposedNewObject(schema *configschema.Block, prior, config cty.Value) cty.Value { if config.IsNull() || !config.IsKnown() { // This is a weird situation, but we'll allow it anyway to free // callers from needing to specifically check for these cases. diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index f95f59068cc7..a58035233227 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -4990,8 +4990,20 @@ func TestContext2Plan_createBeforeDestroy_depends_datasource(t *testing.T) { }, } p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse { + computedVal := req.ProposedNewState.GetAttr("computed") + if computedVal.IsNull() { + computedVal = cty.UnknownVal(cty.String) + } return providers.PlanResourceChangeResponse{ - PlannedState: req.ProposedNewState, + PlannedState: cty.ObjectVal(map[string]cty.Value{ + "num": req.ProposedNewState.GetAttr("num"), + "computed": computedVal, + }), + } + } + p.ReadDataSourceFn = func(req providers.ReadDataSourceRequest) providers.ReadDataSourceResponse { + return providers.ReadDataSourceResponse{ + Diagnostics: tfdiags.Diagnostics(nil).Append(fmt.Errorf("ReadDataSource called, but should not have been")), } } @@ -5004,11 +5016,20 @@ func TestContext2Plan_createBeforeDestroy_depends_datasource(t *testing.T) { ), }) + // We're skipping ctx.Refresh here, which simulates what happens when + // running "terraform plan -refresh=false". As a result, we don't get our + // usual opportunity to read the data source during the refresh step and + // thus the plan call below is forced to produce a deferred read action. + plan, diags := ctx.Plan() + if p.ReadDataSourceCalled { + t.Errorf("ReadDataSource was called on the provider, but should not have been because we didn't refresh") + } if diags.HasErrors() { t.Fatalf("unexpected errors: %s", diags.Err()) } + seenAddrs := make(map[string]struct{}) for _, res := range plan.Changes.Resources { var schema *configschema.Block switch res.Addr.Resource.Resource.Mode { @@ -5023,6 +5044,8 @@ func TestContext2Plan_createBeforeDestroy_depends_datasource(t *testing.T) { t.Fatal(err) } + seenAddrs[ric.Addr.String()] = struct{}{} + t.Run(ric.Addr.String(), func(t *testing.T) { switch i := ric.Addr.String(); i { case "aws_instance.foo[0]": @@ -5046,6 +5069,10 @@ func TestContext2Plan_createBeforeDestroy_depends_datasource(t *testing.T) { t.Fatalf("resource %s should be read, got %s", ric.Addr, ric.Action) } checkVals(t, objectVal(t, schema, map[string]cty.Value{ + // In a normal flow we would've read an exact value in + // ReadDataSource, but because this test doesn't run + // cty.Refresh we have no opportunity to do that lookup + // and a deferred read is forced. "id": cty.UnknownVal(cty.String), "foo": cty.StringVal("0"), }), ric.After) @@ -5054,6 +5081,10 @@ func TestContext2Plan_createBeforeDestroy_depends_datasource(t *testing.T) { t.Fatalf("resource %s should be read, got %s", ric.Addr, ric.Action) } checkVals(t, objectVal(t, schema, map[string]cty.Value{ + // In a normal flow we would've read an exact value in + // ReadDataSource, but because this test doesn't run + // cty.Refresh we have no opportunity to do that lookup + // and a deferred read is forced. "id": cty.UnknownVal(cty.String), "foo": cty.StringVal("1"), }), ric.After) @@ -5062,6 +5093,16 @@ func TestContext2Plan_createBeforeDestroy_depends_datasource(t *testing.T) { } }) } + + wantAddrs := map[string]struct{}{ + "aws_instance.foo[0]": struct{}{}, + "aws_instance.foo[1]": struct{}{}, + "data.aws_vpc.bar[0]": struct{}{}, + "data.aws_vpc.bar[1]": struct{}{}, + } + if !cmp.Equal(seenAddrs, wantAddrs) { + t.Errorf("incorrect addresses in changeset:\n%s", cmp.Diff(wantAddrs, seenAddrs)) + } } // interpolated lists need to be stored in the original order. diff --git a/terraform/eval_read_data.go b/terraform/eval_read_data.go index 54d59eea4e84..d89d02ba9113 100644 --- a/terraform/eval_read_data.go +++ b/terraform/eval_read_data.go @@ -104,7 +104,7 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { return nil, diags.Err() } - proposedNewVal := objchange.ProposedNewObject(schema, priorVal, configVal) + proposedNewVal := objchange.PlannedDataResourceObject(schema, configVal) // If our configuration contains any unknown values then we must defer the // read to the apply phase by producing a "Read" change for this resource, @@ -119,7 +119,11 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { absAddr, ) } - log.Printf("[TRACE] EvalReadData: %s configuration not fully known yet, so deferring to apply phase", absAddr) + if n.ForcePlanRead { + log.Printf("[TRACE] EvalReadData: %s configuration is fully known, but we're forcing a read plan to be created", absAddr) + } else { + log.Printf("[TRACE] EvalReadData: %s configuration not fully known yet, so deferring to apply phase", absAddr) + } err := ctx.Hook(func(h Hook) (HookAction, error) { return h.PreDiff(absAddr, states.CurrentGen, priorVal, proposedNewVal) From 82588af89249982102c77512aa01fdb6c9e5b62f Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 8 Feb 2019 14:46:29 -0500 Subject: [PATCH 09/12] switch blocks based on value type, and check attrs Check attributes on null objects, and fill in unknowns. If we're evaluating the object, it either means we are at the top level, or a NestingSingle block was present, and in either case we need to treat the attributes as null rather than the entire object. Switch on the block types rather than Nesting, so we don't need add any logic to change between List/Tuple or Map/Object when DynamicPseudoType is involved. --- helper/plugin/unknown.go | 78 ++++++++++++---- helper/plugin/unknown_test.go | 168 ++++++++++++++++++++++++++++++---- 2 files changed, 209 insertions(+), 37 deletions(-) diff --git a/helper/plugin/unknown.go b/helper/plugin/unknown.go index cd4c2a6fe411..48e24e5d621c 100644 --- a/helper/plugin/unknown.go +++ b/helper/plugin/unknown.go @@ -8,12 +8,35 @@ import ( ) // SetUnknowns takes a cty.Value, and compares it to the schema setting any null -// leaf values which are computed as unknown. +// values which are computed to unknown. func SetUnknowns(val cty.Value, schema *configschema.Block) cty.Value { - if val.IsNull() || !val.IsKnown() { + if !val.IsKnown() { return val } + // If the object was null, we still need to handle the top level attributes + // which might be computed, but we don't need to expand the blocks. + if val.IsNull() { + objMap := map[string]cty.Value{} + allNull := true + for name, attr := range schema.Attributes { + switch { + case attr.Computed: + objMap[name] = cty.UnknownVal(attr.Type) + allNull = false + default: + objMap[name] = cty.NullVal(attr.Type) + } + } + + // If this object has no unknown attributes, then we can leave it null. + if allNull { + return val + } + + return cty.ObjectVal(objMap) + } + valMap := val.AsValueMap() newVals := make(map[string]cty.Value) @@ -35,12 +58,18 @@ func SetUnknowns(val cty.Value, schema *configschema.Block) cty.Value { continue } - blockType := blockS.Block.ImpliedType() + blockValType := blockVal.Type() + blockElementType := blockS.Block.ImpliedType() - switch blockS.Nesting { - case configschema.NestingSingle: + // This switches on the value type here, so we can correctly switch + // between Tuples/Lists and Maps/Objects. + switch { + case blockS.Nesting == configschema.NestingSingle: + // NestingSingle is the only exception here, where we treat the + // block directly as an object newVals[name] = SetUnknowns(blockVal, &blockS.Block) - case configschema.NestingSet, configschema.NestingList: + + case blockValType.IsSetType(), blockValType.IsListType(), blockValType.IsTupleType(): listVals := blockVal.AsValueSlice() newListVals := make([]cty.Value, 0, len(listVals)) @@ -48,24 +77,26 @@ func SetUnknowns(val cty.Value, schema *configschema.Block) cty.Value { newListVals = append(newListVals, SetUnknowns(v, &blockS.Block)) } - switch blockS.Nesting { - case configschema.NestingSet: + switch { + case blockValType.IsSetType(): switch len(newListVals) { case 0: - newVals[name] = cty.SetValEmpty(blockType) + newVals[name] = cty.SetValEmpty(blockElementType) default: newVals[name] = cty.SetVal(newListVals) } - case configschema.NestingList: + case blockValType.IsListType(): switch len(newListVals) { case 0: - newVals[name] = cty.ListValEmpty(blockType) + newVals[name] = cty.ListValEmpty(blockElementType) default: newVals[name] = cty.ListVal(newListVals) } + case blockValType.IsTupleType(): + newVals[name] = cty.TupleVal(newListVals) } - case configschema.NestingMap: + case blockValType.IsMapType(), blockValType.IsObjectType(): mapVals := blockVal.AsValueMap() newMapVals := make(map[string]cty.Value) @@ -73,15 +104,26 @@ func SetUnknowns(val cty.Value, schema *configschema.Block) cty.Value { newMapVals[k] = SetUnknowns(v, &blockS.Block) } - switch len(newMapVals) { - case 0: - newVals[name] = cty.MapValEmpty(blockType) - default: - newVals[name] = cty.MapVal(newMapVals) + switch { + case blockValType.IsMapType(): + switch len(newMapVals) { + case 0: + newVals[name] = cty.MapValEmpty(blockElementType) + default: + newVals[name] = cty.MapVal(newMapVals) + } + case blockValType.IsObjectType(): + if len(newMapVals) == 0 { + // We need to populate empty values to make a valid object. + for attr, ty := range blockElementType.AttributeTypes() { + newMapVals[attr] = cty.NullVal(ty) + } + } + newVals[name] = cty.ObjectVal(newMapVals) } default: - panic(fmt.Sprintf("failed to set unknown values for nested block %q", name)) + panic(fmt.Sprintf("failed to set unknown values for nested block %q:%#v", name, blockValType)) } } diff --git a/helper/plugin/unknown_test.go b/helper/plugin/unknown_test.go index df4ba42c4d50..4214b18497f7 100644 --- a/helper/plugin/unknown_test.go +++ b/helper/plugin/unknown_test.go @@ -50,14 +50,64 @@ func TestSetUnknowns(t *testing.T) { }, }, }, - cty.ObjectVal(map[string]cty.Value{}), + cty.NullVal(cty.Object(map[string]cty.Type{ + "foo": cty.String, + "bar": cty.String, + "baz": cty.Object(map[string]cty.Type{ + "boz": cty.String, + "biz": cty.String, + }), + })), cty.ObjectVal(map[string]cty.Value{ + "foo": cty.NullVal(cty.String), "bar": cty.UnknownVal(cty.String), }), }, + "null stays null": { + // if the object has no computed attributes, it should stay null + &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "foo": &configschema.Attribute{ + Type: cty.String, + }, + }, + BlockTypes: map[string]*configschema.NestedBlock{ + "baz": { + Nesting: configschema.NestingSet, + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "boz": { + Type: cty.String, + Optional: true, + Computed: true, + }, + }, + }, + }, + }, + }, + cty.NullVal(cty.Object(map[string]cty.Type{ + "foo": cty.String, + "baz": cty.Set(cty.Object(map[string]cty.Type{ + "boz": cty.String, + })), + })), + cty.NullVal(cty.Object(map[string]cty.Type{ + "foo": cty.String, + "baz": cty.Set(cty.Object(map[string]cty.Type{ + "boz": cty.String, + })), + })), + }, "no prior with set": { // the set value should remain null &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "foo": &configschema.Attribute{ + Type: cty.String, + Computed: true, + }, + }, BlockTypes: map[string]*configschema.NestedBlock{ "baz": { Nesting: configschema.NestingSet, @@ -73,8 +123,15 @@ func TestSetUnknowns(t *testing.T) { }, }, }, - cty.ObjectVal(map[string]cty.Value{}), - cty.ObjectVal(map[string]cty.Value{}), + cty.NullVal(cty.Object(map[string]cty.Type{ + "foo": cty.String, + "baz": cty.Set(cty.Object(map[string]cty.Type{ + "boz": cty.String, + })), + })), + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.UnknownVal(cty.String), + }), }, "prior attributes": { &configschema.Block{ @@ -329,24 +386,97 @@ func TestSetUnknowns(t *testing.T) { }), }), }, + "prior nested list with dynamic": { + &configschema.Block{ + BlockTypes: map[string]*configschema.NestedBlock{ + "foo": { + Nesting: configschema.NestingList, + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "bar": { + Type: cty.String, + Optional: true, + Computed: true, + }, + "baz": { + Type: cty.DynamicPseudoType, + Optional: true, + Computed: true, + }, + }, + }, + }, + }, + }, + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.TupleVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "bar": cty.NullVal(cty.String), + "baz": cty.NumberIntVal(8), + }), + }), + }), + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.TupleVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "bar": cty.UnknownVal(cty.String), + "baz": cty.NumberIntVal(8), + }), + }), + }), + }, + "prior nested map with dynamic": { + &configschema.Block{ + BlockTypes: map[string]*configschema.NestedBlock{ + "foo": { + Nesting: configschema.NestingMap, + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "bar": { + Type: cty.String, + Optional: true, + Computed: true, + }, + "baz": { + Type: cty.DynamicPseudoType, + Optional: true, + Computed: true, + }, + }, + }, + }, + }, + }, + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.ObjectVal(map[string]cty.Value{ + "a": cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("beep"), + "baz": cty.NullVal(cty.DynamicPseudoType), + }), + "b": cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("boop"), + "baz": cty.NumberIntVal(8), + }), + }), + }), + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.ObjectVal(map[string]cty.Value{ + "a": cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("beep"), + "baz": cty.UnknownVal(cty.DynamicPseudoType), + }), + "b": cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("boop"), + "baz": cty.NumberIntVal(8), + }), + }), + }), + }, } { t.Run(n, func(t *testing.T) { - // coerce the values because SetUnknowns expects the values to be - // complete, and so we can take shortcuts writing them in the - // test. - v, err := tc.Schema.CoerceValue(tc.Val) - if err != nil { - t.Fatal(err) - } - - expected, err := tc.Schema.CoerceValue(tc.Expected) - if err != nil { - t.Fatal(err) - } - - got := SetUnknowns(v, tc.Schema) - if !got.RawEquals(expected) { - t.Fatalf("\nexpected: %#v\ngot: %#v\n", expected, got) + got := SetUnknowns(tc.Val, tc.Schema) + if !got.RawEquals(tc.Expected) { + t.Fatalf("\nexpected: %#v\ngot: %#v\n", tc.Expected, got) } }) } From 7e186f72d964bb64b310f2a18071abd45f4a1500 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 8 Feb 2019 11:58:08 -0800 Subject: [PATCH 10/12] command: Update "terraform show -json" tests for changed provider contract We now require a provider to populate all of its defaults -- including unknown value placeholders -- during PlanResourceChange. That means the mock provider for testing "terraform show -json" must now manage the population of the computed "id" attribute during plan. To make this logic a little easier, we also change the ApplyResourceChange implementation to fill in a non-null id, since that makes it easier for the mock PlanResourceChange to recognize when it needs to populate that default value during an update. --- command/show_test.go | 20 +++++++++++++++++-- .../show-json/basic-delete/output.json | 8 ++++---- .../show-json/basic-delete/terraform.tfstate | 6 ++++-- .../show-json/basic-update/output.json | 6 +++--- .../show-json/basic-update/terraform.tfstate | 3 ++- 5 files changed, 31 insertions(+), 12 deletions(-) diff --git a/command/show_test.go b/command/show_test.go index 746ea1706151..5febe857818b 100644 --- a/command/show_test.go +++ b/command/show_test.go @@ -244,13 +244,29 @@ func showFixtureProvider() *terraform.MockProvider { p := testProvider() p.GetSchemaReturn = showFixtureSchema() p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse { + idVal := req.ProposedNewState.GetAttr("id") + amiVal := req.ProposedNewState.GetAttr("ami") + if idVal.IsNull() { + idVal = cty.UnknownVal(cty.String) + } return providers.PlanResourceChangeResponse{ - PlannedState: req.ProposedNewState, + PlannedState: cty.ObjectVal(map[string]cty.Value{ + "id": idVal, + "ami": amiVal, + }), } } p.ApplyResourceChangeFn = func(req providers.ApplyResourceChangeRequest) providers.ApplyResourceChangeResponse { + idVal := req.PlannedState.GetAttr("id") + amiVal := req.PlannedState.GetAttr("ami") + if !idVal.IsKnown() { + idVal = cty.StringVal("placeholder") + } return providers.ApplyResourceChangeResponse{ - NewState: cty.UnknownAsNull(req.PlannedState), + NewState: cty.ObjectVal(map[string]cty.Value{ + "id": idVal, + "ami": amiVal, + }), } } return p diff --git a/command/test-fixtures/show-json/basic-delete/output.json b/command/test-fixtures/show-json/basic-delete/output.json index 297ded0e7702..837ed5d010e3 100644 --- a/command/test-fixtures/show-json/basic-delete/output.json +++ b/command/test-fixtures/show-json/basic-delete/output.json @@ -18,7 +18,7 @@ "schema_version": 0, "values": { "ami": "bar", - "id": null + "id": "placeholder" } } ] @@ -37,11 +37,11 @@ ], "before": { "ami": "foo", - "id": null + "id": "placeholder" }, "after": { "ami": "bar", - "id": null + "id": "placeholder" }, "after_unknown": { "ami": false, @@ -61,7 +61,7 @@ ], "before": { "ami": "foo", - "id": null + "id": "placeholder" }, "after": null, "after_unknown": false diff --git a/command/test-fixtures/show-json/basic-delete/terraform.tfstate b/command/test-fixtures/show-json/basic-delete/terraform.tfstate index 4cf6ed7fb932..db49d3e6846f 100644 --- a/command/test-fixtures/show-json/basic-delete/terraform.tfstate +++ b/command/test-fixtures/show-json/basic-delete/terraform.tfstate @@ -14,7 +14,8 @@ { "schema_version": 0, "attributes": { - "ami": "foo" + "ami": "foo", + "id": "placeholder" } } ] @@ -28,7 +29,8 @@ { "schema_version": 0, "attributes": { - "ami": "foo" + "ami": "foo", + "id": "placeholder" } } ] diff --git a/command/test-fixtures/show-json/basic-update/output.json b/command/test-fixtures/show-json/basic-update/output.json index d065edaf0168..4f5f115fb9df 100644 --- a/command/test-fixtures/show-json/basic-update/output.json +++ b/command/test-fixtures/show-json/basic-update/output.json @@ -18,7 +18,7 @@ "schema_version": 0, "values": { "ami": "bar", - "id": null + "id": "placeholder" } } ] @@ -37,11 +37,11 @@ ], "before": { "ami": "bar", - "id": null + "id": "placeholder" }, "after": { "ami": "bar", - "id": null + "id": "placeholder" }, "after_unknown": { "ami": false, diff --git a/command/test-fixtures/show-json/basic-update/terraform.tfstate b/command/test-fixtures/show-json/basic-update/terraform.tfstate index f980cdfd5909..dfc796a88408 100644 --- a/command/test-fixtures/show-json/basic-update/terraform.tfstate +++ b/command/test-fixtures/show-json/basic-update/terraform.tfstate @@ -14,7 +14,8 @@ { "schema_version": 0, "attributes": { - "ami": "bar" + "ami": "bar", + "id": "placeholder" } } ] From e3618f915b82f1717456b3733c3435cf3927ea75 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 8 Feb 2019 12:48:32 -0800 Subject: [PATCH 11/12] backend/local: Fix mock provider in tests We've changed the contract for PlanResourceChange to now require the provider to populate any default values (including unknowns) it wants to set for computed arguments, so our mock provider here now needs to be a little more complex to deal with that. This fixes several of the tests in this package. A minor change to TestLocal_applyEmptyDirDestroy was required to make it properly configure the mock provider so PlanResourceChange can access the schema. --- backend/local/backend_apply_test.go | 4 +-- backend/local/testing.go | 39 ++++++++++++++++++++++++++--- 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/backend/local/backend_apply_test.go b/backend/local/backend_apply_test.go index e7e7a53a9503..f8de90479c3c 100644 --- a/backend/local/backend_apply_test.go +++ b/backend/local/backend_apply_test.go @@ -123,8 +123,7 @@ func TestLocal_applyError(t *testing.T) { b, cleanup := TestLocal(t) defer cleanup() - p := TestLocalProvider(t, b, "test", nil) - p.GetSchemaReturn = &terraform.ProviderSchema{ + schema := &terraform.ProviderSchema{ ResourceTypes: map[string]*configschema.Block{ "test_instance": { Attributes: map[string]*configschema.Attribute{ @@ -134,6 +133,7 @@ func TestLocal_applyError(t *testing.T) { }, }, } + p := TestLocalProvider(t, b, "test", schema) var lock sync.Mutex errored := false diff --git a/backend/local/testing.go b/backend/local/testing.go index 05f7600f9ae7..dccddb5daeed 100644 --- a/backend/local/testing.go +++ b/backend/local/testing.go @@ -6,7 +6,11 @@ import ( "path/filepath" "testing" + "github.com/zclconf/go-cty/cty" + + "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/backend" + "github.com/hashicorp/terraform/configs/configschema" "github.com/hashicorp/terraform/providers" "github.com/hashicorp/terraform/states" "github.com/hashicorp/terraform/states/statemgr" @@ -38,9 +42,14 @@ func TestLocal(t *testing.T) (*Local, func()) { // function, t.Helper doesn't apply and so the log source // isn't correctly shown in the test log output. This seems // unavoidable as long as this is happening so indirectly. - t.Log(diag.Description().Summary) + desc := diag.Description() + if desc.Detail != "" { + t.Logf("%s: %s", desc.Summary, desc.Detail) + } else { + t.Log(desc.Summary) + } if local.CLI != nil { - local.CLI.Error(diag.Description().Summary) + local.CLI.Error(desc.Summary) } } } @@ -59,10 +68,34 @@ func TestLocal(t *testing.T) (*Local, func()) { func TestLocalProvider(t *testing.T, b *Local, name string, schema *terraform.ProviderSchema) *terraform.MockProvider { // Build a mock resource provider for in-memory operations p := new(terraform.MockProvider) + + if schema == nil { + schema = &terraform.ProviderSchema{} // default schema is empty + } p.GetSchemaReturn = schema + p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse { + rSchema, _ := schema.SchemaForResourceType(addrs.ManagedResourceMode, req.TypeName) + if rSchema == nil { + rSchema = &configschema.Block{} // default schema is empty + } + plannedVals := map[string]cty.Value{} + for name, attrS := range rSchema.Attributes { + val := req.ProposedNewState.GetAttr(name) + if attrS.Computed && val.IsNull() { + val = cty.UnknownVal(attrS.Type) + } + plannedVals[name] = val + } + for name := range rSchema.BlockTypes { + // For simplicity's sake we just copy the block attributes over + // verbatim, since this package's mock providers are all relatively + // simple -- we're testing the backend, not esoteric provider features. + plannedVals[name] = req.ProposedNewState.GetAttr(name) + } + return providers.PlanResourceChangeResponse{ - PlannedState: req.ProposedNewState, + PlannedState: cty.ObjectVal(plannedVals), PlannedPrivate: req.PriorPrivate, } } From c20164ab311f6b4e0f700dbb70b1f867d5c9b70b Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 8 Feb 2019 16:33:05 -0500 Subject: [PATCH 12/12] fix CoerceValue to handle changing dynamic types Objects with DynamicPseudoType attributes can't be coerced within a map if a concrete type is set. Change the Value type used to an Object when there is a type mismatch. --- configs/configschema/coerce_value.go | 24 +++++- configs/configschema/coerce_value_test.go | 95 +++++++++++++++++++++++ 2 files changed, 118 insertions(+), 1 deletion(-) diff --git a/configs/configschema/coerce_value.go b/configs/configschema/coerce_value.go index bae5733df711..877e860f3f44 100644 --- a/configs/configschema/coerce_value.go +++ b/configs/configschema/coerce_value.go @@ -225,7 +225,29 @@ func (b *Block) coerceValue(in cty.Value, path cty.Path) (cty.Value, error) { elems[key.AsString()] = val } } - attrs[typeName] = cty.MapVal(elems) + + // If the attribute values here contain any DynamicPseudoTypes, + // the concrete type must be an object. + useObject := false + switch { + case coll.Type().IsObjectType(): + useObject = true + default: + // It's possible that we were given a map, and need to coerce it to an object + ety := coll.Type().ElementType() + for _, v := range elems { + if !v.Type().Equals(ety) { + useObject = true + break + } + } + } + + if useObject { + attrs[typeName] = cty.ObjectVal(elems) + } else { + attrs[typeName] = cty.MapVal(elems) + } default: attrs[typeName] = cty.MapValEmpty(blockS.ImpliedType()) } diff --git a/configs/configschema/coerce_value_test.go b/configs/configschema/coerce_value_test.go index b7cc5f632df5..3286751a3527 100644 --- a/configs/configschema/coerce_value_test.go +++ b/configs/configschema/coerce_value_test.go @@ -436,6 +436,101 @@ func TestCoerceValue(t *testing.T) { }), ``, }, + "dynamic value attributes": { + &Block{ + BlockTypes: map[string]*NestedBlock{ + "foo": { + Nesting: NestingMap, + Block: Block{ + Attributes: map[string]*Attribute{ + "bar": { + Type: cty.String, + Optional: true, + Computed: true, + }, + "baz": { + Type: cty.DynamicPseudoType, + Optional: true, + Computed: true, + }, + }, + }, + }, + }, + }, + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.ObjectVal(map[string]cty.Value{ + "a": cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("beep"), + }), + "b": cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("boop"), + "baz": cty.NumberIntVal(8), + }), + }), + }), + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.ObjectVal(map[string]cty.Value{ + "a": cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("beep"), + "baz": cty.NullVal(cty.DynamicPseudoType), + }), + "b": cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("boop"), + "baz": cty.NumberIntVal(8), + }), + }), + }), + ``, + }, + "dynamic attributes in map": { + // Convert a block represented as a map to an object if a + // DynamicPseudoType causes the element types to mismatch. + &Block{ + BlockTypes: map[string]*NestedBlock{ + "foo": { + Nesting: NestingMap, + Block: Block{ + Attributes: map[string]*Attribute{ + "bar": { + Type: cty.String, + Optional: true, + Computed: true, + }, + "baz": { + Type: cty.DynamicPseudoType, + Optional: true, + Computed: true, + }, + }, + }, + }, + }, + }, + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.MapVal(map[string]cty.Value{ + "a": cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("beep"), + }), + "b": cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("boop"), + }), + }), + }), + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.ObjectVal(map[string]cty.Value{ + "a": cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("beep"), + "baz": cty.NullVal(cty.DynamicPseudoType), + }), + "b": cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("boop"), + "baz": cty.NullVal(cty.DynamicPseudoType), + }), + }), + }), + ``, + }, } for name, test := range tests {