Merge lp:~waigani/juju-core/get-oldConfig-from-state into lp:~go-bot/juju-core/trunk

[Jesse Meek]

Update and Validate Environ Config in State

The main business logic happens in state/state.go. The bulk of the other
changes are mechanical i.e. updating method calls. There are a few things going on:

-Explicitly add/update or remove attributes with state.UpdateEnvironConf, which
replaces state.SetEnvironConf.
- Add ConfigValidator to state.Policy interface.
- Validate environment configuration within state.
    - build new config from UpdateEnvironConf args, get old config from state.
    - pass both configs to st.validate which use the environ policy
 ConfigValidator to get the correct validation method.
    - use the validated config to make the actual updates in state.
- Update or delete tests that rely on an invalid configuration being set, as
    this is now not possible.

Update and Validate Environ Config in State

The main business logic happens in state/state.go. The bulk of the other
changes are mechanical i.e. updating method calls. There are a few things going on:

-Explicitly add/update or remove attributes with state.UpdateEnvironConf, which
replaces state.SetEnvironConf.
- Add ConfigValidator to state.Policy interface.
- Validate environment configuration within state.
    - build new config from UpdateEnvironConf args, get old config from state.
    - pass both configs to st.validate which use the environ policy
 ConfigValidator to get the correct validation method.
    - use the validated config to make the actual updates in state.
- Update or delete tests that rely on an invalid configuration being set, as
    this is now not possible.

A follow up branch will use txn and a retry loop to address a race condition in
updating state config to ensure that concurrent updates to state config do not corrupt one
another.

https://codereview.appspot.com/70190050/

[Andrew Wilkins (axwalk)]

Some preliminary comments. I've only really looked at state.go.

https://codereview.appspot.com/70190050/diff/1/state/state.go
File state/state.go (left):

https://codereview.appspot.com/70190050/diff/1/state/state.go#oldcode282
state/state.go:282: _, err = settings.Write()
Write isn't really good enough here; this does not address the deleted
TODO. The update to settings should assert that there has been no change
to settings since st.EnvironConfig() was called; otherwise the
validation is not complete.

