ProxyCommand support for go.crypto/ssh client

ProxyCommand support for go.crypto/ssh client

Implement support for the ProxyCommand option
in the go.crypto/ssh-based client. This is just
a matter of executing the provided command, and
using the process' stdin/stdout to form a
net.Conn on which the SSH session is created.

https://code.launchpad.net/~axwalk/juju-core/gocryptossh-proxycommand/+merge/211671

(do not edit description out of merge proposal)

[axw, 2014-03-19 06:53:34]

Please take a look.

[fwereade, 2014-03-24 09:34:49]

LGTM so long as it's been used live.

[axw, 2014-03-24 09:57:41]

On 2014/03/24 09:34:49, fwereade wrote:
> LGTM so long as it's been used live.

Thanks. It has been tested live, but the prereq was rolled back as it broke
local (if machine-0 doesn't have sshd). I'll wait till 1.18 it out.

[axw, 2014-03-26 08:20:05]

Please take a look.

[axw, 2014-03-26 08:22:24]

On 2014/03/26 08:20:05, axw wrote:
> Please take a look.

Updated this CL to add a proxy-config attribute to base config which defaults to
true for all providers but local.

[fwereade, 2014-03-27 12:28:56]

Couple of significant fixes needed for the proxy stuff I'm afraid.

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

https://codereview.appspot.com/77300045/diff/20001/cmd/juju/ssh.go#newcode151
cmd/juju/ssh.go:151: // this avoids another API round-trip.
Not valid, sadly: we can't depend on bootstrap config being available.

https://codereview.appspot.com/77300045/diff/20001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/77300045/diff/20001/environs/config/config.go#...
environs/config/config.go:450: panic(fmt.Errorf("proxy-ssh missing in
configuration"))
Configs written by older versions must remain valid. Can we not just treat it as
false-if-missing?

https://codereview.appspot.com/77300045/diff/20001/provider/local/environprov...
File provider/local/environprovider.go (right):

https://codereview.appspot.com/77300045/diff/20001/provider/local/environprov...
provider/local/environprovider.go:111: "proxy-ssh": false,
This is bugging me a bit. Rather than silently overwriting potential bad config,
can we instead just refuse it and error out?

[axw, 2014-03-27 13:50:53]

Please take a look.

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

https://codereview.appspot.com/77300045/diff/20001/cmd/juju/ssh.go#newcode151
cmd/juju/ssh.go:151: // this avoids another API round-trip.
On 2014/03/27 12:28:57, fwereade wrote:
> Not valid, sadly: we can't depend on bootstrap config being available.

Updated to use bootstrap config if there, else go via API. Is this valid?

https://codereview.appspot.com/77300045/diff/20001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/77300045/diff/20001/environs/config/config.go#...
environs/config/config.go:450: panic(fmt.Errorf("proxy-ssh missing in
configuration"))
On 2014/03/27 12:28:57, fwereade wrote:
> Configs written by older versions must remain valid. Can we not just treat it
as
> false-if-missing?

We do. See alwaysOptional & allDefaults. If missing, proxy-ssh takes its value
from alwaysOptional (=false); new environments take the default from allDefaults
(=true).

Got rid of the panic anyway.

[fwereade, 2014-03-28 13:38:15]

LGTM, if you drop the local config check -- it adds an implicit dependency to
cmd, on an implementation detail of environs/config; and we already have a conn
open, so it shouldn't take too long to just ask remotely. Ping me to discuss if
you feel really strongly...

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

https://codereview.appspot.com/77300045/diff/20001/cmd/juju/ssh.go#newcode151
cmd/juju/ssh.go:151: // this avoids another API round-trip.
On 2014/03/27 13:50:54, axw wrote:
> On 2014/03/27 12:28:57, fwereade wrote:
> > Not valid, sadly: we can't depend on bootstrap config being available.
> 
> Updated to use bootstrap config if there, else go via API. Is this valid?

Well, I don't love depending on the immutability, and we already have a conn
open. How much time do we save here, and how often?

[axw, 2014-04-02 02:56:55]

On 2014/03/28 13:38:15, fwereade wrote:
> LGTM, if you drop the local config check -- it adds an implicit dependency to
> cmd, on an implementation detail of environs/config; and we already have a
conn
> open, so it shouldn't take too long to just ask remotely. Ping me to discuss
if
> you feel really strongly...

Fair call. I'll remove the check, and also make proxy-ssh mutable.

> https://codereview.appspot.com/77300045/diff/20001/cmd/juju/ssh.go
> File cmd/juju/ssh.go (right):
> 
> https://codereview.appspot.com/77300045/diff/20001/cmd/juju/ssh.go#newcode151
> cmd/juju/ssh.go:151: // this avoids another API round-trip.
> On 2014/03/27 13:50:54, axw wrote:
> > On 2014/03/27 12:28:57, fwereade wrote:
> > > Not valid, sadly: we can't depend on bootstrap config being available.
> > 
> > Updated to use bootstrap config if there, else go via API. Is this valid?
> 
> Well, I don't love depending on the immutability, and we already have a conn
> open. How much time do we save here, and how often?

It adds about 1s in my tests against Azure (10s vs. 9s). Not a big deal I
suppose.
I just realised that the second "juju ssh" call is making an unnecessary API
connection, so I'll fix that up.

[axw, 2014-04-02 05:43:09]

Please take a look.

[axw, 2014-04-02 06:19:58]

Please take a look.