Merge pull request #421 from runatlantis/handle-panics

Fix bug where Atlantis hangs on terraform crash.

server/events/terraform/terraform_client.go

@@ -23,9 +23,10 @@ import (
 	"regexp"
 	"strings"
 
-	"github.com/mitchellh/go-homedir"
+	"github.com/mitchellh/go-linereader"
 
 	"github.com/hashicorp/go-version"
+	"github.com/mitchellh/go-homedir"
 	"github.com/pkg/errors"
 	"github.com/runatlantis/atlantis/server/logging"
 )
@@ -161,21 +162,69 @@ func (c *DefaultClient) RunCommandWithVersion(log *logging.SimpleLogger, path st
 
 	// append terraform executable name with args
 	tfCmd := fmt.Sprintf("%s %s", tfExecutable, strings.Join(args, " "))
+	out, err := c.crashSafeExec(tfCmd, path, envVars)
+	if err != nil {
+		err = fmt.Errorf("%s: running %q in %q", err, tfCmd, path)
+		log.Debug("error: %s", err)
+		return out, err
+	}
+	log.Info("successfully ran %q in %q", tfCmd, path)
+	return out, err
+}
+
+// crashSafeExec executes tfCmd in dir with the env environment variables. It
+// returns any stderr and stdout output from the command as a combined string.
+// It is "crash safe" in that it handles an edge case related to:
+//    https://github.com/golang/go/issues/18874
+// where when terraform itself panics, it leaves file descriptors open which
+// cause golang to not know the process has terminated.
+// To handle this, we borrow code from
+//    https://github.com/hashicorp/terraform/blob/master/builtin/provisioners/local-exec/resource_provisioner.go#L92
+// and use an os.Pipe to collect the stderr and stdout. This allows golang to
+// know the command has exited and so the call to cmd.Wait() won't block
+// indefinitely.
+//
+// Unfortunately, this causes another issue where we never receive an EOF to
+// our pipe during a terraform panic and so again, we're left waiting
+// indefinitely. To handle this, I've hacked in detection of Terraform panic
+// output as a special case that causes us to exit the loop.
+func (c *DefaultClient) crashSafeExec(tfCmd string, dir string, env []string) (string, error) {
+	pr, pw, err := os.Pipe()
+	if err != nil {
+		return "", errors.Wrap(err, "failed to initialize pipe for output")
+	}
 
 	// We use 'sh -c' so that if extra_args have been specified with env vars,
 	// ex. -var-file=$WORKSPACE.tfvars, then they get substituted.
-	terraformCmd := exec.Command("sh", "-c", tfCmd) // #nosec
-	terraformCmd.Dir = path
-	terraformCmd.Env = envVars
-	out, err := terraformCmd.CombinedOutput()
-	commandStr := strings.Join(terraformCmd.Args, " ")
-	if err != nil {
-		err = fmt.Errorf("%s: running %q in %q", err, commandStr, path)
-		log.Debug("error: %s", err)
-		return string(out), err
+	cmd := exec.Command("sh", "-c", tfCmd) // #nosec
+	cmd.Stdout = pw
+	cmd.Stderr = pw
+	cmd.Dir = dir
+	cmd.Env = env
+
+	err = cmd.Start()
+	if err == nil {
+		err = cmd.Wait()
+	}
+	pw.Close() // nolint: errcheck
+
+	lr := linereader.New(pr)
+	var outputLines []string
+	for line := range lr.Ch {
+		outputLines = append(outputLines, line)
+		// This checks if our output is a Terraform panic. If so, we break
+		// out of the loop because in this case, for some reason to do with
+		// terraform forking itself, we never receive an EOF and
+		// so this will block indefinitely.
+		if len(outputLines) >= 3 &&
+			strings.Join(
+				outputLines[len(outputLines)-3:], "\n") ==
+				tfCrashDelim {
+			break
+		}
 	}
-	log.Info("successfully ran %q in %q", commandStr, path)
-	return string(out), nil
+
+	return strings.Join(outputLines, "\n"), err
 }
 
 // MustConstraint will parse one or more constraints from the given
@@ -195,3 +244,8 @@ func MustConstraint(v string) version.Constraints {
 var rcFileContents = `credentials "app.terraform.io" {
   token = %q
 }`
+
+// tfCrashDelim is what the end of a terraform crash log looks like.
+var tfCrashDelim = `[1]: https://github.com/hashicorp/terraform/issues
+
+!!!!!!!!!!!!!!!!!!!!!!!!!!! TERRAFORM CRASH !!!!!!!!!!!!!!!!!!!!!!!!!!!!`

server/events/terraform/terraform_client_internal_test.go

@@ -79,3 +79,41 @@ func TestGenerateRCFile_ErrIfCannotWrite(t *testing.T) {
 	actErr := generateRCFile("token", "/this/dir/does/not/exist")
 	ErrEquals(t, expErr, actErr)
 }
+
+// I couldn't find an easy way to test the edge case that this function exists
+// for (where terraform panics) so I'm just testing that it executes a normal
+// process as expected.
+func TestCrashSafeExec(t *testing.T) {
+	cases := []struct {
+		cmd    string
+		expErr string
+		expOut string
+	}{
+		{
+			"echo hi",
+			"",
+			"hi",
+		},
+		{
+			"echo yo && exit 1",
+			"exit status 1",
+			"yo",
+		},
+	}
+
+	client := DefaultClient{}
+	for _, c := range cases {
+		t.Run(c.cmd, func(t *testing.T) {
+			tmp, cleanup := TempDir(t)
+			defer cleanup()
+			out, err := client.crashSafeExec(c.cmd, tmp, nil)
+			if c.expErr != "" {
+				ErrEquals(t, c.expErr, err)
+				Equals(t, c.expOut, out)
+			} else {
+				Ok(t, err)
+				Equals(t, c.expOut, out)
+			}
+		})
+	}
+}

server/testfixtures/test-repos/modules-yaml/exp-output-apply-production.txt

@@ -9,6 +9,5 @@ Apply complete! Resources: 1 added, 0 changed, 0 destroyed.
 Outputs:
 
 var = production
-
 ```
 

server/testfixtures/test-repos/modules-yaml/exp-output-apply-staging.txt

@@ -9,6 +9,5 @@ Apply complete! Resources: 1 added, 0 changed, 0 destroyed.
 Outputs:
 
 var = staging
-
 ```
 

server/testfixtures/test-repos/modules-yaml/exp-output-autoplan.txt

@@ -14,7 +14,6 @@ Terraform will perform the following actions:
 + module.null.null_resource.this
       id: <computed>
 Plan: 1 to add, 0 to change, 0 to destroy.
-
 ```
 
 * :arrow_forward: To **apply** this plan, comment:
@@ -36,7 +35,6 @@ Terraform will perform the following actions:
 + module.null.null_resource.this
       id: <computed>
 Plan: 1 to add, 0 to change, 0 to destroy.
-
 ```
 
 * :arrow_forward: To **apply** this plan, comment:

server/testfixtures/test-repos/modules/exp-output-apply-production.txt

@@ -9,6 +9,5 @@ Apply complete! Resources: 1 added, 0 changed, 0 destroyed.
 Outputs:
 
 var = production
-
 ```
 

server/testfixtures/test-repos/modules/exp-output-apply-staging.txt

@@ -9,6 +9,5 @@ Apply complete! Resources: 1 added, 0 changed, 0 destroyed.
 Outputs:
 
 var = staging
-
 ```
 

server/testfixtures/test-repos/modules/exp-output-autoplan-only-staging.txt

@@ -11,7 +11,6 @@ Terraform will perform the following actions:
 + module.null.null_resource.this
       id: <computed>
 Plan: 1 to add, 0 to change, 0 to destroy.
-
 ```
 
 * :arrow_forward: To **apply** this plan, comment:

server/testfixtures/test-repos/modules/exp-output-plan-production.txt

@@ -11,7 +11,6 @@ Terraform will perform the following actions:
 + module.null.null_resource.this
       id: <computed>
 Plan: 1 to add, 0 to change, 0 to destroy.
-
 ```
 
 * :arrow_forward: To **apply** this plan, comment:

server/testfixtures/test-repos/modules/exp-output-plan-staging.txt

@@ -11,7 +11,6 @@ Terraform will perform the following actions:
 + module.null.null_resource.this
       id: <computed>
 Plan: 1 to add, 0 to change, 0 to destroy.
-
 ```
 
 * :arrow_forward: To **apply** this plan, comment:

server/testfixtures/test-repos/simple-yaml/exp-output-apply-all.txt

@@ -13,13 +13,10 @@ Outputs:
 
 var = fromconfig
 workspace = default
-
 ```
 
 ---
 ### 2. dir: `.` workspace: `staging`
-<details><summary>Show Output</summary>
-
 ```diff
 preapply
 
@@ -32,11 +29,9 @@ Outputs:
 
 var = fromfile
 workspace = staging
-
 postapply
 
 ```
-</details>
 
 ---
 

server/testfixtures/test-repos/simple-yaml/exp-output-apply-default.txt

@@ -10,6 +10,5 @@ Outputs:
 
 var = fromconfig
 workspace = default
-
 ```
 

server/testfixtures/test-repos/simple-yaml/exp-output-apply-staging.txt

@@ -1,7 +1,5 @@
 Ran Apply for dir: `.` workspace: `staging`
 
-<details><summary>Show Output</summary>
-
 ```diff
 preapply
 
@@ -14,9 +12,7 @@ Outputs:
 
 var = fromfile
 workspace = staging
-
 postapply
 
 ```
-</details>
 

server/testfixtures/test-repos/simple-yaml/exp-output-autoplan.txt

@@ -18,7 +18,6 @@ Terraform will perform the following actions:
 + null_resource.simple
       id: <computed>
 Plan: 1 to add, 0 to change, 0 to destroy.
-
 postplan
 
 ```
@@ -43,7 +42,6 @@ Terraform will perform the following actions:
 + null_resource.simple
       id: <computed>
 Plan: 1 to add, 0 to change, 0 to destroy.
-
 ```
 
 * :arrow_forward: To **apply** this plan, comment:

server/testfixtures/test-repos/simple/exp-output-apply-var-all.txt

@@ -3,8 +3,6 @@ Ran Apply for 2 projects:
 1. dir: `.` workspace: `new_workspace`
 
 ### 1. dir: `.` workspace: `default`
-<details><summary>Show Output</summary>
-
 ```diff
 null_resource.simple:
 null_resource.simple:
@@ -19,14 +17,10 @@ Outputs:
 
 var = default_workspace
 workspace = default
-
 ```
-</details>
 
 ---
 ### 2. dir: `.` workspace: `new_workspace`
-<details><summary>Show Output</summary>
-
 ```diff
 null_resource.simple:
 null_resource.simple:
@@ -41,9 +35,7 @@ Outputs:
 
 var = new_workspace
 workspace = new_workspace
-
 ```
-</details>
 
 ---
 

server/testfixtures/test-repos/simple/exp-output-apply-var-default-workspace.txt

@@ -1,7 +1,5 @@
 Ran Apply for dir: `.` workspace: `default`
 
-<details><summary>Show Output</summary>
-
 ```diff
 null_resource.simple:
 null_resource.simple:
@@ -16,7 +14,5 @@ Outputs:
 
 var = default_workspace
 workspace = default
-
 ```
-</details>
 

server/testfixtures/test-repos/simple/exp-output-apply-var-new-workspace.txt

@@ -1,7 +1,5 @@
 Ran Apply for dir: `.` workspace: `new_workspace`
 
-<details><summary>Show Output</summary>
-
 ```diff
 null_resource.simple:
 null_resource.simple:
@@ -16,7 +14,5 @@ Outputs:
 
 var = new_workspace
 workspace = new_workspace
-
 ```
-</details>
 

server/testfixtures/test-repos/simple/exp-output-apply-var.txt

@@ -1,7 +1,5 @@
 Ran Apply for dir: `.` workspace: `default`
 
-<details><summary>Show Output</summary>
-
 ```diff
 null_resource.simple:
 null_resource.simple:
@@ -16,7 +14,5 @@ Outputs:
 
 var = overridden
 workspace = default
-
 ```
-</details>
 

server/testfixtures/test-repos/simple/exp-output-apply.txt

@@ -1,7 +1,5 @@
 Ran Apply for dir: `.` workspace: `default`
 
-<details><summary>Show Output</summary>
-
 ```diff
 null_resource.simple:
 null_resource.simple:
@@ -16,7 +14,5 @@ Outputs:
 
 var = default
 workspace = default
-
 ```
-</details>