https://codereview.appspot.com/70190050/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode259
state/state.go:259: func (st *State)
VaidateUpdateEnvironConfig(updateAttrs map[string]interface{},
removeAttrs []string, oldConfig *config.Config) error {
s/Vaidate/Validate/
also, doesn't need to be exported does it?

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode264
state/state.go:264: // Build new config with attributes to add/update
You can get rid of the comments, if statement, forward declared
newConfig/err vars, and just replace with:

     newConfig, err := oldConfig.Apply(updateAttrs)
     if err != nil {
         return err
     }

Apply doesn't care if updateAttrs is nil or empty.

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode274
state/state.go:274: if len(removeAttrs) != 0 {
And then since newConfig exists unconditionally:

     newConfig, err = newConfig.Remove(removeAttrs)
     if err != nil {
         return err
     }

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode304
state/state.go:304: func (st *State) SetEnvironConfig(attrs
map[string]interface{}) error {
Why do we still need SetEnvironConfig? This still has the old race
conditions. *If* it's needed, then SetEnvironConfig and
UpdateEnvironConfig should use a common method that takes a
*state.Settings as input and does the no-concurrent-change check on
that.

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode327
state/state.go:327: oldConfig, err := st.EnvironConfig()
This does a readSettings, so you're reading settings twice. I think it'd
be preferable to just do a readSettings and use its Map() to create a
config.

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode333
state/state.go:333: err = st.VaidateUpdateEnvironConfig(updateAttrs,
removeAttrs, oldConfig)
Since ValidateUpdateEnvironConfig updates the config with attributes to
validate, I wonder if it's worthwhile even having a separate method.
You're duplicating work here by updating settings.

I think it would be simpler to just:
    1. get settings, and create a config from them
    2. apply changes to get a new config, check validity
    3. do something *like* replaceSettingsOp (see settings.go), but
passing in the *state.Settings

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode351
state/state.go:351: // Add/Update attributes.
These comments aren't really useful, the code speaks for itself.

https://codereview.appspot.com/70190050/

[Jesse Meek (waigani)]

Reviewers: mp+209569_code.launchpad.net, axw,

https://codereview.appspot.com/70190050/diff/1/state/state.go
File state/state.go (left):

https://codereview.appspot.com/70190050/diff/1/state/state.go#oldcode282
state/state.go:282: _, err = settings.Write()
On 2014/03/06 04:19:17, axw wrote:
> Write isn't really good enough here; this does not address the deleted
TODO. The
> update to settings should assert that there has been no change to
settings since
> st.EnvironConfig() was called; otherwise the validation is not
complete.

fwereade wanted a txn retry loop, ensuring the underlying settings had
not been changed, in client.EnvironmentSet
(https://codereview.appspot.com/63790045/diff/40001/state/apiserver/client/client.go#newcode791).
As this is a WIP, I have not done it yet. I can make a TODO in
client.EnvironmentSet and write the txn retry loop in a follow up
branch?

https://codereview.appspot.com/70190050/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode259
state/state.go:259: func (st *State)
VaidateUpdateEnvironConfig(updateAttrs map[string]interface{},
removeAttrs []string, oldConfig *config.Config) error {
On 2014/03/06 04:19:17, axw wrote:
> s/Vaidate/Validate/
> also, doesn't need to be exported does it?

Good catch!

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode264
state/state.go:264: // Build new config with attributes to add/update
On 2014/03/06 04:19:17, axw wrote:
> You can get rid of the comments, if statement, forward declared
newConfig/err
> vars, and just replace with:

> newConfig, err := oldConfig.Apply(updateAttrs)
> if err != nil {
> return err
> }

> Apply doesn't care if updateAttrs is nil or empty.

Done.

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode274
state/state.go:274: if len(removeAttrs) != 0 {
On 2014/03/06 04:19:17, axw wrote:
> And then since newConfig exists unconditionally:

> newConfig, err = newConfig.Remove(removeAttrs)
> if err != nil {
> return err
> }

Done.

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode304
state/state.go:304: func (st *State) SetEnvironConfig(attrs
map[string]interface{}) error {
On 2014/03/06 04:19:17, axw wrote:
> Why do we still need SetEnvironConfig? This still has the old race
conditions.
> *If* it's needed, then SetEnvironConfig and UpdateEnvironConfig should
use a
> common method that takes a *state.Settings as input and does the
> no-concurrent-change check on that.

It was needed by testing.ChangeEnvironConfig. ChangeEnvironConfig is
only used in two places, but it is actually easier to use
UpdateEnvironConfig directly instead. So I would be in favour of
deleting SetEnvironConfig and ChangeEnvironConfig.

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode327
state/state.go:327: oldConfig, err := st.EnvironConfig()
On 2014/03/06 04:19:17, axw wrote:
> This does a readSettings, so you're reading settings twice. I think
it'd be
> preferable to just do a readSettings and use its Map() to create a
config.

Done.

https://codereview.appspot.com/70190050/diff/1/state/stat...

[Andrew Wilkins (axwalk)]

https://codereview.appspot.com/70190050/diff/1/state/state.go
File state/state.go (left):

https://codereview.appspot.com/70190050/diff/1/state/state.go#oldcode282
state/state.go:282: _, err = settings.Write()
On 2014/03/06 08:17:48, waigani wrote:
> On 2014/03/06 04:19:17, axw wrote:
> > Write isn't really good enough here; this does not address the
deleted TODO.
> The
> > update to settings should assert that there has been no change to
settings
> since
> > st.EnvironConfig() was called; otherwise the validation is not
complete.

> fwereade wanted a txn retry loop, ensuring the underlying settings had
not been
> changed, in client.EnvironmentSet

(https://codereview.appspot.com/63790045/diff/40001/state/apiserver/client/client.go#newcode791).
> As this is a WIP, I have not done it yet. I can make a TODO in
> client.EnvironmentSet and write the txn retry loop in a follow up
branch?

The assertion is a prerequisite of the looping. The assertion is what
ensures that the settings haven't been changed concurrently. If the
settings are changed underfoot, then the transaction fails with
txn.ErrAborted.

The assertion must happen here, whereas looping may happen here or in
the caller. There's basically no point in making any of these changes
unless that assertion is in place.

https://codereview.appspot.com/70190050/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode304
state/state.go:304: func (st *State) SetEnvironConfig(attrs
map[string]interface{}) error {
On 2014/03/06 08:17:48, waigani wrote:
> On 2014/03/06 04:19:17, axw wrote:
> > Why do we still need SetEnvironConfig? This still has the old race
conditions.
> > *If* it's needed, then SetEnvironConfig and UpdateEnvironConfig
should use a
> > common method that takes a *state.Settings as input and does the
> > no-concurrent-change check on that.

> It was needed by testing.ChangeEnvironConfig. ChangeEnvironConfig is
only used
> in two places, but it is actually easier to use UpdateEnvironConfig
directly
> instead. So I would be in favour of deleting SetEnvironConfig and
> ChangeEnvironConfig.

ChangeEnvironConfig can be modified to compute a delta and pass it into
UpdateEnvironConfig. I know it's a bit arse about, but I don't think we
should be exposing ourselves to bugs just to simplify a test. If you can
just get rid of ChangeEnvironConfig, though, that may be better.

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode333
state/state.go:333: err = st.VaidateUpdateEnvironConfig(updateAttrs,
removeAttrs, oldConfig)
On 2014/03/06 08:17:48, waigani wrote:
> On 2014/03/06 04:19:17, axw wrote:
> > Since ValidateUpdateEnvironConfig updates the config with attributes
to
> > validate, I wonder if it's worthwhile even having a separate method.
You're
> > duplicating work here by updating settings.
> >
> > I think it would be simpler to just:
> > 1. get settings, and create a config from them
> > 2. apply changes to get a new config, check validity
> > 3. do something *like* replaceSettingsOp (see settings.go), but
passing in
> > the *state.Settings

> Do you think something *like* replaceSettingsOp s...

[Jesse Meek (waigani)]

Please take a look.

https://codereview.appspot.com/70190050/

[Jesse Meek (waigani)]

Please take a look.

https://codereview.appspot.com/70190050/

[Jesse Meek (waigani)]

Please take a look.

https://codereview.appspot.com/70190050/

[Andrew Wilkins (axwalk)]

I haven't reviewed everything, this should keep you going for a while
though.

In general: we should not be dropping any tests that are not
*guaranteed* to never work. Since UpdateEnvironConfig is not completely
fixed yet, the tests should remain. This may involve hacking around
validation with a nil policy.

https://codereview.appspot.com/70190050/diff/60001/cmd/juju/environment.go
File cmd/juju/environment.go (right):

https://codereview.appspot.com/70190050/diff/60001/cmd/juju/environment.go#newcode166
cmd/juju/environment.go:166: return
conn.State.UpdateEnvironConfig(c.values, []string{})
Just nil is fine.

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

https://codereview.appspot.com/70190050/diff/60001/cmd/juju/environment_test.go#newcode173
cmd/juju/environment_test.go:173: c.Check(err, gc.ErrorMatches, `no
registered provider for "foo"`)
This has nothing to do with attribute mutability, so doesn't belong
here. Why not just change the value in the map to a real provider type?
You will need to make sure the provider is registered by importing its
package.

https://codereview.appspot.com/70190050/diff/60001/environs/config.go
File environs/config.go (right):

https://codereview.appspot.com/70190050/diff/60001/environs/config.go#newcode138
environs/config.go:138: func Provider(providerType string)
(EnvironProvider, error) {
Making cosmetic changes is fine, but it's best not to mix cosmetic
changes (or refactoring) with functional changes. Otherwise I'm
scrutinising everything to make sure there's no functional change, and
the review takes longer than necessary.

https://codereview.appspot.com/70190050/diff/60001/environs/testing/tools.go
File environs/testing/tools.go (right):

https://codereview.appspot.com/70190050/diff/60001/environs/testing/tools.go#newcode447
environs/testing/tools.go:447: err = st.UpdateEnvironConfig(attrs,
[]string{})
Just pass in the one attribute in a new map. FYI, you can make maps with
map literals like so:
   map[string]interface{}{"ssl-hostname-verification":
SSLHostnameVerification}

https://codereview.appspot.com/70190050/diff/60001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/70190050/diff/60001/juju/conn.go#newcode155
juju/conn.go:155: return c.State.UpdateEnvironConfig(attrs, []string{})
Just pass secrets here, and don't bother updating attrs in the loop.

https://codereview.appspot.com/70190050/diff/60001/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/70190050/diff/60001/juju/conn_test.go#newcode152
juju/conn_test.go:152: // Removing the secret from state should not be
possible, as
Well, that's only true for the dummy provider. Real providers don't have
defaults for secrets. For a real provider you'd have a different
problem: validation would fail because a config without the secrets is
invalid.

In this test it makes sense to disable validation while you're removing
the attribute.

https://codereview.appspot.com/70190050/diff/60001/state/api/firewaller/state_test.go
File state/api/firewaller/state_test.go (right):

https://codereview.appspot.com/70190050/diff/60001/...

[Jesse Meek (waigani)]

Please take a look.

https://codereview.appspot.com/70190050/diff/60001/cmd/juju/environment.go
File cmd/juju/environment.go (right):

https://codereview.appspot.com/70190050/diff/60001/cmd/juju/environment.go#newcode166
cmd/juju/environment.go:166: return
conn.State.UpdateEnvironConfig(c.values, []string{})
On 2014/03/10 03:41:54, axw wrote:
> Just nil is fine.

Done.

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

https://codereview.appspot.com/70190050/diff/60001/cmd/juju/environment_test.go#newcode173
cmd/juju/environment_test.go:173: c.Check(err, gc.ErrorMatches, `no
registered provider for "foo"`)
On 2014/03/10 03:41:54, axw wrote:
> This has nothing to do with attribute mutability, so doesn't belong
here. Why
> not just change the value in the map to a real provider type? You will
need to
> make sure the provider is registered by importing its package.

Done.

https://codereview.appspot.com/70190050/diff/60001/environs/config.go
File environs/config.go (right):

https://codereview.appspot.com/70190050/diff/60001/environs/config.go#newcode138
environs/config.go:138: func Provider(providerType string)
(EnvironProvider, error) {
On 2014/03/10 03:41:54, axw wrote:
> Making cosmetic changes is fine, but it's best not to mix cosmetic
changes (or
> refactoring) with functional changes. Otherwise I'm scrutinising
everything to
> make sure there's no functional change, and the review takes longer
than
> necessary.

Sorry about that. This branch is actually three squashed together. These
changes are in response to William's review of one of those branches.

https://codereview.appspot.com/70190050/diff/60001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/70190050/diff/60001/juju/conn.go#newcode155
juju/conn.go:155: return c.State.UpdateEnvironConfig(attrs, []string{})
On 2014/03/10 03:41:54, axw wrote:
> Just pass secrets here, and don't bother updating attrs in the loop.

Done. I couldn't cast or assert secrets (which is map[string]string) to
map[string]interface{}, so I created a new var secretAttrs with that
type instead.

https://codereview.appspot.com/70190050/diff/60001/state/api/firewaller/state_test.go
File state/api/firewaller/state_test.go (right):

https://codereview.appspot.com/70190050/diff/60001/state/api/firewaller/state_test.go#newcode94
state/api/firewaller/state_test.go:94: // TODO [waigani] test removing
an attribute.
On 2014/03/10 03:41:54, axw wrote:
> I don't think deferral is warranted here. Not sure it's really
necessary to add
> the test either, though: firewaller doesn't care about attributes
being removed.

Done. TODO removed.

https://codereview.appspot.com/70190050/diff/60001/state/api/firewaller/state_test.go#newcode94
state/api/firewaller/state_test.go:94: // TODO [waigani] test removing
an attribute.
On 2014/03/10 03:41:54, axw wrote:
> I don't think deferral is warranted here. Not sure it's really
necessary to add
> the test either, though: firewaller doesn't care about attributes
being removed.

https://codereview.appspot.com/70190050/diff/60001/state/apiserver/client/client.go
File state/apiserver/client/client...

[Jesse Meek (waigani)]

Yikes. I really dug a hole for myself with such a big branch. I've
finally merged in trunk, all tests pass and review comments addressed.
Let me know if I've missed anything.

https://codereview.appspot.com/70190050/

[Andrew Wilkins (axwalk)]

Getting there.

https://codereview.appspot.com/70190050/diff/60001/state/apiserver/keymanager/keymanager.go
File state/apiserver/keymanager/keymanager.go (left):

https://codereview.appspot.com/70190050/diff/60001/state/apiserver/keymanager/keymanager.go#oldcode311
state/apiserver/keymanager/keymanager.go:311: // For now, authorised
keys are global, common to all users.
On 2014/03/13 01:02:03, waigani wrote:
> On 2014/03/10 03:41:54, axw wrote:
> > This comment didn't need to be deleted

> I'm confused. The comment is still there on line 312?

Huh, I must've been on crack. Never mind.

https://codereview.appspot.com/70190050/diff/60001/worker/environ_test.go
File worker/environ_test.go (left):

https://codereview.appspot.com/70190050/diff/60001/worker/environ_test.go#oldcode49
worker/environ_test.go:49: // Create an invalid config by taking the
current config and
On 2014/03/13 01:02:03, waigani wrote:
> On 2014/03/10 03:41:54, axw wrote:
> > This shouldn't go until UpdateEnvironConfig is completely fixed.

> I see that TestInvalidConfig is no longer in trunk, so I assume to is
fine to
> leave this deleted?

I just pulled trunk and it is still there. Not sure what you're looking
at...
http://bazaar.launchpad.net/~go-bot/juju-core/trunk/view/head:/worker/environ_test.go#L48

https://codereview.appspot.com/70190050/diff/60001/worker/machineenvironmentworker/machineenvironmentworker_test.go
File worker/machineenvironmentworker/machineenvironmentworker_test.go
(right):

https://codereview.appspot.com/70190050/diff/60001/worker/machineenvironmentworker/machineenvironmentworker_test.go#newcode128
worker/machineenvironmentworker/machineenvironmentworker_test.go:128:
attrs := map[string]interface{}{}
On 2014/03/13 01:02:03, waigani wrote:
> On 2014/03/10 03:41:54, axw wrote:
> > The conventional way to write this is
> > attrs := make(map[string]interface{})

> Ah. There are a lot of map[string]interface{}{} around (from my code
and
> others). Shall I put off changing these to another day/branch?

No problems, just leave them.

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

https://codereview.appspot.com/70190050/diff/80001/cmd/juju/environment_test.go#newcode14
cmd/juju/environment_test.go:14: _
"launchpad.net/juju-core/provider/dummy"
dummy will already be imported by the base test suite, but you should
import local here.

https://codereview.appspot.com/70190050/diff/80001/environs/statepolicy.go
File environs/statepolicy.go (right):

https://codereview.appspot.com/70190050/diff/80001/environs/statepolicy.go#newcode43
environs/statepolicy.go:43: if p, ok :=
provider.(state.ConfigValidator); ok {
No need for the assertion here, this is always going to be true for
Provider. Just go ahead and "return p, nil".

https://codereview.appspot.com/70190050/diff/80001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/70190050/diff/80001/juju/conn.go#newcode146
juju/conn.go:146: var secretAttrs map[string]interface{}
You need to initialise the map, like so:
     secretAttrs := make(map[string]interface{})

https://codereview.appspot.com/70190050/diff/80001/juju...

[Jesse Meek (waigani)]

Please take a look.

https://codereview.appspot.com/70190050/diff/60001/worker/environ_test.go
File worker/environ_test.go (left):

https://codereview.appspot.com/70190050/diff/60001/worker/environ_test.go#oldcode49
worker/environ_test.go:49: // Create an invalid config by taking the
current config and
On 2014/03/13 03:07:14, axw wrote:
> On 2014/03/13 01:02:03, waigani wrote:
> > On 2014/03/10 03:41:54, axw wrote:
> > > This shouldn't go until UpdateEnvironConfig is completely fixed.
> >
> > I see that TestInvalidConfig is no longer in trunk, so I assume to
is fine to
> > leave this deleted?

> I just pulled trunk and it is still there. Not sure what you're
looking at...

http://bazaar.launchpad.net/~go-bot/juju-core/trunk/view/head:/worker/environ_test.go#L48

Done. I confused my working branch with trunk - facepalm.

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

https://codereview.appspot.com/70190050/diff/80001/cmd/juju/environment_test.go#newcode14
cmd/juju/environment_test.go:14: _
"launchpad.net/juju-core/provider/dummy"
On 2014/03/13 03:07:14, axw wrote:
> dummy will already be imported by the base test suite, but you should
import
> local here.

Done. Yeah, like in the line above - second facepalm.

https://codereview.appspot.com/70190050/diff/80001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/70190050/diff/80001/juju/conn.go#newcode146
juju/conn.go:146: var secretAttrs map[string]interface{}
On 2014/03/13 03:07:14, axw wrote:
> You need to initialise the map, like so:
> secretAttrs := make(map[string]interface{})

Done.

https://codereview.appspot.com/70190050/diff/80001/juju/conn.go#newcode153
juju/conn.go:153: secretAttrs[k] = v
On 2014/03/13 03:07:14, axw wrote:
> This will panic, since you didn't initialise the map. I guess you
didn't run the
> tests after this change?

Done. Strange, I guess not. Passes now.

https://codereview.appspot.com/70190050/diff/80001/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/70190050/diff/80001/juju/conn_test.go#newcode153
juju/conn_test.go:153: // UpdateEnvironConfig vaidates the config and
resets the "secret"
On 2014/03/13 03:07:14, axw wrote:
> s/vaidates/validates/

Done.

https://codereview.appspot.com/70190050/diff/80001/juju/conn_test.go#newcode154
juju/conn_test.go:154: // setting.
On 2014/03/13 03:07:14, axw wrote:
> In normal circumstances it shouldn't be possible, but this is
specifically for a
> test. Not removing it completely misses the point of the test: that if
the
> attributes *aren't* in state, they will be added. This is testing
compatibility
> with legacy code.

> Please put this back to how it was, using a nil policy to override the
removal
> prevention.

Done.

https://codereview.appspot.com/70190050/diff/80001/state/api/common/testing/environwatcher.go
File state/api/common/testing/environwatcher.go (right):

https://codereview.appspot.com/70190050/diff/80001/state/api/common/testing/environwatcher.go#newcode91
state/api/common/testing/environwatcher.go:91: // TODO [waigani] test
removing an attribute.
On 2014/03/13 03:07:14, axw w...

[Jesse Meek (waigani)]

https://codereview.appspot.com/70190050/

[Andrew Wilkins (axwalk)]

A few minor things, then it should be good to land.

https://codereview.appspot.com/70190050/diff/100001/environs/statepolicy.go
File environs/statepolicy.go (right):

https://codereview.appspot.com/70190050/diff/100001/environs/statepolicy.go#newcode43
environs/statepolicy.go:43: return provider.(state.ConfigValidator), nil
No need to type assert, since EnvironProvider implements
state.ConfigValidator. Just "return provider, nil".

https://codereview.appspot.com/70190050/diff/100001/environs/statepolicy.go#newcode44
environs/statepolicy.go:44: return nil,
errors.NewNotImplementedError("ConfigValidator")
Lose the second return

https://codereview.appspot.com/70190050/diff/100001/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/70190050/diff/100001/juju/conn_test.go#newcode143
juju/conn_test.go:143: //Use a state without a nil policy, which will
allow us to set an invalid config
Please leave a space after //, preferably end sentence with a .

https://codereview.appspot.com/70190050/diff/100001/state/api/common/testing/environwatcher.go
File state/api/common/testing/environwatcher.go (right):

https://codereview.appspot.com/70190050/diff/100001/state/api/common/testing/environwatcher.go#newcode92
state/api/common/testing/environwatcher.go:92: err =
s.st.UpdateEnvironConfig(map[string]interface{}{}, []string{"foo"}, nil)
check error

https://codereview.appspot.com/70190050/diff/100001/state/api/common/testing/environwatcher.go#newcode94
state/api/common/testing/environwatcher.go:94: c.Logf("!!!!! %#v",
cf.AllAttrs()["logging-config"])
leftover logging

https://codereview.appspot.com/70190050/diff/100001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/70190050/diff/100001/state/state.go#newcode279
state/state.go:279: type ValidateFn func(updateAttrs
map[string]interface{}, removeAttrs []string, oldConfig *config.Config)
error
This could do with a better name. Out of context (e.g. in godoc), it
wouldn't really be meaningful. I suggest ValidateConfigFunc (Func,
rather than Fn, to be consistent with function types in Go's standard
library).

https://codereview.appspot.com/70190050/diff/100001/state/state.go#newcode316
state/state.go:316: //use validCfg to update settings
Just drop this comment

https://codereview.appspot.com/70190050/diff/100001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/70190050/diff/100001/state/state_test.go#newcode1689
state/state_test.go:1689: if v, found := updateAttrs["agent-version"];
found {
All we really care about is: did the callback get called with the
appropriate arguments, and does its result have the appropriate effect?

So there's some missing coverage there: there's no validation of the
input, and there's no test for a non-nil validator with a nil result
(i.e. has no effect on normal operation).

https://codereview.appspot.com/70190050/diff/100001/state/state_test.go#newcode1818
state/state_test.go:1818: err = settings.Update(nil, bson.D{{"$set",
bson.D{{"name", "foo"}}}})
What is this doing here? Is this because UpdateEnvironConfig won't
change the "name" attribute? If it's needed, you should probably be
using ...

[Jesse Meek (waigani)]

Please take a look.

https://codereview.appspot.com/70190050/diff/100001/environs/statepolicy.go
File environs/statepolicy.go (right):

https://codereview.appspot.com/70190050/diff/100001/environs/statepolicy.go#newcode43
environs/statepolicy.go:43: return provider.(state.ConfigValidator), nil
On 2014/03/17 12:15:56, axw wrote:
> No need to type assert, since EnvironProvider implements
state.ConfigValidator.
> Just "return provider, nil".

Done.

https://codereview.appspot.com/70190050/diff/100001/environs/statepolicy.go#newcode44
environs/statepolicy.go:44: return nil,
errors.NewNotImplementedError("ConfigValidator")
On 2014/03/17 12:15:56, axw wrote:
> Lose the second return

Done.

https://codereview.appspot.com/70190050/diff/100001/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/70190050/diff/100001/juju/conn_test.go#newcode143
juju/conn_test.go:143: //Use a state without a nil policy, which will
allow us to set an invalid config
On 2014/03/17 12:15:56, axw wrote:
> Please leave a space after //, preferably end sentence with a .

Done.

https://codereview.appspot.com/70190050/diff/100001/state/api/common/testing/environwatcher.go
File state/api/common/testing/environwatcher.go (right):

https://codereview.appspot.com/70190050/diff/100001/state/api/common/testing/environwatcher.go#newcode92
state/api/common/testing/environwatcher.go:92: err =
s.st.UpdateEnvironConfig(map[string]interface{}{}, []string{"foo"}, nil)
On 2014/03/17 12:15:56, axw wrote:
> check error

Done.

https://codereview.appspot.com/70190050/diff/100001/state/api/common/testing/environwatcher.go#newcode94
state/api/common/testing/environwatcher.go:94: c.Logf("!!!!! %#v",
cf.AllAttrs()["logging-config"])
On 2014/03/17 12:15:56, axw wrote:
> leftover logging

Done.

https://codereview.appspot.com/70190050/diff/100001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/70190050/diff/100001/state/state.go#newcode279
state/state.go:279: type ValidateFn func(updateAttrs
map[string]interface{}, removeAttrs []string, oldConfig *config.Config)
error
On 2014/03/17 12:15:56, axw wrote:
> This could do with a better name. Out of context (e.g. in godoc), it
wouldn't
> really be meaningful. I suggest ValidateConfigFunc (Func, rather than
Fn, to be
> consistent with function types in Go's standard library).

Done.

https://codereview.appspot.com/70190050/diff/100001/state/state.go#newcode316
state/state.go:316: //use validCfg to update settings
On 2014/03/17 12:15:56, axw wrote:
> Just drop this comment

Done.

https://codereview.appspot.com/70190050/diff/100001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/70190050/diff/100001/state/state_test.go#newcode1689
state/state_test.go:1689: if v, found := updateAttrs["agent-version"];
found {
On 2014/03/17 12:15:56, axw wrote:
> All we really care about is: did the callback get called with the
appropriate
> arguments, and does its result have the appropriate effect?

> So there's some missing coverage there: there's no validation of the
input, and
> there's no test for a non-nil validator with a nil result (i.e. has no
effect on
> normal operati...

[Jesse Meek (waigani)]

https://codereview.appspot.com/70190050/

[Andrew Wilkins (axwalk)]

On 2014/03/17 22:48:37, waigani wrote:

LGTM. Thanks for slogging through it.

https://codereview.appspot.com/70190050/

=== modified file 'cmd/juju/authorisedkeys_test.go'
--- cmd/juju/authorisedkeys_test.go	2014-01-22 22:48:54 +0000
+++ cmd/juju/authorisedkeys_test.go	2014-03-18 02:38:23 +0000
@@ -13,7 +13,6 @@
 	jujutesting "launchpad.net/juju-core/juju/testing"
 	keymanagerserver "launchpad.net/juju-core/state/apiserver/keymanager"
 	keymanagertesting "launchpad.net/juju-core/state/apiserver/keymanager/testing"
-	statetesting "launchpad.net/juju-core/state/testing"
 	coretesting "launchpad.net/juju-core/testing"
 	"launchpad.net/juju-core/testing/testbase"
 	sshtesting "launchpad.net/juju-core/utils/ssh/testing"
@@ -103,7 +102,7 @@
 
 func (s *keySuiteBase) setAuthorisedKeys(c *gc.C, keys ...string) {
 	keyString := strings.Join(keys, "\n")
-	err := statetesting.UpdateConfig(s.State, map[string]interface{}{"authorized-keys": keyString})
+	err := s.State.UpdateEnvironConfig(map[string]interface{}{"authorized-keys": keyString}, nil, nil)
 	c.Assert(err, gc.IsNil)
 	envConfig, err := s.State.EnvironConfig()
 	c.Assert(err, gc.IsNil)
 
=== modified file 'cmd/juju/environment.go'
--- cmd/juju/environment.go	2013-12-17 18:21:26 +0000
+++ cmd/juju/environment.go	2014-03-18 02:38:23 +0000
@@ -162,26 +162,8 @@
 	}
 	defer conn.Close()
 
-	// Here is the magic around setting the attributes:
-	// TODO(thumper): get this magic under test somewhere, and update other call-sites to use it.
-	// Get the existing environment config from the state.
-	oldConfig, err := conn.State.EnvironConfig()
-	if err != nil {
-		return err
-	}
-	// Apply the attributes specified for the command to the state config.
-	newConfig, err := oldConfig.Apply(c.values)
-	if err != nil {
-		return err
-	}
-	// Now validate this new config against the existing config via the provider.
-	provider := conn.Environ.Provider()
-	newProviderConfig, err := provider.Validate(newConfig, oldConfig)
-	if err != nil {
-		return err
-	}
-	// Now try to apply the new validated config.
-	return conn.State.SetEnvironConfig(newProviderConfig, oldConfig)
+	// Update state config with new values
+	return conn.State.UpdateEnvironConfig(c.values, nil, nil)
 }
 
 func (c *SetEnvironmentCommand) Run(ctx *cmd.Context) error {
 
=== modified file 'cmd/juju/environment_test.go'
--- cmd/juju/environment_test.go	2014-02-27 04:43:37 +0000
+++ cmd/juju/environment_test.go	2014-03-18 02:38:23 +0000
@@ -11,6 +11,7 @@
 
 	jujutesting "launchpad.net/juju-core/juju/testing"
 	"launchpad.net/juju-core/provider/dummy"
+	_ "launchpad.net/juju-core/provider/local"
 	"launchpad.net/juju-core/testing"
 )
 
@@ -163,7 +164,7 @@
 
 var immutableConfigTests = map[string]string{
 	"name":          "foo",
-	"type":          "foo",
+	"type":          "local",
 	"firewall-mode": "global",
 	"state-port":    "1",
 	"api-port":      "666",
 
=== modified file 'cmd/juju/upgradejuju_test.go'
--- cmd/juju/upgradejuju_test.go	2014-03-13 23:30:56 +0000
+++ cmd/juju/upgradejuju_test.go	2014-03-18 02:38:23 +0000
@@ -322,20 +322,16 @@
 		}
 
 		// Set up state and environ, and run the command.
-		oldcfg, err := s.State.EnvironConfig()
-		c.Assert(err, gc.IsNil)
 		toolsDir := c.MkDir()
-		cfg, err := oldcfg.Apply(map[string]interface{}{
+		updateAttrs := map[string]interface{}{
 			"agent-version":      test.agentVersion,
 			"tools-metadata-url": "file://" + toolsDir,
-		})
-		c.Assert(err, gc.IsNil)
-		err = s.State.SetEnvironConfig(cfg, oldcfg)
+		}
+		err := s.State.UpdateEnvironConfig(updateAttrs, nil, nil)
 		c.Assert(err, gc.IsNil)
 		versions := make([]version.Binary, len(test.tools))
 		for i, v := range test.tools {
 			versions[i] = version.MustParseBinary(v)
-
 		}
 		if len(versions) > 0 {
 			envtesting.MustUploadFakeToolsVersions(s.Conn.Environ.Storage(), versions...)
@@ -353,7 +349,7 @@
 		}
 
 		// Check expected changes to environ/state.
-		cfg, err = s.State.EnvironConfig()
+		cfg, err := s.State.EnvironConfig()
 		c.Check(err, gc.IsNil)
 		agentVersion, ok := cfg.AgentVersion()
 		c.Check(ok, gc.Equals, true)
@@ -408,14 +404,11 @@
 func (s *UpgradeJujuSuite) Reset(c *gc.C) {
 	s.JujuConnSuite.Reset(c)
 	envtesting.RemoveTools(c, s.Conn.Environ.Storage())
-	oldcfg, err := s.State.EnvironConfig()
-	c.Assert(err, gc.IsNil)
-	cfg, err := oldcfg.Apply(map[string]interface{}{
+	updateAttrs := map[string]interface{}{
 		"default-series": "raring",
 		"agent-version":  "1.2.3",
-	})
-	c.Assert(err, gc.IsNil)
-	err = s.State.SetEnvironConfig(cfg, oldcfg)
+	}
+	err := s.State.UpdateEnvironConfig(updateAttrs, nil, nil)
 	c.Assert(err, gc.IsNil)
 	s.toolsDir = c.MkDir()
 }
 
=== modified file 'cmd/jujud/machine_test.go'
--- cmd/jujud/machine_test.go	2014-03-13 07:54:56 +0000
+++ cmd/jujud/machine_test.go	2014-03-18 02:38:23 +0000
@@ -32,7 +32,6 @@
 	"launchpad.net/juju-core/state/api/params"
 	apirsyslog "launchpad.net/juju-core/state/api/rsyslog"
 	charmtesting "launchpad.net/juju-core/state/apiserver/charmrevisionupdater/testing"
-	statetesting "launchpad.net/juju-core/state/testing"
 	"launchpad.net/juju-core/state/watcher"
 	"launchpad.net/juju-core/testing"
 	coretesting "launchpad.net/juju-core/testing"
@@ -635,7 +634,7 @@
 
 	// Update the keys in the environment.
 	sshKey := sshtesting.ValidKeyOne.Key + " user@host"
-	err := statetesting.UpdateConfig(s.BackingState, map[string]interface{}{"authorized-keys": sshKey})
+	err := s.BackingState.UpdateEnvironConfig(map[string]interface{}{"authorized-keys": sshKey}, nil, nil)
 	c.Assert(err, gc.IsNil)
 
 	// Wait for ssh keys file to be updated.
@@ -723,19 +722,15 @@
 
 	s.primeAgent(c, version.Current, state.JobHostUnits)
 	// Make sure there are some proxy settings to write.
-	oldConfig, err := s.State.EnvironConfig()
-	c.Assert(err, gc.IsNil)
-
 	proxySettings := osenv.ProxySettings{
 		Http:  "http proxy",
 		Https: "https proxy",
 		Ftp:   "ftp proxy",
 	}
 
-	envConfig, err := oldConfig.Apply(config.ProxyConfigMap(proxySettings))
-	c.Assert(err, gc.IsNil)
+	updateAttrs := config.ProxyConfigMap(proxySettings)
 
-	err = s.State.SetEnvironConfig(envConfig, oldConfig)
+	err := s.State.UpdateEnvironConfig(updateAttrs, nil, nil)
 	c.Assert(err, gc.IsNil)
 
 	s.assertJobWithAPI(c, state.JobHostUnits, func(conf agent.Config, st *api.State) {
 
=== modified file 'environs/config.go'
--- environs/config.go	2014-03-05 19:41:34 +0000
+++ environs/config.go	2014-03-18 02:38:23 +0000
@@ -135,13 +135,13 @@
 }
 
 // Provider returns the previously registered provider with the given type.
-func Provider(typ string) (EnvironProvider, error) {
-	if alias, ok := providerAliases[typ]; ok {
-		typ = alias
+func Provider(providerType string) (EnvironProvider, error) {
+	if alias, ok := providerAliases[providerType]; ok {
+		providerType = alias
 	}
-	p, ok := providers[typ]
+	p, ok := providers[providerType]
 	if !ok {
-		return nil, fmt.Errorf("no registered provider for %q", typ)
+		return nil, fmt.Errorf("no registered provider for %q", providerType)
 	}
 	return p, nil
 }
 
=== modified file 'environs/config/config.go'
--- environs/config/config.go	2014-03-14 03:44:08 +0000
+++ environs/config/config.go	2014-03-18 02:38:23 +0000
@@ -662,6 +662,15 @@
 	return allAttrs
 }
 
+// Remove returns a new configuration that has the attributes of c minus attrs.
+func (c *Config) Remove(attrs []string) (*Config, error) {
+	defined := c.AllAttrs()
+	for _, k := range attrs {
+		delete(defined, k)
+	}
+	return New(NoDefaults, defined)
+}
+
 // Apply returns a new configuration that has the attributes of c plus attrs.
 func (c *Config) Apply(attrs map[string]interface{}) (*Config, error) {
 	defined := c.AllAttrs()
 
=== modified file 'environs/statepolicy.go'
--- environs/statepolicy.go	2014-02-19 06:31:52 +0000
+++ environs/statepolicy.go	2014-03-18 02:38:23 +0000
@@ -27,8 +27,18 @@
 	if err != nil {
 		return nil, err
 	}
+
 	if p, ok := env.(state.Prechecker); ok {
 		return p, nil
 	}
 	return nil, errors.NewNotImplementedError("Prechecker")
 }
+
+func (environStatePolicy) ConfigValidator(providerType string) (state.ConfigValidator, error) {
+	provider, err := Provider(providerType)
+	if err != nil {
+		return nil, err
+	}
+
+	return provider, nil
+}
 
=== modified file 'environs/testing/tools.go'
--- environs/testing/tools.go	2014-03-13 22:47:05 +0000
+++ environs/testing/tools.go	2014-03-18 02:38:23 +0000
@@ -442,12 +442,6 @@
 	}}
 
 func SetSSLHostnameVerification(c *gc.C, st *state.State, SSLHostnameVerification bool) {
-	envConfig, err := st.EnvironConfig()
-	c.Assert(err, gc.IsNil)
-	attrs := envConfig.AllAttrs()
-	attrs["ssl-hostname-verification"] = SSLHostnameVerification
-	newConfig, err := config.New(config.NoDefaults, attrs)
-	c.Assert(err, gc.IsNil)
-	err = st.SetEnvironConfig(newConfig, envConfig)
+	err := st.UpdateEnvironConfig(map[string]interface{}{"ssl-hostname-verification": SSLHostnameVerification}, nil, nil)
 	c.Assert(err, gc.IsNil)
 }
 
=== modified file 'juju/conn.go'
--- juju/conn.go	2014-03-07 14:15:28 +0000
+++ juju/conn.go	2014-03-18 02:38:23 +0000
@@ -13,7 +13,6 @@
 
 	"launchpad.net/juju-core/charm"
 	"launchpad.net/juju-core/environs"
-	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/environs/configstore"
 	"launchpad.net/juju-core/errors"
 	"launchpad.net/juju-core/juju/osenv"
@@ -144,20 +143,17 @@
 	if err != nil {
 		return err
 	}
+	secretAttrs := make(map[string]interface{})
 	attrs := cfg.AllAttrs()
 	for k, v := range secrets {
 		if _, exists := attrs[k]; exists {
 			// Environment already has secrets. Won't send again.
 			return nil
 		} else {
-			attrs[k] = v
+			secretAttrs[k] = v
 		}
 	}
-	newcfg, err := config.New(config.NoDefaults, attrs)
-	if err != nil {
-		return err
-	}
-	return c.State.SetEnvironConfig(newcfg, cfg)
+	return c.State.UpdateEnvironConfig(secretAttrs, nil, nil)
 }
 
 // PutCharm uploads the given charm to provider storage, and adds a
 
=== modified file 'juju/conn_test.go'
--- juju/conn_test.go	2014-03-13 07:54:56 +0000
+++ juju/conn_test.go	2014-03-18 02:38:23 +0000
@@ -140,7 +140,8 @@
 	info, _, err := env.StateInfo()
 	c.Assert(err, gc.IsNil)
 	info.Password = utils.UserPasswordHash("side-effect secret", utils.CompatSalt)
-	st, err := state.Open(info, state.DefaultDialOpts(), environs.NewStatePolicy())
+	// Use a state without a nil policy, which will allow us to set an invalid config.
+	st, err := state.Open(info, state.DefaultDialOpts(), state.Policy(nil))
 	c.Assert(err, gc.IsNil)
 	defer assertClose(c, st)
 
@@ -151,15 +152,8 @@
 
 	// Remove the secret from state, and then make sure it gets
 	// pushed back again.
-	attrs = statecfg.AllAttrs()
-	delete(attrs, "secret")
-	newcfg, err := config.New(config.NoDefaults, attrs)
-	c.Assert(err, gc.IsNil)
-	err = st.SetEnvironConfig(newcfg, statecfg)
-	c.Assert(err, gc.IsNil)
-	statecfg, err = st.EnvironConfig()
-	c.Assert(err, gc.IsNil)
-	c.Assert(statecfg.UnknownAttrs()["secret"], gc.IsNil)
+	err = st.UpdateEnvironConfig(map[string]interface{}{}, []string{"secret"}, nil)
+	c.Assert(err, gc.IsNil)
 
 	// Make a new Conn, which will push the secrets.
 	conn, err := juju.NewConn(env)
 
=== modified file 'juju/testing/conn.go'
--- juju/testing/conn.go	2014-02-21 03:57:49 +0000
+++ juju/testing/conn.go	2014-03-18 02:38:23 +0000
@@ -62,6 +62,7 @@
 	oldHome      string
 	oldJujuHome  string
 	environ      environs.Environ
+	DummyConfig  testing.Attrs
 }
 
 const AdminSecret = "dummy-secret"
@@ -220,7 +221,10 @@
 }
 
 func (s *JujuConnSuite) writeSampleConfig(c *gc.C, path string) {
-	attrs := dummy.SampleConfig().Merge(testing.Attrs{
+	if s.DummyConfig == nil {
+		s.DummyConfig = dummy.SampleConfig()
+	}
+	attrs := s.DummyConfig.Merge(testing.Attrs{
 		"admin-secret":  AdminSecret,
 		"agent-version": version.Current.Number.String(),
 	}).Delete("name")
 
=== modified file 'juju/testing/repo.go'
--- juju/testing/repo.go	2014-01-28 17:07:55 +0000
+++ juju/testing/repo.go	2014-03-18 02:38:23 +0000
@@ -25,11 +25,8 @@
 	s.JujuConnSuite.SetUpTest(c)
 	// Change the environ's config to ensure we're using the one in state,
 	// not the one in the local environments.yaml
-	oldcfg, err := s.State.EnvironConfig()
-	c.Assert(err, gc.IsNil)
-	cfg, err := oldcfg.Apply(map[string]interface{}{"default-series": "precise"})
-	c.Assert(err, gc.IsNil)
-	err = s.State.SetEnvironConfig(cfg, oldcfg)
+	updateAttrs := map[string]interface{}{"default-series": "precise"}
+	err := s.State.UpdateEnvironConfig(updateAttrs, nil, nil)
 	c.Assert(err, gc.IsNil)
 	s.RepoPath = os.Getenv("JUJU_REPOSITORY")
 	repoPath := c.MkDir()
 
=== modified file 'juju/testing/utils.go'
--- juju/testing/utils.go	2014-01-22 19:28:08 +0000
+++ juju/testing/utils.go	2014-03-18 02:38:23 +0000
@@ -6,24 +6,10 @@
 import (
 	gc "launchpad.net/gocheck"
 
-	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/state"
-	coretesting "launchpad.net/juju-core/testing"
 )
 
-// ChangeEnvironConfig applies the given change function
-// to the attributes from st.EnvironConfig and
-// sets the state's environment configuration to the result.
-func ChangeEnvironConfig(c *gc.C, st *state.State, change func(coretesting.Attrs) coretesting.Attrs) {
-	cfg, err := st.EnvironConfig()
-	c.Assert(err, gc.IsNil)
-	newCfg, err := config.New(config.NoDefaults, change(cfg.AllAttrs()))
-	c.Assert(err, gc.IsNil)
-	err = st.SetEnvironConfig(newCfg, cfg)
-	c.Assert(err, gc.IsNil)
-}
-
 // AddStateServerMachine adds a "state server" machine to the state so
 // that State.Addresses and State.APIAddresses will work. It returns the
 // added machine. The addresses that those methods will return bear no
 
=== modified file 'state/api/common/testing/environwatcher.go'
--- state/api/common/testing/environwatcher.go	2014-03-13 07:54:56 +0000
+++ state/api/common/testing/environwatcher.go	2014-03-18 02:38:23 +0000
@@ -76,17 +76,26 @@
 	// Initial event.
 	wc.AssertOneChange()
 
-	// Change the environment configuration, check it's detected.
-	attrs := envConfig.AllAttrs()
-	attrs["type"] = "blah"
-	newConfig, err := config.New(config.NoDefaults, attrs)
-	c.Assert(err, gc.IsNil)
-	err = s.st.SetEnvironConfig(newConfig, envConfig)
+	// Change the environment configuration by updating an existing attribute, check it's detected.
+	newAttrs := map[string]interface{}{"logging-config": "juju=ERROR"}
+	err = s.st.UpdateEnvironConfig(newAttrs, nil, nil)
+	c.Assert(err, gc.IsNil)
+	wc.AssertOneChange()
+
+	// Change the environment configuration by adding a new attribute, check it's detected.
+	newAttrs = map[string]interface{}{"foo": "bar"}
+	err = s.st.UpdateEnvironConfig(newAttrs, nil, nil)
+	c.Assert(err, gc.IsNil)
+	wc.AssertOneChange()
+
+	// Change the environment configuration by removing an attribute, check it's detected.
+	err = s.st.UpdateEnvironConfig(map[string]interface{}{}, []string{"foo"}, nil)
 	c.Assert(err, gc.IsNil)
 	wc.AssertOneChange()
 
 	// Change it back to the original config.
-	err = s.st.SetEnvironConfig(envConfig, newConfig)
+	oldAttrs := map[string]interface{}{"logging-config": envConfig.AllAttrs()["logging-config"]}
+	err = s.st.UpdateEnvironConfig(oldAttrs, nil, nil)
 	c.Assert(err, gc.IsNil)
 	wc.AssertOneChange()
 
 
=== modified file 'state/api/firewaller/state_test.go'
--- state/api/firewaller/state_test.go	2014-03-13 07:54:56 +0000
+++ state/api/firewaller/state_test.go	2014-03-18 02:38:23 +0000
@@ -7,7 +7,6 @@
 	jc "github.com/juju/testing/checkers"
 	gc "launchpad.net/gocheck"
 
-	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/state"
 	statetesting "launchpad.net/juju-core/state/testing"
@@ -81,16 +80,14 @@
 	wc.AssertOneChange()
 
 	// Change the environment configuration, check it's detected.
-	attrs := envConfig.AllAttrs()
-	attrs["type"] = "blah"
-	newConfig, err := config.New(config.NoDefaults, attrs)
-	c.Assert(err, gc.IsNil)
-	err = s.State.SetEnvironConfig(newConfig, envConfig)
+	newAttrs := map[string]interface{}{"logging-config": "juju=ERROR"}
+	err = s.State.UpdateEnvironConfig(newAttrs, nil, nil)
 	c.Assert(err, gc.IsNil)
 	wc.AssertOneChange()
 
 	// Change it back to the original config.
-	err = s.State.SetEnvironConfig(envConfig, newConfig)
+	oldAttrs := map[string]interface{}{"logging-config": envConfig.AllAttrs()["logging-config"]}
+	err = s.State.UpdateEnvironConfig(oldAttrs, nil, nil)
 	c.Assert(err, gc.IsNil)
 	wc.AssertOneChange()
 
 
=== modified file 'state/api/keymanager/client_test.go'
--- state/api/keymanager/client_test.go	2014-03-10 13:13:43 +0000
+++ state/api/keymanager/client_test.go	2014-03-18 02:38:23 +0000
@@ -14,7 +14,6 @@
 	"launchpad.net/juju-core/state/api/params"
 	keymanagerserver "launchpad.net/juju-core/state/apiserver/keymanager"
 	keymanagertesting "launchpad.net/juju-core/state/apiserver/keymanager/testing"
-	"launchpad.net/juju-core/state/testing"
 	"launchpad.net/juju-core/utils/ssh"
 	sshtesting "launchpad.net/juju-core/utils/ssh/testing"
 )
@@ -35,7 +34,7 @@
 }
 
 func (s *keymanagerSuite) setAuthorisedKeys(c *gc.C, keys string) {
-	err := testing.UpdateConfig(s.BackingState, map[string]interface{}{"authorized-keys": keys})
+	err := s.BackingState.UpdateEnvironConfig(map[string]interface{}{"authorized-keys": keys}, nil, nil)
 	c.Assert(err, gc.IsNil)
 }
 
 
=== modified file 'state/api/keyupdater/authorisedkeys_test.go'
--- state/api/keyupdater/authorisedkeys_test.go	2014-01-24 12:39:48 +0000
+++ state/api/keyupdater/authorisedkeys_test.go	2014-03-18 02:38:23 +0000
@@ -54,7 +54,7 @@
 }
 
 func (s *keyupdaterSuite) setAuthorisedKeys(c *gc.C, keys string) {
-	err := testing.UpdateConfig(s.BackingState, map[string]interface{}{"authorized-keys": keys})
+	err := s.BackingState.UpdateEnvironConfig(map[string]interface{}{"authorized-keys": keys}, nil, nil)
 	c.Assert(err, gc.IsNil)
 }
 
 
=== modified file 'state/api/logger/logger_test.go'
--- state/api/logger/logger_test.go	2013-09-17 03:12:12 +0000
+++ state/api/logger/logger_test.go	2014-03-18 02:38:23 +0000
@@ -50,7 +50,7 @@
 }
 
 func (s *loggerSuite) setLoggingConfig(c *gc.C, loggingConfig string) {
-	err := testing.UpdateConfig(s.BackingState, map[string]interface{}{"logging-config": loggingConfig})
+	err := s.BackingState.UpdateEnvironConfig(map[string]interface{}{"logging-config": loggingConfig}, nil, nil)
 	c.Assert(err, gc.IsNil)
 }
 
 
=== modified file 'state/apiserver/client/client.go'
--- state/apiserver/client/client.go	2014-03-14 10:28:40 +0000
+++ state/apiserver/client/client.go	2014-03-18 02:38:23 +0000
@@ -761,38 +761,23 @@
 // EnvironmentSet implements the server-side part of the
 // set-environment CLI command.
 func (c *Client) EnvironmentSet(args params.EnvironmentSet) error {
-	// TODO(dimitern,thumper): 2013-11-06 bug #1167616
-	// SetEnvironConfig should take both new and old configs.
-
-	// Get the existing environment config from the state.
-	oldConfig, err := c.api.state.EnvironConfig()
-	if err != nil {
-		return err
-	}
 	// Make sure we don't allow changing agent-version.
-	if v, found := args.Config["agent-version"]; found {
-		oldVersion, _ := oldConfig.AgentVersion()
-		if v != oldVersion.String() {
-			return fmt.Errorf("agent-version cannot be changed")
+	checkAgentVersion := func(updateAttrs map[string]interface{}, removeAttrs []string, oldConfig *config.Config) error {
+		if v, found := updateAttrs["agent-version"]; found {
+			oldVersion, _ := oldConfig.AgentVersion()
+			if v != oldVersion.String() {
+				return fmt.Errorf("agent-version cannot be changed")
+			}
 		}
-	}
-	// Apply the attributes specified for the command to the state config.
-	newConfig, err := oldConfig.Apply(args.Config)
-	if err != nil {
-		return err
-	}
-	env, err := environs.New(oldConfig)
-	if err != nil {
-		return err
-	}
-	// Now validate this new config against the existing config via the provider.
-	provider := env.Provider()
-	newProviderConfig, err := provider.Validate(newConfig, oldConfig)
-	if err != nil {
-		return err
-	}
-	// Now try to apply the new validated config.
-	return c.api.state.SetEnvironConfig(newProviderConfig, oldConfig)
+		return nil
+	}
+
+	// TODO(waigani) 2014-3-11 #1167616
+	// Add a txn retry loop to ensure that the settings on disk have not
+	// changed underneath us.
+
+	return c.api.state.UpdateEnvironConfig(args.Config, nil, checkAgentVersion)
+
 }
 
 // SetEnvironAgentVersion sets the environment agent version.
 
=== modified file 'state/apiserver/client/client_test.go'
--- state/apiserver/client/client_test.go	2014-03-14 10:28:40 +0000
+++ state/apiserver/client/client_test.go	2014-03-18 02:38:23 +0000
@@ -1741,18 +1741,9 @@
 	store, restore := makeMockCharmStore()
 	defer restore()
 
-	oldConfig, err := s.State.EnvironConfig()
-	c.Assert(err, gc.IsNil)
-
-	attrs := coretesting.Attrs(oldConfig.AllAttrs())
-	attrs = attrs.Merge(coretesting.Attrs{
-		"charm-store-auth": "token=value",
-		"test-mode":        true})
-
-	cfg, err := config.New(config.NoDefaults, attrs)
-	c.Assert(err, gc.IsNil)
-
-	err = s.State.SetEnvironConfig(cfg, oldConfig)
+	attrs := map[string]interface{}{"charm-store-auth": "token=value",
+		"test-mode": true}
+	err := s.State.UpdateEnvironConfig(attrs, nil, nil)
 	c.Assert(err, gc.IsNil)
 
 	curl, _ := addCharm(c, store, "dummy")
 
=== modified file 'state/apiserver/keymanager/keymanager.go'
--- state/apiserver/keymanager/keymanager.go	2014-03-05 19:41:34 +0000
+++ state/apiserver/keymanager/keymanager.go	2014-03-18 02:38:23 +0000
@@ -144,14 +144,14 @@
 	return keyInfo
 }
 
-func (api *KeyManagerAPI) writeSSHKeys(currentConfig *config.Config, sshKeys []string) error {
+func (api *KeyManagerAPI) writeSSHKeys(sshKeys []string) error {
 	// Write out the new keys.
 	keyStr := strings.Join(sshKeys, "\n")
-	newConfig, err := currentConfig.Apply(map[string]interface{}{config.AuthKeysConfig: keyStr})
-	if err != nil {
-		return fmt.Errorf("creating new environ config: %v", err)
-	}
-	err = api.state.SetEnvironConfig(newConfig, currentConfig)
+	attrs := map[string]interface{}{config.AuthKeysConfig: keyStr}
+	// TODO(waigani) 2014-03-17 bug #1293324
+	// Pass in validation to ensure SSH keys
+	// have not changed underfoot
+	err := api.state.UpdateEnvironConfig(attrs, nil, nil)
 	if err != nil {
 		return fmt.Errorf("writing environ config: %v", err)
 	}
@@ -159,12 +159,12 @@
 }
 
 // currentKeyDataForAdd gathers data used when adding ssh keys.
-func (api *KeyManagerAPI) currentKeyDataForAdd() (keys []string, fingerprints *set.Strings, cfg *config.Config, err error) {
+func (api *KeyManagerAPI) currentKeyDataForAdd() (keys []string, fingerprints *set.Strings, err error) {
 	fp := set.NewStrings()
 	fingerprints = &fp
-	cfg, err = api.state.EnvironConfig()
+	cfg, err := api.state.EnvironConfig()
 	if err != nil {
-		return nil, nil, nil, err
+		return nil, nil, fmt.Errorf("reading current key data: %v", err)
 	}
 	keys = ssh.SplitAuthorisedKeys(cfg.AuthorizedKeys())
 	for _, key := range keys {
@@ -174,7 +174,7 @@
 		}
 		fingerprints.Add(fingerprint)
 	}
-	return keys, fingerprints, cfg, nil
+	return keys, fingerprints, nil
 }
 
 // AddKeys adds new authorised ssh keys for the specified user.
@@ -195,7 +195,7 @@
 	}
 
 	// For now, authorised keys are global, common to all users.
-	sshKeys, currentFingerprints, currentConfig, err := api.currentKeyDataForAdd()
+	sshKeys, currentFingerprints, err := api.currentKeyDataForAdd()
 	if err != nil {
 		return params.ErrorResults{}, common.ServerError(fmt.Errorf("reading current key data: %v", err))
 	}
@@ -214,7 +214,7 @@
 		}
 		sshKeys = append(sshKeys, key)
 	}
-	err = api.writeSSHKeys(currentConfig, sshKeys)
+	err = api.writeSSHKeys(sshKeys)
 	if err != nil {
 		return params.ErrorResults{}, common.ServerError(err)
 	}
@@ -278,7 +278,7 @@
 	}
 
 	// For now, authorised keys are global, common to all users.
-	sshKeys, currentFingerprints, currentConfig, err := api.currentKeyDataForAdd()
+	sshKeys, currentFingerprints, err := api.currentKeyDataForAdd()
 	if err != nil {
 		return params.ErrorResults{}, common.ServerError(fmt.Errorf("reading current key data: %v", err))
 	}
@@ -297,7 +297,7 @@
 		}
 		sshKeys = append(sshKeys, keyInfo.key)
 	}
-	err = api.writeSSHKeys(currentConfig, sshKeys)
+	err = api.writeSSHKeys(sshKeys)
 	if err != nil {
 		return params.ErrorResults{}, common.ServerError(err)
 	}
@@ -306,13 +306,13 @@
 
 // currentKeyDataForAdd gathers data used when deleting ssh keys.
 func (api *KeyManagerAPI) currentKeyDataForDelete() (
-	keys map[string]string, invalidKeys []string, comments map[string]string, cfg *config.Config, err error) {
+	keys map[string]string, invalidKeys []string, comments map[string]string, err error) {
 
-	// For now, authorised keys are global, common to all users.
-	cfg, err = api.state.EnvironConfig()
+	cfg, err := api.state.EnvironConfig()
 	if err != nil {
-		return nil, nil, nil, nil, fmt.Errorf("reading current key data: %v", err)
+		return nil, nil, nil, fmt.Errorf("reading current key data: %v", err)
 	}
+	// For now, authorised keys are global, common to all users.
 	existingSSHKeys := ssh.SplitAuthorisedKeys(cfg.AuthorizedKeys())
 
 	// Build up a map of keys indexed by fingerprint, and fingerprints indexed by comment
@@ -332,7 +332,7 @@
 			comments[comment] = fingerprint
 		}
 	}
-	return keys, invalidKeys, comments, cfg, nil
+	return keys, invalidKeys, comments, nil
 }
 
 // DeleteKeys deletes the authorised ssh keys for the specified user.
@@ -352,7 +352,7 @@
 		return params.ErrorResults{}, common.ServerError(common.ErrPerm)
 	}
 
-	sshKeys, invalidKeys, keyComments, currentConfig, err := api.currentKeyDataForDelete()
+	sshKeys, invalidKeys, keyComments, err := api.currentKeyDataForDelete()
 	if err != nil {
 		return params.ErrorResults{}, common.ServerError(fmt.Errorf("reading current key data: %v", err))
 	}
@@ -382,7 +382,7 @@
 		return params.ErrorResults{}, common.ServerError(stderrors.New("cannot delete all keys"))
 	}
 
