Merge lp:~axwalk/juju-core/api-push-env-secrets-take2 into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

Push environment secrets after connecting API

This is a much simpler alternative to
https://codereview.appspot.com/22080044/,
implementing the same logic as is present
for the straight-to-state juju.NewConn.

As secrets-pushing will be obsoleted by
synchronous bootstrapping, which is slated
for introduction soon, I have a strong
preference for this option.

There's a drive-by-fix to the EnvironmentSet
API which simplifies testing: allow agent-version
in the parameters as long as it matches the
existing value.

https://codereview.appspot.com/22720043/

Push environment secrets after connecting API

This is a much simpler alternative to
https://codereview.appspot.com/22080044/,
implementing the same logic as is present
for the straight-to-state juju.NewConn.

As secrets-pushing will be obsoleted by
synchronous bootstrapping, which is slated
for introduction soon, I have a strong
preference for this option.

There's a drive-by-fix to the EnvironmentSet
API which simplifies testing: allow agent-version
in the parameters as long as it matches the
existing value.

https://codereview.appspot.com/22720043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+194288_code.launchpad.net,

Message:
Please take a look.

Description:
Push environment secrets after connecting API

This is a much simpler alternative to
https://codereview.appspot.com/22080044/,
implementing the same logic as is present
for the straight-to-state juju.NewConn.

As secrets-pushing will be obsoleted by
synchronous bootstrapping, which is slated
for introduction soon, I have a strong
preference for this option.

There's a drive-by-fix to the EnvironmentSet
API which simplifies testing: allow agent-version
in the parameters as long as it matches the
existing value.

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

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/22720043/

Affected files (+61, -4 lines):
   A [revision details]
   M juju/api.go
   M juju/apiconn_test.go
   M juju/export_test.go
   M state/apiserver/client/client.go
   M state/apiserver/client/client_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20131106165607-vwv921qmnm416g10
+New revision: <email address hidden>

Index: juju/api.go
=== modified file 'juju/api.go'
--- juju/api.go 2013-10-24 18:44:46 +0000
+++ juju/api.go 2013-11-07 06:48:00 +0000
@@ -55,13 +55,40 @@
   if err != nil {
    return nil, err
   }
- // TODO(rog): implement updateSecrets (see Conn.updateSecrets)
+ // TODO(axw) remove this once we have synchronous bootstrap.
+ if err := updateSecrets(environ, st); err != nil {
+ return nil, err
+ }
   return &APIConn{
    Environ: environ,
    State: st,
   }, nil
  }

+// updateSecrets pushes environment secrets to the API server.
+// NOTE: this is a temporary hack, and will disappear when we
+// have synchronous bootstrap.
+var updateSecrets = func(environ environs.Environ, st *api.State) error {
+ secrets, err := environ.Provider().SecretAttrs(environ.Config())
+ if err != nil {
+ return err
+ }
+ client := st.Client()
+ cfg, err := client.EnvironmentGet()
+ if err != nil {
+ return err
+ }
+ for k, v := range secrets {
+ if _, exists := cfg[k]; exists {
+ // Environment already has secrets. Won't send again.
+ return nil
+ } else {
+ cfg[k] = v
+ }
+ }
+ return client.EnvironmentSet(cfg)
+}
+
  // Close terminates the connection to the environment and releases
  // any associated resources.
  func (c *APIConn) Close() error {

Index: juju/apiconn_test.go
=== modified file 'juju/apiconn_test.go'
--- juju/apiconn_test.go 2013-10-24 19:01:23 +0000
+++ juju/apiconn_test.go 2013-11-07 06:48:00 +0000
@@ -52,12 +52,22 @@
   err = bootstrap.Bootstrap(env, constraints.Value{})
   c.Assert(err, gc.IsNil)

+ cfg = env.Config()
+ cfg, err = cfg.Apply(map[string]interface{}{
+ "secret": "fnord",
+ })
+ c.Assert(err, gc.IsNil)
+ err = env.SetConfig(cfg)
+ c.Assert(err, gc.IsNil)
+
   conn, err := juju.NewAPIConn(env, api.DefaultDialOpts())
   c.Assert(err, gc.IsNil)
-
   c.Assert(conn.Environ, gc.Equals, env)
   c.Assert(conn.State, gc.NotNil)

...

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/22720043/

[John A Meinel (jameinel)]

LGTM

I like this quite a bit better than the ones that poked at api.Open.

https://codereview.appspot.com/22720043/

[William Reade (fwereade)]

LGTM, very nice.

https://codereview.appspot.com/22720043/diff/20001/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/22720043/diff/20001/juju/api.go#newcode90
juju/api.go:90: return client.EnvironmentSet(cfg)
Ehh, EnvironmentSet requires a whole config, does it? I guess this is a
one-time thing so it's probably not a big problem.

https://codereview.appspot.com/22720043/diff/20001/juju/apiconn_test.go
File juju/apiconn_test.go (right):

https://codereview.appspot.com/22720043/diff/20001/juju/apiconn_test.go#newcode57
juju/apiconn_test.go:57: "secret": "fnord",
Why did you leave out the value there?

;p

https://codereview.appspot.com/22720043/

[Andrew Wilkins (axwalk)]

https://codereview.appspot.com/22720043/diff/20001/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/22720043/diff/20001/juju/api.go#newcode90
juju/api.go:90: return client.EnvironmentSet(cfg)
On 2013/11/07 09:23:26, fwereade wrote:
> Ehh, EnvironmentSet requires a whole config, does it? I guess this is
a one-time
> thing so it's probably not a big problem.

True, it's sending more than necessary. This is going to be deleted soon
so I'm just going to land as is (otherwise secrets needs to be converted
to map[string]interface{}... not worth the extra lines).

