apiserver/provisioner: First part ProvisonerAPI

apiserver/provisioner: First part ProvisonerAPI

Implemented a ProvisionerAPI server-side facade with
the following methods: Life, Remove, EnsureDead,
SetPasswords, SetStatus, WatchContainers.

Reused most of apiserver/common mixins to implement
the same logic shared with the deployer and machiner
APIs. A drive-by change allows now the Remover mixin
to skip calling EnsureDead, so it can be reused better.

https://code.launchpad.net/~dimitern/juju-core/139-apiserver-provisioner/+merge/186019

(do not edit description out of merge proposal)

[dimitern, 2013-09-17 15:24:58]

Please take a look.

[rog, 2013-09-17 16:37:26]

Great direction; some comments and suggestions below.

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

https://codereview.appspot.com/13720045/diff/3001/state/api/params/internal.g...
state/api/params/internal.go:23: MachineTag    string
These names are going on the wire. Given that they're already in a field named
"ContainerTypes", do they need to be so long?

I'd have thought that Tag and Type might work OK.

It occurs to me that we already have a syntax that
has exactly the information we're trying to specify here,
in "deploy --to" - lxc:34/kvm/5

I wonder if we could just reuse that syntax for
the argument to WatchContainers, and avoid the
new ContainerType type entirely.

https://codereview.appspot.com/13720045/diff/3001/state/apiserver/common/remo...
File state/apiserver/common/remove.go (right):

https://codereview.appspot.com/13720045/diff/3001/state/apiserver/common/remo...
state/apiserver/common/remove.go:20: // NewRemover returns a new Remover. If
before calling Remove() on the
// The callEnsureDead flag specifies whether EnsureDead should
// be called on an entity before removing.

?

I'm slightly dubious about the bool flag argument - I'm
not as dogmatic as some on this point, but I think that
bool flag arguments should feel like an intuitive modification
of the function's behaviour, and I'm not sure that's the
case here.

How about something like this?

type RemoverFlags int
const (
    _ Flags = 1<<iota
    RemoverEnsureDead
)

func NewRemover(st state.EntityFinder, flags RemoverFlags, getCanModify
GetAuthFunc) *Remover

Then it's open for more behaviour modifications if necessary,
and establishes a reasonable precedent for other similar
code.

