juju-core

Merge lp://staging/~rogpeppe/juju-core/311-juju-bootstrap-state-change-password-1.5 into lp://staging/~juju/juju-core/trunk

  1. 311-juju-bootstrap-state-change-password-1.5
  2. Merge into trunk
Proposed by Roger Peppe
Status: Work in progress
Proposed branch: lp://staging/~rogpeppe/juju-core/311-juju-bootstrap-state-change-password-1.5
Merge into: lp://staging/~juju/juju-core/trunk
Diff against target: 14443 lines (+5882/-2363)
148 files modified
.lbox.check (+1/-1)
charm/charm_test.go (+0/-1)
charm/config_test.go (+6/-6)
charm/repo.go (+7/-1)
charm/repo_test.go (+10/-0)
cmd/juju/bootstrap_test.go (+5/-1)
cmd/juju/plugin_test.go (+3/-2)
cmd/juju/ssh.go (+2/-1)
cmd/juju/ssh_test.go (+2/-1)
cmd/juju/status.go (+88/-45)
cmd/juju/status_test.go (+97/-0)
cmd/juju/synctools.go (+102/-6)
cmd/juju/synctools_test.go (+127/-22)
cmd/jujud/agent.go (+5/-35)
cmd/jujud/agent_test.go (+6/-5)
cmd/jujud/bootstrap.go (+57/-28)
cmd/jujud/bootstrap_test.go (+101/-40)
cmd/jujud/machine.go (+2/-2)
cmd/jujud/machine_test.go (+31/-12)
cmd/jujud/unit_test.go (+1/-0)
environs/agent/agent.go (+24/-7)
environs/agent/agent_test.go (+60/-22)
environs/azure/azure_test.go (+26/-0)
environs/azure/certfile.go (+92/-0)
environs/azure/certfile_test.go (+66/-0)
environs/azure/config.go (+144/-0)
environs/azure/config_test.go (+199/-0)
environs/azure/environ.go (+112/-17)
environs/azure/environ_test.go (+104/-0)
environs/azure/environprovider.go (+2/-17)
environs/azure/instance.go (+11/-8)
environs/cloudinit/cloudinit.go (+11/-1)
environs/cloudinit/cloudinit_test.go (+42/-0)
environs/config/config.go (+23/-12)
environs/config/config_test.go (+70/-97)
environs/dummy/environs.go (+70/-38)
environs/dummy/storage.go (+6/-1)
environs/ec2/ec2.go (+74/-50)
environs/ec2/export_test.go (+13/-8)
environs/ec2/image_test.go (+1/-1)
environs/ec2/live_test.go (+11/-11)
environs/ec2/local_test.go (+20/-2)
environs/ec2/state.go (+2/-2)
environs/instances/image.go (+3/-4)
environs/instances/image_test.go (+4/-7)
environs/interface.go (+10/-40)
environs/jujutest/livetests.go (+59/-43)
environs/jujutest/tests.go (+9/-4)
environs/maas/environ.go (+23/-19)
environs/maas/environ_test.go (+12/-12)
environs/maas/environprovider.go (+3/-3)
environs/maas/environprovider_test.go (+2/-2)
environs/maas/instance.go (+20/-17)
environs/maas/state.go (+2/-2)
environs/maas/util.go (+3/-3)
environs/maas/util_test.go (+5/-4)
environs/openstack/export_test.go (+2/-2)
environs/openstack/local_test.go (+59/-48)
environs/openstack/provider.go (+76/-50)
environs/openstack/provider_test.go (+4/-4)
environs/openstack/state.go (+2/-2)
environs/storage.go (+21/-0)
environs/storage_test.go (+45/-0)
instance/instance.go (+65/-0)
juju/api.go (+2/-2)
juju/testing/conn.go (+30/-9)
rpc/jsoncodec/codec.go (+31/-17)
rpc/jsoncodec/codec_test.go (+104/-10)
rpc/jsoncodec/conn.go (+2/-3)
rpc/rpc_test.go (+119/-15)
rpc/server.go (+53/-37)
state/annotator.go (+1/-1)
state/api/apiclient.go (+26/-1)
state/api/machineagent/state.go (+101/-0)
state/api/machineagent/state_test.go (+125/-0)
state/api/machiner/machine.go (+6/-6)
state/api/machiner/machiner.go (+11/-10)
state/api/machiner/machiner_test.go (+12/-52)
state/api/params/params.go (+44/-11)
state/api/params/params_test.go (+2/-1)
state/api/state.go (+11/-13)
state/apiserver/api_test.go (+4/-3)
state/apiserver/apiserver.go (+6/-1)
state/apiserver/common/export_test.go (+11/-0)
state/apiserver/common/interfaces.go (+1/-1)
state/apiserver/common/password.go (+63/-0)
state/apiserver/common/password_test.go (+114/-0)
state/apiserver/login_test.go (+7/-6)
state/apiserver/machine/agent.go (+74/-0)
state/apiserver/machine/agent_test.go (+91/-0)
state/apiserver/machine/common_test.go (+66/-0)
state/apiserver/machine/machiner.go (+12/-12)
state/apiserver/machine/machiner_test.go (+9/-61)
state/apiserver/root.go (+22/-13)
state/apiserver/server_test.go (+5/-3)
state/conn_test.go (+1/-1)
state/constraints.go (+1/-1)
state/container.go (+15/-3)
state/export_test.go (+63/-13)
state/initialize_test.go (+4/-4)
state/life_test.go (+1/-1)
state/machine.go (+30/-22)
state/machine_test.go (+148/-159)
state/megawatcher_internal_test.go (+7/-6)
state/open.go (+3/-1)
state/relation.go (+3/-12)
state/relationunit.go (+5/-2)
state/relationunit_test.go (+10/-17)
state/service.go (+26/-18)
state/service_test.go (+175/-383)
state/settings.go (+2/-2)
state/state.go (+163/-46)
state/state_test.go (+234/-168)
state/tools_test.go (+1/-1)
state/unit.go (+87/-76)
state/unit_test.go (+201/-194)
state/user.go (+2/-2)
state/watcher.go (+28/-16)
state/watcher/watcher.go (+16/-4)
state/watcher/watcher_test.go (+16/-0)
state/watcher_test.go (+163/-0)
testing/checkers/bool.go (+35/-0)
testing/checkers/bool_test.go (+35/-0)
testing/checkers/checker_test.go (+27/-0)
testing/checkers/file.go (+98/-0)
testing/checkers/file_test.go (+97/-0)
testing/checkers/relop.go (+52/-0)
testing/checkers/relop_test.go (+24/-0)
testing/environ.go (+16/-0)
testing/locking.go (+71/-0)
testing/locking_test.go (+45/-0)
testing/mgo.go (+6/-1)
utils/password.go (+11/-1)
worker/deployer/deployer_test.go (+8/-0)
worker/firewaller/firewaller.go (+20/-20)
worker/firewaller/firewaller_test.go (+36/-37)
worker/provisioner/broker.go (+4/-4)
worker/provisioner/provisioner_task.go (+10/-10)
worker/provisioner/provisioner_test.go (+5/-5)
worker/resumer/export_test.go (+16/-0)
worker/resumer/resumer.go (+71/-0)
worker/resumer/resumer_test.go (+57/-0)
worker/runner.go (+75/-50)
worker/runner_test.go (+109/-9)
worker/uniter/filter.go (+51/-5)
worker/uniter/filter_test.go (+39/-10)
worker/uniter/modes.go (+2/-2)
worker/uniter/uniter_test.go (+27/-0)
To merge this branch: bzr merge lp://staging/~rogpeppe/juju-core/311-juju-bootstrap-state-change-password-1.5
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+165675@code.staging.launchpad.net

