From c794bf5bcc1abeeadb4689f6d37316ef73e4a19a Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 7 Feb 2019 14:01:39 -0800 Subject: [PATCH] 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 000000000..b1acb253a --- /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 33e82dd52..1f59e6f41 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 b2f640254..91f9df1f2 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), }), }), }),