-	err = api.writeSSHKeys(currentConfig, keysToWrite)
+	err = api.writeSSHKeys(keysToWrite)
 	if err != nil {
 		return params.ErrorResults{}, common.ServerError(err)
 	}
 
=== modified file 'state/apiserver/keymanager/keymanager_test.go'
--- state/apiserver/keymanager/keymanager_test.go	2014-03-10 13:13:43 +0000
+++ state/apiserver/keymanager/keymanager_test.go	2014-03-18 02:38:23 +0000
@@ -16,7 +16,6 @@
 	"launchpad.net/juju-core/state/apiserver/keymanager"
 	keymanagertesting "launchpad.net/juju-core/state/apiserver/keymanager/testing"
 	apiservertesting "launchpad.net/juju-core/state/apiserver/testing"
-	statetesting "launchpad.net/juju-core/state/testing"
 	"launchpad.net/juju-core/utils/ssh"
 	sshtesting "launchpad.net/juju-core/utils/ssh/testing"
 )
@@ -78,7 +77,7 @@
 }
 
 func (s *keyManagerSuite) setAuthorisedKeys(c *gc.C, keys string) {
-	err := statetesting.UpdateConfig(s.State, map[string]interface{}{"authorized-keys": keys})
+	err := s.State.UpdateEnvironConfig(map[string]interface{}{"authorized-keys": keys}, nil, nil)
 	c.Assert(err, gc.IsNil)
 	envConfig, err := s.State.EnvironConfig()
 	c.Assert(err, gc.IsNil)
 
=== modified file 'state/apiserver/keyupdater/authorisedkeys_test.go'
--- state/apiserver/keyupdater/authorisedkeys_test.go	2013-12-12 05:12:49 +0000
+++ state/apiserver/keyupdater/authorisedkeys_test.go	2014-03-18 02:38:23 +0000
@@ -73,7 +73,7 @@
 }
 
 func (s *authorisedKeysSuite) setAuthorizedKeys(c *gc.C, keys string) {
-	err := statetesting.UpdateConfig(s.State, map[string]interface{}{"authorized-keys": keys})
+	err := s.State.UpdateEnvironConfig(map[string]interface{}{"authorized-keys": keys}, nil, nil)
 	c.Assert(err, gc.IsNil)
 	envConfig, err := s.State.EnvironConfig()
 	c.Assert(err, gc.IsNil)
 
=== modified file 'state/apiserver/logger/logger_test.go'
--- state/apiserver/logger/logger_test.go	2014-01-22 02:08:33 +0000
+++ state/apiserver/logger/logger_test.go	2014-03-18 02:38:23 +0000
@@ -74,7 +74,7 @@
 }
 
 func (s *loggerSuite) setLoggingConfig(c *gc.C, loggingConfig string) {
-	err := statetesting.UpdateConfig(s.State, map[string]interface{}{"logging-config": loggingConfig})
+	err := s.State.UpdateEnvironConfig(map[string]interface{}{"logging-config": loggingConfig}, nil, nil)
 	c.Assert(err, gc.IsNil)
 	envConfig, err := s.State.EnvironConfig()
 	c.Assert(err, gc.IsNil)
 
=== modified file 'state/apiserver/provisioner/provisioner_test.go'
--- state/apiserver/provisioner/provisioner_test.go	2014-03-17 13:22:55 +0000
+++ state/apiserver/provisioner/provisioner_test.go	2014-03-18 02:38:23 +0000
@@ -683,13 +683,10 @@
 }
 
 func (s *withoutStateServerSuite) TestContainerConfig(c *gc.C) {
-	cfg, err := s.State.EnvironConfig()
-	c.Assert(err, gc.IsNil)
-	newCfg, err := cfg.Apply(map[string]interface{}{
+	attrs := map[string]interface{}{
 		"http-proxy": "http://proxy.example.com:9000",
-	})
-	c.Assert(err, gc.IsNil)
-	err = s.State.SetEnvironConfig(newCfg, cfg)
+	}
+	err := s.State.UpdateEnvironConfig(attrs, nil, nil)
 	c.Assert(err, gc.IsNil)
 	expectedProxy := osenv.ProxySettings{
 		Http: "http://proxy.example.com:9000",
 
=== modified file 'state/apiserver/rsyslog/rsyslog.go'
--- state/apiserver/rsyslog/rsyslog.go	2014-02-27 03:46:35 +0000
+++ state/apiserver/rsyslog/rsyslog.go	2014-03-18 02:38:23 +0000
@@ -40,17 +40,9 @@
 		result.Error = common.ServerError(err)
 		return result, nil
 	}
-	old, err := api.st.EnvironConfig()
-	if err != nil {
-		return params.ErrorResult{}, err
-	}
-	cfg, err := old.Apply(map[string]interface{}{"rsyslog-ca-cert": string(args.CACert)})
-	if err != nil {
+	attrs := map[string]interface{}{"rsyslog-ca-cert": string(args.CACert)}
+	if err := api.st.UpdateEnvironConfig(attrs, nil, nil); err != nil {
 		result.Error = common.ServerError(err)
-	} else {
-		if err := api.st.SetEnvironConfig(cfg, old); err != nil {
-			result.Error = common.ServerError(err)
-		}
 	}
 	return result, nil
 }
 
=== added file 'state/configvalidator_test.go'
--- state/configvalidator_test.go	1970-01-01 00:00:00 +0000
+++ state/configvalidator_test.go	2014-03-18 02:38:23 +0000
@@ -0,0 +1,114 @@
+// Copyright 2014 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package state_test
+
+import (
+	gc "launchpad.net/gocheck"
+	"launchpad.net/juju-core/environs/config"
+	"launchpad.net/juju-core/errors"
+	"launchpad.net/juju-core/state"
+	coretesting "launchpad.net/juju-core/testing"
+)
+
+type ConfigValidatorSuite struct {
+	ConnSuite
+	configValidator mockConfigValidator
+}
+
+var _ = gc.Suite(&ConfigValidatorSuite{})
+
+type mockConfigValidator struct {
+	validateError error
+	validateCfg   *config.Config
+	validateOld   *config.Config
+	validateValid *config.Config
+}
+
+// To test UpdateEnvironConfig updates state, Validate returns a config
+// different to both input configs
+func mockValidCfg() (valid *config.Config, err error) {
+	cfg, err := config.New(config.UseDefaults, coretesting.FakeConfig())
+	if err != nil {
+		return nil, err
+	}
+	valid, err = cfg.Apply(map[string]interface{}{
+		"arbitrary-key": "cptn-marvel",
+	})
+	if err != nil {
+		return nil, err
+	}
+	return valid, nil
+}
+
+func (p *mockConfigValidator) Validate(cfg, old *config.Config) (valid *config.Config, err error) {
+	p.validateCfg = cfg
+	p.validateOld = old
+	p.validateValid, p.validateError = mockValidCfg()
+	return p.validateValid, p.validateError
+}
+
+func (s *ConfigValidatorSuite) SetUpTest(c *gc.C) {
+	s.ConnSuite.SetUpTest(c)
+	s.configValidator = mockConfigValidator{}
+	s.policy.getConfigValidator = func(string) (state.ConfigValidator, error) {
+		return &s.configValidator, nil
+	}
+}
+
+func (s *ConfigValidatorSuite) updateEnvironConfig(c *gc.C) error {
+	updateAttrs := map[string]interface{}{
+		"authorized-keys": "different-keys",
+		"arbitrary-key":   "shazam!",
+	}
+	return s.State.UpdateEnvironConfig(updateAttrs, nil, nil)
+}
+
+func (s *ConfigValidatorSuite) TestConfigValidate(c *gc.C) {
+	err := s.updateEnvironConfig(c)
+	c.Assert(err, gc.IsNil)
+}
+
+func (s *ConfigValidatorSuite) TestUpdateEnvironConfigFailsOnConfigValidateError(c *gc.C) {
+	var configValidatorErr error
+	s.policy.getConfigValidator = func(string) (state.ConfigValidator, error) {
+		configValidatorErr = errors.NotFoundf("")
+		return &s.configValidator, configValidatorErr
+	}
+
+	err := s.updateEnvironConfig(c)
+	c.Assert(err, gc.ErrorMatches, " not found")
+}
+
+func (s *ConfigValidatorSuite) TestUpdateEnvironConfigUpdatesState(c *gc.C) {
+	s.updateEnvironConfig(c)
+	stateCfg, err := s.State.EnvironConfig()
+	c.Assert(err, gc.IsNil)
+	newValidCfg, err := mockValidCfg()
+	c.Assert(err, gc.IsNil)
+	c.Assert(stateCfg.AllAttrs()["arbitrary-key"], gc.Equals, newValidCfg.AllAttrs()["arbitrary-key"])
+}
+
+func (s *ConfigValidatorSuite) TestConfigValidateUnimplemented(c *gc.C) {
+	var configValidatorErr error
+	s.policy.getConfigValidator = func(string) (state.ConfigValidator, error) {
+		return nil, configValidatorErr
+	}
+
+	err := s.updateEnvironConfig(c)
+	c.Assert(err, gc.ErrorMatches, "policy returned nil configValidator without an error")
+	configValidatorErr = errors.NewNotImplementedError("Validator")
+	err = s.updateEnvironConfig(c)
+	c.Assert(err, gc.IsNil)
+}
+
+func (s *ConfigValidatorSuite) TestConfigValidateNoPolicy(c *gc.C) {
+	s.policy.getConfigValidator = func(providerType string) (state.ConfigValidator, error) {
+		c.Errorf("should not have been invoked")
+		return nil, nil
+	}
+
+	state.SetPolicy(s.State, nil)
+	err := s.updateEnvironConfig(c)
+	c.Assert(err, gc.IsNil)
+}
 
=== modified file 'state/conn_test.go'
--- state/conn_test.go	2014-03-13 13:42:50 +0000
+++ state/conn_test.go	2014-03-18 02:38:23 +0000
@@ -97,7 +97,8 @@
 }
 
 type mockPolicy struct {
-	getPrechecker func(*config.Config) (state.Prechecker, error)
+	getPrechecker      func(*config.Config) (state.Prechecker, error)
+	getConfigValidator func(string) (state.ConfigValidator, error)
 }
 
 func (p *mockPolicy) Prechecker(cfg *config.Config) (state.Prechecker, error) {
@@ -106,3 +107,10 @@
 	}
 	return nil, errors.NewNotImplementedError("Prechecker")
 }
