From 8547603ff5645edcdcaf2f49fd8a6d18e4cda3b3 Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Tue, 3 Dec 2019 13:23:07 -0500
Subject: [PATCH] deprecation warning for destroy provisioner refs

Add deprecation warning for references from destroy provisioners or
their connections to external resources or values. In order to ensure
resource destruction can be completed correctly, destroy nodes must be
able to evaluate with only their instance state.

We have sufficient information to validate destroy-time provisioners
early on during the config loading process. Later on these can be
converted to hard errors, and only allow self, count.index, and each.key
in destroy provisioners. Limited the provisioner and block evaluation
scope later on is tricky, but if the references can never be loaded,
then they will never be encountered during evaluation.
---
 configs/provisioner.go                        | 55 ++++++++++++++++++-
 configs/resource.go                           | 11 ++++
 .../warning-files/destroy-provisioners.tf     | 40 ++++++++++++++
 3 files changed, 104 insertions(+), 2 deletions(-)
 create mode 100644 configs/testdata/warning-files/destroy-provisioners.tf

diff --git a/configs/provisioner.go b/configs/provisioner.go
index 47b656791..fc5136ddd 100644
--- a/configs/provisioner.go
+++ b/configs/provisioner.go
@@ -50,6 +50,11 @@ func decodeProvisionerBlock(block *hcl.Block) (*Provisioner, hcl.Diagnostics) {
 		}
 	}
 
+	// destroy provisioners can only refer to self
+	if pv.When == ProvisionerWhenDestroy {
+		diags = append(diags, onlySelfRefs(config)...)
+	}
+
 	if attr, exists := content.Attributes["on_failure"]; exists {
 		expr, shimDiags := shimTraversalInString(attr.Expr, true)
 		diags = append(diags, shimDiags...)
@@ -85,8 +90,11 @@ func decodeProvisionerBlock(block *hcl.Block) (*Provisioner, hcl.Diagnostics) {
 			}
 			seenConnection = block
 
-			//conn, connDiags := decodeConnectionBlock(block)
-			//diags = append(diags, connDiags...)
+			// destroy provisioners can only refer to self
+			if pv.When == ProvisionerWhenDestroy {
+				diags = append(diags, onlySelfRefs(block.Body)...)
+			}
+
 			pv.Connection = &Connection{
 				Config:    block.Body,
 				DeclRange: block.DefRange,
@@ -107,6 +115,49 @@ func decodeProvisionerBlock(block *hcl.Block) (*Provisioner, hcl.Diagnostics) {
 	return pv, diags
 }
 
+func onlySelfRefs(body hcl.Body) hcl.Diagnostics {
+	var diags hcl.Diagnostics
+
+	// Provisioners currently do not use any blocks in their configuration.
+	// Blocks are likely to remain solely for meta parameters, but in the case
+	// that blocks are supported for provisioners, we will want to extend this
+	// to find variables in nested blocks.
+	attrs, _ := body.JustAttributes()
+	for _, attr := range attrs {
+		for _, v := range attr.Expr.Variables() {
+			valid := false
+			switch v.RootName() {
+			case "self":
+				valid = true
+			case "count":
+				// count must use "index"
+				if len(v) == 2 {
+					if t, ok := v[1].(hcl.TraverseAttr); ok && t.Name == "index" {
+						valid = true
+					}
+				}
+
+			case "each":
+				if len(v) == 2 {
+					if t, ok := v[1].(hcl.TraverseAttr); ok && t.Name == "key" {
+						valid = true
+					}
+				}
+			}
+
+			if !valid {
+				diags = append(diags, &hcl.Diagnostic{
+					Severity: hcl.DiagWarning,
+					Summary:  "External references from destroy provisioners are deprecated",
+					Detail:   "Destroy time provisioners and their connections may only reference values stored in the instance state, which include 'self', 'count.index', or 'each.key'.",
+					Subject:  attr.Expr.Range().Ptr(),
+				})
+			}
+		}
+	}
+	return diags
+}
+
 // Connection represents a "connection" block when used within either a
 // "resource" or "provisioner" block in a module or file.
 type Connection struct {
diff --git a/configs/resource.go b/configs/resource.go
index 4d5506e29..78f6a240c 100644
--- a/configs/resource.go
+++ b/configs/resource.go
@@ -273,6 +273,17 @@ func decodeResourceBlock(block *hcl.Block) (*Resource, hcl.Diagnostics) {
 		}
 	}
 
+	// Now we can validate the connection block references if there are any destroy provisioners.
+	// TODO: should we eliminate standalone connection blocks?
+	if r.Managed.Connection != nil {
+		for _, p := range r.Managed.Provisioners {
+			if p.When == ProvisionerWhenDestroy {
+				diags = append(diags, onlySelfRefs(r.Managed.Connection.Config)...)
+				break
+			}
+		}
+	}
+
 	return r, diags
 }
 
diff --git a/configs/testdata/warning-files/destroy-provisioners.tf b/configs/testdata/warning-files/destroy-provisioners.tf
new file mode 100644
index 000000000..ff950bc7f
--- /dev/null
+++ b/configs/testdata/warning-files/destroy-provisioners.tf
@@ -0,0 +1,40 @@
+locals {
+  user = "name"
+}
+
+resource "null_resource" "a" {
+  connection {
+    host = self.hostname
+    user = local.user # WARNING: External references from destroy provisioners are deprecated
+  }
+
+  provisioner "remote-exec" {
+    when = destroy
+    index = count.index
+    key = each.key
+
+  }
+}
+
+resource "null_resource" "b" {
+  connection {
+    host = self.hostname
+    # this is OK since there is no destroy provisioner
+    user = local.user
+  }
+
+  provisioner "remote-exec" {
+  }
+}
+
+resource "null_resource" "b" {
+  provisioner "remote-exec" {
+    when = destroy
+    connection {
+      host = self.hostname
+      user = local.user # WARNING: External references from destroy provisioners are deprecated
+    }
+
+    command = "echo ${local.name}" # WARNING: External references from destroy provisioners are deprecated
+  }
+}