Reliably return the correct image ID from pull

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

Author: mtrmac

libimage/copier.go

@@ -175,8 +175,8 @@ type Copier struct {
 // newCopier creates a Copier based on a runtime's system context.
 // Note that fields in options *may* overwrite the counterparts of
 // the specified system context.  Please make sure to call `(*Copier).Close()`.
-func (r *Runtime) newCopier(options *CopyOptions) (*Copier, error) {
-	return NewCopier(options, r.SystemContext())
+func (r *Runtime) newCopier(options *CopyOptions, reportResolvedReference *types.ImageReference) (*Copier, error) {
+	return NewCopier(options, r.SystemContext(), reportResolvedReference)
 }
 
 // storageAllowedPolicyScopes overrides the policy for local storage
@@ -223,7 +223,7 @@ func getDockerAuthConfig(name, passwd, creds, idToken string) (*types.DockerAuth
 // NewCopier creates a Copier based on a provided system context.
 // Note that fields in options *may* overwrite the counterparts of
 // the specified system context.  Please make sure to call `(*Copier).Close()`.
-func NewCopier(options *CopyOptions, sc *types.SystemContext) (*Copier, error) {
+func NewCopier(options *CopyOptions, sc *types.SystemContext, reportResolvedReference *types.ImageReference) (*Copier, error) {
 	c := Copier{extendTimeoutSocket: options.extendTimeoutSocket}
 	sysContextCopy := *sc
 	c.systemContext = &sysContextCopy
@@ -330,6 +330,7 @@ func NewCopier(options *CopyOptions, sc *types.SystemContext) (*Copier, error) {
 	c.imageCopyOptions.SignBySigstorePrivateKeyFile = options.SignBySigstorePrivateKeyFile
 	c.imageCopyOptions.SignSigstorePrivateKeyPassphrase = options.SignSigstorePrivateKeyPassphrase
 	c.imageCopyOptions.ReportWriter = options.Writer
+	c.imageCopyOptions.ReportResolvedReference = reportResolvedReference
 
 	defaultContainerConfig, err := config.Default()
 	if err != nil {

libimage/import.go

@@ -104,7 +104,7 @@ func (r *Runtime) Import(ctx context.Context, path string, options *ImportOption
 		return "", err
 	}
 
-	c, err := r.newCopier(&options.CopyOptions)
+	c, err := r.newCopier(&options.CopyOptions, nil)
 	if err != nil {
 		return "", err
 	}

libimage/manifest_list.go

@@ -792,7 +792,7 @@ func (m *ManifestList) Push(ctx context.Context, destination string, options *Ma
 	// NOTE: we're using the logic in copier to create a proper
 	// types.SystemContext. This prevents us from having an error prone
 	// code duplicate here.
-	copier, err := m.image.runtime.newCopier(&options.CopyOptions)
+	copier, err := m.image.runtime.newCopier(&options.CopyOptions, nil)
 	if err != nil {
 		return "", err
 	}

libimage/pull.go

@@ -17,11 +17,11 @@ import (
 	dockerArchiveTransport "github.com/containers/image/v5/docker/archive"
 	dockerDaemonTransport "github.com/containers/image/v5/docker/daemon"
 	"github.com/containers/image/v5/docker/reference"
-	"github.com/containers/image/v5/manifest"
 	ociArchiveTransport "github.com/containers/image/v5/oci/archive"
 	ociTransport "github.com/containers/image/v5/oci/layout"
 	"github.com/containers/image/v5/pkg/shortnames"
 	storageTransport "github.com/containers/image/v5/storage"
+	"github.com/containers/image/v5/transports"
 	"github.com/containers/image/v5/transports/alltransports"
 	"github.com/containers/image/v5/types"
 	"github.com/containers/storage"
@@ -230,7 +230,7 @@ func nameFromAnnotations(annotations map[string]string) string {
 // copyFromDefault is the default copier for a number of transports.  Other
 // transports require some specific dancing, sometimes Yoga.
 func (r *Runtime) copyFromDefault(ctx context.Context, ref types.ImageReference, options *CopyOptions) ([]string, error) {
-	c, err := r.newCopier(options)
+	c, err := r.newCopier(options, nil)
 	if err != nil {
 		return nil, err
 	}
@@ -386,7 +386,7 @@ func (r *Runtime) copyFromDockerArchive(ctx context.Context, ref types.ImageRefe
 
 // copyFromDockerArchiveReaderReference copies the specified readerRef from reader.
 func (r *Runtime) copyFromDockerArchiveReaderReference(ctx context.Context, reader *dockerArchiveTransport.Reader, readerRef types.ImageReference, options *CopyOptions) ([]string, error) {
-	c, err := r.newCopier(options)
+	c, err := r.newCopier(options, nil)
 	if err != nil {
 		return nil, err
 	}
@@ -456,40 +456,6 @@ func (r *Runtime) copyFromRegistry(ctx context.Context, ref types.ImageReference
 	return pulledIDs, nil
 }
 
-// imageIDForPulledImage makes a best-effort guess at an image ID for
-// a just-pulled image written to destName, where the pull returned manifestBytes
-func (r *Runtime) imageIDForPulledImage(destName reference.Named, manifestBytes []byte) (string, error) {
-	// The caller, copySingleImageFromRegistry, never triggers a multi-platform copy, so manifestBytes
-	// is always a single-platform manifest instance.
-	manifestDigest, err := manifest.Digest(manifestBytes)
-	if err != nil {
-		return "", err
-	}
-	destDigestedName, err := reference.WithDigest(reference.TrimNamed(destName), manifestDigest)
-	if err != nil {
-		return "", err
-	}
-	storeRef, err := storageTransport.Transport.NewStoreReference(r.store, destDigestedName, "")
-	if err != nil {
-		return "", err
-	}
-	// With zstd:chunked partial pulls, the same image can have several
-	// different IDs, depending on which layers of the image were pulled using the
-	// partial pull (are identified by TOC, not by uncompressed digest).
-	//
-	// At this point, from just the manifest digest, we can’t tell which image
-	// is the one that was actually pulled. (They should all have the same contents
-	// unless the image author is malicious.)
-	//
-	// FIXME: To return an accurate value, c/image would need to return the image ID,
-	// not just manifestBytes.
-	_, image, err := storageTransport.ResolveReference(storeRef)
-	if err != nil {
-		return "", fmt.Errorf("looking up a just-pulled image: %w", err)
-	}
-	return image.ID, nil
-}
-
 // copySingleImageFromRegistry pulls the specified, possibly unqualified, name
 // from a registry.  On successful pull it returns the ID of the image in local
 // storage (or, FIXME, a name/ID? that could be resolved in local storage)
@@ -632,7 +598,8 @@ func (r *Runtime) copySingleImageFromRegistry(ctx context.Context, imageName str
 	if socketPath, ok := os.LookupEnv("NOTIFY_SOCKET"); ok {
 		options.extendTimeoutSocket = socketPath
 	}
-	c, err := r.newCopier(&options.CopyOptions)
+	var resolvedReference types.ImageReference
+	c, err := r.newCopier(&options.CopyOptions, &resolvedReference)
 	if err != nil {
 		return "", err
 	}
@@ -673,8 +640,7 @@ func (r *Runtime) copySingleImageFromRegistry(ctx context.Context, imageName str
 				return "", err
 			}
 		}
-		var manifestBytes []byte
-		if manifestBytes, err = c.Copy(ctx, srcRef, destRef); err != nil {
+		if _, err := c.Copy(ctx, srcRef, destRef); err != nil {
 			logrus.Debugf("Error pulling candidate %s: %v", candidateString, err)
 			pullErrors = append(pullErrors, err)
 			continue
@@ -687,11 +653,14 @@ func (r *Runtime) copySingleImageFromRegistry(ctx context.Context, imageName str
 		}
 
 		logrus.Debugf("Pulled candidate %s successfully", candidateString)
-		ids, err := r.imageIDForPulledImage(candidate.Value, manifestBytes)
+		if resolvedReference == nil { // resolvedReference should always be set for storageTransport destinations
+			return "", fmt.Errorf("internal error: After pulling %s, resolvedReference is nil", candidateString)
+		}
+		_, image, err := storageTransport.ResolveReference(resolvedReference)
 		if err != nil {
-			return "", err
+			return "", fmt.Errorf("resolving an already-resolved reference %q to the pulled image: %w", transports.ImageName(resolvedReference), err)
 		}
-		return ids, nil
+		return image.ID, nil
 	}
 
 	if localImage != nil && pullPolicy == config.PullPolicyNewer {

libimage/push.go

@@ -109,7 +109,7 @@ func (r *Runtime) Push(ctx context.Context, source, destination string, options
 		}
 	}
 
-	c, err := r.newCopier(&options.CopyOptions)
+	c, err := r.newCopier(&options.CopyOptions, nil)
 	if err != nil {
 		return nil, err
 	}

libimage/save.go

@@ -119,7 +119,7 @@ func (r *Runtime) saveSingleImage(ctx context.Context, name, format, path string
 		return err
 	}
 
-	c, err := r.newCopier(&options.CopyOptions)
+	c, err := r.newCopier(&options.CopyOptions, nil)
 	if err != nil {
 		return err
 	}
@@ -204,7 +204,7 @@ func (r *Runtime) saveDockerArchive(ctx context.Context, names []string, path st
 		copyOpts := options.CopyOptions
 		copyOpts.dockerArchiveAdditionalTags = local.tags
 
-		c, err := r.newCopier(&copyOpts)
+		c, err := r.newCopier(&copyOpts, nil)
 		if err != nil {
 			return err
 		}