https://codereview.appspot.com/22720043/

[Roger Peppe (rogpeppe)]

LGTM, thanks.

https://codereview.appspot.com/22720043/diff/20001/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/22720043/diff/20001/juju/api.go#newcode54
juju/api.go:54: // TODO(rog): handle errUnauthorized when the API
handles passwords.
d
(this should have gone ages ago!)

https://codereview.appspot.com/22720043/

=== modified file 'juju/api.go'
--- juju/api.go	2013-10-24 18:44:46 +0000
+++ juju/api.go	2013-11-07 07:18:28 +0000
@@ -55,17 +55,45 @@
 	if err != nil {
 		return nil, err
 	}
-	// TODO(rog): implement updateSecrets (see Conn.updateSecrets)
+	// TODO(axw) remove this once we have synchronous bootstrap.
+	if err := updateSecrets(environ, st); err != nil {
+		apiClose(st)
+		return nil, err
+	}
 	return &APIConn{
 		Environ: environ,
 		State:   st,
 	}, nil
 }
 
+// updateSecrets pushes environment secrets to the API server.
+// NOTE: this is a temporary hack, and will disappear when we
+// have synchronous bootstrap.
+var updateSecrets = func(environ environs.Environ, st *api.State) error {
+	secrets, err := environ.Provider().SecretAttrs(environ.Config())
+	if err != nil {
+		return err
+	}
+	client := st.Client()
+	cfg, err := client.EnvironmentGet()
+	if err != nil {
+		return err
+	}
+	for k, v := range secrets {
+		if _, exists := cfg[k]; exists {
+			// Environment already has secrets. Won't send again.
+			return nil
+		} else {
+			cfg[k] = v
+		}
+	}
+	return client.EnvironmentSet(cfg)
+}
+
 // Close terminates the connection to the environment and releases
 // any associated resources.
 func (c *APIConn) Close() error {
-	return c.State.Close()
+	return apiClose(c.State)
 }
 
 // NewAPIClientFromName returns an api.Client connected to the API Server for

=== modified file 'juju/apiconn_test.go'
--- juju/apiconn_test.go	2013-10-24 19:01:23 +0000
+++ juju/apiconn_test.go	2013-11-07 07:18:28 +0000
@@ -52,12 +52,22 @@
 	err = bootstrap.Bootstrap(env, constraints.Value{})
 	c.Assert(err, gc.IsNil)
 
+	cfg = env.Config()
+	cfg, err = cfg.Apply(map[string]interface{}{
+		"secret": "fnord",
+	})
+	c.Assert(err, gc.IsNil)
+	err = env.SetConfig(cfg)
+	c.Assert(err, gc.IsNil)
+
 	conn, err := juju.NewAPIConn(env, api.DefaultDialOpts())
 	c.Assert(err, gc.IsNil)
