Expand store location to include runRoot and opts

Expand the syntax of the store's location to allow it to also contain
the runRoot location (optional, after the root, separated by '+') and a
list of driver options (optional, after the runRoot, separated by ":",
and itself broken up using ",").

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

storage/storage_reference.go

@@ -83,7 +83,12 @@ func (s storageReference) DockerReference() reference.Named {
 // disambiguate between images which may be present in multiple stores and
 // share only their names.
 func (s storageReference) StringWithinTransport() string {
-	storeSpec := "[" + s.transport.store.GraphDriverName() + "@" + s.transport.store.GraphRoot() + "]"
+	optionsList := ""
+	options := s.transport.store.GraphOptions()
+	if len(options) > 0 {
+		optionsList = ":" + strings.Join(options, ",")
+	}
+	storeSpec := "[" + s.transport.store.GraphDriverName() + "@" + s.transport.store.GraphRoot() + "+" + s.transport.store.RunRoot() + optionsList + "]"
 	if s.name == nil {
 		return storeSpec + "@" + s.id
 	}
@@ -94,7 +99,14 @@ func (s storageReference) StringWithinTransport() string {
 }
 
 func (s storageReference) PolicyConfigurationIdentity() string {
-	return s.StringWithinTransport()
+	storeSpec := "[" + s.transport.store.GraphDriverName() + "@" + s.transport.store.GraphRoot() + "]"
+	if s.name == nil {
+		return storeSpec + "@" + s.id
+	}
+	if s.id == "" {
+		return storeSpec + s.reference
+	}
+	return storeSpec + s.reference + "@" + s.id
 }
 
 // Also accept policy that's tied to the combination of the graph root and

storage/storage_reference_test.go

@@ -2,6 +2,7 @@ package storage
 
 import (
 	"fmt"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -57,7 +58,12 @@ var validReferenceTestCases = []struct {
 
 func TestStorageReferenceStringWithinTransport(t *testing.T) {
 	store := newStore(t)
-	storeSpec := fmt.Sprintf("[%s@%s]", store.GraphDriverName(), store.GraphRoot())
+	optionsList := ""
+	options := store.GraphOptions()
+	if len(options) > 0 {
+		optionsList = ":" + strings.Join(options, ",")
+	}
+	storeSpec := fmt.Sprintf("[%s@%s+%s%s]", store.GraphDriverName(), store.GraphRoot(), store.RunRoot(), optionsList)
 
 	for _, c := range validReferenceTestCases {
 		ref, err := Transport.ParseReference(c.input)

storage/storage_test.go

@@ -58,7 +58,7 @@ func TestMain(m *testing.M) {
 	os.Exit(code)
 }
 
-func newStore(t *testing.T) storage.Store {
+func newStoreWithGraphDriverOptions(t *testing.T, options []string) storage.Store {
 	wd, err := ioutil.TempDir(topwd, "test.")
 	if err != nil {
 		t.Fatal(err)
@@ -83,7 +83,7 @@ func newStore(t *testing.T) storage.Store {
 		RunRoot:            run,
 		GraphRoot:          root,
 		GraphDriverName:    "vfs",
-		GraphDriverOptions: []string{},
+		GraphDriverOptions: options,
 		UIDMap:             uidmap,
 		GIDMap:             gidmap,
 	})
@@ -94,6 +94,10 @@ func newStore(t *testing.T) storage.Store {
 	return store
 }
 
+func newStore(t *testing.T) storage.Store {
+	return newStoreWithGraphDriverOptions(t, []string{})
+}
+
 func TestParse(t *testing.T) {
 	store := newStore(t)
 
@@ -162,6 +166,46 @@ func TestParse(t *testing.T) {
 	}
 }
 
+func TestParseWithGraphDriverOptions(t *testing.T) {
+	optionLists := [][]string{
+		{},
+		{"unused1"},
+		{"unused1", "unused2"},
+		{"unused1", "unused2", "unused3"},
+	}
+	for _, optionList := range optionLists {
+		store := newStoreWithGraphDriverOptions(t, optionList)
+		ref, err := Transport.ParseStoreReference(store, "test")
+		if err != nil {
+			t.Fatalf("ParseStoreReference(%q, graph driver options %v) returned error %v", "test", optionList, err)
+		}
+		if ref == nil {
+			t.Fatalf("ParseStoreReference returned nil reference")
+		}
+		spec := ref.StringWithinTransport()
+		ref2, err := Transport.ParseReference(spec)
+		if err != nil {
+			t.Fatalf("ParseReference(%q) returned error %v", "test", err)
+		}
+		if ref == nil {
+			t.Fatalf("ParseReference returned nil reference")
+		}
+		sref, ok := ref2.(*storageReference)
+		if !ok {
+			t.Fatalf("ParseReference returned a reference from transport %s, not one of ours", ref2.Transport().Name())
+		}
+		parsedOptions := sref.transport.store.GraphOptions()
+		if len(parsedOptions) != len(optionList) {
+			t.Fatalf("Lost options between %v and %v", optionList, parsedOptions)
+		}
+		for i := range optionList {
+			if parsedOptions[i] != optionList[i] {
+				t.Fatalf("Mismatched option %d: %v and %v", i, optionList[i], parsedOptions[i])
+			}
+		}
+	}
+}
+
 func systemContext() *types.SystemContext {
 	return &types.SystemContext{}
 }

storage/storage_transport.go

@@ -110,7 +110,12 @@ func (s storageTransport) ParseStoreReference(store storage.Store, ref string) (
 		// recognize.
 		return nil, ErrInvalidReference
 	}
-	storeSpec := "[" + store.GraphDriverName() + "@" + store.GraphRoot() + "]"
+	optionsList := ""
+	options := store.GraphOptions()
+	if len(options) > 0 {
+		optionsList = ":" + strings.Join(options, ",")
+	}
+	storeSpec := "[" + store.GraphDriverName() + "@" + store.GraphRoot() + "+" + store.RunRoot() + optionsList + "]"
 	id := ""
 	if sum.Validate() == nil {
 		id = sum.Hex()
@@ -145,15 +150,11 @@ func (s *storageTransport) GetStore() (storage.Store, error) {
 
 // ParseReference takes a name and/or an ID ("_name_"/"@_id_"/"_name_@_id_"),
 // possibly prefixed with a store specifier in the form "[_graphroot_]" or
-// "[_driver_@_graphroot_]", tries to figure out which it is, and returns it in
+// "[_driver_@_graphroot_]" or "[_driver_@_graphroot_+_runroot_]" or
-// a reference object.  If the _graphroot_ is a location other than the default,
+// "[_driver_@_graphroot_:_options_]" or "[_driver_@_graphroot_+_runroot_:_options_]",
-// it needs to have been previously opened using storage.GetStore(), so that it
+// tries to figure out which it is, and returns it in a reference object.
-// can figure out which run root goes with the graph root.
 func (s *storageTransport) ParseReference(reference string) (types.ImageReference, error) {
-	store, err := s.GetStore()
+	var store storage.Store
-	if err != nil {
-		return nil, err
-	}
 	// Check if there's a store location prefix.  If there is, then it
 	// needs to match a store that was previously initialized using
 	// storage.GetStore(), or be enough to let the storage library fill out
@@ -165,37 +166,63 @@ func (s *storageTransport) ParseReference(reference string) (types.ImageReferenc
 		}
 		storeSpec := reference[1:closeIndex]
 		reference = reference[closeIndex+1:]
-		storeInfo := strings.SplitN(storeSpec, "@", 2)
+		// Peel off a "driver@" from the start.
-		if len(storeInfo) == 1 && storeInfo[0] != "" {
+		driverInfo := ""
-			// One component: the graph root.
+		driverSplit := strings.SplitN(storeSpec, "@", 2)
-			if !filepath.IsAbs(storeInfo[0]) {
+		if len(driverSplit) != 2 {
-				return nil, ErrPathNotAbsolute
+			if storeSpec == "" {
+				return nil, ErrInvalidReference
 			}
-			store2, err := storage.GetStore(storage.StoreOptions{
-				GraphRoot: storeInfo[0],
-			})
-			if err != nil {
-				return nil, err
-			}
-			store = store2
-		} else if len(storeInfo) == 2 && storeInfo[0] != "" && storeInfo[1] != "" {
-			// Two components: the driver type and the graph root.
-			if !filepath.IsAbs(storeInfo[1]) {
-				return nil, ErrPathNotAbsolute
-			}
-			store2, err := storage.GetStore(storage.StoreOptions{
-				GraphDriverName: storeInfo[0],
-				GraphRoot:       storeInfo[1],
-			})
-			if err != nil {
-				return nil, err
-			}
-			store = store2
 		} else {
-			// Anything else: store specified in a form we don't
+			driverInfo = driverSplit[0]
-			// recognize.
+			if driverInfo == "" {
-			return nil, ErrInvalidReference
+				return nil, ErrInvalidReference
+			}
+			storeSpec = driverSplit[1]
+			if storeSpec == "" {
+				return nil, ErrInvalidReference
+			}
 		}
+		// Peel off a ":options" from the end.
+		var options []string
+		optionsSplit := strings.SplitN(storeSpec, ":", 2)
+		if len(optionsSplit) == 2 {
+			options = strings.Split(optionsSplit[1], ",")
+			storeSpec = optionsSplit[0]
+		}
+		// Peel off a "+runroot" from the new end.
+		runRootInfo := ""
+		runRootSplit := strings.SplitN(storeSpec, "+", 2)
+		if len(runRootSplit) == 2 {
+			runRootInfo = runRootSplit[1]
+			storeSpec = runRootSplit[0]
+		}
+		// The rest is our graph root.
+		rootInfo := storeSpec
+		// Check that any paths are absolute paths.
+		if rootInfo != "" && !filepath.IsAbs(rootInfo) {
+			return nil, ErrPathNotAbsolute
+		}
+		if runRootInfo != "" && !filepath.IsAbs(runRootInfo) {
+			return nil, ErrPathNotAbsolute
+		}
+		store2, err := storage.GetStore(storage.StoreOptions{
+			GraphDriverName:    driverInfo,
+			GraphRoot:          rootInfo,
+			RunRoot:            runRootInfo,
+			GraphDriverOptions: options,
+		})
+		if err != nil {
+			return nil, err
+		}
+		store = store2
+	} else {
+		// We didn't have a store spec, so use the default.
+		store2, err := s.GetStore()
+		if err != nil {
+			return nil, err
+		}
+		store = store2
 	}
 	return s.ParseStoreReference(store, reference)
 }
@@ -250,7 +277,7 @@ func (s storageTransport) ValidatePolicyConfigurationScope(scope string) error {
 			return ErrPathNotAbsolute
 		}
 	} else {
-		// Anything else: store specified in a form we don't
+		// Anything else: scope specified in a form we don't
 		// recognize.
 		return ErrInvalidReference
 	}

storage/storage_transport_test.go

@@ -68,24 +68,24 @@ func TestTransportParseReference(t *testing.T) {
 	driver := store.GraphDriverName()
 	root := store.GraphRoot()
 
-	for _, c := range []struct{ prefix, expectedDriver, expectedRoot string }{
+	for _, c := range []struct{ prefix, expectedDriver, expectedRoot, expectedRunRoot string }{
-		{"", driver, root},                              // Implicit store location prefix
+		{"", driver, root, ""},                              // Implicit store location prefix
-		{"[unterminated", "", ""},                       // Unterminated store specifier
+		{"[unterminated", "", "", ""},                       // Unterminated store specifier
-		{"[]", "", ""},                                  // Empty store specifier
+		{"[]", "", "", ""},                                  // Empty store specifier
-		{"[relative/path]", "", ""},                     // Non-absolute graph root path
+		{"[relative/path]", "", "", ""},                     // Non-absolute graph root path
-		{"[" + driver + "@relative/path]", "", ""},      // Non-absolute graph root path
+		{"[" + driver + "@relative/path]", "", "", ""},      // Non-absolute graph root path
-		{"[thisisunknown@" + root + "suffix2]", "", ""}, // Unknown graph driver
+		{"[thisisunknown@" + root + "suffix2]", "", "", ""}, // Unknown graph driver
-
+		{"[" + root + "suffix1]", "", root + "suffix1", ""}, // A valid root path, but no run dir
-		// The next two could be valid, but aren't enough to allow GetStore() to locate a matching
+		{"[" + driver + "@" + root + "suffix3+" + root + "suffix4]",
-		// store, since the reference can't specify a RunRoot.  Without one, GetStore() tries to
+			driver,
-		// match the GraphRoot (possibly combined with the driver name) against a Store that was
+			root + "suffix3",
-		// previously opened using GetStore(), and we haven't done that.
+			root + "suffix4"}, // A valid root@graph+run set
-		// Future versions of the storage library will probably make this easier for locations that
+		{"[" + driver + "@" + root + "suffix3+" + root + "suffix4:options,options,options]",
-		// are shared, by caching the rest of the information inside the graph root so that it can
+			driver,
-		// be looked up later, but since this is a per-test temporary location, that won't help here.
+			root + "suffix3",
-		//{"[" + root + "suffix1]", driver, root + "suffix1"},                // A valid root path
+			root + "suffix4"}, // A valid root@graph+run+options set
-		//{"[" + driver + "@" + root + "suffix3]", driver, root + "suffix3"}, // A valid root@graph pair
 	} {
+		t.Logf("parsing %q", c.prefix+"busybox")
 		ref, err := Transport.ParseReference(c.prefix + "busybox")
 		if c.expectedDriver == "" {
 			assert.Error(t, err, c.prefix)
@@ -95,6 +95,9 @@ func TestTransportParseReference(t *testing.T) {
 			require.True(t, ok, c.prefix)
 			assert.Equal(t, c.expectedDriver, storageRef.transport.store.GraphDriverName(), c.prefix)
 			assert.Equal(t, c.expectedRoot, storageRef.transport.store.GraphRoot(), c.prefix)
+			if c.expectedRunRoot != "" {
+				assert.Equal(t, c.expectedRunRoot, storageRef.transport.store.RunRoot(), c.prefix)
+			}
 		}
 	}
 }