From 154911688a74bc8afe58f22e49f37a97b6933c12 Mon Sep 17 00:00:00 2001
From: Martin Atkins <mart@degeneration.co.uk>
Date: Wed, 20 Feb 2019 11:53:56 -0800
Subject: [PATCH] configs/configupgrade: upgrade expressions inside heredocs

HEREDOC tokens are a little more fussy than normal string sequences
because we need to preserve the whitespace within them along with the
start and end markers while we upgrade any interpolated expressions inside.

We need to do some work locally here because the HCL heredoc processing
"does too much" and throws away information we need to do a faithful
upgrade.

We also need to contend with the fact that Terraform <=0.11 had an older
version of HCL that accidentally permitted a degenerate form of heredoc
where the marker was at the end of the final line, like this:

    degenerate = <<EOT
    this should never have workedEOT

When we migrate this, we'll introduce the additional newline that is now
required, which will unfortunately slightly change the result string to
include a newline when parsed by 0.12, and so we'll need to call this out
as a caveat in the upgrade guide.
---
 .../valid/heredoc-flush/input/heredoc.tf      | 12 +++
 .../valid/heredoc-flush/want/heredoc.tf       | 15 +++
 .../want/versions.tf                          |  0
 .../valid/heredoc-no-interp/input/heredoc.tf  |  6 ++
 .../valid/heredoc-no-interp/want/heredoc.tf   |  6 ++
 .../valid/heredoc-no-interp/want/versions.tf  |  3 +
 .../input/heredoc.tf                          |  0
 .../want/heredoc.tf                           |  3 +-
 .../valid/heredoc-sameline/want/versions.tf   |  3 +
 .../valid/heredoc-standard/input/heredoc.tf   | 12 +++
 .../valid/heredoc-standard/want/heredoc.tf    | 16 +++
 .../valid/heredoc-standard/want/versions.tf   |  3 +
 configs/configupgrade/upgrade_expr.go         | 98 +++++++++++++++++--
 configs/configupgrade/upgrade_native.go       |  6 ++
 14 files changed, 176 insertions(+), 7 deletions(-)
 create mode 100644 configs/configupgrade/test-fixtures/valid/heredoc-flush/input/heredoc.tf
 create mode 100644 configs/configupgrade/test-fixtures/valid/heredoc-flush/want/heredoc.tf
 rename configs/configupgrade/test-fixtures/valid/{heredoc => heredoc-flush}/want/versions.tf (100%)
 create mode 100644 configs/configupgrade/test-fixtures/valid/heredoc-no-interp/input/heredoc.tf
 create mode 100644 configs/configupgrade/test-fixtures/valid/heredoc-no-interp/want/heredoc.tf
 create mode 100644 configs/configupgrade/test-fixtures/valid/heredoc-no-interp/want/versions.tf
 rename configs/configupgrade/test-fixtures/valid/{heredoc => heredoc-sameline}/input/heredoc.tf (100%)
 rename configs/configupgrade/test-fixtures/valid/{heredoc => heredoc-sameline}/want/heredoc.tf (82%)
 create mode 100644 configs/configupgrade/test-fixtures/valid/heredoc-sameline/want/versions.tf
 create mode 100644 configs/configupgrade/test-fixtures/valid/heredoc-standard/input/heredoc.tf
 create mode 100644 configs/configupgrade/test-fixtures/valid/heredoc-standard/want/heredoc.tf
 create mode 100644 configs/configupgrade/test-fixtures/valid/heredoc-standard/want/versions.tf

