Merge lp:~jameinel/juju-core/1.18-scp-multiple-args-1306208 into lp:juju-core/1.18

[John A Meinel]

This changes how "juju scp" parses its arguments, to restore the ability to put flags in more places on the command line.

This does the 'expand' logic differently. If something looks like it starts with an identifier: then it tries to expand it (like it did previously). But it allows that to happen anywhere in the args list, rather than all but the last arg.

The one thing we don't support (but we didn't support before, either), is "juju scp 0:foo myhost:bar", because we always lookup the stuff before the : as a PublicAddress lookup.

I'm not sure that this is 100% perfect, but I do think it is a step in the right direction.

I'm targetting 1.18.1 because I think the behavior of 1.18 is a regression from 1.16 since old command lines no longer work.

This changes how "juju scp" parses its arguments, to restore the ability to put flags in more places on the command line.

This does the 'expand' logic differently. If something looks like it starts with an identifier: then it tries to expand it (like it did previously). But it allows that to happen anywhere in the args list, rather than all but the last arg.

The one thing we don't support (but we didn't support before, either), is "juju scp 0:foo myhost:bar", because we always lookup the stuff before the : as a PublicAddress lookup.

I'm not sure that this is 100% perfect, but I do think it is a step in the right direction.

I'm targetting 1.18.1 because I think the behavior of 1.18 is a regression from 1.16 since old command lines no longer work.

[Go Bot (go-bot)]

The attempt to merge lp:~jameinel/juju-core/1.18-scp-multiple-args-1306208 into lp:juju-core/1.18 failed. Below is the output from the failed tests.

# launchpad.net/juju-core/cmd/plugins/juju-restore
cmd/plugins/juju-restore/restore.go:437: too many arguments in call to "launchpad.net/juju-core/utils/ssh".Copy
mongod: no process found

[Ian Booth (wallyworld)]

review: Approve

[Go Bot (go-bot)]

The attempt to merge lp:~jameinel/juju-core/1.18-scp-multiple-args-1306208 into lp:juju-core/1.18 failed. Below is the output from the failed tests.

# launchpad.net/juju-core/cmd/plugins/juju-restore
cmd/plugins/juju-restore/restore.go:437: too many arguments in call to "launchpad.net/juju-core/utils/ssh".Copy
mongod: no process found

=== modified file 'cmd/juju/scp.go'
--- cmd/juju/scp.go	2014-03-20 02:30:15 +0000
+++ cmd/juju/scp.go	2014-04-10 20:59:22 +0000
@@ -35,12 +35,12 @@
 Copy 2 files from two units to the local backup/ directory, passing -v
 to scp as an extra argument:
 
-    juju scp ubuntu/0:/path/file1 ubuntu/1:/path/file2 backup/ -v
+    juju scp -v ubuntu/0:/path/file1 ubuntu/1:/path/file2 backup/
 
 Recursively copy the directory /var/log/mongodb/ on the first mongodb
 server to the local directory remote-logs:
 
-    juju scp mongodb/0:/var/log/mongodb/ remote-logs/ -r
+    juju scp -r mongodb/0:/var/log/mongodb/ remote-logs/
 
 Copy a local file to the second apache unit of the environment "testing":
 
@@ -64,6 +64,35 @@
 	return nil
 }
 
+// expandArgs takes a list of arguments and looks for ones in the form of
+// 0:some/path or service/0:some/path, and translates them into
+// ubuntu@machine:some/path so they can be passed as arguments to scp, and pass
+// the rest verbatim on to scp
+func expandArgs(args []string, hostFromTarget func(string) (string, error)) ([]string, error) {
+	outArgs := make([]string, len(args))
+	for i, arg := range args {
+		v := strings.SplitN(arg, ":", 2)
+		if strings.HasPrefix(arg, "-") || len(v) <= 1 {
+			// Can't be an interesting target, so just pass it along
+			outArgs[i] = arg
+			continue
+		}
+		host, err := hostFromTarget(v[0])
+		if err != nil {
+			return nil, err
+		}
+		// To ensure this works with IPv6 addresses, we need to
+		// wrap the host with \[..\], so the colons inside will be
+		// interpreted as part of the address and the last one as
+		// separator between host and remote path.
+		if strings.Contains(host, ":") {
+			host = fmt.Sprintf(`\[%s\]`, host)
+		}
+		outArgs[i] = "ubuntu@" + host + ":" + v[1]
+	}
+	return outArgs, nil
+}
+
 // Run resolves c.Target to a machine, or host of a unit and
 // forks ssh with c.Args, if provided.
 func (c *SCPCommand) Run(ctx *cmd.Context) error {
@@ -73,38 +102,9 @@
 		return err
 	}
 	defer c.apiClient.Close()
