Merge lp:~gz/juju-core/tests_os_ssh_isolation_1144704 into lp:~juju/juju-core/trunk

[Martin Packman]

Make openstack tests work with no ssh public key

The juju tests are not run in a well isolated manner, particularly they will
examime the user's home directory and environment variables. This branch fixes
the issue of openstack client setup complaining of no ssh public key is found in
~/.ssh even in tests that are just using the local fake server implementations.

Following the example of how this was fixed for the EC2 tests, this is done in
an ad hoc manner by adding the 'authorized-keys' config with a fake value to
tests that hit this condition.

The complication is for live tests, we really do want to use the user's ssh key,
and the test cases and setup code are used in both cases. Some shuffling of how
suite and test case setup is done was needed to make that work.

https://codereview.appspot.com/7943044/

[Martin Packman (gz)]

Reviewers: mp+154667_code.launchpad.net,

Message:
Please take a look.

Description:
Make openstack tests work with no ssh public key

The juju tests are not run in a well isolated manner, particularly they
will
examime the user's home directory and environment variables. This branch
fixes
the issue of openstack client setup complaining of no ssh public key is
found in
~/.ssh even in tests that are just using the local fake server
implementations.

Following the example of how this was fixed for the EC2 tests, this is
done in
an ad hoc manner by adding the 'authorized-keys' config with a fake
value to
tests that hit this condition.

The complication is for live tests, we really do want to use the user's
ssh key,
and the test cases and setup code are used in both cases. Some shuffling
of how
suite and test case setup is done was needed to make that work.

https://code.launchpad.net/~gz/juju-core/tests_os_ssh_isolation_1144704/+merge/154667

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M environs/openstack/config_test.go
   M environs/openstack/live_test.go
   M environs/openstack/local_test.go

[John A Meinel (jameinel)]

LGTM

Could use a comment about how the copy preserves test isolation, but
otherwise yay.

https://codereview.appspot.com/7943044/diff/1/environs/openstack/live_test.go
File environs/openstack/live_test.go (left):

https://codereview.appspot.com/7943044/diff/1/environs/openstack/live_test.go#oldcode104
environs/openstack/live_test.go:104: }
I think we should add a comment here that we are copying t.Config to
ensure tests stay isolated from eachother.

https://codereview.appspot.com/7943044/diff/1/environs/openstack/local_test.go
File environs/openstack/local_test.go (left):

https://codereview.appspot.com/7943044/diff/1/environs/openstack/local_test.go#oldcode205
environs/openstack/local_test.go:205: }
And probably another comment here.

The reason for the comment is because even if we didn't have config to
change here, we would need the copy, because otherwise the Config map
gets updated as a side effect of other bootstrap type operations. (And
then tests are coupled and very sad.)

https://codereview.appspot.com/7943044/

[William Reade (fwereade)]

One half-baked idea for your consideration. Let me know what you think.

https://codereview.appspot.com/7943044/diff/1/environs/openstack/live_test.go
File environs/openstack/live_test.go (right):

https://codereview.appspot.com/7943044/diff/1/environs/openstack/live_test.go#newcode142
environs/openstack/live_test.go:142: // Config and neither includes the
other so it would need to be copied in both.
I suspect you could embed one of these in each suite that previously had
a plain Config and use it to do all the updating consistently without
needing to change the code that just reads from .Config:

type TestConfig struct {
     Config map[string]interface{}
}

func (cfg *TestConfig) UpdateConfig(kv map[string]interface{}) {
     // ...as in updatedTestConfig, but:
     cfg.Config = newConfig
}

You'd want different creation funcs in different test packages, but the
openstack one would look pretty familiar:

func makeTestConfig(cred *identity.Credentials) jujutest.TestConfig {
     return jujutest.TestConfig{map[string]interface{}{
         // as in makeTestConfig
     }}
}

And would be pretty convenient to use:

