api: Use MachineNonce when opening API as MA

api: Use MachineNonce when opening API as MA

This introduces an extra field in api.Info:
MachineNonce, which needs to be set when
opening/logging in into the API server as
a machine agent (ignored in other cases).
The server ensures the connecting machine
agents won't operate on a wrong or unprovisioned
machine (in the latter case they shouldn't
run anyway and now they stop immediately after
starting).

Added tests for the behavior in apiserver.
It might seem there are a lot of changes,
but that's because it touches one of the
basic types api.Info and how API connections
are established.

These changes will be followed up by another
branch which starts and stops a pinger at
the apiserver automatically when a machine
agent connects successfully, so that we can
get rid of SetAgentAlive eventually.

https://code.launchpad.net/~dimitern/juju-core/072-machineagent-login-nonce/+merge/175791

(do not edit description out of merge proposal)

[dimitern, 2013-07-19 10:16:04]

Please take a look.

[rog, 2013-07-19 11:20:29]

LGTM with a few suggestions below

https://codereview.appspot.com/11424044/diff/1/agent/agent.go
File agent/agent.go (right):

https://codereview.appspot.com/11424044/diff/1/agent/agent.go#newcode214
agent/agent.go:214: if c.MachineNonce != "" && info.MachineNonce == "" {
i think just:

    c.APIInfo.MachineNonce = info.MachineNonce

(without the if) should be ok here. if we were doing this without
backwards compatibility issues, we'd only have
one field, and i can't think of a case where we'd
want the two to be different.

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

https://codereview.appspot.com/11424044/diff/1/cmd/jujud/agent.go#newcode156
cmd/jujud/agent.go:156: params.ErrCode(err) == params.CodeNotProvisioned ||
i think this might be better done inside isFatal.

https://codereview.appspot.com/11424044/diff/1/state/api/state.go
File state/api/state.go (right):

https://codereview.appspot.com/11424044/diff/1/state/api/state.go#newcode16
state/api/state.go:16: // This method is usually called automatically by Open.
// The machine nonce should be empty unless logging
// in as a machine agent.

?

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

https://codereview.appspot.com/11424044/diff/1/state/apiserver/admin.go#newco...
state/apiserver/admin.go:81: // We have authenticated the user, now choose an
appropriate API
the semicolon was correct here.

https://codereview.appspot.com/11424044/diff/1/state/apiserver/admin.go#newco...
state/apiserver/admin.go:91: newRootAsRoot := newRoot.(*srvRoot)
how about:

if m, ok := entity.(*state.Machine); ok && !m.CheckProvisioned(c.MachineNonce) {
    return common.ErrNotProvisioned
}

instead of all this?

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

https://codereview.appspot.com/11424044/diff/1/state/apiserver/pinger_test.go...
state/apiserver/pinger_test.go:36: st := s.OpenAPIAsMachine(c, stm.Tag(),
"password", "fake_nonce")
i think  we should have a constant for "fake_nonce" - it's used as magic in
quite a few places, and it's not just a placeholder - it needs to be that value.

[dimitern, 2013-07-19 13:33:07]

Please take a look.

https://codereview.appspot.com/11424044/diff/1/agent/agent.go
File agent/agent.go (right):

https://codereview.appspot.com/11424044/diff/1/agent/agent.go#newcode214
agent/agent.go:214: if c.MachineNonce != "" && info.MachineNonce == "" {
On 2013/07/19 11:20:29, rog wrote:
> i think just:
> 
>     c.APIInfo.MachineNonce = info.MachineNonce
> 
> (without the if) should be ok here. if we were doing this without
> backwards compatibility issues, we'd only have
> one field, and i can't think of a case where we'd
> want the two to be different.

You mean info.MachineNonce = c.MachineNonce, right? Done.

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

https://codereview.appspot.com/11424044/diff/1/cmd/jujud/agent.go#newcode156
cmd/jujud/agent.go:156: params.ErrCode(err) == params.CodeNotProvisioned ||
On 2013/07/19 11:20:29, rog wrote:
> i think this might be better done inside isFatal.

Done.

https://codereview.appspot.com/11424044/diff/1/state/api/state.go
File state/api/state.go (right):

https://codereview.appspot.com/11424044/diff/1/state/api/state.go#newcode16
state/api/state.go:16: // This method is usually called automatically by Open.
On 2013/07/19 11:20:29, rog wrote:
> // The machine nonce should be empty unless logging
> // in as a machine agent.
> 
> ?

Done.

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

https://codereview.appspot.com/11424044/diff/1/state/apiserver/admin.go#newco...
state/apiserver/admin.go:81: // We have authenticated the user, now choose an
appropriate API
On 2013/07/19 11:20:29, rog wrote:
> the semicolon was correct here.

Done.

https://codereview.appspot.com/11424044/diff/1/state/apiserver/admin.go#newco...
state/apiserver/admin.go:91: newRootAsRoot := newRoot.(*srvRoot)
On 2013/07/19 11:20:29, rog wrote:
> how about:
> 
> if m, ok := entity.(*state.Machine); ok && !m.CheckProvisioned(c.MachineNonce)
{
>     return common.ErrNotProvisioned
> }
> 
> instead of all this?
> 

Done.

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

https://codereview.appspot.com/11424044/diff/1/state/apiserver/pinger_test.go...
state/apiserver/pinger_test.go:36: st := s.OpenAPIAsMachine(c, stm.Tag(),
"password", "fake_nonce")
On 2013/07/19 11:20:29, rog wrote:
> i think  we should have a constant for "fake_nonce" - it's used as magic in
> quite a few places, and it's not just a placeholder - it needs to be that
value.

Well, it's not the only magic string like that: arguably "series" is used a lot
more.

[rog, 2013-07-19 14:01:16]

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

https://codereview.appspot.com/11424044/diff/1/state/apiserver/pinger_test.go...
state/apiserver/pinger_test.go:36: st := s.OpenAPIAsMachine(c, stm.Tag(),
"password", "fake_nonce")
On 2013/07/19 13:33:08, dimitern wrote:
> On 2013/07/19 11:20:29, rog wrote:
> > i think  we should have a constant for "fake_nonce" - it's used as magic in
> > quite a few places, and it's not just a placeholder - it needs to be that
> value.
> 
> Well, it's not the only magic string like that: arguably "series" is used a
lot
> more.

perhaps use state.BootstrapNonce throughout?

[dimitern, 2013-07-19 14:11:34]

On 2013/07/19 14:01:16, rog wrote:
> https://codereview.appspot.com/11424044/diff/1/state/apiserver/pinger_test.go
> File state/apiserver/pinger_test.go (right):
> 
>
https://codereview.appspot.com/11424044/diff/1/state/apiserver/pinger_test.go...
> state/apiserver/pinger_test.go:36: st := s.OpenAPIAsMachine(c, stm.Tag(),
> "password", "fake_nonce")
> On 2013/07/19 13:33:08, dimitern wrote:
> > On 2013/07/19 11:20:29, rog wrote:
> > > i think  we should have a constant for "fake_nonce" - it's used as magic
in
> > > quite a few places, and it's not just a placeholder - it needs to be that
> > value.
> > 
> > Well, it's not the only magic string like that: arguably "series" is used a
> lot
> > more.
> 
> perhaps use state.BootstrapNonce throughout?

This is used only for the bootstrap nodes. In all other cases it is generated
uniquely. I'd rather not change this now if you don't mind.

[fwereade, 2013-07-19 16:25:03]

LGTM in principle, with a couple of vague thoughts (below) and one more
suggestion: how about calling the APIInfo param just "Nonce"? I recall having
some thought that led me to believe that we might have cause to follow the same
model for units one day, and that would render the name misleading.

Also, might be nice to omitempty it.

https://codereview.appspot.com/11424044/diff/9001/cmd/jujud/agent.go
File cmd/jujud/agent.go (right):

https://codereview.appspot.com/11424044/diff/9001/cmd/jujud/agent.go#newcode111
cmd/jujud/agent.go:111: isUpgraded(err) {
This is a bit weirdly formatted. Might look nicer expanded a little bit into a
switch or if/else if/else if? YMMV

https://codereview.appspot.com/11424044/diff/9001/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/11424044/diff/9001/cmd/jujud/machine_test.go#n...
cmd/jujud/machine_test.go:74: conf.APIInfo.MachineNonce = conf.MachineNonce
This conf stuff is all tangly :/. Not much to be done this CL though, I guess.

https://codereview.appspot.com/11424044/diff/9001/environs/cloudinit/cloudini...
File environs/cloudinit/cloudinit_test.go (right):

https://codereview.appspot.com/11424044/diff/9001/environs/cloudinit/cloudini...
environs/cloudinit/cloudinit_test.go:151: echo 'datadir:
/var/lib/juju\\nstateservercert:\\n[^']+stateserverkey:\\n[^']+stateport:
37017\\napiport: 17070\\noldpassword: arble\\nmachinenonce:
FAKE_NONCE\\nstateinfo:\\n  addrs:\\n  - localhost:37017\\n  cacert:\\n[^']+ 
tag: machine-0\\n  password: ""\\noldapipassword: ""\\napiinfo:\\n  addrs:\\n  -
localhost:17070\\n  cacert:\\n[^']+  tag: machine-0\\n  password: ""\\n 
machinenonce: ""\\n' > '/var/lib/juju/agents/machine-0/agent\.conf'
This doesn't look right. Is anything using the nonce apart from the api info
now?

...oh blast. Compatibility. Carry on :).

But maybe we should put it directly into the api info all the same, even if we
can't drop it from the top level?

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

https://codereview.appspot.com/11424044/diff/9001/state/api/params/apierror.g...
state/api/params/apierror.go:45: CodeNotProvisioned      = "not provisioned"
This isn't *quite* the right error, I think, but I can't think of any clear
improvements.

[dimitern, 2013-07-19 17:21:13]

Please take a look.

https://codereview.appspot.com/11424044/diff/9001/cmd/jujud/agent.go
File cmd/jujud/agent.go (right):

https://codereview.appspot.com/11424044/diff/9001/cmd/jujud/agent.go#newcode111
cmd/jujud/agent.go:111: isUpgraded(err) {
On 2013/07/19 16:25:03, fwereade wrote:
> This is a bit weirdly formatted. Might look nicer expanded a little bit into a
> switch or if/else if/else if? YMMV

Done.

https://codereview.appspot.com/11424044/diff/9001/environs/cloudinit/cloudini...
File environs/cloudinit/cloudinit_test.go (right):

https://codereview.appspot.com/11424044/diff/9001/environs/cloudinit/cloudini...
environs/cloudinit/cloudinit_test.go:151: echo 'datadir:
/var/lib/juju\\nstateservercert:\\n[^']+stateserverkey:\\n[^']+stateport:
37017\\napiport: 17070\\noldpassword: arble\\nmachinenonce:
FAKE_NONCE\\nstateinfo:\\n  addrs:\\n  - localhost:37017\\n  cacert:\\n[^']+ 
tag: machine-0\\n  password: ""\\noldapipassword: ""\\napiinfo:\\n  addrs:\\n  -
localhost:17070\\n  cacert:\\n[^']+  tag: machine-0\\n  password: ""\\n 
machinenonce: ""\\n' > '/var/lib/juju/agents/machine-0/agent\.conf'
On 2013/07/19 16:25:03, fwereade wrote:
> This doesn't look right. Is anything using the nonce apart from the api info
> now?
> 
> ...oh blast. Compatibility. Carry on :).
> 
> But maybe we should put it directly into the api info all the same, even if we
> can't drop it from the top level?

It is in the APIInfo. And the top-level is needed by the provisioner . Once it's
using only the API I guess we can remove it from top-level.