provisioner: Start using the API instead of state

provisioner: Start using the API instead of state

This changes the provisioner worker to use the API,
and due to that some small changes were needed, with
regards to authorization. Now only the environment
provisioner uses the environWatcher, the LXC one
just stops it right away after WaitForEnviron returns.
Also the provisioner main loop needed to change a bit
to accommodate this.

During the initial review some concerns were raised
about giving LXC provisioner access to the environment
and thus to cloud secrets. This is partially addressed
here with changes to agent.Config, and now we support
two authenticators: environment and agent config based.

In follow-ups the additional changes to restrict access
to any secrets in the environment configuration will be
made.

Live tested on EC2 and it works!

https://code.launchpad.net/~dimitern/juju-core/144-worker-provisioner-uses-api/+merge/187191

Requires: https://code.launchpad.net/~dimitern/juju-core/143-api-agent-containertype/+merge/186746

(do not edit description out of merge proposal)

[dimitern, 2013-09-24 10:14:51]

Please take a look.

[mue, 2013-09-24 12:53:51]

Not yet sure enough with this, have to get more into it. But here are some first
comments.

https://codereview.appspot.com/13720051/diff/1/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/13720051/diff/1/cmd/jujud/machine.go#newcode192
cmd/jujud/machine.go:192: // At this stage, since we don't embed lxc containers,
just start an lxc
s/lxc/LXC/

https://codereview.appspot.com/13720051/diff/1/worker/provisioner/authenticat...
File worker/provisioner/authentication.go (right):

