Merge lp:~axwalk/juju-core/ssh-disable-proxy into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

cmd/juju: revert proxy changes in CLI

I'm reverting the changes to the CLI only,
as juju ssh support for the local provider
regressed (broken if machine 0 doesn't have
an SSH server available.)

We'll keep the API and utils/ssh changes
needed to re-enable this after 1.18 is
released.

https://codereview.appspot.com/78070043/

cmd/juju: revert proxy changes in CLI

I'm reverting the changes to the CLI only,
as juju ssh support for the local provider
regressed (broken if machine 0 doesn't have
an SSH server available.)

We'll keep the API and utils/ssh changes
needed to re-enable this after 1.18 is
released.

https://codereview.appspot.com/78070043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+211853_code.launchpad.net,

Message:
Please take a look.

Description:
cmd/juju: revert proxy changes in CLI

I'm reverting the changes to the CLI only,
as juju ssh support for the local provider
regressed (broken if machine 0 doesn't have
an SSH server available.)

We'll keep the API and utils/ssh changes
needed to re-enable this after 1.18 is
released.

https://code.launchpad.net/~axwalk/juju-core/ssh-disable-proxy/+merge/211853

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/78070043/

Affected files (+74, -149 lines):
   A [revision details]
   M cmd/juju/scp.go
   M cmd/juju/scp_test.go
   M cmd/juju/ssh.go
   M cmd/juju/ssh_test.go

[Tim Penhey (thumper)]

On 2014/03/20 02:35:16, axw wrote:
> Please take a look.

LGTM

https://codereview.appspot.com/78070043/

=== modified file 'cmd/juju/scp.go'
--- cmd/juju/scp.go	2014-03-19 03:18:41 +0000
+++ cmd/juju/scp.go	2014-03-20 02:33:00 +0000
@@ -106,13 +106,5 @@
 			targets = append(targets, arg)
 		}
 	}
-
-	var options *ssh.Options
-	if c.proxy {
-		options = new(ssh.Options)
-		if err := c.setProxyCommand(options); err != nil {
-			return err
-		}
-	}
-	return ssh.Copy(targets, extraArgs, options)
+	return ssh.Copy(targets, extraArgs, nil)
 }

=== modified file 'cmd/juju/scp_test.go'
--- cmd/juju/scp_test.go	2014-03-19 03:18:41 +0000
+++ cmd/juju/scp_test.go	2014-03-20 02:33:00 +0000
@@ -27,64 +27,67 @@
 	about  string
 	args   []string
 	result string