+
+func (p *mockPolicy) ConfigValidator(providerType string) (state.ConfigValidator, error) {
+	if p.getConfigValidator != nil {
+		return p.getConfigValidator(providerType)
+	}
+	return nil, errors.NewNotImplementedError("ConfigValidator")
+}
 
=== modified file 'state/initialize_test.go'
--- state/initialize_test.go	2014-03-17 16:12:00 +0000
+++ state/initialize_test.go	2014-03-18 02:38:23 +0000
@@ -110,17 +110,18 @@
 func (s *InitializeSuite) TestEnvironConfigWithAdminSecret(c *gc.C) {
 	// admin-secret blocks Initialize.
 	good := testing.EnvironConfig(c)
-	bad, err := good.Apply(map[string]interface{}{"admin-secret": "foo"})
+	badUpdateAttrs := map[string]interface{}{"admin-secret": "foo"}
+	bad, err := good.Apply(badUpdateAttrs)
 
 	_, err = state.Initialize(state.TestingStateInfo(), bad, state.TestingDialOpts(), state.Policy(nil))
 	c.Assert(err, gc.ErrorMatches, "admin-secret should never be written to the state")
 
-	// admin-secret blocks SetEnvironConfig.
+	// admin-secret blocks UpdateEnvironConfig.
 	st := state.TestingInitialize(c, good, state.Policy(nil))
 	st.Close()
 
 	s.openState(c)
-	err = s.State.SetEnvironConfig(bad, good)
+	err = s.State.UpdateEnvironConfig(badUpdateAttrs, nil, nil)
 	c.Assert(err, gc.ErrorMatches, "admin-secret should never be written to the state")
 
 	// EnvironConfig remains inviolate.
@@ -140,12 +141,11 @@
 	_, err = state.Initialize(state.TestingStateInfo(), bad, state.TestingDialOpts(), state.Policy(nil))
 	c.Assert(err, gc.ErrorMatches, "agent-version must always be set in state")
 
-	// Bad agent-version blocks SetEnvironConfig.
 	st := state.TestingInitialize(c, good, state.Policy(nil))
 	st.Close()
 
 	s.openState(c)
-	err = s.State.SetEnvironConfig(bad, good)
+	err = s.State.UpdateEnvironConfig(map[string]interface{}{}, []string{"agent-version"}, nil)
 	c.Assert(err, gc.ErrorMatches, "agent-version must always be set in state")
 
 	// EnvironConfig remains inviolate.
 
=== modified file 'state/machine_test.go'
--- state/machine_test.go	2014-03-13 07:54:56 +0000
+++ state/machine_test.go	2014-03-18 02:38:23 +0000
@@ -79,9 +79,8 @@
 	manual, err := s.machine0.IsManual()
 	c.Assert(err, gc.IsNil)
 	c.Assert(manual, jc.IsFalse)
-	newcfg, err := cfg.Apply(map[string]interface{}{"type": "null"})
-	c.Assert(err, gc.IsNil)
-	err = s.State.SetEnvironConfig(newcfg, cfg)
+	attrs := map[string]interface{}{"type": "null"}
+	err = s.State.UpdateEnvironConfig(attrs, nil, nil)
 	c.Assert(err, gc.IsNil)
 	manual, err = s.machine0.IsManual()
 	c.Assert(err, gc.IsNil)
 
=== modified file 'state/policy.go'
--- state/policy.go	2014-02-20 08:11:19 +0000
+++ state/policy.go	2014-03-18 02:38:23 +0000
@@ -24,6 +24,10 @@
 	// Prechecker takes a *config.Config and returns
 	// a (possibly nil) Prechecker or an error.
 	Prechecker(*config.Config) (Prechecker, error)
+
+	// ConfigValidator takes a string (environ type) and returns
+	// a (possibly nil) ConfigValidator or an error.
+	ConfigValidator(string) (ConfigValidator, error)
 }
 
 // Prechecker is a policy interface that is provided to State
