Merge lp://staging/~mathieu-lonjaret/pyjuju/go-ssh into lp://staging/pyjuju/go

Reviewers: mp+91301_code.launchpad.net,

Message:
Please take a look.

Description:

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

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/5620051/

Affected files:
   A state/connect.go

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, don...

Please take a look.

https://codereview.appspot.com/5620051/

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() {
> ...

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...

Please take a look.

https://codereview.appspot.com/5620051/