Merge pull request #21103 from hashicorp/configuograde-splat-idx

configupgrade: Upgrade indexing of splat syntax

configs/configupgrade/test-fixtures/valid/indexed-splat/input/indexed-splat.tf

@@ -0,0 +1,16 @@
+resource "test_instance" "first_many" {
+  count = 2
+}
+
+resource "test_instance" "one" {
+  image = "${test_instance.first_many.*.id[0]}"
+}
+
+resource "test_instance" "splat_of_one" {
+  image = "${test_instance.one.*.id[0]}"
+}
+
+resource "test_instance" "second_many" {
+  count = "${length(test_instance.first_many)}"
+  security_groups = "${test_instance.first_many.*.id[count.index]}"
+}

configs/configupgrade/test-fixtures/valid/indexed-splat/want/indexed-splat.tf

@@ -0,0 +1,16 @@
+resource "test_instance" "first_many" {
+  count = 2
+}
+
+resource "test_instance" "one" {
+  image = test_instance.first_many[0].id
+}
+
+resource "test_instance" "splat_of_one" {
+  image = test_instance.one.*.id[0]
+}
+
+resource "test_instance" "second_many" {
+  count           = length(test_instance.first_many)
+  security_groups = test_instance.first_many[count.index].id
+}

configs/configupgrade/test-fixtures/valid/indexed-splat/want/versions.tf

@@ -0,0 +1,3 @@
+terraform {
+  required_version = ">= 0.12"
+}

configs/configupgrade/upgrade_expr.go

@@ -248,81 +248,21 @@ Value:
 		// safe to do so.
 		parts := strings.Split(tv.Name, ".")
 
-		// First we need to deal with the .count pseudo-attributes that 0.11 and
-		// prior allowed for resources. These no longer exist, because they
-		// don't do anything we can't do with the length(...) function.
-		if len(parts) > 0 {
-			var rAddr addrs.Resource
-			switch parts[0] {
-			case "data":
-				if len(parts) == 4 && parts[3] == "count" {
-					rAddr.Mode = addrs.DataResourceMode
-					rAddr.Type = parts[1]
-					rAddr.Name = parts[2]
-				}
-			default:
-				if len(parts) == 3 && parts[2] == "count" {
-					rAddr.Mode = addrs.ManagedResourceMode
-					rAddr.Type = parts[0]
-					rAddr.Name = parts[1]
-				}
-			}
-
-			// We need to check if the thing being referenced is actually an
-			// existing resource, because other three-part traversals might
-			// coincidentally end with "count".
-			if hasCount, exists := an.ResourceHasCount[rAddr]; exists {
-				if hasCount {
-					buf.WriteString("length(")
-					buf.WriteString(rAddr.String())
-					buf.WriteString(")")
-				} else {
-					// If the resource does not have count, the .count
-					// attr would've always returned 1 before.
-					buf.WriteString("1")
-				}
-				break Value
-			}
+		transformed := transformCountPseudoAttribute(&buf, parts, an)
+		if transformed {
+			break Value
 		}
 
 		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)
+		vDiags := validateHilAddress(tv.Name, filename)
+		if len(vDiags) > 0 {
+			diags = diags.Append(vDiags)
+			break
 		}
 