-
-	// Parse all arguments, translating those in the form 0:/somepath
-	// or service/0:/somepath into ubuntu@machine:/somepath so they
-	// can be given to scp as targets (source(s) and destination(s)),
-	// and passing any others that look like extra arguments (starting
-	// with "-") verbatim to scp.
-	var targets, extraArgs []string
-	for i, arg := range c.Args {
-		if v := strings.SplitN(arg, ":", 2); len(v) > 1 {
-			host, err := c.hostFromTarget(v[0])
-			if err != nil {
-				return err
-			}
-			// To ensure this works with IPv6 addresses, we need to
-			// wrap the host with \[..\], so the colons inside will be
-			// interpreted as part of the address and the last one as
-			// separator between host and remote path.
-			if strings.Contains(host, ":") {
-				host = fmt.Sprintf(`\[%s\]`, host)
-			}
-			targets = append(targets, "ubuntu@"+host+":"+v[1])
-			continue
-		}
-		if strings.HasPrefix(arg, "-") {
-			if i != len(c.Args)-1 {
-				return fmt.Errorf("unexpected argument %q; extra arguments must be last", arg)
-			}
-			extraArgs = append(extraArgs, arg)
-		} else {
-			// Local path
-			targets = append(targets, arg)
-		}
+	args, err := expandArgs(c.Args, c.hostFromTarget)
+	if err != nil {
+		return err
 	}
-	return ssh.Copy(targets, extraArgs, nil)
+	return ssh.Copy(args, nil)
 }

=== modified file 'cmd/juju/scp_test.go'
--- cmd/juju/scp_test.go	2014-03-20 02:30:15 +0000
+++ cmd/juju/scp_test.go	2014-04-10 20:59:22 +0000
@@ -9,7 +9,9 @@
 	"io/ioutil"
 	"net/url"
 	"path/filepath"
+	"strings"
 
+	jc "github.com/juju/testing/checkers"
 	gc "launchpad.net/gocheck"
 
 	"launchpad.net/juju-core/charm"
@@ -18,11 +20,14 @@
 )
 
 var _ = gc.Suite(&SCPSuite{})
+var _ = gc.Suite(&expandArgsSuite{})
 
 type SCPSuite struct {
 	SSHCommonSuite
 }
 
