From 8d17fcea4ec02ff7b51722fed0dda182550a9e58 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 19 Dec 2018 15:20:23 -0500 Subject: [PATCH 1/3] make connection host Required And provide the connection config for validation --- terraform/eval_validate.go | 8 ++++---- terraform/node_resource_validate.go | 1 + 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/terraform/eval_validate.go b/terraform/eval_validate.go index 06db8da0c..aaae21e8d 100644 --- a/terraform/eval_validate.go +++ b/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, diff --git a/terraform/node_resource_validate.go b/terraform/node_resource_validate.go index 57bc6f1dd..c1e10a6b2 100644 --- a/terraform/node_resource_validate.go +++ b/terraform/node_resource_validate.go @@ -71,6 +71,7 @@ func (n *NodeValidatableResource) EvalTree() EvalNode { Schema: &provisionerSchema, Config: p, ResourceHasCount: hasCount, + ConnConfig: p.Connection, }, ) } From c55228415794c51fd35f589510be5c5e8e80d895 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 19 Dec 2018 15:22:47 -0500 Subject: [PATCH 2/3] don't evaluate an empty connection body There's a required field now, so evaluating an empty block will always fail. --- terraform/eval_apply.go | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/terraform/eval_apply.go b/terraform/eval_apply.go index 8c09e997a..73b8a47a2 100644 --- a/terraform/eval_apply.go +++ b/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() + } } { From 0b59f9cad22dcada6793c37e660345712c6d5529 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 19 Dec 2018 15:23:29 -0500 Subject: [PATCH 3/3] fix provisioner tests Add host where required in the test configs, and fix the mock to check for a null connection. --- terraform/context_apply_test.go | 7 ++---- terraform/eval_validate_test.go | 10 +++++--- terraform/provisioner_mock.go | 25 +++++++++++++------ .../apply-provisioner-conninfo/main.tf | 2 ++ .../test-fixtures/graph-provisioners/main.tf | 1 + 5 files changed, 28 insertions(+), 17 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 760373d03..3fef47809 100644 --- a/terraform/context_apply_test.go +++ b/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") } } diff --git a/terraform/eval_validate_test.go b/terraform/eval_validate_test.go index acf9a1fc0..3f5861a64 100644 --- a/terraform/eval_validate_test.go +++ b/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()) } diff --git a/terraform/provisioner_mock.go b/terraform/provisioner_mock.go index f0682e4f0..f59589164 100644 --- a/terraform/provisioner_mock.go +++ b/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. diff --git a/terraform/test-fixtures/apply-provisioner-conninfo/main.tf b/terraform/test-fixtures/apply-provisioner-conninfo/main.tf index 37db98228..5166d22ba 100644 --- a/terraform/test-fixtures/apply-provisioner-conninfo/main.tf +++ b/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 diff --git a/terraform/test-fixtures/graph-provisioners/main.tf b/terraform/test-fixtures/graph-provisioners/main.tf index 401b16c74..6e1e93aac 100644 --- a/terraform/test-fixtures/graph-provisioners/main.tf +++ b/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}" }