cmd/jujud; others: Disable setting mongo passwds

cmd/jujud; others: Disable setting mongo passwds

This disables settings mongo passwords for agents.
The unit agent is totally cut off from state now,
not only it doesn't connect, but it doesn't have
a password do to so. The machine agent is a bit
more complicated: only machines with JobHostUnits
won't have a mongo password or connection to state.
Due to firewaller not using the API yet, JobManageEnviron
also has to have state connection (and mongo password)
for now.

We don't do anything yet to reset existing passwords
in mongo, but this will come in the following releases.

https://code.launchpad.net/~dimitern/juju-core/149-disable-mongo-passwords-for-api-agents/+merge/188118

(do not edit description out of merge proposal)

[dimitern, 2013-09-27 17:41:50]

Please take a look.

[dimitern, 2013-09-27 17:45:16]

Please take a look.

[jameinel, 2013-09-29 05:31:54]

Really happy this is landing. Great work overall. I have some thoughts, but
nothing that would block landing.

LGTM

https://codereview.appspot.com/14036045/diff/4001/agent/agent.go
File agent/agent.go (right):

https://codereview.appspot.com/14036045/diff/4001/agent/agent.go#newcode356
agent/agent.go:356: func (c *configInternal) Password() string {
I was a little hesitant about this, but I'm more surprised to see that it is
only used by the test suite.

I'm wondering if we'd have a better way to expose it there, so people aren't
tempted to be accessing the password and keeping it around.

But for now, good enough, I guess.

https://codereview.appspot.com/14036045/diff/4001/agent/agent_test.go
File agent/agent_test.go (right):

https://codereview.appspot.com/14036045/diff/4001/agent/agent_test.go#newcode249
agent/agent_test.go:249: c.Assert(utils.PasswordHash(conf.Password()),
gc.Equals, utils.PasswordHash(reread.Password()))
It seems odd to has the password 2 times here to do the comparison. Why not just
compare:
conf.Password(), gc.Equals, reread.Password()?

https://codereview.appspot.com/14036045/diff/4001/cmd/jujud/machine.go
File cmd/jujud/machine.go (left):

https://codereview.appspot.com/14036045/diff/4001/cmd/jujud/machine.go#oldcod...
cmd/jujud/machine.go:177: needsStateWorker = needsStateWorker || stateJobs[job]
Why use a hardcoded if statement here, rather than the state jobs we already
had. I think all you had to do was remove the "JobHostUnits" from the earlier
map.

https://codereview.appspot.com/14036045/diff/4001/cmd/jujud/unit_test.go
File cmd/jujud/unit_test.go (right):

https://codereview.appspot.com/14036045/diff/4001/cmd/jujud/unit_test.go#newc...
cmd/jujud/unit_test.go:195: s.tryOpenState(c, unit.Tag(),
a.Conf.config.Password(), false)
Do you think it might be easier to follow if the functions were split?
So you would have:

s.assertCannotOpenState()
and
s.assertCanOpenState()

rather than a 'false' somewhere in the parameter list to distinguish between
them.

https://codereview.appspot.com/14036045/diff/4001/state/apiserver/common/pass...
File state/apiserver/common/password.go (right):

https://codereview.appspot.com/14036045/diff/4001/state/apiserver/common/pass...
state/apiserver/common/password.go:91: }
Given this is the third place where we have to have the caveat TODO and the
logic of whether or not it is allowed, is it possible to pull all this into a
separate function that we can share?

[dimitern, 2013-09-30 09:08:22]

Please take a look.

https://codereview.appspot.com/14036045/diff/4001/agent/agent.go
File agent/agent.go (right):

https://codereview.appspot.com/14036045/diff/4001/agent/agent.go#newcode356
agent/agent.go:356: func (c *configInternal) Password() string {
On 2013/09/29 05:31:54, jameinel wrote:
> I was a little hesitant about this, but I'm more surprised to see that it is
> only used by the test suite.
> 
> I'm wondering if we'd have a better way to expose it there, so people aren't
> tempted to be accessing the password and keeping it around.
> 
> But for now, good enough, I guess.

I was thinking of a way to do that, possibly exposing the password only for
tests, but couldn't find a suitable one.

https://codereview.appspot.com/14036045/diff/4001/agent/agent_test.go
File agent/agent_test.go (right):

https://codereview.appspot.com/14036045/diff/4001/agent/agent_test.go#newcode249
agent/agent_test.go:249: c.Assert(utils.PasswordHash(conf.Password()),
gc.Equals, utils.PasswordHash(reread.Password()))
On 2013/09/29 05:31:54, jameinel wrote:
> It seems odd to has the password 2 times here to do the comparison. Why not
just
> compare:
> conf.Password(), gc.Equals, reread.Password()?

Done.

https://codereview.appspot.com/14036045/diff/4001/cmd/jujud/machine.go
File cmd/jujud/machine.go (left):

https://codereview.appspot.com/14036045/diff/4001/cmd/jujud/machine.go#oldcod...
cmd/jujud/machine.go:177: needsStateWorker = needsStateWorker || stateJobs[job]
On 2013/09/29 05:31:54, jameinel wrote:
> Why use a hardcoded if statement here, rather than the state jobs we already
> had. I think all you had to do was remove the "JobHostUnits" from the earlier
> map.

Added a method agentEntity.ShouldAccessState(), to move all checks in one place,
except for apiserver/common/password.go, where we have the state machine, so I
added the same call to state.Machine as well.

https://codereview.appspot.com/14036045/diff/4001/cmd/jujud/unit_test.go
File cmd/jujud/unit_test.go (right):

https://codereview.appspot.com/14036045/diff/4001/cmd/jujud/unit_test.go#newc...
cmd/jujud/unit_test.go:195: s.tryOpenState(c, unit.Tag(),
a.Conf.config.Password(), false)
On 2013/09/29 05:31:54, jameinel wrote:
> Do you think it might be easier to follow if the functions were split?
> So you would have:
> 
> s.assertCannotOpenState()
> and
> s.assertCanOpenState()
> 
> rather than a 'false' somewhere in the parameter list to distinguish between
> them.

Done.

https://codereview.appspot.com/14036045/diff/4001/state/apiserver/common/pass...
File state/apiserver/common/password.go (right):

https://codereview.appspot.com/14036045/diff/4001/state/apiserver/common/pass...
state/apiserver/common/password.go:91: }
On 2013/09/29 05:31:54, jameinel wrote:
> Given this is the third place where we have to have the caveat TODO and the
> logic of whether or not it is allowed, is it possible to pull all this into a
> separate function that we can share?

