apiserver/uniter: Service-related API calls

apiserver/uniter: Service-related API calls

This implements the needed server-side uniter API
service methods:
* WatchServiceRelations

In addition:
* Life
* Watch
* CharmURL
Were changed to handle both unit and service tags.

https://code.launchpad.net/~dimitern/juju-core/099-apiserver-uniter-service-ops/+merge/179886

(do not edit description out of merge proposal)

[dimitern, 2013-08-13 09:48:31]

Please take a look.

[rog, 2013-08-13 10:25:25]

Looks good, but a few possible simplifications to be made, I think.

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

https://codereview.appspot.com/12849043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:77: unit, err := u.getUnit(authTag)
I don't see the need to fetch the unit every time
we operate on the service. The service associated
with a unit cannot change, and we have already
fetched the unit when logging into the API.

If there was a way to obtain the actual authenticated
entity from common.Authorizer, then we could just do
something like:

func (u *UniterAPI) getService(tag string) (*state.Service, error) {
    unit := u.auth.Entity().(*state.Unit)
    if tag != unit.ServiceName() {
        return nil, common.ErrPerm
    }
    return unit.Service()
}

and avoid the need to worry about "never happen" errors (which
of course can happen in the presence of network problems)
and save a round trip into the bargain.

https://codereview.appspot.com/12849043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:511: func (u *UniterAPI) WatchService(args
params.Entities) (params.NotifyWatchResults, error) {
I wonder if this should just be "Watch", so that it can apply to watching units
too if we ever want to do that.

https://codereview.appspot.com/12849043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:559: func (u *UniterAPI) ServiceLife(args
params.Entities) (params.LifeResults, error) {
We are already embedding LifeGetter. Why can't that be used instead of this?

https://codereview.appspot.com/12849043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:577: func (u *UniterAPI) ServiceCharmURL(args
params.Entities) (params.StringBoolResults, error) {
I think that the same CharmURL call should be used for both services and units.
The parameters and results are the same.

https://codereview.appspot.com/12849043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:581: if len(args.Entities) == 0 {
This is redundant (same applies to all the other similar code).
Some time it might be nice to remove them all.

[dimitern, 2013-08-13 12:18:55]

Please take a look.

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

https://codereview.appspot.com/12849043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:77: unit, err := u.getUnit(authTag)
On 2013/08/13 10:25:25, rog wrote:
> I don't see the need to fetch the unit every time
> we operate on the service. The service associated
> with a unit cannot change, and we have already
> fetched the unit when logging into the API.
> 
> If there was a way to obtain the actual authenticated
> entity from common.Authorizer, then we could just do
> something like:
> 
> func (u *UniterAPI) getService(tag string) (*state.Service, error) {
>     unit := u.auth.Entity().(*state.Unit)
>     if tag != unit.ServiceName() {
>         return nil, common.ErrPerm
>     }
>     return unit.Service()
> }
> 
> and avoid the need to worry about "never happen" errors (which
> of course can happen in the presence of network problems)
> and save a round trip into the bargain.
> 

Done slightly differently, without having to fetch the unit again from state and
added a GetAuthEntity method to authorizer for that.

https://codereview.appspot.com/12849043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:511: func (u *UniterAPI) WatchService(args
params.Entities) (params.NotifyWatchResults, error) {
On 2013/08/13 10:25:25, rog wrote:
> I wonder if this should just be "Watch", so that it can apply to watching
units
> too if we ever want to do that.

Done, as discussed. Watch works on both units and services now.

https://codereview.appspot.com/12849043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:559: func (u *UniterAPI) ServiceLife(args
params.Entities) (params.LifeResults, error) {
On 2013/08/13 10:25:25, rog wrote:
> We are already embedding LifeGetter. Why can't that be used instead of this?

Done.

https://codereview.appspot.com/12849043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:577: func (u *UniterAPI) ServiceCharmURL(args
params.Entities) (params.StringBoolResults, error) {
On 2013/08/13 10:25:25, rog wrote:
> I think that the same CharmURL call should be used for both services and
units.
> The parameters and results are the same.

Done.

https://codereview.appspot.com/12849043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:581: if len(args.Entities) == 0 {
On 2013/08/13 10:25:25, rog wrote:
> This is redundant (same applies to all the other similar code).
> Some time it might be nice to remove them all.

Why is it redundant?

[jameinel, 2013-08-13 12:41:41]

LGTM

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

https://codereview.appspot.com/12849043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:581: if len(args.Entities) == 0 {
On 2013/08/13 12:18:55, dimitern wrote:
> On 2013/08/13 10:25:25, rog wrote:
> > This is redundant (same applies to all the other similar code).
> > Some time it might be nice to remove them all.
> 
> Why is it redundant?

Because if len is 0 then the for loop doesn't have anything to do and just
returns.

I'd argue that we actually want to consider if a 0 len slice is actually an
error, because the caller is probably passing stuff that we didn't see. Consider
the Machines vs Entities change. A new client will pass a list of Entities to an
old API that will probably see nothing to be processed. That should probably be
an error, and if the only way to detect it is a server-side length-zero args, we
should probably do something about it.

Though maybe you get a ErrPerm trying to access the 'nil' Machine.

https://codereview.appspot.com/12849043/diff/6001/state/apiserver/root.go
File state/apiserver/root.go (right):

https://codereview.appspot.com/12849043/diff/6001/state/apiserver/root.go#new...
state/apiserver/root.go:238: }
Shouldn't this be returning the Entity interface, rather than interface{}?

