Merge lp:~axwalk/juju-core/lp1246983-cli-api-ssh into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

Migrate ssh-based CLI commands to API

Two new client API methods are introduced:
  - ServiceCharmRelations
  - PublicAddress

ServiceCharmRelations returns the names of
all charm relations for the given service.
This method is used to validate hook names
for debug-hooks.

PublicAddress takes a string, which must be
either a machine ID or unit ID. The call will
return the machine or unit's public address,
or an error if the entity does not have one.

The ssh-based commands (ssh, scp, debug-log
and debug-hooks) now use these methods instead
of going directly to state. The one major
change is that, previously, they used the
instance.Instance.WaitDNSName method to get
a host address. We now use PublicAddress,
which is populated by the machine-agent's
address updater.

Note that debug-log does not use any new
log-specific API; that is a much larger task,
and out of scope here.

https://codereview.appspot.com/23380045/

Migrate ssh-based CLI commands to API

Two new client API methods are introduced:
  - ServiceCharmRelations
  - PublicAddress

ServiceCharmRelations returns the names of
all charm relations for the given service.
This method is used to validate hook names
for debug-hooks.

PublicAddress takes a string, which must be
either a machine ID or unit ID. The call will
return the machine or unit's public address,
or an error if the entity does not have one.

The ssh-based commands (ssh, scp, debug-log
and debug-hooks) now use these methods instead
of going directly to state. The one major
change is that, previously, they used the
instance.Instance.WaitDNSName method to get
a host address. We now use PublicAddress,
which is populated by the machine-agent's
address updater.

Note that debug-log does not use any new
log-specific API; that is a much larger task,
and out of scope here.

https://codereview.appspot.com/23380045/

[Andrew Wilkins (axwalk)]

Reviewers: mp+194438_code.launchpad.net,

Message:
Please take a look.

Description:
Migrate ssh-based CLI commands to API

Two new client API methods are introduced:
   - ServiceCharmRelations
   - PublicAddress

ServiceCharmRelations returns the names of
all charm relations for the given service.
This method is used to validate hook names
for debug-hooks.

PublicAddress takes a string, which must be
either a machine ID or unit ID. The call will
return the machine or unit's public address,
or an error if the entity does not have one.

The ssh-based commands (ssh, scp, debug-log
and debug-hooks) now use these methods instead
of going directly to state. The one major
change is that, previously, they used the
instance.Instance.WaitDNSName method to get
a host address. We now use PublicAddress,
which is populated by the machine-agent's
address updater.

Note that debug-log does not use any new
log-specific API; that is a much larger task,
and out of scope here.

https://code.launchpad.net/~axwalk/juju-core/lp1246983-cli-api-ssh/+merge/194438

(do not edit description out of merge proposal)

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

Affected files (+205, -83 lines):
   A [revision details]
   M cmd/juju/debughooks.go
   M cmd/juju/debughooks_test.go
   M cmd/juju/scp.go
   M cmd/juju/ssh.go
   M cmd/juju/ssh_test.go
   M state/api/client.go
   M state/api/params/params.go
   M state/apiserver/client/client.go
   M state/apiserver/client/client_test.go

[Ian Booth (wallyworld)]

LGTM with the test issues fixed.
I'd also like to see a test whereby the public address is not
immediately populated and is updated after a short delay to test that
the retry works.

https://codereview.appspot.com/23380045/diff/1/cmd/juju/debughooks.go
File cmd/juju/debughooks.go (right):

https://codereview.appspot.com/23380045/diff/1/cmd/juju/debughooks.go#newcode61
cmd/juju/debughooks.go:61: results, err :=
c.apiClient.ServiceCharmRelations(service)
small nitpick - can we rename results to relations?

https://codereview.appspot.com/23380045/diff/1/state/apiserver/client/client_test.go
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/23380045/diff/1/state/apiserver/client/client_test.go#newcode1244
state/apiserver/client/client_test.go:1244: c.Assert(err,
gc.ErrorMatches, `unit "wordpress/0" has no public address`)
I'd like to see the above error checks in a separate test eg
TestPublicAddressErrors

https://codereview.appspot.com/23380045/diff/1/state/apiserver/client/client_test.go#newcode1264
state/apiserver/client/client_test.go:1264: // Public address of unit is
taken from its machine
I'd like to see this as a separate test