// ...
     LiveTests: jujutest.LiveTests{
         TestConfig: makeTestConfig(cred),
// ...

// ...
config := makeTestConfig(cred)
// ...
config.UpdateConfig(map[string]interface{}{
     "default-image-id": ...
})
// ...

Sane? On reflection, it might be nicer to stick the map[string]interface
somewhere private and have a Config() method that copies it; that feels
like it might be a firmer guard against accidental pollution.

https://codereview.appspot.com/7943044/

[Martin Packman (gz)]

Please take a look.

https://codereview.appspot.com/7943044/

[Martin Packman (gz)]

On 2013/03/25 23:51:39, dfc wrote:
> LGTM I guess. I'm uneasy that in every test case we use the
authorised-keys key,
> but in practice that configuration item is always unset, defaulting to
the value
> of authorised-key-path, which defaults to $HOME/.ssh/...

I'm not to worried about that, it's easy to test config in isolation and
retesting it in local server tests doesn't add anything. Also, if we
break it, everyone will notice.

> Understanding it is more work, I'd be happier if this mocked
$HOME/.ssh

I'd be happier if all tests that may look at local disk or environment
variables ran inside a test-isolated home, but that was proposed by
Roger previously and rejected. We might want to revist it.

> environs/ec2/live_test.go:52: TestConfig:
jujutest.TestConfig{attrs},
> Why rename this field ?

It's no longer the same thing, I prefer renaming things when there are
semantic changes, helps prevent confusion, bogus merges, and such like.

https://codereview.appspot.com/7943044/

[Martin Packman (gz)]

Please take a look.

https://codereview.appspot.com/7943044/

[John A Meinel (jameinel)]

I certainly think it is better than what we have. I'd like to have a bit
more isolation between tests (.Config() as a method that copies the
internal map, rather than .Config an attribute that exposes it.) But
otherwise:
LGTM

https://codereview.appspot.com/7943044/

[Martin Packman (gz)]

*** Submitted:

Make openstack tests work with no ssh public key

The juju tests are not run in a well isolated manner, particularly they
will
examime the user's home directory and environment variables. This branch
fixes
the issue of openstack client setup complaining of no ssh public key is
found in
~/.ssh even in tests that are just using the local fake server
implementations.

Following the example of how this was fixed for the EC2 tests, this is
done in
an ad hoc manner by adding the 'authorized-keys' config with a fake
value to
tests that hit this condition.

The complication is for live tests, we really do want to use the user's
ssh key,
and the test cases and setup code are used in both cases. Some shuffling
of how
suite and test case setup is done was needed to make that work.

R=jameinel, fwereade, dfc
CC=
https://codereview.appspot.com/7943044

https://codereview.appspot.com/7943044/

=== modified file 'environs/dummy/environs_test.go'
--- environs/dummy/environs_test.go	2012-11-27 09:19:59 +0000
+++ environs/dummy/environs_test.go	2013-03-26 12:52:19 +0000
@@ -20,12 +20,12 @@
 		"ca-private-key":  testing.CAKey,
 	}
 	Suite(&jujutest.LiveTests{
-		Config:         attrs,
+		TestConfig:     jujutest.TestConfig{attrs},
 		CanOpenState:   true,
 		HasProvisioner: false,
 	})
 	Suite(&jujutest.Tests{
-		Config: attrs,
+		TestConfig: jujutest.TestConfig{attrs},
 	})
 }
 