diff --git a/configs/configupgrade/test-fixtures/valid/heredoc-flush/input/heredoc.tf b/configs/configupgrade/test-fixtures/valid/heredoc-flush/input/heredoc.tf
new file mode 100644
index 000000000..918bbb66b
--- /dev/null
+++ b/configs/configupgrade/test-fixtures/valid/heredoc-flush/input/heredoc.tf
@@ -0,0 +1,12 @@
+locals {
+  baz          = { "greeting" = "hello" }
+  cert_options = <<-EOF
+    A
+    B ${lookup(local.baz, "greeting")}
+    C
+  EOF
+}
+
+output "local" {
+  value = "${local.cert_options}"
+}
diff --git a/configs/configupgrade/test-fixtures/valid/heredoc-flush/want/heredoc.tf b/configs/configupgrade/test-fixtures/valid/heredoc-flush/want/heredoc.tf
new file mode 100644
index 000000000..4ffcc6e44
--- /dev/null
+++ b/configs/configupgrade/test-fixtures/valid/heredoc-flush/want/heredoc.tf
@@ -0,0 +1,15 @@
+locals {
+  baz = {
+    "greeting" = "hello"
+  }
+  cert_options = <<-EOF
+    A
+    B ${local.baz["greeting"]}
+    C
+EOF
+
+}
+
+output "local" {
+value = local.cert_options
+}
diff --git a/configs/configupgrade/test-fixtures/valid/heredoc/want/versions.tf b/configs/configupgrade/test-fixtures/valid/heredoc-flush/want/versions.tf
similarity index 100%
rename from configs/configupgrade/test-fixtures/valid/heredoc/want/versions.tf
rename to configs/configupgrade/test-fixtures/valid/heredoc-flush/want/versions.tf
diff --git a/configs/configupgrade/test-fixtures/valid/heredoc-no-interp/input/heredoc.tf b/configs/configupgrade/test-fixtures/valid/heredoc-no-interp/input/heredoc.tf
new file mode 100644
index 000000000..b80973754
--- /dev/null
+++ b/configs/configupgrade/test-fixtures/valid/heredoc-no-interp/input/heredoc.tf
@@ -0,0 +1,6 @@
+variable "foo" {
+  default = <<EOT
+Interpolation sequences ${are not allowed} in here.
+EOT
+
+}
diff --git a/configs/configupgrade/test-fixtures/valid/heredoc-no-interp/want/heredoc.tf b/configs/configupgrade/test-fixtures/valid/heredoc-no-interp/want/heredoc.tf
new file mode 100644
index 000000000..98f62a3b7
--- /dev/null
+++ b/configs/configupgrade/test-fixtures/valid/heredoc-no-interp/want/heredoc.tf
@@ -0,0 +1,6 @@
+variable "foo" {
+  default = <<EOT
+Interpolation sequences $${are not allowed} in here.
+EOT
+
+}
diff --git a/configs/configupgrade/test-fixtures/valid/heredoc-no-interp/want/versions.tf b/configs/configupgrade/test-fixtures/valid/heredoc-no-interp/want/versions.tf
new file mode 100644
index 000000000..d9b6f790b
--- /dev/null
+++ b/configs/configupgrade/test-fixtures/valid/heredoc-no-interp/want/versions.tf
@@ -0,0 +1,3 @@
+terraform {
+  required_version = ">= 0.12"
+}
diff --git a/configs/configupgrade/test-fixtures/valid/heredoc/input/heredoc.tf b/configs/configupgrade/test-fixtures/valid/heredoc-sameline/input/heredoc.tf
similarity index 100%
rename from configs/configupgrade/test-fixtures/valid/heredoc/input/heredoc.tf
rename to configs/configupgrade/test-fixtures/valid/heredoc-sameline/input/heredoc.tf
diff --git a/configs/configupgrade/test-fixtures/valid/heredoc/want/heredoc.tf b/configs/configupgrade/test-fixtures/valid/heredoc-sameline/want/heredoc.tf
similarity index 82%
rename from configs/configupgrade/test-fixtures/valid/heredoc/want/heredoc.tf
rename to configs/configupgrade/test-fixtures/valid/heredoc-sameline/want/heredoc.tf
index edc9ad0ac..4420ebeff 100644
--- a/configs/configupgrade/test-fixtures/valid/heredoc/want/heredoc.tf
+++ b/configs/configupgrade/test-fixtures/valid/heredoc-sameline/want/heredoc.tf
@@ -2,7 +2,8 @@ locals {
   cert_options = <<EOF
 --cert-file=/etc/ssl/etcd/server.crt \
   --peer-trusted-ca-file=/etc/ssl/etcd/ca.crt \
-  --peer-client-cert-auth=trueEOF
+  --peer-client-cert-auth=true
+EOF
 
 }
 