https://codereview.appspot.com/12849043/diff/6001/state/apiserver/uniter/unit...
File state/apiserver/uniter/uniter.go (left):

https://codereview.appspot.com/12849043/diff/6001/state/apiserver/uniter/unit...
state/apiserver/uniter/uniter.go:59: }
I think this would be easier to read as:

if unit, ok := entity.(*state.Unit); ok {
  return unit, nil
}
if service, ok := entity.(*state.Service); ok {
  return service, nil
}
return nil, fmt.Errorf("entity %q must be a unit or a service", tag)

(I don't *think* we put final stops in error messages, but I might be wrong)

[gz, 2013-08-13 13:25:34]

LGTM, but I struggled a little understanding the logic. Some additional comments
on the new internal functions might have helped there.

[rog, 2013-08-13 13:41:52]

much better thanks, with a few more simplifications
below. nearly there!

https://codereview.appspot.com/12849043/diff/6001/state/apiserver/root.go
File state/apiserver/root.go (right):

https://codereview.appspot.com/12849043/diff/6001/state/apiserver/root.go#new...
state/apiserver/root.go:238: }
On 2013/08/13 12:41:41, jameinel wrote:
> Shouldn't this be returning the Entity interface, rather than interface{}?

+1

https://codereview.appspot.com/12849043/diff/6001/state/apiserver/uniter/unit...
File state/apiserver/uniter/uniter.go (right):

https://codereview.appspot.com/12849043/diff/6001/state/apiserver/uniter/unit...
state/apiserver/uniter/uniter.go:44: unit, ok :=
authorizer.GetAuthEntity().(*state.Unit)
We can probably factor this out of the returned closure.

https://codereview.appspot.com/12849043/diff/6001/state/apiserver/uniter/unit...
state/apiserver/uniter/uniter.go:46: return false
I think this should be a panic - something is very wrong if the authenticated
entity is not a unit.

https://codereview.appspot.com/12849043/diff/6001/state/apiserver/uniter/unit...
state/apiserver/uniter/uniter.go:68: entity, err := u.st.FindEntity(tag)
+1 on jam's comments here, but another possibility might be:

func (u *UniterAPI) getUnitOrService(tag string) (state.Entity, error) {
    if kind, _ := names.TagKind(tag); kind != names.UnitTagKind && kind !=
names.ServiceTagKind { 
        return nil, fmt.Errorf("%q is not a unit or service tag", tag)
    }
    return u.st.Entity(tag)
}

which avoids the round trip for non-unit-or-service tags
and is somewhat simpler too. (Given that we have already determined
that we can access the given unit or service (by calling
canAccessUnitOrService), is the ErrPerm error necessary
on not-found?)

In fact, thinking about it, I don't really see why we need
getUnitOrService at all - can't we just use State.Entity?

https://codereview.appspot.com/12849043/diff/6001/state/apiserver/uniter/unit...
state/apiserver/uniter/uniter.go:99: // Make sure our unit agent's unit's
service matches what was asked for.
Hasn't this already been checked by the canAccess function?

(if it hadn't then the dynamic type conversion above
would be a possible panic)

[dimitern, 2013-08-13 14:21:48]

Please take a look.

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

https://codereview.appspot.com/12849043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:581: if len(args.Entities) == 0 {
On 2013/08/13 12:41:41, jameinel wrote:
> On 2013/08/13 12:18:55, dimitern wrote:
> > On 2013/08/13 10:25:25, rog wrote:
> > > This is redundant (same applies to all the other similar code).
> > > Some time it might be nice to remove them all.
> > 
> > Why is it redundant?
> 
> Because if len is 0 then the for loop doesn't have anything to do and just
> returns.
> 
> I'd argue that we actually want to consider if a 0 len slice is actually an
> error, because the caller is probably passing stuff that we didn't see.
Consider
> the Machines vs Entities change. A new client will pass a list of Entities to
an
> old API that will probably see nothing to be processed. That should probably
be
> an error, and if the only way to detect it is a server-side length-zero args,
we
> should probably do something about it.
> 
> Though maybe you get a ErrPerm trying to access the 'nil' Machine.

Ok, I'll remove all such if blocks. We'll have to think about "getting nothing"
when we actually make backward-incompatible changes to the API, as we did with
SetStatus. FWIW originally, I didn't have that check, but I followed your (or
rog's) model in other facades.

https://codereview.appspot.com/12849043/diff/6001/state/apiserver/root.go
File state/apiserver/root.go (right):

https://codereview.appspot.com/12849043/diff/6001/state/apiserver/root.go#new...
state/apiserver/root.go:238: }
On 2013/08/13 13:41:52, rog wrote:
> On 2013/08/13 12:41:41, jameinel wrote:
> > Shouldn't this be returning the Entity interface, rather than interface{}?
> 
> +1

