From 9cdba1f199a91007e7ff3634614e44d6ffcf400b Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Wed, 1 Feb 2017 18:16:16 -0500
Subject: [PATCH 01/18] enable local state locking for apply

Have the LocalBackend lock the state during operations, and enble this
for the apply comand.
---
 backend/backend.go               | 10 +++++++++-
 backend/local/backend.go         |  8 ++++++++
 backend/local/backend_apply.go   | 19 +++++++++++++++----
 backend/local/backend_local.go   |  7 +++++++
 backend/local/backend_plan.go    | 13 ++++++++++++-
 backend/local/backend_refresh.go | 20 ++++++++++++++++----
 command/apply.go                 |  2 ++
 command/meta.go                  |  3 +++
 8 files changed, 72 insertions(+), 10 deletions(-)

diff --git a/backend/backend.go b/backend/backend.go
index 16b8a2ba4..86f885207 100644
--- a/backend/backend.go
+++ b/backend/backend.go
@@ -22,7 +22,8 @@ type Backend interface {
 
 	// State returns the current state for this environment. This state may
 	// not be loaded locally: the proper APIs should be called on state.State
-	// to load the state.
+	// to load the state. If the state.State is a state.Locker, it's up to the
+	// caller to call Lock and Unlock as needed.
 	State() (state.State, error)
 }
 
@@ -38,6 +39,9 @@ type Enhanced interface {
 	// It is up to the implementation to determine what "performing" means.
 	// This DOES NOT BLOCK. The context returned as part of RunningOperation
 	// should be used to block for completion.
+	// If the state used in the operation can be locked, it is the
+	// responsibility of the Backend to lock the state for the duration of the
+	// running operation.
 	Operation(context.Context, *Operation) (*RunningOperation, error)
 }
 
@@ -99,6 +103,10 @@ type Operation struct {
 	// Input/output/control options.
 	UIIn  terraform.UIInput
 	UIOut terraform.UIOutput
+
+	// If LockState is true, the Operation must Lock any
+	// state.Lockers for its duration, and Unlock when complete.
+	LockState bool
 }
 
 // RunningOperation is the result of starting an operation.
diff --git a/backend/local/backend.go b/backend/local/backend.go
index 7e6aa9c0a..e45eaec25 100644
--- a/backend/local/backend.go
+++ b/backend/local/backend.go
@@ -34,6 +34,9 @@ type Local struct {
 	StateOutPath    string
 	StateBackupPath string
 
+	// we only want to create a single instance of the local state
+	state state.State
+
 	// ContextOpts are the base context options to set when initializing a
 	// Terraform context. Many of these will be overridden or merged by
 	// Operation. See Operation for more details.
@@ -100,6 +103,10 @@ func (b *Local) State() (state.State, error) {
 		return b.Backend.State()
 	}
 
+	if b.state != nil {
+		return b.state, nil
+	}
+
 	// Otherwise, we need to load the state.
 	var s state.State = &state.LocalState{
 		Path:    b.StatePath,
@@ -119,6 +126,7 @@ func (b *Local) State() (state.State, error) {
 		}
 	}
 
+	b.state = s
 	return s, nil
 }
 
diff --git a/backend/local/backend_apply.go b/backend/local/backend_apply.go
index 4ba1d27be..381edfb56 100644
--- a/backend/local/backend_apply.go
+++ b/backend/local/backend_apply.go
@@ -8,6 +8,7 @@ import (
 	"github.com/hashicorp/errwrap"
 	"github.com/hashicorp/go-multierror"
 	"github.com/hashicorp/terraform/backend"
+	"github.com/hashicorp/terraform/state"
 	"github.com/hashicorp/terraform/terraform"
 )
 
