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

[mpl]

ssh integration with state info

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

[lp:~mathieu-lonjaret/pyjuju/go-ssh]

50. By Gustavo Niemeyer on 2012-02-02

store: simplify storage of CharmEvent

This will also introduce bson marshaling of charm.URL and
port the store code to mgo tip (time.Time must be used
instead of bson.Timestamp).

R=fwereade
CC=
https://codereview.appspot.com/5606053

51. By Roger Peppe on 2012-02-03

environs/ec2: add code to create juju-specific cloudinit data.

When we create another provider, this code will be factored out,
but for the time being we'll keep it local to environs/ec2.

Also add a function to get ssh authorized keys.

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

52. By mpl on 2012-02-03

ssh integration in state

[mpl (mathieu-lonjaret)]

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

[Roger Peppe (rogpeppe)]

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

[mpl (mathieu-lonjaret)]

Please take a look.

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

[lp:~mathieu-lonjaret/pyjuju/go-ssh]

53. By mpl on 2012-02-06

addresses rog comments

[mpl (mathieu-lonjaret)]

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

[Roger Peppe (rogpeppe)]

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

[lp:~mathieu-lonjaret/pyjuju/go-ssh]

54. By mpl on 2012-02-07

wip on rog comments

55. By mpl on 2012-02-08

wip

56. By mpl on 2012-02-13

ssh tunnel with call to binary, first draft, untested

[mpl (mathieu-lonjaret)]

Please take a look.

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

Unmerged revisions

56. By mpl on 2012-02-13

ssh tunnel with call to binary, first draft, untested

55. By mpl on 2012-02-08

wip

54. By mpl on 2012-02-07

wip on rog comments

53. By mpl on 2012-02-06

addresses rog comments

52. By mpl on 2012-02-03

ssh integration in state

=== added file 'state/connect.go'
--- state/connect.go	1970-01-01 00:00:00 +0000
+++ state/connect.go	2012-02-13 14:18:21 +0000
@@ -0,0 +1,96 @@
+package state
+
+import (
+	"errors"
+	"log"
+	"math/rand"
+	"os"
+	"os/exec"
+	"strconv"
+	"strings"
+
+	"launchpad.net/gozk/zookeeper"
+)
+
+// Info represents information about a juju
+// state server or servers.
+type Info struct {
+	UseSSH        bool
+	Addrs         []string // List of IP address:port
+	SSHUsername   string   // We only implement SSH user/password authentication for now.
+	SSHPassword   string   // We could put these fields in an SSH-specific struct type.
+	SSHAddr       string
+	SSHPortsRange [2]int
+	// all fields should be public.
+}
+
+// TODO(mpl): add the key authentication
+func (info *Info) forwardPort(addr string) (string, error) {
+	localPort := ""
+	parts := strings.Split(addr, ":")
+	if len(parts) != 2 {
+		return localPort, errors.New("Wrong address format; was expecting \"host:port\"")
+	}
+	remoteHost := parts[0]
+	remotePort := parts[1]
+
+	// get a random local port in the defined range
+	if info.SSHPortsRange[1] < info.SSHPortsRange[0] {
+		return localPort, errors.New("Invalid ports range")
+	}
+	randPort := rand.Intn(info.SSHPortsRange[1]-info.SSHPortsRange[0]) + info.SSHPortsRange[0]
+	localPort = strconv.Itoa(randPort)
+
+	cmd, err := exec.LookPath("ssh")
+	if err != nil {
+		return localPort, err
+	}
+	args := []string{cmd,
+		"-o", "PasswordAuthentication no",
+		"-Llocalhost:" + localPort + ":localhost:" + remotePort,
+		info.SSHUsername + "@" + remoteHost}
+	fds := []*os.File{os.Stdin, os.Stdout, os.Stderr}
+
+	_, err = os.StartProcess(args[0], args, &os.ProcAttr{Dir: "/", Files: fds})
+	if err != nil {
+		return localPort, err
+	}
+	return localPort, nil
+}
+
+// Open returns a new State representing the
+// environment connected to using the given info.
+func Open(info *Info) (*State, error) {
+	var (
+		zk      *zookeeper.Conn
+		session <-chan zookeeper.Event
+		err     error
+	)
+	if info.UseSSH {
+		for _, addr := range info.Addrs {
+			localPort, err := info.forwardPort(addr)
+			if err != nil {
+				log.Print(err)
+				continue
+			}
+			proxyAddr := "localhost:" + localPort
+			zk, session, err = zookeeper.Dial(proxyAddr, 5e9)
+			if err != nil {
+				log.Print(err)
+				continue
+			}
+			break
+		}
+	} else {
+		zk, session, err = zookeeper.Dial(strings.Join(info.Addrs, ","), 5e9)
+		if err != nil {
+			return nil, err
+		}
+	}
+	// Wait for connection.
+	event := <-session
+	if event.State != zookeeper.STATE_CONNECTED {
+		return nil, errors.New("Could not connect to zookeeper")
+	}
+	return &State{zk}, nil
+}