Done.

https://codereview.appspot.com/12849043/diff/6001/state/apiserver/uniter/unit...
File state/apiserver/uniter/uniter.go (left):

https://codereview.appspot.com/12849043/diff/6001/state/apiserver/uniter/unit...
state/apiserver/uniter/uniter.go:59: }
On 2013/08/13 12:41:41, jameinel wrote:
> I think this would be easier to read as:
> 
> if unit, ok := entity.(*state.Unit); ok {
>   return unit, nil
> }
> if service, ok := entity.(*state.Service); ok {
>   return service, nil
> }
> return nil, fmt.Errorf("entity %q must be a unit or a service", tag)
> 
> (I don't *think* we put final stops in error messages, but I might be wrong)

Done as Roger suggested.

https://codereview.appspot.com/12849043/diff/6001/state/apiserver/uniter/unit...
File state/apiserver/uniter/uniter.go (right):

https://codereview.appspot.com/12849043/diff/6001/state/apiserver/uniter/unit...
state/apiserver/uniter/uniter.go:44: unit, ok :=
authorizer.GetAuthEntity().(*state.Unit)
On 2013/08/13 13:41:52, rog wrote:
> We can probably factor this out of the returned closure.

Done.

https://codereview.appspot.com/12849043/diff/6001/state/apiserver/uniter/unit...
state/apiserver/uniter/uniter.go:46: return false
On 2013/08/13 13:41:52, rog wrote:
> I think this should be a panic - something is very wrong if the authenticated
> entity is not a unit.

Done.

https://codereview.appspot.com/12849043/diff/6001/state/apiserver/uniter/unit...
state/apiserver/uniter/uniter.go:68: entity, err := u.st.FindEntity(tag)
On 2013/08/13 13:41:52, rog wrote:
> +1 on jam's comments here, but another possibility might be:
> 
> func (u *UniterAPI) getUnitOrService(tag string) (state.Entity, error) {
>     if kind, _ := names.TagKind(tag); kind != names.UnitTagKind && kind !=
> names.ServiceTagKind { 
>         return nil, fmt.Errorf("%q is not a unit or service tag", tag)
>     }
>     return u.st.Entity(tag)
> }
> 
> which avoids the round trip for non-unit-or-service tags
> and is somewhat simpler too. (Given that we have already determined
> that we can access the given unit or service (by calling
> canAccessUnitOrService), is the ErrPerm error necessary
> on not-found?)
> 
> In fact, thinking about it, I don't really see why we need
> getUnitOrService at all - can't we just use State.Entity?

I don't understand what round trip is there - we only get the entity once?
WatchServiceRelations does not use canAccessUnitOrService, that's why it needs
the NotFound->ErrPerm conversion, but thinking about it, it should use canAccess
like the others. Then I can use your suggestion here.

https://codereview.appspot.com/12849043/diff/6001/state/apiserver/uniter/unit...
state/apiserver/uniter/uniter.go:99: // Make sure our unit agent's unit's
service matches what was asked for.
On 2013/08/13 13:41:52, rog wrote:
> Hasn't this already been checked by the canAccess function?
> 
> (if it hadn't then the dynamic type conversion above
> would be a possible panic)

See above. This was necessary because WatchServiceRelations wasn't using
canAccess. Removed.

[dimitern, 2013-08-13 15:09:27]

Please take a look.

[rog, 2013-08-13 15:14:25]

LGTM modulo two trivials below.

https://codereview.appspot.com/12849043/diff/15001/state/apiserver/uniter/uni...
File state/apiserver/uniter/uniter.go (right):

https://codereview.appspot.com/12849043/diff/15001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter.go:56: if !canAccessService(tag) {
return canAccessUnit(tag) || canAccessService(tag)

https://codereview.appspot.com/12849043/diff/1004/state/apiserver/uniter/unit...
File state/apiserver/uniter/uniter.go (right):

https://codereview.appspot.com/12849043/diff/1004/state/apiserver/uniter/unit...
state/apiserver/uniter/uniter.go:295: var unitOrService interface{}
var unitOrService state.Entity

might as well declare the type that FindEntity is actually returning.

[dimitern, 2013-08-13 15:20:18]

Please take a look.

https://codereview.appspot.com/12849043/diff/15001/state/apiserver/uniter/uni...
File state/apiserver/uniter/uniter.go (right):

https://codereview.appspot.com/12849043/diff/15001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter.go:56: if !canAccessService(tag) {
On 2013/08/13 15:14:26, rog wrote:
> return canAccessUnit(tag) || canAccessService(tag)

Done.

https://codereview.appspot.com/12849043/diff/1004/state/apiserver/uniter/unit...
File state/apiserver/uniter/uniter.go (right):

https://codereview.appspot.com/12849043/diff/1004/state/apiserver/uniter/unit...
state/apiserver/uniter/uniter.go:295: var unitOrService interface{}
On 2013/08/13 15:14:26, rog wrote:
> var unitOrService state.Entity
> 
> might as well declare the type that FindEntity is actually returning.

Done.