Merge pull request #17596 from hashicorp/jbardin/remote-exec-error

fix improper remote-exec timeout and communicator error handling

builtin/provisioners/chef/resource_provisioner.go

@@ -684,6 +684,13 @@ func (p *provisioner) runCommand(o terraform.UIOutput, comm communicator.Communi
 	errDoneCh := make(chan struct{})
 	go p.copyOutput(o, outR, outDoneCh)
 	go p.copyOutput(o, errR, errDoneCh)
+	go func() {
+		// Wait for output to clean up
+		outW.Close()
+		errW.Close()
+		<-outDoneCh
+		<-errDoneCh
+	}()
 
 	cmd := &remote.Cmd{
 		Command: command,
@@ -697,18 +704,15 @@ func (p *provisioner) runCommand(o terraform.UIOutput, comm communicator.Communi
 	}
 
 	cmd.Wait()
-	if cmd.ExitStatus != 0 {
+	if cmd.Err() != nil {
-		err = fmt.Errorf(
+		return cmd.Err()
-			"Command %q exited with non-zero exit status: %d", cmd.Command, cmd.ExitStatus)
 	}
 
-	// Wait for output to clean up
+	if cmd.ExitStatus() != 0 {
-	outW.Close()
+		return fmt.Errorf("Command %q exited with non-zero exit status: %d", cmd.Command, cmd.ExitStatus())
-	errW.Close()
+	}
-	<-outDoneCh
-	<-errDoneCh
 
-	return err
+	return nil
 }
 
 func (p *provisioner) copyOutput(o terraform.UIOutput, r io.Reader, doneCh chan<- struct{}) {

builtin/provisioners/habitat/resource_provisioner.go

@@ -740,12 +740,17 @@ func (p *provisioner) copyOutput(o terraform.UIOutput, r io.Reader, doneCh chan<
 func (p *provisioner) runCommand(o terraform.UIOutput, comm communicator.Communicator, command string) error {
 	outR, outW := io.Pipe()
 	errR, errW := io.Pipe()
-	var err error
 
 	outDoneCh := make(chan struct{})
 	errDoneCh := make(chan struct{})
 	go p.copyOutput(o, outR, outDoneCh)
 	go p.copyOutput(o, errR, errDoneCh)
+	defer func() {
+		outW.Close()
+		errW.Close()
+		<-outDoneCh
+		<-errDoneCh
+	}()
 
 	cmd := &remote.Cmd{
 		Command: command,
@@ -753,22 +758,20 @@ func (p *provisioner) runCommand(o terraform.UIOutput, comm communicator.Communi
 		Stderr:  errW,
 	}
 
-	if err = comm.Start(cmd); err != nil {
+	if err := comm.Start(cmd); err != nil {
 		return fmt.Errorf("Error executing command %q: %v", cmd.Command, err)
 	}
 
 	cmd.Wait()
-	if cmd.ExitStatus != 0 {
+	if cmd.Err() != nil {
-		err = fmt.Errorf(
+		return cmd.Err()
-			"Command %q exited with non-zero exit status: %d", cmd.Command, cmd.ExitStatus)
 	}
 
-	outW.Close()
+	if cmd.ExitStatus() != 0 {
-	errW.Close()
+		return fmt.Errorf("Command %q exited with non-zero exit status: %d", cmd.Command, cmd.ExitStatus())
-	<-outDoneCh
+	}
-	<-errDoneCh
 
-	return err
+	return nil
 }
 
 func getBindFromString(bind string) (Bind, error) {

builtin/provisioners/remote-exec/resource_provisioner.go

@@ -156,10 +156,6 @@ func runScripts(
 	o terraform.UIOutput,
 	comm communicator.Communicator,
 	scripts []io.ReadCloser) error {
-	// Wrap out context in a cancelation function that we use to
-	// kill the connection.
-	ctx, cancelFunc := context.WithTimeout(ctx, comm.Timeout())
-	defer cancelFunc()
 
 	// Wait for the context to end and then disconnect
 	go func() {
@@ -200,10 +196,14 @@ func runScripts(
 		if err := comm.Start(cmd); err != nil {
 			return fmt.Errorf("Error starting script: %v", err)
 		}
-
 		cmd.Wait()
-		if cmd.ExitStatus != 0 {
+
-			err = fmt.Errorf("Script exited with non-zero exit status: %d", cmd.ExitStatus)
+		if err := cmd.Err(); err != nil {
+			return fmt.Errorf("Remote command exited with error: %s", err)
+		}
+
+		if cmd.ExitStatus() != 0 {
+			err = fmt.Errorf("Script exited with non-zero exit status: %d", cmd.ExitStatus())
 		}
 
 		// Upload a blank follow up file in the same path to prevent residual

builtin/provisioners/salt-masterless/resource_provisioner.go

@@ -164,8 +164,10 @@ func applyFn(ctx context.Context) error {
 
 		if err == nil {
 			cmd.Wait()
-			if cmd.ExitStatus != 0 {
+			if cmd.Err() != nil {
-				err = fmt.Errorf("Curl exited with non-zero exit status: %d", cmd.ExitStatus)
+				err = cmd.Err()
+			} else if cmd.ExitStatus() != 0 {
+				err = fmt.Errorf("Curl exited with non-zero exit status: %d", cmd.ExitStatus())
 			}
 		}
 
@@ -188,8 +190,10 @@ func applyFn(ctx context.Context) error {
 
 		if err == nil {
 			cmd.Wait()
-			if cmd.ExitStatus != 0 {
+			if cmd.Err() != nil {
-				err = fmt.Errorf("install_salt.sh exited with non-zero exit status: %d", cmd.ExitStatus)
+				err = cmd.Err()
+			} else if cmd.ExitStatus() != 0 {
+				err = fmt.Errorf("install_salt.sh exited with non-zero exit status: %d", cmd.ExitStatus())
 			}
 		}
 		// Wait for output to clean up
@@ -277,17 +281,16 @@ func applyFn(ctx context.Context) error {
 		Stdout:  outW,
 		Stderr:  errW,
 	}
-	if err = comm.Start(cmd); err != nil || cmd.ExitStatus != 0 {
+	if err = comm.Start(cmd); err != nil {
-		if err == nil {
-			err = fmt.Errorf("Bad exit status: %d", cmd.ExitStatus)
-		}
-
 		err = fmt.Errorf("Error executing salt-call: %s", err)
 	}
+
 	if err == nil {
 		cmd.Wait()
-		if cmd.ExitStatus != 0 {
+		if cmd.Err() != nil {
-			err = fmt.Errorf("Script exited with non-zero exit status: %d", cmd.ExitStatus)
+			err = cmd.Err()
+		} else if cmd.ExitStatus() != 0 {
+			err = fmt.Errorf("Script exited with non-zero exit status: %d", cmd.ExitStatus())
 		}
 	}
 	// Wait for output to clean up
@@ -354,14 +357,15 @@ func (p *provisioner) uploadFile(o terraform.UIOutput, comm communicator.Communi
 func (p *provisioner) moveFile(o terraform.UIOutput, comm communicator.Communicator, dst, src string) error {
 	o.Output(fmt.Sprintf("Moving %s to %s", src, dst))
 	cmd := &remote.Cmd{Command: fmt.Sprintf(p.sudo("mv %s %s"), src, dst)}
-	if err := comm.Start(cmd); err != nil || cmd.ExitStatus != 0 {
+	if err := comm.Start(cmd); err != nil {
-		if err == nil {
-			err = fmt.Errorf("Bad exit status: %d", cmd.ExitStatus)
-		}
-
 		return fmt.Errorf("Unable to move %s to %s: %s", src, dst, err)
 	}
-	return nil
+	cmd.Wait()
+	if cmd.ExitStatus() != 0 {
+		return fmt.Errorf("Unable to move %s to %s: exit status: %d", src, dst, cmd.ExitStatus())
+	}
+
+	return cmd.Err()
 }
 
 func (p *provisioner) createDir(o terraform.UIOutput, comm communicator.Communicator, dir string) error {
@@ -372,10 +376,12 @@ func (p *provisioner) createDir(o terraform.UIOutput, comm communicator.Communic
 	if err := comm.Start(cmd); err != nil {
 		return err
 	}
-	if cmd.ExitStatus != 0 {
+
+	cmd.Wait()
+	if cmd.ExitStatus() != 0 {
 		return fmt.Errorf("Non-zero exit status.")
 	}
-	return nil
+	return cmd.Err()
 }
 
 func (p *provisioner) removeDir(o terraform.UIOutput, comm communicator.Communicator, dir string) error {
@@ -386,10 +392,11 @@ func (p *provisioner) removeDir(o terraform.UIOutput, comm communicator.Communic
 	if err := comm.Start(cmd); err != nil {
 		return err
 	}
-	if cmd.ExitStatus != 0 {
+	cmd.Wait()
+	if cmd.ExitStatus() != 0 {
 		return fmt.Errorf("Non-zero exit status.")
 	}
-	return nil
+	return cmd.Err()
 }
 
 func (p *provisioner) uploadDir(o terraform.UIOutput, comm communicator.Communicator, dst, src string, ignore []string) error {

communicator/communicator_mock.go

@@ -42,11 +42,13 @@ func (c *MockCommunicator) ScriptPath() string {
 
 // Start implementation of communicator.Communicator interface
 func (c *MockCommunicator) Start(r *remote.Cmd) error {
+	r.Init()
+
 	if !c.Commands[r.Command] {
 		return fmt.Errorf("Command not found!")
 	}
 
-	r.SetExited(0)
+	r.SetExitStatus(0, nil)
 
 	return nil
 }

communicator/remote/command.go

@@ -23,45 +23,59 @@ type Cmd struct {
 	Stdout io.Writer
 	Stderr io.Writer
 
-	// This will be set to true when the remote command has exited. It
+	// Once Wait returns, his will contain the exit code of the process.
-	// shouldn't be set manually by the user, but there is no harm in
+	exitStatus int
-	// doing so.
-	Exited bool
-
-	// Once Exited is true, this will contain the exit code of the process.
-	ExitStatus int
 
 	// Internal fields
 	exitCh chan struct{}
 
+	// err is used to store any error reported by the Communicator during
+	// execution.
+	err error
+
 	// This thing is a mutex, lock when making modifications concurrently
 	sync.Mutex
 }
 
-// SetExited is a helper for setting that this process is exited. This
+// Init must be called by the Communicator before executing the command.
-// should be called by communicators who are running a remote command in
+func (c *Cmd) Init() {
-// order to set that the command is done.
+	c.Lock()
-func (r *Cmd) SetExited(status int) {
+	defer c.Unlock()
-	r.Lock()
-	defer r.Unlock()
 
-	if r.exitCh == nil {
+	c.exitCh = make(chan struct{})
-		r.exitCh = make(chan struct{})
+}
-	}
 
-	r.Exited = true
+// SetExitStatus stores the exit status of the remote command as well as any
-	r.ExitStatus = status
+// communicator related error. SetExitStatus then unblocks any pending calls
-	close(r.exitCh)
+// to Wait.
+// This should only be called by communicators executing the remote.Cmd.
+func (c *Cmd) SetExitStatus(status int, err error) {
+	c.Lock()
+	defer c.Unlock()
+
+	c.exitStatus = status
+	c.err = err
+
+	close(c.exitCh)
+}
+
+// Err returns any communicator related error.
+func (c *Cmd) Err() error {
+	c.Lock()
+	defer c.Unlock()
+
+	return c.err
+}
+
+// ExitStatus returns the exit status of the remote command
+func (c *Cmd) ExitStatus() int {
+	c.Lock()
+	defer c.Unlock()
+
+	return c.exitStatus
 }
 
 // Wait waits for the remote command to complete.
-func (r *Cmd) Wait() {
+func (c *Cmd) Wait() {
-	// Make sure our condition variable is initialized.
+	<-c.exitCh
-	r.Lock()
-	if r.exitCh == nil {
-		r.exitCh = make(chan struct{})
-	}
-	r.Unlock()
-
-	<-r.exitCh
 }

communicator/ssh/communicator.go

@@ -243,6 +243,8 @@ func (c *Communicator) ScriptPath() string {
 
 // Start implementation of communicator.Communicator interface
 func (c *Communicator) Start(cmd *remote.Cmd) error {
+	cmd.Init()
+
 	session, err := c.newSession()
 	if err != nil {
 		return err
@@ -267,7 +269,7 @@ func (c *Communicator) Start(cmd *remote.Cmd) error {
 	}
 
 	log.Printf("starting remote command: %s", cmd.Command)
-	err = session.Start(cmd.Command + "\n")
+	err = session.Start(strings.TrimSpace(cmd.Command) + "\n")
 	if err != nil {
 		return err
 	}
@@ -286,8 +288,8 @@ func (c *Communicator) Start(cmd *remote.Cmd) error {
 			}
 		}
 
+		cmd.SetExitStatus(exitStatus, err)
 		log.Printf("remote command exited with '%d': %s", exitStatus, cmd.Command)
-		cmd.SetExited(exitStatus)
 	}()
 
 	return nil
@@ -358,10 +360,10 @@ func (c *Communicator) UploadScript(path string, input io.Reader) error {
 				"machine: %s", err)
 	}
 	cmd.Wait()
-	if cmd.ExitStatus != 0 {
+	if cmd.ExitStatus() != 0 {
 		return fmt.Errorf(
 			"Error chmodding script file to 0777 in remote "+
-				"machine %d: %s %s", cmd.ExitStatus, stdout.String(), stderr.String())
+				"machine %d: %s %s", cmd.ExitStatus(), stdout.String(), stderr.String())
 	}
 
 	return nil

communicator/ssh/communicator_test.go

@@ -17,6 +17,7 @@ import (
 	"strconv"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/hashicorp/terraform/communicator/remote"
 	"github.com/hashicorp/terraform/terraform"
@@ -178,6 +179,55 @@ func TestStart(t *testing.T) {
 	}
 }
 
+func TestLostConnection(t *testing.T) {
+	address := newMockLineServer(t, nil)
+	parts := strings.Split(address, ":")
+
+	r := &terraform.InstanceState{
+		Ephemeral: terraform.EphemeralState{
+			ConnInfo: map[string]string{
+				"type":     "ssh",
+				"user":     "user",
+				"password": "pass",
+				"host":     parts[0],
+				"port":     parts[1],
+				"timeout":  "30s",
+			},
+		},
+	}
+
+	c, err := New(r)
+	if err != nil {
+		t.Fatalf("error creating communicator: %s", err)
+	}
+
+	var cmd remote.Cmd
+	stdout := new(bytes.Buffer)
+	cmd.Command = "echo foo"
+	cmd.Stdout = stdout
+
+	err = c.Start(&cmd)
+	if err != nil {
+		t.Fatalf("error executing remote command: %s", err)
+	}
+
+	// The test server can't execute anything, so Wait will block, unless
+	// there's an error.  Disconnect the communicator transport, to cause the
+	// command to fail.
+	go func() {
+		time.Sleep(100 * time.Millisecond)
+		c.Disconnect()
+	}()
+
+	cmd.Wait()
+	if cmd.Err() == nil {
+		t.Fatal("expected communicator error")
+	}
+	if cmd.ExitStatus() != 0 {
+		t.Fatal("command should not have returned an exit status")
+	}
+}
+
 func TestHostKey(t *testing.T) {
 	// get the server's public key
 	signer, err := ssh.ParsePrivateKey([]byte(testServerPrivateKey))

communicator/winrm/communicator.go

@@ -131,6 +131,8 @@ func (c *Communicator) ScriptPath() string {
 
 // Start implementation of communicator.Communicator interface
 func (c *Communicator) Start(rc *remote.Cmd) error {
+	rc.Init()
+
 	err := c.Connect(nil)
 	if err != nil {
 		return err
@@ -168,7 +170,8 @@ func runCommand(shell *winrm.Shell, cmd *winrm.Command, rc *remote.Cmd) {
 
 	cmd.Wait()
 	wg.Wait()
-	rc.SetExited(cmd.ExitCode())
+
+	rc.SetExitStatus(cmd.ExitCode(), nil)
 }
 
 // Upload implementation of communicator.Communicator interface

terraform/context_apply_test.go

@@ -4482,7 +4482,7 @@ func TestContext2Apply_provisionerFail_createBeforeDestroy(t *testing.T) {
 	actual := strings.TrimSpace(state.String())
 	expected := strings.TrimSpace(testTerraformApplyProvisionerFailCreateBeforeDestroyStr)
 	if actual != expected {
-		t.Fatalf("bad: \n%s", actual)
+		t.Fatalf("expected:\n%s\n:got\n%s", expected, actual)
 	}
 }
 

terraform/eval_apply.go

@@ -227,11 +227,8 @@ func (n *EvalApplyProvisioners) Eval(ctx EvalContext) (interface{}, error) {
 			state.Tainted = true
 		}
 
-		if n.Error != nil {
+		*n.Error = multierror.Append(*n.Error, err)
-			*n.Error = multierror.Append(*n.Error, err)
+		return nil, err
-		} else {
-			return nil, err
-		}
 	}
 
 	{

terraform/terraform_test.go

@@ -636,11 +636,12 @@ const testTerraformApplyProvisionerFailCreateNoIdStr = `
 `
 
 const testTerraformApplyProvisionerFailCreateBeforeDestroyStr = `
-aws_instance.bar: (1 deposed)
+aws_instance.bar: (tainted) (1 deposed)
-  ID = bar
+  ID = foo
   provider = provider.aws
-  require_new = abc
+  require_new = xyz
-  Deposed ID 1 = foo (tainted)
+  type = aws_instance
+  Deposed ID 1 = bar
 `
 
 const testTerraformApplyProvisionerResourceRefStr = `

vendor/github.com/masterzen/winrm/client.go

@@ -152,10 +152,20 @@ func (c *Client) RunWithString(command string, stdin string) (string, string, in
 	}
 
 	var outWriter, errWriter bytes.Buffer
-	go io.Copy(&outWriter, cmd.Stdout)
+	var wg sync.WaitGroup
-	go io.Copy(&errWriter, cmd.Stderr)
+	wg.Add(2)
+	go func() {
+		defer wg.Done()
+		io.Copy(&outWriter, cmd.Stdout)
+	}()
+
+	go func() {
+		defer wg.Done()
+		io.Copy(&errWriter, cmd.Stderr)
+	}()
 
 	cmd.Wait()
+	wg.Wait()
 
 	return outWriter.String(), errWriter.String(), cmd.ExitCode(), cmd.err
 }
@@ -176,11 +186,24 @@ func (c Client) RunWithInput(command string, stdout, stderr io.Writer, stdin io.
 		return 1, err
 	}
 
-	go io.Copy(cmd.Stdin, stdin)
+	var wg sync.WaitGroup
-	go io.Copy(stdout, cmd.Stdout)
+	wg.Add(3)
-	go io.Copy(stderr, cmd.Stderr)
+
+	go func() {
+		defer wg.Done()
+		io.Copy(cmd.Stdin, stdin)
+	}()
+	go func() {
+		defer wg.Done()
+		io.Copy(stdout, cmd.Stdout)
+	}()
+	go func() {
+		defer wg.Done()
+		io.Copy(stderr, cmd.Stderr)
+	}()
 
 	cmd.Wait()
+	wg.Wait()
 
 	return cmd.ExitCode(), cmd.err
 

vendor/vendor.json

@@ -1983,16 +1983,20 @@
 			"revisionTime": "2016-06-08T18:30:07Z"
 		},
 		{
-			"checksumSHA1": "8z5kCCFRsBkhXic9jxxeIV3bBn8=",
+			"checksumSHA1": "dVQEUn5TxdIAXczK7rh6qUrq44Q=",
 			"path": "github.com/masterzen/winrm",
-			"revision": "a2df6b1315e6fd5885eb15c67ed259e85854125f",
+			"revision": "7e40f93ae939004a1ef3bd5ff5c88c756ee762bb",
-			"revisionTime": "2017-08-14T13:39:27Z"
+			"revisionTime": "2018-02-24T16:03:50Z",
+			"version": "master",
+			"versionExact": "master"
 		},
 		{
 			"checksumSHA1": "XFSXma+KmkhkIPsh4dTd/eyja5s=",
 			"path": "github.com/masterzen/winrm/soap",
-			"revision": "a2df6b1315e6fd5885eb15c67ed259e85854125f",
+			"revision": "7e40f93ae939004a1ef3bd5ff5c88c756ee762bb",
-			"revisionTime": "2017-08-14T13:39:27Z"
+			"revisionTime": "2018-02-24T16:03:50Z",
+			"version": "master",
+			"versionExact": "master"
 		},
 		{
 			"checksumSHA1": "rCffFCN6TpDAN3Jylyo8RFzhQ9E=",