worker/apiuniter: State to API migration

worker/apiuniter: State to API migration

This replaces all state calls with API calls,
fixes all tests and prepares the uniter worker
to use the API.

This is done separately from worker/uniter, so
it can be reviewed first before merging.

https://code.launchpad.net/~dimitern/juju-core/133-apiuniter-uses-api/+merge/185091

Requires: https://code.launchpad.net/~dimitern/juju-core/132-api-uniter-last-fixes/+merge/185090

(do not edit description out of merge proposal)

[dimitern, 2013-09-11 17:58:28]

Please take a look.

[fwereade, 2013-09-12 08:24:47]

Looking really solid, just a couple of questions.

https://codereview.appspot.com/13355046/diff/3001/worker/apiuniter/context.go
File worker/apiuniter/context.go (right):

https://codereview.appspot.com/13355046/diff/3001/worker/apiuniter/context.go...
worker/apiuniter/context.go:82: ctx.privateAddress, _ = unit.PrivateAddress()
Don't think that's right. Specific no-address-is-set errors we can ignore, but
surely other errors still deserve proper respect :).

https://codereview.appspot.com/13355046/diff/3001/worker/apiuniter/filter.go
File worker/apiuniter/filter.go (right):

https://codereview.appspot.com/13355046/diff/3001/worker/apiuniter/filter.go#...
worker/apiuniter/filter.go:299: }
Isn't this a maybeStopWatcher?

https://codereview.appspot.com/13355046/diff/3001/worker/apiuniter/filter.go#...
worker/apiuniter/filter.go:446: if params.ErrCode(err) == params.CodeNotFound {
Ah, so we do get a NotFound instead of PermissionDenied when trying to access a
missing unit on a still-open and unit-specific connection? Interesting, and
maybe probably correct ;).

https://codereview.appspot.com/13355046/diff/3001/worker/apiuniter/filter.go#...
worker/apiuniter/filter.go:478: if params.ErrCode(err) == params.CodeNotFound {
Likewise, we should be careful with the exact errors. I'm less sure that this
one works as expected.

https://codereview.appspot.com/13355046/diff/3001/worker/apiuniter/uniter.go
File worker/apiuniter/uniter.go (right):

https://codereview.appspot.com/13355046/diff/3001/worker/apiuniter/uniter.go#...
worker/apiuniter/uniter.go:147: u.service, err =
u.st.Service(u.unit.ServiceTag())
Not for now, but for later, if we didn't already discuss this... can we get away
with dropping the Service bits? IIRC we need it for (1) watching for death...
but we have cleanup actions that handle that now and (2) getting the proposed
charm for the unit... which should probably be expressed over the api as the
*unit's* proposed charm, similarly to what we do with agentVersion in upgrade,
and (3) getting the service config settings... which iirc we already express in
terms of the unit anyway.

What else uses it? Some relationy bits, I expect, but are there any that can't
be hidden behind the API?

https://codereview.appspot.com/13355046/diff/3001/worker/apiuniter/uniter_tes...
File worker/apiuniter/uniter_test.go (left):

https://codereview.appspot.com/13355046/diff/3001/worker/apiuniter/uniter_tes...
worker/apiuniter/uniter_test.go:1583: c.Fatalf("subordinate was not made Dying")
Whoa, nice. I expected more churn here.

https://codereview.appspot.com/13355046/diff/3001/worker/apiuniter/uniter_tes...
File worker/apiuniter/uniter_test.go (right):

https://codereview.appspot.com/13355046/diff/3001/worker/apiuniter/uniter_tes...
worker/apiuniter/uniter_test.go:1086: svc, err :=
ctx.st.AddService(csau.serviceName, sch)
if serviceName is configurable here...

https://codereview.appspot.com/13355046/diff/3001/worker/apiuniter/uniter_tes...
worker/apiuniter/uniter_test.go:1151: ctx.uniter =
apiuniter.NewUniter(ctx.s.uniter, "unit-u-0", ctx.dataDir)
...why is the unit tag here not?

[dimitern, 2013-09-12 13:07:59]

Please take a look.

https://codereview.appspot.com/13355046/diff/3001/worker/apiuniter/context.go
File worker/apiuniter/context.go (right):

https://codereview.appspot.com/13355046/diff/3001/worker/apiuniter/context.go...
worker/apiuniter/context.go:82: ctx.privateAddress, _ = unit.PrivateAddress()
On 2013/09/12 08:24:48, fwereade wrote:
> Don't think that's right. Specific no-address-is-set errors we can ignore, but
> surely other errors still deserve proper respect :).

