Code review comment for lp:~axwalk/juju-core/lp1313785-ssh-useproxy

Reviewers: mp+218911_code.launchpad.net,

Message:
Please take a look.

Description:
cmd/juju: check use-proxy before resolving address

We were translating machin/unit IDs to internal addresses
even if use-proxy=false. This manifested most prominently
when attempting to juju ssh to a machine or unit in a 1.18
environment.

Fixes lp:1313785

https://code.launchpad.net/~axwalk/juju-core/lp1313785-ssh-useproxy/+merge/218911

(do not edit description out of merge proposal)

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

Affected files (+23, -19 lines):
   A [revision details]
   M cmd/juju/scp.go
   M cmd/juju/ssh.go
   M cmd/juju/ssh_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140509022908-5uihgtxv6350ydfq
+New revision: <email address hidden>

Index: cmd/juju/scp.go
=== modified file 'cmd/juju/scp.go'
--- cmd/juju/scp.go 2014-04-12 05:53:58 +0000
+++ cmd/juju/scp.go 2014-05-09 05:17:45 +0000
@@ -102,19 +102,19 @@
    return err
   }
   defer c.apiClient.Close()
- args, err := expandArgs(c.Args, c.hostFromTarget)
- if err != nil {
- return err
- }

   var options *ssh.Options
- if proxy, err := c.proxySSH(); err != nil {
+ if c.proxy, err = c.proxySSH(); err != nil {
    return err
- } else if proxy {
+ } else if c.proxy {
    options = new(ssh.Options)
    if err := c.setProxyCommand(options); err != nil {
     return err
    }
   }
+ args, err := expandArgs(c.Args, c.hostFromTarget)
+ if err != nil {
+ return err
+ }
   return ssh.Copy(args, options)
  }

Index: cmd/juju/ssh.go
=== modified file 'cmd/juju/ssh.go'
--- cmd/juju/ssh.go 2014-04-02 06:15:57 +0000
+++ cmd/juju/ssh.go 2014-05-09 05:17:45 +0000
@@ -124,21 +124,22 @@
     }
    }()
   }
- host, err := c.hostFromTarget(c.Target)
- if err != nil {
- return err
- }
   var options ssh.Options
   if c.pty {
    options.EnablePTY()
   }
- if proxy, err := c.proxySSH(); err != nil {
+ var err error
+ if c.proxy, err = c.proxySSH(); err != nil {
    return err
- } else if proxy {
+ } else if c.proxy {
    if err := c.setProxyCommand(&options); err != nil {
     return err
    }
   }
+ host, err := c.hostFromTarget(c.Target)
+ if err != nil {
+ return err
+ }
   cmd := ssh.Command("ubuntu@"+host, c.Args, &options)
   cmd.Stdin = ctx.Stdin
   cmd.Stdout = ctx.Stdout

Index: cmd/juju/ssh_test.go
=== modified file 'cmd/juju/ssh_test.go'
--- cmd/juju/ssh_test.go 2014-04-07 00:36:36 +0000
+++ cmd/juju/ssh_test.go 2014-05-09 05:17:45 +0000
@@ -182,7 +182,7 @@
   attemptStarter.next = func() bool {
    called++
    if called > 1 {
- s.setAddress(m[0], c)
+ s.setAddresses(m[0], c)
    }
    return true
   }
@@ -191,19 +191,20 @@
   c.Assert(called, gc.Equals, 2)
  }

-func (s *SSHCommonSuite) setAddress(m *state.Machine, c *gc.C) {
- addr := instance.NewAddress(fmt.Sprintf("dummyenv-%s.dns", m.Id()),
instance.NetworkPublic)
- err := m.SetAddresses(addr)
+func (s *SSHCommonSuite) setAddresses(m *state.Machine, c *gc.C) {
+ addrPub := instance.NewAddress(fmt.Sprintf("dummyenv-%s.dns", m.Id()),
instance.NetworkPublic)
+ addrPriv := instance.NewAddress(fmt.Sprintf("dummyenv-%s.internal",
m.Id()), instance.NetworkCloudLocal)
+ err := m.SetAddresses(addrPub, addrPriv)
   c.Assert(err, gc.IsNil)
  }

-func (s *SSHCommonSuite) makeMachines(n int, c *gc.C, setAddress bool)
[]*state.Machine {
+func (s *SSHCommonSuite) makeMachines(n int, c *gc.C, setAddresses bool)
[]*state.Machine {
   var machines = make([]*state.Machine, n)
   for i := 0; i < n; i++ {
    m, err := s.State.AddMachine("quantal", state.JobHostUnits)
    c.Assert(err, gc.IsNil)
- if setAddress {
- s.setAddress(m, c)
+ if setAddresses {
+ s.setAddresses(m, c)
    }
    // must set an instance id as the ssh command uses that as a signal the
    // machine has been provisioned