From 8490fc36f733ed428a7ad244b567914235be675f Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Sat, 1 Dec 2018 12:03:16 -0800 Subject: [PATCH] configs/configupgrade: Fix up references to counted/uncounted resources Prior to v0.12 Terraform was liberal about these and allowed them to mismatch, but now it's important to get this right so that resources and resource instances can be used directly as object values, and so we'll fix up any sloppy existing references so things keep working as expected. This is particularly important for the pattern of using count to create conditional resources, since previously the "true" case would create one instance and Terraform would accept an unindexed reference to that. --- configs/configupgrade/analysis.go | 2 + .../valid/traversals/input/traversals.tf | 18 +++++ .../valid/traversals/want/traversals.tf | 18 +++++ configs/configupgrade/upgrade_expr.go | 72 ++++++++++++++++++- configs/configupgrade/upgrade_native.go | 1 + 5 files changed, 109 insertions(+), 2 deletions(-) diff --git a/configs/configupgrade/analysis.go b/configs/configupgrade/analysis.go index 7d16cb7082..9eb93f5ac9 100644 --- a/configs/configupgrade/analysis.go +++ b/configs/configupgrade/analysis.go @@ -162,6 +162,8 @@ func (u *Upgrader) analyze(ms ModuleSources) (*analysis, error) { if o := listVal.Filter("count"); len(o.Items) > 0 { ret.ResourceHasCount[rAddr] = true + } else { + ret.ResourceHasCount[rAddr] = false } var providerKey string diff --git a/configs/configupgrade/test-fixtures/valid/traversals/input/traversals.tf b/configs/configupgrade/test-fixtures/valid/traversals/input/traversals.tf index d7fb045856..ec2aa2ff51 100644 --- a/configs/configupgrade/test-fixtures/valid/traversals/input/traversals.tf +++ b/configs/configupgrade/test-fixtures/valid/traversals/input/traversals.tf @@ -15,9 +15,27 @@ locals { 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}" + + has_index_should = "${test_instance.b.0.id}" + has_index_shouldnt = "${test_instance.c.0.id}" + no_index_should = "${test_instance.a.id}" + no_index_shouldnt = "${test_instance.c.id}" + + has_index_shouldnt_data = "${data.terraform_remote_state.foo.0.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. } + +resource "test_instance" "a" { + count = 1 +} + +resource "test_instance" "b" { + count = "${var.count}" +} + +resource "test_instance" "c" { +} diff --git a/configs/configupgrade/test-fixtures/valid/traversals/want/traversals.tf b/configs/configupgrade/test-fixtures/valid/traversals/want/traversals.tf index e5ffbaf73f..80b24de213 100644 --- a/configs/configupgrade/test-fixtures/valid/traversals/want/traversals.tf +++ b/configs/configupgrade/test-fixtures/valid/traversals/want/traversals.tf @@ -15,9 +15,27 @@ locals { 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 + + has_index_should = test_instance.b[0].id + has_index_shouldnt = test_instance.c.id + no_index_should = test_instance.a[0].id + no_index_shouldnt = test_instance.c.id + + has_index_shouldnt_data = 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. } + +resource "test_instance" "a" { + count = 1 +} + +resource "test_instance" "b" { + count = var.count +} + +resource "test_instance" "c" { +} diff --git a/configs/configupgrade/upgrade_expr.go b/configs/configupgrade/upgrade_expr.go index a2e02361c5..770a264115 100644 --- a/configs/configupgrade/upgrade_expr.go +++ b/configs/configupgrade/upgrade_expr.go @@ -413,8 +413,76 @@ var hilArithmeticOpSyms = map[hilast.ArithmeticOp]string{ // 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) + parts = upgradeCountTraversalParts(parts, an) + parts = upgradeTerraformRemoteStateTraversalParts(parts, an) + return parts +} + +func upgradeCountTraversalParts(parts []string, an *analysis) []string { + // test_instance.foo.id needs to become test_instance.foo[0].id if + // count is set for test_instance.foo. Likewise, if count _isn't_ set + // then test_instance.foo.0.id must become test_instance.foo.id. + if len(parts) < 3 { + return parts + } + var addr addrs.Resource + var idxIdx int + switch parts[0] { + case "data": + addr.Mode = addrs.DataResourceMode + addr.Type = parts[1] + addr.Name = parts[2] + idxIdx = 3 + default: + addr.Mode = addrs.ManagedResourceMode + addr.Type = parts[0] + addr.Name = parts[1] + idxIdx = 2 + } + + hasCount, exists := an.ResourceHasCount[addr] + if !exists { + // Probably not actually a resource instance at all, then. + return parts + } + + // Since at least one attribute is required after a resource reference + // prior to Terraform v0.12, we can assume there will be at least enough + // parts to contain the index even if no index is actually present. + if idxIdx >= len(parts) { + return parts + } + + maybeIdx := parts[idxIdx] + switch { + case hasCount: + if _, err := strconv.Atoi(maybeIdx); err == nil || maybeIdx == "*" { + // Has an index already, so no changes required. + return parts + } + // Need to insert index zero at idxIdx. + log.Printf("[TRACE] configupgrade: %s has count but reference does not have index, so adding one", addr) + newParts := make([]string, len(parts)+1) + copy(newParts, parts[:idxIdx]) + newParts[idxIdx] = "0" + copy(newParts[idxIdx+1:], parts[idxIdx:]) + return newParts + default: + // For removing indexes we'll be more conservative and only remove + // exactly index "0", because other indexes on a resource without + // count are invalid anyway and we're better off letting the normal + // configuration parser deal with that. + if maybeIdx != "0" { + return parts + } + + // Need to remove the index zero. + log.Printf("[TRACE] configupgrade: %s does not have count but reference has index, so removing it", addr) + newParts := make([]string, len(parts)-1) + copy(newParts, parts[:idxIdx]) + copy(newParts[idxIdx:], parts[idxIdx+1:]) + return newParts + } } func upgradeTerraformRemoteStateTraversalParts(parts []string, an *analysis) []string { diff --git a/configs/configupgrade/upgrade_native.go b/configs/configupgrade/upgrade_native.go index b225bc5d1c..cb4ffa8e96 100644 --- a/configs/configupgrade/upgrade_native.go +++ b/configs/configupgrade/upgrade_native.go @@ -309,6 +309,7 @@ func (u *Upgrader) upgradeNativeSyntaxResource(filename string, buf *bytes.Buffe labels := []string{addr.Type, addr.Name} rules := schemaDefaultBodyRules(filename, schema, an, adhocComments) + rules["count"] = normalAttributeRule(filename, cty.Number, an) printComments(buf, item.LeadComment) printBlockOpen(buf, blockType, labels, item.LineComment)