environs, juju: require admin-secret

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)

[rog, 2012-10-12 16:02:11]

Please take a look.

[rog, 2012-10-12 16:03:34]

Please take a look.

[rog, 2012-10-12 16:30:28]

Please take a look.

[rog, 2012-10-12 16:40:21]

Please take a look.

[rog, 2012-10-12 16:42:06]

Please take a look.

[TheMue, 2012-10-12 16:48:06]

LGTM

[dave_cheney.net, 2012-10-15 00:05:09]

On 2012/10/12 16:48:06, TheMue wrote:
> LGTM

Doesn't work for me, happily bootstrapped a broken config

  us-east-2:
    type: ec2
    control-bucket: juju-A218c46225cb7856d9d5ca3c1a685cd87b647996
    region: us-east-1

[dave_cheney.net, 2012-10-15 00:07:39]

> Doesn't work for me, happily bootstrapped a broken config
> 
>   us-east-2:
>     type: ec2
>     control-bucket: juju-A218c46225cb7856d9d5ca3c1a685cd87b647996
>     region: us-east-1

% juju bootstrap -e us-east-2 --upload-tools --debug
2012/10/15 11:04:11 JUJU environs/ec2: opening environment "us-east-2"
2012/10/15 11:04:11 JUJU environs/ec2: bootstrapping environment "us-east-2"
2012/10/15 11:04:19 JUJU environs: putting tools
tools/juju-0.0.1-precise-amd64.tgz (3168kB)
2012/10/15 11:05:18 JUJU:DEBUG juju bootstrap command failed: admin-secret is
required for bootstrap
error: admin-secret is required for bootstrap

We shouldn't get this far, admin-secret needs to be a required config var.

[fwereade, 2012-10-15 06:18:26]

On 2012/10/15 00:07:39, dfc wrote:
> We shouldn't get this far, admin-secret needs to be a required config var.

FWIW, I maintain that user-config is not the same thing as internal-config, and
trying to pretend that they are causes us nothing but pain. Not sure that topic
is really open for discussion, though...

[rog, 2012-10-15 12:45:56]

On 2012/10/15 00:07:39, dfc wrote:
> > Doesn't work for me, happily bootstrapped a broken config
> > 
> >   us-east-2:
> >     type: ec2
> >     control-bucket: juju-A218c46225cb7856d9d5ca3c1a685cd87b647996
> >     region: us-east-1
> 
> % juju bootstrap -e us-east-2 --upload-tools --debug
> 2012/10/15 11:04:11 JUJU environs/ec2: opening environment "us-east-2"
> 2012/10/15 11:04:11 JUJU environs/ec2: bootstrapping environment "us-east-2"
> 2012/10/15 11:04:19 JUJU environs: putting tools
> tools/juju-0.0.1-precise-amd64.tgz (3168kB)
> 2012/10/15 11:05:18 JUJU:DEBUG juju bootstrap command failed: admin-secret is
> required for bootstrap
> error: admin-secret is required for bootstrap
> 
> We shouldn't get this far, admin-secret needs to be a required config var.

As discussed on-line, I don't think admin-secret can be a required config var as
we don't want to push the admin secret into state. We could potentially push a
fake admin-secret, but I think that's asking for trouble.

I've fixed your issue above, I hope, by moving the admin-secret test earlier in
the Bootstrap method.

[rog, 2012-10-15 12:46:37]

Please take a look.

[niemeyer, 2012-10-16 16:31:46]

On 2012/10/15 06:18:26, fwereade wrote:
> On 2012/10/15 00:07:39, dfc wrote:
> > We shouldn't get this far, admin-secret needs to be a required config var.
> 
> FWIW, I maintain that user-config is not the same thing as internal-config,
and
> trying to pretend that they are causes us nothing but pain. Not sure that
topic
> is really open for discussion, though...

I agree, and it seems open for discussion.

The reason I didn't personally suggest anything else is because I'm trying to
avoid derailing into a long thread that would involve a second pipeline of logic
across the whole code base right now (how does the configuration look like? When
do we read the setting and where do we store? How do we provide the data to
Conn, to juju.Open, and even to the environments themselves when they do care
(Bootstrap)?

Using Config avoids all of those in a cheap way.

If you have suggestions for alternatives, I'm certainly interested in hearing
though, even if for the future.

[niemeyer, 2012-10-16 17:18:53]

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#...
environs/jujutest/tests.go:55: c.Assert(err, ErrorMatches, ".*admin-secret.*")
It wouldn't hurt to assert a bit more about the message. Helps even to read the
test.

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.
// 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.

[rog, 2012-10-16 17:31:01]

*** 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.