Merge lp:~rogpeppe/juju-core/105-cloudinit-initial-password into lp:~juju/juju-core/trunk

[Roger Peppe]

environs/cloudinit: use --initial-password

https://codereview.appspot.com/6612054/

[Roger Peppe (rogpeppe)]

Reviewers: mp+128222_code.launchpad.net,

Message:
Please take a look.

Description:
environs/cloudinit: use --initial-password

https://code.launchpad.net/~rogpeppe/juju-core/105-cloudinit-initial-password/+merge/128222

Requires:
https://code.launchpad.net/~rogpeppe/juju-core/107-jujud-bootstrap-initial-password/+merge/128210

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M environs/cloudinit/cloudinit.go
   M environs/cloudinit/cloudinit_test.go
   M environs/ec2/ec2.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: <email address hidden>
+New revision: <email address hidden>

Index: environs/cloudinit/cloudinit.go
=== modified file 'environs/cloudinit/cloudinit.go'
--- environs/cloudinit/cloudinit.go 2012-10-05 10:17:49 +0000
+++ environs/cloudinit/cloudinit.go 2012-10-05 10:48:47 +0000
@@ -38,6 +38,7 @@
   // StateInfo holds the means for the new instance to communicate with the
   // juju state. Unless the new machine is running a state server
(StateServer is
   // set), there must be at least one state server address supplied.
+ // The entity name will be ignored.
   StateInfo *state.Info

   // Tools is juju tools to be used on the new machine.
@@ -127,6 +128,7 @@
     " --instance-id "+cfg.InstanceIdAccessor+
     " --env-config "+shquote(base64yaml(cfg.Config))+
     " --state-servers localhost"+mgoPortSuffix+
+ " --initial-password "+shquote(cfg.StateInfo.Password)+
     debugFlag,
    )

@@ -158,11 +160,13 @@
     " --state-servers '%s'"+
     " --log-file /var/log/juju/%s-agent.log"+
     " --data-dir '%s'"+
+ " --initial-password '%s'"+
     " %s",
    toolsDir, kind,
    cfg.zookeeperHostAddrs(),
    name,
    cfg.DataDir,