https://codereview.appspot.com/23380045/diff/1/state/apiserver/client/client_test.go#newcode1270
state/apiserver/client/client_test.go:1270: // If the unit's machine has
no addresses, the public address
ditto

https://codereview.appspot.com/23380045/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/23380045/diff/1/cmd/juju/debughooks.go
File cmd/juju/debughooks.go (right):

https://codereview.appspot.com/23380045/diff/1/cmd/juju/debughooks.go#newcode61
cmd/juju/debughooks.go:61: results, err :=
c.apiClient.ServiceCharmRelations(service)
On 2013/11/08 03:32:09, wallyworld wrote:
> small nitpick - can we rename results to relations?

The result is a struct which contains the relation names, so as it is I
don't think that makes sense. I'll just change the API to return the
slice directly - it's not likely to grow any more fields.

https://codereview.appspot.com/23380045/diff/1/state/apiserver/client/client_test.go
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/23380045/diff/1/state/apiserver/client/client_test.go#newcode1244
state/apiserver/client/client_test.go:1244: c.Assert(err,
gc.ErrorMatches, `unit "wordpress/0" has no public address`)
On 2013/11/08 03:32:09, wallyworld wrote:
> I'd like to see the above error checks in a separate test eg
> TestPublicAddressErrors

Done.

https://codereview.appspot.com/23380045/diff/1/state/apiserver/client/client_test.go#newcode1264
state/apiserver/client/client_test.go:1264: // Public address of unit is
taken from its machine
On 2013/11/08 03:32:09, wallyworld wrote:
> I'd like to see this as a separate test

Done.

https://codereview.appspot.com/23380045/diff/1/state/apiserver/client/client_test.go#newcode1270
state/apiserver/client/client_test.go:1270: // If the unit's machine has
no addresses, the public address
On 2013/11/08 03:32:09, wallyworld wrote:
> ditto

Done.

https://codereview.appspot.com/23380045/

[Ian Booth (wallyworld)]

Great thanks :-)
Still LGTM

https://codereview.appspot.com/23380045/

=== modified file 'cmd/juju/debughooks.go'
--- cmd/juju/debughooks.go	2013-08-02 10:05:47 +0000
+++ cmd/juju/debughooks.go	2013-11-08 05:25:52 +0000
@@ -10,7 +10,7 @@
 
 	"launchpad.net/juju-core/charm/hooks"
 	"launchpad.net/juju-core/cmd"
-	"launchpad.net/juju-core/state"
+	"launchpad.net/juju-core/names"
 	unitdebug "launchpad.net/juju-core/worker/uniter/debug"
 )
 
@@ -38,6 +38,9 @@
 		return errors.New("no unit name specified")
 	}
 	c.Target = args[0]
+	if !names.IsUnit(c.Target) {
+		return fmt.Errorf("%q is not a valid unit name", c.Target)
+	}
 
 	// If any of the hooks is "*", then debug all hooks.
 	c.hooks = append([]string{}, args[1:]...)
@@ -50,31 +53,29 @@
 	return nil
 }
 
