From 6c5819f9109499621801a6f38b8a71ee360bfd2c Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 29 Mar 2019 16:24:02 -0700 Subject: [PATCH] configs/configupgrade: Prefer block syntax for list-of-object attributes In order to preserve pre-v0.12 idiom for list-of-object attributes, we'll prefer to use block syntax for them except for the special situation where the user explicitly assigns an empty list, where attribute syntax is required in order to allow existing provider logic to differentiate from an implicit lack of blocks. --- .../input/list-of-obj-as-block.tf | 15 ++++++++ .../want/list-of-obj-as-block.tf | 17 ++++++++++ .../list-of-obj-as-block/want/versions.tf | 3 ++ configs/configupgrade/upgrade_body.go | 31 ++++++++++++++--- configs/configupgrade/upgrade_test.go | 1 + lang/blocktoattr/fixup.go | 4 +-- lang/blocktoattr/schema.go | 34 +++++++++++++++++-- lang/blocktoattr/variables.go | 2 +- 8 files changed, 97 insertions(+), 10 deletions(-) create mode 100644 configs/configupgrade/test-fixtures/valid/list-of-obj-as-block/input/list-of-obj-as-block.tf create mode 100644 configs/configupgrade/test-fixtures/valid/list-of-obj-as-block/want/list-of-obj-as-block.tf create mode 100644 configs/configupgrade/test-fixtures/valid/list-of-obj-as-block/want/versions.tf diff --git a/configs/configupgrade/test-fixtures/valid/list-of-obj-as-block/input/list-of-obj-as-block.tf b/configs/configupgrade/test-fixtures/valid/list-of-obj-as-block/input/list-of-obj-as-block.tf new file mode 100644 index 000000000..72e5a3e5a --- /dev/null +++ b/configs/configupgrade/test-fixtures/valid/list-of-obj-as-block/input/list-of-obj-as-block.tf @@ -0,0 +1,15 @@ +resource "test_instance" "from_list" { + list_of_obj = [ + {}, + {}, + ] +} + +resource "test_instance" "already_blocks" { + list_of_obj {} + list_of_obj {} +} + +resource "test_instance" "empty" { + list_of_obj = [] +} diff --git a/configs/configupgrade/test-fixtures/valid/list-of-obj-as-block/want/list-of-obj-as-block.tf b/configs/configupgrade/test-fixtures/valid/list-of-obj-as-block/want/list-of-obj-as-block.tf new file mode 100644 index 000000000..1d77883a8 --- /dev/null +++ b/configs/configupgrade/test-fixtures/valid/list-of-obj-as-block/want/list-of-obj-as-block.tf @@ -0,0 +1,17 @@ +resource "test_instance" "from_list" { + list_of_obj { + } + list_of_obj { + } +} + +resource "test_instance" "already_blocks" { + list_of_obj { + } + list_of_obj { + } +} + +resource "test_instance" "empty" { + list_of_obj = [] +} diff --git a/configs/configupgrade/test-fixtures/valid/list-of-obj-as-block/want/versions.tf b/configs/configupgrade/test-fixtures/valid/list-of-obj-as-block/want/versions.tf new file mode 100644 index 000000000..d9b6f790b --- /dev/null +++ b/configs/configupgrade/test-fixtures/valid/list-of-obj-as-block/want/versions.tf @@ -0,0 +1,3 @@ +terraform { + required_version = ">= 0.12" +} diff --git a/configs/configupgrade/upgrade_body.go b/configs/configupgrade/upgrade_body.go index cc82e9a9f..39f71f3d6 100644 --- a/configs/configupgrade/upgrade_body.go +++ b/configs/configupgrade/upgrade_body.go @@ -13,12 +13,12 @@ import ( hcl1token "github.com/hashicorp/hcl/hcl/token" hcl2 "github.com/hashicorp/hcl2/hcl" hcl2syntax "github.com/hashicorp/hcl2/hcl/hclsyntax" - "github.com/zclconf/go-cty/cty" - "github.com/hashicorp/terraform/configs/configschema" + "github.com/hashicorp/terraform/lang/blocktoattr" "github.com/hashicorp/terraform/registry/regsrc" "github.com/hashicorp/terraform/terraform" "github.com/hashicorp/terraform/tfdiags" + "github.com/zclconf/go-cty/cty" ) // bodyContentRules is a mapping from item names (argument names and block type @@ -318,7 +318,7 @@ func nestedBlockRule(filename string, nestedRules bodyContentRules, an *analysis } } -func nestedBlockRuleWithDynamic(filename string, nestedRules bodyContentRules, nestedSchema *configschema.NestedBlock, an *analysis, adhocComments *commentQueue) bodyItemRule { +func nestedBlockRuleWithDynamic(filename string, nestedRules bodyContentRules, nestedSchema *configschema.NestedBlock, emptyAsAttr bool, an *analysis, adhocComments *commentQueue) bodyItemRule { return func(buf *bytes.Buffer, blockAddr string, item *hcl1ast.ObjectItem) tfdiags.Diagnostics { // In Terraform v0.11 it was possible in some cases to trick Terraform // and providers into accepting HCL's attribute syntax and some HIL @@ -389,6 +389,17 @@ func nestedBlockRuleWithDynamic(filename string, nestedRules bodyContentRules, n blockItems = append(blockItems, item.Val) } + if len(blockItems) == 0 && emptyAsAttr { + // Terraform v0.12's config decoder allows using block syntax for + // certain attribute types, which we prefer as idiomatic usage + // causing us to end up in this function in such cases, but as + // a special case users can still use the attribute syntax to + // explicitly write an empty list. For more information, see + // the lang/blocktoattr package. + printAttribute(buf, item.Keys[0].Token.Value().(string), []byte{'[', ']'}, item.LineComment) + return diags + } + for _, blockItem := range blockItems { switch ti := blockItem.(type) { case *hcl1ast.ObjectType: @@ -455,11 +466,23 @@ func schemaDefaultBodyRules(filename string, schema *configschema.Block, an *ana } for name, attrS := range schema.Attributes { + if aty := attrS.Type; blocktoattr.TypeCanBeBlocks(aty) { + // Terraform's standard body processing rules for arbitrary schemas + // have a special case where list-of-object or set-of-object + // attributes can be specified as a sequence of nested blocks + // instead of a single list attribute. We prefer that form during + // upgrade for historical reasons, to avoid making large changes + // to existing configurations that were following documented idiom. + synthSchema := blocktoattr.SchemaForCtyContainerType(aty) + nestedRules := schemaDefaultBodyRules(filename, &synthSchema.Block, an, adhocComments) + ret[name] = nestedBlockRuleWithDynamic(filename, nestedRules, synthSchema, true, an, adhocComments) + continue + } ret[name] = normalAttributeRule(filename, attrS.Type, an) } for name, blockS := range schema.BlockTypes { nestedRules := schemaDefaultBodyRules(filename, &blockS.Block, an, adhocComments) - ret[name] = nestedBlockRuleWithDynamic(filename, nestedRules, blockS, an, adhocComments) + ret[name] = nestedBlockRuleWithDynamic(filename, nestedRules, blockS, false, an, adhocComments) } return ret diff --git a/configs/configupgrade/upgrade_test.go b/configs/configupgrade/upgrade_test.go index 3501e730f..63262f687 100644 --- a/configs/configupgrade/upgrade_test.go +++ b/configs/configupgrade/upgrade_test.go @@ -198,6 +198,7 @@ var testProviders = map[string]providers.Factory{ "tags": {Type: cty.Map(cty.String), Optional: true}, "security_groups": {Type: cty.List(cty.String), Optional: true}, "subnet_ids": {Type: cty.Set(cty.String), Optional: true}, + "list_of_obj": {Type: cty.List(cty.EmptyObject), Optional: true}, }, BlockTypes: map[string]*configschema.NestedBlock{ "network": { diff --git a/lang/blocktoattr/fixup.go b/lang/blocktoattr/fixup.go index d9cea443f..29a0f0bad 100644 --- a/lang/blocktoattr/fixup.go +++ b/lang/blocktoattr/fixup.go @@ -146,7 +146,7 @@ func (e *fixupBlocksExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostic // the result is imprecise and in particular will just consider all // the attributes to be optional and let the provider eventually decide // whether to return errors if they turn out to be null when required. - schema := schemaForCtyType(e.ety) // this schema's ImpliedType will match e.ety + schema := SchemaForCtyElementType(e.ety) // this schema's ImpliedType will match e.ety spec := schema.DecoderSpec() vals := make([]cty.Value, len(e.blocks)) @@ -167,7 +167,7 @@ func (e *fixupBlocksExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostic func (e *fixupBlocksExpr) Variables() []hcl.Traversal { var ret []hcl.Traversal - schema := schemaForCtyType(e.ety) + schema := SchemaForCtyElementType(e.ety) spec := schema.DecoderSpec() for _, block := range e.blocks { ret = append(ret, hcldec.Variables(block.Body, spec)...) diff --git a/lang/blocktoattr/schema.go b/lang/blocktoattr/schema.go index 161ce15d4..2f2463a5c 100644 --- a/lang/blocktoattr/schema.go +++ b/lang/blocktoattr/schema.go @@ -99,10 +99,11 @@ func effectiveSchema(given *hcl.BodySchema, body hcl.Body, ambiguousNames map[st return ret } -// schemaForCtyType converts a cty object type into an approximately-equivalent -// configschema.Block. If the given type is not an object type then this +// SchemaForCtyElementType converts a cty object type into an +// approximately-equivalent configschema.Block representing the element of +// a list or set. If the given type is not an object type then this // function will panic. -func schemaForCtyType(ty cty.Type) *configschema.Block { +func SchemaForCtyElementType(ty cty.Type) *configschema.Block { atys := ty.AttributeTypes() ret := &configschema.Block{ Attributes: make(map[string]*configschema.Attribute, len(atys)), @@ -115,3 +116,30 @@ func schemaForCtyType(ty cty.Type) *configschema.Block { } return ret } + +// SchemaForCtyContainerType converts a cty list-of-object or set-of-object type +// into an approximately-equivalent configschema.NestedBlock. If the given type +// is not of the expected kind then this function will panic. +func SchemaForCtyContainerType(ty cty.Type) *configschema.NestedBlock { + var nesting configschema.NestingMode + switch { + case ty.IsListType(): + nesting = configschema.NestingList + case ty.IsSetType(): + nesting = configschema.NestingSet + default: + panic("unsuitable type") + } + nested := SchemaForCtyElementType(ty.ElementType()) + return &configschema.NestedBlock{ + Nesting: nesting, + Block: *nested, + } +} + +// TypeCanBeBlocks returns true if the given type is a list-of-object or +// set-of-object type, and would thus be subject to the blocktoattr fixup +// if used as an attribute type. +func TypeCanBeBlocks(ty cty.Type) bool { + return (ty.IsListType() || ty.IsSetType()) && ty.ElementType().IsObjectType() +} diff --git a/lang/blocktoattr/variables.go b/lang/blocktoattr/variables.go index 0be26b973..e123b8aab 100644 --- a/lang/blocktoattr/variables.go +++ b/lang/blocktoattr/variables.go @@ -34,7 +34,7 @@ func walkVariables(node dynblock.WalkVariablesNode, body hcl.Body, schema *confi if blockS, exists := schema.BlockTypes[child.BlockTypeName]; exists { vars = append(vars, walkVariables(child.Node, child.Body(), &blockS.Block)...) } else if attrS, exists := schema.Attributes[child.BlockTypeName]; exists { - synthSchema := schemaForCtyType(attrS.Type.ElementType()) + synthSchema := SchemaForCtyElementType(attrS.Type.ElementType()) vars = append(vars, walkVariables(child.Node, child.Body(), synthSchema)...) } }