Allow assigned units read access to machines

Allow assigned units read access to machines

This branch allows unit agents to use the machiner
API to get a machine object for their assigned
machine. The server side api looks up the machine
life and the necessary auth function is modified
to allow assigned units as well as the machine
owner.

The implementation is done as a helper function
because a downstream branch also uses it. This
work will be used by the unit agent upgrade worker.

https://code.launchpad.net/~wallyworld/juju-core/machiner-access-for-uniter/+merge/205290

(do not edit description out of merge proposal)

[wallyworld, 2014-02-07 07:17:40]

Please take a look.

[dimitern, 2014-02-10 08:41:28]

Looks great so far, I have just a few suggestions.

https://codereview.appspot.com/60920043/diff/1/state/apiserver/common/auth.go
File state/apiserver/common/auth.go (right):

https://codereview.appspot.com/60920043/diff/1/state/apiserver/common/auth.go...
state/apiserver/common/auth.go:13: // OwnerOrDeployedUnitAuthFunc provides an
AuthFunc which returns true if the caller
I still think this ought to be in the Authorizer interfaces, and should be
called AuthOwnerOrDeployedUnit. Then, you can implement it in apiserver/root.go,
as the other auth funcs, and it could be simplified as well (no need to call
GetAuthTag and GetAuthEntity, because you have direct access to both).

https://codereview.appspot.com/60920043/diff/1/state/apiserver/machine/machin...
File state/apiserver/machine/machiner_test.go (right):

https://codereview.appspot.com/60920043/diff/1/state/apiserver/machine/machin...
state/apiserver/machine/machiner_test.go:215: // Test that a unit agent has read
access to the assigned machine for it's unit.
Please put this suite in a separate file, like uniter_test.go.

https://codereview.appspot.com/60920043/diff/1/state/apiserver/machine/machin...
state/apiserver/machine/machiner_test.go:309: {Tag: "machine-1"},
I'd add "machine-0" and "machine-42" to the list of entities, just to be extra
careful and check for ErrPerm.

https://codereview.appspot.com/60920043/diff/1/state/apiserver/machine/machin...
state/apiserver/machine/machiner_test.go:334: {Tag: "machine-1", Addresses:
addresses},
ditto

[wallyworld, 2014-02-11 01:14:41]

Please take a look.

https://codereview.appspot.com/60920043/diff/1/state/apiserver/common/auth.go
File state/apiserver/common/auth.go (right):

https://codereview.appspot.com/60920043/diff/1/state/apiserver/common/auth.go...
state/apiserver/common/auth.go:13: // OwnerOrDeployedUnitAuthFunc provides an
AuthFunc which returns true if the caller
On 2014/02/10 08:41:29, dimitern wrote:
> I still think this ought to be in the Authorizer interfaces, and should be
> called AuthOwnerOrDeployedUnit. Then, you can implement it in
apiserver/root.go,
> as the other auth funcs, and it could be simplified as well (no need to call
> GetAuthTag and GetAuthEntity, because you have direct access to both).

That doesn't work so well. The OwnerOrDeployedUnitAuthFunc func is a little
involved, and also needs to be implemented properly on the FakeAuthorizer. It's
better to have a common helper function and pass in the authorizer to use,
allowing the OwnerOrDeployedUnitAuthFunc to use the primitives offered by the
respective authorisers. As an aside, there a similarly complex bit of auth logic
in deployer which could also be pulled out as a separate function.

https://codereview.appspot.com/60920043/diff/1/state/apiserver/machine/machin...
File state/apiserver/machine/machiner_test.go (right):

https://codereview.appspot.com/60920043/diff/1/state/apiserver/machine/machin...
state/apiserver/machine/machiner_test.go:215: // Test that a unit agent has read
access to the assigned machine for it's unit.
On 2014/02/10 08:41:29, dimitern wrote:
> Please put this suite in a separate file, like uniter_test.go.

Done.

https://codereview.appspot.com/60920043/diff/1/state/apiserver/machine/machin...
state/apiserver/machine/machiner_test.go:309: {Tag: "machine-1"},
On 2014/02/10 08:41:29, dimitern wrote:
> I'd add "machine-0" and "machine-42" to the list of entities, just to be extra
> careful and check for ErrPerm.

Done.

https://codereview.appspot.com/60920043/diff/1/state/apiserver/machine/machin...
state/apiserver/machine/machiner_test.go:309: {Tag: "machine-1"},
On 2014/02/10 08:41:29, dimitern wrote:
> I'd add "machine-0" and "machine-42" to the list of entities, just to be extra
> careful and check for ErrPerm.

Done.

https://codereview.appspot.com/60920043/diff/1/state/apiserver/machine/machin...
state/apiserver/machine/machiner_test.go:334: {Tag: "machine-1", Addresses:
addresses},
On 2014/02/10 08:41:29, dimitern wrote:
> ditto

Done.

https://codereview.appspot.com/60920043/diff/1/state/apiserver/machine/machin...
state/apiserver/machine/machiner_test.go:334: {Tag: "machine-1", Addresses:
addresses},
On 2014/02/10 08:41:29, dimitern wrote:
> ditto

Done.

[dimitern, 2014-02-11 09:53:47]

The diff is screwed, but otherwise LGTM assuming you made what you said.

[fwereade, 2014-02-14 09:08:35]

On 2014/02/11 09:53:47, dimitern wrote:
> The diff is screwed, but otherwise LGTM assuming you made what you said.

as discussed live, the upgrader doesn't need changes so not lgtm