Merge pull request #19707 from hashicorp/jbardin/connection

Validate provisioner connection blocks

terraform/context_apply_test.go

@@ -5675,11 +5675,8 @@ func TestContext2Apply_provisionerDestroyRefInvalid(t *testing.T) {
 		},
 	})
 
-	if _, diags := ctx.Plan(); diags.HasErrors() {
-		t.Fatalf("plan errors: %s", diags.Err())
-	}
-
-	if _, diags := ctx.Apply(); diags == nil {
+	// this was an apply test, but this is now caught in Validation
+	if diags := ctx.Validate(); !diags.HasErrors() {
 		t.Fatal("expected error")
 	}
 }

terraform/eval_apply.go

@@ -494,16 +494,20 @@ func (n *EvalApplyProvisioners) apply(ctx EvalContext, provs []*configs.Provisio
 			connBody = baseConn
 		case localConn != nil:
 			connBody = localConn
-		default: // both are nil, by elimination
-			connBody = hcl.EmptyBody()
 		}
 
-		connInfo, _, connInfoDiags := ctx.EvaluateBlock(connBody, connectionBlockSupersetSchema, instanceAddr, keyData)
-		diags = diags.Append(connInfoDiags)
-		if diags.HasErrors() {
-			// "on failure continue" setting only applies to failures of the
-			// provisioner itself, not to invalid configuration.
-			return diags.Err()
+		// start with an empty connInfo
+		connInfo := cty.NullVal(connectionBlockSupersetSchema.ImpliedType())
+
+		if connBody != nil {
+			var connInfoDiags tfdiags.Diagnostics
+			connInfo, _, connInfoDiags = ctx.EvaluateBlock(connBody, connectionBlockSupersetSchema, instanceAddr, keyData)
+			diags = diags.Append(connInfoDiags)
+			if diags.HasErrors() {
+				// "on failure continue" setting only applies to failures of the
+				// provisioner itself, not to invalid configuration.
+				return diags.Err()
+			}
 		}
 
 		{

terraform/eval_validate.go

@@ -218,6 +218,10 @@ var connectionBlockSupersetSchema = &configschema.Block{
 		// by the config loader and stored away in a separate field.
 
 		// Common attributes for both connection types
+		"host": {
+			Type:     cty.String,
+			Required: true,
+		},
 		"type": {
 			Type:     cty.String,
 			Optional: true,
@@ -230,10 +234,6 @@ var connectionBlockSupersetSchema = &configschema.Block{
 			Type:     cty.String,
 			Optional: true,
 		},
-		"host": {
-			Type:     cty.String,
-			Optional: true,
-		},
 		"port": {
 			Type:     cty.String,
 			Optional: true,

terraform/eval_validate_test.go

@@ -374,8 +374,9 @@ func TestEvalValidateProvisioner_valid(t *testing.T) {
 			Config: hcl.EmptyBody(),
 		},
 		ConnConfig: &configs.Connection{
-			//Type:   "ssh",
-			Config: hcl.EmptyBody(),
+			Config: configs.SynthBody("", map[string]cty.Value{
+				"host": cty.StringVal("foo"),
+			}),
 		},
 	}
 
@@ -421,6 +422,7 @@ func TestEvalValidateProvisioner_warning(t *testing.T) {
 		},
 		ConnConfig: &configs.Connection{
 			Config: configs.SynthBody("", map[string]cty.Value{
+				"host": cty.StringVal("localhost"),
 				"type": cty.StringVal("ssh"),
 			}),
 		},
@@ -442,7 +444,7 @@ func TestEvalValidateProvisioner_warning(t *testing.T) {
 	var diags tfdiags.Diagnostics
 	diags = diags.Append(err)
 	if len(diags) != 1 {
-		t.Fatalf("wrong number of diagsnostics in %#v; want one warning", diags)
+		t.Fatalf("wrong number of diagnostics in %s; want one warning", diags.ErrWithWarnings())
 	}
 
 	if got, want := diags[0].Description().Summary, mp.ValidateProvisionerConfigResponse.Diagnostics[0].Description().Summary; got != want {
@@ -492,7 +494,7 @@ func TestEvalValidateProvisioner_connectionInvalid(t *testing.T) {
 
 	var diags tfdiags.Diagnostics
 	diags = diags.Append(err)
-	if len(diags) != 2 {
+	if len(diags) != 3 {
 		t.Fatalf("wrong number of diagnostics; want two errors\n\n%s", diags.Err())
 	}
 

terraform/node_resource_validate.go

@@ -71,6 +71,7 @@ func (n *NodeValidatableResource) EvalTree() EvalNode {
 					Schema:           &provisionerSchema,
 					Config:           p,
 					ResourceHasCount: hasCount,
+					ConnConfig:       p.Connection,
 				},
 			)
 		}

terraform/provisioner_mock.go

@@ -1,6 +1,7 @@
 package terraform
 
 import (
+	"fmt"
 	"sync"
 
 	"github.com/zclconf/go-cty/cty"
@@ -79,21 +80,29 @@ func (p *MockProvisioner) ProvisionResource(r provisioners.ProvisionResourceRequ
 	p.ProvisionResourceCalled = true
 	p.ProvisionResourceRequest = r
 	if p.ApplyFn != nil {
+		if !r.Config.IsKnown() {
+			panic(fmt.Sprintf("cannot provision with unknown value: %#v", r.Config))
+		}
+
 		schema := p.getSchema()
 		rc := NewResourceConfigShimmed(r.Config, schema.Provisioner)
 		connVal := r.Connection
 		connMap := map[string]string{}
-		for it := connVal.ElementIterator(); it.Next(); {
-			ak, av := it.Element()
-			name := ak.AsString()
 
-			if !av.IsKnown() || av.IsNull() {
-				continue
+		if !connVal.IsNull() && connVal.IsKnown() {
+			for it := connVal.ElementIterator(); it.Next(); {
+				ak, av := it.Element()
+				name := ak.AsString()
+
+				if !av.IsKnown() || av.IsNull() {
+					continue
+				}
+
+				av, _ = convert.Convert(av, cty.String)
+				connMap[name] = av.AsString()
 			}
-
-			av, _ = convert.Convert(av, cty.String)
-			connMap[name] = av.AsString()
 		}
+
 		// We no longer pass the full instance state to a provisioner, so we'll
 		// construct a partial one that should be good enough for what existing
 		// test mocks need.

terraform/test-fixtures/apply-provisioner-conninfo/main.tf

@@ -12,12 +12,14 @@ resource "aws_instance" "foo" {
 
 resource "aws_instance" "bar" {
     connection {
+        host = "localhost"
         type = "telnet"
     }
 
     provisioner "shell" {
         foo = "${aws_instance.foo.value}"
         connection {
+            host = "localhost"
             type = "telnet"
             user = "superuser"
             port = 2222

terraform/test-fixtures/graph-provisioners/main.tf

@@ -25,6 +25,7 @@ resource "aws_load_balancer" "weblb" {
     provisioner "shell" {
         cmd = "add ${aws_instance.web.id}"
         connection {
+            host = "localhost"
             type = "magic"
             user = "${aws_security_group.firewall.id}"
         }