Reviewers: mp+191367_code.launchpad.net,

Message:
Please take a look.

Description:
provider/maas: do not use environ-uuid

As pointed out by fwereade, it could be
confusing to have two things in the system
that both purport to be a uuid for the environ.

Also make the MAAS EnvironProvider.Validate
check that we're not changing the maas-instance-uuid.

https://code.launchpad.net/~rogpeppe/juju-core/453-maas-instance-uuid/+merge/191367

(do not edit description out of merge proposal)


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

Affected files (+58, -37 lines):
   A [revision details]
   M provider/maas/config.go
   M provider/maas/config_test.go
   M provider/maas/environprovider.go
   M provider/maas/environprovider_test.go
   M provider/maas/maas_test.go
   M provider/maas/storage_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-20131016061150-dqoy06taxa0edywm
+New revision: <email address hidden>

Index: provider/maas/config.go
=== modified file 'provider/maas/config.go'
--- provider/maas/config.go	2013-10-15 16:18:32 +0000
+++ provider/maas/config.go	2013-10-16 10:29:29 +0000
@@ -18,14 +18,14 @@
  	// maas-oauth is a colon-separated triplet of:
  	// consumer-key:resource-token:resource-secret
  	"maas-oauth": schema.String(),
-	// environment-uuid is an optional UUID to group the machines
+	// maas-instance-uuid is an optional UUID to group the instances
  	// acquired from MAAS, to support multiple environments per MAAS user.
-	"environment-uuid": schema.String(),
+	"maas-instance-uuid": schema.String(),
  }
  var configDefaults = schema.Defaults{
-	// For backward-compatibility, environment-uuid is the empty string
+	// For backward-compatibility, maas-instance-uuid is the empty string
  	// by default. However, new environments should all use a UUID.
-	"environment-uuid": "",
+	"maas-instance-uuid": "",
  }

  type maasEnvironConfig struct {
@@ -42,7 +42,7 @@
  }

  func (cfg *maasEnvironConfig) maasEnvironmentUUID() string {
-	if uuid, ok := cfg.attrs["environment-uuid"].(string); ok {
+	if uuid, ok := cfg.attrs["maas-instance-uuid"].(string); ok {
  		return uuid
  	}
  	return ""
@@ -72,6 +72,12 @@
  	if err != nil {
  		return nil, err
  	}
+	if oldCfg != nil {
+		oldAttrs := oldCfg.UnknownAttrs()
+		if validated["maas-instance-uuid"] != oldAttrs["maas-instance-uuid"] {
+			return nil, fmt.Errorf("cannot change maas-instance-uuid")
+		}
+	}
  	envCfg := new(maasEnvironConfig)
  	envCfg.Config = cfg
  	envCfg.attrs = validated


Index: provider/maas/config_test.go
=== modified file 'provider/maas/config_test.go'
--- provider/maas/config_test.go	2013-10-15 16:52:58 +0000
+++ provider/maas/config_test.go	2013-10-16 10:42:01 +0000
@@ -47,10 +47,10 @@
  	uuid, err := utils.NewUUID()
  	c.Assert(err, gc.IsNil)
  	ecfg, err := newConfig(map[string]interface{}{
-		"maas-server":      server,
-		"maas-oauth":       oauth,
-		"environment-uuid": uuid.String(),
-		"future-key":       future,
+		"maas-server":        server,
+		"maas-oauth":         oauth,
+		"maas-instance-uuid": uuid.String(),
+		"future-key":         future,
  	})
  	c.Assert(err, gc.IsNil)
  	c.Check(ecfg.maasServer(), gc.Equals, server)
@@ -105,3 +105,19 @@
  	c.Assert(err, gc.NotNil)
  	c.Check(err, gc.ErrorMatches, ".*cannot change name.*")
  }
+
+func (*configSuite) TestValidateCannotChangeUUID(c *gc.C) {
+	baseAttrs := map[string]interface{}{
+		"maas-server":        "http://maas.testing.invalid/maas/",
+		"maas-oauth":         "consumer-key:resource-token:resource-secret",
+		"maas-instance-uuid": "1234-5678",
+	}
+	oldCfg, err := newConfig(baseAttrs)
+	c.Assert(err, gc.IsNil)
+	newCfg, err := oldCfg.Apply(map[string]interface{}{
+		"maas-instance-uuid": "9876-5432",
+	})
+	c.Assert(err, gc.IsNil)
+	_, err = maasEnvironProvider{}.Validate(newCfg, oldCfg.Config)
+	c.Assert(err, gc.ErrorMatches, "cannot change maas-instance-uuid")
+}


Index: provider/maas/environprovider.go
=== modified file 'provider/maas/environprovider.go'
--- provider/maas/environprovider.go	2013-10-15 16:36:59 +0000
+++ provider/maas/environprovider.go	2013-10-16 10:29:29 +0000
@@ -39,23 +39,20 @@
  }

  var errUUIDAlreadySet = errors.New(
-	"environment-uuid is already set; this should not be set by hand")
+	"maas-instance-uuid is already set; this should not be set by hand")

  func (p maasEnvironProvider) Prepare(cfg *config.Config)  