https://codereview.appspot.com/13720051/diff/1/worker/provisioner/authenticat...
worker/provisioner/authentication.go:15: type TaggedPasswordChanger interface {
Would like a comment here.

https://codereview.appspot.com/13720051/diff/1/worker/provisioner/provisioner...
File worker/provisioner/provisioner_task.go (right):

https://codereview.appspot.com/13720051/diff/1/worker/provisioner/provisioner...
worker/provisioner/provisioner_task.go:39: func NewProvisionerTask(
Always feeling a bit puzzled when seeing functions with 5 arguments or more. But
that's only personal.

https://codereview.appspot.com/13720051/diff/1/worker/provisioner/provisioner...
worker/provisioner/provisioner_task.go:178: case params.IsCodeNotFound(err):
"Code" here, "tag" used above, machine "id" in message. How is "code" meant
here? Isn't t a "tag not found"?

[fwereade, 2013-09-24 14:04:13]

OK, I think the Right Thing to do here is to fix SimpleAuthenticator so it
doesn't use an environ in this CL -- so that we can land this and the
secrets-hole-closing branches independently -- and leave all other
architecture-messing for later.

Before we do a release we should also fix the watchers to return tags instead of
ids, but that's not for this CL either; needs a bug, though, and please assign
it to yourself.

Should be ready to land after the SimpleAuthenticator change and a quick pass
over the other comments.

https://codereview.appspot.com/13720051/diff/1/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/13720051/diff/1/cmd/jujud/machine.go#newcode201
cmd/jujud/machine.go:201: if providerType != provider.Local &&
entity.ContainerType() != instance.LXC {
Aw, man, this freaks me out -- same worries as in
https://codereview.appspot.com/13255051/ -- and I think I'm going to take the
same approach: land this bit as is, but immediately assign yourself a tech-debt
bug to resolve it, possibly by means of a machine job. You, me, and axw should
try to chat first thing tomorrow morning, I think.

https://codereview.appspot.com/13720051/diff/1/worker/provisioner/authenticat...
File worker/provisioner/authentication.go (right):

https://codereview.appspot.com/13720051/diff/1/worker/provisioner/authenticat...
worker/provisioner/authentication.go:40: func (auth *simpleAuth)
SetupAuthentication(machine TaggedPasswordChanger) (*state.Info, *api.Info,
error) {
Should we be returning nil state infos or something now? Not actionable now but
worth a chat I think.

https://codereview.appspot.com/13720051/diff/1/worker/provisioner/provisioner.go
File worker/provisioner/provisioner.go (left):

https://codereview.appspot.com/13720051/diff/1/worker/provisioner/provisioner...
worker/provisioner/provisioner.go:85: p.environ, err =
worker.WaitForEnviron(environWatcher, p.st, p.tomb.Dying())
This is a problem. If we're constructing an environ, we've just used the
secrets, even on an LXC provisioner.

https://codereview.appspot.com/13720051/diff/1/worker/provisioner/provisioner.go
File worker/provisioner/provisioner.go (right):

https://codereview.appspot.com/13720051/diff/1/worker/provisioner/provisioner...
worker/provisioner/provisioner.go:101: watcher.Stop(environWatcher, &p.tomb)
I think it's more suitable to do this explicitly here -- shouldn't continue if
there's an error, but can't tell without inspecting the tomb, which is tedious;
so how about a plain old:

if err := environWatcher.Stop(); err != nil {
    return err
}

...and tbh I think you could simplify the bit above:

defer watcher.Stop(environWatcher, &p.tomb)

...in which case, eh, you'd kill the tomb twice with the same error if the stop
failed, but I don't think that'd be a practical concern at any stage.

https://codereview.appspot.com/13720051/diff/1/worker/provisioner/provisioner...
worker/provisioner/provisioner.go:135: if environWatcher != nil {
Ah, and now I see why you did that. But you can simplify this method quite a
lot, I think, if you accept the changes chan alone: if it's not an env
provisioner you pass nil, and if it is you pass environWatcher.Changes(). Then
you can drop the if/else and the duplication of the Dying-handling.

*really* the environment-watching ought to be entirely separate, I think. But
that's not for today.

https://codereview.appspot.com/13720051/diff/1/worker/provisioner/provisioner...
File worker/provisioner/provisioner_task.go (right):

https://codereview.appspot.com/13720051/diff/1/worker/provisioner/provisioner...
worker/provisioner/provisioner_task.go:39: func NewProvisionerTask(
On 2013/09/24 12:53:52, mue wrote:
> Always feeling a bit puzzled when seeing functions with 5 arguments or more.
But
> that's only personal.

It's not only personal, it's a very good point. But it's not one to resolve in
this CL.

https://codereview.appspot.com/13720051/diff/1/worker/provisioner/provisioner...
worker/provisioner/provisioner_task.go:178: case params.IsCodeNotFound(err):
On 2013/09/24 12:53:52, mue wrote:
> "Code" here, "tag" used above, machine "id" in message. How is "code" meant
> here? Isn't t a "tag not found"?

Code is the mechanism by which we communicate error types over the API,
basically.

Id is here because... hmm. Why is id here? I guess we've got the watcher API
producing ids instead of tags? That should be fixed, I think, even if we convert
straight back to ids for the actual nuts and bolts of the task for now.

[rog, 2013-09-24 14:05:45]

Sorry, forgot to publish this earlier.
LGTM modulo the below.

https://codereview.appspot.com/13720051/diff/1/worker/environ.go
File worker/environ.go (right):

https://codereview.appspot.com/13720051/diff/1/worker/environ.go#newcode22
worker/environ.go:22: type EnvironConfiger interface {
Or EnvironConfigGetter ?

https://codereview.appspot.com/13720051/diff/1/worker/provisioner/provisioner.go
File worker/provisioner/provisioner.go (right):

https://codereview.appspot.com/13720051/diff/1/worker/provisioner/provisioner...
worker/provisioner/provisioner.go:123: provisionerTask := NewProvisionerTask(
s/provisionerTask/task/ ?

This is all provisioner code after all, and the type
makes it quite clear what kind of task this is.

https://codereview.appspot.com/13720051/diff/1/worker/provisioner/provisioner...
worker/provisioner/provisioner.go:131: return p.mainLoop(environWatcher,
provisionerTask)
Rather than having one mainLoop function with
the very large if statement, I'd be tempted to do:

if environWatcher != nil {
    return environmentProvisionerMainLoop(environWatcher, provisionerTask)
}
return localProvisionerMainLoop(provisionerTask)

https://codereview.appspot.com/13720051/diff/1/worker/provisioner/provisioner...
worker/provisioner/provisioner.go:164: logger.Errorf("%s provisioner died: %v",
p.st, err)
Using the state as a Stringer seems a little bit icky here.
How about implementing String on ProvisionerType instead?

[dimitern, 2013-09-25 15:32:20]

Please take a look.

https://codereview.appspot.com/13720051/diff/1/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/13720051/diff/1/cmd/jujud/machine.go#newcode192
cmd/jujud/machine.go:192: // At this stage, since we don't embed lxc containers,
just start an lxc
On 2013/09/24 12:53:52, mue wrote:
> s/lxc/LXC/

Done.

https://codereview.appspot.com/13720051/diff/1/cmd/jujud/machine.go#newcode201
cmd/jujud/machine.go:201: if providerType != provider.Local &&
entity.ContainerType() != instance.LXC {
On 2013/09/24 14:04:13, fwereade wrote:
> Aw, man, this freaks me out -- same worries as in
> https://codereview.appspot.com/13255051/ -- and I think I'm going to take the
> same approach: land this bit as is, but immediately assign yourself a
tech-debt
> bug to resolve it, possibly by means of a machine job. You, me, and axw should
> try to chat first thing tomorrow morning, I think.

Added a TODO and a tech-dept bug 
https://bugs.launchpad.net/juju-core/+bug/1230289.

https://codereview.appspot.com/13720051/diff/1/worker/environ.go
File worker/environ.go (right):

https://codereview.appspot.com/13720051/diff/1/worker/environ.go#newcode22
worker/environ.go:22: type EnvironConfiger interface {
On 2013/09/24 14:05:45, rog wrote:
> Or EnvironConfigGetter ?

Done.

https://codereview.appspot.com/13720051/diff/1/worker/provisioner/authenticat...
File worker/provisioner/authentication.go (right):

https://codereview.appspot.com/13720051/diff/1/worker/provisioner/authenticat...
worker/provisioner/authentication.go:15: type TaggedPasswordChanger interface {
On 2013/09/24 12:53:52, mue wrote:
> Would like a comment here.

Done.

https://codereview.appspot.com/13720051/diff/1/worker/provisioner/provisioner.go
File worker/provisioner/provisioner.go (left):

https://codereview.appspot.com/13720051/diff/1/worker/provisioner/provisioner...
worker/provisioner/provisioner.go:85: p.environ, err =
worker.WaitForEnviron(environWatcher, p.st, p.tomb.Dying())
On 2013/09/24 14:04:13, fwereade wrote:
> This is a problem. If we're constructing an environ, we've just used the
> secrets, even on an LXC provisioner.

Will be fixed in a follow-up that blanks the secrets, as discussed.

https://codereview.appspot.com/13720051/diff/1/worker/provisioner/provisioner.go
File worker/provisioner/provisioner.go (right):

https://codereview.appspot.com/13720051/diff/1/worker/provisioner/provisioner...
worker/provisioner/provisioner.go:101: watcher.Stop(environWatcher, &p.tomb)
On 2013/09/24 14:04:13, fwereade wrote:
> I think it's more suitable to do this explicitly here -- shouldn't continue if
> there's an error, but can't tell without inspecting the tomb, which is
tedious;
> so how about a plain old:
> 
> if err := environWatcher.Stop(); err != nil {
>     return err
> }
> 
> ...and tbh I think you could simplify the bit above:
> 
> defer watcher.Stop(environWatcher, &p.tomb)
> 
> ...in which case, eh, you'd kill the tomb twice with the same error if the
stop
> failed, but I don't think that'd be a practical concern at any stage.

Done.

https://codereview.appspot.com/13720051/diff/1/worker/provisioner/provisioner...
worker/provisioner/provisioner.go:123: provisionerTask := NewProvisionerTask(
On 2013/09/24 14:05:45, rog wrote:
> s/provisionerTask/task/ ?
> 
> This is all provisioner code after all, and the type
> makes it quite clear what kind of task this is.

Done.

https://codereview.appspot.com/13720051/diff/1/worker/provisioner/provisioner...
worker/provisioner/provisioner.go:131: return p.mainLoop(environWatcher,
provisionerTask)
On 2013/09/24 14:05:45, rog wrote:
> Rather than having one mainLoop function with
> the very large if statement, I'd be tempted to do:
> 
> if environWatcher != nil {
>     return environmentProvisionerMainLoop(environWatcher, provisionerTask)
> }
> return localProvisionerMainLoop(provisionerTask)

Done slightly differently - as fwereade suggested, I get the changes channel and
pass it here, so if it's nil the select won't use it.

https://codereview.appspot.com/13720051/diff/1/worker/provisioner/provisioner...
worker/provisioner/provisioner.go:135: if environWatcher != nil {
On 2013/09/24 14:04:13, fwereade wrote:
> Ah, and now I see why you did that. But you can simplify this method quite a
> lot, I think, if you accept the changes chan alone: if it's not an env
> provisioner you pass nil, and if it is you pass environWatcher.Changes(). Then
> you can drop the if/else and the duplication of the Dying-handling.
> 
> *really* the environment-watching ought to be entirely separate, I think. But
> that's not for today.

Done.

https://codereview.appspot.com/13720051/diff/1/worker/provisioner/provisioner...
worker/provisioner/provisioner.go:164: logger.Errorf("%s provisioner died: %v",
p.st, err)
On 2013/09/24 14:05:45, rog wrote:
> Using the state as a Stringer seems a little bit icky here.
> How about implementing String on ProvisionerType instead?

This is a mistake - it should've been p.pt actually. Good catch!

https://codereview.appspot.com/13720051/diff/1/worker/provisioner/provisioner...
File worker/provisioner/provisioner_task.go (right):

https://codereview.appspot.com/13720051/diff/1/worker/provisioner/provisioner...
worker/provisioner/provisioner_task.go:178: case params.IsCodeNotFound(err):
On 2013/09/24 14:04:13, fwereade wrote:
> On 2013/09/24 12:53:52, mue wrote:
> > "Code" here, "tag" used above, machine "id" in message. How is "code" meant
> > here? Isn't t a "tag not found"?
> 
> Code is the mechanism by which we communicate error types over the API,
> basically.
> 
> Id is here because... hmm. Why is id here? I guess we've got the watcher API
> producing ids instead of tags? That should be fixed, I think, even if we
convert
> straight back to ids for the actual nuts and bolts of the task for now.

Yes, the watchers return ids/names instead of tags. Added a tech-dept bug for
this: https://bugs.launchpad.net/juju-core/+bug/1229755

[fwereade, 2013-09-25 15:57:38]

LGTM with the following considered/addressed.

https://codereview.appspot.com/13720051/diff/10001/agent/agent.go
File agent/agent.go (right):

https://codereview.appspot.com/13720051/diff/10001/agent/agent.go#newcode316
agent/agent.go:316: return addresses
copy?

https://codereview.appspot.com/13720051/diff/10001/agent/agent.go#newcode321
agent/agent.go:321: return addresses
ditto

https://codereview.appspot.com/13720051/diff/10001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/13720051/diff/10001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:202: // Create jobs for container types, rather than
s/types/provisioners/

https://codereview.appspot.com/13720051/diff/10001/worker/provisioner/authent...
File worker/provisioner/authentication.go (right):

https://codereview.appspot.com/13720051/diff/10001/worker/provisioner/authent...
worker/provisioner/authentication.go:49: return &simpleAuth{stateInfo, apiInfo},
nil
Ha, I've just realised we did this wrong; but don't worry, no wronger than we
already were, and not wrong enough to block. Really we ought to be reusing the
deployer code for getting ca-cert/api-addrs, so we get up-to-date information;
that's probably a bug too.

https://codereview.appspot.com/13720051/diff/10001/worker/provisioner/provisi...
File worker/provisioner/provisioner.go (right):

https://codereview.appspot.com/13720051/diff/10001/worker/provisioner/provisi...
worker/provisioner/provisioner.go:70: machineTag:  agentConfig.Tag(),
we could even maybe drop this field since we've got agentConfig right there.

https://codereview.appspot.com/13720051/diff/10001/worker/provisioner/provisi...
worker/provisioner/provisioner.go:91: }()
change not applied?

https://codereview.appspot.com/13720051/diff/10001/worker/provisioner/provisi...
worker/provisioner/provisioner.go:102: environWatcher = nil
change not applied?

[dimitern, 2013-09-25 16:20:02]

Please take a look.

https://codereview.appspot.com/13720051/diff/10001/agent/agent.go
File agent/agent.go (right):

https://codereview.appspot.com/13720051/diff/10001/agent/agent.go#newcode316
agent/agent.go:316: return addresses
On 2013/09/25 15:57:38, fwereade wrote:
> copy?

Done.

https://codereview.appspot.com/13720051/diff/10001/agent/agent.go#newcode321
agent/agent.go:321: return addresses
On 2013/09/25 15:57:38, fwereade wrote:
> ditto

Done.

https://codereview.appspot.com/13720051/diff/10001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/13720051/diff/10001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:202: // Create jobs for container types, rather than
On 2013/09/25 15:57:38, fwereade wrote:
> s/types/provisioners/

Done.

https://codereview.appspot.com/13720051/diff/10001/worker/provisioner/authent...
File worker/provisioner/authentication.go (right):

https://codereview.appspot.com/13720051/diff/10001/worker/provisioner/authent...
worker/provisioner/authentication.go:49: return &simpleAuth{stateInfo, apiInfo},
nil
On 2013/09/25 15:57:38, fwereade wrote:
> Ha, I've just realised we did this wrong; but don't worry, no wronger than we
> already were, and not wrong enough to block. Really we ought to be reusing the
> deployer code for getting ca-cert/api-addrs, so we get up-to-date information;
> that's probably a bug too.

Will add a TODO then.

https://codereview.appspot.com/13720051/diff/10001/worker/provisioner/provisi...
File worker/provisioner/provisioner.go (right):

https://codereview.appspot.com/13720051/diff/10001/worker/provisioner/provisi...
worker/provisioner/provisioner.go:70: machineTag:  agentConfig.Tag(),
On 2013/09/25 15:57:38, fwereade wrote:
> we could even maybe drop this field since we've got agentConfig right there.

Done.

https://codereview.appspot.com/13720051/diff/10001/worker/provisioner/provisi...
worker/provisioner/provisioner.go:91: }()
On 2013/09/25 15:57:38, fwereade wrote:
> change not applied?

Sorry, forgot. Done.

https://codereview.appspot.com/13720051/diff/10001/worker/provisioner/provisi...
worker/provisioner/provisioner.go:102: environWatcher = nil
On 2013/09/25 15:57:38, fwereade wrote:
> change not applied?

Ditto.

[dimitern, 2013-09-25 17:43:49]

Please take a look.