+type expandArgsSuite struct{}
+
 var scpTests = []struct {
 	about  string
 	args   []string
@@ -34,60 +39,61 @@
 		[]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",
+		[]string{"0:foo", ".", "-rv", "-o", "SomeOption"},
+		commonArgs + "[e-mail]:foo . -rv -o SomeOption\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",
+		[]string{"foo", "0:", "-r", "-v"},
+		commonArgs + "foo [e-mail]: -r -v\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",
+		commonArgs + "[e-mail]:foo [e-mail]:/foo -q\n",
 		"",
-	},
-	{
+	}, {
 		"scp from machine 0 to unit mysql/0 and extra args before",
 		[]string{"-q", "-r", "0:foo", "mysql/0:/foo"},
+		commonArgs + "-q -r [e-mail]:foo [e-mail]:/foo\n",
 		"",
-		`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",
-		"",
-	},
-	{
+		[]string{"mongodb/1:foo", "mongodb/0:", "-r", "-v", "-q", "-l5"},
+		commonArgs + "[e-mail]:foo [e-mail]: -r -v -q -l5\n",
+		"",
+	}, {
+		"scp from unit mongodb/1 to unit mongodb/0 with a --",
+		[]string{"--", "-r", "-v", "mongodb/1:foo", "mongodb/0:", "-q", "-l5"},
+		commonArgs + "-- -r -v [e-mail]:foo [e-mail]: -q -l5\n",
+		"",
+	}, {
 		"scp works with IPv6 addresses",
 		[]string{"ipv6-svc/0:foo", "bar"},
 		commonArgs + `ubuntu@\[2001:db8::\]:foo bar` + "\n",
 		"",
+	}, {
+		"scp with no such machine",
+		[]string{"5:foo", "bar"},
+		"",
+		"machine 5 not found",
 	},
 }
 
@@ -140,3 +146,70 @@
 		}
 	}
 }
+
+var hostsFromTargets = map[string]string{
+	"0":          "dummyenv-0.dns",
+	"mysql/0":    "dummyenv-0.dns",
+	"mongodb/0":  "dummyenv-1.dns",
+	"mongodb/1":  "dummyenv-2.dns",
+	"ipv6-svc/0": "2001:db8::",
+}
+
+func dummyHostsFromTarget(target string) (string, error) {
+	if res, ok := hostsFromTargets[target]; ok {
+		return res, nil
+	}
+	return target, nil
+}
+
+func (s *expandArgsSuite) TestSCPExpandArgs(c *gc.C) {
+	for i, t := range scpTests {
+		if t.error != "" {
+			// We are just running a focused set of tests on
+			// expandArgs, we aren't implementing the full
+			// hostsFromTargets to actually trigger errors
+			continue
+		}
+		c.Logf("test %d: %s -> %s\n", i, t.about, t.args)
+		// expandArgs doesn't add the commonArgs prefix, so strip it
+		// off, along with the trailing '\n'
+		c.Check(strings.HasPrefix(t.result, commonArgs), jc.IsTrue)
+		argString := t.result[len(commonArgs):]
+		c.Check(strings.HasSuffix(argString, "\n"), jc.IsTrue)
+		argString = argString[:len(argString)-1]
+		args := strings.Split(argString, " ")
+		expanded, err := expandArgs(t.args, dummyHostsFromTarget)
+		c.Check(err, gc.IsNil)
+		c.Check(expanded, gc.DeepEquals, args)
+	}
+}
+
+var expandTests = []struct {
+	about  string
+	args   []string
+	result []string
+}{
+	{
+		"don't expand params that start with '-'",
+		[]string{"-0:stuff", "0:foo", "."},
+		[]string{"-0:stuff", "[e-mail]:foo", "."},
+	},
+}
+
+func (s *expandArgsSuite) TestExpandArgs(c *gc.C) {
+	for i, t := range expandTests {
+		c.Logf("test %d: %s -> %s\n", i, t.about, t.args)
+		expanded, err := expandArgs(t.args, dummyHostsFromTarget)
+		c.Check(err, gc.IsNil)
+		c.Check(expanded, gc.DeepEquals, t.result)
+	}
+}
+
+func (s *expandArgsSuite) TestExpandArgsPropagatesErrors(c *gc.C) {
+	erroringHostFromTargets := func(string) (string, error) {
+		return "", fmt.Errorf("this is my error")
+	}
+	expanded, err := expandArgs([]string{"foo:1", "bar"}, erroringHostFromTargets)
+	c.Assert(err, gc.ErrorMatches, "this is my error")
+	c.Check(expanded, gc.IsNil)
+}

=== modified file 'utils/ssh/ssh.go'
--- utils/ssh/ssh.go	2014-03-19 03:18:41 +0000
+++ utils/ssh/ssh.go	2014-04-10 20:59:22 +0000
@@ -81,7 +81,7 @@
 	// Paths are specified in the scp format, [[user@]host:]path. If
 	// any extra arguments are specified in extraArgs, they are passed
 	// verbatim.
-	Copy(targets, extraArgs []string, options *Options) error
+	Copy(args []string, options *Options) error
 }
 
 // Cmd represents a command to be (or being) executed
@@ -236,7 +236,7 @@
 }
 
 // Copy is a short-cut for DefaultClient.Copy.
-func Copy(targets, extraArgs []string, options *Options) error {
+func Copy(args []string, options *Options) error {
 	logger.Debugf("using %s ssh client", chosenClient)
-	return DefaultClient.Copy(targets, extraArgs, options)
+	return DefaultClient.Copy(args, options)
 }

=== modified file 'utils/ssh/ssh_gocrypto.go'
--- utils/ssh/ssh_gocrypto.go	2014-02-24 16:21:43 +0000
+++ utils/ssh/ssh_gocrypto.go	2014-04-10 20:59:22 +0000
@@ -62,7 +62,7 @@
 // Copy implements Client.Copy.
 //
 // Copy is currently unimplemented, and will always return an error.
-func (c *GoCryptoClient) Copy(targets, extraArgs []string, options *Options) error {
+func (c *GoCryptoClient) Copy(args []string, options *Options) error {
 	return fmt.Errorf("scp command is not implemented (OpenSSH scp not available in PATH)")
 }
 

=== modified file 'utils/ssh/ssh_gocrypto_test.go'
--- utils/ssh/ssh_gocrypto_test.go	2014-03-13 07:54:56 +0000
+++ utils/ssh/ssh_gocrypto_test.go	2014-04-10 20:59:22 +0000
@@ -145,6 +145,6 @@
 func (s *SSHGoCryptoCommandSuite) TestCopy(c *gc.C) {
 	client, err := ssh.NewGoCryptoClient()
 	c.Assert(err, gc.IsNil)
-	err = client.Copy([]string{"0.1.2.3:b", c.MkDir()}, nil, nil)
+	err = client.Copy([]string{"0.1.2.3:b", c.MkDir()}, nil)
 	c.Assert(err, gc.ErrorMatches, `scp command is not implemented \(OpenSSH scp not available in PATH\)`)
 }

=== modified file 'utils/ssh/ssh_openssh.go'
--- utils/ssh/ssh_openssh.go	2014-03-27 02:42:08 +0000
+++ utils/ssh/ssh_openssh.go	2014-04-10 20:59:22 +0000
@@ -124,17 +124,16 @@
 }
 
 // Copy implements Client.Copy.
-func (c *OpenSSHClient) Copy(targets, extraArgs []string, userOptions *Options) error {
+func (c *OpenSSHClient) Copy(args []string, userOptions *Options) error {
 	var options Options
 	if userOptions != nil {
 		options = *userOptions
 		options.allocatePTY = false // doesn't make sense for scp
 	}
-	args := opensshOptions(&options, scpKind)
-	args = append(args, extraArgs...)
-	args = append(args, targets...)
-	bin, args := sshpassWrap("scp", args)
-	cmd := exec.Command(bin, args...)
+	allArgs := opensshOptions(&options, scpKind)
+	allArgs = append(allArgs, args...)
+	bin, allArgs := sshpassWrap("scp", allArgs)
+	cmd := exec.Command(bin, allArgs...)
 	var stderr bytes.Buffer
 	cmd.Stderr = &stderr
 	logger.Debugf("running: %s %s", bin, utils.CommandString(args...))

=== modified file 'utils/ssh/ssh_test.go'
--- utils/ssh/ssh_test.go	2014-02-22 13:45:10 +0000
+++ utils/ssh/ssh_test.go	2014-04-10 20:59:22 +0000
@@ -129,7 +129,7 @@
 	opts.AllowPasswordAuthentication()
 	opts.SetIdentities("x", "y")
 	opts.SetPort(2022)
-	err := s.client.Copy([]string{"/tmp/blah", "[e-mail]:baz"}, nil, &opts)
+	err := s.client.Copy([]string{"/tmp/blah", "[e-mail]:baz"}, &opts)
 	c.Assert(err, gc.IsNil)
 	out, err := ioutil.ReadFile(s.fakescp + ".args")
 	c.Assert(err, gc.IsNil)
@@ -137,11 +137,18 @@
 	c.Assert(string(out), gc.Equals, s.fakescp+" -o StrictHostKeyChecking no -i x -i y -P 2022 /tmp/blah [e-mail]:baz\n")
 
 	// Try passing extra args
-	err = s.client.Copy([]string{"/tmp/blah", "[e-mail]:baz"}, []string{"-r", "-v"}, &opts)
-	c.Assert(err, gc.IsNil)
-	out, err = ioutil.ReadFile(s.fakescp + ".args")
-	c.Assert(err, gc.IsNil)
-	c.Assert(string(out), gc.Equals, s.fakescp+" -o StrictHostKeyChecking no -i x -i y -P 2022 -r -v /tmp/blah [e-mail]:baz\n")
+	err = s.client.Copy([]string{"/tmp/blah", "[e-mail]:baz", "-r", "-v"}, &opts)
+	c.Assert(err, gc.IsNil)
+	out, err = ioutil.ReadFile(s.fakescp + ".args")
+	c.Assert(err, gc.IsNil)
+	c.Assert(string(out), gc.Equals, s.fakescp+" -o StrictHostKeyChecking no -i x -i y -P 2022 /tmp/blah [e-mail]:baz -r -v\n")
+
+	// Try interspersing extra args
+	err = s.client.Copy([]string{"-r", "/tmp/blah", "-v", "[e-mail]:baz"}, &opts)
+	c.Assert(err, gc.IsNil)
+	out, err = ioutil.ReadFile(s.fakescp + ".args")
+	c.Assert(err, gc.IsNil)
+	c.Assert(string(out), gc.Equals, s.fakescp+" -o StrictHostKeyChecking no -i x -i y -P 2022 -r /tmp/blah -v [e-mail]:baz\n")
 }
 
 func (s *SSHCommandSuite) TestCommandClientKeys(c *gc.C) {