state: add ssh forwarding functionality

https://code.launchpad.net/~rogpeppe/juju/go-ssh-connect/+merge/100281

(do not edit description out of merge proposal)

[rog, 2012-03-31 11:22:35]

Please take a look.

[niemeyer, 2012-04-11 01:56:20]

I've skimmed over it, and the overall approach looks nice. As a minor, I think
sshDial should wait after it kills the forwarder so that we don't have stuff
running in the background. I'll have a more careful look tomorrow.

[fwereade, 2012-04-17 15:24:15]

IMO this needs at least one "real" test; and I'd like to know how we plan to
cope with the wait-forever-for-remote-host use case; but the general shape
appears to me to be sound.

https://codereview.appspot.com/5970053/diff/2001/state/ssh.go
File state/ssh.go (right):

https://codereview.appspot.com/5970053/diff/2001/state/ssh.go#newcode19
state/ssh.go:19: fwd, err := newSSHForwarder(addr)
Maybe this is one for a future branch, but shouldn't we really be creating a
forwarder for each of N addresses and giving ZK all the local addresses?

https://codereview.appspot.com/5970053/diff/2001/state/ssh.go#newcode87
state/ssh.go:87: var timeout <-chan time.Time
Could we call this waitErrTimeout instead?

https://codereview.appspot.com/5970053/diff/2001/state/ssh.go#newcode105
state/ssh.go:105: // going wrong and quit.
I presume we'll be handling the necessary
wait-for-server-to-exist-and-accept-connections behaviour at a higher level?

https://codereview.appspot.com/5970053/diff/2001/state/ssh.go#newcode124
state/ssh.go:124: case <-timeout:
Ah, interesting, didn't realise you could select on a nil chan.

https://codereview.appspot.com/5970053/diff/2001/state/ssh.go#newcode215
state/ssh.go:215: unknown bool	// Whether we've have not recognised the error
message.
s/have not recognised/failed to recognise/

https://codereview.appspot.com/5970053/diff/2001/state/ssh_test.go
File state/ssh_test.go (right):

https://codereview.appspot.com/5970053/diff/2001/state/ssh_test.go#newcode167
state/ssh_test.go:167: //func (sshSuite) TestSSHConnect(c *C) {
I like the approach above, but I'd be happier if we were to *also* test against
a real ssh executable.

I don't think that test needs the level of sophistication planned here -- just
hitting the happy path should be enough to verify basic sanity -- but I'm not
comfortable leaving it out entirely.

[rog, 2012-04-17 15:42:39]

https://codereview.appspot.com/5970053/diff/2001/state/ssh.go
File state/ssh.go (right):

https://codereview.appspot.com/5970053/diff/2001/state/ssh.go#newcode19
state/ssh.go:19: fwd, err := newSSHForwarder(addr)
On 2012/04/17 15:24:15, fwereade wrote:
> Maybe this is one for a future branch, but shouldn't we really be creating a
> forwarder for each of N addresses and giving ZK all the local addresses?

yes, but i'm sticking with the python version's current functionality for now.
by the time we get around to implementing multiple zk nodes, i hope we can have
a better solution than this horrible hack.

https://codereview.appspot.com/5970053/diff/2001/state/ssh.go#newcode87
state/ssh.go:87: var timeout <-chan time.Time
On 2012/04/17 15:24:15, fwereade wrote:
> Could we call this waitErrTimeout instead?

Done.

https://codereview.appspot.com/5970053/diff/2001/state/ssh.go#newcode105
state/ssh.go:105: // going wrong and quit.
On 2012/04/17 15:24:15, fwereade wrote:
> I presume we'll be handling the necessary
> wait-for-server-to-exist-and-accept-connections behaviour at a higher level?

if the ssh client fails because the server doesn't yet exist, the error (sshErr)
shouldn't be fatal and this loop will repeat.

https://codereview.appspot.com/5970053/diff/2001/state/ssh.go#newcode124
state/ssh.go:124: case <-timeout:
On 2012/04/17 15:24:15, fwereade wrote:
> Ah, interesting, didn't realise you could select on a nil chan.