diff --git a/configs/configupgrade/test-fixtures/valid/heredoc-sameline/want/versions.tf b/configs/configupgrade/test-fixtures/valid/heredoc-sameline/want/versions.tf
new file mode 100644
index 000000000..d9b6f790b
--- /dev/null
+++ b/configs/configupgrade/test-fixtures/valid/heredoc-sameline/want/versions.tf
@@ -0,0 +1,3 @@
+terraform {
+  required_version = ">= 0.12"
+}
diff --git a/configs/configupgrade/test-fixtures/valid/heredoc-standard/input/heredoc.tf b/configs/configupgrade/test-fixtures/valid/heredoc-standard/input/heredoc.tf
new file mode 100644
index 000000000..06ded6fdf
--- /dev/null
+++ b/configs/configupgrade/test-fixtures/valid/heredoc-standard/input/heredoc.tf
@@ -0,0 +1,12 @@
+locals {
+  baz          = { "greeting" = "hello" }
+  cert_options = <<EOF
+    A
+    B ${lookup(local.baz, "greeting")}
+    C
+  EOF
+}
+
+output "local" {
+  value = "${local.cert_options}"
+}
diff --git a/configs/configupgrade/test-fixtures/valid/heredoc-standard/want/heredoc.tf b/configs/configupgrade/test-fixtures/valid/heredoc-standard/want/heredoc.tf
new file mode 100644
index 000000000..50e6c23c6
--- /dev/null
+++ b/configs/configupgrade/test-fixtures/valid/heredoc-standard/want/heredoc.tf
@@ -0,0 +1,16 @@
+locals {
+  baz = {
+    "greeting" = "hello"
+  }
+  cert_options = <<EOF
+    A
+    B ${local.baz["greeting"]}
+    C
+  
+EOF
+
+}
+
+output "local" {
+value = local.cert_options
+}
diff --git a/configs/configupgrade/test-fixtures/valid/heredoc-standard/want/versions.tf b/configs/configupgrade/test-fixtures/valid/heredoc-standard/want/versions.tf
new file mode 100644
index 000000000..d9b6f790b
--- /dev/null
+++ b/configs/configupgrade/test-fixtures/valid/heredoc-standard/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 64115be46..2cc700117 100644
--- a/configs/configupgrade/upgrade_expr.go
+++ b/configs/configupgrade/upgrade_expr.go
@@ -62,13 +62,78 @@ Value:
 			diags = diags.Append(interpDiags)
 
 		case hcl1token.HEREDOC:
-			// TODO: Implement more complex handling to upgrade any
-			// interpolation sequences inside.
+			// HCL1's "Value" method for tokens pulls out the body and removes
+			// any indents in the source for a flush heredoc, which throws away
+			// information we need to upgrade. Therefore we're going to
+			// re-implement a subset of that logic here where we want to retain
+			// the whitespace verbatim even in flush mode.
 
