From ceedeb69a9329165cd7e07f981215c1f4d8a660a Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Sat, 1 Dec 2018 10:28:34 -0800 Subject: [PATCH] configs/configupgrade: Normalize and upgrade reference expressions The reference syntax is not significantly changed, but there are some minor additional restrictions on identifiers in HCL2 and as a special case we need to rewrite references to data.terraform_remote_state . Along with those mandatory upgrades, we will also switch references to using normal index syntax where it's safe to do so, as part of de-emphasizing the old strange integer attribute syntax (like foo.0.bar). --- .../valid/traversals/input/traversals.tf | 23 ++++ .../valid/traversals/want/traversals.tf | 23 ++++ .../valid/traversals/want/versions.tf | 3 + configs/configupgrade/upgrade_expr.go | 106 +++++++++++++++++- configs/configupgrade/upgrade_test.go | 16 +++ 5 files changed, 170 insertions(+), 1 deletion(-) create mode 100644 configs/configupgrade/test-fixtures/valid/traversals/input/traversals.tf create mode 100644 configs/configupgrade/test-fixtures/valid/traversals/want/traversals.tf create mode 100644 configs/configupgrade/test-fixtures/valid/traversals/want/versions.tf diff --git a/configs/configupgrade/test-fixtures/valid/traversals/input/traversals.tf b/configs/configupgrade/test-fixtures/valid/traversals/input/traversals.tf new file mode 100644 index 000000000..d7fb04585 --- /dev/null +++ b/configs/configupgrade/test-fixtures/valid/traversals/input/traversals.tf @@ -0,0 +1,23 @@ +locals { + simple = "${test_instance.foo.bar}" + splat = "${test_instance.foo.*.bar}" + index = "${test_instance.foo.1.bar}" + + after_simple = "${test_instance.foo.bar.0.baz}" + after_splat = "${test_instance.foo.*.bar.0.baz}" + after_index = "${test_instance.foo.1.bar.2.baz}" + + non_ident_attr = "${test_instance.foo.bar.1baz}" + + remote_state_output = "${data.terraform_remote_state.foo.bar}" + remote_state_attr = "${data.terraform_remote_state.foo.backend}" + remote_state_idx_output = "${data.terraform_remote_state.foo.1.bar}" + remote_state_idx_attr = "${data.terraform_remote_state.foo.1.backend}" + remote_state_splat_output = "${data.terraform_remote_state.foo.*.bar}" + remote_state_splat_attr = "${data.terraform_remote_state.foo.*.backend}" +} + +data "terraform_remote_state" "foo" { + # This is just here to make sure the schema for this gets loaded to + # support the remote_state_* checks above. +} diff --git a/configs/configupgrade/test-fixtures/valid/traversals/want/traversals.tf b/configs/configupgrade/test-fixtures/valid/traversals/want/traversals.tf new file mode 100644 index 000000000..e5ffbaf73 --- /dev/null +++ b/configs/configupgrade/test-fixtures/valid/traversals/want/traversals.tf @@ -0,0 +1,23 @@ +locals { + simple = test_instance.foo.bar + splat = test_instance.foo.*.bar + index = test_instance.foo[1].bar + + after_simple = test_instance.foo.bar[0].baz + after_splat = test_instance.foo.*.bar.0.baz + after_index = test_instance.foo[1].bar[2].baz + + non_ident_attr = test_instance.foo.bar["1baz"] + + remote_state_output = data.terraform_remote_state.foo.outputs.bar + remote_state_attr = data.terraform_remote_state.foo.backend + remote_state_idx_output = data.terraform_remote_state.foo[1].outputs.bar + remote_state_idx_attr = data.terraform_remote_state.foo[1].backend + remote_state_splat_output = data.terraform_remote_state.foo.*.outputs.bar + remote_state_splat_attr = data.terraform_remote_state.foo.*.backend +} + +data "terraform_remote_state" "foo" { + # This is just here to make sure the schema for this gets loaded to + # support the remote_state_* checks above. +} diff --git a/configs/configupgrade/test-fixtures/valid/traversals/want/versions.tf b/configs/configupgrade/test-fixtures/valid/traversals/want/versions.tf new file mode 100644 index 000000000..d9b6f790b --- /dev/null +++ b/configs/configupgrade/test-fixtures/valid/traversals/want/versions.tf @@ -0,0 +1,3 @@ +terraform { + required_version = ">= 0.12" +} diff --git a/configs/configupgrade/upgrade_expr.go b/configs/configupgrade/upgrade_expr.go index c25245fef..a2e02361c 100644 --- a/configs/configupgrade/upgrade_expr.go +++ b/configs/configupgrade/upgrade_expr.go @@ -5,8 +5,10 @@ import ( "fmt" "log" "strconv" + "strings" hcl2 "github.com/hashicorp/hcl2/hcl" + hcl2syntax "github.com/hashicorp/hcl2/hcl/hclsyntax" hcl1ast "github.com/hashicorp/hcl/hcl/ast" hcl1printer "github.com/hashicorp/hcl/hcl/printer" @@ -15,6 +17,8 @@ import ( "github.com/hashicorp/hil" hilast "github.com/hashicorp/hil/ast" + "github.com/hashicorp/terraform/addrs" + "github.com/hashicorp/terraform/configs/configschema" "github.com/hashicorp/terraform/tfdiags" ) @@ -142,7 +146,50 @@ Value: } case *hilast.VariableAccess: - buf.WriteString(tv.Name) + // In HIL a variable access is just a single string which might contain + // a mixture of identifiers, dots, integer indices, and splat expressions. + // All of these concepts were formerly interpreted by Terraform itself, + // rather than by HIL. We're going to process this one chunk at a time + // here so we can normalize and introduce some newer syntax where it's + // safe to do so. + parts := strings.Split(tv.Name, ".") + parts = upgradeTraversalParts(parts, an) // might add/remove/change parts + first, remain := parts[0], parts[1:] + buf.WriteString(first) + seenSplat := false + for _, part := range remain { + if part == "*" { + seenSplat = true + buf.WriteString(".*") + continue + } + + // Other special cases apply only if we've not previously + // seen a splat expression marker, since attribute vs. index + // syntax have different interpretations after a simple splat. + if !seenSplat { + if v, err := strconv.Atoi(part); err == nil { + // Looks like it's old-style index traversal syntax foo.0.bar + // so we'll replace with canonical index syntax foo[0].bar. + fmt.Fprintf(&buf, "[%d]", v) + continue + } + if !hcl2syntax.ValidIdentifier(part) { + // This should be rare since HIL's identifier syntax is _close_ + // to HCL2's, but we'll get here if one of the intervening + // parts is not a valid identifier in isolation, since HIL + // did not consider these to be separate identifiers. + // e.g. foo.1bar would be invalid in HCL2; must instead be foo["1bar"]. + buf.WriteByte('[') + printQuotedString(&buf, part) + buf.WriteByte(']') + continue + } + } + + buf.WriteByte('.') + buf.WriteString(part) + } case *hilast.Arithmetic: op, exists := hilArithmeticOpSyms[tv.Op] @@ -362,3 +409,60 @@ var hilArithmeticOpSyms = map[hilast.ArithmeticOp]string{ hilast.ArithmeticOpGreaterThan: " > ", hilast.ArithmeticOpGreaterThanOrEqual: " >= ", } + +// upgradeTraversalParts might alter the given split parts from a HIL-style +// variable access to account for renamings made in Terraform v0.12. +func upgradeTraversalParts(parts []string, an *analysis) []string { + // For now this just deals with data.terraform_remote_state + return upgradeTerraformRemoteStateTraversalParts(parts, an) +} + +func upgradeTerraformRemoteStateTraversalParts(parts []string, an *analysis) []string { + // data.terraform_remote_state.x.foo needs to become + // data.terraform_remote_state.x.outputs.foo unless "foo" is a real + // attribute in the object type implied by the remote state schema. + if len(parts) < 4 { + return parts + } + if parts[0] != "data" || parts[1] != "terraform_remote_state" { + return parts + } + + attrIdx := 3 + if parts[attrIdx] == "*" { + attrIdx = 4 // data.terraform_remote_state.x.*.foo + } else if _, err := strconv.Atoi(parts[attrIdx]); err == nil { + attrIdx = 4 // data.terraform_remote_state.x.1.foo + } + if attrIdx >= len(parts) { + return parts + } + + attrName := parts[attrIdx] + + // Now we'll use the schema of data.terraform_remote_state to decide if + // the user intended this to be an output, or whether it's one of the real + // attributes of this data source. + var schema *configschema.Block + if providerSchema := an.ProviderSchemas["terraform"]; providerSchema != nil { + schema, _ = providerSchema.SchemaForResourceType(addrs.DataResourceMode, "terraform_remote_state") + } + // Schema should be available in all reasonable cases, but might be nil + // if input configuration contains a reference to a remote state data resource + // without actually defining that data resource. In that weird edge case, + // we'll just assume all attributes are outputs. + if schema != nil && schema.ImpliedType().HasAttribute(attrName) { + // User is accessing one of the real attributes, then, and we have + // no need to rewrite it. + return parts + } + + // If we get down here then our task is to produce a new parts slice + // that has the fixed additional attribute name "outputs" inserted at + // attrIdx, retaining all other parts. + newParts := make([]string, len(parts)+1) + copy(newParts, parts[:attrIdx]) + newParts[attrIdx] = "outputs" + copy(newParts[attrIdx+1:], parts[attrIdx:]) + return newParts +} diff --git a/configs/configupgrade/upgrade_test.go b/configs/configupgrade/upgrade_test.go index 1ef467e57..199c8a69e 100644 --- a/configs/configupgrade/upgrade_test.go +++ b/configs/configupgrade/upgrade_test.go @@ -230,6 +230,22 @@ var testProviders = map[string]providers.Factory{ } return p, nil }), + "terraform": providers.Factory(func() (providers.Interface, error) { + p := &terraform.MockProvider{} + p.GetSchemaReturn = &terraform.ProviderSchema{ + DataSources: map[string]*configschema.Block{ + "terraform_remote_state": { + // This is just enough an approximation of the remote state + // schema to check out reference upgrade logic. It is + // intentionally not fully-comprehensive. + Attributes: map[string]*configschema.Attribute{ + "backend": {Type: cty.String, Optional: true}, + }, + }, + }, + } + return p, nil + }), } func init() {