Description of the change

cmd/jujud: do not change password

This prepares the way for the password to be changed
when connecting to the API instead of the state.

We also change bootstrap-state to change the machine
agent's password, so the bootstrap machine agent
won't *need* to change its password, and hence won't
need a connection to the at-that-time-nonexistent
API server.

Until agents are actually connecting to the API,
juju will be a tiny bit less secure, so I plan on
waiting until that's ready to land before landing this
branch.

https://codereview.appspot.com/9738043/

To post a comment you must log in.
Revision history for this message
Roger Peppe (rogpeppe) wrote :

Reviewers: mp+165675_code.launchpad.net,

Message:
Please take a look.

Description:
cmd/jujud: do not change password

This prepares the way for the password to be changed
when connecting to the API instead of the state.

We also change bootstrap-state to change the machine
agent's password, so the bootstrap machine agent
won't *need* to change its password, and hence won't
need a connection to the at-that-time-nonexistent
API server.

Until agents are actually connecting to the API,
juju will be a tiny bit less secure, so I plan on
waiting until that's ready to land before landing this
branch.

https://code.launchpad.net/~rogpeppe/juju-core/311-juju-bootstrap-state-change-password-1.5/+merge/165675

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/9738043/

Affected files:
   A [revision details]
   M cmd/juju/bootstrap_test.go
   M cmd/jujud/agent.go
   M cmd/jujud/agent_test.go
   M cmd/jujud/bootstrap.go
   M cmd/jujud/bootstrap_test.go
   M cmd/jujud/machine_test.go
   M cmd/jujud/unit_test.go
   M environs/agent/agent.go
   M environs/agent/agent_test.go
   M environs/cloudinit/cloudinit.go
   M environs/dummy/environs.go
   M juju/testing/conn.go

Reply
Revision history for this message
Dimiter Naydenov (dimitern) wrote :

