Merge lp:~axwalk/juju-core/local-provider-testability into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

provider/local: always use filestorage on CLI

The local provider is modified such that all
storage interactions from the CLI go directly
to disk, and the provider code no longer starts
an httpstorage listener. To achieve this, we
no longer store bootstrap-ip in the .jenv, but
instead add it to the environment config during
bootstrap. Thus, we can use the presence of the
attribute to decide whether or not to use the
file or http storage. BootstrapStorager no longer
needs to be implemented, so EnableBootsrapStorage
has been eliminated from the implementation.

For old environments, bootstrap-ip will be set
in the .jenv and thus httpstorage is used;
this is fine, as long as the machine agent is
running. There's an edge case that will no longer
work, where the environment is bootstrapped but
machine-0 agent is not running. In this case,
storage is inaccessible.

There are some other drive-by fixes to tests
that improve isolation, and some redundant tests
relating to the old sudo behaviour are removed.
All tests now pass with or without a local provider
running on the machine.

Fixes lp:1271672

https://codereview.appspot.com/49640050/

provider/local: always use filestorage on CLI

The local provider is modified such that all
storage interactions from the CLI go directly
to disk, and the provider code no longer starts
an httpstorage listener. To achieve this, we
no longer store bootstrap-ip in the .jenv, but
instead add it to the environment config during
bootstrap. Thus, we can use the presence of the
attribute to decide whether or not to use the
file or http storage. BootstrapStorager no longer
needs to be implemented, so EnableBootsrapStorage
has been eliminated from the implementation.

For old environments, bootstrap-ip will be set
in the .jenv and thus httpstorage is used;
this is fine, as long as the machine agent is
running. There's an edge case that will no longer
work, where the environment is bootstrapped but
machine-0 agent is not running. In this case,
storage is inaccessible.

There are some other drive-by fixes to tests
that improve isolation, and some redundant tests
relating to the old sudo behaviour are removed.
All tests now pass with or without a local provider
running on the machine.

Fixes lp:1271672

https://codereview.appspot.com/49640050/

[Andrew Wilkins (axwalk)]

Reviewers: mp+204169_code.launchpad.net,

Message:
Please take a look.

Description:
provider/local: always use filestorage on CLI

The local provider is modified such that all
storage interactions from the CLI go directly
to disk, and the provider code no longer starts
an httpstorage listener. To achieve this, we
no longer store bootstrap-ip in the .jenv, but
instead add it to the environment config during
bootstrap. Thus, we can use the presence of the
attribute to decide whether or not to use the
file or http storage. BootstrapStorager no longer
needs to be implemented, so EnableBootsrapStorage
has been eliminated from the implementation.

For old environments, bootstrap-ip will be set
in the .jenv and thus httpstorage is used;
this is fine, as long as the machine agent is
running. There's an edge case that will no longer
work, where the environment is bootstrapped but
machine-0 agent is not running. In this case,
storage is inaccessible.

There are some other drive-by fixes to tests
that improve isolation, and some redundant tests
relating to the old sudo behaviour are removed.
All tests now pass with or without a local provider
running on the machine.

Fixes lp:1271672

https://code.launchpad.net/~axwalk/juju-core/local-provider-testability/+merge/204169

(do not edit description out of merge proposal)

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

Affected files (+95, -169 lines):
   A [revision details]
   M provider/local/config.go
   M provider/local/config_test.go
   M provider/local/environ.go
   M provider/local/environ_test.go
   M provider/local/environprovider.go
   M provider/local/environprovider_test.go
   M provider/local/export_test.go
   M provider/local/instance.go
   M provider/local/local_test.go

[Roger Peppe (rogpeppe)]

LGTM although I'd like a second opinion from someone that's more
familiar with local provider issues.

https://codereview.appspot.com/49640050/diff/1/provider/local/environ.go
File provider/local/environ.go (right):

https://codereview.appspot.com/49640050/diff/1/provider/local/environ.go#newcode235
provider/local/environ.go:235: // continue on to set up things prior to
bootstrap.
This comment doesn't read easily to me.

How about something like this:

// When the localEnviron value is created on the client
// side, the bootstrap-ip attribute will not exist,
// because it is only set *within* the running
// environment, not in the configuration created
// by Prepare.
// Thus when bootstrapIPAddress returns a non-empty
// string, we know we are running server-side and
// thus must use the network API to access the storage.

