From 86e9ba3d659176cd7ea969434e37cb064f23bb43 Mon Sep 17 00:00:00 2001 From: Kristin Laemmert Date: Tue, 11 Aug 2020 11:23:42 -0400 Subject: [PATCH] * backend/local: push responsibility for unlocking state into individual operations * unlock the state if Context() has an error, exactly as backend/remote does today * terraform console and terraform import will exit before unlocking state in case of error in Context() * responsibility for unlocking state in the local backend is pushed down the stack, out of backend.go and into each individual state operation * add tests confirming that state is not locked after apply and plan * backend/local: add checks that the state is unlocked after operations This adds tests to plan, apply and refresh which validate that the state is unlocked after all operations, regardless of exit status. I've also added specific tests that force Context() to fail during each operation to verify that locking behavior specifically. --- backend/local/backend.go | 10 ----- backend/local/backend_apply.go | 9 ++++ backend/local/backend_apply_test.go | 10 +++++ backend/local/backend_local.go | 12 ++++++ backend/local/backend_local_test.go | 57 ++++++++++++++++++++++++++ backend/local/backend_plan.go | 9 ++++ backend/local/backend_plan_test.go | 30 ++++++++++++++ backend/local/backend_refresh.go | 10 +++++ backend/local/backend_refresh_test.go | 26 ++++++++++++ backend/local/testing.go | 26 ++++++++++++ backend/remote/backend.go | 4 +- backend/remote/backend_apply_test.go | 18 ++++++++ backend/remote/backend_context_test.go | 13 ++++++ backend/remote/backend_plan_test.go | 13 ++++++ command/console.go | 13 +++--- command/import.go | 13 +++--- 16 files changed, 247 insertions(+), 26 deletions(-) create mode 100644 backend/local/backend_local_test.go 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.