@@ -40,6 +44,12 @@
 	PrecheckInstance(series string, cons constraints.Value) error
 }
 
+// ConfigValidator is a policy interface that is provided to State
+// to check validity of new configuration attributes before applying them to state.
+type ConfigValidator interface {
+	Validate(cfg, old *config.Config) (valid *config.Config, err error)
+}
+
 // precheckInstance calls the state's assigned policy, if non-nil, to obtain
 // a Prechecker, and calls PrecheckInstance if a non-nil Prechecker is returned.
 func (st *State) precheckInstance(series string, cons constraints.Value) error {
@@ -61,3 +71,21 @@
 	}
 	return prechecker.PrecheckInstance(series, cons)
 }
+
+// validate calls the state's assigned policy, if non-nil, to obtain
+// a Validator, and calls validate if a non-nil Validator is returned.
+func (st *State) validate(cfg, old *config.Config) (valid *config.Config, err error) {
+	if st.policy == nil {
+		return cfg, nil
+	}
+	configValidator, err := st.policy.ConfigValidator(cfg.Type())
+	if errors.IsNotImplementedError(err) {
+		return cfg, nil
+	} else if err != nil {
+		return nil, err
+	}
+	if configValidator == nil {
+		return nil, fmt.Errorf("policy returned nil configValidator without an error")
+	}
+	return configValidator.Validate(cfg, old)
+}
 
