diff --git a/builtin/provisioners/chef/resource_provisioner.go b/builtin/provisioners/chef/resource_provisioner.go index 6edf973f3..ea44e9fd0 100644 --- a/builtin/provisioners/chef/resource_provisioner.go +++ b/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 { - err = fmt.Errorf( - "Command %q exited with non-zero exit status: %d", cmd.Command, cmd.ExitStatus) + if cmd.Err() != nil { + return cmd.Err() } - // Wait for output to clean up - outW.Close() - errW.Close() - <-outDoneCh - <-errDoneCh + if cmd.ExitStatus() != 0 { + return fmt.Errorf("Command %q exited with non-zero exit status: %d", cmd.Command, cmd.ExitStatus()) + } - return err + return nil } func (p *provisioner) copyOutput(o terraform.UIOutput, r io.Reader, doneCh chan<- struct{}) { diff --git a/builtin/provisioners/habitat/resource_provisioner.go b/builtin/provisioners/habitat/resource_provisioner.go index aa404dae1..ada06a88c 100644 --- a/builtin/provisioners/habitat/resource_provisioner.go +++ b/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 { - err = fmt.Errorf( - "Command %q exited with non-zero exit status: %d", cmd.Command, cmd.ExitStatus) + if cmd.Err() != nil { + return cmd.Err() } - outW.Close() - errW.Close() - <-outDoneCh - <-errDoneCh + if cmd.ExitStatus() != 0 { + return fmt.Errorf("Command %q exited with non-zero exit status: %d", cmd.Command, cmd.ExitStatus()) + } - return err + return nil } func getBindFromString(bind string) (Bind, error) { diff --git a/builtin/provisioners/remote-exec/resource_provisioner.go b/builtin/provisioners/remote-exec/resource_provisioner.go index 378a282ed..8c3d671b9 100644 --- a/builtin/provisioners/remote-exec/resource_provisioner.go +++ b/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 diff --git a/builtin/provisioners/salt-masterless/resource_provisioner.go b/builtin/provisioners/salt-masterless/resource_provisioner.go index d206423bc..dd19d419e 100644 --- a/builtin/provisioners/salt-masterless/resource_provisioner.go +++ b/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 { - err = fmt.Errorf("Curl exited with non-zero exit status: %d", cmd.ExitStatus) + if cmd.Err() != nil { + 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 { - err = fmt.Errorf("install_salt.sh exited with non-zero exit status: %d", cmd.ExitStatus) + if cmd.Err() != nil { + 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 == nil { - err = fmt.Errorf("Bad exit status: %d", cmd.ExitStatus) - } - + if err = comm.Start(cmd); err != nil { err = fmt.Errorf("Error executing salt-call: %s", err) } + if err == nil { cmd.Wait() - if cmd.ExitStatus != 0 { - err = fmt.Errorf("Script exited with non-zero exit status: %d", cmd.ExitStatus) + if cmd.Err() != nil { + 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 == nil { - err = fmt.Errorf("Bad exit status: %d", cmd.ExitStatus) - } - + if err := comm.Start(cmd); err != nil { 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 { diff --git a/communicator/communicator_mock.go b/communicator/communicator_mock.go index f1c5ad5e6..7f887b4d3 100644 --- a/communicator/communicator_mock.go +++ b/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 } diff --git a/communicator/remote/command.go b/communicator/remote/command.go index ae16dfa88..0804e70fa 100644 --- a/communicator/remote/command.go +++ b/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 - // shouldn't be set manually by the user, but there is no harm in - // doing so. - Exited bool - - // Once Exited is true, this will contain the exit code of the process. - ExitStatus int + // Once Wait returns, his 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 -// should be called by communicators who are running a remote command in -// order to set that the command is done. -func (r *Cmd) SetExited(status int) { - r.Lock() - defer r.Unlock() +// Init must be called by the Communicator before executing the command. +func (c *Cmd) Init() { + c.Lock() + defer c.Unlock() - if r.exitCh == nil { - r.exitCh = make(chan struct{}) - } + c.exitCh = make(chan struct{}) +} - r.Exited = true - r.ExitStatus = status - close(r.exitCh) +// SetExitStatus stores the exit status of the remote command as well as any +// communicator related error. SetExitStatus then unblocks any pending calls +// 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() { - // Make sure our condition variable is initialized. - r.Lock() - if r.exitCh == nil { - r.exitCh = make(chan struct{}) - } - r.Unlock() - - <-r.exitCh +func (c *Cmd) Wait() { + <-c.exitCh } diff --git a/communicator/ssh/communicator.go b/communicator/ssh/communicator.go index f9d183976..4945f5dd8 100644 --- a/communicator/ssh/communicator.go +++ b/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 diff --git a/communicator/ssh/communicator_test.go b/communicator/ssh/communicator_test.go index 9a2d715f2..4eb3799ce 100644 --- a/communicator/ssh/communicator_test.go +++ b/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)) diff --git a/communicator/winrm/communicator.go b/communicator/winrm/communicator.go index 729e6eb51..90b9fe915 100644 --- a/communicator/winrm/communicator.go +++ b/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 diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 69da40cfc..f53bae1d9 100644 --- a/terraform/context_apply_test.go +++ b/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) } } diff --git a/terraform/eval_apply.go b/terraform/eval_apply.go index 36c98458e..b9b480646 100644 --- a/terraform/eval_apply.go +++ b/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) - } else { - return nil, err - } + *n.Error = multierror.Append(*n.Error, err) + return nil, err } { diff --git a/terraform/terraform_test.go b/terraform/terraform_test.go index 2deb44153..26c3f9880 100644 --- a/terraform/terraform_test.go +++ b/terraform/terraform_test.go @@ -636,11 +636,12 @@ const testTerraformApplyProvisionerFailCreateNoIdStr = ` ` const testTerraformApplyProvisionerFailCreateBeforeDestroyStr = ` -aws_instance.bar: (1 deposed) - ID = bar +aws_instance.bar: (tainted) (1 deposed) + ID = foo provider = provider.aws - require_new = abc - Deposed ID 1 = foo (tainted) + require_new = xyz + type = aws_instance + Deposed ID 1 = bar ` const testTerraformApplyProvisionerResourceRefStr = ` diff --git a/vendor/github.com/masterzen/winrm/client.go b/vendor/github.com/masterzen/winrm/client.go index 732dd61cb..c19515194 100644 --- a/vendor/github.com/masterzen/winrm/client.go +++ b/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) - go io.Copy(&errWriter, cmd.Stderr) + var wg sync.WaitGroup + 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) - go io.Copy(stdout, cmd.Stdout) - go io.Copy(stderr, cmd.Stderr) + var wg sync.WaitGroup + wg.Add(3) + + 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 diff --git a/vendor/vendor.json b/vendor/vendor.json index d6e32b45e..df59aea56 100644 --- a/vendor/vendor.json +++ b/vendor/vendor.json @@ -1983,16 +1983,20 @@ "revisionTime": "2016-06-08T18:30:07Z" }, { - "checksumSHA1": "8z5kCCFRsBkhXic9jxxeIV3bBn8=", + "checksumSHA1": "dVQEUn5TxdIAXczK7rh6qUrq44Q=", "path": "github.com/masterzen/winrm", - "revision": "a2df6b1315e6fd5885eb15c67ed259e85854125f", - "revisionTime": "2017-08-14T13:39:27Z" + "revision": "7e40f93ae939004a1ef3bd5ff5c88c756ee762bb", + "revisionTime": "2018-02-24T16:03:50Z", + "version": "master", + "versionExact": "master" }, { "checksumSHA1": "XFSXma+KmkhkIPsh4dTd/eyja5s=", "path": "github.com/masterzen/winrm/soap", - "revision": "a2df6b1315e6fd5885eb15c67ed259e85854125f", - "revisionTime": "2017-08-14T13:39:27Z" + "revision": "7e40f93ae939004a1ef3bd5ff5c88c756ee762bb", + "revisionTime": "2018-02-24T16:03:50Z", + "version": "master", + "versionExact": "master" }, { "checksumSHA1": "rCffFCN6TpDAN3Jylyo8RFzhQ9E=",