cmd/juju: ssh/scp commands extra arguments; fixes

cmd/juju: ssh/scp commands extra arguments; fixes

Removed the extraneous "--" that the ssh command
adds to the passed arguments, so any extra arguments
after the command will be forwarded to ssh properly.
It worked in 1.16, so this fixes a regression.
This fixes bug #1281577.

The scp command claims in its help that it supports
-- to pass extra arguments to scp, but it does not
and also does not support multiple local/remote
targets to be specified, as OpenSSH scp command does.
This fixes bug #1283412.

Also, improved a lot of ssh-related tests in both
utils/ssh/ and in cmd/juju/, adding new tests as
needed for the extra arguments.

Improved help texts for ssh, scp and add-machine
commands (included examples for the extra args
and multiple targets for scp, added ssh: example
for add-machine).

All changes are tested live:
 - bootstrapping on EC2 and running ssh or scp
 to test extra args and multiple targets;
 - manual bootstrapping
 - add-machine ssh:user@host to an existing env;
 - using juju on Windows, to test the embedded
 go.crypto ssh command.

https://code.launchpad.net/~dimitern/juju-core/302-lp-1281577-ssh-command-extra-args/+merge/207498

Requires: https://code.launchpad.net/~dimitern/juju-core/301-lp-1282553-use-curl-not-wget-precise/+merge/207443

(do not edit description out of merge proposal)

[dimitern, 2014-02-20 16:59:46]

Please take a look.

[gz, 2014-02-20 17:14:00]

LGTM, but there are a few other questionable bits involved with the original
change in r2116 that rely on sane -- behaviour - perhaps we should bug Andrew as
well.

[axw, 2014-02-21 01:46:28]

On 2014/02/20 17:14:00, gz wrote:
> LGTM, but there are a few other questionable bits involved with the original
> change in r2116 that rely on sane -- behaviour - perhaps we should bug Andrew
as
> well.

Which bits?

I don't recall my rationale, but it was probably something like: this is relying
on implementation details, namely that we're using OpenSSH.
We're not necessarily using OpenSSH now, so those args passed through will break
if the embedded client is used. I suppose it's not a big deal.

[dimitern, 2014-02-22 16:05:12]

Please take a look.

[axw, 2014-02-24 04:29:35]

https://codereview.appspot.com/66340045/diff/20001/cmd/juju/scp.go
File cmd/juju/scp.go (left):

https://codereview.appspot.com/66340045/diff/20001/cmd/juju/scp.go#oldcode73
cmd/juju/scp.go:73: // BUG(dfc) This will not work for IPv6 addresses like
2001:db8::1:2:/somepath.
The BUG is still relevant, no?

https://codereview.appspot.com/66340045/diff/20001/cmd/juju/scp.go
File cmd/juju/scp.go (right):

https://codereview.appspot.com/66340045/diff/20001/cmd/juju/scp.go#newcode32
cmd/juju/scp.go:32: Copy 2 files from two units to the local backup/ directory,
passig -v
s/passig/passing/

https://codereview.appspot.com/66340045/diff/20001/cmd/juju/scp.go#newcode35
cmd/juju/scp.go:35: juju scp ubuntu/0:/path/file1 -- -v ubuntu/1:/path/file2
backup/
While it may be possible to do this, is it useful to highlight it as an example?
It seems a bit pathological, and not actually useful, so I'd just move "-- -v"
to the end.

Also, we shouldn't *need* -- at all if the options are at the end, right? It
would good if you could just do:
    juju scp 0:/remote/dir /local/dir -r

We may want to explicitly state that options are not supported unless OpenSSH is
available on the system... although, Copy isn't even implemented using go.crypto
at the moment.

Frankly, I think it's a bad idea to rely on implementation details like this
(which is not your fault of course). It would be better if we just implemented a
subset of the scp options and passed them through. Then we're more easily able
to implement those for the go.crypto client.

