From e2f9204c11bc5623e9284832e2e5231f01112fa2 Mon Sep 17 00:00:00 2001 From: Matthew Hooker Date: Mon, 19 Mar 2018 15:52:43 -0700 Subject: [PATCH 1/5] replace nil ptr exceptions with infinite loops --- communicator/ssh/communicator.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/communicator/ssh/communicator.go b/communicator/ssh/communicator.go index d81ab750a..6fedb479c 100644 --- a/communicator/ssh/communicator.go +++ b/communicator/ssh/communicator.go @@ -423,6 +423,9 @@ func (c *comm) sftpUploadFile(path string, input io.Reader, client *sftp.Client, } cmd.Wait() if cmd.ExitStatus == 0 { + if fi == nil { + return fmt.Errorf("Upload path is a directory, unable to continue.") + } log.Printf("path is a directory; copying file into directory.") path = filepath.Join(path, filepath.Base((*fi).Name())) } @@ -586,6 +589,9 @@ func (c *comm) scpUploadSession(path string, input io.Reader, fi *os.FileInfo) e } cmd.Wait() if cmd.ExitStatus == 0 { + if fi == nil { + return fmt.Errorf("Upload path is a directory, unable to continue.") + } log.Printf("path is a directory; copying file into directory.") target_dir = path target_file = filepath.Base((*fi).Name()) From ccdee2550b599e087a92c402e2504006ab89447a Mon Sep 17 00:00:00 2001 From: Matthew Hooker Date: Tue, 20 Mar 2018 11:34:53 -0700 Subject: [PATCH 2/5] Treat any output directory test command as error. Surfaces any communications from the remote end during file uploads. For example, we might get notifications if we're logging in with the wrong user. Rather than swallow these, let's show them to the user. --- communicator/ssh/communicator.go | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/communicator/ssh/communicator.go b/communicator/ssh/communicator.go index 6fedb479c..b00b99220 100644 --- a/communicator/ssh/communicator.go +++ b/communicator/ssh/communicator.go @@ -413,7 +413,12 @@ func (c *comm) sftpUploadFile(path string, input io.Reader, client *sftp.Client, // find out if destination is a directory (this is to replicate rsync behavior) testDirectoryCommand := fmt.Sprintf(`test -d "%s"`, path) - cmd := &packer.RemoteCmd{Command: testDirectoryCommand} + var stdout, stderr bytes.Buffer + cmd := &packer.RemoteCmd{ + Command: testDirectoryCommand, + Stdout: &stdout, + Stderr: &stderr, + } err := c.Start(cmd) @@ -422,6 +427,12 @@ func (c *comm) sftpUploadFile(path string, input io.Reader, client *sftp.Client, return err } cmd.Wait() + if stdout.Len() > 0 { + return fmt.Errorf("%s", stdout.Bytes()) + } + if stderr.Len() > 0 { + return fmt.Errorf("%s", stderr.Bytes()) + } if cmd.ExitStatus == 0 { if fi == nil { return fmt.Errorf("Upload path is a directory, unable to continue.") @@ -579,7 +590,12 @@ func (c *comm) scpUploadSession(path string, input io.Reader, fi *os.FileInfo) e // find out if destination is a directory (this is to replicate rsync behavior) testDirectoryCommand := fmt.Sprintf(`test -d "%s"`, path) - cmd := &packer.RemoteCmd{Command: testDirectoryCommand} + var stdout, stderr bytes.Buffer + cmd := &packer.RemoteCmd{ + Command: testDirectoryCommand, + Stdout: &stdout, + Stderr: &stderr, + } err := c.Start(cmd) @@ -588,6 +604,12 @@ func (c *comm) scpUploadSession(path string, input io.Reader, fi *os.FileInfo) e return err } cmd.Wait() + if stdout.Len() > 0 { + return fmt.Errorf("%s", stdout.Bytes()) + } + if stderr.Len() > 0 { + return fmt.Errorf("%s", stderr.Bytes()) + } if cmd.ExitStatus == 0 { if fi == nil { return fmt.Errorf("Upload path is a directory, unable to continue.") From 67c7b9d1526e4c283dd94c1fd644b9b350a4f465 Mon Sep 17 00:00:00 2001 From: Matthew Hooker Date: Tue, 20 Mar 2018 16:28:23 -0700 Subject: [PATCH 3/5] display debug log levels --- communicator/ssh/communicator.go | 56 ++++++++++++++++---------------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/communicator/ssh/communicator.go b/communicator/ssh/communicator.go index b00b99220..329a62994 100644 --- a/communicator/ssh/communicator.go +++ b/communicator/ssh/communicator.go @@ -105,7 +105,7 @@ func (c *comm) Start(cmd *packer.RemoteCmd) (err error) { } } - log.Printf("starting remote command: %s", cmd.Command) + log.Printf("[DEBUG] starting remote command: %s", cmd.Command) err = session.Start(cmd.Command + "\n") if err != nil { return @@ -136,12 +136,12 @@ func (c *comm) Start(cmd *packer.RemoteCmd) (err error) { switch err.(type) { case *ssh.ExitError: exitStatus = err.(*ssh.ExitError).ExitStatus() - log.Printf("Remote command exited with '%d': %s", exitStatus, cmd.Command) + log.Printf("[ERROR] Remote command exited with '%d': %s", exitStatus, cmd.Command) case *ssh.ExitMissingError: - log.Printf("Remote command exited without exit status or exit signal.") + log.Printf("[ERROR] Remote command exited without exit status or exit signal.") exitStatus = packer.CmdDisconnect default: - log.Printf("Error occurred waiting for ssh session: %s", err.Error()) + log.Printf("[ERROR] Error occurred waiting for ssh session: %s", err.Error()) } } cmd.SetExited(exitStatus) @@ -158,7 +158,7 @@ func (c *comm) Upload(path string, input io.Reader, fi *os.FileInfo) error { } func (c *comm) UploadDir(dst string, src string, excl []string) error { - log.Printf("Upload dir '%s' to '%s'", src, dst) + log.Printf("[DEBUG] Upload dir '%s' to '%s'", src, dst) if c.config.UseSftp { return c.sftpUploadDirSession(dst, src, excl) } else { @@ -167,7 +167,7 @@ func (c *comm) UploadDir(dst string, src string, excl []string) error { } func (c *comm) DownloadDir(src string, dst string, excl []string) error { - log.Printf("Download dir '%s' to '%s'", src, dst) + log.Printf("[DEBUG] Download dir '%s' to '%s'", src, dst) scpFunc := func(w io.Writer, stdoutR *bufio.Reader) error { dirStack := []string{dst} for { @@ -202,7 +202,7 @@ func (c *comm) DownloadDir(src string, dst string, excl []string) error { var mode int64 var size int64 var name string - log.Printf("Download dir str:%s", fi) + log.Printf("[DEBUG] Download dir str:%s", fi) n, err := fmt.Sscanf(fi[1:], "%o %d %s", &mode, &size, &name) if err != nil || n != 3 { return fmt.Errorf("can't parse server response (%s)", fi) @@ -211,7 +211,7 @@ func (c *comm) DownloadDir(src string, dst string, excl []string) error { return fmt.Errorf("negative file size") } - log.Printf("Download dir mode:%0o size:%d name:%s", mode, size, name) + log.Printf("[DEBUG] Download dir mode:%0o size:%d name:%s", mode, size, name) dst = filepath.Join(dirStack...) switch fi[0] { @@ -246,7 +246,7 @@ func (c *comm) Download(path string, output io.Writer) error { } func (c *comm) newSession() (session *ssh.Session, err error) { - log.Println("opening new ssh session") + log.Println("[DEBUG] Opening new ssh session") if c.client == nil { err = errors.New("client not available") } else { @@ -254,7 +254,7 @@ func (c *comm) newSession() (session *ssh.Session, err error) { } if err != nil { - log.Printf("ssh session open error: '%s', attempting reconnect", err) + log.Printf("[ERROR] ssh session open error: '%s', attempting reconnect", err) if err := c.reconnect(); err != nil { return nil, err } @@ -279,7 +279,7 @@ func (c *comm) reconnect() (err error) { c.conn = nil c.client = nil - log.Printf("reconnecting to TCP connection for SSH") + log.Printf("[DEBUG] reconnecting to TCP connection for SSH") c.conn, err = c.config.Connection() if err != nil { // Explicitly set this to the REAL nil. Connection() can return @@ -290,7 +290,7 @@ func (c *comm) reconnect() (err error) { // http://golang.org/doc/faq#nil_error c.conn = nil - log.Printf("reconnection error: %s", err) + log.Printf("[ERROR] reconnection error: %s", err) return } @@ -298,7 +298,7 @@ func (c *comm) reconnect() (err error) { c.conn = &timeoutConn{c.conn, c.config.Timeout, c.config.Timeout} } - log.Printf("handshaking with SSH") + log.Printf("[DEBUG] handshaking with SSH") // Default timeout to 1 minute if it wasn't specified (zero value). For // when you need to handshake from low orbit. @@ -335,10 +335,10 @@ func (c *comm) reconnect() (err error) { } if err != nil { - log.Printf("handshake error: %s", err) + log.Printf("[ERROR] handshake error: %s", err) return } - log.Printf("handshake complete!") + log.Printf("[DEBUG] handshake complete!") if sshConn != nil { c.client = ssh.NewClient(sshConn, sshChan, req) } @@ -423,7 +423,7 @@ func (c *comm) sftpUploadFile(path string, input io.Reader, client *sftp.Client, err := c.Start(cmd) if err != nil { - log.Printf("Unable to check whether remote path is a dir: %s", err) + log.Printf("[ERROR] Unable to check whether remote path is a dir: %s", err) return err } cmd.Wait() @@ -466,7 +466,7 @@ func (c *comm) sftpUploadDirSession(dst string, src string, excl []string) error sftpFunc := func(client *sftp.Client) error { rootDst := dst if src[len(src)-1] != '/' { - log.Printf("No trailing slash, creating the source directory name") + log.Printf("[DEBUG] No trailing slash, creating the source directory name") rootDst = filepath.Join(dst, filepath.Base(src)) } walkFunc := func(path string, info os.FileInfo, err error) error { @@ -600,7 +600,7 @@ func (c *comm) scpUploadSession(path string, input io.Reader, fi *os.FileInfo) e err := c.Start(cmd) if err != nil { - log.Printf("Unable to check whether remote path is a dir: %s", err) + log.Printf("[ERROR] Unable to check whether remote path is a dir: %s", err) return err } cmd.Wait() @@ -649,7 +649,7 @@ func (c *comm) scpUploadDirSession(dst string, src string, excl []string) error } if src[len(src)-1] != '/' { - log.Printf("No trailing slash, creating the source directory name") + log.Printf("[DEBUG] No trailing slash, creating the source directory name") fi, err := os.Stat(src) if err != nil { return err @@ -750,7 +750,7 @@ func (c *comm) scpSession(scpCommand string, f func(io.Writer, *bufio.Reader) er // Start the sink mode on the other side // TODO(mitchellh): There are probably issues with shell escaping the path - log.Println("Starting remote scp process: ", scpCommand) + log.Println("[DEBUG] Starting remote scp process: ", scpCommand) if err := session.Start(scpCommand); err != nil { return err } @@ -758,7 +758,7 @@ func (c *comm) scpSession(scpCommand string, f func(io.Writer, *bufio.Reader) er // Call our callback that executes in the context of SCP. We ignore // EOF errors if they occur because it usually means that SCP prematurely // ended on the other side. - log.Println("Started SCP session, beginning transfers...") + log.Println("[DEBUG] Started SCP session, beginning transfers...") if err := f(stdinW, stdoutR); err != nil && err != io.EOF { return err } @@ -766,24 +766,24 @@ func (c *comm) scpSession(scpCommand string, f func(io.Writer, *bufio.Reader) er // Close the stdin, which sends an EOF, and then set w to nil so that // our defer func doesn't close it again since that is unsafe with // the Go SSH package. - log.Println("SCP session complete, closing stdin pipe.") + log.Println("[DEBUG] SCP session complete, closing stdin pipe.") stdinW.Close() stdinW = nil // Wait for the SCP connection to close, meaning it has consumed all // our data and has completed. Or has errored. - log.Println("Waiting for SSH session to complete.") + log.Println("[DEBUG] Waiting for SSH session to complete.") err = session.Wait() if err != nil { if exitErr, ok := err.(*ssh.ExitError); ok { // Otherwise, we have an ExitError, meaning we can just read // the exit status - log.Printf("non-zero exit status: %d", exitErr.ExitStatus()) + log.Printf("[DEBUG] non-zero exit status: %d", exitErr.ExitStatus()) stdoutB, err := ioutil.ReadAll(stdoutR) if err != nil { return err } - log.Printf("scp output: %s", stdoutB) + log.Printf("[DEBUG] scp output: %s", stdoutB) // If we exited with status 127, it means SCP isn't available. // Return a more descriptive error for that. @@ -797,7 +797,7 @@ func (c *comm) scpSession(scpCommand string, f func(io.Writer, *bufio.Reader) er return err } - log.Printf("scp stderr (length %d): %s", stderr.Len(), stderr.String()) + log.Printf("[DEBUG] scp stderr (length %d): %s", stderr.Len(), stderr.String()) return nil } @@ -854,7 +854,7 @@ func scpUploadFile(dst string, src io.Reader, w io.Writer, r *bufio.Reader, fi * mode = 0644 - log.Println("Copying input data into temporary file so we can read the length") + log.Println("[DEBUG] Copying input data into temporary file so we can read the length") if _, err := io.Copy(tf, src); err != nil { return err } @@ -897,7 +897,7 @@ func scpUploadFile(dst string, src io.Reader, w io.Writer, r *bufio.Reader, fi * } func scpUploadDirProtocol(name string, w io.Writer, r *bufio.Reader, f func() error, fi os.FileInfo) error { - log.Printf("SCP: starting directory upload: %s", name) + log.Printf("[DEBUG] SCP: starting directory upload: %s", name) mode := fi.Mode().Perm() From 732a532d0eda5ae74dd0b230f6adc92faafb660c Mon Sep 17 00:00:00 2001 From: Matthew Hooker Date: Tue, 20 Mar 2018 16:28:54 -0700 Subject: [PATCH 4/5] pass file info during shell file upload --- provisioner/shell/provisioner.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/provisioner/shell/provisioner.go b/provisioner/shell/provisioner.go index 8dd5796ce..bfecd27ff 100644 --- a/provisioner/shell/provisioner.go +++ b/provisioner/shell/provisioner.go @@ -230,6 +230,7 @@ func (p *Provisioner) Provision(ui packer.Ui, comm packer.Communicator) error { return fmt.Errorf("Error opening shell script: %s", err) } defer f.Close() + info, _ := f.Stat() // Compile the command p.config.ctx.Data = &ExecuteCommandTemplate{ @@ -257,7 +258,7 @@ func (p *Provisioner) Provision(ui packer.Ui, comm packer.Communicator) error { r = &UnixReader{Reader: r} } - if err := comm.Upload(p.config.RemotePath, r, nil); err != nil { + if err := comm.Upload(p.config.RemotePath, r, &info); err != nil { return fmt.Errorf("Error uploading script: %s", err) } From c5e0710a9a1c9d99a8241bc10ec21f3766e3719c Mon Sep 17 00:00:00 2001 From: Matthew Hooker Date: Tue, 20 Mar 2018 16:29:09 -0700 Subject: [PATCH 5/5] Do nothing if we detect destination is a directory We can't modify the destination path in the communicator because it breaks assumptions in the provisioners. For example, we try to chmod in the shell provisioner. The chmod command uses the path as supplied by the user. If the communicator decides to rewrite the path, the provisioner doesn't know that, and so tries to chmod the wrong thing. The best we can do is detect that the destination is a directory and fail. Also correctly surface output from sftp uploader. --- communicator/ssh/communicator.go | 39 ++++++++++++++------------------ 1 file changed, 17 insertions(+), 22 deletions(-) diff --git a/communicator/ssh/communicator.go b/communicator/ssh/communicator.go index 329a62994..218c11be6 100644 --- a/communicator/ssh/communicator.go +++ b/communicator/ssh/communicator.go @@ -413,11 +413,9 @@ func (c *comm) sftpUploadFile(path string, input io.Reader, client *sftp.Client, // find out if destination is a directory (this is to replicate rsync behavior) testDirectoryCommand := fmt.Sprintf(`test -d "%s"`, path) - var stdout, stderr bytes.Buffer + cmd := &packer.RemoteCmd{ Command: testDirectoryCommand, - Stdout: &stdout, - Stderr: &stderr, } err := c.Start(cmd) @@ -427,18 +425,10 @@ func (c *comm) sftpUploadFile(path string, input io.Reader, client *sftp.Client, return err } cmd.Wait() - if stdout.Len() > 0 { - return fmt.Errorf("%s", stdout.Bytes()) - } - if stderr.Len() > 0 { - return fmt.Errorf("%s", stderr.Bytes()) - } if cmd.ExitStatus == 0 { - if fi == nil { - return fmt.Errorf("Upload path is a directory, unable to continue.") - } - log.Printf("path is a directory; copying file into directory.") - path = filepath.Join(path, filepath.Base((*fi).Name())) + return fmt.Errorf( + "Destination path (%s) is a directory that already exists. "+ + "Please ensure the destination is writable.", path) } f, err := client.Create(path) @@ -553,7 +543,7 @@ func (c *comm) sftpDownloadSession(path string, output io.Writer) error { func (c *comm) sftpSession(f func(*sftp.Client) error) error { client, err := c.newSftpClient() if err != nil { - return err + return fmt.Errorf("sftpSession error: %s", err.Error()) } defer client.Close() @@ -579,7 +569,15 @@ func (c *comm) newSftpClient() (*sftp.Client, error) { return nil, err } - return sftp.NewClientPipe(pr, pw) + // Capture stdout so we can return errors to the user + var stdout bytes.Buffer + tee := io.TeeReader(pr, &stdout) + client, err := sftp.NewClientPipe(tee, pw) + if err != nil && stdout.Len() > 0 { + log.Printf("[ERROR] Upload failed: %s", stdout.Bytes()) + } + + return client, err } func (c *comm) scpUploadSession(path string, input io.Reader, fi *os.FileInfo) error { @@ -611,12 +609,9 @@ func (c *comm) scpUploadSession(path string, input io.Reader, fi *os.FileInfo) e return fmt.Errorf("%s", stderr.Bytes()) } if cmd.ExitStatus == 0 { - if fi == nil { - return fmt.Errorf("Upload path is a directory, unable to continue.") - } - log.Printf("path is a directory; copying file into directory.") - target_dir = path - target_file = filepath.Base((*fi).Name()) + return fmt.Errorf( + "Destination path (%s) is a directory that already exists. "+ + "Please ensure the destination is writable.", path) } // On windows, filepath.Dir uses backslash separators (ie. "\tmp").