(environs.Environ, error) {
  	attrs := cfg.UnknownAttrs()
-	uuid, found := attrs["environment-uuid"]
-	if found {
-		if uuid != "" {
-			return nil, errUUIDAlreadySet
-		}
-	} else {
-		uuid, err := utils.NewUUID()
-		if err != nil {
-			return nil, err
-		}
-		attrs["environment-uuid"] = uuid.String()
-	}
-	cfg, err := cfg.Apply(attrs)
+	oldUUID, found := attrs["maas-instance-uuid"]
+	if found && oldUUID != "" {
+		return nil, errUUIDAlreadySet
+	}
+	uuid, err := utils.NewUUID()
+	if err != nil {
+		return nil, err
+	}
+	attrs["maas-instance-uuid"] = uuid.String()
+	cfg, err = cfg.Apply(attrs)
  	if err != nil {
  		return nil, err
  	}


Index: provider/maas/environprovider_test.go
=== modified file 'provider/maas/environprovider_test.go'
--- provider/maas/environprovider_test.go	2013-10-15 16:36:59 +0000
+++ provider/maas/environprovider_test.go	2013-10-16 10:29:29 +0000
@@ -11,6 +11,7 @@

  	"launchpad.net/juju-core/environs/config"
  	"launchpad.net/juju-core/testing"
+	jc "launchpad.net/juju-core/testing/checkers"
  	"launchpad.net/juju-core/utils"
  )

@@ -56,21 +57,20 @@
  	preparedConfig := environ.Config()
  	unknownAttrs := preparedConfig.UnknownAttrs()

-	uuid, ok := unknownAttrs["environment-uuid"]
-	c.Assert(ok, gc.Equals, true)
+	uuid, ok := unknownAttrs["maas-instance-uuid"]

-	_, err = utils.UUIDFromString(uuid.(string))
-	c.Assert(err, gc.IsNil)
+	c.Assert(ok, jc.IsTrue)
+	c.Assert(uuid, jc.Satisfies, utils.IsValidUUIDString)
  }

  func (suite *EnvironProviderSuite)  
TestEnvironmentUUIDShouldNotBeSetByHand(c *gc.C) {
  	testJujuHome := c.MkDir()
  	defer config.SetJujuHome(config.SetJujuHome(testJujuHome))
  	attrs := testing.FakeConfig().Merge(testing.Attrs{
-		"type":             "maas",
-		"maas-oauth":       "aa:bb:cc",
-		"maas-server":      "http://maas.testing.invalid/maas/",
-		"environment-uuid": "foobar",
+		"type":               "maas",
+		"maas-oauth":         "aa:bb:cc",
+		"maas-server":        "http://maas.testing.invalid/maas/",
+		"maas-instance-uuid": "foobar",
  	})
  	config, err := config.New(config.NoDefaults, attrs)
  	c.Assert(err, gc.IsNil)


Index: provider/maas/maas_test.go
=== modified file 'provider/maas/maas_test.go'
--- provider/maas/maas_test.go	2013-10-15 16:52:58 +0000
+++ provider/maas/maas_test.go	2013-10-16 10:29:29 +0000
@@ -57,11 +57,11 @@
  // makeEnviron creates a functional maasEnviron for a test.
  func (suite *providerSuite) makeEnviron() *maasEnviron {
  	attrs := coretesting.FakeConfig().Merge(coretesting.Attrs{
-		"name":             "test env",
-		"type":             "maas",
-		"maas-oauth":       "a:b:c",
-		"maas-server":      suite.testMAASObject.TestServer.URL,
-		"environment-uuid": exampleUUID,
+		"name":               "test env",
+		"type":               "maas",
+		"maas-oauth":         "a:b:c",
+		"maas-server":        suite.testMAASObject.TestServer.URL,
+		"maas-instance-uuid": exampleUUID,
  	})
  	cfg, err := config.New(config.NoDefaults, attrs)
  	if err != nil {


Index: provider/maas/storage_test.go
=== modified file 'provider/maas/storage_test.go'
--- provider/maas/storage_test.go	2013-10-16 05:21:12 +0000
+++ provider/maas/storage_test.go	2013-10-16 10:29:29 +0000
@@ -413,7 +413,7 @@
  	sstor := NewStorage(s.makeEnviron())
  	stor := sstor.(*maasStorage)
  	ecfg := stor.environUnlocked.ecfg()
-	ecfg.attrs["environment-uuid"] = ""
+	ecfg.attrs["maas-instance-uuid"] = ""

  	const name = "myname"
  	c.Assert(stor.prefixWithPrivateNamespace(name), gc.Equals, name)