-
 	c.Assert(conn.Environ, gc.Equals, env)
 	c.Assert(conn.State, gc.NotNil)
 
+	attrs, err := conn.State.Client().EnvironmentGet()
+	c.Assert(attrs["secret"], gc.Equals, "fnord")
+
 	c.Assert(conn.Close(), gc.IsNil)
 }
 
@@ -127,6 +137,7 @@
 		return expectState, nil
 	}
 	defer testbase.PatchValue(juju.APIOpen, apiOpen).Restore()
+	defer testbase.PatchValue(juju.UpdateSecrets, updateSecretsNoop).Restore()
 	st, err := juju.NewAPIFromName("noconfig", store)
 	c.Assert(err, gc.IsNil)
 	c.Assert(st, gc.Equals, expectState)
@@ -175,6 +186,7 @@
 		return nil, expectErr
 	}
 	defer testbase.PatchValue(juju.APIOpen, apiOpen).Restore()
+	defer testbase.PatchValue(juju.UpdateSecrets, updateSecretsNoop).Restore()
 	st, err := juju.NewAPIFromName("noconfig", store)
 	c.Assert(err, gc.Equals, expectErr)
 	c.Assert(st, gc.IsNil)
@@ -201,6 +213,7 @@
 		return cfgOpenedState, nil
 	}
 	defer testbase.PatchValue(juju.APIOpen, apiOpen).Restore()
+	defer testbase.PatchValue(juju.UpdateSecrets, updateSecretsNoop).Restore()
 
 	stateClosed, restoreAPIClose := setAPIClosed()
 	defer restoreAPIClose.Restore()
@@ -265,6 +278,7 @@
 		return cfgOpenedState, nil
 	}
 	defer testbase.PatchValue(juju.APIOpen, apiOpen).Restore()
+	defer testbase.PatchValue(juju.UpdateSecrets, updateSecretsNoop).Restore()
 
 	stateClosed, restoreAPIClose := setAPIClosed()
 	defer restoreAPIClose.Restore()
@@ -415,6 +429,10 @@
 	return stateClosed, testbase.PatchValue(juju.APIClose, apiClose)
 }
 
+func updateSecretsNoop(_ environs.Environ, _ *api.State) error {
+	return nil
+}
+
 // newConfigStoreWithError that will return the given
 // error from ReadInfo.
 func newConfigStoreWithError(err error) configstore.Storage {

=== modified file 'juju/export_test.go'
--- juju/export_test.go	2013-09-19 16:59:12 +0000
+++ juju/export_test.go	2013-11-07 07:18:28 +0000
@@ -5,4 +5,5 @@
 	APIClose             = &apiClose
 	ProviderConnectDelay = &providerConnectDelay
 	NewAPIFromName       = newAPIFromName
+	UpdateSecrets        = &updateSecrets
 )

=== modified file 'state/apiserver/client/client.go'
--- state/apiserver/client/client.go	2013-11-06 14:13:01 +0000
+++ state/apiserver/client/client.go	2013-11-07 07:18:28 +0000
@@ -578,8 +578,11 @@
 		return err
 	}
 	// Make sure we don't allow changing agent-version.
-	if _, found := args.Config["agent-version"]; found {
-		return fmt.Errorf("agent-version cannot be changed")
+	if v, found := args.Config["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)

=== modified file 'state/apiserver/client/client_test.go'
--- state/apiserver/client/client_test.go	2013-11-06 14:13:01 +0000
+++ state/apiserver/client/client_test.go	2013-11-07 07:18:28 +0000
@@ -1261,4 +1261,10 @@
 	args := map[string]interface{}{"agent-version": "9.9.9"}
 	err := s.APIState.Client().EnvironmentSet(args)
 	c.Assert(err, gc.ErrorMatches, "agent-version cannot be changed")
+	// It's okay to pass env back with the same agent-version.
+	cfg, err := s.APIState.Client().EnvironmentGet()
+	c.Assert(err, gc.IsNil)
+	c.Assert(cfg["agent-version"], gc.NotNil)
+	err = s.APIState.Client().EnvironmentSet(cfg)
+	c.Assert(err, gc.IsNil)
 }