Push environment secrets after connecting API

Push environment secrets after connecting API

This change introduces a new client API,
PushEnvironmentSecrets. The api.Open function
is modified to call it directly after a
successful Login if given a non-empty set of
secrets to pass to the server. The
juju/NewAPI* functions have been modified to
pass secrets to api.Open. Non-client callers
of api.Open (i.e. machine agents) will not
invoke PushEnvironmentSecrets.

A follow-up CL will modify Login to return
a flag indicating that secrets have not
previously been sent, and should now be.
The api.Open function will then be modified
to check this flag, and conditionally make
the PushEnvironmentSecrets call.

Also, fix a bug in state.State.SetEnvironmentConfig,
so deleted attributes are removed from Mongo.
Also, modify the dummy provider to not set defaults
in config when calling SecretAttrs. These changes
were necessary for testing PushEnvironmentSecrets
properly.

Fixes lp:1248809

https://code.launchpad.net/~axwalk/juju-core/api-push-env-secrets/+merge/194083

(do not edit description out of merge proposal)

[axw, 2013-11-06 09:19:21]

Please take a look.

[axw, 2013-11-06 09:52:56]

On 2013/11/06 09:19:21, axw wrote:
> Please take a look.

Got carried away with testing the usage in pkg/juju etc., forgot to add tests in
apiserver. Doing that now.

[fwereade, 2013-11-06 12:24:33]

Looking good so far, and my comments probably aren't a very big deal given the
limited lifetime of this code. In particular, I'm pretty sure it'll be gone
before we have to worry about multiple providers.

https://codereview.appspot.com/22080044/diff/1/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/22080044/diff/1/juju/api.go#newcode142
juju/api.go:142: // get an Environ, we just won't try to push secrets.
Worth being explicit that this is a short-term hack that will be replaced by
synchronous bootstrap.

https://codereview.appspot.com/22080044/diff/1/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/22080044/diff/1/state/api/apiclient.go#newcode95
state/api/apiclient.go:95: // have been pushed.
I don't think this is true. Shouldn't agents be able to connect earlier, and
just block on calls that require the environment to be valid?

https://codereview.appspot.com/22080044/diff/1/state/apiserver/client/client.go
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/22080044/diff/1/state/apiserver/client/client....
state/apiserver/client/client.go:347: func (c *Client)
PushEnvironmentSecrets(args params.PushEnvironmentSecrets) error {
A thought: are these environment secrets, or provider secrets? Can we make a
meaningful distinction at this stage?

https://codereview.appspot.com/22080044/diff/1/state/apiserver/client/client....
state/apiserver/client/client.go:348: return nil
Either this should be deleted, or the rest of the method should be ;p.

[axw, 2013-11-07 04:18:42]

Please take a look.

https://codereview.appspot.com/22080044/diff/1/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/22080044/diff/1/juju/api.go#newcode142
juju/api.go:142: // get an Environ, we just won't try to push secrets.
On 2013/11/06 12:24:34, fwereade wrote:
> Worth being explicit that this is a short-term hack that will be replaced by
> synchronous bootstrap.

Done.

I have had several lines of thinking on synchronous bootstrap, and one was that
it just happened to do the first login, so it would push the secrets via this
mechanism. However, after further thought, I think we can do much better (cf. my
email regarding manual bootstrap-alike.)

https://codereview.appspot.com/22080044/diff/1/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/22080044/diff/1/state/api/apiclient.go#newcode95
state/api/apiclient.go:95: // have been pushed.
On 2013/11/06 12:24:34, fwereade wrote:
> I don't think this is true. Shouldn't agents be able to connect earlier, and
> just block on calls that require the environment to be valid?

Hmm, I wasn't considering the machine agents that are also API servers. Apart
from those, you'd have to have add-machine'd or deploy'd, which will do what we
need.

https://codereview.appspot.com/22080044/diff/1/state/apiserver/client/client.go
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/22080044/diff/1/state/apiserver/client/client....
state/apiserver/client/client.go:347: func (c *Client)
PushEnvironmentSecrets(args params.PushEnvironmentSecrets) error {
On 2013/11/06 12:24:34, fwereade wrote:
> A thought: are these environment secrets, or provider secrets? Can we make a
> meaningful distinction at this stage?

The names are provider-specific (they're known to EnvironProvider.SecretAttrs),
but the values are environment-specific.

https://codereview.appspot.com/22080044/diff/1/state/apiserver/client/client....
state/apiserver/client/client.go:348: return nil
On 2013/11/06 12:24:34, fwereade wrote:
> Either this should be deleted, or the rest of the method should be ;p.

Hah, yeah, I found that after I proposed (I had been hunting down a panic -
turned out to be unrelated). I've added tests, and re-enabled this.

[axw, 2013-11-07 04:24:10]

On 2013/11/07 04:18:42, axw wrote:
> Please take a look.
> 
> https://codereview.appspot.com/22080044/diff/1/juju/api.go
> File juju/api.go (right):
> 
> https://codereview.appspot.com/22080044/diff/1/juju/api.go#newcode142
> juju/api.go:142: // get an Environ, we just won't try to push secrets.
> On 2013/11/06 12:24:34, fwereade wrote:
> > Worth being explicit that this is a short-term hack that will be replaced by
> > synchronous bootstrap.
> 
> Done.
> 
> I have had several lines of thinking on synchronous bootstrap, and one was
that
> it just happened to do the first login, so it would push the secrets via this
> mechanism. However, after further thought, I think we can do much better (cf.
my
> email regarding manual bootstrap-alike.)
> 
> https://codereview.appspot.com/22080044/diff/1/state/api/apiclient.go
> File state/api/apiclient.go (right):
> 
>
https://codereview.appspot.com/22080044/diff/1/state/api/apiclient.go#newcode95
> state/api/apiclient.go:95: // have been pushed.
> On 2013/11/06 12:24:34, fwereade wrote:
> > I don't think this is true. Shouldn't agents be able to connect earlier, and
> > just block on calls that require the environment to be valid?
> 
> Hmm, I wasn't considering the machine agents that are also API servers. Apart
> from those, you'd have to have add-machine'd or deploy'd, which will do what
we
> need.
> 
>
https://codereview.appspot.com/22080044/diff/1/state/apiserver/client/client.go
> File state/apiserver/client/client.go (right):
> 
>
https://codereview.appspot.com/22080044/diff/1/state/apiserver/client/client....
> state/apiserver/client/client.go:347: func (c *Client)
> PushEnvironmentSecrets(args params.PushEnvironmentSecrets) error {
> On 2013/11/06 12:24:34, fwereade wrote:
> > A thought: are these environment secrets, or provider secrets? Can we make a
> > meaningful distinction at this stage?
> 
> The names are provider-specific (they're known to
EnvironProvider.SecretAttrs),
> but the values are environment-specific.
> 
>
https://codereview.appspot.com/22080044/diff/1/state/apiserver/client/client....
> state/apiserver/client/client.go:348: return nil
> On 2013/11/06 12:24:34, fwereade wrote:
> > Either this should be deleted, or the rest of the method should be ;p.
> 
> Hah, yeah, I found that after I proposed (I had been hunting down a panic -
> turned out to be unrelated). I've added tests, and re-enabled this.

So, this turned out quite ugly IMHO. I'd like to suggest that we go back to
getenv+setenv until we have synchronous bootstrap. If for whatever reason that
doesn't happen, then we could revisit this.