Merge lp:~julian-edwards/juju-core/maas-uuid-file-prefix into lp:~go-bot/juju-core/trunk

[Julian Edwards]

Use the environment's UUID as a file prefix for all files in MAAS private storage to prevent different environments from overlapping.

[William Reade (fwereade)]

Seems to be missing a test for having no environment-uuid at all? Otherwise LGTM (modulo what it's building on...).

[Julian Edwards (julian-edwards)]

On Wednesday 16 Oct 2013 05:51:10 you wrote:
> Seems to be missing a test for having no environment-uuid at all? Otherwise
> LGTM (modulo what it's building on...).

That's what TesttprefixWithPrivateNamespaceIgnoresEmptyUUID is for.

Thanks for the review.

=== modified file 'provider/maas/environ_test.go'
--- provider/maas/environ_test.go	2013-10-15 16:15:39 +0000
+++ provider/maas/environ_test.go	2013-10-16 05:32:45 +0000
@@ -4,6 +4,7 @@
 package maas
 
 import (
+	"bytes"
 	"encoding/base64"
 	"fmt"
 	"io/ioutil"
@@ -475,7 +476,9 @@
 	// Add a dummy file to storage so we can use that to check the
 	// obtained source later.
 	data := makeRandomBytes(10)
-	suite.testMAASObject.TestServer.NewFile("filename", data)
+	stor := NewStorage(env)
+	err := stor.Put("filename", bytes.NewBuffer([]byte(data)), int64(len(data)))
+	c.Assert(err, gc.IsNil)
 	sources, err := imagemetadata.GetMetadataSources(env)
 	c.Assert(err, gc.IsNil)
 	c.Assert(len(sources), gc.Equals, 2)
@@ -490,7 +493,9 @@
 	// Add a dummy file to storage so we can use that to check the
 	// obtained source later.
 	data := makeRandomBytes(10)
-	suite.testMAASObject.TestServer.NewFile("tools/filename", data)
+	stor := NewStorage(env)
+	err := stor.Put("tools/filename", bytes.NewBuffer([]byte(data)), int64(len(data)))
+	c.Assert(err, gc.IsNil)
 	sources, err := envtools.GetMetadataSources(env)
 	c.Assert(err, gc.IsNil)
 	c.Assert(len(sources), gc.Equals, 1)

=== modified file 'provider/maas/storage.go'
--- provider/maas/storage.go	2013-09-18 22:54:32 +0000
+++ provider/maas/storage.go	2013-10-16 05:32:45 +0000
@@ -11,6 +11,7 @@
 	"io/ioutil"
 	"net/url"
 	"sort"
+	"strings"
 	"sync"
 
 	"launchpad.net/gomaasapi"
@@ -88,8 +89,21 @@
 	return obj, nil
 }
 
+// All filenames need to be namespaced so they are private to this environment.
+// This prevents different environments from interfering with each other.
+// We're using the environment's UUID here.
+func (stor *maasStorage) prefixWithPrivateNamespace(name string) string {
+	env := stor.getSnapshot().environUnlocked
+	prefix := env.ecfg().maasEnvironmentUUID()
+	if prefix != "" {
+		return prefix + "-" + name
+	}
+	return name
+}
+
 // Get is specified in the StorageReader interface.
 func (stor *maasStorage) Get(name string) (io.ReadCloser, error) {
+	name = stor.prefixWithPrivateNamespace(name)
 	fileObj, err := stor.retrieveFileObject(name)
 	if err != nil {
 		return nil, err
@@ -108,6 +122,7 @@
 // extractFilenames returns the filenames from a "list" operation on the
 // MAAS API, sorted by name.
 func (stor *maasStorage) extractFilenames(listResult gomaasapi.JSONObject) ([]string, error) {
+	privatePrefix := stor.prefixWithPrivateNamespace("")
 	list, err := listResult.GetArray()
 	if err != nil {
 		return nil, err
@@ -122,7 +137,8 @@
 		if err != nil {
 			return nil, err
 		}
-		result[index] = filename
+		// When listing files we need to return them without our special prefix.
+		result[index] = strings.TrimPrefix(filename, privatePrefix)
 	}
 	sort.Strings(result)
 	return result, nil
@@ -130,6 +146,7 @@
 
 // List is specified in the StorageReader interface.
 func (stor *maasStorage) List(prefix string) ([]string, error) {
+	prefix = stor.prefixWithPrivateNamespace(prefix)
 	params := make(url.Values)
 	params.Add("prefix", prefix)
 	snapshot := stor.getSnapshot()
@@ -142,6 +159,7 @@
 
 // URL is specified in the StorageReader interface.
 func (stor *maasStorage) URL(name string) (string, error) {
+	name = stor.prefixWithPrivateNamespace(name)
 	fileObj, err := stor.retrieveFileObject(name)
 	if err != nil {
 		return "", err
@@ -174,6 +192,7 @@
 
 // Put is specified in the StorageWriter interface.
 func (stor *maasStorage) Put(name string, r io.Reader, length int64) error {
+	name = stor.prefixWithPrivateNamespace(name)
 	data, err := ioutil.ReadAll(io.LimitReader(r, length))
 	if err != nil {
 		return err
@@ -187,6 +206,7 @@
 
 // Remove is specified in the StorageWriter interface.
 func (stor *maasStorage) Remove(name string) error {
+	name = stor.prefixWithPrivateNamespace(name)
 	// The only thing that can go wrong here, really, is that the file
 	// does not exist.  But deletion is idempotent: deleting a file that
 	// is no longer there anyway is success, not failure.
@@ -200,7 +220,7 @@
 	if err != nil {
 		return err
 	}
-	// Remove all the objects in parallel so that we incur less round-trips.
+	// Remove all the objects in parallel so that we incur fewer round-trips.
 	// If we're in danger of having hundreds of objects,
 	// we'll want to change this to limit the number
 	// of concurrent operations.

=== modified file 'provider/maas/storage_test.go'
--- provider/maas/storage_test.go	2013-10-15 16:15:39 +0000
+++ provider/maas/storage_test.go	2013-10-16 05:32:45 +0000
@@ -29,8 +29,10 @@
 // makeStorage creates a MAAS storage object for the running test.
 func (s *storageSuite) makeStorage(name string) *maasStorage {
 	maasobj := s.testMAASObject.MAASObject
-	env := maasEnviron{name: name, maasClientUnlocked: &maasobj}
-	return NewStorage(&env).(*maasStorage)
+	env := s.makeEnviron()
+	env.name = name
+	env.maasClientUnlocked = &maasobj
+	return NewStorage(env).(*maasStorage)
 }
 
 // makeRandomBytes returns an array of arbitrary byte values.
@@ -50,7 +52,10 @@
 // Or don't, if you want consistent (and debuggable) results.
 func (s *storageSuite) fakeStoredFile(stor storage.Storage, name string) gomaasapi.MAASObject {
 	data := makeRandomBytes(rand.Intn(10))
-	return s.testMAASObject.TestServer.NewFile(name, data)
+	// The filename must be prefixed with the private namespace as we're
+	// bypassing the Put() method that would normally do that.
+	prefixFilename := stor.(*maasStorage).prefixWithPrivateNamespace("") + name
+	return s.testMAASObject.TestServer.NewFile(prefixFilename, data)
 }
 
 func (s *storageSuite) TestGetSnapshotCreatesClone(c *gc.C) {
@@ -93,7 +98,8 @@
 	fileContent, err := file.GetField("content")
 	c.Assert(err, gc.IsNil)
 
-	obj, err := stor.retrieveFileObject(filename)
+	prefixFilename := stor.prefixWithPrivateNamespace(filename)
+	obj, err := stor.retrieveFileObject(prefixFilename)
 	c.Assert(err, gc.IsNil)
 
 	uri, err := obj.GetField("anon_resource_uri")
@@ -117,7 +123,8 @@
 	err := stor.Put(filename, bytes.NewReader(data), int64(len(data)))
 	c.Assert(err, gc.IsNil)
 
-	obj, err := stor.retrieveFileObject(filename)
+	prefixFilename := stor.prefixWithPrivateNamespace(filename)
+	obj, err := stor.retrieveFileObject(prefixFilename)
 	c.Assert(err, gc.IsNil)
 
 	base64Content, err := obj.GetField("content")
@@ -390,3 +397,24 @@
 	c.Assert(err, gc.IsNil)
 	c.Assert(listing, gc.DeepEquals, []string{})
 }
+
+func (s *storageSuite) TestprefixWithPrivateNamespacePrefixesWithUUID(c *gc.C) {
+	sstor := NewStorage(s.makeEnviron())
+	stor := sstor.(*maasStorage)
+	uuid := stor.environUnlocked.ecfg().maasEnvironmentUUID()
+	c.Assert(uuid, gc.Not(gc.Equals), "")
+	expectedPrefix := uuid + "-"
+	const name = "myname"
+	expectedResult := expectedPrefix + name
+	c.Assert(stor.prefixWithPrivateNamespace(name), gc.Equals, expectedResult)
+}
+
+func (s *storageSuite) TesttprefixWithPrivateNamespaceIgnoresEmptyUUID(c *gc.C) {
+	sstor := NewStorage(s.makeEnviron())
+	stor := sstor.(*maasStorage)
+	ecfg := stor.environUnlocked.ecfg()
+	ecfg.attrs["environment-uuid"] = ""
+
+	const name = "myname"
+	c.Assert(stor.prefixWithPrivateNamespace(name), gc.Equals, name)
+}