Merge lp:~axwalk/juju-core/lp1235100-null-provider-storage into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

provider/null: fix storage issues

- Don't require CAKey to create a storage client
- Do not specify a default for storage-auth-key,
  and fail validation if it's not set.

A machine agent will not have an authkey initially,
as it's a secret. It is transmitted on the first
API connection. We need to fail config validation
until that point.

Fixes #1235100
Fixes #1235084

https://codereview.appspot.com/14387043/

provider/null: fix storage issues

- Don't require CAKey to create a storage client
- Do not specify a default for storage-auth-key,
  and fail validation if it's not set.

A machine agent will not have an authkey initially,
as it's a secret. It is transmitted on the first
API connection. We need to fail config validation
until that point.

Fixes #1235100
Fixes #1235084

https://codereview.appspot.com/14387043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+189221_code.launchpad.net,

Message:
Please take a look.

Description:
provider/null: fix storage issues

- Don't require CAKey to create a storage client
- Fall back to HTTP if we don't have an authkey
- Allow authkey to be omitted from config

A machine agent will not have an authkey initially,
as it's a secret. It is transmitted on the first
API connection. The machine agent doesn't need to
modify storage at this time.

Fixes #1235100
Fixes #1235084

https://code.launchpad.net/~axwalk/juju-core/lp1235100-null-provider-storage/+merge/189221

(do not edit description out of merge proposal)

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

Affected files (+14, -10 lines):
   A [revision details]
   M provider/null/config.go
   M provider/null/config_test.go
   M provider/null/environ.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-20131003170932-4tkr0tu40wb0nfgl
+New revision: <email address hidden>

Index: provider/null/config.go
=== modified file 'provider/null/config.go'
--- provider/null/config.go 2013-09-27 02:31:30 +0000
+++ provider/null/config.go 2013-10-04 07:32:01 +0000
@@ -60,7 +60,8 @@
  }

  func (c *environConfig) storageAuthKey() string {
- return c.attrs["storage-auth-key"].(string)
+ authkey, _ := c.attrs["storage-auth-key"].(string)
+ return authkey
  }

  // storageAddr returns an address for connecting to the