it's a very useful technique. in Limbo, to do the same thing you had to create a
dummy channel, which is significantly more awkward.

https://codereview.appspot.com/5970053/diff/2001/state/ssh.go#newcode215
state/ssh.go:215: unknown bool	// Whether we've have not recognised the error
message.
On 2012/04/17 15:24:15, fwereade wrote:
> s/have not recognised/failed to recognise/

Done.

https://codereview.appspot.com/5970053/diff/2001/state/ssh_test.go
File state/ssh_test.go (right):

https://codereview.appspot.com/5970053/diff/2001/state/ssh_test.go#newcode167
state/ssh_test.go:167: //func (sshSuite) TestSSHConnect(c *C) {
On 2012/04/17 15:24:15, fwereade wrote:
> I like the approach above, but I'd be happier if we were to *also* test
against
> a real ssh executable.
> 
> I don't think that test needs the level of sophistication planned here -- just
> hitting the happy path should be enough to verify basic sanity -- but I'm not
> comfortable leaving it out entirely.

me neither - the real test is coming soon, honest!

maybe i have been a bit ambitious - that's probably why it's taking a while. see
what you think when it's ready.

[rog, 2012-04-18 17:27:18]

Please take a look.

[niemeyer, 2012-04-19 04:07:22]

Code looks quite nice. The comments on it are pretty much what's below. I'll
look into the tests tomorrow and play with it a bit.

https://codereview.appspot.com/5970053/diff/15001/state/ssh.go
File state/ssh.go (right):

https://codereview.appspot.com/5970053/diff/15001/state/ssh.go#newcode16
state/ssh.go:16: // These two variables would be constants but they are varied
s/two//

https://codereview.appspot.com/5970053/diff/15001/state/ssh.go#newcode22
state/ssh.go:22: sshKeyFile       string
This shouldn't be here. The ssh identity is configurable. Please check the
Python code and ping me if you'd like to discuss it.

https://codereview.appspot.com/5970053/diff/15001/state/ssh.go#newcode34
state/ssh.go:34: fwd.Kill(nil)
I believe this should be fwd.stop()

https://codereview.appspot.com/5970053/diff/15001/state/ssh.go#newcode42
state/ssh.go:42: fwd.Wait()
That can be fwd.stop()

https://codereview.appspot.com/5970053/diff/15001/state/ssh.go#newcode63
state/ssh.go:63: remoteHost, remotePort, err := net.SplitHostPort(remoteAddr)
Should we have a default for remotePort?

https://codereview.appspot.com/5970053/diff/15001/state/ssh.go#newcode76
state/ssh.go:76: p, err := fwd.start()
Would you mind to s/p/proc/? It's not obvious from context.

https://codereview.appspot.com/5970053/diff/15001/state/ssh.go#newcode166
state/ssh.go:166: // TODO how does the user specify a particular ssh identity?
(IdentityFile?)
Nope. Please check the Python side of things. Should be easy to follow if you
start from the forwarder.