-func (c *DebugHooksCommand) validateHooks(unit *state.Unit) error {
+func (c *DebugHooksCommand) validateHooks() error {
 	if len(c.hooks) == 0 {
 		return nil
 	}
-	service, err := unit.Service()
-	if err != nil {
-		return err
-	}
-	eps, err := service.Endpoints()
-	if err != nil {
-		return err
-	}
+	service := names.UnitService(c.Target)
+	relations, err := c.apiClient.ServiceCharmRelations(service)
+	if err != nil {
+		return err
+	}
+
 	validHooks := make(map[string]bool)
 	for _, hook := range hooks.UnitHooks() {
 		validHooks[string(hook)] = true
 	}
-	for _, ep := range eps {
+	for _, relation := range relations {
 		for _, hook := range hooks.RelationHooks() {
-			hook := fmt.Sprintf("%s-%s", ep.Relation.Name, hook)
+			hook := fmt.Sprintf("%s-%s", relation, hook)
 			validHooks[hook] = true
 		}
 	}
 	for _, hook := range c.hooks {
 		if !validHooks[hook] {
-			return fmt.Errorf("unit %q does not contain hook %q", unit.Name(), hook)
+			return fmt.Errorf("unit %q does not contain hook %q", c.Target, hook)
 		}
 	}
 	return nil
@@ -84,16 +85,13 @@
 // and connects to it via SSH to execute the debug-hooks
 // script.
 func (c *DebugHooksCommand) Run(ctx *cmd.Context) error {
-	conn, err := c.initConn()
-	if err != nil {
-		return err
-	}
-	defer conn.Close()
-	unit, err := conn.State.Unit(c.Target)
-	if err != nil {
-		return err
-	}
-	err = c.validateHooks(unit)
+	var err error
+	c.apiClient, err = c.initAPIClient()
+	if err != nil {
+		return err
+	}
+	defer c.apiClient.Close()
+	err = c.validateHooks()
 	if err != nil {
 		return err
 	}

=== modified file 'cmd/juju/debughooks_test.go'
--- cmd/juju/debughooks_test.go	2013-08-19 11:17:19 +0000
+++ cmd/juju/debughooks_test.go	2013-11-08 05:25:52 +0000
@@ -52,7 +52,7 @@
 }, {
 	info:   `invalid unit syntax`,
 	args:   []string{"mysql"},
-	code:   1,
+	code:   2,
 	stderr: `error: "mysql" is not a valid unit name` + "\n",
 }, {
 	info:   `invalid unit`,
@@ -67,7 +67,7 @@
 }}
 
 func (s *DebugHooksSuite) TestDebugHooksCommand(c *gc.C) {
-	machines := s.makeMachines(3, c)
+	machines := s.makeMachines(3, c, true)
 	dummy := s.AddTestingCharm(c, "dummy")
 	srv, err := s.State.AddService("mysql", dummy)
 	c.Assert(err, gc.IsNil)

=== modified file 'cmd/juju/scp.go'
--- cmd/juju/scp.go	2013-08-13 19:07:35 +0000
+++ cmd/juju/scp.go	2013-11-08 05:25:52 +0000
@@ -38,11 +38,11 @@
 // forks ssh with c.Args, if provided.
 func (c *SCPCommand) Run(ctx *cmd.Context) error {
 	var err error
-	c.Conn, err = c.initConn()
+	c.apiClient, err = c.initAPIClient()
 	if err != nil {
 		return err
 	}
-	defer c.Close()
+	defer c.apiClient.Close()
 
 	// translate arguments in the form 0:/somepath or service/0:/somepath into
 	// ubuntu@machine:/somepath so they can be presented to scp.
@@ -63,6 +63,5 @@
 	cmd.Stdin = ctx.Stdin
 	cmd.Stdout = ctx.Stdout
 	cmd.Stderr = ctx.Stderr
-	c.Close()
 	return cmd.Run()
 }

=== modified file 'cmd/juju/scp_test.go'
--- cmd/juju/scp_test.go	2013-09-27 07:05:45 +0000
+++ cmd/juju/scp_test.go	2013-11-08 05:25:52 +0000
@@ -48,7 +48,7 @@
 }
 
 func (s *SCPSuite) TestSCPCommand(c *gc.C) {
-	m := s.makeMachines(3, c)
+	m := s.makeMachines(3, c, true)
 	ch := coretesting.Charms.Dir("dummy")
 	curl := charm.MustParseURL(
 		fmt.Sprintf("local:quantal/%s-%d", ch.Meta().Name, ch.Revision()),

=== modified file 'cmd/juju/ssh.go'
--- cmd/juju/ssh.go	2013-10-03 16:51:49 +0000
+++ cmd/juju/ssh.go	2013-11-08 05:25:52 +0000
@@ -5,15 +5,13 @@
 
 import (
 	"errors"
-	"fmt"
 	"os/exec"
+	"time"
 
 	"launchpad.net/juju-core/cmd"
-	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/juju"
-	"launchpad.net/juju-core/log"
-	"launchpad.net/juju-core/names"
-	"launchpad.net/juju-core/state"
+	"launchpad.net/juju-core/state/api"
+	"launchpad.net/juju-core/utils"
 )
 
 // SSHCommand is responsible for launching a ssh shell on a given unit or machine.
@@ -24,9 +22,9 @@
 // SSHCommon provides common methods for SSHCommand, SCPCommand and DebugHooksCommand.
 type SSHCommon struct {
 	cmd.EnvCommandBase
-	Target string
-	Args   []string
-	*juju.Conn
+	Target    string
+	Args      []string
+	apiClient *api.Client
 }
 
 const sshDoc = `
@@ -55,13 +53,13 @@
 // Run resolves c.Target to a machine, to the address of a i
 // machine or unit forks ssh passing any arguments provided.
 func (c *SSHCommand) Run(ctx *cmd.Context) error {
-	if c.Conn == nil {
+	if c.apiClient == nil {
 		var err error
-		c.Conn, err = c.initConn()
+		c.apiClient, err = c.initAPIClient()
 		if err != nil {
 			return err
 		}
-		defer c.Close()
+		defer c.apiClient.Close()
 	}
 	host, err := c.hostFromTarget(c.Target)
 	if err != nil {
@@ -73,62 +71,54 @@
 	cmd.Stdin = ctx.Stdin
 	cmd.Stdout = ctx.Stdout
 	cmd.Stderr = ctx.Stderr
-	c.Close()
 	return cmd.Run()
 }
 
-// initConn initialises the state connection.
+// initAPIClient initialises the state connection.
 // It is the caller's responsibility to close the connection.
-func (c *SSHCommon) initConn() (*juju.Conn, error) {
+func (c *SSHCommon) initAPIClient() (*api.Client, error) {
 	var err error
-	c.Conn, err = juju.NewConnFromName(c.EnvName)
-	return c.Conn, err
+	c.apiClient, err = juju.NewAPIClientFromName(c.EnvName)
+	return c.apiClient, err
+}
+
+// attemptStarter is an interface corresponding to utils.AttemptStrategy
+type attemptStarter interface {
+	Start() attempt
+}
+
+type attempt interface {
+	Next() bool
+}
+
+type attemptStrategy utils.AttemptStrategy
+
+func (s attemptStrategy) Start() attempt {
+	return utils.AttemptStrategy(s).Start()
+}
+
+var sshHostFromTargetAttemptStrategy attemptStarter = attemptStrategy{
+	Total: 5 * time.Second,
+	Delay: 500 * time.Millisecond,
 }
 
 func (c *SSHCommon) hostFromTarget(target string) (string, error) {
-	// is the target the id of a machine ?
-	if names.IsMachine(target) {
-		log.Infof("looking up address for machine %s...", target)
-		// TODO(dfc) maybe we should have machine.PublicAddress() ?
-		return c.machinePublicAddress(target)
-	}
-	// maybe the target is a unit ?
-	if names.IsUnit(target) {
-		log.Infof("looking up address for unit %q...", c.Target)
-		unit, err := c.State.Unit(target)
-		if err != nil {
-			return "", err
-		}
-		addr, ok := unit.PublicAddress()
-		if !ok {
-			return "", fmt.Errorf("unit %q has no public address", unit)
-		}
-		return addr, nil
-	}
-	return "", fmt.Errorf("unknown unit or machine %q", target)
-}
-
-func (c *SSHCommon) machinePublicAddress(id string) (string, error) {
-	machine, err := c.State.Machine(id)
+	var addr string
+	var err error
+	// A target may not initially have an address (e.g. the
+	// address updater hasn't yet run), so we must do this in
+	// a loop.
+	for a := sshHostFromTargetAttemptStrategy.Start(); a.Next(); {
+		addr, err = c.apiClient.PublicAddress(target)
+		if err == nil {
+			break
+		}
+	}
 	if err != nil {
 		return "", err
 	}
-	// wait for instance id
-	w := machine.Watch()
-	for _ = range w.Changes() {
-		if instid, err := machine.InstanceId(); err == nil {
-			w.Stop()
-			inst, err := c.Environ.Instances([]instance.Id{instid})
-			if err != nil {
-				return "", err
-			}
-			return inst[0].WaitDNSName()
-		} else if !state.IsNotProvisionedError(err) {
-			return "", err
-		}
-	}
-	// oops, watcher closed before we could get an answer
-	return "", w.Stop()
+	logger.Infof("Resolved public address of %q: %q", target, addr)
+	return addr, nil
 }
 
 // AllowInterspersedFlags for ssh/scp is set to false so that

=== modified file 'cmd/juju/ssh_test.go'
--- cmd/juju/ssh_test.go	2013-09-30 19:40:06 +0000
+++ cmd/juju/ssh_test.go	2013-11-08 05:25:52 +0000
@@ -97,7 +97,7 @@
 }
 
 func (s *SSHSuite) TestSSHCommand(c *gc.C) {
-	m := s.makeMachines(3, c)
+	m := s.makeMachines(3, c, true)
 	ch := coretesting.Charms.Dir("dummy")
 	curl := charm.MustParseURL(
 		fmt.Sprintf("local:quantal/%s-%d", ch.Meta().Name, ch.Revision()),
@@ -128,13 +128,69 @@
 	}
 }
 
-func (s *SSHCommonSuite) makeMachines(n int, c *gc.C) []*state.Machine {
+type callbackAttemptStarter struct {
+	next func() bool
+}
+
+func (s *callbackAttemptStarter) Start() attempt {
+	return callbackAttempt{next: s.next}
+}
+
+type callbackAttempt struct {
+	next func() bool
+}
+
+func (a callbackAttempt) Next() bool {
+	return a.next()
+}
+
+func (s *SSHSuite) TestSSHCommandHostAddressRetry(c *gc.C) {
+	m := s.makeMachines(1, c, false)
+	ctx := coretesting.Context(c)
+	jujucmd := cmd.NewSuperCommand(cmd.SuperCommandParams{})
+	jujucmd.Register(&SSHCommand{})
+
+	var called int
+	next := func() bool {
+		called++
+		return called < 2
+	}
+	attemptStarter := &callbackAttemptStarter{next: next}
+	s.PatchValue(&sshHostFromTargetAttemptStrategy, attemptStarter)
+
+	// Ensure that the ssh command waits for a public address, or the attempt
+	// strategy's Done method returns false.
+	code := cmd.Main(jujucmd, ctx, []string{"ssh", "0"})
+	c.Check(code, gc.Equals, 1)
+	c.Assert(called, gc.Equals, 2)
+	called = 0
+	attemptStarter.next = func() bool {
+		called++
+		s.setAddress(m[0], c)
+		return false
+	}
+	code = cmd.Main(jujucmd, ctx, []string{"ssh", "0"})
+	c.Check(code, gc.Equals, 0)
+	c.Assert(called, gc.Equals, 1)
+}
+
+func (s *SSHCommonSuite) setAddress(m *state.Machine, c *gc.C) {
+	addr := instance.NewAddress(fmt.Sprintf("dummyenv-%s.dns", m.Id()))
+	addr.NetworkScope = instance.NetworkPublic
+	err := m.SetAddresses([]instance.Address{addr})
+	c.Assert(err, gc.IsNil)
+}
+
+func (s *SSHCommonSuite) makeMachines(n int, c *gc.C, setAddress 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)
-		// must set an instance id as the ssh command uses that as a signal the machine
-		// has been provisioned
+		if setAddress {
+			s.setAddress(m, c)
+		}
+		// must set an instance id as the ssh command uses that as a signal the
+		// machine has been provisioned
 		inst, md := testing.AssertStartInstance(c, s.Conn.Environ, m.Id())
 		c.Assert(m.SetProvisioned(inst.Id(), "fake_nonce", md), gc.IsNil)
 		machines[i] = m

=== modified file 'state/api/client.go'
--- state/api/client.go	2013-11-07 00:26:44 +0000
+++ state/api/client.go	2013-11-08 05:25:52 +0000
@@ -63,6 +63,15 @@
 	return c.st.Call("Client", "", "Resolved", p, nil)
 }
 
+// PublicAddress returns the public address of the specified
+// machine or unit.
+func (c *Client) PublicAddress(target string) (string, error) {
+	var results params.PublicAddressResults
+	p := params.PublicAddress{Target: target}
+	err := c.st.Call("Client", "", "PublicAddress", p, &results)
+	return results.PublicAddress, err
+}
+
 // ServiceSetYAML sets configuration options on a service
 // given options in YAML format.
 func (c *Client) ServiceSetYAML(service string, yaml string) error {
@@ -95,6 +104,14 @@
 	return c.st.Call("Client", "", "DestroyRelation", params, nil)
 }
 
+// ServiceCharmRelations returns the service's charms relation names.
+func (c *Client) ServiceCharmRelations(service string) ([]string, error) {
+	var results params.ServiceCharmRelationsResults
+	params := params.ServiceCharmRelations{ServiceName: service}
+	err := c.st.Call("Client", "", "ServiceCharmRelations", params, &results)
+	return results.CharmRelations, err
+}
+
 // AddMachines adds new machines with the supplied parameters.
 func (c *Client) AddMachines(machineParams []params.AddMachineParams) ([]params.AddMachinesResult, error) {
 	args := params.AddMachines{

=== modified file 'state/api/params/params.go'
--- state/api/params/params.go	2013-11-06 22:38:06 +0000
+++ state/api/params/params.go	2013-11-08 05:25:52 +0000
@@ -160,11 +160,31 @@
 	Constraints constraints.Value
 }
 
+// ServiceCharmRelations holds parameters for making the ServiceCharmRelations call.
+type ServiceCharmRelations struct {
+	ServiceName string
+}
+
+// ServiceCharmRelationsResults holds the results of the ServiceCharmRelations call.
+type ServiceCharmRelationsResults struct {
+	CharmRelations []string
+}
+
 // ServiceUnexpose holds parameters for the ServiceUnexpose call.
 type ServiceUnexpose struct {
 	ServiceName string
 }
 
+// PublicAddress holds parameters for the PublicAddress call.
+type PublicAddress struct {
+	Target string
+}
+
+// PublicAddressResults holds results of the PublicAddress call.
+type PublicAddressResults struct {
+	PublicAddress string
+}
+
 // Resolved holds parameters for the Resolved call.
 type Resolved struct {
 	UnitName string

=== modified file 'state/apiserver/client/client.go'
--- state/apiserver/client/client.go	2013-11-08 03:09:36 +0000
+++ state/apiserver/client/client.go	2013-11-08 05:25:52 +0000
@@ -9,7 +9,9 @@
 
 	"launchpad.net/juju-core/charm"
 	"launchpad.net/juju-core/environs"
+	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/juju"
+	"launchpad.net/juju-core/names"
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/state/api"
 	"launchpad.net/juju-core/state/api/params"
@@ -131,6 +133,24 @@
 	return serviceSetSettingsYAML(svc, p.Config)
 }
 
+// ServiceCharmRelations implements the server side of Client.ServiceCharmRelations.
+func (c *Client) ServiceCharmRelations(p params.ServiceCharmRelations) (params.ServiceCharmRelationsResults, error) {
+	var results params.ServiceCharmRelationsResults
+	service, err := c.api.state.Service(p.ServiceName)
+	if err != nil {
+		return results, err
+	}
+	endpoints, err := service.Endpoints()
+	if err != nil {
+		return results, err
+	}
+	results.CharmRelations = make([]string, len(endpoints))
+	for i, endpoint := range endpoints {
+		results.CharmRelations[i] = endpoint.Relation.Name
+	}
+	return results, nil
+}
+
 // Resolved implements the server side of Client.Resolved.
 func (c *Client) Resolved(p params.Resolved) error {
 	unit, err := c.api.state.Unit(p.UnitName)
@@ -140,6 +160,34 @@
 	return unit.Resolve(p.Retry)
 }
 
+// PublicAddress implements the server side of Client.PublicAddress.
+func (c *Client) PublicAddress(p params.PublicAddress) (results params.PublicAddressResults, err error) {
+	switch {
+	case names.IsMachine(p.Target):
+		machine, err := c.api.state.Machine(p.Target)
+		if err != nil {
+			return results, err
+		}
+		addr := instance.SelectPublicAddress(machine.Addresses())
+		if addr == "" {
+			return results, fmt.Errorf("machine %q has no public address", machine)
+		}
+		return params.PublicAddressResults{PublicAddress: addr}, nil
+
+	case names.IsUnit(p.Target):
+		unit, err := c.api.state.Unit(p.Target)
+		if err != nil {
+			return results, err
+		}
+		addr, ok := unit.PublicAddress()
+		if !ok {
+			return results, fmt.Errorf("unit %q has no public address", unit)
+		}
+		return params.PublicAddressResults{PublicAddress: addr}, nil
+	}
+	return results, fmt.Errorf("unknown unit or machine %q", p.Target)
+}
+
 // ServiceExpose changes the juju-managed firewall to expose any ports that
 // were also explicitly marked by units as open.
 func (c *Client) ServiceExpose(args params.ServiceExpose) error {

=== modified file 'state/apiserver/client/client_test.go'
--- state/apiserver/client/client_test.go	2013-11-08 03:09:36 +0000
+++ state/apiserver/client/client_test.go	2013-11-08 05:25:52 +0000
@@ -1222,6 +1222,78 @@
 	c.Assert(obtained, gc.DeepEquals, cons)
 }
 
+func (s *clientSuite) TestClientServiceCharmRelations(c *gc.C) {
+	s.setUpScenario(c)
+	_, err := s.APIState.Client().ServiceCharmRelations("blah")
+	c.Assert(err, gc.ErrorMatches, `service "blah" not found`)
+
+	relations, err := s.APIState.Client().ServiceCharmRelations("wordpress")
+	c.Assert(err, gc.IsNil)
+	c.Assert(relations, gc.DeepEquals, []string{
+		"cache", "db", "juju-info", "logging-dir", "monitoring-port", "url",
+	})
+}
+
+func (s *clientSuite) TestClientPublicAddressErrors(c *gc.C) {
+	s.setUpScenario(c)
+	_, err := s.APIState.Client().PublicAddress("wordpress")
+	c.Assert(err, gc.ErrorMatches, `unknown unit or machine "wordpress"`)
+	_, err = s.APIState.Client().PublicAddress("0")
+	c.Assert(err, gc.ErrorMatches, `machine "0" has no public address`)
+	_, err = s.APIState.Client().PublicAddress("wordpress/0")
+	c.Assert(err, gc.ErrorMatches, `unit "wordpress/0" has no public address`)
+}
+
+func (s *clientSuite) TestClientPublicAddressMachine(c *gc.C) {
+	s.setUpScenario(c)
+
+	// Internally, instance.SelectPublicAddress is used; the "most public"
+	// address is returned.
+	m1, err := s.State.Machine("1")
+	c.Assert(err, gc.IsNil)
+	cloudLocalAddress := instance.NewAddress("cloudlocal")
+	cloudLocalAddress.NetworkScope = instance.NetworkCloudLocal
+	publicAddress := instance.NewAddress("public")
+	publicAddress.NetworkScope = instance.NetworkPublic
+	err = m1.SetAddresses([]instance.Address{cloudLocalAddress})
+	c.Assert(err, gc.IsNil)
+	addr, err := s.APIState.Client().PublicAddress("1")
+	c.Assert(err, gc.IsNil)
+	c.Assert(addr, gc.Equals, "cloudlocal")
+	err = m1.SetAddresses([]instance.Address{cloudLocalAddress, publicAddress})
+	addr, err = s.APIState.Client().PublicAddress("1")
+	c.Assert(err, gc.IsNil)
+	c.Assert(addr, gc.Equals, "public")
+}
+
+func (s *clientSuite) TestClientPublicAddressUnitWithMachine(c *gc.C) {
+	s.setUpScenario(c)
+
+	// Public address of unit is taken from its machine
+	// (if its machine has addresses).
+	m1, err := s.State.Machine("1")
+	publicAddress := instance.NewAddress("public")
+	publicAddress.NetworkScope = instance.NetworkPublic
+	err = m1.SetAddresses([]instance.Address{publicAddress})
+	c.Assert(err, gc.IsNil)
+	addr, err := s.APIState.Client().PublicAddress("wordpress/0")
+	c.Assert(err, gc.IsNil)
+	c.Assert(addr, gc.Equals, "public")
+}
+
+func (s *clientSuite) TestClientPublicAddressUnitWithoutMachine(c *gc.C) {
+	s.setUpScenario(c)
+	// If the unit's machine has no addresses, the public address
+	// comes from the unit's document.
+	u, err := s.State.Unit("wordpress/1")
+	c.Assert(err, gc.IsNil)
+	err = u.SetPublicAddress("127.0.0.1")
+	c.Assert(err, gc.IsNil)
+	addr, err := s.APIState.Client().PublicAddress("wordpress/1")
+	c.Assert(err, gc.IsNil)
+	c.Assert(addr, gc.Equals, "127.0.0.1")
+}
+
 func (s *clientSuite) TestClientEnvironmentGet(c *gc.C) {
 	envConfig, err := s.State.EnvironConfig()
 	c.Assert(err, gc.IsNil)