From c7557452854565177288e3e8716ad436c4eafe2d Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 30 Nov 2018 15:25:04 -0800 Subject: [PATCH] configs/configupgrade: Generalize migration of block bodies The main area of interest in upgrading is dealing with special cases for individual block items, so this generalization allows us to use the same overall body-processing logic for everything but to specialize just how individual items are dealt with, which we match by their names as given in the original input source code. --- configs/configupgrade/upgrade_body.go | 160 ++++++++++++++++ configs/configupgrade/upgrade_native.go | 238 +++++++----------------- 2 files changed, 226 insertions(+), 172 deletions(-) create mode 100644 configs/configupgrade/upgrade_body.go diff --git a/configs/configupgrade/upgrade_body.go b/configs/configupgrade/upgrade_body.go new file mode 100644 index 000000000..bd4b81c5b --- /dev/null +++ b/configs/configupgrade/upgrade_body.go @@ -0,0 +1,160 @@ +package configupgrade + +import ( + "bytes" + "fmt" + + hcl1ast "github.com/hashicorp/hcl/hcl/ast" + hcl1printer "github.com/hashicorp/hcl/hcl/printer" + 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/tfdiags" +) + +// bodyContentRules is a mapping from item names (argument names and block type +// names) to a "rule" function defining what to do with an item of that type. +type bodyContentRules map[string]bodyItemRule + +// bodyItemRule is just a function to write an upgraded representation of a +// particular given item to the given buffer. This is generic to handle various +// different mapping rules, though most values will be those constructed by +// other helper functions below. +type bodyItemRule func(buf *bytes.Buffer, blockAddr string, item *hcl1ast.ObjectItem) tfdiags.Diagnostics + +func normalAttributeRule(filename string, wantTy cty.Type, an *analysis) bodyItemRule { + exprRule := func(val interface{}) ([]byte, tfdiags.Diagnostics) { + return upgradeExpr(val, filename, true, an) + } + return attributeRule(filename, wantTy, an, exprRule) +} + +func noInterpAttributeRule(filename string, wantTy cty.Type, an *analysis) bodyItemRule { + exprRule := func(val interface{}) ([]byte, tfdiags.Diagnostics) { + return upgradeExpr(val, filename, false, an) + } + return attributeRule(filename, wantTy, an, exprRule) +} + +func maybeBareKeywordAttributeRule(filename string, an *analysis, specials map[string]string) bodyItemRule { + exprRule := func(val interface{}) ([]byte, tfdiags.Diagnostics) { + // If the expression is a literal that would be valid as a naked keyword + // then we'll turn it into one. + if lit, isLit := val.(*hcl1ast.LiteralType); isLit { + if lit.Token.Type == hcl1token.STRING { + kw := lit.Token.Value().(string) + if hcl2syntax.ValidIdentifier(kw) { + + // If we have a special mapping rule for this keyword, + // we'll let that override what the user gave. + if override := specials[kw]; override != "" { + kw = override + } + + return []byte(kw), nil + } + } + } + + return upgradeExpr(val, filename, false, an) + } + return attributeRule(filename, cty.String, an, exprRule) +} + +func maybeBareTraversalAttributeRule(filename string, an *analysis) bodyItemRule { + exprRule := func(val interface{}) ([]byte, tfdiags.Diagnostics) { + // If the expression is a literal that would be valid as a naked + // absolute traversal then we'll turn it into one. + if lit, isLit := val.(*hcl1ast.LiteralType); isLit { + if lit.Token.Type == hcl1token.STRING { + trStr := lit.Token.Value().(string) + trSrc := []byte(trStr) + _, trDiags := hcl2syntax.ParseTraversalAbs(trSrc, "", hcl2.Pos{}) + if !trDiags.HasErrors() { + return trSrc, nil + } + } + } + + return upgradeExpr(val, filename, false, an) + } + return attributeRule(filename, cty.String, an, exprRule) +} + +func dependsOnAttributeRule(filename string, an *analysis) bodyItemRule { + // FIXME: Should dig into the individual list items here and try to unwrap + // them as naked references, as well as upgrading any legacy-style index + // references like aws_instance.foo.0 to be aws_instance.foo[0] instead. + exprRule := func(val interface{}) ([]byte, tfdiags.Diagnostics) { + return upgradeExpr(val, filename, false, an) + } + return attributeRule(filename, cty.List(cty.String), an, exprRule) +} + +func attributeRule(filename string, wantTy cty.Type, an *analysis, upgradeExpr func(val interface{}) ([]byte, tfdiags.Diagnostics)) bodyItemRule { + return func(buf *bytes.Buffer, blockAddr string, item *hcl1ast.ObjectItem) tfdiags.Diagnostics { + var diags tfdiags.Diagnostics + + name := item.Keys[0].Token.Value().(string) + + // We'll tolerate a block with no labels here as a degenerate + // way to assign a map, but we can't migrate a block that has + // labels. In practice this should never happen because + // nested blocks in resource blocks did not accept labels + // prior to v0.12. + if len(item.Keys) != 1 { + diags = diags.Append(&hcl2.Diagnostic{ + Severity: hcl2.DiagError, + Summary: "Block where attribute was expected", + Detail: fmt.Sprintf("Within %s the name %q is an attribute name, not a block type.", blockAddr, name), + Subject: hcl1PosRange(filename, item.Keys[0].Pos()).Ptr(), + }) + return diags + } + + valSrc, valDiags := upgradeExpr(item.Val) + diags = diags.Append(valDiags) + printAttribute(buf, item.Keys[0].Token.Value().(string), valSrc, item.LineComment) + + return diags + } +} + +func nestedBlockRule(filename string, nestedRules bodyContentRules, an *analysis) bodyItemRule { + return func(buf *bytes.Buffer, blockAddr string, item *hcl1ast.ObjectItem) tfdiags.Diagnostics { + // TODO: Deal with this. + // In particular we need to handle the tricky case where + // a user attempts to treat a block type name like it's + // an attribute, by producing a "dynamic" block. + hcl1printer.Fprint(buf, item) + buf.WriteByte('\n') + return nil + } +} + +// schemaDefaultBodyRules constructs standard body content rules for the given +// schema. Each call is guaranteed to produce a distinct object so that +// callers can safely mutate the result in order to impose custom rules +// in addition to or instead of those created by default, for situations +// where schema-based and predefined items mix in a single body. +func schemaDefaultBodyRules(filename string, schema *configschema.Block, an *analysis) bodyContentRules { + ret := make(bodyContentRules) + if schema == nil { + // Shouldn't happen in any real case, but often crops up in tests + // where the mock schemas tend to be incomplete. + return ret + } + + for name, attrS := range schema.Attributes { + ret[name] = normalAttributeRule(filename, attrS.Type, an) + } + for name, blockS := range schema.BlockTypes { + nestedRules := schemaDefaultBodyRules(filename, &blockS.Block, an) + ret[name] = nestedBlockRule(filename, nestedRules, an) + } + + return ret +} diff --git a/configs/configupgrade/upgrade_native.go b/configs/configupgrade/upgrade_native.go index 15b4cc439..efc04a932 100644 --- a/configs/configupgrade/upgrade_native.go +++ b/configs/configupgrade/upgrade_native.go @@ -15,10 +15,9 @@ 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/addrs" - "github.com/hashicorp/terraform/configs/configschema" "github.com/hashicorp/terraform/tfdiags" ) @@ -119,138 +118,62 @@ func (u *Upgrader) upgradeNativeSyntaxFile(filename string, src []byte, an *anal diags = diags.Append(moreDiags) case "variable": + if len(labels) != 1 { + diags = diags.Append(&hcl2.Diagnostic{ + Severity: hcl2.DiagError, + Summary: fmt.Sprintf("Invalid %s block", blockType), + Detail: fmt.Sprintf("A %s block must have one label: the variable name.", blockType), + Subject: &declRange, + }) + continue + } + printComments(&buf, item.LeadComment) printBlockOpen(&buf, blockType, labels, item.LineComment) - args := body.List.Items - for i, arg := range args { - if len(arg.Keys) != 1 { - // Should never happen for valid input, since there are no nested blocks expected here. - diags = diags.Append(&hcl2.Diagnostic{ - Severity: hcl2.DiagWarning, - Summary: "Invalid nested block", - Detail: fmt.Sprintf("Blocks of type %q are not expected here, so this was not automatically upgraded.", arg.Keys[0].Token.Value().(string)), - Subject: hcl1PosRange(filename, arg.Keys[0].Pos()).Ptr(), - }) - // Preserve the item as-is, using the hcl1printer package. - buf.WriteString("\n# TF-UPGRADE-TODO: Blocks are not expected here, so this was not automatically upgraded.\n") - hcl1printer.Fprint(&buf, arg) - buf.WriteString("\n\n") - continue - } - - comments := adhocComments.TakeBefore(arg) - for _, group := range comments { - printComments(&buf, group) - buf.WriteByte('\n') // Extra separator after each group - } - - printComments(&buf, arg.LeadComment) - - switch arg.Keys[0].Token.Value() { - case "type": - // It is no longer idiomatic to place the type keyword in quotes, - // so we'll unquote it here as long as it looks like the result - // will be valid. - if lit, isLit := arg.Val.(*hcl1ast.LiteralType); isLit { - if lit.Token.Type == hcl1token.STRING { - kw := lit.Token.Value().(string) - if hcl2syntax.ValidIdentifier(kw) { - - // "list" and "map" in older versions really meant - // list and map of strings, so we'll migrate to - // that and let the user adjust to "any" as - // the element type if desired. - switch strings.TrimSpace(kw) { - case "list": - kw = "list(string)" - case "map": - kw = "map(string)" - } - - printAttribute(&buf, "type", []byte(kw), arg.LineComment) - break - } - } - } - // If we got something invalid there then we'll just fall through - // into the default case and migrate it as a normal expression. - fallthrough - default: - valSrc, valDiags := upgradeExpr(arg.Val, filename, false, an) - diags = diags.Append(valDiags) - printAttribute(&buf, arg.Keys[0].Token.Value().(string), valSrc, arg.LineComment) - } - - // If we have another item and it's more than one line away - // from the current one then we'll print an extra blank line - // to retain that separation. - if (i + 1) < len(args) { - next := args[i+1] - thisPos := arg.Pos() - nextPos := next.Pos() - if nextPos.Line-thisPos.Line > 1 { - buf.WriteByte('\n') - } - } + rules := bodyContentRules{ + "description": noInterpAttributeRule(filename, cty.String, an), + "default": noInterpAttributeRule(filename, cty.DynamicPseudoType, an), + "type": maybeBareKeywordAttributeRule(filename, an, map[string]string{ + // "list" and "map" in older versions were documented to + // mean list and map of strings, so we'll migrate to that + // and let the user adjust it to some other type if desired. + "list": `list(string)`, + "map": `map(string)`, + }), } + u.upgradeBlockBody(filename, fmt.Sprintf("var.%s", labels[0]), &buf, body.List.Items, rules, adhocComments) buf.WriteString("}\n\n") case "output": + if len(labels) != 1 { + diags = diags.Append(&hcl2.Diagnostic{ + Severity: hcl2.DiagError, + Summary: fmt.Sprintf("Invalid %s block", blockType), + Detail: fmt.Sprintf("A %s block must have one label: the output name.", blockType), + Subject: &declRange, + }) + continue + } + printComments(&buf, item.LeadComment) printBlockOpen(&buf, blockType, labels, item.LineComment) - args := body.List.Items - for i, arg := range args { - if len(arg.Keys) != 1 { - // Should never happen for valid input, since there are no nested blocks expected here. - diags = diags.Append(&hcl2.Diagnostic{ - Severity: hcl2.DiagWarning, - Summary: "Invalid nested block", - Detail: fmt.Sprintf("Blocks of type %q are not expected here, so this was not automatically upgraded.", arg.Keys[0].Token.Value().(string)), - Subject: hcl1PosRange(filename, arg.Keys[0].Pos()).Ptr(), - }) - // Preserve the item as-is, using the hcl1printer package. - buf.WriteString("\n# TF-UPGRADE-TODO: Blocks are not expected here, so this was not automatically upgraded.\n") - hcl1printer.Fprint(&buf, arg) - buf.WriteString("\n\n") - continue - } - comments := adhocComments.TakeBefore(arg) - for _, group := range comments { - printComments(&buf, group) - buf.WriteByte('\n') // Extra separator after each group - } - - printComments(&buf, arg.LeadComment) - - interp := false - switch arg.Keys[0].Token.Value() { - case "value": - interp = true - } - - valSrc, valDiags := upgradeExpr(arg.Val, filename, interp, an) - diags = diags.Append(valDiags) - printAttribute(&buf, arg.Keys[0].Token.Value().(string), valSrc, arg.LineComment) - - // If we have another item and it's more than one line away - // from the current one then we'll print an extra blank line - // to retain that separation. - if (i + 1) < len(args) { - next := args[i+1] - thisPos := arg.Pos() - nextPos := next.Pos() - if nextPos.Line-thisPos.Line > 1 { - buf.WriteByte('\n') - } - } + rules := bodyContentRules{ + "description": noInterpAttributeRule(filename, cty.String, an), + "value": normalAttributeRule(filename, cty.DynamicPseudoType, an), + "sensitive": noInterpAttributeRule(filename, cty.Bool, an), + "depends_on": dependsOnAttributeRule(filename, an), } + u.upgradeBlockBody(filename, fmt.Sprintf("output.%s", labels[0]), &buf, body.List.Items, rules, adhocComments) buf.WriteString("}\n\n") case "locals": printComments(&buf, item.LeadComment) printBlockOpen(&buf, blockType, labels, item.LineComment) + // The "locals" block contents are free-form declarations, so + // we'll need to treat this one as special and not do a rules-based + // upgrade as we do for most other block types. args := body.List.Items for i, arg := range args { if len(arg.Keys) != 1 { @@ -360,9 +283,11 @@ func (u *Upgrader) upgradeNativeSyntaxResource(filename string, buf *bytes.Buffe } labels := []string{addr.Type, addr.Name} + rules := schemaDefaultBodyRules(filename, schema, an) + printComments(buf, item.LeadComment) printBlockOpen(buf, blockType, labels, item.LineComment) - u.upgradeBlockBody(filename, addr.String(), buf, body.List.Items, schema, an, adhocComments) + u.upgradeBlockBody(filename, addr.String(), buf, body.List.Items, rules, adhocComments) buf.WriteString("}\n\n") return diags @@ -380,16 +305,17 @@ func (u *Upgrader) upgradeNativeSyntaxProvider(filename string, buf *bytes.Buffe panic(fmt.Sprintf("missing schema for provider type %q", typeName)) } schema := providerSchema.Provider + rules := schemaDefaultBodyRules(filename, schema, an) printComments(buf, item.LeadComment) printBlockOpen(buf, "provider", []string{typeName}, item.LineComment) - u.upgradeBlockBody(filename, fmt.Sprintf("provider.%s", typeName), buf, body.List.Items, schema, an, adhocComments) + u.upgradeBlockBody(filename, fmt.Sprintf("provider.%s", typeName), buf, body.List.Items, rules, adhocComments) buf.WriteString("}\n\n") return diags } -func (u *Upgrader) upgradeBlockBody(filename string, blockAddr string, buf *bytes.Buffer, args []*hcl1ast.ObjectItem, schema *configschema.Block, an *analysis, adhocComments *commentQueue) tfdiags.Diagnostics { +func (u *Upgrader) upgradeBlockBody(filename string, blockAddr string, buf *bytes.Buffer, args []*hcl1ast.ObjectItem, rules bodyContentRules, adhocComments *commentQueue) tfdiags.Diagnostics { var diags tfdiags.Diagnostics for i, arg := range args { @@ -404,61 +330,29 @@ func (u *Upgrader) upgradeBlockBody(filename string, blockAddr string, buf *byte name := arg.Keys[0].Token.Value().(string) //labelKeys := arg.Keys[1:] - switch name { - // TODO: Special case for all of the "meta-arguments" allowed - // in a resource block, such as "count", "lifecycle", - // "provisioner", etc. - - default: - // We'll consult the schema to see how we ought to interpret - // this item. - - if _, isAttr := schema.Attributes[name]; isAttr { - // We'll tolerate a block with no labels here as a degenerate - // way to assign a map, but we can't migrate a block that has - // labels. In practice this should never happen because - // nested blocks in resource blocks did not accept labels - // prior to v0.12. - if len(arg.Keys) != 1 { - diags = diags.Append(&hcl2.Diagnostic{ - Severity: hcl2.DiagError, - Summary: "Block where attribute was expected", - Detail: fmt.Sprintf("Within %s the name %q is an attribute name, not a block type.", blockAddr, name), - Subject: hcl1PosRange(filename, arg.Keys[0].Pos()).Ptr(), - }) - continue - } - - valSrc, valDiags := upgradeExpr(arg.Val, filename, true, an) - diags = diags.Append(valDiags) - printAttribute(buf, arg.Keys[0].Token.Value().(string), valSrc, arg.LineComment) - } else if _, isBlock := schema.BlockTypes[name]; isBlock { - // TODO: Also upgrade blocks. - // In particular we need to handle the tricky case where - // a user attempts to treat a block type name like it's - // an attribute, by producing a "dynamic" block. - hcl1printer.Fprint(buf, arg) - buf.WriteByte('\n') + rule, expected := rules[name] + if !expected { + if arg.Assign.IsValid() { + diags = diags.Append(&hcl2.Diagnostic{ + Severity: hcl2.DiagError, + Summary: "Unrecognized attribute name", + Detail: fmt.Sprintf("No attribute named %q is expected in %s.", name, blockAddr), + Subject: hcl1PosRange(filename, arg.Keys[0].Pos()).Ptr(), + }) } else { - if arg.Assign.IsValid() { - diags = diags.Append(&hcl2.Diagnostic{ - Severity: hcl2.DiagError, - Summary: "Unrecognized attribute name", - Detail: fmt.Sprintf("No attribute named %q is expected in %s.", name, blockAddr), - Subject: hcl1PosRange(filename, arg.Keys[0].Pos()).Ptr(), - }) - } else { - diags = diags.Append(&hcl2.Diagnostic{ - Severity: hcl2.DiagError, - Summary: "Unrecognized block type", - Detail: fmt.Sprintf("Blocks of type %q are not expected in %s.", name, blockAddr), - Subject: hcl1PosRange(filename, arg.Keys[0].Pos()).Ptr(), - }) - } - continue + diags = diags.Append(&hcl2.Diagnostic{ + Severity: hcl2.DiagError, + Summary: "Unrecognized block type", + Detail: fmt.Sprintf("Blocks of type %q are not expected in %s.", name, blockAddr), + Subject: hcl1PosRange(filename, arg.Keys[0].Pos()).Ptr(), + }) } + continue } + itemDiags := rule(buf, blockAddr, arg) + diags = diags.Append(itemDiags) + // If we have another item and it's more than one line away // from the current one then we'll print an extra blank line // to retain that separation.