diff --git a/storage/storage_reference.go b/storage/storage_reference.go index 66a64792..9de2e5df 100644 --- a/storage/storage_reference.go +++ b/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 diff --git a/storage/storage_reference_test.go b/storage/storage_reference_test.go index 343dd43f..37ddcf33 100644 --- a/storage/storage_reference_test.go +++ b/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) diff --git a/storage/storage_test.go b/storage/storage_test.go index 25155a5b..d60b19b6 100644 --- a/storage/storage_test.go +++ b/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{} } diff --git a/storage/storage_transport.go b/storage/storage_transport.go index 6539e7ea..d6418335 100644 --- a/storage/storage_transport.go +++ b/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 -// a reference object. If the _graphroot_ is a location other than the default, -// it needs to have been previously opened using storage.GetStore(), so that it -// can figure out which run root goes with the graph root. +// "[_driver_@_graphroot_]" or "[_driver_@_graphroot_+_runroot_]" or +// "[_driver_@_graphroot_:_options_]" or "[_driver_@_graphroot_+_runroot_:_options_]", +// tries to figure out which it is, and returns it in a reference object. func (s *storageTransport) ParseReference(reference string) (types.ImageReference, error) { - store, err := s.GetStore() - if err != nil { - return nil, err - } + var store storage.Store // 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) - if len(storeInfo) == 1 && storeInfo[0] != "" { - // One component: the graph root. - if !filepath.IsAbs(storeInfo[0]) { - return nil, ErrPathNotAbsolute + // Peel off a "driver@" from the start. + driverInfo := "" + driverSplit := strings.SplitN(storeSpec, "@", 2) + if len(driverSplit) != 2 { + 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 - // recognize. - return nil, ErrInvalidReference + driverInfo = driverSplit[0] + if driverInfo == "" { + 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 } diff --git a/storage/storage_transport_test.go b/storage/storage_transport_test.go index 9dc21abb..bcccfcf4 100644 --- a/storage/storage_transport_test.go +++ b/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 }{ - {"", driver, root}, // Implicit store location prefix - {"[unterminated", "", ""}, // Unterminated store specifier - {"[]", "", ""}, // Empty store specifier - {"[relative/path]", "", ""}, // Non-absolute graph root path - {"[" + driver + "@relative/path]", "", ""}, // Non-absolute graph root path - {"[thisisunknown@" + root + "suffix2]", "", ""}, // Unknown graph driver - - // The next two could be valid, but aren't enough to allow GetStore() to locate a matching - // store, since the reference can't specify a RunRoot. Without one, GetStore() tries to - // match the GraphRoot (possibly combined with the driver name) against a Store that was - // previously opened using GetStore(), and we haven't done that. - // Future versions of the storage library will probably make this easier for locations that - // are shared, by caching the rest of the information inside the graph root so that it can - // be looked up later, but since this is a per-test temporary location, that won't help here. - //{"[" + root + "suffix1]", driver, root + "suffix1"}, // A valid root path - //{"[" + driver + "@" + root + "suffix3]", driver, root + "suffix3"}, // A valid root@graph pair + for _, c := range []struct{ prefix, expectedDriver, expectedRoot, expectedRunRoot string }{ + {"", driver, root, ""}, // Implicit store location prefix + {"[unterminated", "", "", ""}, // Unterminated store specifier + {"[]", "", "", ""}, // Empty store specifier + {"[relative/path]", "", "", ""}, // Non-absolute graph root path + {"[" + driver + "@relative/path]", "", "", ""}, // Non-absolute graph root path + {"[thisisunknown@" + root + "suffix2]", "", "", ""}, // Unknown graph driver + {"[" + root + "suffix1]", "", root + "suffix1", ""}, // A valid root path, but no run dir + {"[" + driver + "@" + root + "suffix3+" + root + "suffix4]", + driver, + root + "suffix3", + root + "suffix4"}, // A valid root@graph+run set + {"[" + driver + "@" + root + "suffix3+" + root + "suffix4:options,options,options]", + driver, + root + "suffix3", + root + "suffix4"}, // A valid root@graph+run+options set } { + 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) + } } } }