=== modified file 'state/state.go'
--- state/state.go	2014-03-13 13:42:50 +0000
+++ state/state.go	2014-03-18 02:38:23 +0000
@@ -259,12 +259,33 @@
 	return ErrExcessiveContention
 }
 
-// SetEnvironConfig replaces the current configuration of the
-// environment with the provided configuration.
-func (st *State) SetEnvironConfig(cfg, old *config.Config) error {
-	if err := checkEnvironConfig(cfg); err != nil {
-		return err
-	}
+func (st *State) buildAndValidateEnvironConfig(updateAttrs map[string]interface{}, removeAttrs []string, oldConfig *config.Config) (validCfg *config.Config, err error) {
+	newConfig, err := oldConfig.Apply(updateAttrs)
+	if err != nil {
+		return nil, err
+	}
+	if len(removeAttrs) != 0 {
+		newConfig, err = newConfig.Remove(removeAttrs)
+		if err != nil {
+			return nil, err
+		}
+	}
+	if err := checkEnvironConfig(newConfig); err != nil {
+		return nil, err
+	}
+	return st.validate(newConfig, oldConfig)
+}
+
+type ValidateConfigFunc func(updateAttrs map[string]interface{}, removeAttrs []string, oldConfig *config.Config) error
+
+// UpateEnvironConfig adds, updates or removes attributes in the current
+// configuration of the environment with the provided updateAttrs and
+// removeAttrs.
+func (st *State) UpdateEnvironConfig(updateAttrs map[string]interface{}, removeAttrs []string, additionalValidation ValidateConfigFunc) error {
+	if len(updateAttrs)+len(removeAttrs) == 0 {
+		return nil
+	}
+
 	// TODO(axw) 2013-12-6 #1167616
 	// Ensure that the settings on disk have not changed
 	// underneath us. The settings changes are actually
@@ -275,13 +296,30 @@
 	if err != nil {
 		return err
 	}
-	newattrs := cfg.AllAttrs()
-	for k, _ := range old.AllAttrs() {
-		if _, ok := newattrs[k]; !ok {
+
+	// Get the existing environment config from state.
+	oldConfig, err := config.New(config.NoDefaults, settings.Map())
+	if err != nil {
+		return err
+	}
+	if additionalValidation != nil {
+		err = additionalValidation(updateAttrs, removeAttrs, oldConfig)
+		if err != nil {
+			return err
+		}
+	}
+	validCfg, err := st.buildAndValidateEnvironConfig(updateAttrs, removeAttrs, oldConfig)
+	if err != nil {
+		return err
+	}
+
+	validAttrs := validCfg.AllAttrs()
+	for k, _ := range oldConfig.AllAttrs() {
+		if _, ok := validAttrs[k]; !ok {
 			settings.Delete(k)
 		}
 	}
-	settings.Update(newattrs)
+	settings.Update(validAttrs)
 	_, err = settings.Write()
 	return err
 }
 
=== modified file 'state/state_test.go'
--- state/state_test.go	2014-03-17 16:39:43 +0000
+++ state/state_test.go	2014-03-18 02:38:23 +0000
@@ -1250,18 +1250,20 @@
 }
 
 func (s *StateSuite) TestEnvironConfig(c *gc.C) {
-	cfg, err := s.State.EnvironConfig()
-	c.Assert(err, gc.IsNil)
-	change, err := cfg.Apply(map[string]interface{}{
+	attrs := map[string]interface{}{
 		"authorized-keys": "different-keys",
 		"arbitrary-key":   "shazam!",
-	})
-	c.Assert(err, gc.IsNil)
-	err = s.State.SetEnvironConfig(change, cfg)
-	c.Assert(err, gc.IsNil)
-	cfg, err = s.State.EnvironConfig()
-	c.Assert(err, gc.IsNil)
-	c.Assert(cfg.AllAttrs(), gc.DeepEquals, change.AllAttrs())
+	}
+	cfg, err := s.State.EnvironConfig()
+	c.Assert(err, gc.IsNil)
+	err = s.State.UpdateEnvironConfig(attrs, nil, nil)
+	c.Assert(err, gc.IsNil)
+	cfg, err = cfg.Apply(attrs)
+	c.Assert(err, gc.IsNil)
+	oldCfg, err := s.State.EnvironConfig()
+	c.Assert(err, gc.IsNil)
+
+	c.Assert(oldCfg, gc.DeepEquals, cfg)
 }
 
 func (s *StateSuite) TestEnvironConstraints(c *gc.C) {
@@ -1681,6 +1683,37 @@
 	})
 }
 
