Add storage.ResolveReference; deprecate GetImage and GetStoreImage

See the added comments for details.

This allows things like
> ref := ParseReference(userInput)
> ref2, img := ResolveReference(ref)
> src := ref2.NewImageSource

while ensuring that img and src are _guaranteed_ to refer
to the same image.

Signed-off-by: Miloslav Trmač <[email protected]>

storage/storage_reference.go

@@ -10,6 +10,7 @@ import (
 
 	"github.com/containers/image/v5/docker/reference"
 	"github.com/containers/image/v5/manifest"
+	"github.com/containers/image/v5/transports"
 	"github.com/containers/image/v5/types"
 	"github.com/containers/storage"
 	digest "github.com/opencontainers/go-digest"
@@ -283,3 +284,29 @@ func (s storageReference) NewImageSource(ctx context.Context, sys *types.SystemC
 func (s storageReference) NewImageDestination(ctx context.Context, sys *types.SystemContext) (types.ImageDestination, error) {
 	return newImageDestination(sys, s)
 }
+
+// ResolveReference finds the underlying storage image for a storage.Transport reference.
+// It returns that image, and an updated reference which can be used to refer back to the _same_
+// image again.
+//
+// This matters if the input reference contains a tagged name; the destination of the tag can
+// move in local storage. The updated reference returned by this function contains the resolved
+// image ID, so later uses of that updated reference will either continue to refer to the same
+// image, or fail.
+//
+// Note that it _is_ possible for the later uses to fail, either because the image was removed
+// completely, or because the name used in the reference was untaged (even if the underlying image
+// ID still exists in local storage).
+func ResolveReference(ref types.ImageReference) (types.ImageReference, *storage.Image, error) {
+	sref, ok := ref.(*storageReference)
+	if !ok {
+		return nil, nil, fmt.Errorf("trying to resolve a non-%s: reference %q", Transport.Name(),
+			transports.ImageName(ref))
+	}
+	clone := *sref // A shallow copy we can update
+	img, err := clone.resolveImage(nil)
+	if err != nil {
+		return nil, nil, err
+	}
+	return clone, img, nil
+}

storage/storage_reference_test.go

@@ -9,6 +9,9 @@ import (
 	"testing"
 
 	"github.com/containers/image/v5/docker/reference"
+	"github.com/containers/image/v5/pkg/blobinfocache/memory"
+	"github.com/containers/storage"
+	"github.com/containers/storage/pkg/archive"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -97,14 +100,19 @@ func TestStorageReferenceDockerReference(t *testing.T) {
 	}
 }
 
-func TestStorageReferenceStringWithinTransport(t *testing.T) {
-	store := newStore(t)
+// The […] part of references created for store
+func storeSpecForStringWithinTransport(store storage.Store) string {
 	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)
+	return fmt.Sprintf("[%s@%s+%s%s]", store.GraphDriverName(), store.GraphRoot(), store.RunRoot(), optionsList)
+}
+
+func TestStorageReferenceStringWithinTransport(t *testing.T) {
+	store := newStore(t)
+	storeSpec := storeSpecForStringWithinTransport(store)
 
 	for _, c := range validReferenceTestCases {
 		ref, err := Transport.ParseReference(c.input)
@@ -142,3 +150,47 @@ func TestStorageReferencePolicyConfigurationNamespaces(t *testing.T) {
 }
 
 // NewImage, NewImageSource, NewImageDestination, DeleteImage tested in storage_test.go
+
+func TestResolveReference(t *testing.T) {
+	// This is, so far, only a minimal smoke test
+
+	ensureTestCanCreateImages(t)
+
+	store := newStore(t)
+	storeSpec := storeSpecForStringWithinTransport(store)
+	cache := memory.New()
+
+	id := "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+
+	// Create an image with a known name and ID
+	ref, err := Transport.ParseStoreReference(store, "test@"+id)
+	require.NoError(t, err)
+	createImage(t, ref, cache, []testBlob{makeLayer(t, archive.Gzip)}, nil)
+
+	for _, c := range []struct {
+		input    string
+		expected string // "" on error
+	}{
+		{ // No ID match
+			"@bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", "",
+		},
+		{"@" + id, "@" + id},                                  // ID-only lookup
+		{"test", "docker.io/library/test:latest@" + id},       // Name is resolved to include ID
+		{"nottest", ""},                                       // No name match
+		{"test@" + id, "docker.io/library/test:latest@" + id}, // Name+ID works, and is unchanged
+		{"nottest@" + id, ""},                                 // Name mismatch is rejected even with an ID
+	} {
+		input, err := Transport.ParseStoreReference(store, c.input)
+		require.NoError(t, err, c.input)
+		inputClone := *input
+		resolved, img, err := ResolveReference(input)
+		if c.expected == "" {
+			assert.Error(t, err, c.input)
+		} else {
+			require.NoError(t, err, c.input)
+			require.Equal(t, &inputClone, input) // input was not modified in-place
+			assert.Equal(t, id, img.ID, c.input)
+			assert.Equal(t, storeSpec+c.expected, resolved.StringWithinTransport(), c.input)
+		}
+	}
+}

storage/storage_transport.go

@@ -48,9 +48,24 @@ type StoreTransport interface {
 	GetStoreIfSet() storage.Store
 	// GetImage retrieves the image from the transport's store that's named
 	// by the reference.
+	// Deprecated: Surprisingly, with a StoreTransport reference which contains an ID,
+	// this ignores that ID; and repeated calls of GetStoreImage with the same named reference
+	// can return different images, with no way for the caller to "freeze" the storage.Image identity
+	// without discarding the name entirely.
+	//
+	// Use storage.ResolveReference instead.
 	GetImage(types.ImageReference) (*storage.Image, error)
 	// GetStoreImage retrieves the image from a specified store that's named
 	// by the reference.
+	//
+	// Deprecated: Surprisingly, with a StoreTransport reference which contains an ID,
+	// this ignores that ID; and repeated calls of GetStoreImage with the same named reference
+	// can return different images, with no way for the caller to "freeze" the storage.Image identity
+	// without discarding the name entirely.
+	//
+	// Also, a StoreTransport reference already contains a store, so providing another one is redundant.
+	//
+	// Use storage.ResolveReference instead.
 	GetStoreImage(storage.Store, types.ImageReference) (*storage.Image, error)
 	// ParseStoreReference parses a reference, overriding any store
 	// specification that it may contain.
@@ -290,6 +305,14 @@ func (s *storageTransport) ParseReference(reference string) (types.ImageReferenc
 	return s.ParseStoreReference(store, reference)
 }
 
+// Deprecated: Surprisingly, with a StoreTransport reference which contains an ID,
+// this ignores that ID; and repeated calls of GetStoreImage with the same named reference
+// can return different images, with no way for the caller to "freeze" the storage.Image identity
+// without discarding the name entirely.
+//
+// Also, a StoreTransport reference already contains a store, so providing another one is redundant.
+//
+// Use storage.ResolveReference instead.
 func (s storageTransport) GetStoreImage(store storage.Store, ref types.ImageReference) (*storage.Image, error) {
 	dref := ref.DockerReference()
 	if dref != nil {
@@ -306,6 +329,12 @@ func (s storageTransport) GetStoreImage(store storage.Store, ref types.ImageRefe
 	return nil, storage.ErrImageUnknown
 }
 
+// Deprecated: Surprisingly, with a StoreTransport reference which contains an ID,
+// this ignores that ID; and repeated calls of GetStoreImage with the same named reference
+// can return different images, with no way for the caller to "freeze" the storage.Image identity
+// without discarding the name entirely.
+//
+// Use storage.ResolveReference instead.
 func (s *storageTransport) GetImage(ref types.ImageReference) (*storage.Image, error) {
 	store, err := s.GetStore()
 	if err != nil {