+		printHilTraversalPartsAsHcl2(&buf, parts)
+
 	case *hilast.Arithmetic:
 		op, exists := hilArithmeticOpSyms[tv.Op]
 		if !exists {
@@ -553,14 +493,74 @@ Value:
 		buf.Write(falseSrc)
 
 	case *hilast.Index:
-		targetSrc, exprDiags := upgradeExpr(tv.Target, filename, true, an)
-		diags = diags.Append(exprDiags)
+		target, ok := tv.Target.(*hilast.VariableAccess)
+		if !ok {
+			panic(fmt.Sprintf("Index node with unsupported target type (%T)", tv.Target))
+		}
+		parts := strings.Split(target.Name, ".")
+
 		keySrc, exprDiags := upgradeExpr(tv.Key, filename, true, an)
 		diags = diags.Append(exprDiags)
-		buf.Write(targetSrc)
-		buf.WriteString("[")
-		buf.Write(keySrc)
-		buf.WriteString("]")
+
+		transformed := transformCountPseudoAttribute(&buf, parts, an)
+		if transformed {
+			break Value
+		}
+
+		parts = upgradeTraversalParts(parts, an) // might add/remove/change parts
+
+		vDiags := validateHilAddress(target.Name, filename)
+		if len(vDiags) > 0 {
+			diags = diags.Append(vDiags)
+			break
+		}
+
+		first, remain := parts[0], parts[1:]
+
+		var rAddr addrs.Resource
+		switch parts[0] {
+		case "data":
+			if len(parts) == 5 && parts[3] == "*" {
+				rAddr.Mode = addrs.DataResourceMode
+				rAddr.Type = parts[1]
+				rAddr.Name = parts[2]
+			}
+		default:
+			if len(parts) == 4 && parts[2] == "*" {
+				rAddr.Mode = addrs.ManagedResourceMode
+				rAddr.Type = parts[0]
+				rAddr.Name = parts[1]
+			}
+		}
+
+		// We need to check if the thing being referenced has count
+		// to retain backward compatibility
+		hasCount := false
+		if v, exists := an.ResourceHasCount[rAddr]; exists {
+			hasCount = v
+		}
+
+		hasSplat := false
+
+		buf.WriteString(first)
+		for _, part := range remain {
+			// Attempt to convert old-style splat indexing to new one
+			// e.g. res.label.*.attr[idx] to res.label[idx].attr
+			if part == "*" && hasCount {
+				hasSplat = true
+				buf.WriteString(fmt.Sprintf("[%s]", keySrc))
+				continue
+			}
+
+			buf.WriteByte('.')
+			buf.WriteString(part)
+		}
+
+		if !hasSplat {
+			buf.WriteString("[")
+			buf.Write(keySrc)
+			buf.WriteString("]")
+		}
 
 	case *hilast.Output:
 		if len(tv.Exprs) == 1 {
@@ -614,6 +614,122 @@ Value:
 	return buf.Bytes(), diags
 }
 
+func validateHilAddress(address, filename string) tfdiags.Diagnostics {
+	parts := strings.Split(address, ".")
+	var diags tfdiags.Diagnostics
+
+	label, ok := getResourceLabel(parts)
+	if ok && !hcl2syntax.ValidIdentifier(label) {
+		// We can't get any useful source location out of HIL unfortunately
+		diags = diags.Append(tfdiags.Sourceless(
+			tfdiags.Error,
+			fmt.Sprintf("Invalid address (%s) in ./%s", address, filename),
+			// The label could be invalid for another reason
+			// but this is the most likely, so we add it as hint
+			"Names of objects (resources, modules, etc) may no longer start with digits."))
+	}
+
+	return diags
+}
+
+func getResourceLabel(parts []string) (string, bool) {
+	if len(parts) < 1 {
+		return "", false
+	}
+
+	if parts[0] == "data" {
+		if len(parts) < 3 {
+			return "", false
+		}
+		return parts[2], true
+	}
+
+	if len(parts) < 2 {
+		return "", false
+	}
+
+	return parts[1], true
+}
+
+// transformCountPseudoAttribute deals with the .count pseudo-attributes
+// that 0.11 and prior allowed for resources. These no longer exist,
+// because they don't do anything we can't do with the length(...) function.
+func transformCountPseudoAttribute(buf *bytes.Buffer, parts []string, an *analysis) (transformed bool) {
+	if len(parts) > 0 {
+		var rAddr addrs.Resource
+		switch parts[0] {
+		case "data":
+			if len(parts) == 4 && parts[3] == "count" {
+				rAddr.Mode = addrs.DataResourceMode
+				rAddr.Type = parts[1]
+				rAddr.Name = parts[2]
+			}
+		default:
+			if len(parts) == 3 && parts[2] == "count" {
+				rAddr.Mode = addrs.ManagedResourceMode
+				rAddr.Type = parts[0]
+				rAddr.Name = parts[1]
+			}
+		}
+		// We need to check if the thing being referenced is actually an
+		// existing resource, because other three-part traversals might
+		// coincidentally end with "count".
+		if hasCount, exists := an.ResourceHasCount[rAddr]; exists {
+			if hasCount {
+				buf.WriteString("length(")
+				buf.WriteString(rAddr.String())
+				buf.WriteString(")")
+			} else {
+				// If the resource does not have count, the .count
+				// attr would've always returned 1 before.
+				buf.WriteString("1")
+			}
+			transformed = true
+			return
+		}
+	}
+	return
+}
+
+func printHilTraversalPartsAsHcl2(buf *bytes.Buffer, parts []string) {
+	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)
+	}
+}
+
 func upgradeHeredocBody(buf *bytes.Buffer, val *hilast.Output, filename string, an *analysis) tfdiags.Diagnostics {
 	var diags tfdiags.Diagnostics
 

configs/configupgrade/upgrade_test.go

@@ -2,6 +2,7 @@ package configupgrade
 
 import (
 	"bytes"
+	"flag"
 	"io"
 	"io/ioutil"
 	"log"
@@ -286,6 +287,7 @@ func init() {
 }
 
 func TestMain(m *testing.M) {
+	flag.Parse()
 	if testing.Verbose() {
 		// if we're verbose, use the logging requested by TF_LOG
 		logging.SetOutput()