From 701d09580826a9857c76f2a2b674577e7a41757b Mon Sep 17 00:00:00 2001
From: Alisdair McDiarmid <alisdair@users.noreply.github.com>
Date: Fri, 14 Feb 2020 13:47:29 -0500
Subject: [PATCH] command: Fix stale lock when exiting early

If an error occurs on creating the context for console or import, we
would fail to unlock the state. Fix this by unlocking slightly earlier.
Affects console and import commands.

Fixes #23318
---
 command/console.go                            | 11 +--
 command/import.go                             | 12 ++--
 command/import_test.go                        | 69 +++++++++++++++++++
 .../testdata/import-provider-invalid/main.tf  | 15 ++++
 4 files changed, 96 insertions(+), 11 deletions(-)
 create mode 100644 command/testdata/import-provider-invalid/main.tf

diff --git a/command/console.go b/command/console.go
index 4992496b3..9974fb8e8 100644
--- a/command/console.go
+++ b/command/console.go
@@ -96,11 +96,6 @@ 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
-	}
 
 	defer func() {
 		err := opReq.StateLocker.Unlock(nil)
@@ -109,6 +104,12 @@ 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 e8d0f32ce..96e35fb35 100644
--- a/command/import.go
+++ b/command/import.go
@@ -203,13 +203,7 @@ 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
-	}
 
-	// Make sure to unlock the state
 	defer func() {
 		err := opReq.StateLocker.Unlock(nil)
 		if err != nil {
@@ -217,6 +211,12 @@ 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.
diff --git a/command/import_test.go b/command/import_test.go
index ee3166696..3171091fc 100644
--- a/command/import_test.go
+++ b/command/import_test.go
@@ -258,6 +258,75 @@ func TestImport_remoteState(t *testing.T) {
 	testStateOutput(t, statePath, testImportStr)
 }
 
+// early failure on import should not leave stale lock
+func TestImport_initializationErrorShouldUnlock(t *testing.T) {
+	td := tempDir(t)
+	copy.CopyDir(testFixturePath("import-provider-remote-state"), td)
+	defer os.RemoveAll(td)
+	defer testChdir(t, td)()
+
+	statePath := "imported.tfstate"
+
+	// init our backend
+	ui := cli.NewMockUi()
+	m := Meta{
+		testingOverrides: metaOverridesForProvider(testProvider()),
+		Ui:               ui,
+	}
+
+	ic := &InitCommand{
+		Meta: m,
+		providerInstaller: &mockProviderInstaller{
+			Providers: map[string][]string{
+				"test": []string{"1.2.3"},
+			},
+
+			Dir: m.pluginDir(),
+		},
+	}
+
+	// (Using log here rather than t.Log so that these messages interleave with other trace logs)
+	log.Print("[TRACE] TestImport_initializationErrorShouldUnlock running: terraform init")
+	if code := ic.Run([]string{}); code != 0 {
+		t.Fatalf("init failed\n%s", ui.ErrorWriter)
+	}
+
+	// overwrite the config with one including a resource from an invalid provider
+	copy.CopyFile(filepath.Join(testFixturePath("import-provider-invalid"), "main.tf"), filepath.Join(td, "main.tf"))
+
+	p := testProvider()
+	ui = new(cli.MockUi)
+	c := &ImportCommand{
+		Meta: Meta{
+			testingOverrides: metaOverridesForProvider(p),
+			Ui:               ui,
+		},
+	}
+
+	args := []string{
+		"unknown_instance.baz",
+		"bar",
+	}
+	log.Printf("[TRACE] TestImport_initializationErrorShouldUnlock running: terraform import %s %s", args[0], args[1])
+
+	// this should fail
+	if code := c.Run(args); code != 1 {
+		fmt.Println(ui.OutputWriter)
+		t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String())
+	}
+
+	// specifically, it should fail due to a missing provider
+	msg := ui.ErrorWriter.String()
+	if want := "Could not satisfy plugin requirements"; !strings.Contains(msg, want) {
+		t.Errorf("incorrect message\nwant substring: %s\ngot:\n%s", want, msg)
+	}
+
+	// verify that the local state was unlocked after initialization error
+	if _, err := os.Stat(filepath.Join(td, fmt.Sprintf(".%s.lock.info", statePath))); !os.IsNotExist(err) {
+		t.Fatal("state left locked after import")
+	}
+}
+
 func TestImport_providerConfigWithVar(t *testing.T) {
 	defer testChdir(t, testFixturePath("import-provider-var"))()
 
diff --git a/command/testdata/import-provider-invalid/main.tf b/command/testdata/import-provider-invalid/main.tf
new file mode 100644
index 000000000..c156850d3
--- /dev/null
+++ b/command/testdata/import-provider-invalid/main.tf
@@ -0,0 +1,15 @@
+terraform {
+	backend "local" {
+		path = "imported.tfstate"
+	}
+}
+
+provider "test" {
+    foo = "bar"
+}
+
+resource "test_instance" "foo" {
+}
+
+resource "unknown_instance" "baz" {
+}