Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(313)

Issue 6653050: environs, juju: require admin-secret

Can't Edit
Can't Publish+Mail
Start Review
Created:
13 years ago by rog
Modified:
13 years ago
Reviewers:
mp+129462
Visibility:
Public.

Description

environs, juju: require admin-secret The admin-secret configuration value is required for bootstrapping and connecting to an environment, but we can't make config.Config require it because it should never be pushed into the state. So we add checks to juju.NewConn and implementations of Environ.Bootstrap to require that it be set. This unfortunately means that all tests must connect with authentication set, which means the changes are necessarily bulky. https://code.launchpad.net/~rogpeppe/juju-core/126-mandatory-admin-secret/+merge/129462 Requires: https://code.launchpad.net/~rogpeppe/juju-core/127-jujud-bootstrap-do-not-use-jujuconnsuite/+merge/129395 (do not edit description out of merge proposal)

Patch Set 1 #

Patch Set 2 : environs, juju: require admin-secret #

Patch Set 3 : environs, juju: require admin-secret #

Patch Set 4 : environs, juju: require admin-secret #

Patch Set 5 : environs, juju: require admin-secret #

Patch Set 6 : environs, juju: require admin-secret #

Patch Set 7 : environs, juju: require admin-secret #

Patch Set 8 : environs, juju: require admin-secret #

Total comments: 4

Patch Set 9 : environs, juju: require admin-secret #

Unified diffs Side-by-side diffs Delta from patch set Stats (+118 lines, -27 lines) Patch
A [revision details] View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
M environs/dummy/environs.go View 1 2 3 4 5 6 7 8 2 chunks +6 lines, -4 lines 0 comments Download
M environs/dummy/environs_test.go View 1 chunk +1 line, -0 lines 0 comments Download
M environs/ec2/ec2.go View 1 2 3 4 5 6 7 8 2 chunks +5 lines, -5 lines 0 comments Download
M environs/ec2/local_test.go View 1 chunk +1 line, -0 lines 0 comments Download
M environs/jujutest/tests.go View 1 2 3 4 5 6 7 8 1 chunk +9 lines, -0 lines 0 comments Download
M environs/open_test.go View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M juju/conn.go View 1 chunk +3 lines, -0 lines 0 comments Download
M juju/conn_test.go View 1 2 3 7 chunks +59 lines, -9 lines 0 comments Download
M juju/deploy_test.go View 2 chunks +4 lines, -2 lines 0 comments Download
M juju/testing/conn.go View 1 2 3 4 5 6 7 8 4 chunks +16 lines, -2 lines 0 comments Download
M worker/provisioner/provisioner_test.go View 5 chunks +11 lines, -5 lines 0 comments Download

Messages

Total messages: 14
rog
Please take a look.
13 years ago (2012-10-12 16:02:11 UTC) #1
rog
Please take a look.
13 years ago (2012-10-12 16:03:34 UTC) #2
rog
Please take a look.
13 years ago (2012-10-12 16:30:28 UTC) #3
rog
Please take a look.
13 years ago (2012-10-12 16:40:21 UTC) #4
rog
Please take a look.
13 years ago (2012-10-12 16:42:06 UTC) #5
TheMue
LGTM
13 years ago (2012-10-12 16:48:06 UTC) #6
dave_cheney.net
On 2012/10/12 16:48:06, TheMue wrote: > LGTM Doesn't work for me, happily bootstrapped a broken ...
13 years ago (2012-10-15 00:05:09 UTC) #7
dave_cheney.net
> Doesn't work for me, happily bootstrapped a broken config > > us-east-2: > type: ...
13 years ago (2012-10-15 00:07:39 UTC) #8
fwereade
On 2012/10/15 00:07:39, dfc wrote: > We shouldn't get this far, admin-secret needs to be ...
13 years ago (2012-10-15 06:18:26 UTC) #9
rog
On 2012/10/15 00:07:39, dfc wrote: > > Doesn't work for me, happily bootstrapped a broken ...
13 years ago (2012-10-15 12:45:56 UTC) #10
rog
Please take a look.
13 years ago (2012-10-15 12:46:37 UTC) #11
niemeyer
On 2012/10/15 06:18:26, fwereade wrote: > On 2012/10/15 00:07:39, dfc wrote: > > We shouldn't ...
13 years ago (2012-10-16 16:31:46 UTC) #12
niemeyer
Quite straightforward. LGTM with a trivial note: https://codereview.appspot.com/6653050/diff/16001/environs/jujutest/tests.go File environs/jujutest/tests.go (right): https://codereview.appspot.com/6653050/diff/16001/environs/jujutest/tests.go#newcode55 environs/jujutest/tests.go:55: c.Assert(err, ErrorMatches, ...
13 years ago (2012-10-16 17:18:53 UTC) #13
rog
13 years ago (2012-10-16 17:31:01 UTC) #14
*** Submitted:

environs, juju: require admin-secret

The admin-secret configuration value is required
for bootstrapping and connecting to an environment,
but we can't make config.Config require it because
it should never be pushed into the state.

So we add checks to juju.NewConn and implementations
of Environ.Bootstrap to require that it be set.
This unfortunately means that all tests must
connect with authentication set, which means
the changes are necessarily bulky.

R=TheMue, dfc, fwereade, niemeyer
CC=
https://codereview.appspot.com/6653050

https://codereview.appspot.com/6653050/diff/16001/environs/jujutest/tests.go
File environs/jujutest/tests.go (right):

https://codereview.appspot.com/6653050/diff/16001/environs/jujutest/tests.go#...
environs/jujutest/tests.go:55: c.Assert(err, ErrorMatches, ".*admin-secret.*")
On 2012/10/16 17:18:53, niemeyer wrote:
> It wouldn't hurt to assert a bit more about the message. Helps even to read
the
> test.

Done.

https://codereview.appspot.com/6653050/diff/16001/juju/testing/conn.go
File juju/testing/conn.go (right):

https://codereview.appspot.com/6653050/diff/16001/juju/testing/conn.go#newcod...
juju/testing/conn.go:136: // by the test independently of JujuConnSuite.
On 2012/10/16 17:18:53, niemeyer wrote:
> // Bootstrap will set the admin password, and render non-authorized use
> // impossible. s.State may still hold the right password, so try to reset
> // the password so that the MgoSuite soft-resetting works. If that fails,
> // it will still work, but it will take a while since it has to kill the
> // whole database and start over.

much better. done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b