-	proxy  bool
 	error  string
 }{
 	{
-		about:  "scp from machine 0 to current dir",
-		args:   []string{"0:foo", "."},
-		result: commonArgsNoProxy + "[e-mail]:foo .\n",
-	},
-	{
-		about:  "scp from machine 0 to current dir with extra args",
-		args:   []string{"0:foo", ".", "-rv -o SomeOption"},
-		result: commonArgsNoProxy + "-rv -o SomeOption [e-mail]:foo .\n",
-	},
-	{
-		about:  "scp from current dir to machine 0",
-		args:   []string{"foo", "0:"},
-		result: commonArgsNoProxy + "foo [e-mail]:\n",
-	},
-	{
-		about:  "scp from current dir to machine 0 with extra args",
-		args:   []string{"foo", "0:", "-r -v"},
-		result: commonArgsNoProxy + "-r -v foo [e-mail]:\n",
-	},
-	{
-		about:  "scp from machine 0 to unit mysql/0",
-		args:   []string{"0:foo", "mysql/0:/foo"},
-		result: commonArgsNoProxy + "[e-mail]:foo [e-mail]:/foo\n",
-	},
-	{
-		about:  "scp from machine 0 to unit mysql/0 and extra args",
-		args:   []string{"0:foo", "mysql/0:/foo", "-q"},
-		result: commonArgsNoProxy + "-q [e-mail]:foo [e-mail]:/foo\n",
-	},
-	{
-		about: "scp from machine 0 to unit mysql/0 and extra args before",
-		args:  []string{"-q", "-r", "0:foo", "mysql/0:/foo"},
-		error: `unexpected argument "-q"; extra arguments must be last`,
-	},
-	{
-		about:  "scp two local files to unit mysql/0",
-		args:   []string{"file1", "file2", "mysql/0:/foo/"},
-		result: commonArgsNoProxy + "file1 file2 [e-mail]:/foo/\n",
-	},
-	{
-		about:  "scp from unit mongodb/1 to unit mongodb/0 and multiple extra args",
-		args:   []string{"mongodb/1:foo", "mongodb/0:", "-r -v -q -l5"},
-		result: commonArgsNoProxy + "-r -v -q -l5 [e-mail]:foo [e-mail]:\n",
-	},
-	{
-		about:  "scp works with IPv6 addresses",
-		args:   []string{"ipv6-svc/0:foo", "bar"},
-		result: commonArgsNoProxy + `ubuntu@\[2001:db8::\]:foo bar` + "\n",
-	},
-	{
-		about:  "scp from machine 0 to unit mysql/0 with proxy",
-		args:   []string{"0:foo", "mysql/0:/foo"},
-		result: commonArgs + "[e-mail]:foo [e-mail]:/foo\n",
-		proxy:  true,
+		"scp from machine 0 to current dir",
+		[]string{"0:foo", "."},
+		commonArgs + "[e-mail]:foo .\n",
+		"",
+	},
+	{
+		"scp from machine 0 to current dir with extra args",
+		[]string{"0:foo", ".", "-rv -o SomeOption"},
+		commonArgs + "-rv -o SomeOption [e-mail]:foo .\n",
+		"",
+	},
+	{
+		"scp from current dir to machine 0",
+		[]string{"foo", "0:"},
+		commonArgs + "foo [e-mail]:\n",
+		"",
+	},
+	{
+		"scp from current dir to machine 0 with extra args",
+		[]string{"foo", "0:", "-r -v"},
+		commonArgs + "-r -v foo [e-mail]:\n",
+		"",
+	},
+	{
+		"scp from machine 0 to unit mysql/0",
+		[]string{"0:foo", "mysql/0:/foo"},
+		commonArgs + "[e-mail]:foo [e-mail]:/foo\n",
+		"",
+	},
+	{
+		"scp from machine 0 to unit mysql/0 and extra args",
+		[]string{"0:foo", "mysql/0:/foo", "-q"},
+		commonArgs + "-q [e-mail]:foo [e-mail]:/foo\n",
+		"",
+	},
+	{
+		"scp from machine 0 to unit mysql/0 and extra args before",
+		[]string{"-q", "-r", "0:foo", "mysql/0:/foo"},
+		"",
+		`unexpected argument "-q"; extra arguments must be last`,
+	},
+	{
+		"scp two local files to unit mysql/0",
+		[]string{"file1", "file2", "mysql/0:/foo/"},
+		commonArgs + "file1 file2 [e-mail]:/foo/\n",
+		"",
+	},
+	{
+		"scp from unit mongodb/1 to unit mongodb/0 and multiple extra args",
+		[]string{"mongodb/1:foo", "mongodb/0:", "-r -v -q -l5"},
+		commonArgs + "-r -v -q -l5 [e-mail]:foo [e-mail]:\n",
+		"",
+	},
+	{
+		"scp works with IPv6 addresses",
+		[]string{"ipv6-svc/0:foo", "bar"},
+		commonArgs + `ubuntu@\[2001:db8::\]:foo bar` + "\n",
+		"",
 	},
 }
 
@@ -119,7 +122,6 @@
 		c.Logf("test %d: %s -> %s\n", i, t.about, t.args)
 		ctx := coretesting.Context(c)
 		scpcmd := &SCPCommand{}
