diff --git a/backend/local/backend.go b/backend/local/backend.go index e73959499..866c4899a 100644 --- a/backend/local/backend.go +++ b/backend/local/backend.go @@ -336,16 +336,6 @@ func (b *Local) Operation(ctx context.Context, op *backend.Operation) (*backend. defer stop() defer cancel() - // the state was locked during context creation, unlock the state when - // the operation completes - defer func() { - err := op.StateLocker.Unlock(nil) - if err != nil { - b.ShowDiagnostics(err) - runningOp.Result = backend.OperationFailure - } - }() - defer b.opLock.Unlock() f(stopCtx, cancelCtx, op, runningOp) }() diff --git a/backend/local/backend_apply.go b/backend/local/backend_apply.go index 8377fd378..bfcf7f506 100644 --- a/backend/local/backend_apply.go +++ b/backend/local/backend_apply.go @@ -56,6 +56,15 @@ func (b *Local) opApply( b.ReportResult(runningOp, diags) return } + // the state was locked during succesfull context creation; unlock the state + // when the operation completes + defer func() { + err := op.StateLocker.Unlock(nil) + if err != nil { + b.ShowDiagnostics(err) + runningOp.Result = backend.OperationFailure + } + }() // Before we do anything else we'll take a snapshot of the prior state // so we can use it for some fixups to our detection of whether the plan diff --git a/backend/local/backend_apply_test.go b/backend/local/backend_apply_test.go index 54c1c0299..73c384d42 100644 --- a/backend/local/backend_apply_test.go +++ b/backend/local/backend_apply_test.go @@ -62,6 +62,7 @@ test_instance.foo: provider = provider["registry.terraform.io/hashicorp/test"] ami = bar `) + } func TestLocal_applyEmptyDir(t *testing.T) { @@ -90,6 +91,9 @@ func TestLocal_applyEmptyDir(t *testing.T) { if _, err := os.Stat(b.StateOutPath); err == nil { t.Fatal("should not exist") } + + // the backend should be unlocked after a run + assertBackendStateUnlocked(t, b) } func TestLocal_applyEmptyDirDestroy(t *testing.T) { @@ -179,6 +183,9 @@ test_instance.foo: provider = provider["registry.terraform.io/hashicorp/test"] ami = bar `) + + // the backend should be unlocked after a run + assertBackendStateUnlocked(t, b) } func TestLocal_applyBackendFail(t *testing.T) { @@ -229,6 +236,9 @@ test_instance.foo: provider = provider["registry.terraform.io/hashicorp/test"] ami = bar `) + + // the backend should be unlocked after a run + assertBackendStateUnlocked(t, b) } type backendWithFailingState struct { diff --git a/backend/local/backend_local.go b/backend/local/backend_local.go index 4fffc5b66..b06de8016 100644 --- a/backend/local/backend_local.go +++ b/backend/local/backend_local.go @@ -49,6 +49,18 @@ func (b *Local) context(op *backend.Operation) (*terraform.Context, *configload. diags = diags.Append(errwrap.Wrapf("Error locking state: {{err}}", err)) return nil, nil, nil, diags } + + defer func() { + // If we're returning with errors, and thus not producing a valid + // context, we'll want to avoid leaving the workspace locked. + if diags.HasErrors() { + err := op.StateLocker.Unlock(nil) + if err != nil { + diags = diags.Append(errwrap.Wrapf("Error unlocking state: {{err}}", err)) + } + } + }() + log.Printf("[TRACE] backend/local: reading remote state for workspace %q", op.Workspace) if err := s.RefreshState(); err != nil { diags = diags.Append(errwrap.Wrapf("Error loading state: {{err}}", err)) diff --git a/backend/local/backend_local_test.go b/backend/local/backend_local_test.go new file mode 100644 index 000000000..3354b709d --- /dev/null +++ b/backend/local/backend_local_test.go @@ -0,0 +1,57 @@ +package local + +import ( + "testing" + + "github.com/hashicorp/terraform/backend" + "github.com/hashicorp/terraform/internal/initwd" +) + +func TestLocalContext(t *testing.T) { + configDir := "./testdata/empty" + b, cleanup := TestLocal(t) + defer cleanup() + + _, configLoader, configCleanup := initwd.MustLoadConfigForTests(t, configDir) + defer configCleanup() + + op := &backend.Operation{ + ConfigDir: configDir, + ConfigLoader: configLoader, + Workspace: backend.DefaultStateName, + LockState: true, + } + + _, _, diags := b.Context(op) + if diags.HasErrors() { + t.Fatalf("unexpected error: %s", diags.Err().Error()) + } + + // Context() retains a lock on success + assertBackendStateLocked(t, b) +} + +func TestLocalContext_error(t *testing.T) { + configDir := "./testdata/apply" + b, cleanup := TestLocal(t) + defer cleanup() + + _, configLoader, configCleanup := initwd.MustLoadConfigForTests(t, configDir) + defer configCleanup() + + op := &backend.Operation{ + ConfigDir: configDir, + ConfigLoader: configLoader, + Workspace: backend.DefaultStateName, + LockState: true, + } + + _, _, diags := b.Context(op) + if !diags.HasErrors() { + t.Fatal("unexpected success") + } + + // Context() unlocks the state on failure + assertBackendStateUnlocked(t, b) + +} diff --git a/backend/local/backend_plan.go b/backend/local/backend_plan.go index aff6c958c..4ce5f893c 100644 --- a/backend/local/backend_plan.go +++ b/backend/local/backend_plan.go @@ -75,6 +75,15 @@ func (b *Local) opPlan( b.ReportResult(runningOp, diags) return } + // the state was locked during succesfull context creation; unlock the state + // when the operation completes + defer func() { + err := op.StateLocker.Unlock(nil) + if err != nil { + b.ShowDiagnostics(err) + runningOp.Result = backend.OperationFailure + } + }() // Before we do anything else we'll take a snapshot of the prior state // so we can use it for some fixups to our detection of whether the plan diff --git a/backend/local/backend_plan_test.go b/backend/local/backend_plan_test.go index dea0058b1..ca40ca635 100644 --- a/backend/local/backend_plan_test.go +++ b/backend/local/backend_plan_test.go @@ -41,6 +41,9 @@ func TestLocal_planBasic(t *testing.T) { if !p.PlanResourceChangeCalled { t.Fatal("PlanResourceChange should be called") } + + // the backend should be unlocked after a run + assertBackendStateUnlocked(t, b) } func TestLocal_planInAutomation(t *testing.T) { @@ -128,6 +131,33 @@ func TestLocal_planNoConfig(t *testing.T) { if !strings.Contains(output, "configuration") { t.Fatalf("bad: %s", err) } + + // the backend should be unlocked after a run + assertBackendStateUnlocked(t, b) +} + +// This test validates the state lacking behavior when the inner call to +// Context() fails +func TestLocal_plan_context_error(t *testing.T) { + b, cleanup := TestLocal(t) + defer cleanup() + + op, configCleanup := testOperationPlan(t, "./testdata/plan") + defer configCleanup() + op.PlanRefresh = true + + // we coerce a failure in Context() by omitting the provider schema + run, err := b.Operation(context.Background(), op) + if err != nil { + t.Fatalf("bad: %s", err) + } + <-run.Done() + if run.Result != backend.OperationFailure { + t.Fatalf("plan operation succeeded") + } + + // the backend should be unlocked after a run + assertBackendStateUnlocked(t, b) } func TestLocal_planOutputsChanged(t *testing.T) { diff --git a/backend/local/backend_refresh.go b/backend/local/backend_refresh.go index d1fd4a7db..3a4343b86 100644 --- a/backend/local/backend_refresh.go +++ b/backend/local/backend_refresh.go @@ -50,6 +50,16 @@ func (b *Local) opRefresh( return } + // the state was locked during succesfull context creation; unlock the state + // when the operation completes + defer func() { + err := op.StateLocker.Unlock(nil) + if err != nil { + b.ShowDiagnostics(err) + runningOp.Result = backend.OperationFailure + } + }() + // Set our state runningOp.State = opState.State() if !runningOp.State.HasResources() { diff --git a/backend/local/backend_refresh_test.go b/backend/local/backend_refresh_test.go index 4b2bcc2df..d6554c47a 100644 --- a/backend/local/backend_refresh_test.go +++ b/backend/local/backend_refresh_test.go @@ -46,6 +46,9 @@ test_instance.foo: ID = yes provider = provider["registry.terraform.io/hashicorp/test"] `) + + // the backend should be unlocked after a run + assertBackendStateUnlocked(t, b) } func TestLocal_refreshNoConfig(t *testing.T) { @@ -202,6 +205,28 @@ test_instance.foo: `) } +// This test validates the state lacking behavior when the inner call to +// Context() fails +func TestLocal_refresh_context_error(t *testing.T) { + b, cleanup := TestLocal(t) + defer cleanup() + testStateFile(t, b.StatePath, testRefreshState()) + op, configCleanup := testOperationRefresh(t, "./testdata/apply") + defer configCleanup() + + // we coerce a failure in Context() by omitting the provider schema + + run, err := b.Operation(context.Background(), op) + if err != nil { + t.Fatalf("bad: %s", err) + } + <-run.Done() + if run.Result == backend.OperationSuccess { + t.Fatal("operation succeeded; want failure") + } + assertBackendStateUnlocked(t, b) +} + func testOperationRefresh(t *testing.T, configDir string) (*backend.Operation, func()) { t.Helper() @@ -211,6 +236,7 @@ func testOperationRefresh(t *testing.T, configDir string) (*backend.Operation, f Type: backend.OperationTypeRefresh, ConfigDir: configDir, ConfigLoader: configLoader, + LockState: true, }, configCleanup } diff --git a/backend/local/testing.go b/backend/local/testing.go index 336df8811..0e6d426f1 100644 --- a/backend/local/testing.go +++ b/backend/local/testing.go @@ -225,3 +225,29 @@ func mustResourceInstanceAddr(s string) addrs.AbsResourceInstance { } return addr } + +// assertBackendStateUnlocked attempts to lock the backend state. Failure +// indicates that the state was indeed locked and therefore this function will +// return true. +func assertBackendStateUnlocked(t *testing.T, b *Local) bool { + t.Helper() + stateMgr, _ := b.StateMgr(backend.DefaultStateName) + if _, err := stateMgr.Lock(statemgr.NewLockInfo()); err != nil { + t.Errorf("state is already locked: %s", err.Error()) + return false + } + return true +} + +// assertBackendStateLocked attempts to lock the backend state. Failure +// indicates that the state was already locked and therefore this function will +// return false. +func assertBackendStateLocked(t *testing.T, b *Local) bool { + t.Helper() + stateMgr, _ := b.StateMgr(backend.DefaultStateName) + if _, err := stateMgr.Lock(statemgr.NewLockInfo()); err != nil { + return true + } + t.Error("unexpected success locking state") + return true +} diff --git a/backend/remote/backend.go b/backend/remote/backend.go index 94132618c..e8e9bc343 100644 --- a/backend/remote/backend.go +++ b/backend/remote/backend.go @@ -18,8 +18,8 @@ import ( "github.com/hashicorp/terraform-svchost/disco" "github.com/hashicorp/terraform/backend" "github.com/hashicorp/terraform/configs/configschema" - "github.com/hashicorp/terraform/state" "github.com/hashicorp/terraform/state/remote" + "github.com/hashicorp/terraform/states/statemgr" "github.com/hashicorp/terraform/terraform" "github.com/hashicorp/terraform/tfdiags" tfversion "github.com/hashicorp/terraform/version" @@ -591,7 +591,7 @@ func (b *Remote) DeleteWorkspace(name string) error { } // StateMgr implements backend.Enhanced. -func (b *Remote) StateMgr(name string) (state.State, error) { +func (b *Remote) StateMgr(name string) (statemgr.Full, error) { if b.workspace == "" && name == backend.DefaultStateName { return nil, backend.ErrDefaultWorkspaceNotSupported } diff --git a/backend/remote/backend_apply_test.go b/backend/remote/backend_apply_test.go index aca57bd18..7fe5c1c0c 100644 --- a/backend/remote/backend_apply_test.go +++ b/backend/remote/backend_apply_test.go @@ -15,6 +15,7 @@ import ( "github.com/hashicorp/terraform/backend" "github.com/hashicorp/terraform/internal/initwd" "github.com/hashicorp/terraform/plans/planfile" + "github.com/hashicorp/terraform/states/statemgr" "github.com/hashicorp/terraform/terraform" "github.com/mitchellh/cli" ) @@ -75,6 +76,12 @@ func TestRemote_applyBasic(t *testing.T) { if !strings.Contains(output, "1 added, 0 changed, 0 destroyed") { t.Fatalf("expected apply summery in output: %s", output) } + + stateMgr, _ := b.StateMgr(backend.DefaultStateName) + // An error suggests that the state was not unlocked after apply + if _, err := stateMgr.Lock(statemgr.NewLockInfo()); err != nil { + t.Fatalf("unexpected error locking state after apply: %s", err.Error()) + } } func TestRemote_applyCanceled(t *testing.T) { @@ -98,6 +105,11 @@ func TestRemote_applyCanceled(t *testing.T) { if run.Result == backend.OperationSuccess { t.Fatal("expected apply operation to fail") } + + stateMgr, _ := b.StateMgr(backend.DefaultStateName) + if _, err := stateMgr.Lock(statemgr.NewLockInfo()); err != nil { + t.Fatalf("unexpected error locking state after cancelling apply: %s", err.Error()) + } } func TestRemote_applyWithoutPermissions(t *testing.T) { @@ -386,6 +398,12 @@ func TestRemote_applyNoConfig(t *testing.T) { if !strings.Contains(errOutput, "configuration files found") { t.Fatalf("expected configuration files error, got: %v", errOutput) } + + stateMgr, _ := b.StateMgr(backend.DefaultStateName) + // An error suggests that the state was not unlocked after apply + if _, err := stateMgr.Lock(statemgr.NewLockInfo()); err != nil { + t.Fatalf("unexpected error locking state after failed apply: %s", err.Error()) + } } func TestRemote_applyNoChanges(t *testing.T) { diff --git a/backend/remote/backend_context_test.go b/backend/remote/backend_context_test.go index a7fa5e713..48a6a8525 100644 --- a/backend/remote/backend_context_test.go +++ b/backend/remote/backend_context_test.go @@ -8,6 +8,7 @@ import ( "github.com/hashicorp/terraform/backend" "github.com/hashicorp/terraform/configs" "github.com/hashicorp/terraform/internal/initwd" + "github.com/hashicorp/terraform/states/statemgr" "github.com/zclconf/go-cty/cty" ) @@ -186,6 +187,7 @@ func TestRemoteContextWithVars(t *testing.T) { ConfigDir: configDir, ConfigLoader: configLoader, Workspace: backend.DefaultStateName, + LockState: true, } v := test.Opts @@ -205,10 +207,21 @@ func TestRemoteContextWithVars(t *testing.T) { if errStr != test.WantError { t.Fatalf("wrong error\ngot: %s\nwant: %s", errStr, test.WantError) } + // When Context() returns an error, it should unlock the state, + // so re-locking it is expected to succeed. + stateMgr, _ := b.StateMgr(backend.DefaultStateName) + if _, err := stateMgr.Lock(statemgr.NewLockInfo()); err != nil { + t.Fatalf("unexpected error locking state: %s", err.Error()) + } } else { if diags.HasErrors() { t.Fatalf("unexpected error\ngot: %s\nwant: ", diags.Err().Error()) } + // When Context() succeeds, this should fail w/ "workspace already locked" + stateMgr, _ := b.StateMgr(backend.DefaultStateName) + if _, err := stateMgr.Lock(statemgr.NewLockInfo()); err == nil { + t.Fatal("unexpected success locking state after Context") + } } }) } diff --git a/backend/remote/backend_plan_test.go b/backend/remote/backend_plan_test.go index 52b116b82..a2c6e4ad2 100644 --- a/backend/remote/backend_plan_test.go +++ b/backend/remote/backend_plan_test.go @@ -15,6 +15,7 @@ import ( "github.com/hashicorp/terraform/backend" "github.com/hashicorp/terraform/internal/initwd" "github.com/hashicorp/terraform/plans/planfile" + "github.com/hashicorp/terraform/states/statemgr" "github.com/hashicorp/terraform/terraform" "github.com/mitchellh/cli" ) @@ -62,6 +63,12 @@ func TestRemote_planBasic(t *testing.T) { if !strings.Contains(output, "1 to add, 0 to change, 0 to destroy") { t.Fatalf("expected plan summary in output: %s", output) } + + stateMgr, _ := b.StateMgr(backend.DefaultStateName) + // An error suggests that the state was not unlocked after the operation finished + if _, err := stateMgr.Lock(statemgr.NewLockInfo()); err != nil { + t.Fatalf("unexpected error locking state after successful plan: %s", err.Error()) + } } func TestRemote_planCanceled(t *testing.T) { @@ -85,6 +92,12 @@ func TestRemote_planCanceled(t *testing.T) { if run.Result == backend.OperationSuccess { t.Fatal("expected plan operation to fail") } + + stateMgr, _ := b.StateMgr(backend.DefaultStateName) + // An error suggests that the state was not unlocked after the operation finished + if _, err := stateMgr.Lock(statemgr.NewLockInfo()); err != nil { + t.Fatalf("unexpected error locking state after cancelled plan: %s", err.Error()) + } } func TestRemote_planLongLine(t *testing.T) { diff --git a/command/console.go b/command/console.go index 1fa2cb875..359d9dca0 100644 --- a/command/console.go +++ b/command/console.go @@ -92,8 +92,13 @@ func (c *ConsoleCommand) Run(args []string) int { // Get the context ctx, _, ctxDiags := local.Context(opReq) + diags = diags.Append(ctxDiags) + if ctxDiags.HasErrors() { + c.showDiagnostics(diags) + return 1 + } - // Creating the context can result in a lock, so ensure we release it + // Successfully creating the context can result in a lock, so ensure we release it defer func() { err := opReq.StateLocker.Unlock(nil) if err != nil { @@ -101,12 +106,6 @@ func (c *ConsoleCommand) Run(args []string) int { } }() - diags = diags.Append(ctxDiags) - if ctxDiags.HasErrors() { - c.showDiagnostics(diags) - return 1 - } - // Setup the UI so we can output directly to stdout ui := &cli.BasicUi{ Writer: wrappedstreams.Stdout(), diff --git a/command/import.go b/command/import.go index f09524b53..f5891ed6d 100644 --- a/command/import.go +++ b/command/import.go @@ -200,8 +200,13 @@ func (c *ImportCommand) Run(args []string) int { // Get the context ctx, state, ctxDiags := local.Context(opReq) + diags = diags.Append(ctxDiags) + if ctxDiags.HasErrors() { + c.showDiagnostics(diags) + return 1 + } - // Creating the context can result in a lock, so ensure we release it + // Successfully creating the context can result in a lock, so ensure we release it defer func() { err := opReq.StateLocker.Unlock(nil) if err != nil { @@ -209,12 +214,6 @@ func (c *ImportCommand) Run(args []string) int { } }() - diags = diags.Append(ctxDiags) - if ctxDiags.HasErrors() { - c.showDiagnostics(diags) - return 1 - } - // Perform the import. Note that as you can see it is possible for this // API to import more than one resource at once. For now, we only allow // one while we stabilize this feature.