Adding some abstractions for the communicators

This is needed as preperation for adding WinRM support. There is still
one error in the tests which needs another look, but other than that it
seems like were now ready to start working on the WinRM part…

builtin/provisioners/file/resource_provisioner.go

@@ -6,25 +6,22 @@ import (
 	"os"
 	"time"
 
+	"github.com/hashicorp/terraform/communicator"
 	"github.com/hashicorp/terraform/helper/config"
-	helper "github.com/hashicorp/terraform/helper/ssh"
 	"github.com/hashicorp/terraform/terraform"
 	"github.com/mitchellh/go-homedir"
 )
 
+// ResourceProvisioner represents a file provisioner
 type ResourceProvisioner struct{}
 
+// Apply executes the file provisioner
 func (p *ResourceProvisioner) Apply(
 	o terraform.UIOutput,
 	s *terraform.InstanceState,
 	c *terraform.ResourceConfig) error {
-	// Ensure the connection type is SSH
+	// Get a new communicator
-	if err := helper.VerifySSH(s); err != nil {
+	comm, err := communicator.New(s)
-		return err
-	}
-
-	// Get the SSH configuration
-	conf, err := helper.ParseSSHConfig(s)
 	if err != nil {
 		return err
 	}
@@ -46,9 +43,10 @@ func (p *ResourceProvisioner) Apply(
 	if !ok {
 		return fmt.Errorf("Unsupported 'destination' type! Must be string.")
 	}
-	return p.copyFiles(conf, src, dst)
+	return p.copyFiles(comm, src, dst)
 }
 
+// Validate checks if the required arguments are configured
 func (p *ResourceProvisioner) Validate(c *terraform.ResourceConfig) (ws []string, es []error) {
 	v := &config.Validator{
 		Required: []string{
@@ -60,24 +58,16 @@ func (p *ResourceProvisioner) Validate(c *terraform.ResourceConfig) (ws []string
 }
 
 // copyFiles is used to copy the files from a source to a destination
-func (p *ResourceProvisioner) copyFiles(conf *helper.SSHConfig, src, dst string) error {
+func (p *ResourceProvisioner) copyFiles(comm communicator.Communicator, src, dst string) error {
-	// Get the SSH client config
+	// Wait and retry until we establish the connection
-	config, err := helper.PrepareConfig(conf)
+	err := retryFunc(comm.Timeout(), func() error {
-	if err != nil {
+		err := comm.Connect(nil)
-		return err
-	}
-	defer config.CleanupConfig()
-
-	// Wait and retry until we establish the SSH connection
-	var comm *helper.SSHCommunicator
-	err = retryFunc(conf.TimeoutVal, func() error {
-		host := fmt.Sprintf("%s:%d", conf.Host, conf.Port)
-		comm, err = helper.New(host, config)
 		return err
 	})
 	if err != nil {
 		return err
 	}
+	defer comm.Disconnect()
 
 	info, err := os.Stat(src)
 	if err != nil {

builtin/provisioners/remote-exec/resource_provisioner.go

@@ -10,29 +10,22 @@ import (
 	"strings"
 	"time"
 
-	helper "github.com/hashicorp/terraform/helper/ssh"
+	"github.com/hashicorp/terraform/communicator"
+	"github.com/hashicorp/terraform/communicator/remote"
 	"github.com/hashicorp/terraform/terraform"
 	"github.com/mitchellh/go-linereader"
 )
 
-const (
+// ResourceProvisioner represents a remote exec provisioner
-	// DefaultShebang is added at the top of the script file
-	DefaultShebang = "#!/bin/sh"
-)
-
 type ResourceProvisioner struct{}
 
+// Apply executes the remote exec provisioner
 func (p *ResourceProvisioner) Apply(
 	o terraform.UIOutput,
 	s *terraform.InstanceState,
 	c *terraform.ResourceConfig) error {
-	// Ensure the connection type is SSH
+	// Get a new communicator
-	if err := helper.VerifySSH(s); err != nil {
+	comm, err := communicator.New(s)
-		return err
-	}
-
-	// Get the SSH configuration
-	conf, err := helper.ParseSSHConfig(s)
 	if err != nil {
 		return err
 	}
@@ -47,12 +40,13 @@ func (p *ResourceProvisioner) Apply(
 	}
 
 	// Copy and execute each script
-	if err := p.runScripts(o, conf, scripts); err != nil {
+	if err := p.runScripts(o, comm, scripts); err != nil {
 		return err
 	}
 	return nil
 }
 
+// Validate checks if the required arguments are configured
 func (p *ResourceProvisioner) Validate(c *terraform.ResourceConfig) (ws []string, es []error) {
 	num := 0
 	for name := range c.Raw {
@@ -76,7 +70,7 @@ func (p *ResourceProvisioner) Validate(c *terraform.ResourceConfig) (ws []string
 // generateScript takes the configuration and creates a script to be executed
 // from the inline configs
 func (p *ResourceProvisioner) generateScript(c *terraform.ResourceConfig) (string, error) {
-	lines := []string{DefaultShebang}
+	var lines []string
 	command, ok := c.Config["inline"]
 	if ok {
 		switch cmd := command.(type) {
@@ -165,46 +159,20 @@ func (p *ResourceProvisioner) collectScripts(c *terraform.ResourceConfig) ([]io.
 // runScripts is used to copy and execute a set of scripts
 func (p *ResourceProvisioner) runScripts(
 	o terraform.UIOutput,
-	conf *helper.SSHConfig,
+	comm communicator.Communicator,
 	scripts []io.ReadCloser) error {
-	// Get the SSH client config
+	// Wait and retry until we establish the connection
-	config, err := helper.PrepareConfig(conf)
+	err := retryFunc(comm.Timeout(), func() error {
-	if err != nil {
+		err := comm.Connect(o)
-		return err
-	}
-	defer config.CleanupConfig()
-
-	o.Output(fmt.Sprintf(
-		"Connecting to remote host via SSH...\n"+
-			"  Host: %s\n"+
-			"  User: %s\n"+
-			"  Password: %v\n"+
-			"  Private key: %v"+
-			"  SSH Agent: %v",
-		conf.Host, conf.User,
-		conf.Password != "",
-		conf.KeyFile != "",
-		conf.Agent,
-	))
-
-	// Wait and retry until we establish the SSH connection
-	var comm *helper.SSHCommunicator
-	err = retryFunc(conf.TimeoutVal, func() error {
-		host := fmt.Sprintf("%s:%d", conf.Host, conf.Port)
-		comm, err = helper.New(host, config)
-		if err != nil {
-			o.Output(fmt.Sprintf("Connection error, will retry: %s", err))
-		}
-
 		return err
 	})
 	if err != nil {
 		return err
 	}
+	defer comm.Disconnect()
 
-	o.Output("Connected! Executing scripts...")
 	for _, script := range scripts {
-		var cmd *helper.RemoteCmd
+		var cmd *remote.Cmd
 		outR, outW := io.Pipe()
 		errR, errW := io.Pipe()
 		outDoneCh := make(chan struct{})
@@ -212,30 +180,20 @@ func (p *ResourceProvisioner) runScripts(
 		go p.copyOutput(o, outR, outDoneCh)
 		go p.copyOutput(o, errR, errDoneCh)
 
-		err := retryFunc(conf.TimeoutVal, func() error {
+		err = retryFunc(comm.Timeout(), func() error {
-			remotePath := conf.RemotePath()
+			if err := comm.UploadScript(comm.ScriptPath(), script); err != nil {
-
-			if err := comm.Upload(remotePath, script); err != nil {
 				return fmt.Errorf("Failed to upload script: %v", err)
 			}
-			cmd = &helper.RemoteCmd{
-				Command: fmt.Sprintf("chmod 0777 %s", remotePath),
-			}
-			if err := comm.Start(cmd); err != nil {
-				return fmt.Errorf(
-					"Error chmodding script file to 0777 in remote "+
-						"machine: %s", err)
-			}
-			cmd.Wait()
 
-			cmd = &helper.RemoteCmd{
+			cmd = &remote.Cmd{
-				Command: remotePath,
+				Command: comm.ScriptPath(),
 				Stdout:  outW,
 				Stderr:  errW,
 			}
 			if err := comm.Start(cmd); err != nil {
 				return fmt.Errorf("Error starting script: %v", err)
 			}
+
 			return nil
 		})
 		if err == nil {

communicator/communicator.go

@@ -0,0 +1,52 @@
+package communicator
+
+import (
+	"fmt"
+	"io"
+	"time"
+
+	"github.com/hashicorp/terraform/communicator/remote"
+	"github.com/hashicorp/terraform/communicator/ssh"
+	"github.com/hashicorp/terraform/terraform"
+)
+
+// Communicator is an interface that must be implemented by all communicators
+// used for any of the provisioners
+type Communicator interface {
+	// Connect is used to setup the connection
+	Connect(terraform.UIOutput) error
+
+	// Disconnect is used to terminate the connection
+	Disconnect() error
+
+	// Timeout returns the configured connection timeout
+	Timeout() time.Duration
+
+	// ScriptPath returns the configured script path
+	ScriptPath() string
+
+	// Start executes a remote command in a new session
+	Start(*remote.Cmd) error
+
+	// Upload is used to upload a single file
+	Upload(string, io.Reader) error
+
+	// UploadScript is used to upload a file as a executable script
+	UploadScript(string, io.Reader) error
+
+	// UploadDir is used to upload a directory
+	UploadDir(string, string, []string) error
+}
+
+// New returns a configured Communicator or an error if the connection type is not supported
+func New(s *terraform.InstanceState) (Communicator, error) {
+	connType := s.Ephemeral.ConnInfo["type"]
+	switch connType {
+	case "ssh", "": // The default connection type is ssh, so if connType is empty use ssh
+		return ssh.New(s)
+	//case "winrm":
+	//	return winrm.New()
+	default:
+		return nil, fmt.Errorf("Connection type '%s' not supported", connType)
+	}
+}

communicator/communicator_test.go

@@ -0,0 +1,24 @@
+package communicator
+
+import (
+	"testing"
+
+	"github.com/hashicorp/terraform/terraform"
+)
+
+func TestCommunicator_new(t *testing.T) {
+	r := &terraform.InstanceState{
+		Ephemeral: terraform.EphemeralState{
+			ConnInfo: map[string]string{
+				"type": "telnet",
+			},
+		},
+	}
+	if _, err := New(r); err == nil {
+		t.Fatalf("expected error with telnet")
+	}
+	r.Ephemeral.ConnInfo["type"] = "ssh"
+	if _, err := New(r); err != nil {
+		t.Fatalf("err: %v", err)
+	}
+}

communicator/remote/command.go

@@ -0,0 +1,67 @@
+package remote
+
+import (
+	"io"
+	"sync"
+)
+
+// Cmd represents a remote command being prepared or run.
+type Cmd struct {
+	// Command is the command to run remotely. This is executed as if
+	// it were a shell command, so you are expected to do any shell escaping
+	// necessary.
+	Command string
+
+	// Stdin specifies the process's standard input. If Stdin is
+	// nil, the process reads from an empty bytes.Buffer.
+	Stdin io.Reader
+
+	// Stdout and Stderr represent the process's standard output and
+	// error.
+	//
+	// If either is nil, it will be set to ioutil.Discard.
+	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
+
+	// Internal fields
+	exitCh chan struct{}
+
+	// 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()
+
+	if r.exitCh == nil {
+		r.exitCh = make(chan struct{})
+	}
+
+	r.Exited = true
+	r.ExitStatus = status
+	close(r.exitCh)
+}
+
+// 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
+}

communicator/remote/command_test.go

@@ -0,0 +1 @@
+package remote

communicator/ssh/communicator.go

@@ -11,84 +11,30 @@ import (
 	"net"
 	"os"
 	"path/filepath"
-	"sync"
 	"time"
 
+	"github.com/hashicorp/terraform/communicator/remote"
+	"github.com/hashicorp/terraform/terraform"
 	"golang.org/x/crypto/ssh"
 )
 
-// RemoteCmd represents a remote command being prepared or run.
+const (
-type RemoteCmd struct {
+	// DefaultShebang is added at the top of a SSH script file
-	// Command is the command to run remotely. This is executed as if
+	DefaultShebang = "#!/bin/sh\n"
-	// it were a shell command, so you are expected to do any shell escaping
+)
-	// necessary.
-	Command string
 
-	// Stdin specifies the process's standard input. If Stdin is
+type communicator struct {
-	// nil, the process reads from an empty bytes.Buffer.
+	connInfo *ConnectionInfo
-	Stdin io.Reader
+	config   *SSHConfig
-
-	// Stdout and Stderr represent the process's standard output and
-	// error.
-	//
-	// If either is nil, it will be set to ioutil.Discard.
-	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
-
-	// Internal fields
-	exitCh chan struct{}
-
-	// 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 *RemoteCmd) SetExited(status int) {
-	r.Lock()
-	defer r.Unlock()
-
-	if r.exitCh == nil {
-		r.exitCh = make(chan struct{})
-	}
-
-	r.Exited = true
-	r.ExitStatus = status
-	close(r.exitCh)
-}
-
-// Wait waits for the remote command to complete.
-func (r *RemoteCmd) Wait() {
-	// Make sure our condition variable is initialized.
-	r.Lock()
-	if r.exitCh == nil {
-		r.exitCh = make(chan struct{})
-	}
-	r.Unlock()
-
-	<-r.exitCh
-}
-
-type SSHCommunicator struct {
 	client   *ssh.Client
-	config  *Config
 	conn     net.Conn
 	address  string
 }
 
-// Config is the structure used to configure the SSH communicator.
+// SSHConfig is the structure used to configure the SSH communicator.
-type Config struct {
+type SSHConfig struct {
 	// The configuration of the Go SSH connection
-	SSHConfig *ssh.ClientConfig
+	Config *ssh.ClientConfig
 
 	// Connection returns a new connection. The current connection
 	// in use will be closed as part of the Close method, or in the
@@ -103,27 +49,105 @@ type Config struct {
 	SSHAgentConn net.Conn
 }
 
-// New creates a new packer.Communicator implementation over SSH. This takes
+// New creates a new communicator implementation over SSH. This takes
 // an already existing TCP connection and SSH configuration.
-func New(address string, config *Config) (result *SSHCommunicator, err error) {
+func New(s *terraform.InstanceState) (*communicator, error) {
-	// Establish an initial connection and connect
+	connInfo, err := ParseConnectionInfo(s)
-	result = &SSHCommunicator{
+	if err != nil {
-		config:  config,
+		return nil, err
-		address: address,
 	}
 
-	if err = result.reconnect(); err != nil {
+	comm := &communicator{connInfo: connInfo}
-		result = nil
-		return
-	}
 
-	return
+	return comm, nil
 }
 
-func (c *SSHCommunicator) Start(cmd *RemoteCmd) (err error) {
+// Connect implementation of communicator.Communicator interface
+func (c *communicator) Connect(o terraform.UIOutput) (err error) {
+	if c.conn != nil {
+		c.conn.Close()
+	}
+
+	// Set the conn and client to nil since we'll recreate it
+	c.conn = nil
+	c.client = nil
+
+	c.config, err = PrepareSSHConfig(c.connInfo)
+	if err != nil {
+		return err
+	}
+
+	if o != nil {
+		o.Output(fmt.Sprintf(
+			"Connecting to remote host via SSH...\n"+
+				"  Host: %s\n"+
+				"  User: %s\n"+
+				"  Password: %v\n"+
+				"  Private key: %v"+
+				"  SSH Agent: %v",
+			c.connInfo.Host, c.connInfo.User,
+			c.connInfo.Password != "",
+			c.connInfo.KeyFile != "",
+			c.connInfo.Agent,
+		))
+	}
+
+	log.Printf("connecting to TCP connection for SSH")
+	c.conn, err = c.config.Connection()
+	if err != nil {
+		// Explicitly set this to the REAL nil. Connection() can return
+		// a nil implementation of net.Conn which will make the
+		// "if c.conn == nil" check fail above. Read here for more information
+		// on this psychotic language feature:
+		//
+		// http://golang.org/doc/faq#nil_error
+		c.conn = nil
+
+		log.Printf("connection error: %s", err)
+		return err
+	}
+
+	log.Printf("handshaking with SSH")
+	host := fmt.Sprintf("%s:%d", c.connInfo.Host, c.connInfo.Port)
+	sshConn, sshChan, req, err := ssh.NewClientConn(c.conn, host, c.config.Config)
+	if err != nil {
+		log.Printf("handshake error: %s", err)
+		return err
+	}
+
+	c.client = ssh.NewClient(sshConn, sshChan, req)
+
+	if o != nil {
+		o.Output("Connected!")
+	}
+
+	return err
+}
+
+// Disconnect implementation of communicator.Communicator interface
+func (c *communicator) Disconnect() error {
+	if c.config.SSHAgentConn != nil {
+		return c.config.SSHAgentConn.Close()
+	}
+
+	return nil
+}
+
+// Timeout implementation of communicator.Communicator interface
+func (c *communicator) Timeout() time.Duration {
+	return c.connInfo.TimeoutVal
+}
+
+// Timeout implementation of communicator.Communicator interface
+func (c *communicator) ScriptPath() string {
+	return c.connInfo.ScriptPath
+}
+
+// Start implementation of communicator.Communicator interface
+func (c *communicator) Start(cmd *remote.Cmd) error {
 	session, err := c.newSession()
 	if err != nil {
-		return
+		return err
 	}
 
 	// Setup our session
@@ -139,15 +163,15 @@ func (c *SSHCommunicator) Start(cmd *RemoteCmd) (err error) {
 			ssh.TTY_OP_OSPEED: 14400, // output speed = 14.4kbaud
 		}
 
-		if err = session.RequestPty("xterm", 80, 40, termModes); err != nil {
+		if err := session.RequestPty("xterm", 80, 40, termModes); err != nil {
-			return
+			return err
 		}
 	}
 
 	log.Printf("starting remote command: %s", cmd.Command)
 	err = session.Start(cmd.Command + "\n")
 	if err != nil {
-		return
+		return err
 	}
 
 	// Start a goroutine to wait for the session to end and set the
@@ -168,10 +192,11 @@ func (c *SSHCommunicator) Start(cmd *RemoteCmd) (err error) {
 		cmd.SetExited(exitStatus)
 	}()
 
-	return
+	return nil
 }
 
-func (c *SSHCommunicator) Upload(path string, input io.Reader) error {
+// Upload implementation of communicator.Communicator interface
+func (c *communicator) Upload(path string, input io.Reader) error {
 	// The target directory and file for talking the SCP protocol
 	targetDir := filepath.Dir(path)
 	targetFile := filepath.Base(path)
@@ -188,7 +213,30 @@ func (c *SSHCommunicator) Upload(path string, input io.Reader) error {
 	return c.scpSession("scp -vt "+targetDir, scpFunc)
 }
 
-func (c *SSHCommunicator) UploadDir(dst string, src string, excl []string) error {
+// UploadScript implementation of communicator.Communicator interface
+func (c *communicator) UploadScript(path string, input io.Reader) error {
+	script := bytes.NewBufferString(DefaultShebang)
+	script.ReadFrom(input)
+
+	if err := c.Upload(path, script); err != nil {
+		return err
+	}
+
+	cmd := &remote.Cmd{
+		Command: fmt.Sprintf("chmod 0777 %s", c.connInfo.ScriptPath),
+	}
+	if err := c.Start(cmd); err != nil {
+		return fmt.Errorf(
+			"Error chmodding script file to 0777 in remote "+
+				"machine: %s", err)
+	}
+	cmd.Wait()
+
+	return nil
+}
+
+// UploadDir implementation of communicator.Communicator interface
+func (c *communicator) UploadDir(dst string, src string, excl []string) error {
 	log.Printf("Upload dir '%s' to '%s'", src, dst)
 	scpFunc := func(w io.Writer, r *bufio.Reader) error {
 		uploadEntries := func() error {
@@ -217,11 +265,12 @@ func (c *SSHCommunicator) UploadDir(dst string, src string, excl []string) error
 	return c.scpSession("scp -rvt "+dst, scpFunc)
 }
 
-func (c *SSHCommunicator) Download(string, io.Writer) error {
+// Download implementation of communicator.Communicator interface
+func (c *communicator) Download(string, io.Writer) error {
 	panic("not implemented yet")
 }
 
-func (c *SSHCommunicator) newSession() (session *ssh.Session, err error) {
+func (c *communicator) newSession() (session *ssh.Session, err error) {
 	log.Println("opening new ssh session")
 	if c.client == nil {
 		err = errors.New("client not available")
@@ -231,7 +280,7 @@ func (c *SSHCommunicator) newSession() (session *ssh.Session, err error) {
 
 	if err != nil {
 		log.Printf("ssh session open error: '%s', attempting reconnect", err)
-		if err := c.reconnect(); err != nil {
+		if err := c.Connect(nil); err != nil {
 			return nil, err
 		}
 
@@ -241,43 +290,7 @@ func (c *SSHCommunicator) newSession() (session *ssh.Session, err error) {
 	return session, nil
 }
 
-func (c *SSHCommunicator) reconnect() (err error) {
+func (c *communicator) scpSession(scpCommand string, f func(io.Writer, *bufio.Reader) error) error {
-	if c.conn != nil {
-		c.conn.Close()
-	}
-
-	// Set the conn and client to nil since we'll recreate it
-	c.conn = nil
-	c.client = nil
-
-	log.Printf("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
-		// a nil implementation of net.Conn which will make the
-		// "if c.conn == nil" check fail above. Read here for more information
-		// on this psychotic language feature:
-		//
-		// http://golang.org/doc/faq#nil_error
-		c.conn = nil
-
-		log.Printf("reconnection error: %s", err)
-		return
-	}
-
-	log.Printf("handshaking with SSH")
-	sshConn, sshChan, req, err := ssh.NewClientConn(c.conn, c.address, c.config.SSHConfig)
-	if err != nil {
-		log.Printf("handshake error: %s", err)
-	}
-	if sshConn != nil {
-		c.client = ssh.NewClient(sshConn, sshChan, req)
-	}
-
-	return
-}
-
-func (c *SSHCommunicator) scpSession(scpCommand string, f func(io.Writer, *bufio.Reader) error) error {
 	session, err := c.newSession()
 	if err != nil {
 		return err
@@ -382,7 +395,7 @@ func checkSCPStatus(r *bufio.Reader) error {
 func scpUploadFile(dst string, src io.Reader, w io.Writer, r *bufio.Reader) error {
 	// Create a temporary file where we can copy the contents of the src
 	// so that we can determine the length, since SCP is length-prefixed.
-	tf, err := ioutil.TempFile("", "packer-upload")
+	tf, err := ioutil.TempFile("", "terraform-upload")
 	if err != nil {
 		return fmt.Errorf("Error creating temporary file for upload: %s", err)
 	}

communicator/ssh/communicator_test.go

@@ -6,8 +6,11 @@ import (
 	"bytes"
 	"fmt"
 	"net"
+	"strings"
 	"testing"
 
+	"github.com/hashicorp/terraform/communicator/remote"
+	"github.com/hashicorp/terraform/terraform"
 	"golang.org/x/crypto/ssh"
 )
 
@@ -105,67 +108,62 @@ func newMockLineServer(t *testing.T) string {
 }
 
 func TestNew_Invalid(t *testing.T) {
-	clientConfig := &ssh.ClientConfig{
+	address := newMockLineServer(t)
-		User: "user",
+	parts := strings.Split(address, ":")
-		Auth: []ssh.AuthMethod{
+
-			ssh.Password("i-am-invalid"),
+	r := &terraform.InstanceState{
+		Ephemeral: terraform.EphemeralState{
+			ConnInfo: map[string]string{
+				"type":     "ssh",
+				"user":     "user",
+				"password": "i-am-invalid",
+				"host":     parts[0],
+				"port":     parts[1],
+				"timeout":  "30s",
+			},
 		},
 	}
 
-	address := newMockLineServer(t)
+	c, err := New(r)
-	conn := func() (net.Conn, error) {
-		conn, err := net.Dial("tcp", address)
 	if err != nil {
-			t.Errorf("Unable to accept incoming connection: %v", err)
+		t.Fatalf("error creating communicator: %s", err)
-		}
-		return conn, err
 	}
 
-	config := &Config{
+	err = c.Connect(nil)
-		Connection: conn,
-		SSHConfig:  clientConfig,
-	}
-
-	_, err := New(address, config)
 	if err == nil {
 		t.Fatal("should have had an error connecting")
 	}
 }
 
 func TestStart(t *testing.T) {
-	clientConfig := &ssh.ClientConfig{
+	address := newMockLineServer(t)
-		User: "user",
+	parts := strings.Split(address, ":")
-		Auth: []ssh.AuthMethod{
+
-			ssh.Password("pass"),
+	r := &terraform.InstanceState{
+		Ephemeral: terraform.EphemeralState{
+			ConnInfo: map[string]string{
+				"type":     "ssh",
+				"user":     "user",
+				"password": "pass",
+				"host":     parts[0],
+				"port":     parts[1],
+				"timeout":  "30s",
+			},
 		},
 	}
 
-	address := newMockLineServer(t)
+	c, err := New(r)
-	conn := func() (net.Conn, error) {
-		conn, err := net.Dial("tcp", address)
 	if err != nil {
-			t.Fatalf("unable to dial to remote side: %s", err)
+		t.Fatalf("error creating communicator: %s", err)
-		}
-		return conn, err
 	}
 
-	config := &Config{
+	var cmd remote.Cmd
-		Connection: conn,
-		SSHConfig:  clientConfig,
-	}
-
-	client, err := New(address, config)
-	if err != nil {
-		t.Fatalf("error connecting to SSH: %s", err)
-	}
-
-	var cmd RemoteCmd
 	stdout := new(bytes.Buffer)
 	cmd.Command = "echo foo"
 	cmd.Stdout = stdout
 
-	err = client.Start(&cmd)
+	err = c.Start(&cmd)
 	if err != nil {
-		t.Fatalf("error executing command: %s", err)
+		t.Fatalf("error executing remote command: %s", err)
 	}
 }

communicator/ssh/provisioner.go

@@ -5,11 +5,8 @@ import (
 	"fmt"
 	"io/ioutil"
 	"log"
-	"math/rand"
 	"net"
 	"os"
-	"strconv"
-	"strings"
 	"time"
 
 	"github.com/hashicorp/terraform/terraform"
@@ -34,10 +31,10 @@ const (
 	DefaultTimeout = 5 * time.Minute
 )
 
-// SSHConfig is decoded from the ConnInfo of the resource. These
+// ConnectionInfo is decoded from the ConnInfo of the resource. These are the
-// are the only keys we look at. If a KeyFile is given, that is used
+// only keys we look at. If a KeyFile is given, that is used instead
-// instead of a password.
+// of a password.
-type SSHConfig struct {
+type ConnectionInfo struct {
 	User       string
 	Password   string
 	KeyFile    string `mapstructure:"key_file"`
@@ -49,31 +46,13 @@ type SSHConfig struct {
 	TimeoutVal time.Duration `mapstructure:"-"`
 }
 
-func (c *SSHConfig) RemotePath() string {
+// ParseConnectionInfo is used to convert the ConnInfo of the InstanceState into
-	return strings.Replace(
+// a ConnectionInfo struct
-		c.ScriptPath, "%RAND%",
+func ParseConnectionInfo(s *terraform.InstanceState) (*ConnectionInfo, error) {
-		strconv.FormatInt(int64(rand.Int31()), 10), -1)
+	connInfo := &ConnectionInfo{}
-}
-
-// VerifySSH is used to verify the ConnInfo is usable by remote-exec
-func VerifySSH(s *terraform.InstanceState) error {
-	connType := s.Ephemeral.ConnInfo["type"]
-	switch connType {
-	case "":
-	case "ssh":
-	default:
-		return fmt.Errorf("Connection type '%s' not supported", connType)
-	}
-	return nil
-}
-
-// ParseSSHConfig is used to convert the ConnInfo of the InstanceState into
-// a SSHConfig struct
-func ParseSSHConfig(s *terraform.InstanceState) (*SSHConfig, error) {
-	sshConf := &SSHConfig{}
 	decConf := &mapstructure.DecoderConfig{
 		WeaklyTypedInput: true,
-		Result:           sshConf,
+		Result:           connInfo,
 	}
 	dec, err := mapstructure.NewDecoder(decConf)
 	if err != nil {
@@ -82,21 +61,21 @@ func ParseSSHConfig(s *terraform.InstanceState) (*SSHConfig, error) {
 	if err := dec.Decode(s.Ephemeral.ConnInfo); err != nil {
 		return nil, err
 	}
-	if sshConf.User == "" {
+	if connInfo.User == "" {
-		sshConf.User = DefaultUser
+		connInfo.User = DefaultUser
 	}
-	if sshConf.Port == 0 {
+	if connInfo.Port == 0 {
-		sshConf.Port = DefaultPort
+		connInfo.Port = DefaultPort
 	}
-	if sshConf.ScriptPath == "" {
+	if connInfo.ScriptPath == "" {
-		sshConf.ScriptPath = DefaultScriptPath
+		connInfo.ScriptPath = DefaultScriptPath
 	}
-	if sshConf.Timeout != "" {
+	if connInfo.Timeout != "" {
-		sshConf.TimeoutVal = safeDuration(sshConf.Timeout, DefaultTimeout)
+		connInfo.TimeoutVal = safeDuration(connInfo.Timeout, DefaultTimeout)
 	} else {
-		sshConf.TimeoutVal = DefaultTimeout
+		connInfo.TimeoutVal = DefaultTimeout
 	}
-	return sshConf, nil
+	return connInfo, nil
 }
 
 // safeDuration returns either the parsed duration or a default value
@@ -109,16 +88,16 @@ func safeDuration(dur string, defaultDur time.Duration) time.Duration {
 	return d
 }
 
-// PrepareConfig is used to turn the *SSHConfig provided into a
+// PrepareSSHConfig is used to turn the *ConnectionInfo provided into a
-// usable *Config for client initialization.
+// usable *SSHConfig for client initialization.
-func PrepareConfig(conf *SSHConfig) (*Config, error) {
+func PrepareSSHConfig(connInfo *ConnectionInfo) (*SSHConfig, error) {
 	var conn net.Conn
 	var err error
 
 	sshConf := &ssh.ClientConfig{
-		User: conf.User,
+		User: connInfo.User,
 	}
-	if conf.Agent {
+	if connInfo.Agent {
 		sshAuthSock := os.Getenv("SSH_AUTH_SOCK")
 
 		if sshAuthSock == "" {
@@ -138,14 +117,14 @@ func PrepareConfig(conf *SSHConfig) (*Config, error) {
 
 		sshConf.Auth = append(sshConf.Auth, ssh.PublicKeys(signers...))
 	}
-	if conf.KeyFile != "" {
+	if connInfo.KeyFile != "" {
-		fullPath, err := homedir.Expand(conf.KeyFile)
+		fullPath, err := homedir.Expand(connInfo.KeyFile)
 		if err != nil {
 			return nil, fmt.Errorf("Failed to expand home directory: %v", err)
 		}
 		key, err := ioutil.ReadFile(fullPath)
 		if err != nil {
-			return nil, fmt.Errorf("Failed to read key file '%s': %v", conf.KeyFile, err)
+			return nil, fmt.Errorf("Failed to read key file '%s': %v", connInfo.KeyFile, err)
 		}
 
 		// We parse the private key on our own first so that we can
@@ -153,40 +132,32 @@ func PrepareConfig(conf *SSHConfig) (*Config, error) {
 		block, _ := pem.Decode(key)
 		if block == nil {
 			return nil, fmt.Errorf(
-				"Failed to read key '%s': no key found", conf.KeyFile)
+				"Failed to read key '%s': no key found", connInfo.KeyFile)
 		}
 		if block.Headers["Proc-Type"] == "4,ENCRYPTED" {
 			return nil, fmt.Errorf(
 				"Failed to read key '%s': password protected keys are\n"+
-					"not supported. Please decrypt the key prior to use.", conf.KeyFile)
+					"not supported. Please decrypt the key prior to use.", connInfo.KeyFile)
 		}
 
 		signer, err := ssh.ParsePrivateKey(key)
 		if err != nil {
-			return nil, fmt.Errorf("Failed to parse key file '%s': %v", conf.KeyFile, err)
+			return nil, fmt.Errorf("Failed to parse key file '%s': %v", connInfo.KeyFile, err)
 		}
 
 		sshConf.Auth = append(sshConf.Auth, ssh.PublicKeys(signer))
 	}
-	if conf.Password != "" {
+	if connInfo.Password != "" {
 		sshConf.Auth = append(sshConf.Auth,
-			ssh.Password(conf.Password))
+			ssh.Password(connInfo.Password))
 		sshConf.Auth = append(sshConf.Auth,
-			ssh.KeyboardInteractive(PasswordKeyboardInteractive(conf.Password)))
+			ssh.KeyboardInteractive(PasswordKeyboardInteractive(connInfo.Password)))
 	}
-	host := fmt.Sprintf("%s:%d", conf.Host, conf.Port)
+	host := fmt.Sprintf("%s:%d", connInfo.Host, connInfo.Port)
-	config := &Config{
+	config := &SSHConfig{
-		SSHConfig:    sshConf,
+		Config:       sshConf,
 		Connection:   ConnectFunc("tcp", host),
 		SSHAgentConn: conn,
 	}
 	return config, nil
 }
-
-func (c *Config) CleanupConfig() error {
-	if c.SSHAgentConn != nil {
-		return c.SSHAgentConn.Close()
-	}
-
-	return nil
-}

communicator/ssh/provisioner_test.go

@@ -0,0 +1,50 @@
+package ssh
+
+import (
+	"testing"
+
+	"github.com/hashicorp/terraform/terraform"
+)
+
+func TestProvisioner_connInfo(t *testing.T) {
+	r := &terraform.InstanceState{
+		Ephemeral: terraform.EphemeralState{
+			ConnInfo: map[string]string{
+				"type":     "ssh",
+				"user":     "root",
+				"password": "supersecret",
+				"key_file": "/my/key/file.pem",
+				"host":     "127.0.0.1",
+				"port":     "22",
+				"timeout":  "30s",
+			},
+		},
+	}
+
+	conf, err := ParseConnectionInfo(r)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	if conf.User != "root" {
+		t.Fatalf("bad: %v", conf)
+	}
+	if conf.Password != "supersecret" {
+		t.Fatalf("bad: %v", conf)
+	}
+	if conf.KeyFile != "/my/key/file.pem" {
+		t.Fatalf("bad: %v", conf)
+	}
+	if conf.Host != "127.0.0.1" {
+		t.Fatalf("bad: %v", conf)
+	}
+	if conf.Port != 22 {
+		t.Fatalf("bad: %v", conf)
+	}
+	if conf.Timeout != "30s" {
+		t.Fatalf("bad: %v", conf)
+	}
+	if conf.ScriptPath != DefaultScriptPath {
+		t.Fatalf("bad: %v", conf)
+	}
+}

helper/ssh/provisioner_test.go

@@ -1,97 +0,0 @@
-package ssh
-
-import (
-	"regexp"
-	"testing"
-
-	"github.com/hashicorp/terraform/terraform"
-)
-
-func TestSSHConfig_RemotePath(t *testing.T) {
-	cases := []struct {
-		Input   string
-		Pattern string
-	}{
-		{
-			"/tmp/script.sh",
-			`^/tmp/script\.sh$`,
-		},
-		{
-			"/tmp/script_%RAND%.sh",
-			`^/tmp/script_(\d+)\.sh$`,
-		},
-	}
-
-	for _, tc := range cases {
-		config := &SSHConfig{ScriptPath: tc.Input}
-		output := config.RemotePath()
-
-		match, err := regexp.Match(tc.Pattern, []byte(output))
-		if err != nil {
-			t.Fatalf("bad: %s\n\nerr: %s", tc.Input, err)
-		}
-		if !match {
-			t.Fatalf("bad: %s\n\n%s", tc.Input, output)
-		}
-	}
-}
-
-func TestResourceProvider_verifySSH(t *testing.T) {
-	r := &terraform.InstanceState{
-		Ephemeral: terraform.EphemeralState{
-			ConnInfo: map[string]string{
-				"type": "telnet",
-			},
-		},
-	}
-	if err := VerifySSH(r); err == nil {
-		t.Fatalf("expected error with telnet")
-	}
-	r.Ephemeral.ConnInfo["type"] = "ssh"
-	if err := VerifySSH(r); err != nil {
-		t.Fatalf("err: %v", err)
-	}
-}
-
-func TestResourceProvider_sshConfig(t *testing.T) {
-	r := &terraform.InstanceState{
-		Ephemeral: terraform.EphemeralState{
-			ConnInfo: map[string]string{
-				"type":     "ssh",
-				"user":     "root",
-				"password": "supersecret",
-				"key_file": "/my/key/file.pem",
-				"host":     "127.0.0.1",
-				"port":     "22",
-				"timeout":  "30s",
-			},
-		},
-	}
-
-	conf, err := ParseSSHConfig(r)
-	if err != nil {
-		t.Fatalf("err: %v", err)
-	}
-
-	if conf.User != "root" {
-		t.Fatalf("bad: %v", conf)
-	}
-	if conf.Password != "supersecret" {
-		t.Fatalf("bad: %v", conf)
-	}
-	if conf.KeyFile != "/my/key/file.pem" {
-		t.Fatalf("bad: %v", conf)
-	}
-	if conf.Host != "127.0.0.1" {
-		t.Fatalf("bad: %v", conf)
-	}
-	if conf.Port != 22 {
-		t.Fatalf("bad: %v", conf)
-	}
-	if conf.Timeout != "30s" {
-		t.Fatalf("bad: %v", conf)
-	}
-	if conf.ScriptPath != DefaultScriptPath {
-		t.Fatalf("bad: %v", conf)
-	}
-}

plugin/client_test.go

@@ -99,7 +99,7 @@ func TestClient_Stderr(t *testing.T) {
 
 func TestClient_Stdin(t *testing.T) {
 	// Overwrite stdin for this test with a temporary file
-	tf, err := ioutil.TempFile("", "packer")
+	tf, err := ioutil.TempFile("", "terraform")
 	if err != nil {
 		t.Fatalf("err: %s", err)
 	}