-		scpcmd.proxy = t.proxy
 
 		err := scpcmd.Init(t.args)
 		c.Check(err, gc.IsNil)

=== modified file 'cmd/juju/ssh.go'
--- cmd/juju/ssh.go	2014-03-19 03:18:41 +0000
+++ cmd/juju/ssh.go	2014-03-20 02:33:00 +0000
@@ -6,13 +6,8 @@
 import (
 	"errors"
 	"fmt"
-	"net"
-	"os"
-	"os/exec"
 	"time"
 
-	"launchpad.net/gnuflag"
-
 	"launchpad.net/juju-core/cmd"
 	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/juju"
@@ -31,34 +26,13 @@
 // SSHCommon provides common methods for SSHCommand, SCPCommand and DebugHooksCommand.
 type SSHCommon struct {
 	cmd.EnvCommandBase
-	proxy     bool
 	Target    string
 	Args      []string
 	apiClient *api.Client
-	apiAddr   string
 	// Only used for compatibility with 1.16
 	rawConn *juju.Conn
 }
 
-func (c *SSHCommon) SetFlags(f *gnuflag.FlagSet) {
-	c.EnvCommandBase.SetFlags(f)
-	f.BoolVar(&c.proxy, "proxy", true, "proxy through the API server")
-}
-
-// setProxyCommand sets the proxy command option.
-func (c *SSHCommon) setProxyCommand(options *ssh.Options) error {
-	apiServerHost, _, err := net.SplitHostPort(c.apiAddr)
-	if err != nil {
-		return fmt.Errorf("failed to get proxy address: %v", err)
-	}
-	juju, err := getJujuExecutable()
-	if err != nil {
-		return fmt.Errorf("failed to get juju executable path: %v", err)
-	}
-	options.SetProxyCommand(juju, "ssh", "--proxy=false", apiServerHost, "-T", "nc -q0 %h %p")
-	return nil
-}
-
 const sshDoc = `
 Launch an ssh shell on the machine identified by the <target> parameter.
 <target> can be either a machine id  as listed by "juju status" in the
@@ -101,12 +75,6 @@
 	return nil
 }
 
-// getJujuExecutable returns the path to the juju
-// executable, or an error if it could not be found.
-var getJujuExecutable = func() (string, error) {
-	return exec.LookPath(os.Args[0])
-}
-
 // Run resolves c.Target to a machine, to the address of a i
 // machine or unit forks ssh passing any arguments provided.
 func (c *SSHCommand) Run(ctx *cmd.Context) error {
@@ -124,11 +92,6 @@
 	}
 	var options ssh.Options
 	options.EnablePTY()
-	if c.proxy {
-		if err := c.setProxyCommand(&options); err != nil {
-			return err
-		}
-	}
 	cmd := ssh.Command("ubuntu@"+host, c.Args, &options)
 	cmd.Stdin = ctx.Stdin
 	cmd.Stdout = ctx.Stdout
@@ -139,13 +102,9 @@
 // initAPIClient initialises the API connection.
 // It is the caller's responsibility to close the connection.
 func (c *SSHCommon) initAPIClient() (*api.Client, error) {
-	st, err := juju.NewAPIFromName(c.EnvName)
-	if err != nil {
-		return nil, err
-	}
-	c.apiClient = st.Client()
-	c.apiAddr = st.Addr()
-	return c.apiClient, nil
+	var err error
+	c.apiClient, err = juju.NewAPIClientFromName(c.EnvName)
+	return c.apiClient, err
 }
 
 // attemptStarter is an interface corresponding to utils.AttemptStrategy
@@ -199,15 +158,9 @@
 		if err != nil {
 			return "", err
 		}
-		var addr string
-		if c.proxy {
-			if addr = instance.SelectInternalAddress(machine.Addresses(), false); addr == "" {
-				return "", fmt.Errorf("machine %q has no internal address", machine)
-			}
-		} else {
-			if addr = instance.SelectPublicAddress(machine.Addresses()); addr == "" {
-				return "", fmt.Errorf("machine %q has no public address", machine)
-			}
+		addr := instance.SelectPublicAddress(machine.Addresses())
+		if addr == "" {
+			return "", fmt.Errorf("machine %q has no public address", machine)
 		}
 		return addr, nil
 	}
@@ -218,16 +171,9 @@
 		if err != nil {
 			return "", err
 		}
-		var addr string
-		var ok bool
-		if c.proxy {
-			if addr, ok = unit.PrivateAddress(); !ok {
-				return "", fmt.Errorf("unit %q has no internal address", unit)
-			}
-		} else {
-			if addr, ok = unit.PublicAddress(); !ok {
-				return "", fmt.Errorf("unit %q has no public address", unit)
-			}
+		addr, ok := unit.PublicAddress()
+		if !ok {
+			return "", fmt.Errorf("unit %q has no public address", unit)
 		}
 		return addr, nil
 	}
@@ -235,11 +181,6 @@
 }
 
 func (c *SSHCommon) hostFromTarget(target string) (string, error) {
-	// If the target is neither a machine nor a unit,
-	// assume it's a hostname and try it directly.
-	if !names.IsMachine(target) && !names.IsUnit(target) {
-		return target, nil
-	}
 	var addr string
 	var err error
 	var useStateConn bool
@@ -248,13 +189,9 @@
 	// a loop.
 	for a := sshHostFromTargetAttemptStrategy.Start(); a.Next(); {
 		if !useStateConn {
-			if c.proxy {
-				addr, err = c.apiClient.PrivateAddress(target)
-			} else {
-				addr, err = c.apiClient.PublicAddress(target)
-			}
+			addr, err = c.apiClient.PublicAddress(target)
 			if params.IsCodeNotImplemented(err) {
-				logger.Infof("API server does not support Client.PrivateAddress falling back to 1.16 compatibility mode (direct DB access)")
+				logger.Infof("API server does not support Client.PublicAddress falling back to 1.16 compatibility mode (direct DB access)")
 				useStateConn = true
 			}
 		}

=== modified file 'cmd/juju/ssh_test.go'
--- cmd/juju/ssh_test.go	2014-03-19 03:18:41 +0000
+++ cmd/juju/ssh_test.go	2014-03-20 02:33:00 +0000
@@ -39,7 +39,6 @@
 
 func (s *SSHCommonSuite) SetUpTest(c *gc.C) {
 	s.JujuConnSuite.SetUpTest(c)
-	s.PatchValue(&getJujuExecutable, func() (string, error) { return "juju", nil })
 
 	s.bin = c.MkDir()
 	s.PatchEnvPathPrepend(s.bin)
@@ -54,10 +53,8 @@
 }
 
 const (
-	commonArgsNoProxy = `-o StrictHostKeyChecking no -o PasswordAuthentication no `
-	commonArgs        = `-o StrictHostKeyChecking no -o ProxyCommand juju ssh --proxy=false 127.0.0.1 -T "nc -q0 %h %p" -o PasswordAuthentication no `
-	sshArgs           = commonArgs + `-t -t `
-	sshArgsNoProxy    = commonArgsNoProxy + `-t -t `
+	commonArgs = `-o StrictHostKeyChecking no -o PasswordAuthentication no `
+	sshArgs    = commonArgs + `-t -t `
 )
 
 var sshTests = []struct {
@@ -85,11 +82,6 @@
 		[]string{"ssh", "mongodb/1", "ls", "/"},
 		sshArgs + "[e-mail] ls /\n",
 	},
-	{
-		"connect to unit mysql/0 without proxy",
-		[]string{"ssh", "--proxy=false", "mysql/0"},
-		sshArgsNoProxy + "[e-mail]\n",
-	},
 }
 
 func (s *SSHSuite) TestSSHCommand(c *gc.C) {