+func (s *StateSuite) TestAdditionalValidation(c *gc.C) {
+	updateAttrs := map[string]interface{}{"logging-config": "juju=ERROR"}
+	configValidator1 := func(updateAttrs map[string]interface{}, removeAttrs []string, oldConfig *config.Config) error {
+		c.Assert(updateAttrs, gc.DeepEquals, map[string]interface{}{"logging-config": "juju=ERROR"})
+		if _, found := updateAttrs["logging-config"]; found {
+			return fmt.Errorf("cannot change logging-config")
+		}
+		return nil
+	}
+	removeAttrs := []string{"logging-config"}
+	configValidator2 := func(updateAttrs map[string]interface{}, removeAttrs []string, oldConfig *config.Config) error {
+		c.Assert(removeAttrs, gc.DeepEquals, []string{"logging-config"})
+		for _, i := range removeAttrs {
+			if i == "logging-config" {
+				return fmt.Errorf("cannot remove logging-config")
+			}
+		}
+		return nil
+	}
+	configValidator3 := func(updateAttrs map[string]interface{}, removeAttrs []string, oldConfig *config.Config) error {
+		return nil
+	}
+
+	err := s.State.UpdateEnvironConfig(updateAttrs, nil, configValidator1)
+	c.Assert(err, gc.ErrorMatches, "cannot change logging-config")
+	err = s.State.UpdateEnvironConfig(nil, removeAttrs, configValidator2)
+	c.Assert(err, gc.ErrorMatches, "cannot remove logging-config")
+	err = s.State.UpdateEnvironConfig(updateAttrs, nil, configValidator3)
+	c.Assert(err, gc.IsNil)
+}
+
 type attrs map[string]interface{}
 
 func (s *StateSuite) TestWatchEnvironConfig(c *gc.C) {
@@ -1699,11 +1732,10 @@
 	assertChange := func(change attrs) {
 		cfg, err := s.State.EnvironConfig()
 		c.Assert(err, gc.IsNil)
+		cfg, err = cfg.Apply(change)
+		c.Assert(err, gc.IsNil)
 		if change != nil {
-			oldcfg := cfg
-			cfg, err = cfg.Apply(change)
-			c.Assert(err, gc.IsNil)
-			err = s.State.SetEnvironConfig(cfg, oldcfg)
+			err = s.State.UpdateEnvironConfig(change, nil, nil)
 			c.Assert(err, gc.IsNil)
 		}
 		s.State.StartSync()
@@ -1761,7 +1793,6 @@
 func (s *StateSuite) TestWatchEnvironConfigCorruptConfig(c *gc.C) {
 	cfg, err := s.State.EnvironConfig()
 	c.Assert(err, gc.IsNil)
-	oldcfg := cfg
 
 	// Corrupt the environment configuration.
 	settings := s.Session.DB("juju").C("settings")
@@ -1797,9 +1828,11 @@
 	}
 
 	// Fix the configuration.
-	err = s.State.SetEnvironConfig(cfg, oldcfg)
+	err = settings.UpdateId("e", bson.D{{"$set", bson.D{{"name", "foo"}}}})
 	c.Assert(err, gc.IsNil)
 	fixed := cfg.AllAttrs()
+	err = s.State.UpdateEnvironConfig(fixed, nil, nil)
+	c.Assert(err, gc.IsNil)
 
 	s.State.StartSync()
 	select {
@@ -2559,9 +2592,7 @@
 func (s *StateSuite) changeEnviron(c *gc.C, envConfig *config.Config, name string, value interface{}) {
 	attrs := envConfig.AllAttrs()
 	attrs[name] = value
-	newConfig, err := config.New(config.NoDefaults, attrs)
-	c.Assert(err, gc.IsNil)
-	c.Assert(s.State.SetEnvironConfig(newConfig, envConfig), gc.IsNil)
+	c.Assert(s.State.UpdateEnvironConfig(attrs, nil, nil), gc.IsNil)
 }
 
 func (s *StateSuite) assertAgentVersion(c *gc.C, envConfig *config.Config, vers string) {
 
=== modified file 'state/testing/agent.go'
--- state/testing/agent.go	2013-09-11 03:48:05 +0000
+++ state/testing/agent.go	2014-03-18 02:38:23 +0000
@@ -11,5 +11,5 @@
 // SetAgentVersion sets the current agent version in the state's
 // environment configuration.
 func SetAgentVersion(st *state.State, vers version.Number) error {
-	return UpdateConfig(st, map[string]interface{}{"agent-version": vers.String()})
+	return st.UpdateEnvironConfig(map[string]interface{}{"agent-version": vers.String()}, nil, nil)
 }
 
=== removed file 'state/testing/config.go'
--- state/testing/config.go	2013-12-06 04:47:45 +0000
+++ state/testing/config.go	1970-01-01 00:00:00 +0000
@@ -1,22 +0,0 @@
-// Copyright 2013 Canonical Ltd.
-// Licensed under the AGPLv3, see LICENCE file for details.
-
-package testing
-
-import (
-	"launchpad.net/juju-core/state"
-)
-
-// UpdateConfig sets the current agent version in the state's
-// environment configuration.
-func UpdateConfig(st *state.State, newValues map[string]interface{}) error {
-	cfg, err := st.EnvironConfig()
-	if err != nil {
-		return err
-	}
-	newcfg, err := cfg.Apply(newValues)
-	if err != nil {
-		return err
-	}
-	return st.SetEnvironConfig(newcfg, cfg)
-}
 
=== modified file 'upgrades/deprecatedattributes.go'
--- upgrades/deprecatedattributes.go	2014-03-07 02:48:20 +0000
+++ upgrades/deprecatedattributes.go	2014-03-18 02:38:23 +0000
@@ -3,29 +3,16 @@
 
 package upgrades
 
-import (
-	"fmt"
-
-	"launchpad.net/juju-core/environs/config"
-)
-
 func processDeprecatedAttributes(context Context) error {
 	st := context.State()
-	cfg, err := st.EnvironConfig()
-	if err != nil {
-		return fmt.Errorf("failed to read current config: %v", err)
+	removeAttrs := []string{
+		"public-bucket",
+		"public-bucket-region",
+		"public-bucket-url",
+		"default-image-id",
+		"default-instance-type",
+		"shared-storage-port",
 	}
-	newAttrs := cfg.AllAttrs()
-	delete(newAttrs, "public-bucket")
-	delete(newAttrs, "public-bucket-region")
-	delete(newAttrs, "public-bucket-url")
-	delete(newAttrs, "default-image-id")
-	delete(newAttrs, "default-instance-type")
-	delete(newAttrs, "shared-storage-port")
 	// TODO (wallyworld) - delete tools-url in 1.20
-	newCfg, err := config.New(config.NoDefaults, newAttrs)
-	if err != nil {
-		return fmt.Errorf("failed to create new config: %v", err)
-	}
-	return st.SetEnvironConfig(newCfg, cfg)
+	return st.UpdateEnvironConfig(map[string]interface{}{}, removeAttrs, nil)
 }
 
=== modified file 'upgrades/deprecatedattributes_test.go'
--- upgrades/deprecatedattributes_test.go	2014-03-13 07:54:56 +0000
+++ upgrades/deprecatedattributes_test.go	2014-03-18 02:38:23 +0000
@@ -27,19 +27,16 @@
 		apiState:    apiState,
 		state:       s.State,
 	}
-	cfg, err := s.State.EnvironConfig()
-	c.Assert(err, gc.IsNil)
 	// Add in old attributes.
-	newCfg, err := cfg.Apply(map[string]interface{}{
+	newCfg := map[string]interface{}{
 		"public-bucket":         "foo",
 		"public-bucket-region":  "bar",
 		"public-bucket-url":     "shazbot",
 		"default-instance-type": "vulch",
 		"default-image-id":      "1234",
 		"shared-storage-port":   1234,
-	})
-	c.Assert(err, gc.IsNil)
-	err = s.State.SetEnvironConfig(newCfg, cfg)
+	}
+	err := s.State.UpdateEnvironConfig(newCfg, nil, nil)
 	c.Assert(err, gc.IsNil)
 }
 
 
=== modified file 'upgrades/rsyslogport.go'
--- upgrades/rsyslogport.go	2014-02-28 05:44:03 +0000
+++ upgrades/rsyslogport.go	2014-03-18 02:38:23 +0000
@@ -9,15 +9,8 @@
 
 func updateRsyslogPort(context Context) error {
 	st := context.State()
-	old, err := st.EnvironConfig()
-	if err != nil {
-		return err
-	}
-	cfg, err := old.Apply(map[string]interface{}{
+	attrs := map[string]interface{}{
 		"syslog-port": config.DefaultSyslogPort,
-	})
-	if err != nil {
-		return err
 	}
-	return st.SetEnvironConfig(cfg, old)
+	return st.UpdateEnvironConfig(attrs, nil, nil)
 }
 
=== modified file 'worker/authenticationworker/worker_test.go'
--- worker/authenticationworker/worker_test.go	2013-12-23 05:18:58 +0000
+++ worker/authenticationworker/worker_test.go	2014-03-18 02:38:23 +0000
@@ -15,7 +15,6 @@
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/state/api"
 	"launchpad.net/juju-core/state/api/keyupdater"
-	"launchpad.net/juju-core/state/testing"
 	coretesting "launchpad.net/juju-core/testing"
 	"launchpad.net/juju-core/utils/ssh"
 	sshtesting "launchpad.net/juju-core/utils/ssh/testing"
@@ -90,7 +89,7 @@
 
 func (s *workerSuite) setAuthorisedKeys(c *gc.C, keys ...string) {
 	keyStr := strings.Join(keys, "\n")
-	err := testing.UpdateConfig(s.BackingState, map[string]interface{}{"authorized-keys": keyStr})
+	err := s.BackingState.UpdateEnvironConfig(map[string]interface{}{"authorized-keys": keyStr}, nil, nil)
 	c.Assert(err, gc.IsNil)
 	s.BackingState.StartSync()
 }
 
=== modified file 'worker/environ_test.go'
--- worker/environ_test.go	2013-12-05 03:42:25 +0000
+++ worker/environ_test.go	2014-03-18 02:38:23 +0000
@@ -46,30 +46,34 @@
 }
 
 func (s *waitForEnvironSuite) TestInvalidConfig(c *gc.C) {
+	var oldType string
+	oldType = s.Conn.Environ.Config().AllAttrs()["type"].(string)
+
 	// Create an invalid config by taking the current config and
 	// tweaking the provider type.
-	var oldType string
-	testing.ChangeEnvironConfig(c, s.State, func(attrs coretesting.Attrs) coretesting.Attrs {
-		oldType = attrs["type"].(string)
-		return attrs.Merge(coretesting.Attrs{"type": "unknown"})
-	})
-	w := s.State.WatchForEnvironConfigChanges()
+	info := s.StateInfo(c)
+	opts := state.DefaultDialOpts()
+	st2, err := state.Open(info, opts, state.Policy(nil))
+	c.Assert(err, gc.IsNil)
+	defer st2.Close()
+	err = st2.UpdateEnvironConfig(map[string]interface{}{"type": "unknown"}, nil, nil)
+	c.Assert(err, gc.IsNil)
+
+	w := st2.WatchForEnvironConfigChanges()
 	defer stopWatcher(c, w)
 	done := make(chan environs.Environ)
 	go func() {
-		env, err := worker.WaitForEnviron(w, s.State, nil)
+		env, err := worker.WaitForEnviron(w, st2, nil)
 		c.Check(err, gc.IsNil)
 		done <- env
 	}()
 	// Wait for the loop to process the invalid configuratrion
 	<-worker.LoadedInvalid
 
-	testing.ChangeEnvironConfig(c, s.State, func(attrs coretesting.Attrs) coretesting.Attrs {
-		return attrs.Merge(coretesting.Attrs{
-			"type":   oldType,
-			"secret": "environ_test",
-		})
-	})
+	st2.UpdateEnvironConfig(map[string]interface{}{
+		"type":   oldType,
+		"secret": "environ_test",
+	}, nil, nil)
 
 	env := <-done
 	c.Assert(env, gc.NotNil)
 
=== modified file 'worker/firewaller/firewaller_test.go'
--- worker/firewaller/firewaller_test.go	2014-01-22 15:40:18 +0000
+++ worker/firewaller/firewaller_test.go	2014-03-18 02:38:23 +0000
@@ -4,13 +4,13 @@
 package firewaller_test
 
 import (
+	"launchpad.net/juju-core/environs/config"
 	"reflect"
 	stdtesting "testing"
 	"time"
 
 	gc "launchpad.net/gocheck"
 
-	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/juju"
 	"launchpad.net/juju-core/juju/testing"
@@ -37,6 +37,10 @@
 	firewaller *apifirewaller.State
 }
 
+type FirewallerGlobalModeSuite struct {
+	FirewallerSuite
+}
+
 var _ worker.Worker = (*firewaller.Firewaller)(nil)
 
 // assertPorts retrieves the open ports of the instance and compares them
@@ -91,6 +95,13 @@
 
 var _ = gc.Suite(&FirewallerSuite{})
 
+func (s FirewallerGlobalModeSuite) SetUpTest(c *gc.C) {
+	add := map[string]interface{}{"firewall-mode": config.FwGlobal}
+	s.DummyConfig = dummy.SampleConfig().Merge(add).Delete("admin-secret", "ca-private-key")
+
+	s.FirewallerSuite.SetUpTest(c)
+}
+
 func (s *FirewallerSuite) SetUpTest(c *gc.C) {
 	s.JujuConnSuite.SetUpTest(c)
 	s.charm = s.AddTestingCharm(c, "dummy")
@@ -129,23 +140,6 @@
 	return u, m
 }
 
-func (s *FirewallerSuite) setGlobalMode(c *gc.C) {
-	// TODO(rog) This should not be possible - you shouldn't
-	// be able to set the firewalling mode after an environment
-	// has bootstrapped.
-	oldConfig := s.Conn.Environ.Config()
-	attrs := oldConfig.AllAttrs()
-	delete(attrs, "admin-secret")
-	delete(attrs, "ca-private-key")
-	attrs["firewall-mode"] = config.FwGlobal
-	newConfig, err := config.New(config.NoDefaults, attrs)
-	c.Assert(err, gc.IsNil)
-	err = s.State.SetEnvironConfig(newConfig, oldConfig)
-	c.Assert(err, gc.IsNil)
-	err = s.Conn.Environ.SetConfig(newConfig)
-	c.Assert(err, gc.IsNil)
-}
-
 // startInstance starts a new instance for the given machine.
 func (s *FirewallerSuite) startInstance(c *gc.C, m *state.Machine) instance.Instance {
 	inst, hc := testing.AssertStartInstance(c, s.Conn.Environ, m.Id())
@@ -573,10 +567,7 @@
 	c.Assert(err, gc.IsNil)
 }
 
-func (s *FirewallerSuite) TestGlobalMode(c *gc.C) {
-	// Change configuration.
-	s.setGlobalMode(c)
-
+func (s *FirewallerGlobalModeSuite) TestGlobalMode(c *gc.C) {
 	// Start firewaller and open ports.
 	fw, err := firewaller.NewFirewaller(s.firewaller)
 	c.Assert(err, gc.IsNil)
@@ -621,10 +612,7 @@
 	s.assertEnvironPorts(c, nil)
 }
 
-func (s *FirewallerSuite) TestGlobalModeStartWithUnexposedService(c *gc.C) {
-	// Change configuration.
-	s.setGlobalMode(c)
-
+func (s *FirewallerGlobalModeSuite) TestGlobalModeStartWithUnexposedService(c *gc.C) {
 	m, err := s.State.AddMachine("quantal", state.JobHostUnits)
 	c.Assert(err, gc.IsNil)
 	s.startInstance(c, m)
@@ -650,10 +638,7 @@
 	s.assertEnvironPorts(c, []instance.Port{{"tcp", 80}})
 }
 
-func (s *FirewallerSuite) TestGlobalModeRestart(c *gc.C) {
-	// Change configuration.
-	s.setGlobalMode(c)
-
+func (s *FirewallerGlobalModeSuite) TestGlobalModeRestart(c *gc.C) {
 	// Start firewaller and open ports.
 	fw, err := firewaller.NewFirewaller(s.firewaller)
 	c.Assert(err, gc.IsNil)
@@ -688,10 +673,7 @@
 	s.assertEnvironPorts(c, []instance.Port{{"tcp", 80}, {"tcp", 8888}})
 }
 
-func (s *FirewallerSuite) TestGlobalModeRestartUnexposedService(c *gc.C) {
-	// Change configuration.
-	s.setGlobalMode(c)
-
+func (s *FirewallerGlobalModeSuite) TestGlobalModeRestartUnexposedService(c *gc.C) {
 	// Start firewaller and open ports.
 	fw, err := firewaller.NewFirewaller(s.firewaller)
 	c.Assert(err, gc.IsNil)
@@ -724,10 +706,7 @@
 	s.assertEnvironPorts(c, nil)
 }
 
-func (s *FirewallerSuite) TestGlobalModeRestartPortCount(c *gc.C) {
-	// Change configuration.
-	s.setGlobalMode(c)
-
+func (s *FirewallerGlobalModeSuite) TestGlobalModeRestartPortCount(c *gc.C) {
 	// Start firewaller and open ports.
 	fw, err := firewaller.NewFirewaller(s.firewaller)
 	c.Assert(err, gc.IsNil)
 
=== modified file 'worker/instancepoller/observer_test.go'
--- worker/instancepoller/observer_test.go	2014-03-05 19:41:34 +0000
+++ worker/instancepoller/observer_test.go	2014-03-18 02:38:23 +0000
@@ -12,6 +12,7 @@
 	gc "launchpad.net/gocheck"
 
 	"launchpad.net/juju-core/juju/testing"
+	"launchpad.net/juju-core/state"
 	coretesting "launchpad.net/juju-core/testing"
 )
 
@@ -43,17 +44,18 @@
 
 	env := obs.Environ()
 	c.Assert(env.Config().AllAttrs(), gc.DeepEquals, originalConfig.AllAttrs())
-
 	var oldType string
+	oldType = env.Config().AllAttrs()["type"].(string)
+
+	info := s.StateInfo(c)
+	opts := state.DefaultDialOpts()
+	st2, err := state.Open(info, opts, state.Policy(nil))
+	defer st2.Close()
+
 	// Change to an invalid configuration and check
 	// that the observer's environment remains the same.
-	testing.ChangeEnvironConfig(c, s.State, func(attrs coretesting.Attrs) coretesting.Attrs {
-		oldType = attrs["type"].(string)
-		return attrs.Merge(coretesting.Attrs{
-			"type": "invalid",
-		})
-	})
-	s.State.StartSync()
+	st2.UpdateEnvironConfig(map[string]interface{}{"type": "invalid"}, nil, nil)
+	st2.StartSync()
 
 	// Wait for the observer to register the invalid environment
 	timeout := time.After(coretesting.LongWait)
@@ -74,13 +76,8 @@
 
 	// Change the environment back to a valid configuration
 	// with a different name and check that we see it.
-	testing.ChangeEnvironConfig(c, s.State, func(attrs coretesting.Attrs) coretesting.Attrs {
-		return attrs.Merge(coretesting.Attrs{
-			"type": oldType,
-			"name": "a-new-name",
-		})
-	})
-	s.State.StartSync()
+	st2.UpdateEnvironConfig(map[string]interface{}{"type": oldType, "name": "a-new-name"}, nil, nil)
+	st2.StartSync()
 
 	for a := coretesting.LongAttempt.Start(); a.Next(); {
 		env := obs.Environ()
 
=== modified file 'worker/machineenvironmentworker/machineenvironmentworker_test.go'
--- worker/machineenvironmentworker/machineenvironmentworker_test.go	2014-03-13 07:54:56 +0000
+++ worker/machineenvironmentworker/machineenvironmentworker_test.go	2014-03-18 02:38:23 +0000
@@ -119,8 +119,6 @@
 }
 
 func (s *MachineEnvironmentWatcherSuite) updateConfig(c *gc.C) (osenv.ProxySettings, osenv.ProxySettings) {
-	oldConfig, err := s.State.EnvironConfig()
-	c.Assert(err, gc.IsNil)
 
 	proxySettings := osenv.ProxySettings{
 		Http:    "http proxy",
@@ -128,9 +126,10 @@
 		Ftp:     "ftp proxy",
 		NoProxy: "no proxy",
 	}
-
-	envConfig, err := oldConfig.Apply(config.ProxyConfigMap(proxySettings))
-	c.Assert(err, gc.IsNil)
+	attrs := map[string]interface{}{}
+	for k, v := range config.ProxyConfigMap(proxySettings) {
+		attrs[k] = v
+	}
 
 	// We explicitly set apt proxy settings as well to show that it is the apt
 	// settings that are used for the apt config, and not just the normal
@@ -141,10 +140,11 @@
 		Https: "apt https proxy",
 		Ftp:   "apt ftp proxy",
 	}
-	envConfig, err = envConfig.Apply(config.AptProxyConfigMap(aptProxySettings))
-	c.Assert(err, gc.IsNil)
+	for k, v := range config.AptProxyConfigMap(aptProxySettings) {
+		attrs[k] = v
+	}
 
-	err = s.State.SetEnvironConfig(envConfig, oldConfig)
+	err := s.State.UpdateEnvironConfig(attrs, nil, nil)
 	c.Assert(err, gc.IsNil)
 
 	return proxySettings, aptProxySettings
 
=== modified file 'worker/provisioner/provisioner_test.go'
--- worker/provisioner/provisioner_test.go	2014-03-13 07:54:56 +0000
+++ worker/provisioner/provisioner_test.go	2014-03-18 02:38:23 +0000
@@ -96,11 +96,8 @@
 // that causes the given environMethod of the dummy provider to return
 // an error, which is also returned as a message to be checked.
 func breakDummyProvider(c *gc.C, st *state.State, environMethod string) string {
-	oldCfg, err := st.EnvironConfig()
-	c.Assert(err, gc.IsNil)
-	cfg, err := oldCfg.Apply(map[string]interface{}{"broken": environMethod})
-	c.Assert(err, gc.IsNil)
-	err = st.SetEnvironConfig(cfg, oldCfg)
+	attrs := map[string]interface{}{"broken": environMethod}
+	err := st.UpdateEnvironConfig(attrs, nil, nil)
 	c.Assert(err, gc.IsNil)
 	return fmt.Sprintf("dummy.%s is broken", environMethod)
 }
@@ -121,21 +118,21 @@
 // so the Settings returned from the watcher will not pass
 // validation.
 func (s *CommonProvisionerSuite) invalidateEnvironment(c *gc.C) {
-	attrs := s.cfg.AllAttrs()
-	attrs["type"] = "unknown"
-	invalidCfg, err := config.New(config.NoDefaults, attrs)
+	st, err := state.Open(s.StateInfo(c), state.DefaultDialOpts(), state.Policy(nil))
 	c.Assert(err, gc.IsNil)
-	err = s.State.SetEnvironConfig(invalidCfg, s.cfg)
+	defer st.Close()
+	attrs := map[string]interface{}{"type": "unknown"}
+	err = st.UpdateEnvironConfig(attrs, nil, nil)
 	c.Assert(err, gc.IsNil)
 }
 
 // fixEnvironment undoes the work of invalidateEnvironment.
-func (s *CommonProvisionerSuite) fixEnvironment() error {
-	cfg, err := s.State.EnvironConfig()
-	if err != nil {
-		return err
-	}
-	return s.State.SetEnvironConfig(s.cfg, cfg)
+func (s *CommonProvisionerSuite) fixEnvironment(c *gc.C) error {
+	st, err := state.Open(s.StateInfo(c), state.DefaultDialOpts(), state.Policy(nil))
+	c.Assert(err, gc.IsNil)
+	defer st.Close()
+	attrs := map[string]interface{}{"type": s.cfg.AllAttrs()["type"]}
+	return st.UpdateEnvironConfig(attrs, nil, nil)
 }
 
 // stopper is stoppable.
@@ -415,7 +412,7 @@
 	}
 
 	// Unbreak the environ config.
-	err = s.fixEnvironment()
+	err = s.fixEnvironment(c)
 	c.Assert(err, gc.IsNil)
 
 	// Restart the PA to make sure the machine is skipped again.
@@ -480,7 +477,7 @@
 	// the PA should not create it
 	s.checkNoOperations(c)
 
-	err = s.fixEnvironment()
+	err = s.fixEnvironment(c)
 	c.Assert(err, gc.IsNil)
 
 	s.checkStartInstance(c, m)
@@ -611,20 +608,15 @@
 	// the PA should create it using the old environment
 	s.checkStartInstance(c, m)
 
-	err = s.fixEnvironment()
+	err = s.fixEnvironment(c)
 	c.Assert(err, gc.IsNil)
 
 	// insert our observer
 	cfgObserver := make(chan *config.Config, 1)
 	provisioner.SetObserver(p, cfgObserver)
 
-	oldcfg, err := s.State.EnvironConfig()
-	c.Assert(err, gc.IsNil)
-	attrs := oldcfg.AllAttrs()
-	attrs["secret"] = "beef"
-	cfg, err := config.New(config.NoDefaults, attrs)
-	c.Assert(err, gc.IsNil)
-	err = s.State.SetEnvironConfig(cfg, oldcfg)
+	err = s.State.UpdateEnvironConfig(map[string]interface{}{"secret": "beef"}, nil, nil)
+	c.Assert(err, gc.IsNil)
 
 	s.BackingState.StartSync()
 
@@ -666,13 +658,9 @@
 	c.Assert(m1.Remove(), gc.IsNil)
 
 	// turn on safe mode
-	oldcfg, err := s.State.EnvironConfig()
-	c.Assert(err, gc.IsNil)
-	attrs := oldcfg.AllAttrs()
-	attrs["provisioner-safe-mode"] = true
-	cfg, err := config.New(config.NoDefaults, attrs)
-	c.Assert(err, gc.IsNil)
-	err = s.State.SetEnvironConfig(cfg, oldcfg)
+	attrs := map[string]interface{}{"provisioner-safe-mode": true}
+	err = s.State.UpdateEnvironConfig(attrs, nil, nil)
+	c.Assert(err, gc.IsNil)
 
 	// start a new provisioner to shut down only the machine still in state.
 	p = s.newEnvironProvisioner(c)
@@ -712,13 +700,9 @@
 	provisioner.SetObserver(p, cfgObserver)
 
 	// turn on safe mode
-	oldcfg, err := s.State.EnvironConfig()
-	c.Assert(err, gc.IsNil)
-	attrs := oldcfg.AllAttrs()
-	attrs["provisioner-safe-mode"] = true
-	cfg, err := config.New(config.NoDefaults, attrs)
-	c.Assert(err, gc.IsNil)
-	err = s.State.SetEnvironConfig(cfg, oldcfg)
+	attrs := map[string]interface{}{"provisioner-safe-mode": true}
+	err = s.State.UpdateEnvironConfig(attrs, nil, nil)
+	c.Assert(err, gc.IsNil)
 
 	s.BackingState.StartSync()
 
 
=== modified file 'worker/uniter/uniter_test.go'
--- worker/uniter/uniter_test.go	2014-03-17 13:45:26 +0000
+++ worker/uniter/uniter_test.go	2014-03-18 02:38:23 +0000
@@ -1959,16 +1959,13 @@
 type setProxySettings osenv.ProxySettings
 
 func (s setProxySettings) step(c *gc.C, ctx *context) {
-	old, err := ctx.st.EnvironConfig()
-	c.Assert(err, gc.IsNil)
-	cfg, err := old.Apply(map[string]interface{}{
+	attrs := map[string]interface{}{
 		"http-proxy":  s.Http,
 		"https-proxy": s.Https,
 		"ftp-proxy":   s.Ftp,
 		"no-proxy":    s.NoProxy,
-	})
-	c.Assert(err, gc.IsNil)
-	err = ctx.st.SetEnvironConfig(cfg, old)
+	}
+	err := ctx.st.UpdateEnvironConfig(attrs, nil, nil)
 	c.Assert(err, gc.IsNil)
 	// wait for the new values...
 	expected := (osenv.ProxySettings)(s)