diff --git a/configs/configschema/decoder_spec.go b/configs/configschema/decoder_spec.go index 41a3fc4971..333274503c 100644 --- a/configs/configschema/decoder_spec.go +++ b/configs/configschema/decoder_spec.go @@ -101,15 +101,6 @@ func (b *Block) DecoderSpec() hcldec.Spec { childSpec := blockS.Block.DecoderSpec() - // We can only validate 0 or 1 for MinItems, because a dynamic block - // may satisfy any number of min items while only having a single - // block in the config. We cannot validate MaxItems because a - // configuration may have any number of dynamic blocks. - minItems := 0 - if blockS.MinItems > 1 { - minItems = 1 - } - switch blockS.Nesting { case NestingSingle, NestingGroup: ret[name] = &hcldec.BlockSpec{ @@ -134,13 +125,15 @@ func (b *Block) DecoderSpec() hcldec.Spec { ret[name] = &hcldec.BlockTupleSpec{ TypeName: name, Nested: childSpec, - MinItems: minItems, + MinItems: blockS.MinItems, + MaxItems: blockS.MaxItems, } } else { ret[name] = &hcldec.BlockListSpec{ TypeName: name, Nested: childSpec, - MinItems: minItems, + MinItems: blockS.MinItems, + MaxItems: blockS.MaxItems, } } case NestingSet: @@ -154,7 +147,8 @@ func (b *Block) DecoderSpec() hcldec.Spec { ret[name] = &hcldec.BlockSetSpec{ TypeName: name, Nested: childSpec, - MinItems: minItems, + MinItems: blockS.MinItems, + MaxItems: blockS.MaxItems, } case NestingMap: // We prefer to use a list where possible, since it makes our diff --git a/configs/configschema/decoder_spec_test.go b/configs/configschema/decoder_spec_test.go index 1eefc4d8b6..a6571eaa64 100644 --- a/configs/configschema/decoder_spec_test.go +++ b/configs/configschema/decoder_spec_test.go @@ -344,15 +344,12 @@ func TestBlockDecoderSpec(t *testing.T) { }, &hcl.Block{ Type: "foo", - Body: hcl.EmptyBody(), + Body: unknownBody{hcl.EmptyBody()}, }, }, }), cty.ObjectVal(map[string]cty.Value{ - "foo": cty.ListVal([]cty.Value{ - cty.EmptyObjectVal, - cty.EmptyObjectVal, - }), + "foo": cty.UnknownVal(cty.List(cty.EmptyObject)), }), 0, // max items cannot be validated during decode }, @@ -372,14 +369,12 @@ func TestBlockDecoderSpec(t *testing.T) { Blocks: hcl.Blocks{ &hcl.Block{ Type: "foo", - Body: hcl.EmptyBody(), + Body: unknownBody{hcl.EmptyBody()}, }, }, }), cty.ObjectVal(map[string]cty.Value{ - "foo": cty.ListVal([]cty.Value{ - cty.EmptyObjectVal, - }), + "foo": cty.UnknownVal(cty.List(cty.EmptyObject)), }), 0, }, @@ -401,6 +396,7 @@ func TestBlockDecoderSpec(t *testing.T) { for name, test := range tests { t.Run(name, func(t *testing.T) { spec := test.Schema.DecoderSpec() + got, diags := hcldec.Decode(test.TestBody, spec, nil) if len(diags) != test.DiagCount { t.Errorf("wrong number of diagnostics %d; want %d", len(diags), test.DiagCount) @@ -427,6 +423,16 @@ func TestBlockDecoderSpec(t *testing.T) { } } +// this satisfies hcldec.UnknownBody to simulate a dynamic block with an +// unknown number of values. +type unknownBody struct { + hcl.Body +} + +func (b unknownBody) Unknown() bool { + return true +} + func TestAttributeDecoderSpec(t *testing.T) { tests := map[string]struct { Schema *Attribute diff --git a/go.mod b/go.mod index 2673b62b38..1fa462e567 100644 --- a/go.mod +++ b/go.mod @@ -65,7 +65,7 @@ require ( github.com/hashicorp/go-uuid v1.0.1 github.com/hashicorp/go-version v1.2.1 github.com/hashicorp/hcl v0.0.0-20170504190234-a4b07c25de5f - github.com/hashicorp/hcl/v2 v2.9.1 + github.com/hashicorp/hcl/v2 v2.10.0 github.com/hashicorp/memberlist v0.1.0 // indirect github.com/hashicorp/serf v0.0.0-20160124182025-e4ec8cc423bb // indirect github.com/hashicorp/terraform-config-inspect v0.0.0-20210209133302-4fd17a0faac2 diff --git a/go.sum b/go.sum index 84b6ca85f4..2d7b56f240 100644 --- a/go.sum +++ b/go.sum @@ -358,8 +358,8 @@ github.com/hashicorp/golang-lru v0.5.1/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ github.com/hashicorp/hcl v0.0.0-20170504190234-a4b07c25de5f h1:UdxlrJz4JOnY8W+DbLISwf2B8WXEolNRA8BGCwI9jws= github.com/hashicorp/hcl v0.0.0-20170504190234-a4b07c25de5f/go.mod h1:oZtUIOe8dh44I2q6ScRibXws4Ajl+d+nod3AaR9vL5w= github.com/hashicorp/hcl/v2 v2.0.0/go.mod h1:oVVDG71tEinNGYCxinCYadcmKU9bglqW9pV3txagJ90= -github.com/hashicorp/hcl/v2 v2.9.1 h1:eOy4gREY0/ZQHNItlfuEZqtcQbXIxzojlP301hDpnac= -github.com/hashicorp/hcl/v2 v2.9.1/go.mod h1:FwWsfWEjyV/CMj8s/gqAuiviY72rJ1/oayI9WftqcKg= +github.com/hashicorp/hcl/v2 v2.10.0 h1:1S1UnuhDGlv3gRFV4+0EdwB+znNP5HmcGbIqwnSCByg= +github.com/hashicorp/hcl/v2 v2.10.0/go.mod h1:FwWsfWEjyV/CMj8s/gqAuiviY72rJ1/oayI9WftqcKg= github.com/hashicorp/memberlist v0.1.0 h1:qSsCiC0WYD39lbSitKNt40e30uorm2Ss/d4JGU1hzH8= github.com/hashicorp/memberlist v0.1.0/go.mod h1:ncdBp14cuox2iFOq3kDiquKU6fqsTBc3W6JvZwjxxsE= github.com/hashicorp/serf v0.0.0-20160124182025-e4ec8cc423bb h1:ZbgmOQt8DOg796figP87/EFCVx2v2h9yRvwHF/zceX4= diff --git a/lang/blocktoattr/fixup.go b/lang/blocktoattr/fixup.go index 0af708ec40..056946ab99 100644 --- a/lang/blocktoattr/fixup.go +++ b/lang/blocktoattr/fixup.go @@ -41,6 +41,17 @@ type fixupBody struct { names map[string]struct{} } +type unknownBlock interface { + Unknown() bool +} + +func (b *fixupBody) Unknown() bool { + if u, ok := b.original.(unknownBlock); ok { + return u.Unknown() + } + return false +} + // Content decodes content from the body. The given schema must be the lower-level // representation of the same schema that was previously passed to FixUpBlockAttrs, // or else the result is undefined. diff --git a/plans/objchange/compatible.go b/plans/objchange/compatible.go index 0ef4b01e76..e4ece3f976 100644 --- a/plans/objchange/compatible.go +++ b/plans/objchange/compatible.go @@ -75,20 +75,12 @@ func assertObjectCompatible(schema *configschema.Block, planned, actual cty.Valu plannedV, _ := planned.GetAttr(name).Unmark() actualV, _ := actual.GetAttr(name).Unmark() - // As a special case, if there were any blocks whose leaf attributes - // are all unknown then we assume (possibly incorrectly) that the - // HCL dynamic block extension is in use with an unknown for_each - // argument, and so we will do looser validation here that allows - // for those blocks to have expanded into a different number of blocks - // if the for_each value is now known. - maybeUnknownBlocks := couldHaveUnknownBlockPlaceholder(plannedV, blockS, false) - path := append(path, cty.GetAttrStep{Name: name}) switch blockS.Nesting { case configschema.NestingSingle, configschema.NestingGroup: // If an unknown block placeholder was present then the placeholder // may have expanded out into zero blocks, which is okay. - if maybeUnknownBlocks && actualV.IsNull() { + if !plannedV.IsKnown() && actualV.IsNull() { continue } moreErrs := assertObjectCompatible(&blockS.Block, plannedV, actualV, path) @@ -102,14 +94,6 @@ func assertObjectCompatible(schema *configschema.Block, planned, actual cty.Valu continue } - if maybeUnknownBlocks { - // When unknown blocks are present the final blocks may be - // at different indices than the planned blocks, so unfortunately - // we can't do our usual checks in this case without generating - // false negatives. - continue - } - plannedL := plannedV.LengthInt() actualL := actualV.LengthInt() if plannedL != actualL { @@ -144,7 +128,7 @@ func assertObjectCompatible(schema *configschema.Block, planned, actual cty.Valu moreErrs := assertObjectCompatible(&blockS.Block, plannedEV, actualEV, append(path, cty.GetAttrStep{Name: k})) errs = append(errs, moreErrs...) } - if !maybeUnknownBlocks { // new blocks may appear if unknown blocks were present in the plan + if plannedV.IsKnown() { // new blocks may appear if unknown blocks were present in the plan for k := range actualAtys { if _, ok := plannedAtys[k]; !ok { errs = append(errs, path.NewErrorf("new block key %q has appeared", k)) @@ -158,7 +142,7 @@ func assertObjectCompatible(schema *configschema.Block, planned, actual cty.Valu } plannedL := plannedV.LengthInt() actualL := actualV.LengthInt() - if plannedL != actualL && !maybeUnknownBlocks { // new blocks may appear if unknown blocks were persent in the plan + if plannedL != actualL && plannedV.IsKnown() { // new blocks may appear if unknown blocks were persent in the plan errs = append(errs, path.NewErrorf("block count changed from %d to %d", plannedL, actualL)) continue } @@ -177,7 +161,7 @@ func assertObjectCompatible(schema *configschema.Block, planned, actual cty.Valu continue } - if maybeUnknownBlocks { + if !plannedV.IsKnown() { // When unknown blocks are present the final number of blocks // may be different, either because the unknown set values // become equal and are collapsed, or the count is unknown due @@ -328,96 +312,6 @@ func indexStrForErrors(v cty.Value) string { } } -// couldHaveUnknownBlockPlaceholder is a heuristic that recognizes how the -// HCL dynamic block extension behaves when it's asked to expand a block whose -// for_each argument is unknown. In such cases, it generates a single placeholder -// block with all leaf attribute values unknown, and once the for_each -// expression becomes known the placeholder may be replaced with any number -// of blocks, so object compatibility checks would need to be more liberal. -// -// Set "nested" if testing a block that is nested inside a candidate block -// placeholder; this changes the interpretation of there being no blocks of -// a type to allow for there being zero nested blocks. -func couldHaveUnknownBlockPlaceholder(v cty.Value, blockS *configschema.NestedBlock, nested bool) bool { - switch blockS.Nesting { - case configschema.NestingSingle, configschema.NestingGroup: - if nested && v.IsNull() { - return true // for nested blocks, a single block being unset doesn't disqualify from being an unknown block placeholder - } - return couldBeUnknownBlockPlaceholderElement(v, blockS) - default: - // These situations should be impossible for correct providers, but - // we permit the legacy SDK to produce some incorrect outcomes - // for compatibility with its existing logic, and so we must be - // tolerant here. - if !v.IsKnown() { - return true - } - if v.IsNull() { - return false // treated as if the list were empty, so we would see zero iterations below - } - - // Unmark before we call ElementIterator in case this iterable is marked sensitive. - // This can arise in the case where a member of a Set is sensitive, and thus the - // whole Set is marked sensitive - v, _ := v.Unmark() - // For all other nesting modes, our value should be something iterable. - for it := v.ElementIterator(); it.Next(); { - _, ev := it.Element() - if couldBeUnknownBlockPlaceholderElement(ev, blockS) { - return true - } - } - - // Our default changes depending on whether we're testing the candidate - // block itself or something nested inside of it: zero blocks of a type - // can never contain a dynamic block placeholder, but a dynamic block - // placeholder might contain zero blocks of one of its own nested block - // types, if none were set in the config at all. - return nested - } -} - -func couldBeUnknownBlockPlaceholderElement(v cty.Value, schema *configschema.NestedBlock) bool { - if v.IsNull() { - return false // null value can never be a placeholder element - } - if !v.IsKnown() { - return true // this should never happen for well-behaved providers, but can happen with the legacy SDK opt-outs - } - for name := range schema.Attributes { - av := v.GetAttr(name) - - // Unknown block placeholders contain only unknown or null attribute - // values, depending on whether or not a particular attribute was set - // explicitly inside the content block. Note that this is imprecise: - // non-placeholders can also match this, so this function can generate - // false positives. - if av.IsKnown() && !av.IsNull() { - - // FIXME: only required for the legacy SDK, but we don't have a - // separate codepath to switch the comparisons, and we still want - // the rest of the checks from AssertObjectCompatible to apply. - // - // The legacy SDK cannot handle missing strings from set elements, - // and will insert an empty string into the planned value. - // Skipping these treats them as null values in this case, - // preventing false alerts from AssertObjectCompatible. - if schema.Nesting == configschema.NestingSet && av.Type() == cty.String && av.AsString() == "" { - continue - } - - return false - } - } - for name, blockS := range schema.BlockTypes { - if !couldHaveUnknownBlockPlaceholder(v.GetAttr(name), blockS, true) { - return false - } - } - return true -} - // assertSetValuesCompatible checks that each of the elements in a can // be correlated with at least one equivalent element in b and vice-versa, // using the given correlation function. diff --git a/plans/objchange/compatible_test.go b/plans/objchange/compatible_test.go index 678ce8ce34..942a477eaa 100644 --- a/plans/objchange/compatible_test.go +++ b/plans/objchange/compatible_test.go @@ -30,10 +30,6 @@ func TestAssertObjectCompatible(t *testing.T) { "foo": cty.StringVal("bar"), "bar": cty.NullVal(cty.String), // simulating the situation where bar isn't set in the config at all }) - fooBarBlockDynamicPlaceholder := cty.ObjectVal(map[string]cty.Value{ - "foo": cty.UnknownVal(cty.String), - "bar": cty.NullVal(cty.String), // simulating the situation where bar isn't set in the config at all - }) tests := []struct { Schema *configschema.Block @@ -919,13 +915,11 @@ func TestAssertObjectCompatible(t *testing.T) { }, }, }, - cty.ObjectVal(map[string]cty.Value{ - "key": cty.ObjectVal(map[string]cty.Value{ - // One wholly unknown block is what "dynamic" blocks - // generate when the for_each expression is unknown. - "foo": cty.UnknownVal(cty.String), + cty.UnknownVal(cty.Object(map[string]cty.Type{ + "key": cty.Object(map[string]cty.Type{ + "foo": cty.String, }), - }), + })), cty.ObjectVal(map[string]cty.Value{ "key": cty.NullVal(cty.Object(map[string]cty.Type{ "foo": cty.String, @@ -1011,11 +1005,9 @@ func TestAssertObjectCompatible(t *testing.T) { }, }, }, - cty.ObjectVal(map[string]cty.Value{ - "key": cty.ListVal([]cty.Value{ - fooBarBlockDynamicPlaceholder, // the presence of this disables some of our checks - }), - }), + cty.UnknownVal(cty.Object(map[string]cty.Type{ + "key": cty.List(fooBarBlockValue.Type()), + })), cty.ObjectVal(map[string]cty.Value{ "key": cty.ListVal([]cty.Value{ cty.ObjectVal(map[string]cty.Value{ @@ -1026,35 +1018,7 @@ func TestAssertObjectCompatible(t *testing.T) { }), }), }), - nil, // a single block whose attrs are all unknown is allowed to expand into multiple, because that's how dynamic blocks behave when for_each is unknown - }, - { - &configschema.Block{ - BlockTypes: map[string]*configschema.NestedBlock{ - "key": { - Nesting: configschema.NestingList, - Block: schemaWithFooBar, - }, - }, - }, - cty.ObjectVal(map[string]cty.Value{ - "key": cty.ListVal([]cty.Value{ - fooBarBlockValue, // the presence of one static block does not negate that the following element looks like a dynamic placeholder - fooBarBlockDynamicPlaceholder, // the presence of this disables some of our checks - }), - }), - cty.ObjectVal(map[string]cty.Value{ - "key": cty.ListVal([]cty.Value{ - fooBlockValue, - cty.ObjectVal(map[string]cty.Value{ - "foo": cty.StringVal("hello"), - }), - cty.ObjectVal(map[string]cty.Value{ - "foo": cty.StringVal("world"), - }), - }), - }), - nil, // as above, the presence of a block whose attrs are all unknown indicates dynamic block expansion, so our usual count checks don't apply + nil, // an unknown block is allowed to expand into multiple, because that's how dynamic blocks behave when for_each is unknown }, { &configschema.Block{ @@ -1195,14 +1159,11 @@ func TestAssertObjectCompatible(t *testing.T) { }, }, cty.ObjectVal(map[string]cty.Value{ - "block": cty.SetVal([]cty.Value{ - cty.ObjectVal(map[string]cty.Value{ - "foo": cty.UnknownVal(cty.String), + "block": cty.UnknownVal(cty.Set( + cty.Object(map[string]cty.Type{ + "foo": cty.String, }), - cty.ObjectVal(map[string]cty.Value{ - "foo": cty.UnknownVal(cty.String), - }), - }), + )), }), cty.ObjectVal(map[string]cty.Value{ "block": cty.SetVal([]cty.Value{ @@ -1221,47 +1182,6 @@ func TestAssertObjectCompatible(t *testing.T) { // indicates this may be a dynamic block, and the length is unknown nil, }, - { - &configschema.Block{ - BlockTypes: map[string]*configschema.NestedBlock{ - "block": { - Nesting: configschema.NestingSet, - Block: schemaWithFooBar, - }, - }, - }, - // The legacy SDK cannot handle missing strings in sets, and will - // insert empty strings to the planned value. Empty strings should - // be handled as nulls, and this object should represent a possible - // dynamic block. - cty.ObjectVal(map[string]cty.Value{ - "block": cty.SetVal([]cty.Value{ - cty.ObjectVal(map[string]cty.Value{ - "foo": cty.UnknownVal(cty.String), - "bar": cty.StringVal(""), - }), - }), - }), - cty.ObjectVal(map[string]cty.Value{ - "block": cty.SetVal([]cty.Value{ - cty.ObjectVal(map[string]cty.Value{ - "foo": cty.StringVal("hello"), - "bar": cty.StringVal(""), - }), - cty.ObjectVal(map[string]cty.Value{ - "foo": cty.StringVal("world"), - "bar": cty.StringVal(""), - }), - cty.ObjectVal(map[string]cty.Value{ - "foo": cty.StringVal("nope"), - "bar": cty.StringVal(""), - }), - }), - }), - // there is no error here, because the presence of unknowns - // indicates this may be a dynamic block, and the length is unknown - nil, - }, { &configschema.Block{ BlockTypes: map[string]*configschema.NestedBlock{ @@ -1335,11 +1255,7 @@ func TestAssertObjectCompatible(t *testing.T) { }, }, cty.ObjectVal(map[string]cty.Value{ - "block": cty.SetVal([]cty.Value{ - cty.ObjectVal(map[string]cty.Value{ - "foo": cty.UnknownVal(cty.String), - }), - }), + "block": cty.UnknownVal(cty.Set(fooBlockValue.Type())), }), cty.ObjectVal(map[string]cty.Value{ "block": cty.SetVal([]cty.Value{ @@ -1364,11 +1280,7 @@ func TestAssertObjectCompatible(t *testing.T) { }, }, cty.ObjectVal(map[string]cty.Value{ - "block2": cty.SetVal([]cty.Value{ - cty.ObjectVal(map[string]cty.Value{ - "foo": cty.UnknownVal(cty.String), - }), - }), + "block2": cty.UnknownVal(cty.Set(fooBlockValue.Type())), }), cty.ObjectVal(map[string]cty.Value{ "block2": cty.SetValEmpty(cty.Object(map[string]cty.Type{ @@ -1406,37 +1318,6 @@ func TestAssertObjectCompatible(t *testing.T) { }), nil, }, - { - &configschema.Block{ - BlockTypes: map[string]*configschema.NestedBlock{ - "block": { - Nesting: configschema.NestingSet, - Block: schemaWithFooBar, - }, - }, - }, - cty.ObjectVal(map[string]cty.Value{ - "block": cty.SetVal([]cty.Value{ - cty.ObjectVal(map[string]cty.Value{ - "foo": cty.UnknownVal(cty.String), - "bar": cty.NullVal(cty.String), - }), - }), - }), - cty.ObjectVal(map[string]cty.Value{ - "block": cty.SetVal([]cty.Value{ - cty.ObjectVal(map[string]cty.Value{ - "foo": cty.StringVal("a"), - "bar": cty.StringVal(""), - }), - cty.ObjectVal(map[string]cty.Value{ - "foo": cty.StringVal("b"), - "bar": cty.StringVal(""), - }), - }), - }), - nil, - }, { &configschema.Block{ BlockTypes: map[string]*configschema.NestedBlock{ diff --git a/plans/objchange/objchange.go b/plans/objchange/objchange.go index fade668c28..6bfc06e22c 100644 --- a/plans/objchange/objchange.go +++ b/plans/objchange/objchange.go @@ -93,6 +93,12 @@ func proposedNew(schema *configschema.Block, prior, config cty.Value) cty.Value } func proposedNewNestedBlock(schema *configschema.NestedBlock, prior, config cty.Value) cty.Value { + // The only time we should encounter an entirely unknown block is from the + // use of dynamic with an unknown for_each expression. + if !config.IsKnown() { + return config + } + var newV cty.Value switch schema.Nesting { @@ -103,7 +109,7 @@ func proposedNewNestedBlock(schema *configschema.NestedBlock, prior, config cty. case configschema.NestingList: // Nested blocks are correlated by index. configVLen := 0 - if config.IsKnown() && !config.IsNull() { + if !config.IsNull() { configVLen = config.LengthInt() } if configVLen > 0 { diff --git a/terraform/context_validate_test.go b/terraform/context_validate_test.go index 0361e30e44..2c3454b1a6 100644 --- a/terraform/context_validate_test.go +++ b/terraform/context_validate_test.go @@ -2000,3 +2000,95 @@ func TestContext2Validate_sensitiveProvisionerConfig(t *testing.T) { t.Fatal("ValidateProvisionerConfig not called") } } + +func TestContext2Plan_validateMinMaxDynamicBlock(t *testing.T) { + p := new(MockProvider) + p.GetProviderSchemaResponse = getProviderSchemaResponseFromProviderSchema(&ProviderSchema{ + ResourceTypes: map[string]*configschema.Block{ + "test_instance": { + Attributes: map[string]*configschema.Attribute{ + "id": { + Type: cty.String, + Computed: true, + }, + "things": { + Type: cty.List(cty.String), + Computed: true, + }, + }, + BlockTypes: map[string]*configschema.NestedBlock{ + "foo": { + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "bar": {Type: cty.String, Optional: true}, + }, + }, + Nesting: configschema.NestingList, + MinItems: 2, + MaxItems: 3, + }, + }, + }, + }, + }) + + m := testModuleInline(t, map[string]string{ + "main.tf": ` +resource "test_instance" "a" { + // MinItems 2 + foo { + bar = "a" + } + foo { + bar = "b" + } +} + +resource "test_instance" "b" { + // one dymamic block can satisfy MinItems of 2 + dynamic "foo" { + for_each = test_instance.a.things + content { + bar = foo.value + } + } +} + +resource "test_instance" "c" { + // we may have more than MaxItems dynamic blocks when they are unknown + foo { + bar = "b" + } + dynamic "foo" { + for_each = test_instance.a.things + content { + bar = foo.value + } + } + dynamic "foo" { + for_each = test_instance.a.things + content { + bar = "${foo.value}-2" + } + } + dynamic "foo" { + for_each = test_instance.b.things + content { + bar = foo.value + } + } +} +`}) + + ctx := testContext2(t, &ContextOpts{ + Config: m, + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + diags := ctx.Validate() + if diags.HasErrors() { + t.Fatal(diags.ErrWithWarnings()) + } +}