https://codereview.appspot.com/66340045/diff/20001/cmd/juju/scp.go#newcode91
cmd/juju/scp.go:91: if strings.HasPrefix(arg, "-") {
Actually, one of the more common usages of pass-through args that I've seen so
far is "-o", "option". This won't work, since the option value will end up in
the wrong place.

https://codereview.appspot.com/66340045/diff/20001/utils/ssh/ssh.go
File utils/ssh/ssh.go (right):

https://codereview.appspot.com/66340045/diff/20001/utils/ssh/ssh.go#newcode72
utils/ssh/ssh.go:72: // Copy copies file(s) between the local host a remote
host(s).
s/a /and /
(or just "local and remote")

https://codereview.appspot.com/66340045/diff/20001/utils/ssh/ssh.go#newcode74
utils/ssh/ssh.go:74: // any extra arguments are specified in extraArgs, they are
passed
Client is implementation agnostic. Talking about scp here is wrong.

https://codereview.appspot.com/66340045/diff/20001/utils/ssh/ssh_openssh.go
File utils/ssh/ssh_openssh.go (right):

https://codereview.appspot.com/66340045/diff/20001/utils/ssh/ssh_openssh.go#n...
utils/ssh/ssh_openssh.go:17: var opensshCommonOptions =
map[string][]string{"-o": []string{"StrictHostKeyChecking no"}}
What's the point of this change? To group options? Why? Seems to be complicating
things for no practical benefit.

[dimitern, 2014-02-24 17:31:02]

Thanks for the review!
Find below my answers, and please review the follow-up which implements the
suggestions, here: https://codereview.appspot.com/67820048

https://codereview.appspot.com/66340045/diff/20001/cmd/juju/scp.go
File cmd/juju/scp.go (left):

https://codereview.appspot.com/66340045/diff/20001/cmd/juju/scp.go#oldcode73
cmd/juju/scp.go:73: // BUG(dfc) This will not work for IPv6 addresses like
2001:db8::1:2:/somepath.
On 2014/02/24 04:29:36, axw wrote:
> The BUG is still relevant, no?

I thought I fixed it, but I forgot. According to
http://marc.info/?l=openssh-unix-dev&m=112975365929888 we need to add
`\[`+host+`\]:` so that IPv6 addresses will be properly separated from the path.
Fixed and added a test for it.

https://codereview.appspot.com/66340045/diff/20001/cmd/juju/scp.go
File cmd/juju/scp.go (right):

https://codereview.appspot.com/66340045/diff/20001/cmd/juju/scp.go#newcode32
cmd/juju/scp.go:32: Copy 2 files from two units to the local backup/ directory,
passig -v
On 2014/02/24 04:29:36, axw wrote:
> s/passig/passing/

Done.

https://codereview.appspot.com/66340045/diff/20001/cmd/juju/scp.go#newcode35
cmd/juju/scp.go:35: juju scp ubuntu/0:/path/file1 -- -v ubuntu/1:/path/file2
backup/
On 2014/02/24 04:29:36, axw wrote:
> While it may be possible to do this, is it useful to highlight it as an
example?
> It seems a bit pathological, and not actually useful, so I'd just move "-- -v"
> to the end.
> 
> Also, we shouldn't *need* -- at all if the options are at the end, right? It
> would good if you could just do:
>     juju scp 0:/remote/dir /local/dir -r

Good point, I've changed the implementation, help text, and tests to accept
extra arguments at the end only and -- is no longer needed or allowed.

> 
> We may want to explicitly state that options are not supported unless OpenSSH
is
> available on the system... although, Copy isn't even implemented using
go.crypto
> at the moment.
> 

Mentioning OpenSSH is useful, added.

Also improved the error message given by go.crypto's Copy, because:
$ juju scp xx yy
ERROR Copy is not implemented

> Frankly, I think it's a bad idea to rely on implementation details like this
> (which is not your fault of course). It would be better if we just implemented
a
> subset of the scp options and passed them through. Then we're more easily able
> to implement those for the go.crypto client.

Yeah, that's definitely a future improvement worth doing, but not now.

https://codereview.appspot.com/66340045/diff/20001/cmd/juju/scp.go#newcode91
cmd/juju/scp.go:91: if strings.HasPrefix(arg, "-") {
On 2014/02/24 04:29:36, axw wrote:
> Actually, one of the more common usages of pass-through args that I've seen so
> far is "-o", "option". This won't work, since the option value will end up in
> the wrong place.

It works actually, added -o SomeOption in the extra args tests.

https://codereview.appspot.com/66340045/diff/20001/utils/ssh/ssh.go
File utils/ssh/ssh.go (right):

https://codereview.appspot.com/66340045/diff/20001/utils/ssh/ssh.go#newcode72
utils/ssh/ssh.go:72: // Copy copies file(s) between the local host a remote
host(s).
On 2014/02/24 04:29:36, axw wrote:
> s/a /and /
> (or just "local and remote")

Done.

https://codereview.appspot.com/66340045/diff/20001/utils/ssh/ssh.go#newcode74
utils/ssh/ssh.go:74: // any extra arguments are specified in extraArgs, they are
passed
On 2014/02/24 04:29:36, axw wrote:
> Client is implementation agnostic. Talking about scp here is wrong.

Removed the reference.

https://codereview.appspot.com/66340045/diff/20001/utils/ssh/ssh_openssh.go
File utils/ssh/ssh_openssh.go (right):

https://codereview.appspot.com/66340045/diff/20001/utils/ssh/ssh_openssh.go#n...
utils/ssh/ssh_openssh.go:17: var opensshCommonOptions =
map[string][]string{"-o": []string{"StrictHostKeyChecking no"}}
On 2014/02/24 04:29:36, axw wrote:
> What's the point of this change? To group options? Why? Seems to be
complicating
> things for no practical benefit.

The point is to improve the debug logging mostly, because I had a really hard
time initially figuring out what's going on (command and prepared arguments
format, etc.). Since opensshOptions used to build a []string directly from
*Options, it wasn't possible to recreate the actual generated command line for
ssh/scp, so I can log it. My goal was to log a valid command, so that if you
copy it from the log and paste it in a terminal will do the same.