LGTM, nice!

https://codereview.appspot.com/9738043/diff/5001/cmd/juju/bootstrap_test.go
File cmd/juju/bootstrap_test.go (right):

https://codereview.appspot.com/9738043/diff/5001/cmd/juju/bootstrap_test.go#newcode58
cmd/juju/bootstrap_test.go:58: func (s *BootstrapSuite) TestTest(c *C) {
TestTest :)

https://codereview.appspot.com/9738043/

Reply
Revision history for this message
William Reade (fwereade) wrote :

reserving judgment for now, until I see the rest of the pipeline, but I
have suspicions.

https://codereview.appspot.com/9738043/diff/5001/cmd/jujud/bootstrap.go
File cmd/jujud/bootstrap.go (right):

https://codereview.appspot.com/9738043/diff/5001/cmd/jujud/bootstrap.go#newcode140
cmd/jujud/bootstrap.go:140: }
I guess if we fail here, we have a borked environment regardless. So,
given the failure modes we've already embraced, ok, I guess... but it's
a bit surprising nonetheless. Comment perhaps?

https://codereview.appspot.com/9738043/diff/5001/cmd/jujud/bootstrap_test.go
File cmd/jujud/bootstrap_test.go (right):

https://codereview.appspot.com/9738043/diff/5001/cmd/jujud/bootstrap_test.go#newcode198
cmd/jujud/bootstrap_test.go:198: info.Tag, info.Password = "",
testPasswordHash
wait, why can user-admin log in as ""?

https://codereview.appspot.com/9738043/diff/5001/cmd/jujud/bootstrap_test.go#newcode220
cmd/jujud/bootstrap_test.go:220:
c.Assert(machineConf1.StateInfo.Password, Equals,
machineConf1.APIInfo.Password)
Are we duplicating tag/password for the same entities everywhere? If so,
why?

https://codereview.appspot.com/9738043/diff/5001/environs/agent/agent.go
File environs/agent/agent.go (right):

https://codereview.appspot.com/9738043/diff/5001/environs/agent/agent.go#newcode208
environs/agent/agent.go:208: func (c *Conf) OpenAPI() (st *api.State,
newPassword string, err error) {
Why are we duplicating this back-to-front newPassword-juggling
behaviour?

https://codereview.appspot.com/9738043/diff/5001/environs/dummy/environs.go
File environs/dummy/environs.go (left):

https://codereview.appspot.com/9738043/diff/5001/environs/dummy/environs.go#oldcode465
environs/dummy/environs.go:465: // logic is done.
"TODO: this whole test-only parallel implementation of bootstrap-state
is an open invitation to bugs and insanity", perhaps?

https://codereview.appspot.com/9738043/

Reply
Revision history for this message
Roger Peppe (rogpeppe) wrote :

Please take a look.

https://codereview.appspot.com/9738043/diff/5001/cmd/jujud/bootstrap.go
File cmd/jujud/bootstrap.go (right):

https://codereview.appspot.com/9738043/diff/5001/cmd/jujud/bootstrap.go#newcode140
cmd/jujud/bootstrap.go:140: }
On 2013/05/27 20:13:26, fwereade wrote:
> I guess if we fail here, we have a borked environment regardless. So,
 given the
> failure modes we've already embraced, ok, I guess... but it's a bit
 surprising
> nonetheless. Comment perhaps?

i'm not sure what kind of comment you're thinking here.
how is failing to write the configuration any different
from any of the above failure modes?

https://codereview.appspot.com/9738043/diff/5001/cmd/jujud/bootstrap_test.go
File cmd/jujud/bootstrap_test.go (right):

https://codereview.appspot.com/9738043/diff/5001/cmd/jujud/bootstrap_test.go#newcode198
cmd/jujud/bootstrap_test.go:198: info.Tag, info.Password = "",
testPasswordHash
On 2013/05/27 20:13:26, fwereade wrote:
> wait, why can user-admin log in as ""?

the empty string means "admin" when logging into the state. see the docs
on state.Info.Tag. i disclaim all responsibility for that decision :-)

https://codereview.appspot.com/9738043/diff/5001/cmd/jujud/bootstrap_test.go#newcode220
cmd/jujud/bootstrap_test.go:220:
c.Assert(machineConf1.StateInfo.Password, Equals,
machineConf1.APIInfo.Password)
On 2013/05/27 20:13:26, fwereade wrote:
> Are we duplicating tag/password for the same entities everywhere? If
 so, why?

we use the same password for the state that we do for the API.
is that bad?

https://codereview.appspot.com/9738043/diff/5001/environs/agent/agent.go
File environs/agent/agent.go (right):