https://codereview.appspot.com/13720045/diff/3001/state/apiserver/common/remo...
state/apiserver/common/remove.go:50: if err = remover.EnsureDead(); err != nil {
err := ...

please.

https://codereview.appspot.com/13720045/diff/3001/state/apiserver/provisioner...
File state/apiserver/provisioner/provisioner.go (right):

https://codereview.appspot.com/13720045/diff/3001/state/apiserver/provisioner...
state/apiserver/provisioner/provisioner.go:39: // We allow access to any machine
in the environment.
This isn't great, as discussed online.

https://codereview.appspot.com/13720045/diff/3001/state/apiserver/provisioner...
File state/apiserver/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/13720045/diff/3001/state/apiserver/provisioner...
state/apiserver/provisioner/provisioner_test.go:44: s.machines = nil
I'd put this just before the loop that appends to s.machines.

https://codereview.appspot.com/13720045/diff/3001/state/apiserver/provisioner...
state/apiserver/provisioner/provisioner_test.go:48: var err error
Move this declaration to just before it's needed (the NewProvisionerAPI call) ?

Or (my slight preference):

prov, err := provisioner.NewProvisionerAPI(....)
c.Assert(err, gc.IsNil)
s.provisioner = prov

https://codereview.appspot.com/13720045/diff/3001/state/apiserver/provisioner...
state/apiserver/provisioner/provisioner_test.go:129: func (s *provisionerSuite)
TestLife(c *gc.C) {
it seems to me that these tests of common functionality (Life, Remove etc)
could have their own common test code. So much almost-duplicated
code. The only unique code we're needing to test in provisioner is
the WatchContainers stuff.

https://codereview.appspot.com/13720045/diff/3001/state/apiserver/provisioner...
state/apiserver/provisioner/provisioner_test.go:329: wc1.AssertNoChange()
what about subsequent events?

[dimitern, 2013-09-18 08:46:07]

Please take a look.

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

https://codereview.appspot.com/13720045/diff/3001/state/api/params/internal.g...
state/api/params/internal.go:23: MachineTag    string
On 2013/09/17 16:37:27, rog wrote:
> These names are going on the wire. Given that they're already in a field named
> "ContainerTypes", do they need to be so long?
> 
> I'd have thought that Tag and Type might work OK.
> 
> It occurs to me that we already have a syntax that
> has exactly the information we're trying to specify here,
> in "deploy --to" - lxc:34/kvm/5
> 
> I wonder if we could just reuse that syntax for
> the argument to WatchContainers, and avoid the
> new ContainerType type entirely.

I'm renaming the fields to Tag and Type, respectively.

https://codereview.appspot.com/13720045/diff/3001/state/apiserver/common/remo...
File state/apiserver/common/remove.go (right):

https://codereview.appspot.com/13720045/diff/3001/state/apiserver/common/remo...
state/apiserver/common/remove.go:20: // NewRemover returns a new Remover. If
before calling Remove() on the
On 2013/09/17 16:37:27, rog wrote:
> // The callEnsureDead flag specifies whether EnsureDead should
> // be called on an entity before removing.
> 
> ?
> 
> I'm slightly dubious about the bool flag argument - I'm
> not as dogmatic as some on this point, but I think that
> bool flag arguments should feel like an intuitive modification
> of the function's behaviour, and I'm not sure that's the
> case here.
> 
> How about something like this?
> 
> type RemoverFlags int
> const (
>     _ Flags = 1<<iota
>     RemoverEnsureDead
> )
> 
> func NewRemover(st state.EntityFinder, flags RemoverFlags, getCanModify
> GetAuthFunc) *Remover
> 
> Then it's open for more behaviour modifications if necessary,
> and establishes a reasonable precedent for other similar
> code.
> 

I think this will unnecessary complicate the definition, without bringing much
value. I'd prefer to have the flag as a boolean, at least until we decide we'll
have other flags, at which point your suggestion makes sense. I also updated the
doc comment as suggested.

https://codereview.appspot.com/13720045/diff/3001/state/apiserver/common/remo...
state/apiserver/common/remove.go:50: if err = remover.EnsureDead(); err != nil {
On 2013/09/17 16:37:27, rog wrote:
> err := ...
> 
> please.

Done.

https://codereview.appspot.com/13720045/diff/3001/state/apiserver/provisioner...
File state/apiserver/provisioner/provisioner.go (right):

https://codereview.appspot.com/13720045/diff/3001/state/apiserver/provisioner...
state/apiserver/provisioner/provisioner.go:39: // We allow access to any machine
in the environment.
On 2013/09/17 16:37:27, rog wrote:
> This isn't great, as discussed online.

Done.

https://codereview.appspot.com/13720045/diff/3001/state/apiserver/provisioner...
File state/apiserver/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/13720045/diff/3001/state/apiserver/provisioner...
state/apiserver/provisioner/provisioner_test.go:44: s.machines = nil
On 2013/09/17 16:37:27, rog wrote:
> I'd put this just before the loop that appends to s.machines.

Done.

https://codereview.appspot.com/13720045/diff/3001/state/apiserver/provisioner...
state/apiserver/provisioner/provisioner_test.go:48: var err error
On 2013/09/17 16:37:27, rog wrote:
> Move this declaration to just before it's needed (the NewProvisionerAPI call)
?
> 
> Or (my slight preference):
> 
> prov, err := provisioner.NewProvisionerAPI(....)
> c.Assert(err, gc.IsNil)
> s.provisioner = prov

Done.

https://codereview.appspot.com/13720045/diff/3001/state/apiserver/provisioner...
state/apiserver/provisioner/provisioner_test.go:129: func (s *provisionerSuite)
TestLife(c *gc.C) {
On 2013/09/17 16:37:27, rog wrote:
> it seems to me that these tests of common functionality (Life, Remove etc)
> could have their own common test code. So much almost-duplicated
> code. The only unique code we're needing to test in provisioner is
> the WatchContainers stuff.

They are not entirely duplicated, because the authorization logic is different
for different facades. For example, the uniter API accepts several different
entities for Life(), this one here accepts only machines, etc.

https://codereview.appspot.com/13720045/diff/3001/state/apiserver/provisioner...
state/apiserver/provisioner/provisioner_test.go:329: wc1.AssertNoChange()
On 2013/09/17 16:37:27, rog wrote:
> what about subsequent events?

Watcher are more thoroughly tested in the client-side API. Here we just test the
server-side part, resources registration and the initial event. It's the same as
for other facades.

[dimitern, 2013-09-18 09:13:28]

Please take a look.

[rog, 2013-09-18 09:37:51]

LGTM modulo the suggestions below.

https://codereview.appspot.com/13720045/diff/3001/state/apiserver/provisioner...
File state/apiserver/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/13720045/diff/3001/state/apiserver/provisioner...
state/apiserver/provisioner/provisioner_test.go:129: func (s *provisionerSuite)
TestLife(c *gc.C) {
On 2013/09/18 08:46:07, dimitern wrote:
> On 2013/09/17 16:37:27, rog wrote:
> > it seems to me that these tests of common functionality (Life, Remove etc)
> > could have their own common test code. So much almost-duplicated
> > code. The only unique code we're needing to test in provisioner is
> > the WatchContainers stuff.
> 
> They are not entirely duplicated, because the authorization logic is different
> for different facades. For example, the uniter API accepts several different
> entities for Life(), this one here accepts only machines, etc.

I'm sure it's possible to parameterise such a test with the
entities to be tested. Wouldn't that be better than nearly duplicating
100s of lines in several test suites?

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

https://codereview.appspot.com/13720045/diff/9001/state/api/params/internal.g...
state/api/params/internal.go:28: type ContainerTypes struct {
As discussed online, this name isn't great. Let's go with WatchContainers and
WatchContainer.

https://codereview.appspot.com/13720045/diff/9001/state/apiserver/provisioner...
File state/apiserver/provisioner/provisioner.go (right):

https://codereview.appspot.com/13720045/diff/9001/state/apiserver/provisioner...
state/apiserver/provisioner/provisioner.go:48: machine, err := st.Machine(id)
I don't think you need this db round trip.
I think:

parentId := state.ParentId(id)

should do the job just fine.

https://codereview.appspot.com/13720045/diff/9001/state/apiserver/provisioner...
state/apiserver/provisioner/provisioner.go:63: // All containers with the
authenticated machine as a
This all seems a bit more complex than necessary.
How about something like this?

return func(tag string) bool {
    if !isMachineAgent {
        return false
    }
    if tag == authEntityTag {
        // An agent can always access its own machine.
        return true
    }
    _, id, err := names.ParseTag(tag, names.MachineTagKind)
    if err != nil {
        return false
    }
    parentId := state.ParentId(id)
    if isEnvironManager {
        // The environment manager can access any machine with no parent.
        return parentId == ""
    }
    // Any provisioner can access direct children of itself.
    return names.MachineTag(parentId) == authEntityTag
}

[dimitern, 2013-09-18 09:41:42]

https://codereview.appspot.com/13720045/diff/3001/state/apiserver/provisioner...
File state/apiserver/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/13720045/diff/3001/state/apiserver/provisioner...
state/apiserver/provisioner/provisioner_test.go:129: func (s *provisionerSuite)
TestLife(c *gc.C) {
On 2013/09/18 09:37:51, rog wrote:
> On 2013/09/18 08:46:07, dimitern wrote:
> > On 2013/09/17 16:37:27, rog wrote:
> > > it seems to me that these tests of common functionality (Life, Remove etc)
> > > could have their own common test code. So much almost-duplicated
> > > code. The only unique code we're needing to test in provisioner is
> > > the WatchContainers stuff.
> > 
> > They are not entirely duplicated, because the authorization logic is
different
> > for different facades. For example, the uniter API accepts several different
> > entities for Life(), this one here accepts only machines, etc.
> 
> I'm sure it's possible to parameterise such a test with the
> entities to be tested. Wouldn't that be better than nearly duplicating
> 100s of lines in several test suites?

It's probably possible, but definitely out of scope for this CL.

https://codereview.appspot.com/13720045/diff/9001/state/apiserver/provisioner...
File state/apiserver/provisioner/provisioner.go (right):

https://codereview.appspot.com/13720045/diff/9001/state/apiserver/provisioner...
state/apiserver/provisioner/provisioner.go:48: machine, err := st.Machine(id)
On 2013/09/18 09:37:51, rog wrote:
> I don't think you need this db round trip.
> I think:
> 
> parentId := state.ParentId(id)
> 
> should do the job just fine.

We need to make sure the machine exists first.

https://codereview.appspot.com/13720045/diff/9001/state/apiserver/provisioner...
state/apiserver/provisioner/provisioner.go:63: // All containers with the
authenticated machine as a
On 2013/09/18 09:37:51, rog wrote:
> This all seems a bit more complex than necessary.
> How about something like this?
> 
> return func(tag string) bool {
>     if !isMachineAgent {
>         return false
>     }
>     if tag == authEntityTag {
>         // An agent can always access its own machine.
>         return true
>     }
>     _, id, err := names.ParseTag(tag, names.MachineTagKind)
>     if err != nil {
>         return false
>     }
>     parentId := state.ParentId(id)
>     if isEnvironManager {
>         // The environment manager can access any machine with no parent.
>         return parentId == ""
>     }
>     // Any provisioner can access direct children of itself.
>     return names.MachineTag(parentId) == authEntityTag
> }

This won't work if isMachineAgent == false and isEnvironManger == true.

[rog, 2013-09-18 10:05:48]

https://codereview.appspot.com/13720045/diff/9001/state/apiserver/provisioner...
File state/apiserver/provisioner/provisioner.go (right):

https://codereview.appspot.com/13720045/diff/9001/state/apiserver/provisioner...
state/apiserver/provisioner/provisioner.go:63: // All containers with the
authenticated machine as a
> This won't work if isMachineAgent == false and isEnvironManger == true.

True. Slightly simpler still then:

return func(tag string) bool {
    if isMachineAgent && tag == authEntityTag {
        // A machine agent can always access its own machine.
        return true
    }
    _, id, err := names.ParseTag(tag, names.MachineTagKind)
    if err != nil {
        return false
    }
    parentId := state.ParentId(id)
    if isEnvironManager {
        // The environment manager can access any machine with no parent.
        return parentId == ""
    }
    // Any provisioner can access direct children of itself.
    return names.MachineTag(parentId) == authEntityTag
}

[dimitern, 2013-09-18 10:12:03]

On 2013/09/18 10:05:48, rog wrote:
>
https://codereview.appspot.com/13720045/diff/9001/state/apiserver/provisioner...
> File state/apiserver/provisioner/provisioner.go (right):
> 
>
https://codereview.appspot.com/13720045/diff/9001/state/apiserver/provisioner...
> state/apiserver/provisioner/provisioner.go:63: // All containers with the
> authenticated machine as a
> > This won't work if isMachineAgent == false and isEnvironManger == true.
> 
> True. Slightly simpler still then:
> 
> return func(tag string) bool {
>     if isMachineAgent && tag == authEntityTag {
>         // A machine agent can always access its own machine.
>         return true
>     }
>     _, id, err := names.ParseTag(tag, names.MachineTagKind)
>     if err != nil {
>         return false
>     }
>     parentId := state.ParentId(id)
>     if isEnvironManager {
>         // The environment manager can access any machine with no parent.
>         return parentId == ""
>     }
>     // Any provisioner can access direct children of itself.
>     return names.MachineTag(parentId) == authEntityTag
> }

Done slightly differently. I want to explicitly test for isMachineAgent before
testing the parentIdTag == authEntityTag.

[dimitern, 2013-09-18 10:13:29]

Please take a look.

[rog, 2013-09-18 10:20:18]

LGTM with one final suggestion.

https://codereview.appspot.com/13720045/diff/17007/state/apiserver/provisione...
File state/apiserver/provisioner/provisioner.go (right):

https://codereview.appspot.com/13720045/diff/17007/state/apiserver/provisione...
state/apiserver/provisioner/provisioner.go:61: return isMachineAgent
the above condition can't hold without the auth entity being a machine agent (if
the auth entity tag isn't a machine, then it can't match a machine tag).

if you don't like that, then:

return isMachineAgent && names.MachineTag(parentId) == authEntityTag

should be sufficient.

[dimitern, 2013-09-18 10:24:44]

Please take a look.