ssh integration with state info

https://code.launchpad.net/~mathieu-lonjaret/juju/go-ssh/+merge/91301

(do not edit description out of merge proposal)

[mpl, 2012-02-06 00:13:34]

Please take a look.

[rog, 2012-02-06 09:36:38]

thanks for doing this.
needs a bit of work but not too much!

most importantly it needs some tests.

https://codereview.appspot.com/5620051/diff/2001/state/connect.go
File state/connect.go (right):

https://codereview.appspot.com/5620051/diff/2001/state/connect.go#newcode13
state/connect.go:13: const proxy = "localhost:7890"
this can go (see below)

https://codereview.appspot.com/5620051/diff/2001/state/connect.go#newcode22
state/connect.go:22: SSHPassword string // we could probably stuff all this in
rather than having a sentence going down that right hand comment, i think an
actual comment would be better.
// We only implement SSH user/password authentication for now.
// We could put these fields in an SSH-specific struct type.

https://codereview.appspot.com/5620051/diff/2001/state/connect.go#newcode24
state/connect.go:24: // add whatever else is necessary for making the
connection.
delete this comment

https://codereview.appspot.com/5620051/diff/2001/state/connect.go#newcode28
state/connect.go:28: type myClientPassword struct {
i'm not keen on "my" - i don't think it adds useful info.

i think this might be better:

// clientPassword implements the ssh ClientPassword interface
// for a single SSH user.
type clientPassword struct {

https://codereview.appspot.com/5620051/diff/2001/state/connect.go#newcode40
state/connect.go:40: // takes the addr of the zk server to reach, and returns a
s/takes/initTunnel takes/

although i'm not sure initTunnel is a great name.
how about dialSSH ?

https://codereview.appspot.com/5620051/diff/2001/state/connect.go#newcode53
state/connect.go:53: conn, err := client.Dial("tcp", addr)
return client.Dial("tcp", addr)

https://codereview.appspot.com/5620051/diff/2001/state/connect.go#newcode65
state/connect.go:65: func (info *Info) forwarder(c chan int) {
i think the caller should be able to see the error when the dial of the ssh
address fails.

i think it's a mistake that the proxy is on a fixed port - that means that we
can't have two of these going on at the same time.

how about something like this? you'd call it every time you wanted to make a new
connection. it would be possible to see if there's an existing proxy for a given
address and reuse if so, but i don't think it's worth it for the time being.

(warning: i haven't even compiled the code below)


// newProxy returns a local proxy for the given address.
// It dials info.SSHAddr, arranges an ssh port forward
// from there to addr, listens on the proxy address
// and copies data between it and the ssh connection.
func (info *Info) newProxy(addr string) (proxy string, err error) {
    l, err := net.Listen("tcp", "localhost:0")
    if err != nil {
        return "", err
    }
    proxyAddr := l.Addr().String()

    zkServer, err := info.initTunnel(addr)
    if err != nil {
        return "", err
    }

    go func() {
       defer l.Close()
       defer zkServer.Close()

       zkClient, err := l.Accept()
       if err != nil {
           log.Printf("error accepting connection on %v: %v", l.Addr(), err)
           return
       }
       defer zkClient.Close()
       done := make(chan bool)
       go forwarder(zkClient, zkServer, done)
       go forwarder(zkServer, zkClient, done)
       <-done
       <-done
    }()
    return proxyAddr, err
}

func forwarder(w io.Writer, r io.Reader, done chan bool){
     _, err := io.Copy(w, r)
     if err != nil {
         log.Printf("network I/O error: %v", err)
     }
     done <- true
}

https://codereview.appspot.com/5620051/diff/2001/state/connect.go#newcode122
state/connect.go:122: // TODO(mpl): loop over the zk addresses instead of only
using the 1st one
it's trivially done. just use strings.Join(info.Addrs[0], ",") as the argument
to Dial.

[mpl, 2012-02-06 22:18:56]

Please take a look.

[mpl, 2012-02-06 22:29:09]

Tests will come with the next commit.

https://codereview.appspot.com/5620051/diff/2001/state/connect.go
File state/connect.go (right):

https://codereview.appspot.com/5620051/diff/2001/state/connect.go#newcode13
state/connect.go:13: const proxy = "localhost:7890"
On 2012/02/06 09:36:39, rog wrote:
> this can go (see below)

Done.

https://codereview.appspot.com/5620051/diff/2001/state/connect.go#newcode22
state/connect.go:22: SSHPassword string // we could probably stuff all this in
On 2012/02/06 09:36:39, rog wrote:
> rather than having a sentence going down that right hand comment, i think an
> actual comment would be better.
> // We only implement SSH user/password authentication for now.
> // We could put these fields in an SSH-specific struct type.

Done.

https://codereview.appspot.com/5620051/diff/2001/state/connect.go#newcode24
state/connect.go:24: // add whatever else is necessary for making the
connection.
On 2012/02/06 09:36:39, rog wrote:
> delete this comment

Done.

https://codereview.appspot.com/5620051/diff/2001/state/connect.go#newcode28
state/connect.go:28: type myClientPassword struct {
On 2012/02/06 09:36:39, rog wrote:
> i'm not keen on "my" - i don't think it adds useful info.
> 
> i think this might be better:
> 
> // clientPassword implements the ssh ClientPassword interface
> // for a single SSH user.
> type clientPassword struct {

Done.

https://codereview.appspot.com/5620051/diff/2001/state/connect.go#newcode40
state/connect.go:40: // takes the addr of the zk server to reach, and returns a
On 2012/02/06 09:36:39, rog wrote:
> s/takes/initTunnel takes/
> 
> although i'm not sure initTunnel is a great name.
> how about dialSSH ?

as agreed on irc, sshDial. Done.

https://codereview.appspot.com/5620051/diff/2001/state/connect.go#newcode53
state/connect.go:53: conn, err := client.Dial("tcp", addr)
On 2012/02/06 09:36:39, rog wrote:
> return client.Dial("tcp", addr)

Done.

https://codereview.appspot.com/5620051/diff/2001/state/connect.go#newcode65
state/connect.go:65: func (info *Info) forwarder(c chan int) {
On 2012/02/06 09:36:39, rog wrote:
> i think the caller should be able to see the error when the dial of the ssh
> address fails.
> 
> i think it's a mistake that the proxy is on a fixed port - that means that we
> can't have two of these going on at the same time.
> 
> how about something like this? you'd call it every time you wanted to make a
new
> connection. it would be possible to see if there's an existing proxy for a
given
> address and reuse if so, but i don't think it's worth it for the time being.
> 
> (warning: i haven't even compiled the code below)
> 
> 
> // newProxy returns a local proxy for the given address.
> // It dials info.SSHAddr, arranges an ssh port forward
> // from there to addr, listens on the proxy address
> // and copies data between it and the ssh connection.
> func (info *Info) newProxy(addr string) (proxy string, err error) {
>     l, err := net.Listen("tcp", "localhost:0")
>     if err != nil {
>         return "", err
>     }
>     proxyAddr := l.Addr().String()
> 
>     zkServer, err := info.initTunnel(addr)
>     if err != nil {
>         return "", err
>     }
> 
>     go func() {
>        defer l.Close()
>        defer zkServer.Close()
> 
>        zkClient, err := l.Accept()
>        if err != nil {
>            log.Printf("error accepting connection on %v: %v", l.Addr(), err)
>            return
>        }
>        defer zkClient.Close()
>        done := make(chan bool)
>        go forwarder(zkClient, zkServer, done)
>        go forwarder(zkServer, zkClient, done)
>        <-done
>        <-done
>     }()
>     return proxyAddr, err
> }
> 
> func forwarder(w io.Writer, r io.Reader, done chan bool){
>      _, err := io.Copy(w, r)
>      if err != nil {
>          log.Printf("network I/O error: %v", err)
>      }
>      done <- true
> }

Ok. but it looks like the proxy is only used for one Dial, since there's no
looping around Accept? It's fine by me, but I'm just making sure that's what we
want.

Also, I was using a chan to make sure that the proxy was as ready as possible to
be accepting before letting zookeeper.Dial happen. Why is it better to drop it?

https://codereview.appspot.com/5620051/diff/2001/state/connect.go#newcode122
state/connect.go:122: // TODO(mpl): loop over the zk addresses instead of only
using the 1st one
On 2012/02/06 09:36:39, rog wrote:
> it's trivially done. just use strings.Join(info.Addrs[0], ",") as the argument
> to Dial.

Done.

[TheMue, 2012-02-07 08:23:43]

https://codereview.appspot.com/5620051/diff/2001/state/connect.go
File state/connect.go (right):

https://codereview.appspot.com/5620051/diff/2001/state/connect.go#newcode28
state/connect.go:28: type myClientPassword struct {
Type declaration and method for clientPassword should not be in the middle of
Info and its methods.

https://codereview.appspot.com/5620051/diff/2001/state/connect.go#newcode107
state/connect.go:107: func (info *Info) Open() (*State, error) {
I'm still not happy with the idea of "opening an info to get a state". I still
prefer the more functional approach of "opening a state to get a state with an
info to control this opening.".

[rog, 2012-02-07 12:45:40]

https://codereview.appspot.com/5620051/diff/2001/state/connect.go
File state/connect.go (right):

https://codereview.appspot.com/5620051/diff/2001/state/connect.go#newcode65
state/connect.go:65: func (info *Info) forwarder(c chan int) {
On 2012/02/06 22:29:09, mpl wrote:
> On 2012/02/06 09:36:39, rog wrote:
> > i think the caller should be able to see the error when the dial of the ssh
> > address fails.
> > 
> > i think it's a mistake that the proxy is on a fixed port - that means that
we
> > can't have two of these going on at the same time.
> > 
> > how about something like this? you'd call it every time you wanted to make a
> new
> > connection. it would be possible to see if there's an existing proxy for a
> given
> > address and reuse if so, but i don't think it's worth it for the time being.
> > 
> > (warning: i haven't even compiled the code below)
> > 
> > 
> > // newProxy returns a local proxy for the given address.
> > // It dials info.SSHAddr, arranges an ssh port forward
> > // from there to addr, listens on the proxy address
> > // and copies data between it and the ssh connection.
> > func (info *Info) newProxy(addr string) (proxy string, err error) {
> >     l, err := net.Listen("tcp", "localhost:0")
> >     if err != nil {
> >         return "", err
> >     }
> >     proxyAddr := l.Addr().String()
> > 
> >     zkServer, err := info.initTunnel(addr)
> >     if err != nil {
> >         return "", err
> >     }
> > 
> >     go func() {
> >        defer l.Close()
> >        defer zkServer.Close()
> > 
> >        zkClient, err := l.Accept()
> >        if err != nil {
> >            log.Printf("error accepting connection on %v: %v", l.Addr(), err)
> >            return
> >        }
> >        defer zkClient.Close()
> >        done := make(chan bool)
> >        go forwarder(zkClient, zkServer, done)
> >        go forwarder(zkServer, zkClient, done)
> >        <-done
> >        <-done
> >     }()
> >     return proxyAddr, err
> > }
> > 
> > func forwarder(w io.Writer, r io.Reader, done chan bool){
> >      _, err := io.Copy(w, r)
> >      if err != nil {
> >          log.Printf("network I/O error: %v", err)
> >      }
> >      done <- true
> > }
> 
> Ok. but it looks like the proxy is only used for one Dial, since there's no
> looping around Accept? It's fine by me, but I'm just making sure that's what
we
> want.

hmm, thinking about this, perhaps we do want a loop, as the zk client might dial
its address multiple times.
that means we also need some way of telling the proxy to go away when the State
is closed. (perhaps closing the state should just close the listener, which
should cause Accept to fail)

> Also, I was using a chan to make sure that the proxy was as ready as possible
to
> be accepting before letting zookeeper.Dial happen. Why is it better to drop
it?

after the Listen has been done, the proxy is ready to accept.

https://codereview.appspot.com/5620051/diff/2001/state/connect.go#newcode107
state/connect.go:107: func (info *Info) Open() (*State, error) {
On 2012/02/07 08:23:44, TheMue wrote:
> I'm still not happy with the idea of "opening an info to get a state". I still
> prefer the more functional approach of "opening a state to get a state with an
> info to control this opening.".

ok. as discussed on IRC.
go with:

func Open(info *Info) (*State, error)

the current Open method can be removed, and the only place that uses it
(SetUpTest) should be changed to use this function.

https://codereview.appspot.com/5620051/diff/7001/state/connect.go
File state/connect.go (right):

https://codereview.appspot.com/5620051/diff/7001/state/connect.go#newcode42
state/connect.go:42: // dialSSH takes the addr of the zk server to reach, and
returns
s/dialSSH/sshDial/

[mpl, 2012-02-13 14:16:51]

Please take a look.