=== modified file 'environs/ec2/live_test.go'
--- environs/ec2/live_test.go	2013-03-20 21:35:46 +0000
+++ environs/ec2/live_test.go	2013-03-26 12:52:19 +0000
@@ -50,7 +50,7 @@
 	}
 	Suite(&LiveTests{
 		LiveTests: jujutest.LiveTests{
-			Config:         attrs,
+			TestConfig:     jujutest.TestConfig{attrs},
 			Attempt:        *ec2.ShortAttempt,
 			CanOpenState:   true,
 			HasProvisioner: true,
@@ -68,7 +68,8 @@
 
 func (t *LiveTests) SetUpSuite(c *C) {
 	t.LoggingSuite.SetUpSuite(c)
-	e, err := environs.NewFromAttrs(t.Config)
+	// TODO: Share code from jujutest.LiveTests for creating environment
+	e, err := environs.NewFromAttrs(t.TestConfig.Config)
 	c.Assert(err, IsNil)
 
 	// Environ.PublicStorage() is read only.

=== modified file 'environs/ec2/local_test.go'
--- environs/ec2/local_test.go	2013-03-25 17:11:21 +0000
+++ environs/ec2/local_test.go	2013-03-26 12:52:19 +0000
@@ -100,19 +100,19 @@
 
 	Suite(&localServerSuite{
 		Tests: jujutest.Tests{
-			Config: attrs,
+			TestConfig: jujutest.TestConfig{attrs},
 		},
 	})
 	Suite(&localLiveSuite{
 		LiveTests: LiveTests{
 			LiveTests: jujutest.LiveTests{
-				Config: attrs,
+				TestConfig: jujutest.TestConfig{attrs},
 			},
 		},
 	})
 	Suite(&localNonUSEastSuite{
 		tests: jujutest.Tests{
-			Config: attrs,
+			TestConfig: jujutest.TestConfig{attrs},
 		},
 		srv: localServer{
 			config: &s3test.Config{

=== modified file 'environs/jujutest/livetests.go'
--- environs/jujutest/livetests.go	2013-03-21 04:04:27 +0000
+++ environs/jujutest/livetests.go	2013-03-26 12:52:19 +0000
@@ -24,8 +24,8 @@
 type LiveTests struct {
 	coretesting.LoggingSuite
 
-	// Config holds the configuration attributes for opening an environment.
-	Config map[string]interface{}
+	// TestConfig contains the configuration attributes for opening an environment.
+	TestConfig TestConfig
 
 	// Env holds the currently opened environment.
 	Env environs.Environ
@@ -47,8 +47,8 @@
 
 func (t *LiveTests) SetUpSuite(c *C) {
 	t.LoggingSuite.SetUpSuite(c)
-	e, err := environs.NewFromAttrs(t.Config)
-	c.Assert(err, IsNil, Commentf("opening environ %#v", t.Config))
+	e, err := environs.NewFromAttrs(t.TestConfig.Config)
+	c.Assert(err, IsNil, Commentf("opening environ %#v", t.TestConfig.Config))
 	c.Assert(e, NotNil)
 	t.Env = e
 	c.Logf("environment configuration: %#v", publicAttrs(e))

=== modified file 'environs/jujutest/tests.go'
--- environs/jujutest/tests.go	2013-03-20 21:19:46 +0000
+++ environs/jujutest/tests.go	2013-03-26 12:52:19 +0000
@@ -15,6 +15,25 @@
 	"sort"
 )
 
+// TestConfig contains the configuration for the environment
+// This is a is an indirection to make it harder for tests to accidentally
+// share the underlying map.
+type TestConfig struct {
+	Config map[string]interface{}
+}
+
+// UpdateConfig modifies the configuration safely by creating a new map
+func (testConfig *TestConfig) UpdateConfig(update map[string]interface{}) {
+	newConfig := map[string]interface{}{}
+	for key, val := range testConfig.Config {
+		newConfig[key] = val
+	}
+	for key, val := range update {
+		newConfig[key] = val
+	}
+	testConfig.Config = newConfig
+}
+
 // Tests is a gocheck suite containing tests verifying juju functionality
 // against the environment with the given configuration. The
 // tests are not designed to be run against a live server - the Environ
@@ -22,14 +41,14 @@
 // may be executed.
 type Tests struct {
 	coretesting.LoggingSuite
-	Config map[string]interface{}
-	Env    environs.Environ
+	TestConfig TestConfig
+	Env        environs.Environ
 }
 
 // Open opens an instance of the testing environment.
 func (t *Tests) Open(c *C) environs.Environ {
-	e, err := environs.NewFromAttrs(t.Config)
-	c.Assert(err, IsNil, Commentf("opening environ %#v", t.Config))
+	e, err := environs.NewFromAttrs(t.TestConfig.Config)
+	c.Assert(err, IsNil, Commentf("opening environ %#v", t.TestConfig.Config))
 	c.Assert(e, NotNil)
 	return e
 }

=== modified file 'environs/openstack/config_test.go'
--- environs/openstack/config_test.go	2013-03-08 20:31:57 +0000
+++ environs/openstack/config_test.go	2013-03-26 12:52:19 +0000
@@ -57,7 +57,8 @@
 	envs := attrs{
 		"environments": attrs{
 			"testenv": attrs{
-				"type": "openstack",
+				"type":            "openstack",
+				"authorized-keys": "fakekey",
 			},
 		},
 	}
@@ -372,7 +373,8 @@
 	envs := attrs{
 		"environments": attrs{
 			"testenv": attrs{
-				"type": "openstack",
+				"type":            "openstack",
+				"authorized-keys": "fakekey",
 			},
 		},
 	}
@@ -397,7 +399,8 @@
 	envs := attrs{
 		"environments": attrs{
 			"testenv": attrs{
-				"type": "openstack",
+				"type":            "openstack",
+				"authorized-keys": "fakekey",
 			},
 		},
 	}

=== modified file 'environs/openstack/live_test.go'
--- environs/openstack/live_test.go	2013-02-27 10:24:35 +0000
+++ environs/openstack/live_test.go	2013-03-26 12:52:19 +0000
@@ -26,7 +26,7 @@
 	return fmt.Sprintf("%x", buf)
 }
 
-func makeTestConfig() map[string]interface{} {
+func makeTestConfig(cred *identity.Credentials) map[string]interface{} {
 	// The following attributes hold the environment configuration
 	// for running the OpenStack integration tests.
 	//
@@ -36,22 +36,33 @@
 	//  secret-key: $OS_PASSWORD
 	//
 	attrs := map[string]interface{}{
-		"name":           "sample-" + randomName(),
-		"type":           "openstack",
-		"auth-mode":      "userpass",
-		"control-bucket": "juju-test-" + randomName(),
-		"ca-cert":        coretesting.CACert,
-		"ca-private-key": coretesting.CAKey,
+		"name":            "sample-" + randomName(),
+		"type":            "openstack",
+		"auth-mode":       "userpass",
+		"control-bucket":  "juju-test-" + randomName(),
+		"ca-cert":         coretesting.CACert,
+		"ca-private-key":  coretesting.CAKey,
+		"authorized-keys": "fakekey",
+		"admin-secret":    "secret",
+		"username":        cred.User,
+		"password":        cred.Secrets,
+		"region":          cred.Region,
+		"auth-url":        cred.URL,
+		"tenant-name":     cred.TenantName,
 	}
 	return attrs
 }
 
 // Register tests to run against a real Openstack instance.
 func registerLiveTests(cred *identity.Credentials, testImageDetails openstack.ImageDetails) {
+	config := makeTestConfig(cred)
+	config["default-image-id"] = testImageDetails.ImageId
+	config["default-instance-type"] = testImageDetails.Flavor
 	Suite(&LiveTests{
 		cred: cred,
 		LiveTests: jujutest.LiveTests{
-			Attempt: *openstack.ShortAttempt,
+			TestConfig: jujutest.TestConfig{config},
+			Attempt:    *openstack.ShortAttempt,
 			// TODO: Bug #1133263, once the infrastructure is set up,
 			//       enable The state tests on openstack
 			CanOpenState: false,
@@ -79,29 +90,18 @@
 
 func (t *LiveTests) SetUpSuite(c *C) {
 	t.LoggingSuite.SetUpSuite(c)
-	// Get an authenticated Goose client to extract some configuration parameters for the test environment.
+	// Update some Config items now that we have services running.
+	// This is setting the public-bucket-url and auth-url because that
+	// information is set during startup of the localLiveSuite
 	cl := client.NewClient(t.cred, identity.AuthUserPass, nil)
 	err := cl.Authenticate()
 	c.Assert(err, IsNil)
 	publicBucketURL, err := cl.MakeServiceURL("object-store", nil)
 	c.Assert(err, IsNil)
-	attrs := makeTestConfig()
-	attrs["admin-secret"] = "secret"
-	attrs["username"] = t.cred.User
-	attrs["password"] = t.cred.Secrets
-	attrs["region"] = t.cred.Region
-	attrs["auth-url"] = t.cred.URL
-	attrs["tenant-name"] = t.cred.TenantName
-	attrs["public-bucket-url"] = publicBucketURL
-	attrs["default-image-id"] = t.testImageId
-	attrs["default-instance-type"] = t.testFlavor
-	t.Config = attrs
-	t.LiveTests = jujutest.LiveTests{
-		Config:         attrs,
-		Attempt:        *openstack.ShortAttempt,
-		CanOpenState:   false, // no state; local tests (unless -live is passed)
-		HasProvisioner: false, // don't deploy anything
-	}
+	t.TestConfig.UpdateConfig(map[string]interface{}{
+		"public-bucket-url": publicBucketURL,
+		"auth-url":          t.cred.URL,
+	})
 	t.LiveTests.SetUpSuite(c)
 	// Environ.PublicStorage() is read only.
 	// For testing, we create a specific storage instance which is authorised to write to

=== modified file 'environs/openstack/local_test.go'
--- environs/openstack/local_test.go	2013-03-20 21:19:46 +0000
+++ environs/openstack/local_test.go	2013-03-26 12:52:19 +0000
@@ -92,13 +92,25 @@
 		Region:     "some region",
 		TenantName: "some tenant",
 	}
+	config := makeTestConfig(cred)
+	config["authorized-keys"] = "fakekey"
+	config["default-image-id"] = "1"
+	config["default-instance-type"] = "m1.small"
 	Suite(&localLiveSuite{
 		LiveTests: LiveTests{
-			cred: cred,
+			cred:        cred,
+			testImageId: "1",
+			testFlavor:  "m1.small",
+			LiveTests: jujutest.LiveTests{
+				TestConfig: jujutest.TestConfig{config},
+			},
 		},
 	})
 	Suite(&localServerSuite{
 		cred: cred,
+		Tests: jujutest.Tests{
+			TestConfig: jujutest.TestConfig{config},
+		},
 	})
 }
 
@@ -117,6 +129,7 @@
 	s.Mux = http.NewServeMux()
 	s.Server.Config.Handler = s.Mux
 	cred.URL = s.Server.URL
+	c.Logf("Started service at: %v", s.Server.URL)
 	s.Service = openstackservice.New(cred)
 	s.Service.SetupHTTP(s.Mux)
 	openstack.ShortTimeouts(true)
@@ -139,9 +152,6 @@
 func (s *localLiveSuite) SetUpSuite(c *C) {
 	s.LoggingSuite.SetUpSuite(c)
 	c.Logf("Running live tests using openstack service test double")
-
-	s.testImageId = "1"
-	s.testFlavor = "m1.small"
 	s.srv.start(c, s.cred)
 	s.LiveTests.SetUpSuite(c)
 }
@@ -185,25 +195,12 @@
 	s.LoggingSuite.TearDownSuite(c)
 }
 
-func testConfig(cred *identity.Credentials) map[string]interface{} {
-	attrs := makeTestConfig()
-	attrs["admin-secret"] = "secret"
-	attrs["username"] = cred.User
-	attrs["password"] = cred.Secrets
-	attrs["region"] = cred.Region
-	attrs["auth-url"] = cred.URL
-	attrs["tenant-name"] = cred.TenantName
-	attrs["default-image-id"] = "1"
-	attrs["default-instance-type"] = "m1.small"
-	return attrs
-}
-
 func (s *localServerSuite) SetUpTest(c *C) {
 	s.LoggingSuite.SetUpTest(c)
 	s.srv.start(c, s.cred)
-	s.Tests = jujutest.Tests{
-		Config: testConfig(s.cred),
-	}
+	s.TestConfig.UpdateConfig(map[string]interface{}{
+		"auth-url": s.cred.URL,
+	})
 	s.Tests.SetUpTest(c)
 	writeablePublicStorage := openstack.WritablePublicStorage(s.Env)
 	putFakeTools(c, writeablePublicStorage)
@@ -227,12 +224,11 @@
 	)
 	defer cleanup()
 	// Create a config that matches s.Config but with use-floating-ip set to true
-	newconfig := make(map[string]interface{}, len(s.Config))
-	for k, v := range s.Config {
-		newconfig[k] = v
-	}
-	newconfig["use-floating-ip"] = true
-	env, err := environs.NewFromAttrs(newconfig)
+	s.TestConfig.UpdateConfig(map[string]interface{}{
+		"use-floating-ip": true,
+	})
+	// TODO: Just share jujutest.Tests.Open rather than accessing .Config
+	env, err := environs.NewFromAttrs(s.TestConfig.Config)
 	c.Assert(err, IsNil)
 	err = environs.Bootstrap(env, constraints.Value{})
 	c.Assert(err, ErrorMatches, ".*cannot allocate a public IP as needed.*")