Added agentEntity.ShouldAccessState() method. We cannot easily have the
functionality in a single place, because the agent API doesn't provide
machine-specific API calls, but has jobs, which we can use there. And the
PasswordChanger doesn't use the agent API, but has an actual state.Machine
instance (after a type assertion), so it can call its methods.

[fwereade, 2013-10-01 07:42:08]

A few minor comments, and an overwhelming stress that jujud seems to be more
complex and entangled after this than it was before. The Password method in
particular STM to be a complex and confusing way of looking like we're testing
things we're not actually testing at all... we should probably talk some more
about this.

https://codereview.appspot.com/14036045/diff/12001/agent/agent.go
File agent/agent.go (right):

https://codereview.appspot.com/14036045/diff/12001/agent/agent.go#newcode132
agent/agent.go:132: newPassword     string
as discussed, I don't think this is ok -- it looks like real code.

https://codereview.appspot.com/14036045/diff/12001/agent/agent.go#newcode356
agent/agent.go:356: func (c *configInternal) Password() string {
as does this -- surely we can test this by checking the actual password we write
out instead of writing all this fake code?

https://codereview.appspot.com/14036045/diff/12001/worker/provisioner/provisi...
File worker/provisioner/provisioner_test.go (left):

https://codereview.appspot.com/14036045/diff/12001/worker/provisioner/provisi...
worker/provisioner/provisioner_test.go:167: c.Assert(err, gc.IsNil)
Why did we drop this?

https://codereview.appspot.com/14036045/diff/12001/worker/provisioner/provisi...
worker/provisioner/provisioner_test.go:180: st.Close()
/me raises eyebrow

[dimitern, 2013-10-01 08:25:58]

Please take a look.

[dimitern, 2013-10-01 08:48:55]

Please take a look.

https://codereview.appspot.com/14036045/diff/12001/agent/agent.go
File agent/agent.go (right):

https://codereview.appspot.com/14036045/diff/12001/agent/agent.go#newcode132
agent/agent.go:132: newPassword     string
On 2013/10/01 07:42:08, fwereade wrote:
> as discussed, I don't think this is ok -- it looks like real code.

Removed.

https://codereview.appspot.com/14036045/diff/12001/agent/agent.go#newcode356
agent/agent.go:356: func (c *configInternal) Password() string {
On 2013/10/01 07:42:08, fwereade wrote:
> as does this -- surely we can test this by checking the actual password we
write
> out instead of writing all this fake code?

As discussed, unexported and added export_test for the internal tests. In
cmd/jujud tests I just reread the config and use OpenState and OpenAPI directly.

https://codereview.appspot.com/14036045/diff/12001/worker/provisioner/provisi...
File worker/provisioner/provisioner_test.go (left):

https://codereview.appspot.com/14036045/diff/12001/worker/provisioner/provisi...
worker/provisioner/provisioner_test.go:167: c.Assert(err, gc.IsNil)
On 2013/10/01 07:42:08, fwereade wrote:
> Why did we drop this?

What we are testing here no longer makes sense for the provisioner, since it's
using the API and never connects to state. And it's kinda related to the changes
in this CL, so it's a bit of a clean up.

https://codereview.appspot.com/14036045/diff/12001/worker/provisioner/provisi...
worker/provisioner/provisioner_test.go:180: st.Close()
On 2013/10/01 07:42:08, fwereade wrote:
> /me raises eyebrow

Removed st from above, so this is no longer needed.

[dimitern, 2013-10-01 08:53:21]

Please take a look.

[fwereade, 2013-10-01 09:09:35]

LGTM, thanks very much :).

Please do try to break this live before you land it ;p

https://codereview.appspot.com/14036045/diff/38001/agent/export_test.go
File agent/export_test.go (right):

https://codereview.appspot.com/14036045/diff/38001/agent/export_test.go#newcode8
agent/export_test.go:8: return confInternal.password()
you know, we could even move the body of that method in here

[dimitern, 2013-10-01 10:48:38]

Please take a look.

https://codereview.appspot.com/14036045/diff/38001/agent/export_test.go
File agent/export_test.go (right):

https://codereview.appspot.com/14036045/diff/38001/agent/export_test.go#newcode8
agent/export_test.go:8: return confInternal.password()
On 2013/10/01 09:09:35, fwereade wrote:
> you know, we could even move the body of that method in here

Done.