@@ -28,12 +29,22 @@ func (b *Local) opApply(
 	b.ContextOpts.Hooks = append(b.ContextOpts.Hooks, countHook, stateHook)
 
 	// Get our context
-	tfCtx, state, err := b.context(op)
+	tfCtx, opState, err := b.context(op)
 	if err != nil {
 		runningOp.Err = err
 		return
 	}
 
+	// context acquired the state, and therefor the lock.
+	// Unlock it when the operation is complete
+	defer func() {
+		if s, ok := opState.(state.Locker); op.LockState && ok {
+			if err := s.Unlock(); err != nil {
+				log.Printf("[ERROR]: %s", err)
+			}
+		}
+	}()
+
 	// Setup the state
 	runningOp.State = tfCtx.State()
 
@@ -58,7 +69,7 @@ func (b *Local) opApply(
 	}
 
 	// Setup our hook for continuous state updates
-	stateHook.State = state
+	stateHook.State = opState
 
 	// Start the apply in a goroutine so that we can be interrupted.
 	var applyState *terraform.State
@@ -98,11 +109,11 @@ func (b *Local) opApply(
 	runningOp.State = applyState
 
 	// Persist the state
-	if err := state.WriteState(applyState); err != nil {
+	if err := opState.WriteState(applyState); err != nil {
 		runningOp.Err = fmt.Errorf("Failed to save state: %s", err)
 		return
 	}
-	if err := state.PersistState(); err != nil {
+	if err := opState.PersistState(); err != nil {
 		runningOp.Err = fmt.Errorf("Failed to save state: %s", err)
 		return
 	}
diff --git a/backend/local/backend_local.go b/backend/local/backend_local.go
index 9f5d418d4..f816aeae8 100644
--- a/backend/local/backend_local.go
+++ b/backend/local/backend_local.go
@@ -23,6 +23,13 @@ func (b *Local) context(op *backend.Operation) (*terraform.Context, state.State,
 	if err != nil {
 		return nil, nil, errwrap.Wrapf("Error loading state: {{err}}", err)
 	}
+
+	if s, ok := s.(state.Locker); op.LockState && ok {
+		if err := s.Lock(op.Type.String()); err != nil {
+			return nil, nil, errwrap.Wrapf("Error locking state: {{err}}", err)
+		}
+	}
+
 	if err := s.RefreshState(); err != nil {
 		return nil, nil, errwrap.Wrapf("Error loading state: {{err}}", err)
 	}
diff --git a/backend/local/backend_plan.go b/backend/local/backend_plan.go
index 3598cceca..2c117aae2 100644
--- a/backend/local/backend_plan.go
+++ b/backend/local/backend_plan.go
@@ -11,6 +11,7 @@ import (
 	"github.com/hashicorp/terraform/backend"
 	"github.com/hashicorp/terraform/command/format"
 	"github.com/hashicorp/terraform/config/module"
+	"github.com/hashicorp/terraform/state"
 	"github.com/hashicorp/terraform/terraform"
 )
 
@@ -51,12 +52,22 @@ func (b *Local) opPlan(
 	b.ContextOpts.Hooks = append(b.ContextOpts.Hooks, countHook)
 
 	// Get our context
-	tfCtx, _, err := b.context(op)
+	tfCtx, opState, err := b.context(op)
 	if err != nil {
 		runningOp.Err = err
 		return
 	}
 
+	// context acquired the state, and therefor the lock.
+	// Unlock it when the operation is complete
+	defer func() {
+		if s, ok := opState.(state.Locker); op.LockState && ok {
+			if err := s.Unlock(); err != nil {
+				log.Printf("[ERROR]: %s", err)
+			}
+		}
+	}()
+
 	// Setup the state
 	runningOp.State = tfCtx.State()
 
diff --git a/backend/local/backend_refresh.go b/backend/local/backend_refresh.go
index a79ffd6af..d3f41cc21 100644
--- a/backend/local/backend_refresh.go
+++ b/backend/local/backend_refresh.go
@@ -3,10 +3,12 @@ package local
 import (
 	"context"
 	"fmt"
+	"log"
 	"os"
 
 	"github.com/hashicorp/errwrap"
 	"github.com/hashicorp/terraform/backend"
+	"github.com/hashicorp/terraform/state"
 )
 
 func (b *Local) opRefresh(
@@ -40,14 +42,24 @@ func (b *Local) opRefresh(
 	}
 
 	// Get our context
-	tfCtx, state, err := b.context(op)
+	tfCtx, opState, err := b.context(op)
 	if err != nil {
 		runningOp.Err = err
 		return
 	}
 
+	// context acquired the state, and therefor the lock.
+	// Unlock it when the operation is complete
+	defer func() {
+		if s, ok := opState.(state.Locker); op.LockState && ok {
+			if err := s.Unlock(); err != nil {
+				log.Printf("[ERROR]: %s", err)
+			}
+		}
+	}()
+
 	// Set our state
-	runningOp.State = state.State()
+	runningOp.State = opState.State()
 
 	// Perform operation and write the resulting state to the running op
 	newState, err := tfCtx.Refresh()
@@ -58,11 +70,11 @@ func (b *Local) opRefresh(
 	}
 
 	// Write and persist the state
-	if err := state.WriteState(newState); err != nil {
+	if err := opState.WriteState(newState); err != nil {
 		runningOp.Err = errwrap.Wrapf("Error writing state: {{err}}", err)
 		return
 	}
-	if err := state.PersistState(); err != nil {
+	if err := opState.PersistState(); err != nil {
 		runningOp.Err = errwrap.Wrapf("Error saving state: {{err}}", err)
 		return
 	}
diff --git a/command/apply.go b/command/apply.go
index 0dc51dfa4..05a7e5988 100644
--- a/command/apply.go
+++ b/command/apply.go
@@ -47,6 +47,7 @@ func (c *ApplyCommand) Run(args []string) int {
 	cmdFlags.StringVar(&c.Meta.statePath, "state", "", "path")
 	cmdFlags.StringVar(&c.Meta.stateOutPath, "state-out", "", "path")
 	cmdFlags.StringVar(&c.Meta.backupPath, "backup", "", "path")
+	cmdFlags.BoolVar(&c.Meta.lockState, "state-lock", true, "lock state")
 	cmdFlags.Usage = func() { c.Ui.Error(c.Help()) }
 	if err := cmdFlags.Parse(args); err != nil {
 		return 1
@@ -182,6 +183,7 @@ func (c *ApplyCommand) Run(args []string) int {
 	opReq.Plan = plan
 	opReq.PlanRefresh = refresh
 	opReq.Type = backend.OperationTypeApply
+	opReq.LockState = c.Meta.lockState
 
 	// Perform the operation
 	ctx, ctxCancel := context.WithCancel(context.Background())
diff --git a/command/meta.go b/command/meta.go
index 64361eeab..75e66355d 100644
--- a/command/meta.go
+++ b/command/meta.go
@@ -83,12 +83,15 @@ type Meta struct {
 	// shadow is used to enable/disable the shadow graph
 	//
 	// provider is to specify specific resource providers
+	//
+	// lockState is set to false to disable state locking
 	statePath    string
 	stateOutPath string
 	backupPath   string
 	parallelism  int
 	shadow       bool
 	provider     string
+	lockState    bool
 }
 
 // initStatePaths is used to initialize the default values for

From 1078781487b9e3b198f95486886271ae999ff1d8 Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Thu, 2 Feb 2017 17:27:53 -0500
Subject: [PATCH 02/18] Change lock reason -> info

This makes it more apparent that the information passed in isn't
required nor will it conform to any standard. There may be call sites
that can't provide good contextual info, and we don't want to count on
that value.
---
 state/state.go | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/state/state.go b/state/state.go
index 210eb62c8..532e14872 100644
--- a/state/state.go
+++ b/state/state.go
@@ -42,7 +42,9 @@ type StatePersister interface {
 }
 
 // Locker is implemented to lock state during command execution.
+// The optional info parameter can be recorded with the lock, but the
+// implementation should not depend in its value.
 type Locker interface {
-	Lock(reason string) error
+	Lock(info string) error
 	Unlock() error
 }

From dd19cb202dd41c3230394cc9c0c1f018f788cff4 Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Thu, 2 Feb 2017 18:08:12 -0500
Subject: [PATCH 03/18] add locking to plan and refresh commands

---
 command/plan.go    | 2 ++
 command/refresh.go | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/command/plan.go b/command/plan.go
index 98b01e151..f3aa4355c 100644
--- a/command/plan.go
+++ b/command/plan.go
@@ -31,6 +31,7 @@ func (c *PlanCommand) Run(args []string) int {
 		&c.Meta.parallelism, "parallelism", DefaultParallelism, "parallelism")
 	cmdFlags.StringVar(&c.Meta.statePath, "state", "", "path")
 	cmdFlags.BoolVar(&detailed, "detailed-exitcode", false, "detailed-exitcode")
+	cmdFlags.BoolVar(&c.Meta.lockState, "state-lock", true, "lock state")
 	cmdFlags.Usage = func() { c.Ui.Error(c.Help()) }
 	if err := cmdFlags.Parse(args); err != nil {
 		return 1
@@ -84,6 +85,7 @@ func (c *PlanCommand) Run(args []string) int {
 	opReq.PlanRefresh = refresh
 	opReq.PlanOutPath = outPath
 	opReq.Type = backend.OperationTypePlan
+	opReq.LockState = c.Meta.lockState
 
 	// Perform the operation
 	op, err := b.Operation(context.Background(), opReq)
diff --git a/command/refresh.go b/command/refresh.go
index 194e95a33..6898d1302 100644
--- a/command/refresh.go
+++ b/command/refresh.go
@@ -23,6 +23,7 @@ func (c *RefreshCommand) Run(args []string) int {
 	cmdFlags.IntVar(&c.Meta.parallelism, "parallelism", 0, "parallelism")
 	cmdFlags.StringVar(&c.Meta.stateOutPath, "state-out", "", "path")
 	cmdFlags.StringVar(&c.Meta.backupPath, "backup", "", "path")
+	cmdFlags.BoolVar(&c.Meta.lockState, "state-lock", true, "lock state")
 	cmdFlags.Usage = func() { c.Ui.Error(c.Help()) }
 	if err := cmdFlags.Parse(args); err != nil {
 		return 1
@@ -52,6 +53,7 @@ func (c *RefreshCommand) Run(args []string) int {
 	opReq := c.Operation()
 	opReq.Type = backend.OperationTypeRefresh
 	opReq.Module = mod
+	opReq.LockState = c.Meta.lockState
 
 	// Perform the operation
 	op, err := b.Operation(context.Background(), opReq)

From 94f2f4d6ae93e0fbf869c98b39055a30d0009e7f Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Fri, 3 Feb 2017 13:07:34 -0500
Subject: [PATCH 04/18] Create state files first for backup tests

Previously when runnign a plan with no exitsing state, the plan would be
written out and then backed up on the next WriteState by another
BackupState instance. Since we now maintain a single State instance
thoughout an operation, the backup happens before any state exists so no
backup file is created.

This is OK, as the backup state the tests were checking for is from the
plan file, which already exists separate from the state.
---
 command/apply_test.go        | 10 +++++++++-
 command/meta_backend_test.go |  7 +++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/command/apply_test.go b/command/apply_test.go
index 77667ae3e..72d52ddae 100644
--- a/command/apply_test.go
+++ b/command/apply_test.go
@@ -15,6 +15,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/hashicorp/terraform/state"
 	"github.com/hashicorp/terraform/terraform"
 	"github.com/mitchellh/cli"
 )
@@ -550,7 +551,8 @@ func TestApply_plan(t *testing.T) {
 }
 
 func TestApply_plan_backup(t *testing.T) {
-	planPath := testPlanFile(t, testPlan(t))
+	plan := testPlan(t)
+	planPath := testPlanFile(t, plan)
 	statePath := testTempFile(t)
 	backupPath := testTempFile(t)
 
@@ -563,6 +565,12 @@ func TestApply_plan_backup(t *testing.T) {
 		},
 	}
 
+	// create a state file that needs to be backed up
+	err := (&state.LocalState{Path: statePath}).WriteState(plan.State)
+	if err != nil {
+		t.Fatal(err)
+	}
+
 	args := []string{
 		"-state-out", statePath,
 		"-backup", backupPath,
diff --git a/command/meta_backend_test.go b/command/meta_backend_test.go
index c9cff296a..e73fa6b31 100644
--- a/command/meta_backend_test.go
+++ b/command/meta_backend_test.go
@@ -8,6 +8,7 @@ import (
 	"testing"
 
 	"github.com/hashicorp/terraform/helper/copy"
+	"github.com/hashicorp/terraform/state"
 	"github.com/hashicorp/terraform/terraform"
 	"github.com/mitchellh/cli"
 )
@@ -2208,6 +2209,12 @@ func TestMetaBackend_planLocalStatePath(t *testing.T) {
 	// Create an alternate output path
 	statePath := "foo.tfstate"
 
+	// put a initial state there that needs to be backed up
+	err := (&state.LocalState{Path: statePath}).WriteState(original)
+	if err != nil {
+		t.Fatal(err)
+	}
+
 	// Setup the meta
 	m := testMetaBackend(t, nil)
 	m.stateOutPath = statePath

From a157ebbccd7d920852625d2c04b66d1c62c79bb2 Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Fri, 3 Feb 2017 14:15:08 -0500
Subject: [PATCH 05/18] add -lock-state usage to plan/refresh/apply/destr

---
 command/apply.go   | 4 ++++
 command/plan.go    | 2 ++
 command/refresh.go | 2 ++
 3 files changed, 8 insertions(+)

diff --git a/command/apply.go b/command/apply.go
index 05a7e5988..4e29262a9 100644
--- a/command/apply.go
+++ b/command/apply.go
@@ -274,6 +274,8 @@ Options:
                          modifying. Defaults to the "-state-out" path with
                          ".backup" extension. Set to "-" to disable backup.
 
+  -lock-state=true       Lock the state file when locking is supported.
+
   -input=true            Ask for input for variables if not directly set.
 
   -no-color              If specified, output won't contain any color.
@@ -321,6 +323,8 @@ Options:
 
   -force                 Don't ask for input for destroy confirmation.
 
+  -lock-state=true       Lock the state file when locking is supported.
+
   -no-color              If specified, output won't contain any color.
 
   -parallelism=n         Limit the number of concurrent operations.
diff --git a/command/plan.go b/command/plan.go
index f3aa4355c..0bae85292 100644
--- a/command/plan.go
+++ b/command/plan.go
@@ -143,6 +143,8 @@ Options:
 
   -input=true         Ask for input for variables if not directly set.
 
+  -lock-state=true    Lock the state file when locking is supported.
+
   -module-depth=n     Specifies the depth of modules to show in the output.
                       This does not affect the plan itself, only the output
                       shown. By default, this is -1, which will expand all.
diff --git a/command/refresh.go b/command/refresh.go
index 6898d1302..f51386648 100644
--- a/command/refresh.go
+++ b/command/refresh.go
@@ -96,6 +96,8 @@ Options:
 
   -input=true         Ask for input for variables if not directly set.
 
+  -lock-state=true    Lock the state file when locking is supported.
+
   -no-color           If specified, output won't contain any color.
 
   -state=path         Path to read and save state (unless state-out

From 91608843a4f3ba0fec01d73ec3e93b4f03151a4d Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Fri, 3 Feb 2017 14:23:24 -0500
Subject: [PATCH 06/18] Add state locking in taint/untaint

---
 command/taint.go   | 21 +++++++++++++++++----
 command/untaint.go | 21 +++++++++++++++++----
 2 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/command/taint.go b/command/taint.go
index 1d60729a6..6d652fdf4 100644
--- a/command/taint.go
+++ b/command/taint.go
@@ -5,6 +5,7 @@ import (
 	"log"
 	"strings"
 
+	"github.com/hashicorp/terraform/state"
 	"github.com/hashicorp/terraform/terraform"
 )
 
@@ -25,6 +26,7 @@ func (c *TaintCommand) Run(args []string) int {
 	cmdFlags.StringVar(&c.Meta.statePath, "state", DefaultStateFilename, "path")
 	cmdFlags.StringVar(&c.Meta.stateOutPath, "state-out", "", "path")
 	cmdFlags.StringVar(&c.Meta.backupPath, "backup", "", "path")
+	cmdFlags.BoolVar(&c.Meta.lockState, "state-lock", true, "lock state")
 	cmdFlags.Usage = func() { c.Ui.Error(c.Help()) }
 	if err := cmdFlags.Parse(args); err != nil {
 		return 1
@@ -64,14 +66,23 @@ func (c *TaintCommand) Run(args []string) int {
 	}
 
 	// Get the state
-	state, err := b.State()
+	st, err := b.State()
 	if err != nil {
 		c.Ui.Error(fmt.Sprintf("Failed to load state: %s", err))
 		return 1
 	}
 
+	if s, ok := st.(state.Locker); c.Meta.lockState && ok {
+		if err := s.Lock("taint"); err != nil {
+			c.Ui.Error(fmt.Sprintf("Failed to lock state: %s", err))
+			return 1
+		}
+
+		defer s.Unlock()
+	}
+
 	// Get the actual state structure
-	s := state.State()
+	s := st.State()
 	if s.Empty() {
 		if allowMissing {
 			return c.allowMissingExit(name, module)
@@ -129,11 +140,11 @@ func (c *TaintCommand) Run(args []string) int {
 	rs.Taint()
 
 	log.Printf("[INFO] Writing state output to: %s", c.Meta.StateOutPath())
-	if err := state.WriteState(s); err != nil {
+	if err := st.WriteState(s); err != nil {
 		c.Ui.Error(fmt.Sprintf("Error writing state file: %s", err))
 		return 1
 	}
-	if err := state.PersistState(); err != nil {
+	if err := st.PersistState(); err != nil {
 		c.Ui.Error(fmt.Sprintf("Error writing state file: %s", err))
 		return 1
 	}
@@ -166,6 +177,8 @@ Options:
                       modifying. Defaults to the "-state-out" path with
                       ".backup" extension. Set to "-" to disable backup.
 
+  -lock-state=true    Lock the state file when locking is supported.
+
   -module=path        The module path where the resource lives. By
                       default this will be root. Child modules can be specified
                       by names. Ex. "consul" or "consul.vpc" (nested modules).
diff --git a/command/untaint.go b/command/untaint.go
index 4be0590fa..925a73fc3 100644
--- a/command/untaint.go
+++ b/command/untaint.go
@@ -4,6 +4,8 @@ import (
 	"fmt"
 	"log"
 	"strings"
+
+	"github.com/hashicorp/terraform/state"
 )
 
 // UntaintCommand is a cli.Command implementation that manually untaints
@@ -51,14 +53,23 @@ func (c *UntaintCommand) Run(args []string) int {
 	}
 
 	// Get the state
-	state, err := b.State()
+	st, err := b.State()
 	if err != nil {
 		c.Ui.Error(fmt.Sprintf("Failed to load state: %s", err))
 		return 1
 	}
 
+	if s, ok := st.(state.Locker); c.Meta.lockState && ok {
+		if err := s.Lock("untaint"); err != nil {
+			c.Ui.Error(fmt.Sprintf("Failed to lock state: %s", err))
+			return 1
+		}
+
+		defer s.Unlock()
+	}
+
 	// Get the actual state structure
-	s := state.State()
+	s := st.State()
 	if s.Empty() {
 		if allowMissing {
 			return c.allowMissingExit(name, module)
@@ -116,11 +127,11 @@ func (c *UntaintCommand) Run(args []string) int {
 	rs.Untaint()
 
 	log.Printf("[INFO] Writing state output to: %s", c.Meta.StateOutPath())
-	if err := state.WriteState(s); err != nil {
+	if err := st.WriteState(s); err != nil {
 		c.Ui.Error(fmt.Sprintf("Error writing state file: %s", err))
 		return 1
 	}
-	if err := state.PersistState(); err != nil {
+	if err := st.PersistState(); err != nil {
 		c.Ui.Error(fmt.Sprintf("Error writing state file: %s", err))
 		return 1
 	}
@@ -153,6 +164,8 @@ Options:
                       modifying. Defaults to the "-state-out" path with
                       ".backup" extension. Set to "-" to disable backup.
 
+  -lock-state=true    Lock the state file when locking is supported.
+
   -module=path        The module path where the resource lives. By
                       default this will be root. Child modules can be specified
                       by names. Ex. "consul" or "consul.vpc" (nested modules).

From a2b5811f50731f58a1766cbfcb4ceb7d094fe85d Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Fri, 3 Feb 2017 14:47:08 -0500
Subject: [PATCH 07/18] Remove "expires" from lock info.

We are not going to handle lock expiration, at least at this time, so
remove the Expires fields to avoid any confusion.
---
 state/local.go     |  9 +++------
 state/remote/s3.go | 12 ++++--------
 2 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/state/local.go b/state/local.go
index 01d696442..e1129ebbe 100644
--- a/state/local.go
+++ b/state/local.go
@@ -19,16 +19,14 @@ type lockInfo struct {
 	Path string
 	// The time the lock was taken
 	Created time.Time
-	// The time this lock expires
-	Expires time.Time
-	// The lock reason passed to State.Lock
+	// Extra info passed to State.Lock
 	Reason string
 }
 
 // return the lock info formatted in an error
 func (l *lockInfo) Err() error {
-	return fmt.Errorf("state file %q locked. created:%s, expires:%s, reason:%s",
-		l.Path, l.Created, l.Expires, l.Reason)
+	return fmt.Errorf("state file %q locked. created:%s, reason:%s",
+		l.Path, l.Created, l.Reason)
 }
 
 // LocalState manages a state storage that is local to the filesystem.
@@ -229,7 +227,6 @@ func (s *LocalState) writeLockInfo(reason string) error {
 	lockInfo := &lockInfo{
 		Path:    s.Path,
 		Created: time.Now().UTC(),
-		Expires: time.Now().Add(time.Hour).UTC(),
 		Reason:  reason,
 	}
 
diff --git a/state/remote/s3.go b/state/remote/s3.go
index 772737e11..367335865 100644
--- a/state/remote/s3.go
+++ b/state/remote/s3.go
@@ -207,7 +207,6 @@ func (c *S3Client) Lock(reason string) error {
 		Item: map[string]*dynamodb.AttributeValue{
 			"LockID":  {S: aws.String(stateName)},
 			"Created": {S: aws.String(time.Now().UTC().Format(time.RFC3339))},
-			"Expires": {S: aws.String(time.Now().Add(time.Hour).UTC().Format(time.RFC3339))},
 			"Info":    {S: aws.String(reason)},
 		},
 		TableName:           aws.String(c.lockTable),
@@ -220,7 +219,7 @@ func (c *S3Client) Lock(reason string) error {
 			Key: map[string]*dynamodb.AttributeValue{
 				"LockID": {S: aws.String(fmt.Sprintf("%s/%s", c.bucketName, c.keyName))},
 			},
-			ProjectionExpression: aws.String("LockID, Created, Expires, Info"),
+			ProjectionExpression: aws.String("LockID, Created, Info"),
 			TableName:            aws.String(c.lockTable),
 		}
 
@@ -229,19 +228,16 @@ func (c *S3Client) Lock(reason string) error {
 			return fmt.Errorf("s3 state file %q locked, cfailed to retrive info: %s", stateName, err)
 		}
 
-		var created, expires, info string
+		var created, info string
 		if v, ok := resp.Item["Created"]; ok && v.S != nil {
 			created = *v.S
 		}
-		if v, ok := resp.Item["Expires"]; ok && v.S != nil {
-			expires = *v.S
-		}
 		if v, ok := resp.Item["Info"]; ok && v.S != nil {
 			info = *v.S
 		}
 
-		return fmt.Errorf("state file %q locked. created:%s, expires:%s, reason:%s",
-			stateName, created, expires, info)
+		return fmt.Errorf("state file %q locked. created:%s, reason:%s",
+			stateName, created, info)
 
 	}
 	return nil

From 6a20c35d613fb4c581f6bde2249010b93e36f27a Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Fri, 3 Feb 2017 14:55:13 -0500
Subject: [PATCH 08/18] apply-test

---
 command/apply_test.go | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/command/apply_test.go b/command/apply_test.go
index 72d52ddae..b50b9cd14 100644
--- a/command/apply_test.go
+++ b/command/apply_test.go
@@ -8,10 +8,12 @@ import (
 	"net/http"
 	"net/url"
 	"os"
+	"os/exec"
 	"path/filepath"
 	"reflect"
 	"strings"
 	"sync"
+	"syscall"
 	"testing"
 	"time"
 
@@ -59,6 +61,36 @@ func TestApply(t *testing.T) {
 	}
 }
 
+// test apply with locked state
+func TestApply_lockedState(t *testing.T) {
+	statePath := testTempFile(t)
+
+	locker := exec.Command("go", "run", "testadata/statelocker.go", statePath)
+	locker.Stderr = os.Stderr
+	if err := locker.Start(); err != nil {
+		t.Fatal(err)
+	}
+
+	defer locker.Process.Signal(syscall.SIGTERM)
+
+	p := testProvider()
+	ui := new(cli.MockUi)
+	c := &ApplyCommand{
+		Meta: Meta{
+			ContextOpts: testCtxConfig(p),
+			Ui:          ui,
+		},
+	}
+
+	args := []string{
+		"-state", statePath,
+		testFixturePath("apply"),
+	}
+	if code := c.Run(args); code != 0 {
+		t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String())
+	}
+}
+
 // high water mark counter
 type hwm struct {
 	sync.Mutex

From fb60b6f6f293697c4ce12974251b4146b03362ea Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Fri, 3 Feb 2017 15:31:21 -0500
Subject: [PATCH 09/18] Add separate program for locking state files

Depending on the implementation, local state locks may be reentrant
within the same process. Use a separate process to test locked state
files.
---
 command/testdata/statelocker.go | 43 +++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)
 create mode 100644 command/testdata/statelocker.go

diff --git a/command/testdata/statelocker.go b/command/testdata/statelocker.go
new file mode 100644
index 000000000..a70e3d6da
--- /dev/null
+++ b/command/testdata/statelocker.go
@@ -0,0 +1,43 @@
+// statelocker use used for testing command with a locked state.
+// This will lock the state file at a given path, then wait for a sigal. On
+// SIGINT and SIGTERM the state will be Unlocked before exit.
+package main
+
+import (
+	"io"
+	"log"
+	"os"
+	"os/signal"
+	"syscall"
+
+	"github.com/hashicorp/terraform/state"
+)
+
+func main() {
+	if len(os.Args) != 2 {
+		log.Fatal(os.Args[0], "statefile")
+	}
+
+	s := &state.LocalState{
+		Path: os.Args[1],
+	}
+
+	err := s.Lock("command test")
+	if err != nil {
+		io.WriteString(os.Stderr, err.Error())
+		return
+	}
+
+	// signal to the caller that we're locked
+	io.WriteString(os.Stdout, "LOCKED")
+
+	defer func() {
+		if err := s.Unlock(); err != nil {
+			io.WriteString(os.Stderr, err.Error())
+		}
+	}()
+
+	c := make(chan os.Signal, 1)
+	signal.Notify(c, syscall.SIGINT, syscall.SIGTERM)
+	<-c
+}

From bd65ddbcaaf83d2a865341d0412b229b370d340e Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Fri, 3 Feb 2017 15:32:40 -0500
Subject: [PATCH 10/18] Add test for apply/refresh on locked state files

Verify that these operations fail when a state file is locked.
---
 command/apply_test.go   | 19 ++++++++++---------
 command/command_test.go | 37 +++++++++++++++++++++++++++++++++++++
 command/refresh_test.go | 37 +++++++++++++++++++++++++++++++++++++
 3 files changed, 84 insertions(+), 9 deletions(-)

diff --git a/command/apply_test.go b/command/apply_test.go
index b50b9cd14..3981b01ad 100644
--- a/command/apply_test.go
+++ b/command/apply_test.go
@@ -8,12 +8,10 @@ import (
 	"net/http"
 	"net/url"
 	"os"
-	"os/exec"
 	"path/filepath"
 	"reflect"
 	"strings"
 	"sync"
-	"syscall"
 	"testing"
 	"time"
 
@@ -65,13 +63,11 @@ func TestApply(t *testing.T) {
 func TestApply_lockedState(t *testing.T) {
 	statePath := testTempFile(t)
 
-	locker := exec.Command("go", "run", "testadata/statelocker.go", statePath)
-	locker.Stderr = os.Stderr
-	if err := locker.Start(); err != nil {
+	unlock, err := testLockState(statePath)
+	if err != nil {
 		t.Fatal(err)
 	}
-
-	defer locker.Process.Signal(syscall.SIGTERM)
+	defer unlock()
 
 	p := testProvider()
 	ui := new(cli.MockUi)
@@ -86,8 +82,13 @@ func TestApply_lockedState(t *testing.T) {
 		"-state", statePath,
 		testFixturePath("apply"),
 	}
-	if code := c.Run(args); code != 0 {
-		t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String())
+	if code := c.Run(args); code == 0 {
+		t.Fatal("expected error")
+	}
+
+	output := ui.ErrorWriter.String()
+	if !strings.Contains(output, "locked") {
+		t.Fatal("command output does not look like a lock error:", output)
 	}
 }
 
diff --git a/command/command_test.go b/command/command_test.go
index 7be56bcda..9418e8571 100644
--- a/command/command_test.go
+++ b/command/command_test.go
@@ -6,14 +6,17 @@ import (
 	"encoding/base64"
 	"encoding/json"
 	"flag"
+	"fmt"
 	"io"
 	"io/ioutil"
 	"log"
 	"net/http"
 	"net/http/httptest"
 	"os"
+	"os/exec"
 	"path/filepath"
 	"strings"
+	"syscall"
 	"testing"
 
 	"github.com/hashicorp/go-getter"
@@ -529,3 +532,37 @@ func testRemoteState(t *testing.T, s *terraform.State, c int) (*terraform.Remote
 
 	return remote, srv
 }
+
+// testlockState calls a separate process to the lock the state file at path.
+// deferFunc should be called in the caller to properly unlock the file.
+func testLockState(path string) (func(), error) {
+	locker := exec.Command("go", "run", "testdata/statelocker.go", path)
+	pr, pw, err := os.Pipe()
+	if err != nil {
+		return nil, err
+	}
+	defer pr.Close()
+	defer pw.Close()
+	locker.Stderr = pw
+	locker.Stdout = pw
+
+	if err := locker.Start(); err != nil {
+		return nil, err
+	}
+	deferFunc := func() {
+		locker.Process.Signal(syscall.SIGTERM)
+		locker.Wait()
+	}
+
+	// wait for the process to lock
+	buf := make([]byte, 1024)
+	n, err := pr.Read(buf)
+	if err != nil {
+		return deferFunc, fmt.Errorf("read from statelocker returned: %s", err)
+	}
+
+	if string(buf[:n]) != "LOCKED" {
+		return deferFunc, fmt.Errorf("statelocker wrote", string(buf[:n]))
+	}
+	return deferFunc, nil
+}
diff --git a/command/refresh_test.go b/command/refresh_test.go
index b5bfe7950..4c79aec68 100644
--- a/command/refresh_test.go
+++ b/command/refresh_test.go
@@ -59,6 +59,43 @@ func TestRefresh(t *testing.T) {
 	}
 }
 
+func TestRefresh_lockedState(t *testing.T) {
+	state := testState()
+	statePath := testStateFile(t, state)
+
+	unlock, err := testLockState(statePath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer unlock()
+
+	p := testProvider()
+	ui := new(cli.MockUi)
+	c := &RefreshCommand{
+		Meta: Meta{
+			ContextOpts: testCtxConfig(p),
+			Ui:          ui,
+		},
+	}
+
+	p.RefreshFn = nil
+	p.RefreshReturn = &terraform.InstanceState{ID: "yes"}
+
+	args := []string{
+		"-state", statePath,
+		testFixturePath("refresh"),
+	}
+
+	if code := c.Run(args); code == 0 {
+		t.Fatal("expected error")
+	}
+
+	output := ui.ErrorWriter.String()
+	if !strings.Contains(output, "locked") {
+		t.Fatal("command output does not look like a lock error:", output)
+	}
+}
+
 func TestRefresh_badState(t *testing.T) {
 	p := testProvider()
 	ui := new(cli.MockUi)

From f3e4c05250654fb536e7249e9a2dce77b8722ade Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Fri, 3 Feb 2017 15:56:19 -0500
Subject: [PATCH 11/18] build the statelocker binary before running

this way we can signal it directly to amke sure it exits cleanly.
---
 command/command_test.go         | 22 ++++++++++++++++++++--
 command/testdata/statelocker.go | 10 ++++++++--
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/command/command_test.go b/command/command_test.go
index 9418e8571..d3bded69a 100644
--- a/command/command_test.go
+++ b/command/command_test.go
@@ -536,9 +536,26 @@ func testRemoteState(t *testing.T, s *terraform.State, c int) (*terraform.Remote
 // testlockState calls a separate process to the lock the state file at path.
 // deferFunc should be called in the caller to properly unlock the file.
 func testLockState(path string) (func(), error) {
-	locker := exec.Command("go", "run", "testdata/statelocker.go", path)
+	// build and run the binary ourselves so we can quickly terminate it for cleanup
+	buildDir, err := ioutil.TempDir("", "locker")
+	if err != nil {
+		return nil, err
+	}
+	cleanFunc := func() {
+		os.RemoveAll(buildDir)
+	}
+
+	lockBin := filepath.Join(buildDir, "statelocker")
+	out, err := exec.Command("go", "build", "-o", lockBin, "testdata/statelocker.go").CombinedOutput()
+	if err != nil {
+		cleanFunc()
+		return nil, fmt.Errorf("%s %s", err, out)
+	}
+
+	locker := exec.Command(lockBin, path)
 	pr, pw, err := os.Pipe()
 	if err != nil {
+		cleanFunc()
 		return nil, err
 	}
 	defer pr.Close()
@@ -550,6 +567,7 @@ func testLockState(path string) (func(), error) {
 		return nil, err
 	}
 	deferFunc := func() {
+		cleanFunc()
 		locker.Process.Signal(syscall.SIGTERM)
 		locker.Wait()
 	}
@@ -562,7 +580,7 @@ func testLockState(path string) (func(), error) {
 	}
 
 	if string(buf[:n]) != "LOCKED" {
-		return deferFunc, fmt.Errorf("statelocker wrote", string(buf[:n]))
+		return deferFunc, fmt.Errorf("statelocker wrote: %s", string(buf[:n]))
 	}
 	return deferFunc, nil
 }
diff --git a/command/testdata/statelocker.go b/command/testdata/statelocker.go
index a70e3d6da..8f25f9d33 100644
--- a/command/testdata/statelocker.go
+++ b/command/testdata/statelocker.go
@@ -9,6 +9,7 @@ import (
 	"os"
 	"os/signal"
 	"syscall"
+	"time"
 
 	"github.com/hashicorp/terraform/state"
 )
@@ -38,6 +39,11 @@ func main() {
 	}()
 
 	c := make(chan os.Signal, 1)
-	signal.Notify(c, syscall.SIGINT, syscall.SIGTERM)
-	<-c
+	signal.Notify(c, syscall.SIGINT, syscall.SIGTERM, syscall.SIGHUP)
+
+	// timeout after 10 second in case we don't get cleaned up by the test
+	select {
+	case <-time.After(10 * time.Second):
+	case <-c:
+	}
 }

From 9fa436e0bd30fdecdd7d051dce1bd8c0c0b5c1ea Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Fri, 3 Feb 2017 16:02:22 -0500
Subject: [PATCH 12/18] Add test for locked state in plan

---
 command/plan_test.go | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/command/plan_test.go b/command/plan_test.go
index 3f0e2cf26..b9e25bf22 100644
--- a/command/plan_test.go
+++ b/command/plan_test.go
@@ -39,6 +39,44 @@ func TestPlan(t *testing.T) {
 	}
 }
 
+func TestPlan_lockedState(t *testing.T) {
+	cwd, err := os.Getwd()
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	testPath := testFixturePath("plan")
+	unlock, err := testLockState(filepath.Join(testPath, DefaultStateFilename))
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer unlock()
+
+	if err := os.Chdir(testPath); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+	defer os.Chdir(cwd)
+
+	p := testProvider()
+	ui := new(cli.MockUi)
+	c := &PlanCommand{
+		Meta: Meta{
+			ContextOpts: testCtxConfig(p),
+			Ui:          ui,
+		},
+	}
+
+	args := []string{}
+	if code := c.Run(args); code == 0 {
+		t.Fatal("expected error")
+	}
+
+	output := ui.ErrorWriter.String()
+	if !strings.Contains(output, "locked") {
+		t.Fatal("command output does not look like a lock error:", output)
+	}
+}
+
 func TestPlan_plan(t *testing.T) {
 	tmp, cwd := testCwd(t)
 	defer testFixCwd(t, tmp, cwd)

From 82e59cd8268c9d1ab8da21823b596a18d722fdd0 Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Fri, 3 Feb 2017 16:06:01 -0500
Subject: [PATCH 13/18] Add test for destroy with locked state

---
 command/apply_destroy_test.go | 52 +++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/command/apply_destroy_test.go b/command/apply_destroy_test.go
index b82ffdf47..6e1277cca 100644
--- a/command/apply_destroy_test.go
+++ b/command/apply_destroy_test.go
@@ -92,6 +92,58 @@ func TestApply_destroy(t *testing.T) {
 	}
 }
 
+func TestApply_destroyLockedState(t *testing.T) {
+	originalState := &terraform.State{
+		Modules: []*terraform.ModuleState{
+			&terraform.ModuleState{
+				Path: []string{"root"},
+				Resources: map[string]*terraform.ResourceState{
+					"test_instance.foo": &terraform.ResourceState{
+						Type: "test_instance",
+						Primary: &terraform.InstanceState{
+							ID: "bar",
+						},
+					},
+				},
+			},
+		},
+	}
+
+	statePath := testStateFile(t, originalState)
+
+	unlock, err := testLockState(statePath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer unlock()
+
+	p := testProvider()
+	ui := new(cli.MockUi)
+	c := &ApplyCommand{
+		Destroy: true,
+		Meta: Meta{
+			ContextOpts: testCtxConfig(p),
+			Ui:          ui,
+		},
+	}
+
+	// Run the apply command pointing to our existing state
+	args := []string{
+		"-force",
+		"-state", statePath,
+		testFixturePath("apply"),
+	}
+
+	if code := c.Run(args); code == 0 {
+		t.Fatal("expected error")
+	}
+
+	output := ui.ErrorWriter.String()
+	if !strings.Contains(output, "locked") {
+		t.Fatal("command output does not look like a lock error:", output)
+	}
+}
+
 func TestApply_destroyPlan(t *testing.T) {
 	planPath := testPlanFile(t, &terraform.Plan{
 		Module: testModule(t, "apply"),

From cd96bb5aca3b118b54f6588d34fb60aff57691b9 Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Fri, 3 Feb 2017 16:13:42 -0500
Subject: [PATCH 14/18] Add test/untaint tests with locked state

add missing lock-state flag to untaint
---
 command/taint_test.go   | 45 +++++++++++++++++++++++++++++++++++++++++
 command/untaint.go      |  1 +
 command/untaint_test.go | 45 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 91 insertions(+)

diff --git a/command/taint_test.go b/command/taint_test.go
index c2ad652fa..26c42b6a5 100644
--- a/command/taint_test.go
+++ b/command/taint_test.go
@@ -2,6 +2,7 @@ package command
 
 import (
 	"os"
+	"strings"
 	"testing"
 
 	"github.com/hashicorp/terraform/terraform"
@@ -44,6 +45,50 @@ func TestTaint(t *testing.T) {
 	testStateOutput(t, statePath, testTaintStr)
 }
 
+func TestTaint_lockedState(t *testing.T) {
+	state := &terraform.State{
+		Modules: []*terraform.ModuleState{
+			&terraform.ModuleState{
+				Path: []string{"root"},
+				Resources: map[string]*terraform.ResourceState{
+					"test_instance.foo": &terraform.ResourceState{
+						Type: "test_instance",
+						Primary: &terraform.InstanceState{
+							ID: "bar",
+						},
+					},
+				},
+			},
+		},
+	}
+	statePath := testStateFile(t, state)
+
+	unlock, err := testLockState(statePath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer unlock()
+	ui := new(cli.MockUi)
+	c := &TaintCommand{
+		Meta: Meta{
+			Ui: ui,
+		},
+	}
+
+	args := []string{
+		"-state", statePath,
+		"test_instance.foo",
+	}
+	if code := c.Run(args); code == 0 {
+		t.Fatal("expected error")
+	}
+
+	output := ui.ErrorWriter.String()
+	if !strings.Contains(output, "locked") {
+		t.Fatal("command output does not look like a lock error:", output)
+	}
+}
+
 func TestTaint_backup(t *testing.T) {
 	// Get a temp cwd
 	tmp, cwd := testCwd(t)
diff --git a/command/untaint.go b/command/untaint.go
index 925a73fc3..6612d1940 100644
--- a/command/untaint.go
+++ b/command/untaint.go
@@ -25,6 +25,7 @@ func (c *UntaintCommand) Run(args []string) int {
 	cmdFlags.StringVar(&c.Meta.statePath, "state", DefaultStateFilename, "path")
 	cmdFlags.StringVar(&c.Meta.stateOutPath, "state-out", "", "path")
 	cmdFlags.StringVar(&c.Meta.backupPath, "backup", "", "path")
+	cmdFlags.BoolVar(&c.Meta.lockState, "state-lock", true, "lock state")
 	cmdFlags.Usage = func() { c.Ui.Error(c.Help()) }
 	if err := cmdFlags.Parse(args); err != nil {
 		return 1
diff --git a/command/untaint_test.go b/command/untaint_test.go
index 4923a6f7c..ed63de78d 100644
--- a/command/untaint_test.go
+++ b/command/untaint_test.go
@@ -50,6 +50,51 @@ test_instance.foo:
 	testStateOutput(t, statePath, expected)
 }
 
+func TestUntaint_lockedState(t *testing.T) {
+	state := &terraform.State{
+		Modules: []*terraform.ModuleState{
+			&terraform.ModuleState{
+				Path: []string{"root"},
+				Resources: map[string]*terraform.ResourceState{
+					"test_instance.foo": &terraform.ResourceState{
+						Type: "test_instance",
+						Primary: &terraform.InstanceState{
+							ID:      "bar",
+							Tainted: true,
+						},
+					},
+				},
+			},
+		},
+	}
+	statePath := testStateFile(t, state)
+	unlock, err := testLockState(statePath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer unlock()
+
+	ui := new(cli.MockUi)
+	c := &UntaintCommand{
+		Meta: Meta{
+			Ui: ui,
+		},
+	}
+
+	args := []string{
+		"-state", statePath,
+		"test_instance.foo",
+	}
+	if code := c.Run(args); code == 0 {
+		t.Fatal("expected error")
+	}
+
+	output := ui.ErrorWriter.String()
+	if !strings.Contains(output, "locked") {
+		t.Fatal("command output does not look like a lock error:", output)
+	}
+}
+
 func TestUntaint_backup(t *testing.T) {
 	// Get a temp cwd
 	tmp, cwd := testCwd(t)

From e92559f5182b199a2cf26b5fdbf4c528eb3fa243 Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Fri, 3 Feb 2017 18:58:18 -0500
Subject: [PATCH 15/18] Cleanup state file during Unlock

Close and remove the file descriptor from LocalState if we Unlock the
state. Also remove an empty state file if we created it and it was never
written to. This is mostly to clean up after tests, but doesn't hurt to
not leave empty files around.
---
 state/local.go | 51 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 35 insertions(+), 16 deletions(-)

diff --git a/state/local.go b/state/local.go
index e1129ebbe..b34d04837 100644
--- a/state/local.go
+++ b/state/local.go
@@ -39,6 +39,10 @@ type LocalState struct {
 
 	// the file handle corresponding to PathOut
 	stateFileOut *os.File
+	// created is set to tru if stateFileOut didn't exist before we created it.
+	// This is mostly so we can clean up emtpy files during tests, but doesn't
+	// hurt to remove file we never wrote to.
+	created bool
 
 	state     *terraform.State
 	readState *terraform.State
@@ -75,8 +79,26 @@ func (s *LocalState) Lock(reason string) error {
 }
 
 func (s *LocalState) Unlock() error {
+	// we can't be locked if we don't have a file
+	if s.stateFileOut == nil {
+		return nil
+	}
+
 	os.Remove(s.lockInfoPath())
-	return s.unlock()
+
+	fileName := s.stateFileOut.Name()
+
+	unlockErr := s.unlock()
+	s.stateFileOut.Close()
+	s.stateFileOut = nil
+
+	// clean up the state file if we created it an never wrote to it
+	stat, err := os.Stat(fileName)
+	if err == nil && stat.Size() == 0 && s.created {
+		os.Remove(fileName)
+	}
+
+	return unlockErr
 }
 
 // Open the state file, creating the directories and file as needed.
@@ -85,28 +107,25 @@ func (s *LocalState) createStateFiles() error {
 		s.PathOut = s.Path
 	}
 
-	f, err := createFileAndDirs(s.PathOut)
+	// yes this could race, but we only use it to clean up empty files
+	if _, err := os.Stat(s.PathOut); os.IsNotExist(err) {
+		s.created = true
+	}
+
+	// Create all the directories
+	if err := os.MkdirAll(filepath.Dir(s.PathOut), 0755); err != nil {
+		return err
+	}
+
+	f, err := os.OpenFile(s.PathOut, os.O_RDWR|os.O_CREATE, 0666)
 	if err != nil {
 		return err
 	}
+
 	s.stateFileOut = f
 	return nil
 }
 
-func createFileAndDirs(path string) (*os.File, error) {
-	// Create all the directories
-	if err := os.MkdirAll(filepath.Dir(path), 0755); err != nil {
-		return nil, err
-	}
-
-	f, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, 0666)
-	if err != nil {
-		return nil, err
-	}
-
-	return f, nil
-}
-
 // WriteState for LocalState always persists the state as well.
 // TODO: this should use a more robust method of writing state, by first
 // writing to a temp file on the same filesystem, and renaming the file over

From 0d7752b0f501f409aa4fe422b8dbe1cf94048db1 Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Mon, 6 Feb 2017 09:54:15 -0500
Subject: [PATCH 16/18] Update runningOp.Err with State.Unlock error

Have the defer'ed State.Unlock call append any error to the
RunningOperation.Err field. Local error would be rare and
self-correcting, but when the backend.Local is using a remote state the
error may require user intervention.
---
 backend/local/backend_apply.go | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/backend/local/backend_apply.go b/backend/local/backend_apply.go
index 381edfb56..4c0804717 100644
--- a/backend/local/backend_apply.go
+++ b/backend/local/backend_apply.go
@@ -40,7 +40,14 @@ func (b *Local) opApply(
 	defer func() {
 		if s, ok := opState.(state.Locker); op.LockState && ok {
 			if err := s.Unlock(); err != nil {
-				log.Printf("[ERROR]: %s", err)
+				runningOp.Err = multierror.Append(runningOp.Err,
+					errwrap.Wrapf("Error unlocking state:\n\n"+
+						"{{err}}\n\n"+
+						"The Terraform operation completed but there was an error unlocking the state.\n"+
+						"This may require unlocking the state manually with the `terraform unlock` command\n",
+						err,
+					),
+				)
 			}
 		}
 	}()

From 07903189f1637c0a39861ac3d6372c509ab63a8d Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Mon, 6 Feb 2017 09:58:04 -0500
Subject: [PATCH 17/18] s/Meta.lockState/Meta.stateLock/g

---
 command/apply.go   | 4 ++--
 command/meta.go    | 2 +-
 command/plan.go    | 4 ++--
 command/refresh.go | 4 ++--
 command/taint.go   | 4 ++--
 command/untaint.go | 4 ++--
 6 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/command/apply.go b/command/apply.go
index 4e29262a9..e772a8e01 100644
--- a/command/apply.go
+++ b/command/apply.go
@@ -47,7 +47,7 @@ func (c *ApplyCommand) Run(args []string) int {
 	cmdFlags.StringVar(&c.Meta.statePath, "state", "", "path")
 	cmdFlags.StringVar(&c.Meta.stateOutPath, "state-out", "", "path")
 	cmdFlags.StringVar(&c.Meta.backupPath, "backup", "", "path")
-	cmdFlags.BoolVar(&c.Meta.lockState, "state-lock", true, "lock state")
+	cmdFlags.BoolVar(&c.Meta.stateLock, "state-lock", true, "lock state")
 	cmdFlags.Usage = func() { c.Ui.Error(c.Help()) }
 	if err := cmdFlags.Parse(args); err != nil {
 		return 1
@@ -183,7 +183,7 @@ func (c *ApplyCommand) Run(args []string) int {
 	opReq.Plan = plan
 	opReq.PlanRefresh = refresh
 	opReq.Type = backend.OperationTypeApply
-	opReq.LockState = c.Meta.lockState
+	opReq.LockState = c.Meta.stateLock
 
 	// Perform the operation
 	ctx, ctxCancel := context.WithCancel(context.Background())
diff --git a/command/meta.go b/command/meta.go
index 75e66355d..cc29b2aaf 100644
--- a/command/meta.go
+++ b/command/meta.go
@@ -91,7 +91,7 @@ type Meta struct {
 	parallelism  int
 	shadow       bool
 	provider     string
-	lockState    bool
+	stateLock    bool
 }
 
 // initStatePaths is used to initialize the default values for
diff --git a/command/plan.go b/command/plan.go
index 0bae85292..485202e2a 100644
--- a/command/plan.go
+++ b/command/plan.go
@@ -31,7 +31,7 @@ func (c *PlanCommand) Run(args []string) int {
 		&c.Meta.parallelism, "parallelism", DefaultParallelism, "parallelism")
 	cmdFlags.StringVar(&c.Meta.statePath, "state", "", "path")
 	cmdFlags.BoolVar(&detailed, "detailed-exitcode", false, "detailed-exitcode")
-	cmdFlags.BoolVar(&c.Meta.lockState, "state-lock", true, "lock state")
+	cmdFlags.BoolVar(&c.Meta.stateLock, "state-lock", true, "lock state")
 	cmdFlags.Usage = func() { c.Ui.Error(c.Help()) }
 	if err := cmdFlags.Parse(args); err != nil {
 		return 1
@@ -85,7 +85,7 @@ func (c *PlanCommand) Run(args []string) int {
 	opReq.PlanRefresh = refresh
 	opReq.PlanOutPath = outPath
 	opReq.Type = backend.OperationTypePlan
-	opReq.LockState = c.Meta.lockState
+	opReq.LockState = c.Meta.stateLock
 
 	// Perform the operation
 	op, err := b.Operation(context.Background(), opReq)
diff --git a/command/refresh.go b/command/refresh.go
index f51386648..decc3a968 100644
--- a/command/refresh.go
+++ b/command/refresh.go
@@ -23,7 +23,7 @@ func (c *RefreshCommand) Run(args []string) int {
 	cmdFlags.IntVar(&c.Meta.parallelism, "parallelism", 0, "parallelism")
 	cmdFlags.StringVar(&c.Meta.stateOutPath, "state-out", "", "path")
 	cmdFlags.StringVar(&c.Meta.backupPath, "backup", "", "path")
-	cmdFlags.BoolVar(&c.Meta.lockState, "state-lock", true, "lock state")
+	cmdFlags.BoolVar(&c.Meta.stateLock, "state-lock", true, "lock state")
 	cmdFlags.Usage = func() { c.Ui.Error(c.Help()) }
 	if err := cmdFlags.Parse(args); err != nil {
 		return 1
@@ -53,7 +53,7 @@ func (c *RefreshCommand) Run(args []string) int {
 	opReq := c.Operation()
 	opReq.Type = backend.OperationTypeRefresh
 	opReq.Module = mod
-	opReq.LockState = c.Meta.lockState
+	opReq.LockState = c.Meta.stateLock
 
 	// Perform the operation
 	op, err := b.Operation(context.Background(), opReq)
diff --git a/command/taint.go b/command/taint.go
index 6d652fdf4..6cacdd398 100644
--- a/command/taint.go
+++ b/command/taint.go
@@ -26,7 +26,7 @@ func (c *TaintCommand) Run(args []string) int {
 	cmdFlags.StringVar(&c.Meta.statePath, "state", DefaultStateFilename, "path")
 	cmdFlags.StringVar(&c.Meta.stateOutPath, "state-out", "", "path")
 	cmdFlags.StringVar(&c.Meta.backupPath, "backup", "", "path")
-	cmdFlags.BoolVar(&c.Meta.lockState, "state-lock", true, "lock state")
+	cmdFlags.BoolVar(&c.Meta.stateLock, "state-lock", true, "lock state")
 	cmdFlags.Usage = func() { c.Ui.Error(c.Help()) }
 	if err := cmdFlags.Parse(args); err != nil {
 		return 1
@@ -72,7 +72,7 @@ func (c *TaintCommand) Run(args []string) int {
 		return 1
 	}
 
-	if s, ok := st.(state.Locker); c.Meta.lockState && ok {
+	if s, ok := st.(state.Locker); c.Meta.stateLock && ok {
 		if err := s.Lock("taint"); err != nil {
 			c.Ui.Error(fmt.Sprintf("Failed to lock state: %s", err))
 			return 1
diff --git a/command/untaint.go b/command/untaint.go
index 6612d1940..ca3050c73 100644
--- a/command/untaint.go
+++ b/command/untaint.go
@@ -25,7 +25,7 @@ func (c *UntaintCommand) Run(args []string) int {
 	cmdFlags.StringVar(&c.Meta.statePath, "state", DefaultStateFilename, "path")
 	cmdFlags.StringVar(&c.Meta.stateOutPath, "state-out", "", "path")
 	cmdFlags.StringVar(&c.Meta.backupPath, "backup", "", "path")
-	cmdFlags.BoolVar(&c.Meta.lockState, "state-lock", true, "lock state")
+	cmdFlags.BoolVar(&c.Meta.stateLock, "state-lock", true, "lock state")
 	cmdFlags.Usage = func() { c.Ui.Error(c.Help()) }
 	if err := cmdFlags.Parse(args); err != nil {
 		return 1
@@ -60,7 +60,7 @@ func (c *UntaintCommand) Run(args []string) int {
 		return 1
 	}
 
-	if s, ok := st.(state.Locker); c.Meta.lockState && ok {
+	if s, ok := st.(state.Locker); c.Meta.stateLock && ok {
 		if err := s.Lock("untaint"); err != nil {
 			c.Ui.Error(fmt.Sprintf("Failed to lock state: %s", err))
 			return 1

From eb8e5ac739829c3e52c8a20824b0ff549d26aa2b Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Mon, 6 Feb 2017 10:07:32 -0500
Subject: [PATCH 18/18] Change CLI flag to '-lock'

---
 command/apply.go   | 6 +++---
 command/plan.go    | 4 ++--
 command/refresh.go | 4 ++--
 command/taint.go   | 4 ++--
 command/untaint.go | 4 ++--
 5 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/command/apply.go b/command/apply.go
index e772a8e01..76dea35b8 100644
--- a/command/apply.go
+++ b/command/apply.go
@@ -47,7 +47,7 @@ func (c *ApplyCommand) Run(args []string) int {
 	cmdFlags.StringVar(&c.Meta.statePath, "state", "", "path")
 	cmdFlags.StringVar(&c.Meta.stateOutPath, "state-out", "", "path")
 	cmdFlags.StringVar(&c.Meta.backupPath, "backup", "", "path")
-	cmdFlags.BoolVar(&c.Meta.stateLock, "state-lock", true, "lock state")
+	cmdFlags.BoolVar(&c.Meta.stateLock, "lock", true, "lock state")
 	cmdFlags.Usage = func() { c.Ui.Error(c.Help()) }
 	if err := cmdFlags.Parse(args); err != nil {
 		return 1
@@ -274,7 +274,7 @@ Options:
                          modifying. Defaults to the "-state-out" path with
                          ".backup" extension. Set to "-" to disable backup.
 
-  -lock-state=true       Lock the state file when locking is supported.
+  -lock=true             Lock the state file when locking is supported.
 
   -input=true            Ask for input for variables if not directly set.
 
@@ -323,7 +323,7 @@ Options:
 
   -force                 Don't ask for input for destroy confirmation.
 
-  -lock-state=true       Lock the state file when locking is supported.
+  -lock=true             Lock the state file when locking is supported.
 
   -no-color              If specified, output won't contain any color.
 
diff --git a/command/plan.go b/command/plan.go
index 485202e2a..5b89634ed 100644
--- a/command/plan.go
+++ b/command/plan.go
@@ -31,7 +31,7 @@ func (c *PlanCommand) Run(args []string) int {
 		&c.Meta.parallelism, "parallelism", DefaultParallelism, "parallelism")
 	cmdFlags.StringVar(&c.Meta.statePath, "state", "", "path")
 	cmdFlags.BoolVar(&detailed, "detailed-exitcode", false, "detailed-exitcode")
-	cmdFlags.BoolVar(&c.Meta.stateLock, "state-lock", true, "lock state")
+	cmdFlags.BoolVar(&c.Meta.stateLock, "lock", true, "lock state")
 	cmdFlags.Usage = func() { c.Ui.Error(c.Help()) }
 	if err := cmdFlags.Parse(args); err != nil {
 		return 1
@@ -143,7 +143,7 @@ Options:
 
   -input=true         Ask for input for variables if not directly set.
 
-  -lock-state=true    Lock the state file when locking is supported.
+  -lock=true          Lock the state file when locking is supported.
 
   -module-depth=n     Specifies the depth of modules to show in the output.
                       This does not affect the plan itself, only the output
diff --git a/command/refresh.go b/command/refresh.go
index decc3a968..c9c5527b9 100644
--- a/command/refresh.go
+++ b/command/refresh.go
@@ -23,7 +23,7 @@ func (c *RefreshCommand) Run(args []string) int {
 	cmdFlags.IntVar(&c.Meta.parallelism, "parallelism", 0, "parallelism")
 	cmdFlags.StringVar(&c.Meta.stateOutPath, "state-out", "", "path")
 	cmdFlags.StringVar(&c.Meta.backupPath, "backup", "", "path")
-	cmdFlags.BoolVar(&c.Meta.stateLock, "state-lock", true, "lock state")
+	cmdFlags.BoolVar(&c.Meta.stateLock, "lock", true, "lock state")
 	cmdFlags.Usage = func() { c.Ui.Error(c.Help()) }
 	if err := cmdFlags.Parse(args); err != nil {
 		return 1
@@ -96,7 +96,7 @@ Options:
 
   -input=true         Ask for input for variables if not directly set.
 
-  -lock-state=true    Lock the state file when locking is supported.
+  -lock=true          Lock the state file when locking is supported.
 
   -no-color           If specified, output won't contain any color.
 
diff --git a/command/taint.go b/command/taint.go
index 6cacdd398..7900fee9a 100644
--- a/command/taint.go
+++ b/command/taint.go
@@ -26,7 +26,7 @@ func (c *TaintCommand) Run(args []string) int {
 	cmdFlags.StringVar(&c.Meta.statePath, "state", DefaultStateFilename, "path")
 	cmdFlags.StringVar(&c.Meta.stateOutPath, "state-out", "", "path")
 	cmdFlags.StringVar(&c.Meta.backupPath, "backup", "", "path")
-	cmdFlags.BoolVar(&c.Meta.stateLock, "state-lock", true, "lock state")
+	cmdFlags.BoolVar(&c.Meta.stateLock, "lock", true, "lock state")
 	cmdFlags.Usage = func() { c.Ui.Error(c.Help()) }
 	if err := cmdFlags.Parse(args); err != nil {
 		return 1
@@ -177,7 +177,7 @@ Options:
                       modifying. Defaults to the "-state-out" path with
                       ".backup" extension. Set to "-" to disable backup.
 
-  -lock-state=true    Lock the state file when locking is supported.
+  -lock=true          Lock the state file when locking is supported.
 
   -module=path        The module path where the resource lives. By
                       default this will be root. Child modules can be specified
diff --git a/command/untaint.go b/command/untaint.go
index ca3050c73..88c5d35cf 100644
--- a/command/untaint.go
+++ b/command/untaint.go
@@ -25,7 +25,7 @@ func (c *UntaintCommand) Run(args []string) int {
 	cmdFlags.StringVar(&c.Meta.statePath, "state", DefaultStateFilename, "path")
 	cmdFlags.StringVar(&c.Meta.stateOutPath, "state-out", "", "path")
 	cmdFlags.StringVar(&c.Meta.backupPath, "backup", "", "path")
-	cmdFlags.BoolVar(&c.Meta.stateLock, "state-lock", true, "lock state")
+	cmdFlags.BoolVar(&c.Meta.stateLock, "lock", true, "lock state")
 	cmdFlags.Usage = func() { c.Ui.Error(c.Help()) }
 	if err := cmdFlags.Parse(args); err != nil {
 		return 1
@@ -165,7 +165,7 @@ Options:
                       modifying. Defaults to the "-state-out" path with
                       ".backup" extension. Set to "-" to disable backup.
 
-  -lock-state=true    Lock the state file when locking is supported.
+  -lock=true          Lock the state file when locking is supported.
 
   -module=path        The module path where the resource lives. By
                       default this will be root. Child modules can be specified