Index: provider/null/config_test.go
=== modified file 'provider/null/config_test.go'
--- provider/null/config_test.go 2013-09-27 02:31:30 +0000
+++ provider/null/config_test.go 2013-10-04 07:32:01 +0000
@@ -28,10 +28,9 @@

  func minimalConfigValues() map[string]interface{} {
   return map[string]interface{}{
- "name": "test",
- "type": provider.Null,
- "bootstrap-host": "hostname",
- "storage-auth-key": "whatever",
+ "name": "test",
+ "type": provider.Null,
+ "bootstrap-host": "hostname",
    // While the ca-cert bits aren't entirely minimal, they avoid the need
    // to set up a fake home.
    "ca-cert": coretesting.CACert,

Index: provider/null/environ.go
=== modified file 'provider/null/environ.go'
--- provider/null/environ.go 2013-10-02 10:38:04 +0000
+++ provider/null/environ.go 2013-10-04 07:32:01 +0000
@@ -165,9 +165,8 @@
   if e.bootstrapStorage != nil {
    return e.bootstrapStorage
   }
- caCertPEM, caKeyPEM := e.StorageCACert(), e.StorageCAKey()
- if caCertPEM != nil && caKeyPEM != nil {
- authkey := e.StorageAuthKey()
+ caCertPEM, authkey := e.StorageCACert(), e.StorageAuthKey()
+ if caCertPEM != nil && authkey != "" {
    storage, err := httpstorage.ClientTLS(e.envConfig().storageAddr(),
caCertPEM, authkey)
    if err != nil {
     // Should be impossible, since ca-cert will always be validated.
@@ -176,9 +175,12 @@
     return storage
    }
   } else {
- logger.Errorf("missing CA cert or private key")
+ logger.Infof("missing CA cert or auth-key")
   }
- return nil
+ // It's not necessarily an error not to have the au...

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/14387043/

[William Reade (fwereade)]

Definitely needs config tests. Otherwise looking good -- the stuff in
environ is not directly testable,but you should be able to verify that
you can't create an environ without the fields you need.

https://codereview.appspot.com/14387043/diff/4001/provider/null/config.go
File provider/null/config.go (right):

https://codereview.appspot.com/14387043/diff/4001/provider/null/config.go#newcode25
provider/null/config.go:25: "storage-auth-key": schema.String(),
Needs a test that this fails verification if missing.

https://codereview.appspot.com/14387043/

[Andrew Wilkins (axwalk)]

https://codereview.appspot.com/14387043/diff/4001/provider/null/config.go
File provider/null/config.go (right):

https://codereview.appspot.com/14387043/diff/4001/provider/null/config.go#newcode25
provider/null/config.go:25: "storage-auth-key": schema.String(),
On 2013/10/04 09:23:04, fwereade wrote:
> Needs a test that this fails verification if missing.

Done, and actually this line should just be deleted (we don't want a
default, and we certainly don't want a default of schema.String() ;))

https://codereview.appspot.com/14387043/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/14387043/

[William Reade (fwereade)]

LGTM pending live verification

https://codereview.appspot.com/14387043/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/14387043/

[Roger Peppe (rogpeppe)]

LGTM with one possible thought

https://codereview.appspot.com/14387043/diff/15001/provider/null/environ.go
File provider/null/environ.go (right):

https://codereview.appspot.com/14387043/diff/15001/provider/null/environ.go#newcode178
provider/null/environ.go:178: logger.Errorf("missing CA cert or
auth-key")
This could actually be a panic, I think, as we shouldn't be able to get
to this stage without both of those things set.

https://codereview.appspot.com/14387043/

=== modified file 'provider/null/config.go'
--- provider/null/config.go	2013-09-27 02:31:30 +0000
+++ provider/null/config.go	2013-10-04 09:36:06 +0000
@@ -22,7 +22,6 @@
 		"bootstrap-user":    "",
 		"storage-listen-ip": "",
 		"storage-port":      8040,
-		"storage-auth-key":  schema.Omit,
 	}
 )
 

=== modified file 'provider/null/config_test.go'
--- provider/null/config_test.go	2013-09-27 02:31:30 +0000
+++ provider/null/config_test.go	2013-10-04 09:36:06 +0000
@@ -61,6 +61,11 @@
 	_, err = nullProvider{}.Validate(testConfig, nil)
 	c.Assert(err, gc.ErrorMatches, "bootstrap-host must be specified")
 
+	testConfig, err = testConfig.Apply(map[string]interface{}{"storage-auth-key": nil})
+	c.Assert(err, gc.IsNil)
+	_, err = nullProvider{}.Validate(testConfig, nil)
+	c.Assert(err, gc.ErrorMatches, "storage-auth-key: expected string, got nothing")
+
 	testConfig = minimalConfig(c)
 	valid, err := nullProvider{}.Validate(testConfig, nil)
 	c.Assert(err, gc.IsNil)

=== modified file 'provider/null/environ.go'
--- provider/null/environ.go	2013-10-02 10:38:04 +0000
+++ provider/null/environ.go	2013-10-04 09:36:06 +0000
@@ -165,9 +165,8 @@
 	if e.bootstrapStorage != nil {
 		return e.bootstrapStorage
 	}
-	caCertPEM, caKeyPEM := e.StorageCACert(), e.StorageCAKey()
-	if caCertPEM != nil && caKeyPEM != nil {
-		authkey := e.StorageAuthKey()
+	caCertPEM, authkey := e.StorageCACert(), e.StorageAuthKey()
+	if caCertPEM != nil && authkey != "" {
 		storage, err := httpstorage.ClientTLS(e.envConfig().storageAddr(), caCertPEM, authkey)
 		if err != nil {
 			// Should be impossible, since ca-cert will always be validated.
@@ -176,7 +175,7 @@
 			return storage
 		}
 	} else {
-		logger.Errorf("missing CA cert or private key")
+		logger.Errorf("missing CA cert or auth-key")
 	}
 	return nil
 }