-			// TODO: If a heredoc has its termination delimeter inline (which is
-			// a bug that worked in terraform 0.11, so we need to support it
-			// here), move the delimiter to a new line.
-			buf.WriteString(tv.Text)
+			firstNewlineIdx := strings.IndexByte(tv.Text, '\n')
+			if firstNewlineIdx < 0 {
+				// Should never happen, because tv.Value would already have
+				// panicked above in this case.
+				panic("heredoc doesn't contain newline")
+			}
+			introducer := tv.Text[:firstNewlineIdx+1]
+			marker := introducer[2:] // trim off << prefix
+			if marker[0] == '-' {
+				marker = marker[1:] // also trim of - prefix for flush heredoc
+			}
+			body := tv.Text[len(introducer) : len(tv.Text)-len(marker)]
+			flush := introducer[2] == '-'
+			if flush {
+				// HCL1 treats flush heredocs differently, trimming off any
+				// spare whitespace that might appear after the trailing
+				// newline, and so we must replicate that here to avoid
+				// introducing additional whitespace in the output.
+				body = strings.TrimRight(body, " \t")
+			}
+
+			// Now we have:
+			//  - introducer is the first line, like "<<-FOO\n"
+			//  - marker is the end marker, like "FOO\n"
+			//  - body is the raw data between the introducer and the marker,
+			//    which we need to do recursive upgrading for.
+
+			buf.WriteString(introducer)
+			if !interp {
+				// Easy case: escape all interpolation-looking sequences.
+				printHeredocLiteralFromHILOutput(&buf, body)
+			} else {
+				hilNode, err := hil.Parse(body)
+				if err != nil {
+					diags = diags.Append(&hcl2.Diagnostic{
+						Severity: hcl2.DiagError,
+						Summary:  "Invalid interpolated string",
+						Detail:   fmt.Sprintf("Interpolation parsing failed: %s", err),
+						Subject:  hcl1PosRange(filename, tv.Pos).Ptr(),
+					})
+				}
+				if _, ok := hilNode.(*hilast.Output); !ok {
+					// hil.Parse usually produces an output, but it can sometimes
+					// produce an isolated expression if the input is entirely
+					// a single interpolation.
+					hilNode = &hilast.Output{
+						Exprs: []hilast.Node{hilNode},
+						Posx:  hilNode.Pos(),
+					}
+				}
+				interpDiags := upgradeHeredocBody(&buf, hilNode.(*hilast.Output), filename, an)
+				diags = diags.Append(interpDiags)
+			}
+			if !strings.HasSuffix(body, "\n") {
+				// The versions of HCL1 vendored into Terraform <=0.11
+				// incorrectly allowed the end marker to appear at the end of
+				// the final line of the body, rather than on a line of its own.
+				// That is no longer valid in HCL2, so we need to fix it up.
+				buf.WriteByte('\n')
+			}
+			// NOTE: Marker intentionally contains an extra newline here because
+			// we need to ensure that any follow-on expression bits end up on
+			// a separate line, or else the HCL2 parser won't be able to
+			// recognize the heredoc marker. This causes an extra empty line
+			// in some cases, which we accept for simplicity's sake.
+			buf.WriteString(marker)
 
 		case hcl1token.BOOL:
 			if litVal.(bool) {
@@ -428,6 +493,27 @@ Value:
 	return buf.Bytes(), diags
 }
 
+func upgradeHeredocBody(buf *bytes.Buffer, val *hilast.Output, filename string, an *analysis) tfdiags.Diagnostics {
+	var diags tfdiags.Diagnostics
+
+	for _, item := range val.Exprs {
+		if lit, ok := item.(*hilast.LiteralNode); ok {
+			if litStr, ok := lit.Value.(string); ok {
+				printHeredocLiteralFromHILOutput(buf, litStr)
+				continue
+			}
+		}
+		interped, interpDiags := upgradeExpr(item, filename, true, an)
+		diags = diags.Append(interpDiags)
+
+		buf.WriteString("${")
+		buf.Write(interped)
+		buf.WriteString("}")
+	}
+
+	return diags
+}
+
 func upgradeTraversalExpr(val interface{}, filename string, an *analysis) ([]byte, tfdiags.Diagnostics) {
 	if lit, ok := val.(*hcl1ast.LiteralType); ok && lit.Token.Type == hcl1token.STRING {
 		trStr := lit.Token.Value().(string)
diff --git a/configs/configupgrade/upgrade_native.go b/configs/configupgrade/upgrade_native.go
index 2e8d91fe0..b2090ed94 100644
--- a/configs/configupgrade/upgrade_native.go
+++ b/configs/configupgrade/upgrade_native.go
@@ -627,6 +627,12 @@ func printStringLiteralFromHILOutput(buf *bytes.Buffer, val string) {
 	buf.WriteString(val)
 }
 
+func printHeredocLiteralFromHILOutput(buf *bytes.Buffer, val string) {
+	val = strings.Replace(val, `${`, `$${`, -1)
+	val = strings.Replace(val, `%{`, `%%{`, -1)
+	buf.WriteString(val)
+}
+
 func collectAdhocComments(f *hcl1ast.File) *commentQueue {
 	comments := make(map[hcl1token.Pos]*hcl1ast.CommentGroup)
 	for _, c := range f.Comments {