+ cfg.StateInfo.Password,
    args,
   )
   conf := &upstart.Conf{
@@ -252,6 +256,9 @@
   if cfg.Tools.URL == "" {
    return requiresError("tools URL")
   }
+ if cfg.StateInfo == nil {
+ return requiresError("state info")
+ }
   if cfg.StateServer {
    if cfg.InstanceIdAccessor == "" {
     return requiresError("instance id accessor")
@@ -260,8 +267,13 @@
     return requiresError("environment configuration")
    }
   } else {
- if cfg.StateInfo == nil || len(cfg.StateInfo.Addrs) == 0 {
- return requiresError("zookeeper hosts")
+ if len(cfg.StateInfo.Addrs) == 0 {
+ return requiresError("state hosts")
+ }
+ }
+ for _, r := range cfg.StateInfo.Password {
+ if r == '\'' || r == '\\' || r < 32 {
+ return fmt.Errorf("invalid machine configuration: password has
disallowed characters")
    }
   }
   return nil

Index: environs/cloudinit/cloudinit_test.go
=== modified file 'environs/cloudinit/cloudinit_test.go'
--- environs/cloudinit/cloudinit_test.go 2012-09-28 13:17:11 +0000
+++ environs/cloudinit/cloudinit_test.go 2012-10-05 09:37:03 +0000
@@ -43,8 +43,11 @@
    AuthorizedKeys: "sshkey1",
    Tools: newSimpleTools("1.2.3-linux-amd64"),
    StateServer:...

[Gustavo Niemeyer (niemeyer)]

One minor:

https://codereview.appspot.com/6612054/diff/2001/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/6612054/diff/2001/environs/cloudinit/cloudinit.go#newcode41
environs/cloudinit/cloudinit.go:41: // The entity name will be ignored.
Rather than ignoring it, we can make pretty good use of it.

Let's validate that when we're running bootstrap-state we've got an
empty EntityName, and when we're not running it, we've got one that
matches the entity name of the machine being started. If those fail, we
can error out and complain that the provider is mis-implemented.

https://codereview.appspot.com/6612054/

[Roger Peppe (rogpeppe)]

https://codereview.appspot.com/6612054/diff/2001/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/6612054/diff/2001/environs/cloudinit/cloudinit.go#newcode41
environs/cloudinit/cloudinit.go:41: // The entity name will be ignored.
On 2012/10/05 15:43:41, niemeyer wrote:
> Rather than ignoring it, we can make pretty good use of it.

> Let's validate that when we're running bootstrap-state we've got an
empty
> EntityName, and when we're not running it, we've got one that matches
the entity
> name of the machine being started. If those fail, we can error out and
complain
> that the provider is mis-implemented.

ok. it's slightly strange that when running bootstrap-state, the
password is actually for the admin *and* the machine entity, but seems
reasonable. do you think it's ok to assume the machine entity name
format is well defined, or should we pass in a Machine here instead of a
machine id?

https://codereview.appspot.com/6612054/

[Gustavo Niemeyer (niemeyer)]

https://codereview.appspot.com/6612054/diff/2001/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/6612054/diff/2001/environs/cloudinit/cloudinit.go#newcode41
environs/cloudinit/cloudinit.go:41: // The entity name will be ignored.
On 2012/10/05 16:40:12, rog wrote:
> On 2012/10/05 15:43:41, niemeyer wrote:
> > Rather than ignoring it, we can make pretty good use of it.
> >
> > Let's validate that when we're running bootstrap-state we've got an
empty
> > EntityName, and when we're not running it, we've got one that
matches the
> entity
> > name of the machine being started. If those fail, we can error out
and
> complain
> > that the provider is mis-implemented.

> ok. it's slightly strange that when running bootstrap-state, the
password is
> actually for the admin *and* the machine entity, but seems reasonable.
do you
> think it's ok to assume the machine entity name format is well
defined, or
> should we pass in a Machine here instead of a machine id?

if cfg.EntityName != "machine-" + strconv.Itoa(cfg.MachineId) {
     return fmt.Errorf("invalid machine configuration: need
authentication for machine %d", cfg.MachineId)
}

Let's not spend too much time on something that trivial.

https://codereview.appspot.com/6612054/

[Roger Peppe (rogpeppe)]

Please take a look.

https://codereview.appspot.com/6612054/

[Roger Peppe (rogpeppe)]

Please take a look.

https://codereview.appspot.com/6612054/

[Roger Peppe (rogpeppe)]

https://codereview.appspot.com/6612054/diff/2001/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/6612054/diff/2001/environs/cloudinit/cloudinit.go#newcode41
environs/cloudinit/cloudinit.go:41: // The entity name will be ignored.
On 2012/10/05 17:13:04, niemeyer wrote:
> On 2012/10/05 16:40:12, rog wrote:
> > On 2012/10/05 15:43:41, niemeyer wrote:
> > > Rather than ignoring it, we can make pretty good use of it.
> > >
> > > Let's validate that when we're running bootstrap-state we've got
an empty
> > > EntityName, and when we're not running it, we've got one that
matches the
> > entity
> > > name of the machine being started. If those fail, we can error out
and
> > complain
> > > that the provider is mis-implemented.
> >
> > ok. it's slightly strange that when running bootstrap-state, the
password is
> > actually for the admin *and* the machine entity, but seems
reasonable. do you
> > think it's ok to assume the machine entity name format is well
defined, or
> > should we pass in a Machine here instead of a machine id?

> if cfg.EntityName != "machine-" + strconv.Itoa(cfg.MachineId) {
> return fmt.Errorf("invalid machine configuration: need
authentication for
> machine %d", cfg.MachineId)
> }

> Let's not spend too much time on something that trivial.

Done.

https://codereview.appspot.com/6612054/

[Gustavo Niemeyer (niemeyer)]

LGTM

https://codereview.appspot.com/6612054/diff/7002/environs/jujutest/test.go
File environs/jujutest/test.go (right):

https://codereview.appspot.com/6612054/diff/7002/environs/jujutest/test.go#newcode18
environs/jujutest/test.go:18: EntityName: fmt.Sprintf("machine-%d",
machineId),
Password: "unimportant",

https://codereview.appspot.com/6612054/

[Roger Peppe (rogpeppe)]

Please take a look.

https://codereview.appspot.com/6612054/

[Roger Peppe (rogpeppe)]

Please take a look.

https://codereview.appspot.com/6612054/diff/7002/environs/jujutest/test.go
File environs/jujutest/test.go (right):

https://codereview.appspot.com/6612054/diff/7002/environs/jujutest/test.go#newcode18
environs/jujutest/test.go:18: EntityName: fmt.Sprintf("machine-%d",
machineId),
On 2012/10/05 17:39:18, niemeyer wrote:
> Password: "unimportant",

Done. But I've moved this function into juju/testing, because it's
useful elsewhere now.

https://codereview.appspot.com/6612054/

[Gustavo Niemeyer (niemeyer)]

LGTM

https://codereview.appspot.com/6612054/diff/20005/juju/testing/conn.go
File juju/testing/conn.go (right):

https://codereview.appspot.com/6612054/diff/20005/juju/testing/conn.go#newcode42
juju/testing/conn.go:42: // machine id of the machine to be started
Please reindent.

https://codereview.appspot.com/6612054/

[lp:~rogpeppe/juju-core/105-cloudinit-initial-password]

634. By Roger Peppe on 2012-10-08

merge trunk

635. By Roger Peppe on 2012-10-08

environs/jujutest: fix InvalidStateInfo reference

[Roger Peppe (rogpeppe)]

Please take a look.

https://codereview.appspot.com/6612054/

[Roger Peppe (rogpeppe)]

*** Submitted:

environs/cloudinit: use --initial-password

R=TheMue, niemeyer
CC=
https://codereview.appspot.com/6612054

https://codereview.appspot.com/6612054/

=== modified file 'cmd/juju/ssh_test.go'
--- cmd/juju/ssh_test.go	2012-10-01 01:02:35 +0000
+++ cmd/juju/ssh_test.go	2012-10-08 16:19:21 +0000
@@ -121,7 +121,7 @@
 		c.Assert(err, IsNil)
 		// must set an instance id as the ssh command uses that as a signal the machine
 		// has been provisioned
-		inst, err := s.Conn.Environ.StartInstance(m.Id(), nil, nil)
+		inst, err := s.Conn.Environ.StartInstance(m.Id(), testing.InvalidStateInfo(m.Id()), nil)
 		c.Assert(err, IsNil)
 		c.Assert(m.SetInstanceId(inst.Id()), IsNil)
 		machines[i] = m

=== modified file 'cmd/juju/status_test.go'
--- cmd/juju/status_test.go	2012-09-28 07:21:43 +0000
+++ cmd/juju/status_test.go	2012-10-08 16:19:21 +0000
@@ -59,7 +59,7 @@
 		func(st *state.State, conn *juju.Conn, c *C) {
 			m, err := st.Machine(0)
 			c.Assert(err, IsNil)
-			inst, err := conn.Environ.StartInstance(m.Id(), nil, nil)
+			inst, err := conn.Environ.StartInstance(m.Id(), testing.InvalidStateInfo(m.Id()), nil)
 			c.Assert(err, IsNil)
 			err = m.SetInstanceId(inst.Id())
 			c.Assert(err, IsNil)
@@ -146,7 +146,7 @@
 				m, err := st.AddMachine(state.MachinerWorker)
 				c.Assert(err, IsNil)
 				c.Assert(m.Id(), Equals, i)
-				inst, err := conn.Environ.StartInstance(m.Id(), nil, nil)
+				inst, err := conn.Environ.StartInstance(m.Id(), testing.InvalidStateInfo(m.Id()), nil)
 				c.Assert(err, IsNil)
 				err = m.SetInstanceId(inst.Id())
 				c.Assert(err, IsNil)

=== modified file 'environs/cloudinit/cloudinit.go'
--- environs/cloudinit/cloudinit.go	2012-10-05 10:17:49 +0000
+++ environs/cloudinit/cloudinit.go	2012-10-08 16:19:21 +0000
@@ -9,6 +9,7 @@
 	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/log"
 	"launchpad.net/juju-core/state"
+	"launchpad.net/juju-core/trivial"
 	"launchpad.net/juju-core/upstart"
 	"path"
 	"strings"
@@ -38,6 +39,8 @@
 	// StateInfo holds the means for the new instance to communicate with the
 	// juju state. Unless the new machine is running a state server (StateServer is
 	// set), there must be at least one state server address supplied.
+	// The entity name must match that of the machine being started,
+	// or be empty when starting a state server.
 	StateInfo *state.Info
 
 	// Tools is juju tools to be used on the new machine.
@@ -62,12 +65,6 @@
 	Config *config.Config
 }
 
-type requiresError string
-
-func (e requiresError) Error() string {
-	return "invalid machine configuration: missing " + string(e)
-}
-
 func addScripts(c *cloudinit.Config, scripts ...string) {
 	for _, s := range scripts {
 		c.AddRunCmd(s)
@@ -127,6 +124,7 @@
 			" --instance-id "+cfg.InstanceIdAccessor+
 			" --env-config "+shquote(base64yaml(cfg.Config))+
 			" --state-servers localhost"+mgoPortSuffix+
+			" --initial-password "+shquote(cfg.StateInfo.Password)+
 			debugFlag,
 		)
 
@@ -158,11 +156,13 @@
 			" --state-servers '%s'"+
 			" --log-file /var/log/juju/%s-agent.log"+
 			" --data-dir '%s'"+
+			" --initial-password '%s'"+
 			" %s",
 		toolsDir, kind,
-		cfg.zookeeperHostAddrs(),
+		cfg.stateHostAddrs(),
 		name,
 		cfg.DataDir,
+		cfg.StateInfo.Password,
 		args,
 	)
 	conf := &upstart.Conf{
@@ -218,7 +218,7 @@
 	return environs.ToolsDir(cfg.DataDir, cfg.Tools.Binary)
 }
 
-func (cfg *MachineConfig) zookeeperHostAddrs() string {
+func (cfg *MachineConfig) stateHostAddrs() string {
 	var hosts []string
 	if cfg.StateServer {
 		hosts = append(hosts, "localhost"+mgoPortSuffix)
@@ -236,32 +236,53 @@
 	return `'` + strings.Replace(s, `'`, `'"'"'`, -1) + `'`
 }
 
-func verifyConfig(cfg *MachineConfig) error {
+type requiresError string
+
+func (e requiresError) Error() string {
+	return "invalid machine configuration: missing " + string(e)
+}
+
+func verifyConfig(cfg *MachineConfig) (err error) {
+	defer trivial.ErrorContextf(&err, "invalid machine configuration")
 	if cfg.MachineId < 0 {
-		return fmt.Errorf("invalid machine configuration: negative machine id")
+		return fmt.Errorf("negative machine id")
 	}
 	if cfg.ProviderType == "" {
-		return requiresError("provider type")
+		return fmt.Errorf("missing provider type")
 	}
 	if cfg.DataDir == "" {
-		return requiresError("var directory")
+		return fmt.Errorf("missing var directory")
 	}
 	if cfg.Tools == nil {
-		return requiresError("tools")
+		return fmt.Errorf("missing tools")
 	}
 	if cfg.Tools.URL == "" {
-		return requiresError("tools URL")
+		return fmt.Errorf("missing tools URL")
+	}
+	if cfg.StateInfo == nil {
+		return fmt.Errorf("missing state info")
 	}
 	if cfg.StateServer {
 		if cfg.InstanceIdAccessor == "" {
-			return requiresError("instance id accessor")
+			return fmt.Errorf("missing instance id accessor")
 		}
 		if cfg.Config == nil {
-			return requiresError("environment configuration")
+			return fmt.Errorf("missing environment configuration")
+		}
+		if cfg.StateInfo.EntityName != "" {
+			return fmt.Errorf("entity name must be blank when starting a state server")
 		}
 	} else {
-		if cfg.StateInfo == nil || len(cfg.StateInfo.Addrs) == 0 {
-			return requiresError("zookeeper hosts")
+		if len(cfg.StateInfo.Addrs) == 0 {
+			return fmt.Errorf("missing state hosts")
+		}
+		if cfg.StateInfo.EntityName != fmt.Sprintf("machine-%d", cfg.MachineId) {
+			return fmt.Errorf("entity name must match started machine")
+		}
+	}
+	for _, r := range cfg.StateInfo.Password {
+		if r == '\'' || r == '\\' || r < 32 {
+			return fmt.Errorf("password has disallowed characters")
 		}
 	}
 	return nil

=== modified file 'environs/cloudinit/cloudinit_test.go'
--- environs/cloudinit/cloudinit_test.go	2012-09-28 13:17:11 +0000
+++ environs/cloudinit/cloudinit_test.go	2012-10-08 16:19:21 +0000
@@ -43,8 +43,11 @@
 		AuthorizedKeys:     "sshkey1",
 		Tools:              newSimpleTools("1.2.3-linux-amd64"),
 		StateServer:        true,
-		Config:             envConfig,
-		DataDir:            "/var/lib/juju",
+		StateInfo: &state.Info{
+			Password: "arble",
+		},
+		Config:  envConfig,
+		DataDir: "/var/lib/juju",
 	},
 	{
 		MachineId:      99,
@@ -53,7 +56,11 @@
 		DataDir:        "/var/lib/juju",
 		StateServer:    false,
 		Tools:          newSimpleTools("1.2.3-linux-amd64"),
-		StateInfo:      &state.Info{Addrs: []string{"zk1"}},
+		StateInfo: &state.Info{
+			Addrs:      []string{"state-addr.example.com"},
+			EntityName: "machine-99",
+			Password:   "arble",
+		},
 	},
 }
 
@@ -87,9 +94,18 @@
 	t.checkPackage(c, "git")
 
 	if t.cfg.StateServer {
-		t.checkScripts(c, "jujud machine --state-servers 'localhost:37017' .* --machine-id [0-9]+")
+		t.checkScripts(c, "jujud bootstrap-state"+
+			".* --state-servers localhost:37017"+
+			".*--initial-password '"+t.cfg.StateInfo.Password+"'")
+		t.checkScripts(c, "jujud machine"+
+			" --state-servers 'localhost:37017' "+
+			".*--initial-password '"+t.cfg.StateInfo.Password+"'"+
+			".* --machine-id [0-9]+")
 	} else {
-		t.checkScripts(c, "jujud machine --state-servers '"+strings.Join(t.cfg.StateInfo.Addrs, ",")+"' .* --machine-id [0-9]+")
+		t.checkScripts(c, "jujud machine"+
+			" --state-servers '"+strings.Join(t.cfg.StateInfo.Addrs, ",")+"'"+
+			".*--initial-password '"+t.cfg.StateInfo.Password+"'"+
+			" .*--machine-id [0-9]+")
 	}
 }
 
@@ -219,33 +235,59 @@
 	err    string
 	mutate func(*cloudinit.MachineConfig)
 }{
-	{"negative machine id", func(cfg *cloudinit.MachineConfig) { cfg.MachineId = -1 }},
-	{"missing provider type", func(cfg *cloudinit.MachineConfig) { cfg.ProviderType = "" }},
+	{"negative machine id", func(cfg *cloudinit.MachineConfig) {
+		cfg.MachineId = -1
+	}},
+	{"missing provider type", func(cfg *cloudinit.MachineConfig) {
+		cfg.ProviderType = ""
+	}},
 	{"missing instance id accessor", func(cfg *cloudinit.MachineConfig) {
 		cfg.InstanceIdAccessor = ""
 	}},
 	{"missing environment configuration", func(cfg *cloudinit.MachineConfig) {
 		cfg.Config = nil
 	}},
-	{"missing zookeeper hosts", func(cfg *cloudinit.MachineConfig) {
-		cfg.StateServer = false
+	{"missing state info", func(cfg *cloudinit.MachineConfig) {
 		cfg.StateInfo = nil
 	}},
-	{"missing zookeeper hosts", func(cfg *cloudinit.MachineConfig) {
+	{"missing state hosts", func(cfg *cloudinit.MachineConfig) {
 		cfg.StateServer = false
-		cfg.StateInfo = &state.Info{}
+		cfg.StateInfo = &state.Info{EntityName: "machine-99"}
 	}},
 	{"missing var directory", func(cfg *cloudinit.MachineConfig) {
 		cfg.DataDir = ""
-		cfg.StateInfo = &state.Info{}
 	}},
 	{"missing tools", func(cfg *cloudinit.MachineConfig) {
 		cfg.Tools = nil
-		cfg.StateInfo = &state.Info{}
 	}},
 	{"missing tools URL", func(cfg *cloudinit.MachineConfig) {
 		cfg.Tools = &state.Tools{}
-		cfg.StateInfo = &state.Info{}
+	}},
+	{"entity name must match started machine", func(cfg *cloudinit.MachineConfig) {
+		cfg.StateServer = false
+		info := *cfg.StateInfo
+		info.EntityName = "machine-0"
+		cfg.StateInfo = &info
+	}},
+	{"entity name must match started machine", func(cfg *cloudinit.MachineConfig) {
+		cfg.StateServer = false
+		info := *cfg.StateInfo
+		info.EntityName = ""
+		cfg.StateInfo = &info
+	}},
+	{"entity name must be blank when starting a state server", func(cfg *cloudinit.MachineConfig) {
+		info := *cfg.StateInfo
+		info.EntityName = "machine-0"
+		cfg.StateInfo = &info
+	}},
+	{"password has disallowed characters", func(cfg *cloudinit.MachineConfig) {
+		cfg.StateInfo.Password = "'"
+	}},
+	{"password has disallowed characters", func(cfg *cloudinit.MachineConfig) {
+		cfg.StateInfo.Password = "\\"
+	}},
+	{"password has disallowed characters", func(cfg *cloudinit.MachineConfig) {
+		cfg.StateInfo.Password = "\n"
 	}},
 }
 
@@ -259,15 +301,18 @@
 		MachineId:          99,
 		Tools:              newSimpleTools("9.9.9-linux-arble"),
 		AuthorizedKeys:     "sshkey1",
-		StateInfo:          &state.Info{Addrs: []string{"zkhost"}},
-		Config:             envConfig,
-		DataDir:            "/var/lib/juju",
+		StateInfo: &state.Info{
+			Addrs: []string{"host"},
+		},
+		Config:  envConfig,
+		DataDir: "/var/lib/juju",
 	}
 	// check that the base configuration does not give an error
 	_, err := cloudinit.New(cfg)
 	c.Assert(err, IsNil)
 
-	for _, test := range verifyTests {
+	for i, test := range verifyTests {
+		c.Logf("test %d. %s", i, test.err)
 		cfg1 := *cfg
 		test.mutate(&cfg1)
 		t, err := cloudinit.New(&cfg1)

=== modified file 'environs/dummy/environs.go'
--- environs/dummy/environs.go	2012-09-21 11:45:08 +0000
+++ environs/dummy/environs.go	2012-10-08 16:19:21 +0000
@@ -468,6 +468,9 @@
 	}
 	e.state.mu.Lock()
 	defer e.state.mu.Unlock()
+	if info.EntityName != fmt.Sprintf("machine-%d", machineId) {
+		return nil, fmt.Errorf("entity name must match started machine")
+	}
 	if tools != nil && (strings.HasPrefix(tools.Series, "unknown") || strings.HasPrefix(tools.Arch, "unknown")) {
 		return nil, fmt.Errorf("cannot find image for %s-%s", tools.Series, tools.Arch)
 	}

=== modified file 'environs/ec2/ec2.go'
--- environs/ec2/ec2.go	2012-10-04 14:23:00 +0000
+++ environs/ec2/ec2.go	2012-10-08 16:19:21 +0000
@@ -233,7 +233,9 @@
 	if err != nil {
 		return fmt.Errorf("unable to determine inital configuration: %v", err)
 	}
-	inst, err := e.startInstance(0, nil, tools, true, config)
+	// TODO add initial password argument to Bootstrap.
+	info := &state.Info{}
+	inst, err := e.startInstance(0, info, tools, true, config)
 	if err != nil {
 		return fmt.Errorf("cannot start bootstrap instance: %v", err)
 	}

=== modified file 'environs/ec2/live_test.go'
--- environs/ec2/live_test.go	2012-10-05 08:40:36 +0000
+++ environs/ec2/live_test.go	2012-10-08 16:19:21 +0000
@@ -10,7 +10,8 @@
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/environs/ec2"
 	"launchpad.net/juju-core/environs/jujutest"
-	"launchpad.net/juju-core/testing"
+	"launchpad.net/juju-core/juju/testing"
+	coretesting "launchpad.net/juju-core/testing"
 	"strings"
 	"time"
 )
@@ -64,7 +65,7 @@
 // LiveTests contains tests that can be run against the Amazon servers.
 // Each test runs using the same ec2 connection.
 type LiveTests struct {
-	testing.LoggingSuite
+	coretesting.LoggingSuite
 	jujutest.LiveTests
 }
 
@@ -102,7 +103,7 @@
 // TODO(niemeyer): Looks like many of those tests should be moved to jujutest.LiveTests.
 
 func (t *LiveTests) TestInstanceDNSName(c *C) {
-	inst, err := t.Env.StartInstance(30, jujutest.InvalidStateInfo, nil)
+	inst, err := t.Env.StartInstance(30, testing.InvalidStateInfo(30), nil)
 	c.Assert(err, IsNil)
 	defer t.Env.StopInstances([]environs.Instance{inst})
 	dns, err := inst.WaitDNSName()
@@ -153,7 +154,7 @@
 		})
 	c.Assert(err, IsNil)
 
-	inst0, err := t.Env.StartInstance(98, jujutest.InvalidStateInfo, nil)
+	inst0, err := t.Env.StartInstance(98, testing.InvalidStateInfo(98), nil)
 	c.Assert(err, IsNil)
 	defer t.Env.StopInstances([]environs.Instance{inst0})
 
@@ -161,7 +162,7 @@
 	// before starting it, to check that it's reused correctly.
 	oldMachineGroup := createGroup(c, ec2conn, groups[2].Name, "old machine group")
 
-	inst1, err := t.Env.StartInstance(99, jujutest.InvalidStateInfo, nil)
+	inst1, err := t.Env.StartInstance(99, testing.InvalidStateInfo(99), nil)
 	c.Assert(err, IsNil)
 	defer t.Env.StopInstances([]environs.Instance{inst1})
 
@@ -292,12 +293,12 @@
 	// It would be nice if this test was in jujutest, but
 	// there's no way for jujutest to fabricate a valid-looking
 	// instance id.
-	inst0, err := t.Env.StartInstance(40, jujutest.InvalidStateInfo, nil)
+	inst0, err := t.Env.StartInstance(40, testing.InvalidStateInfo(40), nil)
 	c.Assert(err, IsNil)
 
 	inst1 := ec2.FabricateInstance(inst0, "i-aaaaaaaa")
 
-	inst2, err := t.Env.StartInstance(41, jujutest.InvalidStateInfo, nil)
+	inst2, err := t.Env.StartInstance(41, testing.InvalidStateInfo(41), nil)
 	c.Assert(err, IsNil)
 
 	err = t.Env.StopInstances([]environs.Instance{inst0, inst1, inst2})

=== modified file 'environs/ec2/local_test.go'
--- environs/ec2/local_test.go	2012-10-05 08:40:36 +0000
+++ environs/ec2/local_test.go	2012-10-08 16:19:21 +0000
@@ -240,6 +240,7 @@
 	// check that a new instance will be started without
 	// zookeeper, with a machine agent, and without a
 	// provisioning agent.
+	info.EntityName = "machine-1"
 	inst1, err := t.env.StartInstance(1, info, nil)
 	c.Assert(err, IsNil)
 	inst = t.srv.ec2srv.Instance(inst1.Id())

=== modified file 'environs/jujutest/livetests.go'
--- environs/jujutest/livetests.go	2012-10-05 15:27:07 +0000
+++ environs/jujutest/livetests.go	2012-10-08 16:19:21 +0000
@@ -7,8 +7,9 @@
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/juju"
+	"launchpad.net/juju-core/juju/testing"
 	"launchpad.net/juju-core/state"
-	"launchpad.net/juju-core/testing"
+	coretesting "launchpad.net/juju-core/testing"
 	"launchpad.net/juju-core/trivial"
 	"launchpad.net/juju-core/version"
 	"time"
@@ -21,7 +22,7 @@
 	c.Assert(err, IsNil)
 	c.Check(insts, HasLen, 0)
 
-	inst, err := t.Env.StartInstance(0, InvalidStateInfo, nil)
+	inst, err := t.Env.StartInstance(0, testing.InvalidStateInfo(0), nil)
 	c.Assert(err, IsNil)
 	c.Assert(inst, NotNil)
 	id0 := inst.Id()
@@ -66,7 +67,7 @@
 }
 
 func (t *LiveTests) TestPorts(c *C) {
-	inst1, err := t.Env.StartInstance(1, InvalidStateInfo, nil)
+	inst1, err := t.Env.StartInstance(1, testing.InvalidStateInfo(1), nil)
 	c.Assert(err, IsNil)
 	c.Assert(inst1, NotNil)
 	defer t.Env.StopInstances([]environs.Instance{inst1})
@@ -74,7 +75,7 @@
 	c.Assert(err, IsNil)
 	c.Assert(ports, HasLen, 0)
 
-	inst2, err := t.Env.StartInstance(2, InvalidStateInfo, nil)
+	inst2, err := t.Env.StartInstance(2, testing.InvalidStateInfo(2), nil)
 	c.Assert(err, IsNil)
 	c.Assert(inst2, NotNil)
 	ports, err = inst2.Ports(2)
@@ -159,14 +160,14 @@
 	c.Assert(err, IsNil)
 
 	// Create instances and check open ports on both instances.
-	inst1, err := t.Env.StartInstance(1, InvalidStateInfo, nil)
+	inst1, err := t.Env.StartInstance(1, testing.InvalidStateInfo(1), nil)
 	c.Assert(err, IsNil)
 	defer t.Env.StopInstances([]environs.Instance{inst1})
 	ports, err := inst1.Ports(1)
 	c.Assert(err, IsNil)
 	c.Assert(ports, HasLen, 0)
 
-	inst2, err := t.Env.StartInstance(2, InvalidStateInfo, nil)
+	inst2, err := t.Env.StartInstance(2, testing.InvalidStateInfo(2), nil)
 	c.Assert(err, IsNil)
 	ports, err = inst2.Ports(2)
 	c.Assert(err, IsNil)
@@ -260,7 +261,7 @@
 	// Create a new service and deploy a unit of it.
 	c.Logf("deploying service")
 	repoDir := c.MkDir()
-	url := testing.Charms.ClonedURL(repoDir, mtools0.Series, "dummy")
+	url := coretesting.Charms.ClonedURL(repoDir, mtools0.Series, "dummy")
 	sch, err := conn.PutCharm(url, &charm.LocalRepository{repoDir}, false)
 
 	c.Assert(err, IsNil)
@@ -572,7 +573,7 @@
 		URL:    url,
 	}
 
-	inst, err := t.Env.StartInstance(4, InvalidStateInfo, tools)
+	inst, err := t.Env.StartInstance(4, testing.InvalidStateInfo(4), tools)
 	if inst != nil {
 		err := t.Env.StopInstances([]environs.Instance{inst})
 		c.Check(err, IsNil)

=== modified file 'environs/jujutest/test.go'
--- environs/jujutest/test.go	2012-10-02 02:02:42 +0000
+++ environs/jujutest/test.go	2012-10-08 16:19:21 +0000
@@ -3,17 +3,10 @@
 import (
 	. "launchpad.net/gocheck"
 	"launchpad.net/juju-core/environs"
-	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/testing"
 	"time"
 )
 
-// InvalidStateInfo holds information about no state - it will always give
-// an error when connected to.
-var InvalidStateInfo = &state.Info{
-	Addrs: []string{"0.1.2.3:1234"},
-}
-
 // Tests is a gocheck suite containing tests verifying juju functionality
 // against the environment with Name that must exist within Environs. The
 // tests are not designed to be run against a live server - the Environ

=== modified file 'environs/jujutest/tests.go'
--- environs/jujutest/tests.go	2012-09-26 10:27:02 +0000
+++ environs/jujutest/tests.go	2012-10-08 16:19:21 +0000
@@ -6,6 +6,7 @@
 	"io/ioutil"
 	. "launchpad.net/gocheck"
 	"launchpad.net/juju-core/environs"
+	"launchpad.net/juju-core/juju/testing"
 	"net/http"
 	"time"
 )
@@ -17,12 +18,12 @@
 	c.Assert(err, IsNil)
 	c.Assert(insts, HasLen, 0)
 
-	inst0, err := e.StartInstance(0, InvalidStateInfo, nil)
+	inst0, err := e.StartInstance(0, testing.InvalidStateInfo(0), nil)
 	c.Assert(err, IsNil)
 	c.Assert(inst0, NotNil)
 	id0 := inst0.Id()
 
-	inst1, err := e.StartInstance(1, InvalidStateInfo, nil)
+	inst1, err := e.StartInstance(1, testing.InvalidStateInfo(1), nil)
 	c.Assert(err, IsNil)
 	c.Assert(inst1, NotNil)
 	id1 := inst1.Id()

=== modified file 'juju/testing/conn.go'
--- juju/testing/conn.go	2012-09-21 16:03:23 +0000
+++ juju/testing/conn.go	2012-10-08 16:19:21 +0000
@@ -37,6 +37,17 @@
 	oldHome string
 }
 
+// InvalidStateInfo holds information about no state - it will always give
+// an error when connected to. The machine id gives the
+// machine id of the machine to be started 
+func InvalidStateInfo(machineId int) *state.Info {
+	return &state.Info{
+		Addrs:      []string{"0.1.2.3:1234"},
+		EntityName: fmt.Sprintf("machine-%d", machineId),
+		Password:   "unimportant",
+	}
+}
+
 var config = []byte(`
 environments:
     dummyenv:

=== modified file 'worker/firewaller/firewaller_test.go'
--- worker/firewaller/firewaller_test.go	2012-09-28 17:40:29 +0000
+++ worker/firewaller/firewaller_test.go	2012-10-08 16:19:21 +0000
@@ -72,9 +72,16 @@
 	return u, m
 }
 
+func (s *FirewallerSuite) StateInfo(c *C, m *state.Machine) *state.Info {
+	info := s.JujuConnSuite.StateInfo(c)
+	info.EntityName = m.EntityName()
+	info.Password = "irrelevant"
+	return info
+}
+
 // startInstance starts a new instance for the given machine.
 func (s *FirewallerSuite) startInstance(c *C, m *state.Machine) environs.Instance {
-	inst, err := s.Conn.Environ.StartInstance(m.Id(), s.StateInfo(c), nil)
+	inst, err := s.Conn.Environ.StartInstance(m.Id(), s.StateInfo(c, m), nil)
 	c.Assert(err, IsNil)
 	err = m.SetInstanceId(inst.Id())
 	c.Assert(err, IsNil)
@@ -251,7 +258,7 @@
 func (s *FirewallerSuite) TestFirewallerStartWithPartialState(c *C) {
 	m, err := s.State.AddMachine(state.MachinerWorker)
 	c.Assert(err, IsNil)
-	inst, err := s.Conn.Environ.StartInstance(m.Id(), s.StateInfo(c), nil)
+	inst, err := s.Conn.Environ.StartInstance(m.Id(), s.StateInfo(c, m), nil)
 	c.Assert(err, IsNil)
 	err = m.SetInstanceId(inst.Id())
 	c.Assert(err, IsNil)

=== modified file 'worker/provisioner/provisioner.go'
--- worker/provisioner/provisioner.go	2012-09-27 23:26:16 +0000
+++ worker/provisioner/provisioner.go	2012-10-08 16:19:21 +0000
@@ -223,7 +223,9 @@
 	// however as the PA only knows one state.Info, and that info is used by MAs and 
 	// UAs to locate the ZK for this environment, it is logical to use the same 
 	// state.Info as the PA. 
-	inst, err := p.environ.StartInstance(m.Id(), p.info, nil)
+	info := *p.info
+	info.EntityName = m.EntityName()
+	inst, err := p.environ.StartInstance(m.Id(), &info, nil)
 	if err != nil {
 		log.Printf("provisioner cannot start machine %s: %v", m, err)
 		return err

=== modified file 'worker/provisioner/provisioner_test.go'
--- worker/provisioner/provisioner_test.go	2012-09-28 07:21:43 +0000
+++ worker/provisioner/provisioner_test.go	2012-10-08 16:19:21 +0000
@@ -73,7 +73,9 @@
 		case o := <-s.op:
 			switch o := o.(type) {
 			case dummy.OpStartInstance:
-				c.Assert(o.Info, DeepEquals, s.StateInfo(c))
+				info := s.StateInfo(c)
+				info.EntityName = m.EntityName()
+				c.Assert(o.Info, DeepEquals, info)
 				c.Assert(o.MachineId, Equals, m.Id())
 				c.Assert(o.Instance, NotNil)
 				s.checkInstanceId(c, m, o.Instance)