environs/config: more restrictive New

environs/config: more restrictive New

In most cases, we don't want to suck
default config values from environment
variables, $HOME, etc. This change
makes it possible to be sure that it
is not doing so when we don't want it to.

Unfortunately, as with most config changes,
the repercussions are widespread.
I have taken the opportunity to centralise
the basic configuration attributes
for the tests, so changing the configuration
should be easier in the future.

Still not done: providers still take defaults
from the environment. That will
change in another CL.

Two drive-by changes:
- jujutest.LifeTests.TestConfig becomes
a coretesting.Attrs, rather than its own type.
I couldn't see that the indirection made any
difference to potential map sharing.

- firewall modes are no longer typed.
There was a conflict between configurations
attributes specified as strings and configurations
specified with the typed constants, and the
most straightforward way out is just to use
strings. Almost nothing mentions the type
name anyway, which indicates that it's not
offering a great deal of protection in practice.

https://code.launchpad.net/~rogpeppe/juju-core/381-config-no-defaults/+merge/183253

(do not edit description out of merge proposal)

[rog, 2013-08-30 19:30:52]

Please take a look.

[rog, 2013-08-30 20:46:34]

Please take a look.

[jameinel, 2013-09-01 11:43:16]

I don't quite understand the motivation for this. Is this just that we don't
want to look at ENV vars on the State nodes?

Has that ever been a problem?

I didn't see any major problems with the patch, but I couldn't help but feel it
was a whole lot of churn without a tangible gain.

[fwereade, 2013-09-02 09:29:39]

On 2013/09/01 11:43:16, jameinel wrote:
> I don't quite understand the motivation for this. Is this just that we don't
> want to look at ENV vars on the State nodes?
> 
> Has that ever been a problem?

To be fair: yes, it has been a serious problem in the past, and it probably will
be again. The test suite has historically been almost hilariously unaware of the
environments the code needs to operate in, and even small steps towards sanity
here are valuable on their own merits.

> I didn't see any major problems with the patch, but I couldn't help but feel
it
> was a whole lot of churn without a tangible gain.

So, as above, I do think the gain is tangible, and I am grateful for it in
isolation.

However, Roger, I'm worried that you've taken on too much here. This STM to be
an outgrowth of environ work you offered to take on quickly... but that was
almost a month ago, and it appears to be costing us dearly wrt API progress.
Dimiter's *long* holiday has been and gone, and I don't think you've done any
uniter work, and that's actually pretty catastrophic.

If jam's actually reviewed the change I have no objection to landing it though
-- it's a sunk cost, so we may as well derive what benefit we can. But let's
please not sink any more.

[jameinel, 2013-09-03 08:10:23]

I have a couple of questions about the core bits, but the rest just looks like
mechanical fallout changes.

https://codereview.appspot.com/13355044/diff/7001/cmd/juju/environment_test.go
File cmd/juju/environment_test.go (right):

https://codereview.appspot.com/13355044/diff/7001/cmd/juju/environment_test.g...
cmd/juju/environment_test.go:68: pattern := fmt.Sprintf("%s%s: %s", any, key,
any)
I don't quite understand why you need any on the second half.

However, I'm more concerned about any on the first half. Which means that a key
of "bar" will be matched if you have:
foo-bar: baz

Because all you're matching is that $KEY: exists. I think you actually need some
sort of regex multiline mode and that ^$KEY: $.
Also, we aren't calling QuoteMeta on the key, so "foo.bar" will match more than
we expect.

I'm guessing it isn't strictly a problem, and you didn't introduce it, but it
does seem suspicious.