https://codereview.appspot.com/5970053/diff/15001/state/ssh.go#newcode179
state/ssh.go:179: c := exec.Command(
This begs to be a single line.

https://codereview.appspot.com/5970053/diff/15001/state/ssh.go#newcode197
state/ssh.go:197: wait <- c.Wait()
Nice.

https://codereview.appspot.com/5970053/diff/15001/state/ssh.go#newcode201
state/ssh.go:201: go func() {
defer close just in case?

https://codereview.appspot.com/5970053/diff/15001/state/ssh.go#newcode211
state/ssh.go:211: }
We don't need stripNewline:

line = strings.TrimRight(line, "\r\n")

https://codereview.appspot.com/5970053/diff/15001/state/ssh.go#newcode264
state/ssh.go:264: log.Printf("state: ssh client says %q", s)
Something like "state: ssh: %s" would be more readable in the logs.

https://codereview.appspot.com/5970053/diff/15001/state/ssh.go#newcode277
state/ssh.go:277: err.msg = "Invalid SSH key: " + err.msg
Modifying the real SSH message seems unhelpful and perhaps misleading depending
on what's inside err.msg.

https://codereview.appspot.com/5970053/diff/15001/state/ssh.go#newcode281
state/ssh.go:281: err.msg = "SSH forwarding error: " + err.msg
Same. I'd not change the message here. If it makes sense to have a prefix, seems
saner to have a simple one like "state: ssh:" inside err.Error.

[rog, 2012-04-19 10:54:38]

Please take a look.

https://codereview.appspot.com/5970053/diff/15001/state/ssh.go
File state/ssh.go (right):

https://codereview.appspot.com/5970053/diff/15001/state/ssh.go#newcode16
state/ssh.go:16: // These two variables would be constants but they are varied
On 2012/04/19 04:07:22, niemeyer wrote:
> s/two//

Done.

https://codereview.appspot.com/5970053/diff/15001/state/ssh.go#newcode22
state/ssh.go:22: sshKeyFile       string
On 2012/04/19 04:07:22, niemeyer wrote:
> This shouldn't be here. The ssh identity is configurable. Please check the
> Python code and ping me if you'd like to discuss it.

Done.

https://codereview.appspot.com/5970053/diff/15001/state/ssh.go#newcode34
state/ssh.go:34: fwd.Kill(nil)
On 2012/04/19 04:07:22, niemeyer wrote:
> I believe this should be fwd.stop()

Done.

https://codereview.appspot.com/5970053/diff/15001/state/ssh.go#newcode42
state/ssh.go:42: fwd.Wait()
On 2012/04/19 04:07:22, niemeyer wrote:
> That can be fwd.stop()

Done.

https://codereview.appspot.com/5970053/diff/15001/state/ssh.go#newcode63
state/ssh.go:63: remoteHost, remotePort, err := net.SplitHostPort(remoteAddr)
On 2012/04/19 04:07:22, niemeyer wrote:
> Should we have a default for remotePort?

i don't *think* so. i think it's reasonable to expect the remoteAddr to be well
formed - it's not an address coming from the user, after all, and the default is
elsewhere (zkPort in environs/ec2). YMMV.

https://codereview.appspot.com/5970053/diff/15001/state/ssh.go#newcode76
state/ssh.go:76: p, err := fwd.start()
On 2012/04/19 04:07:22, niemeyer wrote:
> Would you mind to s/p/proc/? It's not obvious from context.

Done.

https://codereview.appspot.com/5970053/diff/15001/state/ssh.go#newcode166
state/ssh.go:166: // TODO how does the user specify a particular ssh identity?
(IdentityFile?)
On 2012/04/19 04:07:22, niemeyer wrote:
> Nope. Please check the Python side of things. Should be easy to follow if you
> start from the forwarder.

Done.

https://codereview.appspot.com/5970053/diff/15001/state/ssh.go#newcode179
state/ssh.go:179: c := exec.Command(
On 2012/04/19 04:07:22, niemeyer wrote:
> This begs to be a single line.

done (a hang over from when all the args were specified in line)

https://codereview.appspot.com/5970053/diff/15001/state/ssh.go#newcode201
state/ssh.go:201: go func() {
On 2012/04/19 04:07:22, niemeyer wrote:
> defer close just in case?

just in case of what? i'm not sure that it's strictly legal to close twice, and
the output will be closed when the forwarder is stopped. if the forwarder isn't
stopped, it's an error anyway and is entitled to leak. in the end, the finaliser
will close it in that case.

https://codereview.appspot.com/5970053/diff/15001/state/ssh.go#newcode211
state/ssh.go:211: }
On 2012/04/19 04:07:22, niemeyer wrote:
> We don't need stripNewline:
> 
> line = strings.TrimRight(line, "\r\n")

thanks. i'd forgotten about TrimRight.

https://codereview.appspot.com/5970053/diff/15001/state/ssh.go#newcode277
state/ssh.go:277: err.msg = "Invalid SSH key: " + err.msg
On 2012/04/19 04:07:22, niemeyer wrote:
> Modifying the real SSH message seems unhelpful and perhaps misleading
depending
> on what's inside err.msg.

i took these heuristics directly from the python code. the ssh message is indeed
misleading in this context (it just says "Permission denied" without mentioning
anything about keys - sample message: "Permission denied
(publickey,keyboard-interactive).")

the forwarding error is more of a marginal case. example error: "channel 3: open
failed: connect failed: Connection refused". i'll lose the "SSH forwarding
error" in that case, as it's moderately self-explanatory.

[niemeyer, 2012-04-19 20:08:10]

LGTM, given only trivial stuff. Great work.

https://codereview.appspot.com/5970053/diff/15001/state/ssh.go
File state/ssh.go (right):

https://codereview.appspot.com/5970053/diff/15001/state/ssh.go#newcode277
state/ssh.go:277: err.msg = "Invalid SSH key: " + err.msg
On 2012/04/19 10:54:39, rog wrote:
> On 2012/04/19 04:07:22, niemeyer wrote:
> > Modifying the real SSH message seems unhelpful and perhaps misleading
> depending
> > on what's inside err.msg.
> 
> i took these heuristics directly from the python code. the ssh message is
indeed
> misleading in this context (it just says "Permission denied" without
mentioning
> anything about keys - sample message: "Permission denied
> (publickey,keyboard-interactive).")

I'd prefer to drop the guessing work here. If we get Permission denied from ssh,
let's show whatever we got. We shouldn't pretend to know better.

https://codereview.appspot.com/5970053/diff/17001/state/ssh.go
File state/ssh.go (right):

https://codereview.appspot.com/5970053/diff/17001/state/ssh.go#newcode223
state/ssh.go:223: func stripNewline(s string) string {
This can go away now.

https://codereview.appspot.com/5970053/diff/17001/state/ssh_test.go
File state/ssh_test.go (right):

https://codereview.appspot.com/5970053/diff/17001/state/ssh_test.go#newcode42
state/ssh_test.go:42: Funcs(template.FuncMap{"xs": xs}).Parse(`#!/bin/sh
This looks unnecessarily confusing.

E.g.:

var (
        scriptTemplate = new(template.Template)
        scriptFuncs = template.FuncMap{"xs": xs}
        scriptText = `
...
`
)

func init() {
        scriptTemplate.Funcs(scriptFuncs)
        template.Must(scriptTemplate.Parse(scriptText))
}

https://codereview.appspot.com/5970053/diff/17001/state/ssh_test.go#newcode52
state/ssh_test.go:52: case ` + backQuote + `cat {{.Dir}}/runcount` + backQuote +
`
$(cat ...) avoids the use of back quotes here.

https://codereview.appspot.com/5970053/diff/17001/state/ssh_test.go#newcode78
state/ssh_test.go:78: // executable named ssh that exhibits a particular desired
misbehaviour.
Given the alternatives, sounds good.

https://codereview.appspot.com/5970053/diff/17001/state/ssh_test.go#newcode115
state/ssh_test.go:115: sshRetryInterval = oldRetryInterval
defer is so nice.

https://codereview.appspot.com/5970053/diff/17001/state/ssh_test.go#newcode123
state/ssh_test.go:123: c.Assert(err, NotNil)
Why not ErrorMatches here?

https://codereview.appspot.com/5970053/diff/17001/state/ssh_test.go#newcode148
state/ssh_test.go:148: case <-fwd.Dying():
Dead() might be more adequate here.

https://codereview.appspot.com/5970053/diff/17001/state/ssh_test.go#newcode331
state/ssh_test.go:331: //sshTest represents a running SSH test.
This is missing a space at the start.

https://codereview.appspot.com/5970053/diff/17001/state/ssh_test.go#newcode370
state/ssh_test.go:370: // client tests that a client can contact a server
through the
I found the client and server names a bit confusing when reading their use
above. Would be nice to have verbs here. Something like dialTwice and
acceptTwice, for example, which is what these methods actually do.

https://codereview.appspot.com/5970053/diff/17001/state/ssh_test.go#newcode408
state/ssh_test.go:408: // server is the server side of the client-server
equation.
LOL! Best explanation ever! :-)

[rog, 2012-04-20 11:54:05]

Please take a look.

https://codereview.appspot.com/5970053/diff/15001/state/ssh.go
File state/ssh.go (right):

https://codereview.appspot.com/5970053/diff/15001/state/ssh.go#newcode277
state/ssh.go:277: err.msg = "Invalid SSH key: " + err.msg
On 2012/04/19 20:08:10, niemeyer wrote:
> On 2012/04/19 10:54:39, rog wrote:
> > On 2012/04/19 04:07:22, niemeyer wrote:
> > > Modifying the real SSH message seems unhelpful and perhaps misleading
> > depending
> > > on what's inside err.msg.
> > 
> > i took these heuristics directly from the python code. the ssh message is
> indeed
> > misleading in this context (it just says "Permission denied" without
> mentioning
> > anything about keys - sample message: "Permission denied
> > (publickey,keyboard-interactive).")
> 
> I'd prefer to drop the guessing work here. If we get Permission denied from
ssh,
> let's show whatever we got. We shouldn't pretend to know better.

Done.

https://codereview.appspot.com/5970053/diff/17001/state/ssh.go
File state/ssh.go (right):

https://codereview.appspot.com/5970053/diff/17001/state/ssh.go#newcode223
state/ssh.go:223: func stripNewline(s string) string {
On 2012/04/19 20:08:10, niemeyer wrote:
> This can go away now.

Done.

https://codereview.appspot.com/5970053/diff/17001/state/ssh_test.go
File state/ssh_test.go (right):

https://codereview.appspot.com/5970053/diff/17001/state/ssh_test.go#newcode42
state/ssh_test.go:42: Funcs(template.FuncMap{"xs": xs}).Parse(`#!/bin/sh
On 2012/04/19 20:08:10, niemeyer wrote:
> This looks unnecessarily confusing.
> 
> E.g.:
> 
> var (
>         scriptTemplate = new(template.Template)
>         scriptFuncs = template.FuncMap{"xs": xs}
>         scriptText = `
> ...
> `
> )
> 
> func init() {
>         scriptTemplate.Funcs(scriptFuncs)
>         template.Must(scriptTemplate.Parse(scriptText))
> }

Done (slightly differently).

https://codereview.appspot.com/5970053/diff/17001/state/ssh_test.go#newcode52
state/ssh_test.go:52: case ` + backQuote + `cat {{.Dir}}/runcount` + backQuote +
`
On 2012/04/19 20:08:10, niemeyer wrote:
> $(cat ...) avoids the use of back quotes here.

I was trying to avoid bash-specific syntax, but i guess it doesn't matter so
much.
Done.

https://codereview.appspot.com/5970053/diff/17001/state/ssh_test.go#newcode123
state/ssh_test.go:123: c.Assert(err, NotNil)
On 2012/04/19 20:08:10, niemeyer wrote:
> Why not ErrorMatches here?

Done.

https://codereview.appspot.com/5970053/diff/17001/state/ssh_test.go#newcode148
state/ssh_test.go:148: case <-fwd.Dying():
On 2012/04/19 20:08:10, niemeyer wrote:
> Dead() might be more adequate here.

Done.

https://codereview.appspot.com/5970053/diff/17001/state/ssh_test.go#newcode331
state/ssh_test.go:331: //sshTest represents a running SSH test.
On 2012/04/19 20:08:10, niemeyer wrote:
> This is missing a space at the start.

Done.

https://codereview.appspot.com/5970053/diff/17001/state/ssh_test.go#newcode370
state/ssh_test.go:370: // client tests that a client can contact a server
through the
On 2012/04/19 20:08:10, niemeyer wrote:
> I found the client and server names a bit confusing when reading their use
> above. Would be nice to have verbs here. Something like dialTwice and
> acceptTwice, for example, which is what these methods actually do.

Done.

https://codereview.appspot.com/5970053/diff/17001/state/ssh_test.go#newcode408
state/ssh_test.go:408: // server is the server side of the client-server
equation.
On 2012/04/19 20:08:10, niemeyer wrote:
> LOL! Best explanation ever! :-)

ahem, probably not my finest documentation ever.

[rog, 2012-04-20 12:26:43]

*** Submitted:



state: add ssh forwarding functionality

R=niemeyer, fwereade
CC=
https://codereview.appspot.com/5970053