command/apply: apply from plan respects -backup and -state-out

Fixes #5409

I didn't expect this to be such a rabbit hole!

Based on git history, it appears that for "historical reasons"(tm),
setting up the various `state.State` structures for a plan were
_completely different logic_ than a normal `terraform apply`. This meant
that it was skipping things like disabling backups with `-backup="-"`.

This PR unifies loading from a plan to the normal state setup mechanism.
A few tests that were failing prior to this PR were added, no existing
tests were changed.

command/apply_test.go

@@ -508,6 +508,79 @@ func TestApply_plan(t *testing.T) {
 	}
 }
 
+func TestApply_plan_backup(t *testing.T) {
+	planPath := testPlanFile(t, testPlan(t))
+	statePath := testTempFile(t)
+	backupPath := testTempFile(t)
+
+	p := testProvider()
+	ui := new(cli.MockUi)
+	c := &ApplyCommand{
+		Meta: Meta{
+			ContextOpts: testCtxConfig(p),
+			Ui:          ui,
+		},
+	}
+
+	args := []string{
+		"-state", statePath,
+		"-backup", backupPath,
+		planPath,
+	}
+	if code := c.Run(args); code != 0 {
+		t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String())
+	}
+
+	{
+		// Should have a backup file
+		f, err := os.Open(backupPath)
+		if err != nil {
+			t.Fatalf("err: %s", err)
+		}
+
+		_, err = terraform.ReadState(f)
+		f.Close()
+		if err != nil {
+			t.Fatalf("err: %s", err)
+		}
+	}
+}
+
+func TestApply_plan_noBackup(t *testing.T) {
+	planPath := testPlanFile(t, testPlan(t))
+	statePath := testTempFile(t)
+
+	p := testProvider()
+	ui := new(cli.MockUi)
+	c := &ApplyCommand{
+		Meta: Meta{
+			ContextOpts: testCtxConfig(p),
+			Ui:          ui,
+		},
+	}
+
+	args := []string{
+		"-state", statePath,
+		"-backup", "-",
+		planPath,
+	}
+	if code := c.Run(args); code != 0 {
+		t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String())
+	}
+
+	// Ensure there is no backup
+	_, err := os.Stat(statePath + DefaultBackupExtension)
+	if err == nil || !os.IsNotExist(err) {
+		t.Fatalf("backup should not exist")
+	}
+
+	// Ensure there is no literal "-"
+	_, err = os.Stat("-")
+	if err == nil || !os.IsNotExist(err) {
+		t.Fatalf("backup should not exist")
+	}
+}
+
 func TestApply_plan_remoteState(t *testing.T) {
 	// Disable test mode so input would be asked
 	test = false

command/command_test.go

@@ -98,6 +98,20 @@ func testModule(t *testing.T, name string) *module.Tree {
 	return mod
 }
 
+// testPlan returns a non-nil noop plan.
+func testPlan(t *testing.T) *terraform.Plan {
+	state := terraform.NewState()
+	state.RootModule().Outputs["foo"] = &terraform.OutputState{
+		Type:  "string",
+		Value: "foo",
+	}
+
+	return &terraform.Plan{
+		Module: testModule(t, "apply"),
+		State:  state,
+	}
+}
+
 func testPlanFile(t *testing.T, plan *terraform.Plan) string {
 	path := testTempFile(t)
 

command/meta.go

@@ -112,18 +112,24 @@ func (m *Meta) Context(copts contextOpts) (*terraform.Context, bool, error) {
 		plan, err := terraform.ReadPlan(f)
 		f.Close()
 		if err == nil {
-			// Setup our state
-			state, statePath, err := StateFromPlan(m.statePath, m.stateOutPath, plan)
+			// Setup our state, force it to use our plan's state
+			stateOpts := m.StateOpts()
+			if plan != nil {
+				stateOpts.ForceState = plan.State
+			}
+
+			// Get the state
+			result, err := State(stateOpts)
 			if err != nil {
 				return nil, false, fmt.Errorf("Error loading plan: %s", err)
 			}
 
 			// Set our state
-			m.state = state
+			m.state = result.State
 
 			// this is used for printing the saved location later
 			if m.stateOutPath == "" {
-				m.stateOutPath = statePath
+				m.stateOutPath = result.StatePath
 			}
 
 			if len(m.variables) > 0 {

command/state.go

@@ -3,7 +3,6 @@ package command
 import (
 	"fmt"
 	"os"
-	"path/filepath"
 	"strings"
 
 	"github.com/hashicorp/errwrap"
@@ -33,6 +32,10 @@ type StateOpts struct {
 	// it is assumed to be the path where the state is stored locally
 	// plus the DefaultBackupExtension.
 	BackupPath string
+
+	// ForceState is a state structure to force the value to be. This
+	// is used by Terraform plans (which contain their state).
+	ForceState *terraform.State
 }
 
 // StateResult is the result of calling State and holds various different
@@ -75,6 +78,12 @@ func State(opts *StateOpts) (*StateResult, error) {
 			if err := ls.RefreshState(); err != nil {
 				return nil, err
 			}
+
+			// If we have a forced state, set it
+			if opts.ForceState != nil {
+				ls.SetState(opts.ForceState)
+			}
+
 			is := &state.InmemState{}
 			is.WriteState(ls.State())
 
@@ -88,14 +97,30 @@ func State(opts *StateOpts) (*StateResult, error) {
 				return nil, err
 			}
 		} else {
-			if _, err := os.Stat(opts.RemotePath); err == nil {
-				// We have a remote state, initialize that.
-				remote, err = remoteStateFromPath(
+			// If we have a forced state that is remote, then we load that
+			if opts.ForceState != nil &&
+				opts.ForceState.Remote != nil &&
+				opts.ForceState.Remote.Type != "" {
+				var err error
+				remote, err = remoteState(
+					opts.ForceState,
 					opts.RemotePath,
-					opts.RemoteRefresh)
+					false)
 				if err != nil {
 					return nil, err
 				}
+			} else {
+				// Only if we have no forced state, we check our normal
+				// remote path.
+				if _, err := os.Stat(opts.RemotePath); err == nil {
+					// We have a remote state, initialize that.
+					remote, err = remoteStateFromPath(
+						opts.RemotePath,
+						opts.RemoteRefresh)
+					if err != nil {
+						return nil, err
+					}
+				}
 			}
 		}
 
@@ -106,6 +131,15 @@ func State(opts *StateOpts) (*StateResult, error) {
 		}
 	}
 
+	// If we have a forced state and we were able to initialize that
+	// into a remote state, we don't do any local state stuff. This is
+	// because normally we're able to test whether we should do local vs.
+	// remote by checking file existence. With ForceState, file existence
+	// doesn't work because neither may exist, so we use state attributes.
+	if opts.ForceState != nil && result.Remote != nil {
+		opts.LocalPath = ""
+	}
+
 	// Do we have a local state?
 	if opts.LocalPath != "" {
 		local := &state.LocalState{
@@ -120,23 +154,30 @@ func State(opts *StateOpts) (*StateResult, error) {
 			result.LocalPath = local.PathOut
 		}
 
-		err := local.RefreshState()
-		if err == nil {
-			if result.State != nil && !result.State.State().Empty() {
-				if !local.State().Empty() {
-					// We already have a remote state... that is an error.
-					return nil, fmt.Errorf(
-						"Remote state found, but state file '%s' also present.",
-						opts.LocalPath)
-				}
+		// If we're forcing, then set it
+		if opts.ForceState != nil {
+			local.SetState(opts.ForceState)
+		} else {
+			// If we're not forcing, then we load the state directly
+			// from disk.
+			err := local.RefreshState()
+			if err == nil {
+				if result.State != nil && !result.State.State().Empty() {
+					if !local.State().Empty() {
+						// We already have a remote state... that is an error.
+						return nil, fmt.Errorf(
+							"Remote state found, but state file '%s' also present.",
+							opts.LocalPath)
+					}
 
-				// Empty state
-				local = nil
+					// Empty state
+					local = nil
+				}
+			}
+			if err != nil {
+				return nil, errwrap.Wrapf(
+					"Error reading local state: {{err}}", err)
 			}
-		}
-		if err != nil {
-			return nil, errwrap.Wrapf(
-				"Error reading local state: {{err}}", err)
 		}
 
 		if local != nil {
@@ -167,43 +208,6 @@ func State(opts *StateOpts) (*StateResult, error) {
 	return result, nil
 }
 
-// StateFromPlan gets our state from the plan.
-func StateFromPlan(
-	localPath, outPath string,
-	plan *terraform.Plan) (state.State, string, error) {
-	var result state.State
-	resultPath := localPath
-	if plan != nil && plan.State != nil &&
-		plan.State.Remote != nil && plan.State.Remote.Type != "" {
-		var err error
-
-		// It looks like we have a remote state in the plan, so
-		// we have to initialize that.
-		resultPath = filepath.Join(DefaultDataDir, DefaultStateFilename)
-		result, err = remoteState(plan.State, resultPath, false)
-		if err != nil {
-			return nil, "", err
-		}
-	}
-
-	if result == nil {
-		local := &state.LocalState{
-			Path:    resultPath,
-			PathOut: outPath,
-		}
-		local.SetState(plan.State)
-		result = local
-	}
-
-	// If we have a result, make sure to back it up
-	result = &state.BackupState{
-		Real: result,
-		Path: resultPath + DefaultBackupExtension,
-	}
-
-	return result, resultPath, nil
-}
-
 func remoteState(
 	local *terraform.State,
 	localPath string, refresh bool) (*state.CacheState, error) {