From f93f7e5b5c86eb317abd99df6df1871752b5f54e Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 6 Dec 2018 11:56:43 -0800 Subject: [PATCH] configs/configupgrade: Remove redundant list brackets In early versions of Terraform where the interpolation language didn't have any real list support, list brackets around a single string was the signal to split the string on a special uuid separator to produce a list just in time for processing, giving expressions like this: foo = ["${test_instance.foo.*.id}"] Logically this is weird because it looks like it should produce a list of lists of strings. When we added real list support in Terraform 0.7 we retained support for this behavior by trimming off extra levels of list during evaluation, and inadvertently continued relying on this notation for correct type checking. During the Terraform 0.10 line we fixed the type checker bugs (a few remaining issues notwithstanding) so that it was finally possible to use the more intuitive form: foo = "${test_instance.foo.*.id}" ...but we continued trimming off extra levels of list for backward compatibility. Terraform 0.12 finally removes that compatibility shim, causing redundant list brackets to be interpreted as a list of lists. This upgrade rule attempts to identify situations that are relying on the old compatibility behavior and trim off the redundant extra brackets. It's not possible to do this fully-generally using only static analysis, but we can gather enough information through or partial type inference mechanism here to deal with the most common situations automatically and produce a TF-UPGRADE-TODO comment for more complex scenarios where the user intent isn't decidable with only static analysis. In particular, this handles by far the most common situation of wrapping list brackets around a splat expression like the first example above. After this and the other upgrade rules are applied, the first example above will become: foo = test_instance.foo.*.id --- .../redundant-list/input/redundant-list.tf | 52 +++++++++++++ .../redundant-list/want/redundant-list.tf | 60 +++++++++++++++ .../valid/redundant-list/want/versions.tf | 3 + configs/configupgrade/upgrade_body.go | 74 ++++++++++++++++++- configs/configupgrade/upgrade_expr.go | 5 ++ configs/configupgrade/upgrade_test.go | 9 ++- 6 files changed, 197 insertions(+), 6 deletions(-) create mode 100644 configs/configupgrade/test-fixtures/valid/redundant-list/input/redundant-list.tf create mode 100644 configs/configupgrade/test-fixtures/valid/redundant-list/want/redundant-list.tf create mode 100644 configs/configupgrade/test-fixtures/valid/redundant-list/want/versions.tf diff --git a/configs/configupgrade/test-fixtures/valid/redundant-list/input/redundant-list.tf b/configs/configupgrade/test-fixtures/valid/redundant-list/input/redundant-list.tf new file mode 100644 index 000000000..35e30466d --- /dev/null +++ b/configs/configupgrade/test-fixtures/valid/redundant-list/input/redundant-list.tf @@ -0,0 +1,52 @@ +variable "listy" { + type = "list" +} + +resource "test_instance" "other" { + count = 2 +} + +resource "test_instance" "bad1" { + security_groups = ["${test_instance.other.*.id}"] +} + +resource "test_instance" "bad2" { + security_groups = ["${var.listy}"] +} + +resource "test_instance" "bad3" { + security_groups = ["${module.foo.outputs_always_dynamic}"] +} + +resource "test_instance" "bad4" { + security_groups = ["${list("a", "b", "c")}"] +} + +# The rest of these should keep the same amount of list-ness + +resource "test_instance" "ok1" { + security_groups = [] +} + +resource "test_instance" "ok2" { + security_groups = ["notalist"] +} + +resource "test_instance" "ok3" { + security_groups = ["${path.module}"] +} + +resource "test_instance" "ok4" { + security_groups = [["foo"], ["bar"]] +} + +resource "test_instance" "ok5" { + security_groups = "${test_instance.other.*.id}" +} + +resource "test_instance" "ok6" { + security_groups = [ + "${test_instance.other1.*.id}", + "${test_instance.other2.*.id}", + ] +} diff --git a/configs/configupgrade/test-fixtures/valid/redundant-list/want/redundant-list.tf b/configs/configupgrade/test-fixtures/valid/redundant-list/want/redundant-list.tf new file mode 100644 index 000000000..34724b978 --- /dev/null +++ b/configs/configupgrade/test-fixtures/valid/redundant-list/want/redundant-list.tf @@ -0,0 +1,60 @@ +variable "listy" { + type = list(string) +} + +resource "test_instance" "other" { + count = 2 +} + +resource "test_instance" "bad1" { + security_groups = test_instance.other.*.id +} + +resource "test_instance" "bad2" { + security_groups = var.listy +} + +resource "test_instance" "bad3" { + # TF-UPGRADE-TODO: In Terraform v0.10 and earlier, it was sometimes necessary to + # force an interpolation expression to be interpreted as a list by wrapping it + # in an extra set of list brackets. That form was supported for compatibilty in + # v0.11, but is no longer supported in Terraform v0.12. + # + # If the expression in the following list itself returns a list, remove the + # brackets to avoid interpretation as a list of lists. If the expression + # returns a single list item then leave it as-is and remove this TODO comment. + security_groups = [module.foo.outputs_always_dynamic] +} + +resource "test_instance" "bad4" { + security_groups = ["a", "b", "c"] +} + +# The rest of these should keep the same amount of list-ness + +resource "test_instance" "ok1" { + security_groups = [] +} + +resource "test_instance" "ok2" { + security_groups = ["notalist"] +} + +resource "test_instance" "ok3" { + security_groups = [path.module] +} + +resource "test_instance" "ok4" { + security_groups = [["foo"], ["bar"]] +} + +resource "test_instance" "ok5" { + security_groups = test_instance.other.*.id +} + +resource "test_instance" "ok6" { + security_groups = [ + test_instance.other1.*.id, + test_instance.other2.*.id, + ] +} diff --git a/configs/configupgrade/test-fixtures/valid/redundant-list/want/versions.tf b/configs/configupgrade/test-fixtures/valid/redundant-list/want/versions.tf new file mode 100644 index 000000000..d9b6f790b --- /dev/null +++ b/configs/configupgrade/test-fixtures/valid/redundant-list/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 39bb48a55..e91dc4318 100644 --- a/configs/configupgrade/upgrade_body.go +++ b/configs/configupgrade/upgrade_body.go @@ -153,7 +153,7 @@ func dependsOnAttributeRule(filename string, an *analysis) bodyItemRule { } } -func attributeRule(filename string, wantTy cty.Type, an *analysis, upgradeExpr func(val interface{}) ([]byte, tfdiags.Diagnostics)) bodyItemRule { +func attributeRule(filename string, wantTy cty.Type, an *analysis, exprRule func(val interface{}) ([]byte, tfdiags.Diagnostics)) bodyItemRule { return func(buf *bytes.Buffer, blockAddr string, item *hcl1ast.ObjectItem) tfdiags.Diagnostics { var diags tfdiags.Diagnostics @@ -174,7 +174,77 @@ func attributeRule(filename string, wantTy cty.Type, an *analysis, upgradeExpr f return diags } - valSrc, valDiags := upgradeExpr(item.Val) + val := item.Val + + if typeIsSettableFromTupleCons(wantTy) && !typeIsSettableFromTupleCons(wantTy.ElementType()) { + // In Terraform circa 0.10 it was required to wrap any expression + // that produces a list in HCL list brackets to allow type analysis + // to complete successfully, even though logically that ought to + // have produced a list of lists. + // + // In Terraform 0.12 this construct _does_ produce a list of lists, so + // we need to update expressions that look like older usage. We can't + // do this exactly with static analysis, but we can make a best-effort + // and produce a warning if type inference is impossible for a + // particular expression. This should give good results for common + // simple examples, like splat expressions. + // + // There are four possible cases here: + // - The value isn't an HCL1 list expression at all, or is one that + // contains more than one item, in which case this special case + // does not apply. + // - The inner expression after upgrading can be proven to return + // a sequence type, in which case we must definitely remove + // the wrapping brackets. + // - The inner expression after upgrading can be proven to return + // a non-sequence type, in which case we fall through and treat + // the whole item like a normal expression. + // - Static type analysis is impossible (it returns cty.DynamicPseudoType), + // in which case we will make no changes but emit a warning and + // a TODO comment for the user to decide whether a change needs + // to be made in practice. + if list, ok := val.(*hcl1ast.ListType); ok { + if len(list.List) == 1 { + maybeAlsoList := list.List[0] + if exprSrc, diags := upgradeExpr(maybeAlsoList, filename, true, an); !diags.HasErrors() { + // Ideally we would set "self" here but we don't have + // enough context to set it and in practice not setting + // it only affects expressions inside provisioner and + // connection blocks, and the list-wrapping thing isn't + // common there. + gotTy := an.InferExpressionType(exprSrc, nil) + if typeIsSettableFromTupleCons(gotTy) { + // Since this expression was already inside HCL list brackets, + // the ultimate result would be a list of lists and so we + // need to unwrap it by taking just the portion within + // the brackets here. + val = maybeAlsoList + } + if gotTy == cty.DynamicPseudoType { + // User must decide. + diags = diags.Append(&hcl2.Diagnostic{ + Severity: hcl2.DiagError, + Summary: "Possible legacy dynamic list usage", + Detail: "This list may be using the legacy redundant-list expression style from Terraform v0.10 and earlier. If the expression within these brackets returns a list itself, remove these brackets.", + Subject: hcl1PosRange(filename, list.Lbrack).Ptr(), + }) + buf.WriteString( + "# TF-UPGRADE-TODO: In Terraform v0.10 and earlier, it was sometimes necessary to\n" + + "# force an interpolation expression to be interpreted as a list by wrapping it\n" + + "# in an extra set of list brackets. That form was supported for compatibilty in\n" + + "# v0.11, but is no longer supported in Terraform v0.12.\n" + + "#\n" + + "# If the expression in the following list itself returns a list, remove the\n" + + "# brackets to avoid interpretation as a list of lists. If the expression\n" + + "# returns a single list item then leave it as-is and remove this TODO comment.\n", + ) + } + } + } + } + } + + valSrc, valDiags := exprRule(val) diags = diags.Append(valDiags) printAttribute(buf, item.Keys[0].Token.Value().(string), valSrc, item.LineComment) diff --git a/configs/configupgrade/upgrade_expr.go b/configs/configupgrade/upgrade_expr.go index fb4464731..062500585 100644 --- a/configs/configupgrade/upgrade_expr.go +++ b/configs/configupgrade/upgrade_expr.go @@ -9,6 +9,7 @@ import ( hcl2 "github.com/hashicorp/hcl2/hcl" hcl2syntax "github.com/hashicorp/hcl2/hcl/hclsyntax" + "github.com/zclconf/go-cty/cty" hcl1ast "github.com/hashicorp/hcl/hcl/ast" hcl1printer "github.com/hashicorp/hcl/hcl/printer" @@ -546,3 +547,7 @@ func upgradeTerraformRemoteStateTraversalParts(parts []string, an *analysis) []s copy(newParts[attrIdx+1:], parts[attrIdx:]) return newParts } + +func typeIsSettableFromTupleCons(ty cty.Type) bool { + return ty.IsListType() || ty.IsTupleType() || ty.IsSetType() +} diff --git a/configs/configupgrade/upgrade_test.go b/configs/configupgrade/upgrade_test.go index 199c8a69e..92ca69f66 100644 --- a/configs/configupgrade/upgrade_test.go +++ b/configs/configupgrade/upgrade_test.go @@ -190,10 +190,11 @@ var testProviders = map[string]providers.Factory{ ResourceTypes: map[string]*configschema.Block{ "test_instance": { Attributes: map[string]*configschema.Attribute{ - "id": {Type: cty.String, Computed: true}, - "type": {Type: cty.String, Optional: true}, - "image": {Type: cty.String, Optional: true}, - "tags": {Type: cty.Map(cty.String), Optional: true}, + "id": {Type: cty.String, Computed: true}, + "type": {Type: cty.String, Optional: true}, + "image": {Type: cty.String, Optional: true}, + "tags": {Type: cty.Map(cty.String), Optional: true}, + "security_groups": {Type: cty.List(cty.String), Optional: true}, }, BlockTypes: map[string]*configschema.NestedBlock{ "network": {