https://codereview.appspot.com/9738043/diff/5001/environs/agent/agent.go#newcode208
environs/agent/agent.go:208: func (c *Conf) OpenAPI() (st *api.State,
newPassword string, err error) {
On 2013/05/27 20:13:26, fwereade wrote:
> Why are we duplicating this back-to-front newPassword-juggling
 behaviour?

what do you suggest instead? we still need to do exactly the same thing
AFAICS. i wasn't thinking of redesigning this logic at this point, but i
can do if you'd like it to change.

https://codereview.appspot.com/9738043/diff/5001/environs/dummy/environs.go
File environs/dummy/environs.go (left):

https://codereview.appspot.com/9738043/diff/5001/environs/dummy/environs.go#oldcode465
environs/dummy/environs.go:465: // logic is done.
On 2013/05/27 20:13:26, fwereade wrote:
> "TODO: this whole test-only parallel implementation of bootstrap-state
 is an
> open invitation to bugs and insanity", perhaps?

that's not really a TODO. i've added something slightly different.

https://codereview.appspot.com/9738043/

Reply
Revision history for this message
William Reade (fwereade) wrote :
Download full text (3.3 KiB)

LGTM; I do find all this stuff a bit tricky to follow, though, and I
feel like there's a simpler model waiting to come out. Let's wait and
see if one emerges when we're actually using the API.

https://codereview.appspot.com/9738043/diff/5001/cmd/jujud/bootstrap.go
File cmd/jujud/bootstrap.go (right):

https://codereview.appspot.com/9738043/diff/5001/cmd/jujud/bootstrap.go#newcode140
cmd/jujud/bootstrap.go:140: }
On 2013/06/07 17:01:55, rog wrote:
> On 2013/05/27 20:13:26, fwereade wrote:
> > I guess if we fail here, we have a borked environment regardless.
 So, given
> the
> > failure modes we've already embraced, ok, I guess... but it's a bit
 surprising
> > nonetheless. Comment perhaps?

> i'm not sure what kind of comment you're thinking here.
> how is failing to write the configuration any different
> from any of the above failure modes?

Yeah, I guess it's no different; this is just the latest expression of
my paranoia that one-shotting bootstrap, rather than ensuring it
actually completes, will cause us surprising and upsetting problems one
day. No action needed here.

https://codereview.appspot.com/9738043/diff/5001/cmd/jujud/bootstrap_test.go
File cmd/jujud/bootstrap_test.go (right):

https://codereview.appspot.com/9738043/diff/5001/cmd/jujud/bootstrap_test.go#newcode198
cmd/jujud/bootstrap_test.go:198: info.Tag, info.Password = "",
testPasswordHash
On 2013/06/07 17:01:55, rog wrote:
> On 2013/05/27 20:13:26, fwereade wrote:
> > wait, why can user-admin log in as ""?

> the empty string means "admin" when logging into the state. see the
docs on
> state.Info.Tag. i disclaim all responsibility for that decision :-)

ok, thanks for pointing that out

https://codereview.appspot.com/9738043/diff/5001/cmd/jujud/bootstrap_test.go#newcode220
cmd/jujud/bootstrap_test.go:220:
c.Assert(machineConf1.StateInfo.Password, Equals,
machineConf1.APIInfo.Password)
On 2013/06/07 17:01:55, rog wrote:
> On 2013/05/27 20:13:26, fwereade wrote:
> > Are we duplicating tag/password for the same entities everywhere? If
 so, why?

> we use the same password for the state that we do for the API.
> is that bad?

I reserve judgment on that; I think it smells a little bit, because API
access is a very different thing to state access... but I'm more
thinking that the *.Info types are feeling a bit overloaded, and that --
if the two chunks of data are fundamentally the same rather than just
coincidentally so -- separating that common auth data is probably a
pretty good idea.

Really, I was just looking for clarification.

https://codereview.appspot.com/9738043/diff/5001/environs/agent/agent.go
File environs/agent/agent.go (right):

https://codereview.appspot.com/9738043/diff/5001/environs/agent/agent.go#newcode208
environs/agent/agent.go:208: func (c *Conf) OpenAPI() (st *api.State,
newPassword string, err error) {
On 2013/06/07 17:01:55, rog wrote:
> On 2013/05/27 20:13:26, fwereade wrote:
> > Why are we duplicating this back-to-front newPassword-juggling
 behaviour?

> what do you suggest instead? we still need to do exactly the same
thing AFAICS.
> i wasn't thinking of redesigning this logic at this point, but i can
do if you'd
> like it to change.

Not worth making ...

Read more...

Reply
Revision history for this message
Roger Peppe (rogpeppe) wrote :

Please take a look.

https://codereview.appspot.com/9738043/

Reply
Revision history for this message
Roger Peppe (rogpeppe) wrote :

Please take a look.

https://codereview.appspot.com/9738043/

Reply
Revision history for this message
Roger Peppe (rogpeppe) wrote :

Please take a look.

https://codereview.appspot.com/9738043/

Reply

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches