Merge lp:~allenap/juju-core/maas-environment-uuid-use into lp:~go-bot/juju-core/trunk

[Gavin Panella]

Use environment-uuid when interacting with MAAS

The environment-uuid is passed - as agent_name - to MAAS when acquiring
and listing nodes. This will allow multiple Juju environments to coexist
in a single MAAS user's account.

In addition, it prevents environment-uuid from being set in
environments.yaml, and returns an appropriate error message. This is
intended to stop people from inadvertently clobbering existing
environments.

It also starts cleaning up MAAS's providerSuite by dropping the environ
attribute in favour of makeEnviron(). The former was initialised to a
half-working environ that caused strange test failures.

https://codereview.appspot.com/14644045/

Use environment-uuid when interacting with MAAS

The environment-uuid is passed - as agent_name - to MAAS when acquiring
and listing nodes. This will allow multiple Juju environments to coexist
in a single MAAS user's account.

In addition, it prevents environment-uuid from being set in
environments.yaml, and returns an appropriate error message. This is
intended to stop people from inadvertently clobbering existing
environments.

It also starts cleaning up MAAS's providerSuite by dropping the environ
attribute in favour of makeEnviron(). The former was initialised to a
half-working environ that caused strange test failures.

https://codereview.appspot.com/14644045/

[Gavin Panella (allenap)]

Reviewers: mp+191249_code.launchpad.net,

Message:
Please take a look.

Description:
Use environment-uuid when interacting with MAAS

The environment-uuid is passed - as agent_name - to MAAS when acquiring
and listing nodes. This will allow multiple Juju environments to coexist
in a single MAAS user's account.

In addition, it prevents environment-uuid from being set in
environments.yaml, and returns an appropriate error message. This is
intended to stop people from inadvertently clobbering existing
environments.

It also starts cleaning up MAAS's providerSuite by dropping the environ
attribute in favour of makeEnviron(). The former was initialised to a
half-working environ that caused strange test failures.

https://code.launchpad.net/~allenap/juju-core/maas-environment-uuid-use/+merge/191249

(do not edit description out of merge proposal)

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

Affected files (+188, -76 lines):
   A [revision details]
   M provider/maas/config.go
   M provider/maas/config_test.go
   M provider/maas/environ.go
   M provider/maas/environ_test.go
   M provider/maas/environprovider.go
   M provider/maas/environprovider_test.go
   M provider/maas/instance_test.go
   M provider/maas/maas_test.go
   M provider/maas/storage_test.go

[Gavin Panella (allenap)]

Please take a look.

https://codereview.appspot.com/14644045/

[Tim Penhey (thumper)]

LGTM - with one question and one concern.

What does older MAAS installs do with unexpected GET parameters? When
the new agent_name field is passed through, is this going to cause
problems with older systems? Also, I wonder where we should document
the required versions.

I foresee a problem when a new juju is running against an older MAAS,
the new agent_name is passed through, and also passed through as a
filter when listing. Is the unknown field similarly ignored by the
filter code?

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

https://codereview.appspot.com/14644045/diff/4001/provider/maas/config.go#newcode23
provider/maas/config.go:23: "environment-uuid": schema.String(),
I do think that we should move this into the general environ config, but
this can happen later. For expediency, I'm happy to have this just in
MAAS for now.

https://codereview.appspot.com/14644045/diff/4001/provider/maas/environ.go
File provider/maas/environ.go (right):

https://codereview.appspot.com/14644045/diff/4001/provider/maas/environ.go#newcode174
provider/maas/environ.go:174: acquireParams.Add("agent_name",
environ.ecfg().maasEnvironmentUUID())
Is "agent_name" a new MAAS parameter? The current docs don't list it.

https://codereview.appspot.com/14644045/diff/4001/provider/maas/environ.go#newcode326
provider/maas/environ.go:326: filter.Add("agent_name",
environ.ecfg().maasEnvironmentUUID())
Beautiful :-)

https://codereview.appspot.com/14644045/diff/4001/provider/maas/environprovider.go
File provider/maas/environprovider.go (right):

https://codereview.appspot.com/14644045/diff/4001/provider/maas/environprovider.go#newcode45
provider/maas/environprovider.go:45: attrs := cfg.UnknownAttrs()
Didn't this bit land as r1984 on lp:juju-core?

https://codereview.appspot.com/14644045/diff/4001/provider/maas/instance_test.go
File provider/maas/instance_test.go (right):

https://codereview.appspot.com/14644045/diff/4001/provider/maas/instance_test.go#newcode39
provider/maas/instance_test.go:39: func (s *instanceTest)
TestStringWithoutHostname(c *gc.C) {
Thank you. There is a tendency in some juju tests to have too much
tested in one test.

https://codereview.appspot.com/14644045/

[Tim Penhey (thumper)]

As long as the "agent_name" is ignored by older MAAS versions, this should be fine.

review: Approve (conditional)

[Julian Edwards (julian-edwards)]

It will be ignored, by design. This is a good thing, it basically leaves the functionality unaltered on old maases.

=== modified file 'provider/maas/config.go'
--- provider/maas/config.go	2013-08-09 04:58:34 +0000
+++ provider/maas/config.go	2013-10-15 16:59:34 +0000
@@ -18,8 +18,15 @@
 	// maas-oauth is a colon-separated triplet of:
 	// consumer-key:resource-token:resource-secret
 	"maas-oauth": schema.String(),
-}
-var configDefaults = schema.Defaults{}
+	// environment-uuid is an optional UUID to group the machines
+	// acquired from MAAS, to support multiple environments per MAAS user.
+	"environment-uuid": schema.String(),
+}
+var configDefaults = schema.Defaults{
+	// For backward-compatibility, environment-uuid is the empty string
+	// by default. However, new environments should all use a UUID.
+	"environment-uuid": "",
+}
 
 type maasEnvironConfig struct {
 	*config.Config
@@ -34,6 +41,13 @@
 	return cfg.attrs["maas-oauth"].(string)
 }
 
+func (cfg *maasEnvironConfig) maasEnvironmentUUID() string {
+	if uuid, ok := cfg.attrs["environment-uuid"].(string); ok {
+		return uuid
+	}
+	return ""
+}
+
 func (prov maasEnvironProvider) newConfig(cfg *config.Config) (*maasEnvironConfig, error) {
 	validCfg, err := prov.Validate(cfg, nil)
 	if err != nil {
 
=== modified file 'provider/maas/config_test.go'
--- provider/maas/config_test.go	2013-09-20 02:33:04 +0000
+++ provider/maas/config_test.go	2013-10-15 16:59:34 +0000
@@ -9,6 +9,7 @@
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/testing"
 	"launchpad.net/juju-core/testing/testbase"
+	"launchpad.net/juju-core/utils"
 )
 
 type configSuite struct {
@@ -43,17 +44,30 @@
 	server := "http://maas.testing.invalid/maas/"
 	oauth := "consumer-key:resource-token:resource-secret"
 	future := "futurama"
+	uuid, err := utils.NewUUID()
+	c.Assert(err, gc.IsNil)
 	ecfg, err := newConfig(map[string]interface{}{
-		"maas-server": server,
-		"maas-oauth":  oauth,
-		"future-key":  future,
+		"maas-server":      server,
+		"maas-oauth":       oauth,
+		"environment-uuid": uuid.String(),
+		"future-key":       future,
 	})
 	c.Assert(err, gc.IsNil)
 	c.Check(ecfg.maasServer(), gc.Equals, server)
 	c.Check(ecfg.maasOAuth(), gc.DeepEquals, oauth)
+	c.Check(ecfg.maasEnvironmentUUID(), gc.Equals, uuid.String())
 	c.Check(ecfg.UnknownAttrs()["future-key"], gc.DeepEquals, future)
 }
 
+func (*configSuite) TestEnvironmentUUIDDefault(c *gc.C) {
+	ecfg, err := newConfig(map[string]interface{}{
+		"maas-server": "http://maas.testing.invalid/maas/",
+		"maas-oauth":  "consumer-key:resource-token:resource-secret",
+	})
+	c.Assert(err, gc.IsNil)
+	c.Check(ecfg.maasEnvironmentUUID(), gc.Equals, "")
+}
+
 func (*configSuite) TestChecksWellFormedMaasServer(c *gc.C) {
 	_, err := newConfig(map[string]interface{}{
 		"maas-server": "This should have been a URL.",
 
=== modified file 'provider/maas/environ.go'
--- provider/maas/environ.go	2013-10-02 00:29:29 +0000
+++ provider/maas/environ.go	2013-10-15 16:59:34 +0000
@@ -170,12 +170,13 @@
 
 // acquireNode allocates a node from the MAAS.
 func (environ *maasEnviron) acquireNode(cons constraints.Value, possibleTools tools.List) (gomaasapi.MAASObject, *tools.Tools, error) {
-	constraintsParams := convertConstraints(cons)
+	acquireParams := convertConstraints(cons)
+	acquireParams.Add("agent_name", environ.ecfg().maasEnvironmentUUID())
 	var result gomaasapi.JSONObject
 	var err error
 	for a := shortAttempt.Start(); a.Next(); {
 		client := environ.getMAASClient().GetSubObject("nodes/")
-		result, err = client.CallPost("acquire", constraintsParams)
+		result, err = client.CallPost("acquire", acquireParams)
 		if err == nil {
 			break
 		}
@@ -322,6 +323,7 @@
 func (environ *maasEnviron) instances(ids []instance.Id) ([]instance.Instance, error) {
 	nodeListing := environ.getMAASClient().GetSubObject("nodes")
 	filter := getSystemIdValues(ids)
+	filter.Add("agent_name", environ.ecfg().maasEnvironmentUUID())
 	listNodeObjects, err := nodeListing.CallGet("list", filter)
 	if err != nil {
 		return nil, err
 
=== modified file 'provider/maas/environ_test.go'
--- provider/maas/environ_test.go	2013-10-01 12:03:33 +0000
+++ provider/maas/environ_test.go	2013-10-15 16:59:34 +0000
@@ -26,7 +26,6 @@
 	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/juju/testing"
 	"launchpad.net/juju-core/provider/common"
-	coretesting "launchpad.net/juju-core/testing"
 	jc "launchpad.net/juju-core/testing/checkers"
 	"launchpad.net/juju-core/tools"
 	"launchpad.net/juju-core/utils"
@@ -54,31 +53,12 @@
 	return ecfg.Config
 }
 
-// makeEnviron creates a functional maasEnviron for a test.
-func (suite *environSuite) makeEnviron() *maasEnviron {
-	attrs := coretesting.FakeConfig().Merge(coretesting.Attrs{
-		"name":        suite.environ.Name(),
-		"type":        "maas",
-		"maas-oauth":  "a:b:c",
-		"maas-server": suite.testMAASObject.TestServer.URL,
-	})
-	cfg, err := config.New(config.NoDefaults, attrs)
-	if err != nil {
-		panic(err)
-	}
-	env, err := NewEnviron(cfg)
-	if err != nil {
-		panic(err)
-	}
-	return env
-}
-
 func (suite *environSuite) setupFakeProviderStateFile(c *gc.C) {
 	suite.testMAASObject.TestServer.NewFile(common.StateFile, []byte("test file content"))
 }
 
 func (suite *environSuite) setupFakeTools(c *gc.C) {
-	stor := NewStorage(suite.environ)
+	stor := NewStorage(suite.makeEnviron())
 	envtesting.UploadFakeTools(c, stor)
 }
 
@@ -139,7 +119,7 @@
 	resourceURI, _ := node.GetField("resource_uri")
 	instanceIds := []instance.Id{instance.Id(resourceURI)}
 
-	instances, err := suite.environ.Instances(instanceIds)
+	instances, err := suite.makeEnviron().Instances(instanceIds)
 
 	c.Check(err, gc.IsNil)
 	c.Check(len(instances), gc.Equals, 1)
@@ -149,7 +129,7 @@
 func (suite *environSuite) TestInstancesReturnsErrNoInstancesIfEmptyParameter(c *gc.C) {
 	input := `{"system_id": "test"}`
 	suite.testMAASObject.TestServer.NewNode(input)
-	instances, err := suite.environ.Instances([]instance.Id{})
+	instances, err := suite.makeEnviron().Instances([]instance.Id{})
 
 	c.Check(err, gc.Equals, environs.ErrNoInstances)
 	c.Check(instances, gc.IsNil)
@@ -158,14 +138,14 @@
 func (suite *environSuite) TestInstancesReturnsErrNoInstancesIfNilParameter(c *gc.C) {
 	input := `{"system_id": "test"}`
 	suite.testMAASObject.TestServer.NewNode(input)
-	instances, err := suite.environ.Instances(nil)
+	instances, err := suite.makeEnviron().Instances(nil)
 
 	c.Check(err, gc.Equals, environs.ErrNoInstances)
 	c.Check(instances, gc.IsNil)
 }
 
 func (suite *environSuite) TestInstancesReturnsErrNoInstancesIfNoneFound(c *gc.C) {
-	_, err := suite.environ.Instances([]instance.Id{"unknown"})
+	_, err := suite.makeEnviron().Instances([]instance.Id{"unknown"})
 	c.Check(err, gc.Equals, environs.ErrNoInstances)
 }
 
@@ -174,7 +154,7 @@
 	node := suite.testMAASObject.TestServer.NewNode(input)
 	resourceURI, _ := node.GetField("resource_uri")
 
-	instances, err := suite.environ.AllInstances()
+	instances, err := suite.makeEnviron().AllInstances()
 
 	c.Check(err, gc.IsNil)
 	c.Check(len(instances), gc.Equals, 1)
@@ -182,7 +162,7 @@
 }
 
 func (suite *environSuite) TestAllInstancesReturnsEmptySliceIfNoInstance(c *gc.C) {
-	instances, err := suite.environ.AllInstances()
+	instances, err := suite.makeEnviron().AllInstances()
 
 	c.Check(err, gc.IsNil)
 	c.Check(len(instances), gc.Equals, 0)
@@ -198,7 +178,7 @@
 	instanceId2 := instance.Id("unknown systemID")
 	instanceIds := []instance.Id{instanceId1, instanceId2}
 
-	instances, err := suite.environ.Instances(instanceIds)
+	instances, err := suite.makeEnviron().Instances(instanceIds)
 
 	c.Check(err, gc.Equals, environs.ErrPartialInstances)
 	c.Check(len(instances), gc.Equals, 1)
@@ -298,7 +278,7 @@
 }
 
 func (suite *environSuite) TestAcquireNode(c *gc.C) {
-	stor := NewStorage(suite.environ)
+	stor := NewStorage(suite.makeEnviron())
 	fakeTools := envtesting.MustUploadFakeToolsVersions(stor, version.Current)[0]
 	env := suite.makeEnviron()
 	suite.testMAASObject.TestServer.NewNode(`{"system_id": "node0", "hostname": "host0"}`)
@@ -313,7 +293,7 @@
 }
 
 func (suite *environSuite) TestAcquireNodeTakesConstraintsIntoAccount(c *gc.C) {
-	stor := NewStorage(suite.environ)
+	stor := NewStorage(suite.makeEnviron())
 	fakeTools := envtesting.MustUploadFakeToolsVersions(stor, version.Current)[0]
 	env := suite.makeEnviron()
 	suite.testMAASObject.TestServer.NewNode(`{"system_id": "node0", "hostname": "host0"}`)
@@ -329,6 +309,21 @@
 	c.Assert(nodeRequestValues[0].Get("mem"), gc.Equals, "1024")
 }
 
+func (suite *environSuite) TestAcquireNodePassedEnvironmentUUID(c *gc.C) {
+	stor := NewStorage(suite.makeEnviron())
+	fakeTools := envtesting.MustUploadFakeToolsVersions(stor, version.Current)[0]
+	env := suite.makeEnviron()
+	suite.testMAASObject.TestServer.NewNode(`{"system_id": "node0", "hostname": "host0"}`)
+
+	_, _, err := env.acquireNode(constraints.Value{}, tools.List{fakeTools})
+
+	c.Check(err, gc.IsNil)
+	requestValues := suite.testMAASObject.TestServer.NodeOperationRequestValues()
+	nodeRequestValues, found := requestValues["node0"]
+	c.Assert(found, gc.Equals, true)
+	c.Assert(nodeRequestValues[0].Get("agent_name"), gc.Equals, exampleUUID)
+}
+
 func (*environSuite) TestConvertConstraints(c *gc.C) {
 	var testValues = []struct {
 		constraints    constraints.Value
@@ -352,13 +347,13 @@
 func (suite *environSuite) getInstance(systemId string) *maasInstance {
 	input := `{"system_id": "` + systemId + `"}`
 	node := suite.testMAASObject.TestServer.NewNode(input)
-	return &maasInstance{&node, suite.environ}
+	return &maasInstance{&node, suite.makeEnviron()}
 }
 
 func (suite *environSuite) TestStopInstancesReturnsIfParameterEmpty(c *gc.C) {
 	suite.getInstance("test1")
 
-	err := suite.environ.StopInstances([]instance.Instance{})
+	err := suite.makeEnviron().StopInstances([]instance.Instance{})
 	c.Check(err, gc.IsNil)
 	operations := suite.testMAASObject.TestServer.NodeOperations()
 	c.Check(operations, gc.DeepEquals, map[string][]string{})
@@ -370,7 +365,7 @@
 	suite.getInstance("test3")
 	instances := []instance.Instance{instance1, instance2}
 
-	err := suite.environ.StopInstances(instances)
+	err := suite.makeEnviron().StopInstances(instances)
 
 	c.Check(err, gc.IsNil)
 	operations := suite.testMAASObject.TestServer.NodeOperations()
@@ -383,7 +378,7 @@
 	hostname := "test"
 	input := `{"system_id": "system_id", "hostname": "` + hostname + `"}`
 	node := suite.testMAASObject.TestServer.NewNode(input)
-	testInstance := &maasInstance{&node, suite.environ}
+	testInstance := &maasInstance{&node, suite.makeEnviron()}
 	err := common.SaveState(
 		env.Storage(),
 		&common.BootstrapState{StateInstances: []instance.Id{testInstance.Id()}})
 
=== modified file 'provider/maas/environprovider.go'
--- provider/maas/environprovider.go	2013-10-11 14:05:04 +0000
+++ provider/maas/environprovider.go	2013-10-15 16:59:34 +0000
@@ -4,6 +4,7 @@
 package maas
 
 import (
+	"errors"
 	"os"
 
 	"launchpad.net/loggo"
@@ -37,8 +38,27 @@
 	return env, nil
 }
 
+var errUUIDAlreadySet = errors.New(
+	"environment-uuid is already set; this should not be set by hand")
+
 func (p maasEnvironProvider) Prepare(cfg *config.Config) (environs.Environ, error) {
-	// TODO any attributes to prepare?
+	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)
+	if err != nil {
+		return nil, err
+	}
 	return p.Open(cfg)
 }
 
 
=== modified file 'provider/maas/environprovider_test.go'
--- provider/maas/environprovider_test.go	2013-09-25 17:04:52 +0000
+++ provider/maas/environprovider_test.go	2013-10-15 16:59:34 +0000
@@ -11,6 +11,7 @@
 
 	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/testing"
+	"launchpad.net/juju-core/utils"
 )
 
 type EnvironProviderSuite struct {
@@ -31,13 +32,53 @@
 	config, err := config.New(config.NoDefaults, attrs)
 	c.Assert(err, gc.IsNil)
 
-	secretAttrs, err := suite.environ.Provider().SecretAttrs(config)
+	secretAttrs, err := suite.makeEnviron().Provider().SecretAttrs(config)
 	c.Assert(err, gc.IsNil)
 
 	expectedAttrs := map[string]string{"maas-oauth": oauth}
 	c.Check(secretAttrs, gc.DeepEquals, expectedAttrs)
 }
 
+func (suite *EnvironProviderSuite) TestUnknownAttrsContainEnvironmentUUID(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/",
+	})
+	config, err := config.New(config.NoDefaults, attrs)
+	c.Assert(err, gc.IsNil)
+
+	environ, err := suite.makeEnviron().Provider().Prepare(config)
+	c.Assert(err, gc.IsNil)
+
+	preparedConfig := environ.Config()
+	unknownAttrs := preparedConfig.UnknownAttrs()
+
+	uuid, ok := unknownAttrs["environment-uuid"]
+	c.Assert(ok, gc.Equals, true)
+
+	_, err = utils.UUIDFromString(uuid.(string))
+	c.Assert(err, gc.IsNil)
+}
+
+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",
+	})
+	config, err := config.New(config.NoDefaults, attrs)
+	c.Assert(err, gc.IsNil)
+
+	_, err = suite.makeEnviron().Provider().Prepare(config)
+	c.Assert(err, gc.Equals, errUUIDAlreadySet)
+}
+
 // create a temporary file with the given content.  The file will be cleaned
 // up at the end of the test calling this method.
 func createTempFile(c *gc.C, content []byte) string {
@@ -65,7 +106,7 @@
 	_MAASInstanceFilename = filename
 	defer func() { _MAASInstanceFilename = old_MAASInstanceFilename }()
 
-	provider := suite.environ.Provider()
+	provider := suite.makeEnviron().Provider()
 	publicAddress, err := provider.PublicAddress()
 	c.Assert(err, gc.IsNil)
 	c.Check(publicAddress, gc.Equals, hostname)
@@ -85,7 +126,7 @@
 	})
 	config, err := config.New(config.NoDefaults, attrs)
 	c.Assert(err, gc.IsNil)
-	env, err := suite.environ.Provider().Open(config)
+	env, err := suite.makeEnviron().Provider().Open(config)
 	// When Open() fails (i.e. returns a non-nil error), it returns an
 	// environs.Environ interface object with a nil value and a nil
 	// type.
 
=== modified file 'provider/maas/instance_test.go'
--- provider/maas/instance_test.go	2013-10-10 11:40:54 +0000
+++ provider/maas/instance_test.go	2013-10-15 16:59:34 +0000
@@ -21,7 +21,7 @@
 	jsonValue := `{"system_id": "system_id", "test": "test"}`
 	obj := s.testMAASObject.TestServer.NewNode(jsonValue)
 	resourceURI, _ := obj.GetField("resource_uri")
-	instance := maasInstance{&obj, s.environ}
+	instance := maasInstance{&obj, s.makeEnviron()}
 
 	c.Check(string(instance.Id()), gc.Equals, resourceURI)
 }
@@ -29,19 +29,21 @@
 func (s *instanceTest) TestString(c *gc.C) {
 	jsonValue := `{"hostname": "thethingintheplace", "system_id": "system_id", "test": "test"}`
 	obj := s.testMAASObject.TestServer.NewNode(jsonValue)
-	instance := &maasInstance{&obj, s.environ}
+	instance := &maasInstance{&obj, s.makeEnviron()}
 	hostname, err := instance.DNSName()
 	c.Assert(err, gc.IsNil)
 	expected := hostname + ":" + string(instance.Id())
 	c.Assert(fmt.Sprint(instance), gc.Equals, expected)
+}
 
+func (s *instanceTest) TestStringWithoutHostname(c *gc.C) {
 	// For good measure, test what happens if we don't have a hostname.
-	jsonValue = `{"system_id": "system_id", "test": "test"}`
-	obj = s.testMAASObject.TestServer.NewNode(jsonValue)
-	instance = &maasInstance{&obj, s.environ}
-	hostname, err = instance.DNSName()
+	jsonValue := `{"system_id": "system_id", "test": "test"}`
+	obj := s.testMAASObject.TestServer.NewNode(jsonValue)
+	instance := &maasInstance{&obj, s.makeEnviron()}
+	_, err := instance.DNSName()
 	c.Assert(err, gc.NotNil)
-	expected = fmt.Sprintf("<DNSName failed: %q>", err) + ":" + string(instance.Id())
+	expected := fmt.Sprintf("<DNSName failed: %q>", err) + ":" + string(instance.Id())
 	c.Assert(fmt.Sprint(instance), gc.Equals, expected)
 }
 
@@ -49,7 +51,7 @@
 	jsonValue := `{"system_id": "system_id", "test": "test"}`
 	obj := s.testMAASObject.TestServer.NewNode(jsonValue)
 	s.testMAASObject.TestServer.ChangeNode("system_id", "test2", "test2")
-	instance := maasInstance{&obj, s.environ}
+	instance := maasInstance{&obj, s.makeEnviron()}
 
 	err := instance.refreshInstance()
 
@@ -62,7 +64,7 @@
 func (s *instanceTest) TestDNSName(c *gc.C) {
 	jsonValue := `{"hostname": "DNS name", "system_id": "system_id"}`
 	obj := s.testMAASObject.TestServer.NewNode(jsonValue)
-	instance := maasInstance{&obj, s.environ}
+	instance := maasInstance{&obj, s.makeEnviron()}
 
 	dnsName, err := instance.DNSName()
 
@@ -83,7 +85,7 @@
 			"ip_addresses": [ "1.2.3.4", "fe80::d806:dbff:fe23:1199" ]
 		}`
 	obj := s.testMAASObject.TestServer.NewNode(jsonValue)
-	inst := maasInstance{&obj, s.environ}
+	inst := maasInstance{&obj, s.makeEnviron()}
 
 	expected := []instance.Address{
 		{Value: "testing.invalid", Type: instance.HostName, NetworkScope: instance.NetworkPublic},
@@ -105,7 +107,7 @@
 		"system_id": "system_id"
 		}`
 	obj := s.testMAASObject.TestServer.NewNode(jsonValue)
-	inst := maasInstance{&obj, s.environ}
+	inst := maasInstance{&obj, s.makeEnviron()}
 
 	addr, err := inst.Addresses()
 	c.Assert(err, gc.IsNil)
@@ -121,7 +123,7 @@
 		"ip_addresses": "incompatible"
 		}`
 	obj := s.testMAASObject.TestServer.NewNode(jsonValue)
-	inst := maasInstance{&obj, s.environ}
+	inst := maasInstance{&obj, s.makeEnviron()}
 
 	_, err := inst.Addresses()
 	c.Assert(err, gc.NotNil)
@@ -134,7 +136,7 @@
 		"ip_addresses": [42]
 		}`
 	obj := s.testMAASObject.TestServer.NewNode(jsonValue)
-	inst := maasInstance{&obj, s.environ}
+	inst := maasInstance{&obj, s.makeEnviron()}
 
 	_, err := inst.Addresses()
 	c.Assert(err, gc.NotNil)
 
=== modified file 'provider/maas/maas_test.go'
--- provider/maas/maas_test.go	2013-09-20 02:53:59 +0000
+++ provider/maas/maas_test.go	2013-10-15 16:59:34 +0000
@@ -9,7 +9,9 @@
 	gc "launchpad.net/gocheck"
 	"launchpad.net/gomaasapi"
 
+	"launchpad.net/juju-core/environs/config"
 	envtesting "launchpad.net/juju-core/environs/testing"
+	coretesting "launchpad.net/juju-core/testing"
 	"launchpad.net/juju-core/testing/testbase"
 )
 
@@ -20,7 +22,6 @@
 type providerSuite struct {
 	testbase.LoggingSuite
 	envtesting.ToolsFixture
-	environ         *maasEnviron
 	testMAASObject  *gomaasapi.TestMAASObject
 	restoreTimeouts func()
 }
@@ -32,7 +33,6 @@
 	s.LoggingSuite.SetUpSuite(c)
 	TestMAASObject := gomaasapi.NewTestMAAS("1.0")
 	s.testMAASObject = TestMAASObject
-	s.environ = &maasEnviron{name: "test env", maasClientUnlocked: &TestMAASObject.MAASObject}
 }
 
 func (s *providerSuite) SetUpTest(c *gc.C) {
@@ -51,3 +51,25 @@
 	s.restoreTimeouts()
 	s.LoggingSuite.TearDownSuite(c)
 }
+
+const exampleUUID = "dfb69555-0bc4-4d1f-85f2-4ee390974984"
+
+// 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,
+	})
+	cfg, err := config.New(config.NoDefaults, attrs)
+	if err != nil {
+		panic(err)
+	}
+	env, err := NewEnviron(cfg)
+	if err != nil {
+		panic(err)
+	}
+	return env
+}
 
=== modified file 'provider/maas/storage_test.go'
--- provider/maas/storage_test.go	2013-09-18 22:54:32 +0000
+++ provider/maas/storage_test.go	2013-10-15 16:59:34 +0000
@@ -144,20 +144,20 @@
 
 func (s *storageSuite) TestGetReturnsNotFoundErrorIfNotFound(c *gc.C) {
 	const filename = "lost-data"
-	stor := NewStorage(s.environ)
+	stor := NewStorage(s.makeEnviron())
 	_, err := storage.Get(stor, filename)
 	c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
 }
 
 func (s *storageSuite) TestListReturnsEmptyIfNoFilesStored(c *gc.C) {
-	stor := NewStorage(s.environ)
+	stor := NewStorage(s.makeEnviron())
 	listing, err := storage.List(stor, "")
 	c.Assert(err, gc.IsNil)
 	c.Check(listing, gc.DeepEquals, []string{})
 }
 
 func (s *storageSuite) TestListReturnsAllFilesIfPrefixEmpty(c *gc.C) {
-	stor := NewStorage(s.environ)
+	stor := NewStorage(s.makeEnviron())
 	files := []string{"1a", "2b", "3c"}
 	for _, name := range files {
 		s.fakeStoredFile(stor, name)
@@ -169,7 +169,7 @@
 }
 
 func (s *storageSuite) TestListSortsResults(c *gc.C) {
-	stor := NewStorage(s.environ)
+	stor := NewStorage(s.makeEnviron())
 	files := []string{"4d", "1a", "3c", "2b"}
 	for _, name := range files {
 		s.fakeStoredFile(stor, name)
@@ -181,7 +181,7 @@
 }
 
 func (s *storageSuite) TestListReturnsNoFilesIfNoFilesMatchPrefix(c *gc.C) {
-	stor := NewStorage(s.environ)
+	stor := NewStorage(s.makeEnviron())
 	s.fakeStoredFile(stor, "foo")
 
 	listing, err := storage.List(stor, "bar")
@@ -190,7 +190,7 @@
 }
 
 func (s *storageSuite) TestListReturnsOnlyFilesWithMatchingPrefix(c *gc.C) {
-	stor := NewStorage(s.environ)
+	stor := NewStorage(s.makeEnviron())
 	s.fakeStoredFile(stor, "abc")
 	s.fakeStoredFile(stor, "xyz")
 
@@ -200,7 +200,7 @@
 }
 
 func (s *storageSuite) TestListMatchesPrefixOnly(c *gc.C) {
-	stor := NewStorage(s.environ)
+	stor := NewStorage(s.makeEnviron())
 	s.fakeStoredFile(stor, "abc")
 	s.fakeStoredFile(stor, "xabc")
 
@@ -210,7 +210,7 @@
 }
 
 func (s *storageSuite) TestListOperatesOnFlatNamespace(c *gc.C) {
-	stor := NewStorage(s.environ)
+	stor := NewStorage(s.makeEnviron())
 	s.fakeStoredFile(stor, "a/b/c/d")
 
 	listing, err := storage.List(stor, "a/b")
@@ -233,7 +233,7 @@
 
 func (s *storageSuite) TestURLReturnsURLCorrespondingToFile(c *gc.C) {
 	const filename = "my-file.txt"
-	stor := NewStorage(s.environ).(*maasStorage)
+	stor := NewStorage(s.makeEnviron()).(*maasStorage)
 	file := s.fakeStoredFile(stor, filename)
 	// The file contains an anon_resource_uri, which lacks a network part
 	// (but will probably contain a query part).  anonURL will be the
@@ -256,7 +256,7 @@
 	const filename = "broken-toaster.jpg"
 	contents := []byte("Contents here")
 	length := int64(len(contents))
-	stor := NewStorage(s.environ)
+	stor := NewStorage(s.makeEnviron())
 
 	err := stor.Put(filename, bytes.NewReader(contents), length)
 
@@ -271,7 +271,7 @@
 
 func (s *storageSuite) TestPutOverwritesFile(c *gc.C) {
 	const filename = "foo.bar"
-	stor := NewStorage(s.environ)
+	stor := NewStorage(s.makeEnviron())
 	s.fakeStoredFile(stor, filename)
 	newContents := []byte("Overwritten")
 
@@ -292,7 +292,7 @@
 	const filename = "xyzzyz.2.xls"
 	const length = 5
 	contents := []byte("abcdefghijklmnopqrstuvwxyz")
-	stor := NewStorage(s.environ)
+	stor := NewStorage(s.makeEnviron())
 
 	err := stor.Put(filename, bytes.NewReader(contents), length)
 	c.Assert(err, gc.IsNil)
@@ -310,7 +310,7 @@
 	const filename = "a-file-which-is-mine"
 	oldContents := []byte("abcdefghijklmnopqrstuvwxyz")
 	newContents := []byte("xyz")
-	stor := NewStorage(s.environ)
+	stor := NewStorage(s.makeEnviron())
 	err := stor.Put(filename, bytes.NewReader(oldContents), int64(len(oldContents)))
 	c.Assert(err, gc.IsNil)
 
@@ -329,7 +329,7 @@
 
 func (s *storageSuite) TestRemoveDeletesFile(c *gc.C) {
 	const filename = "doomed.txt"
-	stor := NewStorage(s.environ)
+	stor := NewStorage(s.makeEnviron())
 	s.fakeStoredFile(stor, filename)
 
 	err := stor.Remove(filename)
@@ -345,7 +345,7 @@
 
 func (s *storageSuite) TestRemoveIsIdempotent(c *gc.C) {
 	const filename = "half-a-file"
-	stor := NewStorage(s.environ)
+	stor := NewStorage(s.makeEnviron())
 	s.fakeStoredFile(stor, filename)
 
 	err := stor.Remove(filename)
@@ -358,7 +358,7 @@
 func (s *storageSuite) TestNamesMayHaveSlashes(c *gc.C) {
 	const filename = "name/with/slashes"
 	content := []byte("File contents")
-	stor := NewStorage(s.environ)
+	stor := NewStorage(s.makeEnviron())
 
 	err := stor.Put(filename, bytes.NewReader(content), int64(len(content)))
 	c.Assert(err, gc.IsNil)