apiserver: (Set)(Private|Public)Address in Uniter

apiserver: (Set)(Private|Public)Address in Uniter

This implements the server-side uniter API calls:
* PublicAddress
* SetPublicAddress
* PrivateAddress
* SetPrivateAddress

...for units.

https://code.launchpad.net/~dimitern/juju-core/094-apiserver-uniter-unit-ops2/+merge/179421

(do not edit description out of merge proposal)

[dimitern, 2013-08-09 12:04:30]

Please take a look.

[natefinch, 2013-08-09 13:11:04]

LGTM modulo a couple minor code clarifications and some comments on those
exported methods.

https://codereview.appspot.com/12696043/diff/1/state/apiserver/uniter/uniter.go
File state/apiserver/uniter/uniter.go (right):

https://codereview.appspot.com/12696043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:59: func (u *UniterAPI) PublicAddress(args
params.Entities) (params.StringBoolResults, error) {
Comments on exported methods, please

https://codereview.appspot.com/12696043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:134: }
I would probably rewrite this to be a little clearer... right now,
common.ErrPerm is declared pretty far away from where it's used. Also, I'm not
entirely sure why unit is declared outside the loop.

I'd do something like this:

for i, entity := range args.Entities {
    r := result.Results[i]
    if !canRead(entity.Tag) {
        r.Error = common.ServerError(common.ErrPerm)
        continue
    }
    if unit, err := u.getUnit(entity.Tag); err == nil {
        r.Result, r.Ok := unit.PrivateAddress()
    } else {
        r.Error = common.ServerError(err)
    }
}

The same style modification can be made to SetPrivateAddress, PublicAddress and
SetPublicAddress.

[gz, 2013-08-09 13:40:50]

LGTM. As mentioned, I was to rip out these methods on the server side as soon as
possible, and implement the client side functions using different back end
methods. Landing this first, then doing that, should be a reasonable way of
avoiding conflicts.

https://codereview.appspot.com/12696043/diff/1/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/12696043/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:28: type StringBoolResult struct {
This is slightly farcical. The StringsResult seems like a concession to
pragmatism, but don't we generally want these result structs to be semantic,
rather than just creating lit-of-type-name structs?

In this particular case, if feels like we should expost !Ok as an Error return,
and just give a string back.

https://codereview.appspot.com/12696043/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:59: type SetEntityAddress struct {
Hm, would be nice to reuse this for machine, but currently the interface for
that takes []Address.

https://codereview.appspot.com/12696043/diff/1/state/apiserver/uniter/uniter.go
File state/apiserver/uniter/uniter.go (right):

https://codereview.appspot.com/12696043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:86: func (u *UniterAPI) SetPublicAddress(args
params.SetEntityAddresses) (params.ErrorResults, error) {
Hm, a lot of boilerplate in these functions, not the end of the world as they
shouldn't last long.

[dimitern, 2013-08-09 13:47:52]

Please take a look.

https://codereview.appspot.com/12696043/diff/1/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/12696043/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:28: type StringBoolResult struct {
On 2013/08/09 13:40:50, gz wrote:
> This is slightly farcical. The StringsResult seems like a concession to
> pragmatism, but don't we generally want these result structs to be semantic,
> rather than just creating lit-of-type-name structs?
> 
> In this particular case, if feels like we should expost !Ok as an Error
return,
> and just give a string back.

It was discussed to reuse result structs like this and make them non-specific
when possible. This drastically minimizes the number of identical, but
differently named params structs.

https://codereview.appspot.com/12696043/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:59: type SetEntityAddress struct {
On 2013/08/09 13:40:50, gz wrote:
> Hm, would be nice to reuse this for machine, but currently the interface for
> that takes []Address.

Hmm.. perhaps, but the []Address contains both public and private addresses
there, right?

https://codereview.appspot.com/12696043/diff/1/state/apiserver/uniter/uniter.go
File state/apiserver/uniter/uniter.go (right):

https://codereview.appspot.com/12696043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:59: func (u *UniterAPI) PublicAddress(args
params.Entities) (params.StringBoolResults, error) {
On 2013/08/09 13:11:04, nate.finch wrote:
> Comments on exported methods, please

Done.

https://codereview.appspot.com/12696043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:86: func (u *UniterAPI) SetPublicAddress(args
params.SetEntityAddresses) (params.ErrorResults, error) {
On 2013/08/09 13:40:50, gz wrote:
> Hm, a lot of boilerplate in these functions, not the end of the world as they
> shouldn't last long.

It's unavoidable unfortunately :( Ideas how to minimize that, without obscuring
the code are welcome!

https://codereview.appspot.com/12696043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:134: }
On 2013/08/09 13:11:04, nate.finch wrote:
> I would probably rewrite this to be a little clearer... right now,
> common.ErrPerm is declared pretty far away from where it's used. Also, I'm not
> entirely sure why unit is declared outside the loop.
> 
> I'd do something like this:
> 
> for i, entity := range args.Entities {
>     r := result.Results[i]
>     if !canRead(entity.Tag) {
>         r.Error = common.ServerError(common.ErrPerm)
>         continue
>     }
>     if unit, err := u.getUnit(entity.Tag); err == nil {
>         r.Result, r.Ok := unit.PrivateAddress()
>     } else {
>         r.Error = common.ServerError(err)
>     }
> }
> 
> The same style modification can be made to SetPrivateAddress, PublicAddress
and
> SetPublicAddress.

This won't work as suggested, because we want to report ErrPerm in all but the
specified cases, for security reasons. Any "unit foo not found" should be
reported as ErrPerm. That's the reason why ErrPerm is in the beginning of the
block, to emphasize it's the default error.

[natefinch, 2013-08-09 14:27:43]

I'm not sure this does what you think it does.... or I'm misunderstanding either
the code or what you expect it to do.

https://codereview.appspot.com/12696043/diff/1/state/apiserver/uniter/uniter.go
File state/apiserver/uniter/uniter.go (right):

https://codereview.appspot.com/12696043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:134: }
On 2013/08/09 13:47:52, dimitern wrote:
> 
> This won't work as suggested, because we want to report ErrPerm in all but the
> specified cases, for security reasons. Any "unit foo not found" should be
> reported as ErrPerm. That's the reason why ErrPerm is in the beginning of the
> block, to emphasize it's the default error. 

(your code)

	var unit *state.Unit
	for i, entity := range args.Entities {
		err := common.ErrPerm
		if canRead(entity.Tag) {
			unit, err = u.getUnit(entity.Tag)
			if err == nil {
				address, ok := unit.PrivateAddress()
				result.Results[i].Result = address
				result.Results[i].Ok = ok
			}
		}
		result.Results[i].Error = common.ServerError(err)
	}

walkthrough:
we set err to ErrPerm.
if we can't read, we do nothing.
we then set results.Error to err

if we can read, then we set err to whatever getUnit returns.
we then set results.Error to err

Are you sure your code is doing what you said it is? It looks like getUnit just
overwrites ErrPerm with whatever it returns, and that's what is set on the
result's Error.

I think my code does the same thing. If that's not right, then I think it's a
bug in the code as written.

[natefinch, 2013-08-09 14:41:20]

Dimiter and I spoke on IRC and I better understand what's going on. Consider
this a LGTM. :)