https://codereview.appspot.com/13355044/diff/7001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/13355044/diff/7001/environs/config/config.go#n...
environs/config/config.go:162: return fmt.Errorf("attribute %q is not allowed in
configuration", attr)
I thought we were actually pushing to move these fields into the
.environments/*.yaml file so that we don't have to have 3+ files around for a
given environment.

I realize we can change it when we get there, but it seems like a pretty strong
statement here.

https://codereview.appspot.com/13355044/diff/7001/environs/tools/urls_test.go
File environs/tools/urls_test.go (right):

https://codereview.appspot.com/13355044/diff/7001/environs/tools/urls_test.go...
environs/tools/urls_test.go:30: attrs := dummy.SampleConfig
I really like having the baseline SampleConfig.

https://codereview.appspot.com/13355044/diff/7001/testing/attrs.go
File testing/attrs.go (right):

https://codereview.appspot.com/13355044/diff/7001/testing/attrs.go#newcode6
testing/attrs.go:6: 
I really like this bit.

[rog, 2013-09-11 17:59:12]

I realised that the live tests didn't pass and that the changes I made were
backwardly incompatible. I've fixed live tests (in ec2, at any rate) and checked
that we can upgrade OK.

https://codereview.appspot.com/13355044/diff/7001/cmd/juju/environment_test.go
File cmd/juju/environment_test.go (right):

https://codereview.appspot.com/13355044/diff/7001/cmd/juju/environment_test.g...
cmd/juju/environment_test.go:68: pattern := fmt.Sprintf("%s%s: %s", any, key,
any)
On 2013/09/03 08:10:23, jameinel wrote:
> I don't quite understand why you need any on the second half.
> 
> However, I'm more concerned about any on the first half. Which means that a
key
> of "bar" will be matched if you have:
> foo-bar: baz

I've fixed that.

> Because all you're matching is that $KEY: exists. I think you actually need
some
> sort of regex multiline mode and that ^$KEY: $.

We can't do that, sadly, because the value isn't necessarily all on one line.

> Also, we aren't calling QuoteMeta on the key, so "foo.bar" will match more
than
> we expect.

That's true, but I'm not too concerned about it, as we don't have any keys with
metachars in.

https://codereview.appspot.com/13355044/diff/7001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/13355044/diff/7001/environs/config/config.go#n...
environs/config/config.go:162: return fmt.Errorf("attribute %q is not allowed in
configuration", attr)
On 2013/09/03 08:10:23, jameinel wrote:
> I thought we were actually pushing to move these fields into the
> .environments/*.yaml file so that we don't have to have 3+ files around for a
> given environment.
> 
> I realize we can change it when we get there, but it seems like a pretty
strong
> statement here.

The certificate and private key between them add up to more than 1.5KB. That
really clutters up a manually edited file. Also, authorized keys are
conventionally held in files in ~/.ssh - it seems reasonable to be able to refer
to such a file. In the future, the need for the cert attributes should go away,
as we will generate the certificates at bootstrap time, but authorized-keys-path
may well stay.

If I deleted these attributes now, then I'd be making a backwardly incompatible
change, which I am reluctant to do right now.

[rog, 2013-09-11 18:00:50]

Please take a look.

[jameinel, 2013-09-12 06:29:52]

https://codereview.appspot.com/13355044/diff/7001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/13355044/diff/7001/environs/config/config.go#n...
environs/config/config.go:162: return fmt.Errorf("attribute %q is not allowed in
configuration", attr)
On 2013/09/11 17:59:13, rog wrote:
> On 2013/09/03 08:10:23, jameinel wrote:
> > I thought we were actually pushing to move these fields into the
> > .environments/*.yaml file so that we don't have to have 3+ files around for
a
> > given environment.
> > 
> > I realize we can change it when we get there, but it seems like a pretty
> strong
> > statement here.
> 
> The certificate and private key between them add up to more than 1.5KB. That
> really clutters up a manually edited file. Also, authorized keys are
> conventionally held in files in ~/.ssh - it seems reasonable to be able to
refer
> to such a file. In the future, the need for the cert attributes should go
away,
> as we will generate the certificates at bootstrap time, but
authorized-keys-path
> may well stay.
> 
> If I deleted these attributes now, then I'd be making a backwardly
incompatible
> change, which I am reluctant to do right now.

I realize that 1.5kb may seem like a bit, but it is actually pretty small. And
it moves us from "here is a collection of 3 things you need, 1 of them you have
to merge with your existing file, and 2 that you need to put next to it" into
"here is 1 file you need to copy into place". That is a *huge* win for
usability.

I've already run into this 2-3 times personally (go-bot was restored by Martin,
and he had to dig around until he could find the original gobot-cert.pem file
that we need to access node-0). I've run into it just running on my laptop vs my
desktop, etc.

I'm *really* looking forward to when we just have 1 file that has everything you
need for 1 environment.

[rog, 2013-09-12 06:47:44]

On 12 September 2013 07:29,  <john.meinel@canonical.com> wrote:
>
> https://codereview.appspot.com/13355044/diff/7001/environs/config/config.go
> File environs/config/config.go (right):
>
>
https://codereview.appspot.com/13355044/diff/7001/environs/config/config.go#n...
> environs/config/config.go:162: return fmt.Errorf("attribute %q is not
> allowed in configuration", attr)
> On 2013/09/11 17:59:13, rog wrote:
>>
>> On 2013/09/03 08:10:23, jameinel wrote:
>> > I thought we were actually pushing to move these fields into the
>> > .environments/*.yaml file so that we don't have to have 3+ files
>
> around for a
>>
>> > given environment.
>> >
>> > I realize we can change it when we get there, but it seems like a
>
> pretty
>>
>> strong
>> > statement here.
>
>
>> The certificate and private key between them add up to more than
>
> 1.5KB. That
>>
>> really clutters up a manually edited file. Also, authorized keys are
>> conventionally held in files in ~/.ssh - it seems reasonable to be
>
> able to refer
>>
>> to such a file. In the future, the need for the cert attributes should
>
> go away,
>>
>> as we will generate the certificates at bootstrap time, but
>
> authorized-keys-path
>>
>> may well stay.
>
>
>> If I deleted these attributes now, then I'd be making a backwardly
>
> incompatible
>>
>> change, which I am reluctant to do right now.
>
>
> I realize that 1.5kb may seem like a bit, but it is actually pretty
> small. And it moves us from "here is a collection of 3 things you need,
> 1 of them you have to merge with your existing file, and 2 that you need
> to put next to it" into "here is 1 file you need to copy into place".
> That is a *huge* win for usability.
>
> I've already run into this 2-3 times personally (go-bot was restored by
> Martin, and he had to dig around until he could find the original
> gobot-cert.pem file that we need to access node-0). I've run into it
> just running on my laptop vs my desktop, etc.
>
> I'm *really* looking forward to when we just have 1 file that has
> everything you need for 1 environment.

You can of course do this right now - the paths are just for convenience
(well, apart from the fact that the certificate is automatically generated)

We are definitely aiming towards a place where a single file holds
everything necessary to access an environment (but it won't be
environments.yaml, but .environments/<envname>.yaml)

I'm still awaiting a LGTM (or a definite rejection) for this branch BTW.

[fwereade, 2013-09-12 09:20:16]

This is great, but I have enough quibbles that I can't quite issue the magic
acronym. Let me know what you think.

https://codereview.appspot.com/13355044/diff/20053/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/13355044/diff/20053/environs/config/config.go#...
environs/config/config.go:56: NoDefaults  Defaulting = false
There's something a bit off about the "defaults" terminology. To me it's not so
much about defaults as about reading from an... environment. But that word's
overloaded in juju.

https://codereview.appspot.com/13355044/diff/20053/environs/config/config.go#...
environs/config/config.go:82: func New(withDefaults Defaulting, attrs
map[string]interface{}) (*Config, error) {
Can we maybe unexport this and have two separate constructors? The
non-"defaulting" case would I think be plain New, and the rare, weird, scary
case could get a more explicit name... NewUsingCliEnvironment or something? That
feels like a small step towards a happier place, because it would then be kinda
natural to add an interface param that let us wrap all the messy external
context up into a single thing that we can use as a missing-values source for a
config.

https://codereview.appspot.com/13355044/diff/20053/environs/config/config.go#...
environs/config/config.go:160: for _, attr := range allowedWithDefaultsOnly {
oo, nice.

https://codereview.appspot.com/13355044/diff/20053/environs/config/config.go#...
environs/config/config.go:202: if caCertOK || caKeyOK {
This looks definitely funky, but I can see it's preserved behaviour. IIRC it's
the cause of all the `ca-private-key: ""` junk that clutters up some of the test
suites; but I don't recall why the presence of the ca cert requires the presence
of the private key. Can you remind me?

https://codereview.appspot.com/13355044/diff/20053/environs/config/config.go#...
environs/config/config.go:229: return false
not `val == false`? seems inconsistent with the below

https://codereview.appspot.com/13355044/diff/20053/environs/config/config.go#...
environs/config/config.go:297: // it is not found or is zero.
Hmm, a panic on zero seems pretty bad. Panic on missing, or wrong type, both
seem fine though.

https://codereview.appspot.com/13355044/diff/20053/environs/config/config.go#...
environs/config/config.go:366: func (c *Config) FirewallMode() string {
Don't these have their own type?

https://codereview.appspot.com/13355044/diff/20053/environs/config/config.go#...
environs/config/config.go:469: "agent-version":        schema.Omit,
This one isn't *always* optional -- it's needed to bootstrap with, and forever
after that. But you can start a bootstrap without it, so it is sometimes
optional.

https://codereview.appspot.com/13355044/diff/20053/environs/config/config.go#...
environs/config/config.go:471: "authorized-keys":      schema.Omit,
This also isn't always optional IIRC -- if it's missing something critical falls
over and throws up.

https://codereview.appspot.com/13355044/diff/20053/environs/config/config.go#...
environs/config/config.go:474: "ca-private-key-path":  schema.Omit,
...and these are actually *bad* in a non-defaults context, so they aren't always
optional either.

https://codereview.appspot.com/13355044/diff/20053/environs/config/config.go#...
environs/config/config.go:490: }
So, yeah, I think I'm fine with what this is really doing, but the
alwaysOptional naming messes with me. Can you think of something a bit more
precise? And maybe call out the following mandatoryWithoutDefaults, etc, as
modifying these?

https://codereview.appspot.com/13355044/diff/20053/provider/dummy/environs.go
File provider/dummy/environs.go (right):

https://codereview.appspot.com/13355044/diff/20053/provider/dummy/environs.go...
provider/dummy/environs.go:73: }
I fret that this is more than somewhat vulnerable to accidental cross-test
pollution. Can we make it `func SampleConfig() testing.Attrs` please?

https://codereview.appspot.com/13355044/diff/20053/testing/environ.go
File testing/environ.go (right):

https://codereview.appspot.com/13355044/diff/20053/testing/environ.go#newcode29
testing/environ.go:29: }
Similarly, while it is true that when I've seen this one in use it's always
nicely Merge/Delete'd, IMO it's still all too easy to use it directly a test and
change it without meaning to.

https://codereview.appspot.com/13355044/diff/20053/worker/firewaller/firewall...
File worker/firewaller/firewaller_test.go (right):

https://codereview.appspot.com/13355044/diff/20053/worker/firewaller/firewall...
worker/firewaller/firewaller_test.go:111: // has bootstrapped.
Would you make this a bug please?

[rog, 2013-09-12 13:21:29]

Please take a look.

https://codereview.appspot.com/13355044/diff/20053/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/13355044/diff/20053/environs/config/config.go#...
environs/config/config.go:56: NoDefaults  Defaulting = false
On 2013/09/12 09:20:17, fwereade wrote:
> There's something a bit off about the "defaults" terminology. To me it's not
so
> much about defaults as about reading from an... environment. But that word's
> overloaded in juju.

Yeah, it's kinda both. Will be fixed when below issue is addressed.

https://codereview.appspot.com/13355044/diff/20053/environs/config/config.go#...
environs/config/config.go:82: func New(withDefaults Defaulting, attrs
map[string]interface{}) (*Config, error) {
On 2013/09/12 09:20:17, fwereade wrote:
> Can we maybe unexport this and have two separate constructors? The
> non-"defaulting" case would I think be plain New, and the rare, weird, scary
> case could get a more explicit name... NewUsingCliEnvironment or something?
That
> feels like a small step towards a happier place, because it would then be
kinda
> natural to add an interface param that let us wrap all the messy external
> context up into a single thing that we can use as a missing-values source for
a
> config.

As discussed online, this is useful for smoke-testing existing unchanged uses;
will change as above in a subsequent CL.

https://codereview.appspot.com/13355044/diff/20053/environs/config/config.go#...
environs/config/config.go:202: if caCertOK || caKeyOK {
On 2013/09/12 09:20:17, fwereade wrote:
> This looks definitely funky, but I can see it's preserved behaviour. IIRC it's
> the cause of all the `ca-private-key: ""` junk that clutters up some of the
test
> suites; but I don't recall why the presence of the ca cert requires the
presence
> of the private key. Can you remind me?

The presence of the ca cert doesn't require the presence of the private key (but
the presence of the private key *does* require the cert). verifyKeyPair works
even if key is nil.

This isn't actually the cause of the `ca-private-key: ""` junk - that was to
suppress the default key reading behaviour when ca-private-key is omitted. An
earlier version of this CL did actually remove that junk entirely, but
unfortunately it's backwardly incompatible - we need to be able to read the old
version of the config, and old versions need to be able to read this version, so
I had to reinstate it. It should be able to go in a subsequent version.

https://codereview.appspot.com/13355044/diff/20053/environs/config/config.go#...
environs/config/config.go:229: return false
On 2013/09/12 09:20:17, fwereade wrote:
> not `val == false`? seems inconsistent with the below

I *think* it's right - we're saying that bool attributes have to be specified
explicitly. Unlike int and string values (where for backward compatibility
reasons we need to treat 0 and "" as unspecified) both possible values of a bool
attribute signify that the attribute is specified.

https://codereview.appspot.com/13355044/diff/20053/environs/config/config.go#...
environs/config/config.go:297: // it is not found or is zero.
On 2013/09/12 09:20:17, fwereade wrote:
> Hmm, a panic on zero seems pretty bad. Panic on missing, or wrong type, both
> seem fine though.

Zero means empty for an integer attribute, and Validate checks all integer
attributes are non-empty, so this should never be possible, hence
I believe the panic is appropriate.

https://codereview.appspot.com/13355044/diff/20053/environs/config/config.go#...
environs/config/config.go:366: func (c *Config) FirewallMode() string {
On 2013/09/12 09:20:17, fwereade wrote:
> Don't these have their own type?

Not any more - I mention this in the CL description.

https://codereview.appspot.com/13355044/diff/20053/environs/config/config.go#...
environs/config/config.go:490: }
On 2013/09/12 09:20:17, fwereade wrote:
> So, yeah, I think I'm fine with what this is really doing, but the
> alwaysOptional naming messes with me. Can you think of something a bit more
> precise? And maybe call out the following mandatoryWithoutDefaults, etc, as
> modifying these?

I agree with you about the name not being quite right, but I can't currently
think of a better name.

I've adjusted the comment to take into account your remarks - suggestions for a
better name are welcome.

https://codereview.appspot.com/13355044/diff/20053/provider/dummy/environs.go
File provider/dummy/environs.go (right):

https://codereview.appspot.com/13355044/diff/20053/provider/dummy/environs.go...
provider/dummy/environs.go:73: }
On 2013/09/12 09:20:17, fwereade wrote:
> I fret that this is more than somewhat vulnerable to accidental cross-test
> pollution. Can we make it `func SampleConfig() testing.Attrs` please?

Done.

https://codereview.appspot.com/13355044/diff/20053/testing/environ.go
File testing/environ.go (right):

https://codereview.appspot.com/13355044/diff/20053/testing/environ.go#newcode29
testing/environ.go:29: }
On 2013/09/12 09:20:17, fwereade wrote:
> Similarly, while it is true that when I've seen this one in use it's always
> nicely Merge/Delete'd, IMO it's still all too easy to use it directly a test
and
> change it without meaning to.

Done.

https://codereview.appspot.com/13355044/diff/20053/worker/firewaller/firewall...
File worker/firewaller/firewaller_test.go (right):

https://codereview.appspot.com/13355044/diff/20053/worker/firewaller/firewall...
worker/firewaller/firewaller_test.go:111: // has bootstrapped.
On 2013/09/12 09:20:17, fwereade wrote:
> Would you make this a bug please?

Done. lp#1224456

[fwereade, 2013-09-12 13:59:49]

LGTM if you're ok with the int thing.

https://codereview.appspot.com/13355044/diff/20053/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/13355044/diff/20053/environs/config/config.go#...
environs/config/config.go:297: // it is not found or is zero.
On 2013/09/12 13:21:29, rog wrote:
> On 2013/09/12 09:20:17, fwereade wrote:
> > Hmm, a panic on zero seems pretty bad. Panic on missing, or wrong type, both
> > seem fine though.
> 
> Zero means empty for an integer attribute, and Validate checks all integer
> attributes are non-empty, so this should never be possible, hence
> I believe the panic is appropriate.

Ah, I see. In that case I question 0 having a special meaning... was it really
like this before? If you're preserving existing behaviour then let it stand, but
please either make it a bug or explain why 0 should be disallowed for int
values.

https://codereview.appspot.com/13355044/diff/38053/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/13355044/diff/38053/environs/config/config.go#...
environs/config/config.go:471: // with NoDefaults and are checked at the later
Verify stage.
Thanks, this helps a lot.

[rog, 2013-09-12 14:15:44]

Please take a look.

https://codereview.appspot.com/13355044/diff/20053/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/13355044/diff/20053/environs/config/config.go#...
environs/config/config.go:297: // it is not found or is zero.
On 2013/09/12 13:59:50, fwereade wrote:
> On 2013/09/12 13:21:29, rog wrote:
> > On 2013/09/12 09:20:17, fwereade wrote:
> > > Hmm, a panic on zero seems pretty bad. Panic on missing, or wrong type,
both
> > > seem fine though.
> > 
> > Zero means empty for an integer attribute, and Validate checks all integer
> > attributes are non-empty, so this should never be possible, hence
> > I believe the panic is appropriate.
> 
> Ah, I see. In that case I question 0 having a special meaning... was it really
> like this before? If you're preserving existing behaviour then let it stand,
but
> please either make it a bug or explain why 0 should be disallowed for int
> values.

See https://bugs.launchpad.net/juju-core/+bug/1224492

[rog, 2013-09-12 14:26:03]

Please take a look.

[rog, 2013-09-12 14:57:14]

Please take a look.