From 1cd5ec22ed883a167ada5d5845ab69b17eab6920 Mon Sep 17 00:00:00 2001
From: Martin Atkins <mart@degeneration.co.uk>
Date: Tue, 9 Oct 2018 12:19:24 -0700
Subject: [PATCH] backend/local: Require caller to set PlanOutBackend with
 PlanOutPath

We can't generate a valid plan file without a backend configuration to
write into it, but it's the responsibility of the caller (the command
package) to manage the backend configuration mechanism, so we require it
to tell us what to write here.

This feels a little strange because the backend in principle knows its
own config, but in practice the backend only knows the _processed_ version
of the config, not the raw configuration value that was used to configure
it.
---
 backend/local/backend_plan.go      | 12 +++++++-----
 backend/local/backend_plan_test.go | 26 ++++++++++++++++++++++++++
 backend/local/testing.go           |  4 ++++
 plans/planfile/tfplan.go           |  7 +++++++
 4 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/backend/local/backend_plan.go b/backend/local/backend_plan.go
index 6d2639d323..a50426f3a1 100644
--- a/backend/local/backend_plan.go
+++ b/backend/local/backend_plan.go
@@ -114,12 +114,14 @@ func (b *Local) opPlan(
 
 	// Save the plan to disk
 	if path := op.PlanOutPath; path != "" {
-		if op.PlanOutBackend != nil {
-			plan.Backend = *op.PlanOutBackend
-		} else {
-			op.PlanOutBackend = &plans.Backend{}
-			plan.Backend = *op.PlanOutBackend
+		if op.PlanOutBackend == nil {
+			// This is always a bug in the operation caller; it's not valid
+			// to set PlanOutPath without also setting PlanOutBackend.
+			diags = diags.Append(fmt.Errorf("PlanOutPath set without also setting PlanOutBackend (this is a bug in Terraform)"))
+			b.ReportResult(runningOp, diags)
+			return
 		}
+		plan.Backend = *op.PlanOutBackend
 
 		// We may have updated the state in the refresh step above, but we
 		// will freeze that updated state in the plan file for now and
diff --git a/backend/local/backend_plan_test.go b/backend/local/backend_plan_test.go
index 7ce9e84521..541a58ad8a 100644
--- a/backend/local/backend_plan_test.go
+++ b/backend/local/backend_plan_test.go
@@ -174,6 +174,18 @@ func TestLocal_planDestroy(t *testing.T) {
 	op.Destroy = true
 	op.PlanRefresh = true
 	op.PlanOutPath = planPath
+	cfg := cty.ObjectVal(map[string]cty.Value{
+		"path": cty.StringVal(b.StatePath),
+	})
+	cfgRaw, err := plans.NewDynamicValue(cfg, cfg.Type())
+	if err != nil {
+		t.Fatal(err)
+	}
+	op.PlanOutBackend = &plans.Backend{
+		// Just a placeholder so that we can generate a valid plan file.
+		Type: "local",
+		Config: cfgRaw,
+	}
 
 	run, err := b.Operation(context.Background(), op)
 	if err != nil {
@@ -213,6 +225,18 @@ func TestLocal_planOutPathNoChange(t *testing.T) {
 	op, configCleanup := testOperationPlan(t, "./test-fixtures/plan")
 	defer configCleanup()
 	op.PlanOutPath = planPath
+	cfg := cty.ObjectVal(map[string]cty.Value{
+		"path": cty.StringVal(b.StatePath),
+	})
+	cfgRaw, err := plans.NewDynamicValue(cfg, cfg.Type())
+	if err != nil {
+		t.Fatal(err)
+	}
+	op.PlanOutBackend = &plans.Backend{
+		// Just a placeholder so that we can generate a valid plan file.
+		Type: "local",
+		Config: cfgRaw,
+	}
 
 	run, err := b.Operation(context.Background(), op)
 	if err != nil {
@@ -314,6 +338,8 @@ func testPlanState() *states.State {
 }
 
 func testReadPlan(t *testing.T, path string) *plans.Plan {
+	t.Helper()
+
 	p, err := planfile.Open(path)
 	if err != nil {
 		t.Fatalf("err: %s", err)
diff --git a/backend/local/testing.go b/backend/local/testing.go
index 3ac066ff22..bc833dc999 100644
--- a/backend/local/testing.go
+++ b/backend/local/testing.go
@@ -34,6 +34,10 @@ func TestLocal(t *testing.T) (*Local, func()) {
 			var diags tfdiags.Diagnostics
 			diags = diags.Append(vals...)
 			for _, diag := range diags {
+				// NOTE: Since the caller here is not directly the TestLocal
+				// function, t.Helper doesn't apply and so the log source
+				// isn't correctly shown in the test log output. This seems
+				// unavoidable as long as this is happening so indirectly.
 				t.Log(diag.Description().Summary)
 				if local.CLI != nil {
 					local.CLI.Error(diag.Description().Summary)
diff --git a/plans/planfile/tfplan.go b/plans/planfile/tfplan.go
index 2db909b9fb..e1deeb083a 100644
--- a/plans/planfile/tfplan.go
+++ b/plans/planfile/tfplan.go
@@ -351,6 +351,13 @@ func writeTfplan(plan *plans.Plan, w io.Writer) error {
 		rawPlan.Variables[name] = valueToTfplan(val)
 	}
 
+	if plan.Backend.Type == "" || plan.Backend.Config == nil {
+		// This suggests a bug in the code that created the plan, since it
+		// ought to always have a backend populated, even if it's the default
+		// "local" backend with a local state file.
+		return fmt.Errorf("plan does not have a backend configuration")
+	}
+
 	rawPlan.Backend = &planproto.Backend{
 		Type:      plan.Backend.Type,
 		Config:    valueToTfplan(plan.Backend.Config),