That's right! Done.

https://codereview.appspot.com/13355046/diff/3001/worker/apiuniter/filter.go
File worker/apiuniter/filter.go (right):

https://codereview.appspot.com/13355046/diff/3001/worker/apiuniter/filter.go#...
worker/apiuniter/filter.go:299: }
On 2013/09/12 08:24:48, fwereade wrote:
> Isn't this a maybeStopWatcher?

Well, wrt the comment above - configw and relationsw both get restarted, so we
need their last value when trying to stop them, hence the explicit defer. Maybe
I'm not getting it right?

https://codereview.appspot.com/13355046/diff/3001/worker/apiuniter/filter.go#...
worker/apiuniter/filter.go:446: if params.ErrCode(err) == params.CodeNotFound {
On 2013/09/12 08:24:48, fwereade wrote:
> Ah, so we do get a NotFound instead of PermissionDenied when trying to access
a
> missing unit on a still-open and unit-specific connection? Interesting, and
> maybe probably correct ;).

Not sure, we probably don't actually. It deserves a quick test to make sure we
get it, otherwise just check for CodeUnauthorized.

EDIT: After testing, the code indeed works as expected - accessUnit in UniterAPI
just checks that authenticated entity's tag matches the one we're asked about.
Hence, the permission check will pass even if the unit is no longer in state,
and FindEntity will return NotFound, which will be relayed as CodeNotFound.

https://codereview.appspot.com/13355046/diff/3001/worker/apiuniter/filter.go#...
worker/apiuniter/filter.go:478: if params.ErrCode(err) == params.CodeNotFound {
On 2013/09/12 08:24:48, fwereade wrote:
> Likewise, we should be careful with the exact errors. I'm less sure that this
> one works as expected.

Will test and change as needed, see above.

EDIT: Tested and the code should work as expected. accessService in UniterAPI
checks only the service tag matches the tag given to it, and the service tag is
constructed from the authenticated entity's tag. Hence, the service will be
allowed for access and subsequently FindEntity will return NotFound, which will
be relayed as CodeNotFound.

https://codereview.appspot.com/13355046/diff/3001/worker/apiuniter/uniter.go
File worker/apiuniter/uniter.go (right):

https://codereview.appspot.com/13355046/diff/3001/worker/apiuniter/uniter.go#...
worker/apiuniter/uniter.go:147: u.service, err =
u.st.Service(u.unit.ServiceTag())
On 2013/09/12 08:24:48, fwereade wrote:
> Not for now, but for later, if we didn't already discuss this... can we get
away
> with dropping the Service bits? IIRC we need it for (1) watching for death...
> but we have cleanup actions that handle that now and (2) getting the proposed
> charm for the unit... which should probably be expressed over the api as the
> *unit's* proposed charm, similarly to what we do with agentVersion in upgrade,
> and (3) getting the service config settings... which iirc we already express
in
> terms of the unit anyway.
> 
> What else uses it? Some relationy bits, I expect, but are there any that can't
> be hidden behind the API?

This deserves further discussion and it's definitely out of scope for this CL.

https://codereview.appspot.com/13355046/diff/3001/worker/apiuniter/uniter_tes...
File worker/apiuniter/uniter_test.go (right):

https://codereview.appspot.com/13355046/diff/3001/worker/apiuniter/uniter_tes...
worker/apiuniter/uniter_test.go:1151: ctx.uniter =
apiuniter.NewUniter(ctx.s.uniter, "unit-u-0", ctx.dataDir)
On 2013/09/12 08:24:48, fwereade wrote:
> ...why is the unit tag here not?

It should be perhaps, although I added the serviceName above just to simulate
trying to connect to the wrong unit.

[fwereade, 2013-09-12 14:14:51]

LGTM