?

https://codereview.appspot.com/49640050/diff/1/provider/local/environprovider_test.go
File provider/local/environprovider_test.go (left):

https://codereview.appspot.com/49640050/diff/1/provider/local/environprovider_test.go#oldcode59
provider/local/environprovider_test.go:59: c.Skip("fails if local
provider running already")
I presume you've checked that this doesn't still
fail if a local environment is currently running.

https://codereview.appspot.com/49640050/diff/1/provider/local/export_test.go
File provider/local/export_test.go (right):

https://codereview.appspot.com/49640050/diff/1/provider/local/export_test.go#newcode54
provider/local/export_test.go:54: func MockAddressForInterface() func()
{
This might be easier just as:

PatchValue(local.GetAddressForInterface, func(name string) (string,
error) etc)

but perhaps that's a change for a future CL.

https://codereview.appspot.com/49640050/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/49640050/diff/1/provider/local/environ.go
File provider/local/environ.go (right):

https://codereview.appspot.com/49640050/diff/1/provider/local/environ.go#newcode235
provider/local/environ.go:235: // continue on to set up things prior to
bootstrap.
On 2014/02/03 09:37:40, rog wrote:
> This comment doesn't read easily to me.

> How about something like this:

> // When the localEnviron value is created on the client
> // side, the bootstrap-ip attribute will not exist,
> // because it is only set *within* the running
> // environment, not in the configuration created
> // by Prepare.
> // Thus when bootstrapIPAddress returns a non-empty
> // string, we know we are running server-side and
> // thus must use the network API to access the storage.

> ?

Done.

https://codereview.appspot.com/49640050/diff/1/provider/local/environprovider_test.go
File provider/local/environprovider_test.go (left):

https://codereview.appspot.com/49640050/diff/1/provider/local/environprovider_test.go#oldcode59
provider/local/environprovider_test.go:59: c.Skip("fails if local
provider running already")
On 2014/02/03 09:37:40, rog wrote:
> I presume you've checked that this doesn't still
> fail if a local environment is currently running.

Correct.

https://codereview.appspot.com/49640050/diff/1/provider/local/export_test.go
File provider/local/export_test.go (right):

https://codereview.appspot.com/49640050/diff/1/provider/local/export_test.go#newcode54
provider/local/export_test.go:54: func MockAddressForInterface() func()
{
On 2014/02/03 09:37:40, rog wrote:
> This might be easier just as:

> PatchValue(local.GetAddressForInterface, func(name string) (string,
error) etc)

> but perhaps that's a change for a future CL.

I've changed it to use PatchValue here. Another CL in the future can
move that up the stack if it's deemed desirable.

https://codereview.appspot.com/49640050/

[Tim Penhey (thumper)]

LGTM

https://codereview.appspot.com/49640050/diff/20001/provider/local/environprovider_test.go
File provider/local/environprovider_test.go (left):

https://codereview.appspot.com/49640050/diff/20001/provider/local/environprovider_test.go#oldcode59
provider/local/environprovider_test.go:59: c.Skip("fails if local
provider running already")
\o/

https://codereview.appspot.com/49640050/

[Go Bot (go-bot)]

The attempt to merge lp:~axwalk/juju-core/local-provider-testability into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core 0.016s
ok launchpad.net/juju-core/agent 1.186s
ok launchpad.net/juju-core/agent/tools 0.230s
ok launchpad.net/juju-core/bzr 6.706s
ok launchpad.net/juju-core/cert 3.436s
ok launchpad.net/juju-core/charm 0.610s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.037s
ok launchpad.net/juju-core/cloudinit/sshinit 1.205s
ok launchpad.net/juju-core/cmd 0.247s
ok launchpad.net/juju-core/cmd/charm-admin 0.845s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 228.578s
ok launchpad.net/juju-core/cmd/jujud 60.449s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 12.696s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/constraints 0.038s
ok launchpad.net/juju-core/container 0.053s
ok launchpad.net/juju-core/container/factory 0.055s
ok launchpad.net/juju-core/container/kvm 0.272s
ok launchpad.net/juju-core/container/kvm/mock 0.048s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 0.250s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.268s
ok launchpad.net/juju-core/environs 3.155s
ok launchpad.net/juju-core/environs/bootstrap 4.535s
ok launchpad.net/juju-core/environs/cloudinit 0.615s
ok launchpad.net/juju-core/environs/config 3.416s
ok launchpad.net/juju-core/environs/configstore 0.039s
ok launchpad.net/juju-core/environs/filestorage 0.031s
ok launchpad.net/juju-core/environs/httpstorage 0.968s
ok launchpad.net/juju-core/environs/imagemetadata 0.655s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.059s
ok launchpad.net/juju-core/environs/jujutest 0.245s
ok launchpad.net/juju-core/environs/manual 9.377s
ok launchpad.net/juju-core/environs/simplestreams 0.359s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 1.249s
ok launchpad.net/juju-core/environs/storage 1.233s
ok launchpad.net/juju-core/environs/sync 33.641s
ok launchpad.net/juju-core/environs/testing 0.201s
ok launchpad.net/juju-core/environs/tools 6.914s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.015s
ok launchpad.net/juju-core/instance 0.023s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 22.728s
ok launchpad.net/juju-core/juju/osenv 0.019s
? launchpad.net/juju-core/juju/testing [no test files]
ok launchpad.net/juju-core/log 0.014s
ok launchpad.net/juju-core/log/syslog 0.022s
ok launchpad.net/juju-co...

[Andrew Wilkins (axwalk)]

I think I've identified the problem in replicaset_test.go, will propose a fix later.

=== modified file 'provider/local/config.go'
--- provider/local/config.go	2014-02-04 19:12:08 +0000
+++ provider/local/config.go	2014-02-12 02:56:43 +0000
@@ -83,18 +83,12 @@
 	return filepath.Join(c.rootDir(), "log")
 }
 
-// A config is bootstrapped if the bootstrap-ip address has been set.
-func (c *environConfig) bootstrapped() bool {
-	_, found := c.attrs["bootstrap-ip"]
-	return found
-}
-
+// bootstrapIPAddress returns the IP address of the bootstrap machine.
+// As of 1.18 this is only set inside the environment, and not in the
+// .jenv file.
 func (c *environConfig) bootstrapIPAddress() string {
-	addr, found := c.attrs["bootstrap-ip"]
-	if found {
-		return addr.(string)
-	}
-	return ""
+	addr, _ := c.attrs["bootstrap-ip"].(string)
+	return addr
 }
 
 func (c *environConfig) storagePort() int {

=== modified file 'provider/local/config_test.go'
--- provider/local/config_test.go	2014-01-23 05:21:21 +0000
+++ provider/local/config_test.go	2014-02-12 02:56:43 +0000
@@ -4,14 +4,13 @@
 package local_test
 
 import (
-	"os"
 	"path/filepath"
-	"runtime"
-	"syscall"
 
 	gc "launchpad.net/gocheck"
 
+	"launchpad.net/juju-core/constraints"
 	"launchpad.net/juju-core/environs/config"
+	envtesting "launchpad.net/juju-core/environs/testing"
 	"launchpad.net/juju-core/juju/osenv"
 	"launchpad.net/juju-core/provider"
 	"launchpad.net/juju-core/provider/local"
@@ -24,10 +23,6 @@
 
 var _ = gc.Suite(&configSuite{})
 
-func (s *configSuite) SetUpTest(c *gc.C) {
-	s.baseProviderSuite.SetUpTest(c)
-}
-
 func minimalConfigValues() map[string]interface{} {
 	return testing.FakeConfig().Merge(testing.Attrs{
 		"name": "test",
@@ -114,64 +109,8 @@
 func (s *configSuite) TestBootstrapAsRoot(c *gc.C) {
 	restore := local.SetRootCheckFunction(func() bool { return true })
 	defer restore()
-	_, err := local.Provider.Prepare(minimalConfig(c))
+	env, err := local.Provider.Prepare(minimalConfig(c))
+	c.Assert(err, gc.IsNil)
+	err = env.Bootstrap(envtesting.NewBootstrapContext(testing.Context(c)), constraints.Value{})
 	c.Assert(err, gc.ErrorMatches, "bootstrapping a local environment must not be done as root")
 }
-
-type configRootSuite struct {
-	baseProviderSuite
-}
-
-var _ = gc.Suite(&configRootSuite{})
-
-func (s *configRootSuite) SetUpTest(c *gc.C) {
-	s.baseProviderSuite.SetUpTest(c)
-	// Skip if not linux
-	if runtime.GOOS != "linux" {
-		c.Skip("not running linux")
-	}
-	// Skip if not running as root.
-	if os.Getuid() != 0 {
-		c.Skip("not running as root")
-	}
-}
-
-func (s *configRootSuite) TestCreateDirsNoUserJustRoot(c *gc.C) {
-	defer os.Setenv("SUDO_UID", os.Getenv("SUDO_UID"))
-	defer os.Setenv("SUDO_GID", os.Getenv("SUDO_GID"))
-
-	os.Setenv("SUDO_UID", "")
-	os.Setenv("SUDO_GID", "")
-
-	testConfig := minimalConfig(c)
-	err := local.CreateDirs(c, testConfig)
-	c.Assert(err, gc.IsNil)
-	// Check that the dirs are owned by root.
-	for _, dir := range local.CheckDirs(c, testConfig) {
-		info, err := os.Stat(dir)
-		c.Assert(err, gc.IsNil)
-		// This call is linux specific, but then so is sudo
-		c.Assert(info.Sys().(*syscall.Stat_t).Uid, gc.Equals, uint32(0))
-		c.Assert(info.Sys().(*syscall.Stat_t).Gid, gc.Equals, uint32(0))
-	}
-}
-
-func (s *configRootSuite) TestCreateDirsAsUser(c *gc.C) {
-	defer os.Setenv("SUDO_UID", os.Getenv("SUDO_UID"))
-	defer os.Setenv("SUDO_GID", os.Getenv("SUDO_GID"))
-
-	os.Setenv("SUDO_UID", "1000")
-	os.Setenv("SUDO_GID", "1000")
-
-	testConfig := minimalConfig(c)
-	err := local.CreateDirs(c, testConfig)
-	c.Assert(err, gc.IsNil)
-	// Check that the dirs are owned by the UID/GID set above..
-	for _, dir := range local.CheckDirs(c, testConfig) {
-		info, err := os.Stat(dir)
-		c.Assert(err, gc.IsNil)
-		// This call is linux specific, but then so is sudo
-		c.Assert(info.Sys().(*syscall.Stat_t).Uid, gc.Equals, uint32(1000))
-		c.Assert(info.Sys().(*syscall.Stat_t).Gid, gc.Equals, uint32(1000))
-	}
-}

=== modified file 'provider/local/environ.go'
--- provider/local/environ.go	2014-02-05 08:01:22 +0000
+++ provider/local/environ.go	2014-02-12 02:56:43 +0000
@@ -51,6 +51,8 @@
 	localMutex       sync.Mutex
 	config           *environConfig
 	name             string
+	bridgeAddress    string
+	localStorage     storage.Storage
 	storageListener  net.Listener
 	containerManager container.Manager
 }
@@ -127,6 +129,18 @@
 		return err
 	}
 
+	// Record the bootstrap IP, so the containers know where to go for storage.
+	cfg, err := env.Config().Apply(map[string]interface{}{
+		"bootstrap-ip": env.bridgeAddress,
+	})
+	if err == nil {
+		err = env.SetConfig(cfg)
+	}
+	if err != nil {
+		logger.Errorf("failed to apply bootstrap-ip to config: %v", err)
+		return err
+	}
+
 	bootstrapJobs, err := agent.MarshalBootstrapJobs(state.JobManageEnviron)
 	if err != nil {
 		return err
@@ -147,7 +161,7 @@
 		agent.StorageAddr:   env.config.storageAddr(),
 		agent.BootstrapJobs: bootstrapJobs,
 	}
-	if err := environs.FinishMachineConfig(mcfg, env.Config(), cons); err != nil {
+	if err := environs.FinishMachineConfig(mcfg, cfg, cons); err != nil {
 		return err
 	}
 	// don't write proxy settings for local machine
@@ -197,22 +211,6 @@
 	return env.config.Config
 }
 
-func createLocalStorageListener(dir, address string) (net.Listener, error) {
-	info, err := os.Stat(dir)
-	if os.IsNotExist(err) {
-		return nil, fmt.Errorf("storage directory %q does not exist, bootstrap first", dir)
-	} else if err != nil {
-		return nil, err
-	} else if !info.Mode().IsDir() {
-		return nil, fmt.Errorf("%q exists but is not a directory (and it needs to be)", dir)
-	}
-	storage, err := filestorage.NewFileStorageWriter(dir, filestorage.UseDefaultTmpDir)
-	if err != nil {
-		return nil, err
-	}
-	return httpstorage.Serve(address, storage)
-}
-
 // SetConfig is specified in the Environ interface.
 func (env *localEnviron) SetConfig(cfg *config.Config) error {
 	ecfg, err := providerInstance.newConfig(cfg)
@@ -235,19 +233,19 @@
 		return err
 	}
 
-	// Here is the end of normal config setting.
-	if ecfg.bootstrapped() {
+	// When the localEnviron value is created on the client
+	// side, the bootstrap-ip attribute will not exist,
+	// because it is only set *within* the running
+	// environment, not in the configuration created by
+	// Prepare.
+	//
+	// When bootstrapIPAddress returns a non-empty string,
+	// we know we are running server-side and thus must use
+	// httpstorage.
+	if addr := ecfg.bootstrapIPAddress(); addr != "" {
+		env.bridgeAddress = addr
 		return nil
 	}
-	if err := ensureNotRoot(); err != nil {
-		return err
-	}
-	return env.bootstrapAddressAndStorage(cfg)
-}
-
-// bootstrapAddressAndStorage finishes up the setup of the environment in
-// situations where there is no machine agent running yet.
-func (env *localEnviron) bootstrapAddressAndStorage(cfg *config.Config) error {
 	// If we get to here, it is because we haven't yet bootstrapped an
 	// environment, and saved the config in it, or we are running a command
 	// from the command line, so it is ok to work on the assumption that we
@@ -255,7 +253,16 @@
 	if err := env.config.createDirs(); err != nil {
 		return err
 	}
+	// Record the network bridge address and create a filestorage.
+	if err := env.resolveBridgeAddress(cfg); err != nil {
+		return err
+	}
+	return env.setLocalStorage()
+}
 
+// resolveBridgeAddress finishes up the setup of the environment in
+// situations where there is no machine agent running yet.
+func (env *localEnviron) resolveBridgeAddress(cfg *config.Config) error {
 	// We need the provider config to get the network bridge.
 	config, err := providerInstance.newConfig(cfg)
 	if err != nil {
@@ -266,48 +273,22 @@
 	bridgeAddress, err := getAddressForInterface(networkBridge)
 	if err != nil {
 		logger.Infof("configure a different bridge using 'network-bridge' in the config file")
-		return fmt.Errorf("cannot find address of network-bridge: %q", networkBridge)
+		return fmt.Errorf("cannot find address of network-bridge: %q: %v", networkBridge, err)
 	}
 	logger.Debugf("found %q as address for %q", bridgeAddress, networkBridge)
-	cfg, err = cfg.Apply(map[string]interface{}{
-		"bootstrap-ip": bridgeAddress,
-	})
-	if err != nil {
-		logger.Errorf("failed to apply new addresses to config: %v", err)
-		return err
-	}
-	// Now recreate the config based on the settings with the bootstrap id.
-	config, err = providerInstance.newConfig(cfg)
-	if err != nil {
-		logger.Errorf("failed to create new environ config: %v", err)
-		return err
-	}
-	env.config = config
-
-	return env.setupLocalStorage()
+	env.bridgeAddress = bridgeAddress
+	return nil
 }
 
-// setupLocalStorage looks to see if there is someone listening on the storage
-// address port.  If there is we assume that it is ours and all is good.  If
-// there is no one listening on that port, create listeners for storage
-// for the duration of the commands execution.
-func (env *localEnviron) setupLocalStorage() error {
-	// Try to listen to the storageAddress.
-	logger.Debugf("checking %s to see if machine agent running storage listener", env.config.storageAddr())
-	connection, err := net.Dial("tcp", env.config.storageAddr())
+// setLocalStorage creates a filestorage so tools can
+// be synced and so forth without having a machine agent
+// running.
+func (env *localEnviron) setLocalStorage() error {
+	storage, err := filestorage.NewFileStorageWriter(env.config.storageDir(), filestorage.UseDefaultTmpDir)
 	if err != nil {
-		logger.Debugf("nope, start some")
-		// These listeners are part of the environment structure so as to remain
-		// referenced for the duration of the open environment.  This is only for
-		// environs that have been created due to a user command.
-		env.storageListener, err = createLocalStorageListener(env.config.storageDir(), env.config.storageAddr())
-		if err != nil {
-			return err
-		}
-	} else {
-		logger.Debugf("yes, don't start local storage listeners")
-		connection.Close()
+		return err
 	}
+	env.localStorage = storage
 	return nil
 }
 
@@ -396,14 +377,13 @@
 
 // Storage is specified in the Environ interface.
 func (env *localEnviron) Storage() storage.Storage {
+	// localStorage is non-nil if we're running from the CLI
+	if env.localStorage != nil {
+		return env.localStorage
+	}
 	return httpstorage.Client(env.config.storageAddr())
 }
 
-// Implements environs.BootstrapStorager.
-func (env *localEnviron) EnableBootstrapStorage() error {
-	return env.setupLocalStorage()
-}
-
 // Destroy is specified in the Environ interface.
 func (env *localEnviron) Destroy() error {
 	// Kill all running instances. This must be done as

=== modified file 'provider/local/environ_test.go'
--- provider/local/environ_test.go	2014-02-05 08:01:22 +0000
+++ provider/local/environ_test.go	2014-02-12 02:56:43 +0000
@@ -39,6 +39,11 @@
 	s.ToolsFixture.SetUpTest(c)
 }
 
+func (s *environSuite) TearDownTest(c *gc.C) {
+	s.ToolsFixture.TearDownTest(c)
+	s.baseProviderSuite.TearDownTest(c)
+}
+
 func (*environSuite) TestOpenFailsWithProtectedDirectories(c *gc.C) {
 	testConfig := minimalConfig(c)
 	testConfig, err := testConfig.Apply(map[string]interface{}{

=== modified file 'provider/local/environprovider.go'
--- provider/local/environprovider.go	2014-02-04 19:12:08 +0000
+++ provider/local/environprovider.go	2014-02-12 02:56:43 +0000
@@ -7,6 +7,7 @@
 	"fmt"
 	"net"
 	"os"
+	"os/user"
 	"syscall"
 
 	"github.com/loggo/loggo"
@@ -32,6 +33,8 @@
 	environs.RegisterProvider(provider.Local, providerInstance)
 }
 
+var userCurrent = user.Current
+
 // Open implements environs.EnvironProvider.Open.
 func (environProvider) Open(cfg *config.Config) (environs.Environ, error) {
 	logger.Infof("opening environment %q", cfg.Name())
@@ -72,8 +75,16 @@
 	return environ, nil
 }
 
+var detectAptProxies = utils.DetectAptProxies
+
 // Prepare implements environs.EnvironProvider.Prepare.
 func (p environProvider) Prepare(cfg *config.Config) (environs.Environ, error) {
+	// The user must not set bootstrap-ip; this is determined by the provider,
+	// and its presence used to determine whether the environment has yet been
+	// bootstrapped.
+	if _, ok := cfg.UnknownAttrs()["bootstrap-ip"]; ok {
+		return nil, fmt.Errorf("bootstrap-ip must not be specified")
+	}
 	err := checkLocalPort(cfg.StatePort(), "state port")
 	if err != nil {
 		return nil, err
@@ -103,7 +114,7 @@
 	if cfg.AptHttpProxy() == "" &&
 		cfg.AptHttpsProxy() == "" &&
 		cfg.AptFtpProxy() == "" {
-		proxy, err := utils.DetectAptProxies()
+		proxy, err := detectAptProxies()
 		if err != nil {
 			return nil, err
 		}
@@ -122,7 +133,7 @@
 }
 
 // checkLocalPort checks that the passed port is not used so far.
-func checkLocalPort(port int, description string) error {
+var checkLocalPort = func(port int, description string) error {
 	logger.Infof("checking %s", description)
 	// Try to connect the port on localhost.
 	address := fmt.Sprintf("localhost:%d", port)

=== modified file 'provider/local/environprovider_test.go'
--- provider/local/environprovider_test.go	2014-01-29 04:00:15 +0000
+++ provider/local/environprovider_test.go	2014-02-12 02:56:43 +0000
@@ -53,10 +53,13 @@
 	s.PatchEnvironment("ftp_proxy", "")
 	s.PatchEnvironment("FTP_PROXY", "")
 	s.HookCommandOutput(&utils.AptCommandOutput, nil, nil)
+	restore := testbase.PatchValue(local.CheckLocalPort, func(port int, desc string) error {
+		return nil
+	})
+	s.AddSuiteCleanup(func(*gc.C) { restore() })
 }
 
 func (s *prepareSuite) TestPrepareCapturesEnvironment(c *gc.C) {
-	c.Skip("fails if local provider running already")
 	baseConfig, err := config.New(config.UseDefaults, map[string]interface{}{
 		"type": provider.Local,
 		"name": "test",

=== modified file 'provider/local/export_test.go'
--- provider/local/export_test.go	2014-02-04 19:12:08 +0000
+++ provider/local/export_test.go	2014-02-12 02:56:43 +0000
@@ -6,12 +6,14 @@
 	gc "launchpad.net/gocheck"
 
 	"launchpad.net/juju-core/environs/config"
-	"launchpad.net/juju-core/utils"
+	"launchpad.net/juju-core/testing/testbase"
 )
 
 var (
-	Provider        = providerInstance
-	FinishBootstrap = &finishBootstrap
+	Provider         = providerInstance
+	FinishBootstrap  = &finishBootstrap
+	CheckLocalPort   = &checkLocalPort
+	DetectAptProxies = &detectAptProxies
 )
 
 // SetRootCheckFunction allows tests to override the check for a root user.
@@ -50,13 +52,8 @@
 // MockAddressForInterface replaces the getAddressForInterface with a function
 // that returns a constant localhost ip address.
 func MockAddressForInterface() func() {
-	getAddressForInterface = func(name string) (string, error) {
+	return testbase.PatchValue(&getAddressForInterface, func(name string) (string, error) {
 		logger.Debugf("getAddressForInterface called for %s", name)
 		return "127.0.0.1", nil
-	}
-	return func() {
-		getAddressForInterface = utils.GetAddressForInterface
-	}
+	})
 }
-
-var CheckLocalPort = checkLocalPort

=== modified file 'provider/local/instance.go'
--- provider/local/instance.go	2014-01-23 05:21:21 +0000
+++ provider/local/instance.go	2014-02-12 02:56:43 +0000
@@ -41,7 +41,7 @@
 		}, {
 			NetworkScope: instance.NetworkCloudLocal,
 			Type:         instance.Ipv4Address,
-			Value:        inst.env.config.bootstrapIPAddress(),
+			Value:        inst.env.bridgeAddress,
 		}}
 		return addrs, nil
 	}
@@ -51,7 +51,7 @@
 // DNSName implements instance.Instance.DNSName.
 func (inst *localInstance) DNSName() (string, error) {
 	if inst.id == bootstrapInstanceId {
-		return inst.env.config.bootstrapIPAddress(), nil
+		return inst.env.bridgeAddress, nil
 	}
 	// Get the IPv4 address from eth0
 	return getAddressForInterface("eth0")

=== modified file 'provider/local/local_test.go'
--- provider/local/local_test.go	2013-10-14 14:47:52 +0000
+++ provider/local/local_test.go	2014-02-12 02:56:43 +0000
@@ -34,15 +34,17 @@
 }
 
 func (*localSuite) TestCheckLocalPort(c *gc.C) {
-	// Block a ports
-	addr := fmt.Sprintf(":%d", 65501)
-	ln, err := net.Listen("tcp", addr)
+	// Listen on a random port.
+	ln, err := net.Listen("tcp", ":0")
 	c.Assert(err, gc.IsNil)
 	defer ln.Close()
-
-	err = local.CheckLocalPort(65501, "test port")
-	c.Assert(err, gc.ErrorMatches, "cannot use 65501 as test port, already in use")
-
-	err = local.CheckLocalPort(65502, "another test port")
+	port := ln.Addr().(*net.TCPAddr).Port
+
+	checkLocalPort := *local.CheckLocalPort
+	err = checkLocalPort(port, "test port")
+	c.Assert(err, gc.ErrorMatches, fmt.Sprintf("cannot use %d as test port, already in use", port))
+
+	ln.Close()
+	err = checkLocalPort(port, "test port, no longer in use")
 	c.Assert(err, gc.IsNil)
 }