Do not Close the ImageSource in UnparsedImage/Image

Remove the .Close() methods from UnparsedImage/Image, which closed the
underlying ImageSource.  Instead, just require the caller to ensure
that the ImageSource is not closed as long as the UnparsedImage/Image
are used.

This allows using several independent UnparsedImage/Image instances
for a shared ImageSource; notably independent Image objects for the
individual image instances in a manifest list.  (copy.Image is already
simpler although it is only using a single instance.)

To keep ImageReference.NewImage simple and not to break all the external
callers of this, also add a simple ImageCloser wrapper which retains
the ImageSource closing functionality, and return it from image.FromSource
and ImageReference.NewImage implementations.

(It's very likely many of the NewImage callers would be surprised by how this
handles manifest lists, and it is very tempting to break this API, at least
by renaming, to force the callers to consider this; however, this would be
better done after eliminating the need of ImageReference.NewImage entirely,
by replacing the specialized types.Image extensions with something else, first.)

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

copy/copy.go

@@ -134,6 +134,11 @@ func Image(policyContext *signature.PolicyContext, destRef, srcRef types.ImageRe
 	if err != nil {
 		return errors.Wrapf(err, "Error initializing source %s", transports.ImageName(srcRef))
 	}
+	defer func() {
+		if err := rawSource.Close(); err != nil {
+			retErr = errors.Wrapf(retErr, " (src: %v)", err)
+		}
+	}()
 
 	c := &copier{
 		copiedBlobs:      make(map[digest.Digest]digest.Digest),
@@ -146,13 +151,6 @@ func Image(policyContext *signature.PolicyContext, destRef, srcRef types.ImageRe
 	}
 
 	unparsedImage := image.UnparsedInstance(rawSource, nil)
-	defer func() {
-		if unparsedImage != nil {
-			if err := unparsedImage.Close(); err != nil {
-				retErr = errors.Wrapf(retErr, " (unparsed: %v)", err)
-			}
-		}
-	}()
 	multiImage, err := isMultiImage(unparsedImage)
 	if err != nil {
 		return errors.Wrapf(err, "Error determining manifest MIME type for %s", transports.ImageName(srcRef))
@@ -171,12 +169,6 @@ func Image(policyContext *signature.PolicyContext, destRef, srcRef types.ImageRe
 	if err != nil {
 		return errors.Wrapf(err, "Error initializing image from source %s", transports.ImageName(srcRef))
 	}
-	unparsedImage = nil
-	defer func() {
-		if err := src.Close(); err != nil {
-			retErr = errors.Wrapf(retErr, " (source: %v)", err)
-		}
-	}()
 
 	if err := checkImageDestinationForCurrentRuntimeOS(src, dest); err != nil {
 		return err

copy/manifest_test.go

@@ -35,9 +35,6 @@ type fakeImageSource string
 func (f fakeImageSource) Reference() types.ImageReference {
 	panic("Unexpected call to a mock function")
 }
-func (f fakeImageSource) Close() error {
-	panic("Unexpected call to a mock function")
-}
 func (f fakeImageSource) Manifest() ([]byte, string, error) {
 	if string(f) == "" {
 		return nil, "", errors.New("Manifest() directed to fail")

directory/directory_transport.go

@@ -134,11 +134,12 @@ func (ref dirReference) PolicyConfigurationNamespaces() []string {
 	return res
 }
 
-// NewImage returns a types.Image for this reference, possibly specialized for this ImageTransport.
-// The caller must call .Close() on the returned Image.
+// NewImage returns a types.ImageCloser for this reference, possibly specialized for this ImageTransport.
+// The caller must call .Close() on the returned ImageCloser.
 // NOTE: If any kind of signature verification should happen, build an UnparsedImage from the value returned by NewImageSource,
 // verify that UnparsedImage, and convert it into a real Image via image.FromUnparsedImage.
-func (ref dirReference) NewImage(ctx *types.SystemContext) (types.Image, error) {
+// WARNING: This may not do the right thing for a manifest list, see image.FromSource for details.
+func (ref dirReference) NewImage(ctx *types.SystemContext) (types.ImageCloser, error) {
 	src := newImageSource(ref)
 	return image.FromSource(src)
 }

docker/archive/transport.go

@@ -125,11 +125,12 @@ func (ref archiveReference) PolicyConfigurationNamespaces() []string {
 	return []string{}
 }
 
-// NewImage returns a types.Image for this reference, possibly specialized for this ImageTransport.
-// The caller must call .Close() on the returned Image.
+// NewImage returns a types.ImageCloser for this reference, possibly specialized for this ImageTransport.
+// The caller must call .Close() on the returned ImageCloser.
 // NOTE: If any kind of signature verification should happen, build an UnparsedImage from the value returned by NewImageSource,
 // verify that UnparsedImage, and convert it into a real Image via image.FromUnparsedImage.
-func (ref archiveReference) NewImage(ctx *types.SystemContext) (types.Image, error) {
+// WARNING: This may not do the right thing for a manifest list, see image.FromSource for details.
+func (ref archiveReference) NewImage(ctx *types.SystemContext) (types.ImageCloser, error) {
 	src := newImageSource(ctx, ref)
 	return ctrImage.FromSource(src)
 }

docker/daemon/daemon_transport.go

@@ -151,9 +151,12 @@ func (ref daemonReference) PolicyConfigurationNamespaces() []string {
 	return []string{}
 }
 
-// NewImage returns a types.Image for this reference.
-// The caller must call .Close() on the returned Image.
-func (ref daemonReference) NewImage(ctx *types.SystemContext) (types.Image, error) {
+// NewImage returns a types.ImageCloser for this reference, possibly specialized for this ImageTransport.
+// The caller must call .Close() on the returned ImageCloser.
+// NOTE: If any kind of signature verification should happen, build an UnparsedImage from the value returned by NewImageSource,
+// verify that UnparsedImage, and convert it into a real Image via image.FromUnparsedImage.
+// WARNING: This may not do the right thing for a manifest list, see image.FromSource for details.
+func (ref daemonReference) NewImage(ctx *types.SystemContext) (types.ImageCloser, error) {
 	src, err := newImageSource(ctx, ref)
 	if err != nil {
 		return nil, err

docker/docker_image.go

@@ -12,17 +12,17 @@ import (
 	"github.com/pkg/errors"
 )
 
-// Image is a Docker-specific implementation of types.Image with a few extra methods
+// Image is a Docker-specific implementation of types.ImageCloser with a few extra methods
 // which are specific to Docker.
 type Image struct {
-	types.Image
+	types.ImageCloser
 	src *dockerImageSource
 }
 
 // newImage returns a new Image interface type after setting up
 // a client to the registry hosting the given image.
 // The caller must call .Close() on the returned Image.
-func newImage(ctx *types.SystemContext, ref dockerReference) (types.Image, error) {
+func newImage(ctx *types.SystemContext, ref dockerReference) (types.ImageCloser, error) {
 	s, err := newImageSource(ctx, ref)
 	if err != nil {
 		return nil, err
@@ -31,7 +31,7 @@ func newImage(ctx *types.SystemContext, ref dockerReference) (types.Image, error
 	if err != nil {
 		return nil, err
 	}
-	return &Image{Image: img, src: s}, nil
+	return &Image{ImageCloser: img, src: s}, nil
 }
 
 // SourceRefFullName returns a fully expanded name for the repository this image is in.

docker/docker_transport.go

@@ -122,11 +122,12 @@ func (ref dockerReference) PolicyConfigurationNamespaces() []string {
 	return policyconfiguration.DockerReferenceNamespaces(ref.ref)
 }
 
-// NewImage returns a types.Image for this reference, possibly specialized for this ImageTransport.
-// The caller must call .Close() on the returned Image.
+// NewImage returns a types.ImageCloser for this reference, possibly specialized for this ImageTransport.
+// The caller must call .Close() on the returned ImageCloser.
 // NOTE: If any kind of signature verification should happen, build an UnparsedImage from the value returned by NewImageSource,
 // verify that UnparsedImage, and convert it into a real Image via image.FromUnparsedImage.
-func (ref dockerReference) NewImage(ctx *types.SystemContext) (types.Image, error) {
+// WARNING: This may not do the right thing for a manifest list, see image.FromSource for details.
+func (ref dockerReference) NewImage(ctx *types.SystemContext) (types.ImageCloser, error) {
 	return newImage(ctx, ref)
 }
 

image/docker_schema2_test.go

@@ -320,7 +320,7 @@ func (ref refImageReferenceMock) PolicyConfigurationIdentity() string {
 func (ref refImageReferenceMock) PolicyConfigurationNamespaces() []string {
 	panic("unexpected call to a mock function")
 }
-func (ref refImageReferenceMock) NewImage(ctx *types.SystemContext) (types.Image, error) {
+func (ref refImageReferenceMock) NewImage(ctx *types.SystemContext) (types.ImageCloser, error) {
 	panic("unexpected call to a mock function")
 }
 func (ref refImageReferenceMock) NewImageSource(ctx *types.SystemContext) (types.ImageSource, error) {

image/memory.go

@@ -33,11 +33,6 @@ func (i *memoryImage) Reference() types.ImageReference {
 	return nil
 }
 
-// Close removes resources associated with an initialized UnparsedImage, if any.
-func (i *memoryImage) Close() error {
-	return nil
-}
-
 // Size returns the size of the image as stored, if known, or -1 if not.
 func (i *memoryImage) Size() (int64, error) {
 	return -1, nil

image/sourced.go

@@ -7,11 +7,19 @@ import (
 	"github.com/containers/image/types"
 )
 
-// FromSource returns a types.Image implementation for the default instance of source.
+// imageCloser implements types.ImageCloser, perhaps allowing simple users
+// to use a single object without having keep a reference to a types.ImageSource
+// only to call types.ImageSource.Close().
+type imageCloser struct {
+	types.Image
+	src types.ImageSource
+}
+
+// FromSource returns a types.ImageCloser implementation for the default instance of source.
 // If source is a manifest list, .Manifest() still returns the manifest list,
 // but other methods transparently return data from an appropriate image instance.
 //
-// The caller must call .Close() on the returned Image.
+// The caller must call .Close() on the returned ImageCloser.
 //
 // FromSource “takes ownership” of the input ImageSource and will call src.Close()
 // when the image is closed.  (This does not prevent callers from using both the
@@ -20,8 +28,19 @@ import (
 //
 // NOTE: If any kind of signature verification should happen, build an UnparsedImage from the value returned by NewImageSource,
 // verify that UnparsedImage, and convert it into a real Image via image.FromUnparsedImage instead of calling this function.
-func FromSource(src types.ImageSource) (types.Image, error) {
-	return FromUnparsedImage(UnparsedInstance(src, nil))
+func FromSource(src types.ImageSource) (types.ImageCloser, error) {
+	img, err := FromUnparsedImage(UnparsedInstance(src, nil))
+	if err != nil {
+		return nil, err
+	}
+	return &imageCloser{
+		Image: img,
+		src:   src,
+	}, nil
+}
+
+func (ic *imageCloser) Close() error {
+	return ic.src.Close()
 }
 
 // sourcedImage is a general set of utilities for working with container images,
@@ -40,20 +59,15 @@ type sourcedImage struct {
 }
 
 // FromUnparsedImage returns a types.Image implementation for unparsed.
-// The caller must call .Close() on the returned Image.
+// If unparsed represents a manifest list, .Manifest() still returns the manifest list,
+// but other methods transparently return data from an appropriate single image.
 //
-// FromSource “takes ownership” of the input UnparsedImage and will call uparsed.Close()
-// when the image is closed.  (This does not prevent callers from using both the
-// UnparsedImage and ImageSource objects simultaneously, but it means that they only need to
-// keep a reference to the Image.)
+// The Image must not be used after the underlying ImageSource is Close()d.
 func FromUnparsedImage(unparsed *UnparsedImage) (types.Image, error) {
 	// Note that the input parameter above is specifically *image.UnparsedImage, not types.UnparsedImage:
 	// we want to be able to use unparsed.src.  We could make that an explicit interface, but, well,
 	// this is the only UnparsedImage implementation around, anyway.
 
-	// Also, we do not explicitly implement types.Image.Close; we let the implementation fall through to
-	// unparsed.Close.
-
 	// NOTE: It is essential for signature verification that all parsing done in this object happens on the same manifest which is returned by unparsed.Manifest().
 	manifestBlob, manifestMIMEType, err := unparsed.Manifest()
 	if err != nil {

image/unparsed.go

@@ -23,13 +23,9 @@ type UnparsedImage struct {
 }
 
 // UnparsedInstance returns a types.UnparsedImage implementation for (source, instanceDigest).
-// If instanceDigest is not nil, it contains a digest of the specific manifest instance to retrieve (when the primary manifest is a manifest list);
-// The caller must call .Close() on the returned UnparsedImage.
+// If instanceDigest is not nil, it contains a digest of the specific manifest instance to retrieve (when the primary manifest is a manifest list).
 //
-// UnparsedInstance “takes ownership” of the input ImageSource and will call src.Close()
-// when the image is closed.  (This does not prevent callers from using both the
-// UnparsedImage and ImageSource objects simultaneously, but it means that they only need to
-// keep a reference to the UnparsedImage.)
+// The UnparsedImage must not be used after the underlying ImageSource is Close()d.
 func UnparsedInstance(src types.ImageSource, instanceDigest *digest.Digest) *UnparsedImage {
 	return &UnparsedImage{
 		src:            src,
@@ -44,11 +40,6 @@ func (i *UnparsedImage) Reference() types.ImageReference {
 	return i.src.Reference()
 }
 
-// Close removes resources associated with an initialized UnparsedImage, if any.
-func (i *UnparsedImage) Close() error {
-	return i.src.Close()
-}
-
 // Manifest is like ImageSource.GetManifest, but the result is cached; it is OK to call this however often you need.
 func (i *UnparsedImage) Manifest() ([]byte, string, error) {
 	if i.cachedManifest == nil {

oci/archive/oci_transport.go

@@ -154,9 +154,12 @@ func (ref ociArchiveReference) PolicyConfigurationNamespaces() []string {
 	return res
 }
 
-// NewImage returns a types.Image for this reference, possibly specialized for this ImageTransport.
-// The caller must call .Close() on the returned Image.
-func (ref ociArchiveReference) NewImage(ctx *types.SystemContext) (types.Image, error) {
+// NewImage returns a types.ImageCloser for this reference, possibly specialized for this ImageTransport.
+// The caller must call .Close() on the returned ImageCloser.
+// NOTE: If any kind of signature verification should happen, build an UnparsedImage from the value returned by NewImageSource,
+// verify that UnparsedImage, and convert it into a real Image via image.FromUnparsedImage.
+// WARNING: This may not do the right thing for a manifest list, see image.FromSource for details.
+func (ref ociArchiveReference) NewImage(ctx *types.SystemContext) (types.ImageCloser, error) {
 	src, err := newImageSource(ctx, ref)
 	if err != nil {
 		return nil, err

oci/layout/oci_transport.go

@@ -177,11 +177,12 @@ func (ref ociReference) PolicyConfigurationNamespaces() []string {
 	return res
 }
 
-// NewImage returns a types.Image for this reference, possibly specialized for this ImageTransport.
-// The caller must call .Close() on the returned Image.
+// NewImage returns a types.ImageCloser for this reference, possibly specialized for this ImageTransport.
+// The caller must call .Close() on the returned ImageCloser.
 // NOTE: If any kind of signature verification should happen, build an UnparsedImage from the value returned by NewImageSource,
 // verify that UnparsedImage, and convert it into a real Image via image.FromUnparsedImage.
-func (ref ociReference) NewImage(ctx *types.SystemContext) (types.Image, error) {
+// WARNING: This may not do the right thing for a manifest list, see image.FromSource for details.
+func (ref ociReference) NewImage(ctx *types.SystemContext) (types.ImageCloser, error) {
 	src, err := newImageSource(ctx, ref)
 	if err != nil {
 		return nil, err

openshift/openshift_transport.go

@@ -125,11 +125,12 @@ func (ref openshiftReference) PolicyConfigurationNamespaces() []string {
 	return policyconfiguration.DockerReferenceNamespaces(ref.dockerReference)
 }
 
-// NewImage returns a types.Image for this reference, possibly specialized for this ImageTransport.
-// The caller must call .Close() on the returned Image.
+// NewImage returns a types.ImageCloser for this reference, possibly specialized for this ImageTransport.
+// The caller must call .Close() on the returned ImageCloser.
 // NOTE: If any kind of signature verification should happen, build an UnparsedImage from the value returned by NewImageSource,
 // verify that UnparsedImage, and convert it into a real Image via image.FromUnparsedImage.
-func (ref openshiftReference) NewImage(ctx *types.SystemContext) (types.Image, error) {
+// WARNING: This may not do the right thing for a manifest list, see image.FromSource for details.
+func (ref openshiftReference) NewImage(ctx *types.SystemContext) (types.ImageCloser, error) {
 	src, err := newImageSource(ctx, ref)
 	if err != nil {
 		return nil, err

ostree/ostree_transport.go

@@ -168,11 +168,12 @@ func (ref ostreeReference) PolicyConfigurationNamespaces() []string {
 	return res
 }
 
-// NewImage returns a types.Image for this reference, possibly specialized for this ImageTransport.
-// The caller must call .Close() on the returned Image.
+// NewImage returns a types.ImageCloser for this reference, possibly specialized for this ImageTransport.
+// The caller must call .Close() on the returned ImageCloser.
 // NOTE: If any kind of signature verification should happen, build an UnparsedImage from the value returned by NewImageSource,
 // verify that UnparsedImage, and convert it into a real Image via image.FromUnparsedImage.
-func (ref ostreeReference) NewImage(ctx *types.SystemContext) (types.Image, error) {
+// WARNING: This may not do the right thing for a manifest list, see image.FromSource for details.
+func (ref ostreeReference) NewImage(ctx *types.SystemContext) (types.ImageCloser, error) {
 	return nil, errors.New("Reading ostree: images is currently not supported")
 }
 

signature/policy_eval_signedby_test.go

@@ -15,16 +15,16 @@ import (
 )
 
 // dirImageMock returns a types.UnparsedImage for a directory, claiming a specified dockerReference.
-// The caller must call .Close() on the returned UnparsedImage.
-func dirImageMock(t *testing.T, dir, dockerReference string) types.UnparsedImage {
+// The caller must call the returned close callback when done.
+func dirImageMock(t *testing.T, dir, dockerReference string) (types.UnparsedImage, func() error) {
 	ref, err := reference.ParseNormalizedNamed(dockerReference)
 	require.NoError(t, err)
 	return dirImageMockWithRef(t, dir, refImageReferenceMock{ref})
 }
 
 // dirImageMockWithRef returns a types.UnparsedImage for a directory, claiming a specified ref.
-// The caller must call .Close() on the returned UnparsedImage.
-func dirImageMockWithRef(t *testing.T, dir string, ref types.ImageReference) types.UnparsedImage {
+// The caller must call the returned close callback when done.
+func dirImageMockWithRef(t *testing.T, dir string, ref types.ImageReference) (types.UnparsedImage, func() error) {
 	srcRef, err := directory.NewReference(dir)
 	require.NoError(t, err)
 	src, err := srcRef.NewImageSource(nil)
@@ -32,7 +32,7 @@ func dirImageMockWithRef(t *testing.T, dir string, ref types.ImageReference) typ
 	return image.UnparsedInstance(&dirImageSourceMock{
 		ImageSource: src,
 		ref:         ref,
-	}, nil)
+	}, nil), src.Close
 }
 
 // dirImageSourceMock inherits dirImageSource, but overrides its Reference method.
@@ -48,8 +48,8 @@ func (d *dirImageSourceMock) Reference() types.ImageReference {
 func TestPRSignedByIsSignatureAuthorAccepted(t *testing.T) {
 	ktGPG := SBKeyTypeGPGKeys
 	prm := NewPRMMatchExact()
-	testImage := dirImageMock(t, "fixtures/dir-img-valid", "testing/manifest:latest")
-	defer testImage.Close()
+	testImage, closer := dirImageMock(t, "fixtures/dir-img-valid", "testing/manifest:latest")
+	defer closer()
 	testImageSig, err := ioutil.ReadFile("fixtures/dir-img-valid/signature-1")
 	require.NoError(t, err)
 
@@ -153,8 +153,8 @@ func TestPRSignedByIsSignatureAuthorAccepted(t *testing.T) {
 	assertSARRejectedPolicyRequirement(t, sar, parsedSig, err)
 
 	// Error reading image manifest
-	image := dirImageMock(t, "fixtures/dir-img-no-manifest", "testing/manifest:latest")
-	defer image.Close()
+	image, closer := dirImageMock(t, "fixtures/dir-img-no-manifest", "testing/manifest:latest")
+	defer closer()
 	sig, err = ioutil.ReadFile("fixtures/dir-img-no-manifest/signature-1")
 	require.NoError(t, err)
 	pr, err = NewPRSignedByKeyPath(ktGPG, "fixtures/public-key.gpg", prm)
@@ -163,8 +163,8 @@ func TestPRSignedByIsSignatureAuthorAccepted(t *testing.T) {
 	assertSARRejected(t, sar, parsedSig, err)
 
 	// Error computing manifest digest
-	image = dirImageMock(t, "fixtures/dir-img-manifest-digest-error", "testing/manifest:latest")
-	defer image.Close()
+	image, closer = dirImageMock(t, "fixtures/dir-img-manifest-digest-error", "testing/manifest:latest")
+	defer closer()
 	sig, err = ioutil.ReadFile("fixtures/dir-img-manifest-digest-error/signature-1")
 	require.NoError(t, err)
 	pr, err = NewPRSignedByKeyPath(ktGPG, "fixtures/public-key.gpg", prm)
@@ -173,8 +173,8 @@ func TestPRSignedByIsSignatureAuthorAccepted(t *testing.T) {
 	assertSARRejected(t, sar, parsedSig, err)
 
 	// A valid signature with a non-matching manifest
-	image = dirImageMock(t, "fixtures/dir-img-modified-manifest", "testing/manifest:latest")
-	defer image.Close()
+	image, closer = dirImageMock(t, "fixtures/dir-img-modified-manifest", "testing/manifest:latest")
+	defer closer()
 	sig, err = ioutil.ReadFile("fixtures/dir-img-modified-manifest/signature-1")
 	require.NoError(t, err)
 	pr, err = NewPRSignedByKeyPath(ktGPG, "fixtures/public-key.gpg", prm)
@@ -205,8 +205,8 @@ func TestPRSignedByIsRunningImageAllowed(t *testing.T) {
 	prm := NewPRMMatchExact()
 
 	// A simple success case: single valid signature.
-	image := dirImageMock(t, "fixtures/dir-img-valid", "testing/manifest:latest")
-	defer image.Close()
+	image, closer := dirImageMock(t, "fixtures/dir-img-valid", "testing/manifest:latest")
+	defer closer()
 	pr, err := NewPRSignedByKeyPath(ktGPG, "fixtures/public-key.gpg", prm)
 	require.NoError(t, err)
 	allowed, err := pr.isRunningImageAllowed(image)
@@ -215,48 +215,48 @@ func TestPRSignedByIsRunningImageAllowed(t *testing.T) {
 	// Error reading signatures
 	invalidSigDir := createInvalidSigDir(t)
 	defer os.RemoveAll(invalidSigDir)
-	image = dirImageMock(t, invalidSigDir, "testing/manifest:latest")
-	defer image.Close()
+	image, closer = dirImageMock(t, invalidSigDir, "testing/manifest:latest")
+	defer closer()
 	pr, err = NewPRSignedByKeyPath(ktGPG, "fixtures/public-key.gpg", prm)
 	require.NoError(t, err)
 	allowed, err = pr.isRunningImageAllowed(image)
 	assertRunningRejected(t, allowed, err)
 
 	// No signatures
-	image = dirImageMock(t, "fixtures/dir-img-unsigned", "testing/manifest:latest")
-	defer image.Close()
+	image, closer = dirImageMock(t, "fixtures/dir-img-unsigned", "testing/manifest:latest")
+	defer closer()
 	pr, err = NewPRSignedByKeyPath(ktGPG, "fixtures/public-key.gpg", prm)
 	require.NoError(t, err)
 	allowed, err = pr.isRunningImageAllowed(image)
 	assertRunningRejectedPolicyRequirement(t, allowed, err)
 
 	// 1 invalid signature: use dir-img-valid, but a non-matching Docker reference
-	image = dirImageMock(t, "fixtures/dir-img-valid", "testing/manifest:notlatest")
-	defer image.Close()
+	image, closer = dirImageMock(t, "fixtures/dir-img-valid", "testing/manifest:notlatest")
+	defer closer()
 	pr, err = NewPRSignedByKeyPath(ktGPG, "fixtures/public-key.gpg", prm)
 	require.NoError(t, err)
 	allowed, err = pr.isRunningImageAllowed(image)
 	assertRunningRejectedPolicyRequirement(t, allowed, err)
 
 	// 2 valid signatures
-	image = dirImageMock(t, "fixtures/dir-img-valid-2", "testing/manifest:latest")
-	defer image.Close()
+	image, closer = dirImageMock(t, "fixtures/dir-img-valid-2", "testing/manifest:latest")
+	defer closer()
 	pr, err = NewPRSignedByKeyPath(ktGPG, "fixtures/public-key.gpg", prm)
 	require.NoError(t, err)
 	allowed, err = pr.isRunningImageAllowed(image)
 	assertRunningAllowed(t, allowed, err)
 
 	// One invalid, one valid signature (in this order)
-	image = dirImageMock(t, "fixtures/dir-img-mixed", "testing/manifest:latest")
-	defer image.Close()
+	image, closer = dirImageMock(t, "fixtures/dir-img-mixed", "testing/manifest:latest")
+	defer closer()
 	pr, err = NewPRSignedByKeyPath(ktGPG, "fixtures/public-key.gpg", prm)
 	require.NoError(t, err)
 	allowed, err = pr.isRunningImageAllowed(image)
 	assertRunningAllowed(t, allowed, err)
 
 	// 2 invalid signatures: use dir-img-valid-2, but a non-matching Docker reference
-	image = dirImageMock(t, "fixtures/dir-img-valid-2", "testing/manifest:notlatest")
-	defer image.Close()
+	image, closer = dirImageMock(t, "fixtures/dir-img-valid-2", "testing/manifest:notlatest")
+	defer closer()
 	pr, err = NewPRSignedByKeyPath(ktGPG, "fixtures/public-key.gpg", prm)
 	require.NoError(t, err)
 	allowed, err = pr.isRunningImageAllowed(image)

signature/policy_eval_simple_test.go

@@ -34,7 +34,7 @@ func (ref nameOnlyImageReferenceMock) PolicyConfigurationIdentity() string {
 func (ref nameOnlyImageReferenceMock) PolicyConfigurationNamespaces() []string {
 	panic("unexpected call to a mock function")
 }
-func (ref nameOnlyImageReferenceMock) NewImage(ctx *types.SystemContext) (types.Image, error) {
+func (ref nameOnlyImageReferenceMock) NewImage(ctx *types.SystemContext) (types.ImageCloser, error) {
 	panic("unexpected call to a mock function")
 }
 func (ref nameOnlyImageReferenceMock) NewImageSource(ctx *types.SystemContext) (types.ImageSource, error) {

signature/policy_eval_test.go

@@ -92,7 +92,7 @@ func (ref pcImageReferenceMock) PolicyConfigurationNamespaces() []string {
 	}
 	return policyconfiguration.DockerReferenceNamespaces(ref.ref)
 }
-func (ref pcImageReferenceMock) NewImage(ctx *types.SystemContext) (types.Image, error) {
+func (ref pcImageReferenceMock) NewImage(ctx *types.SystemContext) (types.ImageCloser, error) {
 	panic("unexpected call to a mock function")
 }
 func (ref pcImageReferenceMock) NewImageSource(ctx *types.SystemContext) (types.ImageSource, error) {
@@ -205,8 +205,8 @@ func TestPolicyContextRequirementsForImageRef(t *testing.T) {
 }
 
 // pcImageMock returns a types.UnparsedImage for a directory, claiming a specified dockerReference and implementing PolicyConfigurationIdentity/PolicyConfigurationNamespaces.
-// The caller must call .Close() on the returned Image.
-func pcImageMock(t *testing.T, dir, dockerReference string) types.UnparsedImage {
+// The caller must call the returned close callback when done.
+func pcImageMock(t *testing.T, dir, dockerReference string) (types.UnparsedImage, func() error) {
 	ref, err := reference.ParseNormalizedNamed(dockerReference)
 	require.NoError(t, err)
 	return dirImageMockWithRef(t, dir, pcImageReferenceMock{"docker", ref})
@@ -255,85 +255,85 @@ func TestPolicyContextGetSignaturesWithAcceptedAuthor(t *testing.T) {
 	defer pc.Destroy()
 
 	// Success
-	img := pcImageMock(t, "fixtures/dir-img-valid", "testing/manifest:latest")
-	defer img.Close()
+	img, closer := pcImageMock(t, "fixtures/dir-img-valid", "testing/manifest:latest")
+	defer closer()
 	sigs, err := pc.GetSignaturesWithAcceptedAuthor(img)
 	require.NoError(t, err)
 	assert.Equal(t, []*Signature{expectedSig}, sigs)
 
 	// Two signatures
 	// FIXME? Use really different signatures for this?
-	img = pcImageMock(t, "fixtures/dir-img-valid-2", "testing/manifest:latest")
-	defer img.Close()
+	img, closer = pcImageMock(t, "fixtures/dir-img-valid-2", "testing/manifest:latest")
+	defer closer()
 	sigs, err = pc.GetSignaturesWithAcceptedAuthor(img)
 	require.NoError(t, err)
 	assert.Equal(t, []*Signature{expectedSig, expectedSig}, sigs)
 
 	// No signatures
-	img = pcImageMock(t, "fixtures/dir-img-unsigned", "testing/manifest:latest")
-	defer img.Close()
+	img, closer = pcImageMock(t, "fixtures/dir-img-unsigned", "testing/manifest:latest")
+	defer closer()
 	sigs, err = pc.GetSignaturesWithAcceptedAuthor(img)
 	require.NoError(t, err)
 	assert.Empty(t, sigs)
 
 	// Only invalid signatures
-	img = pcImageMock(t, "fixtures/dir-img-modified-manifest", "testing/manifest:latest")
-	defer img.Close()
+	img, closer = pcImageMock(t, "fixtures/dir-img-modified-manifest", "testing/manifest:latest")
+	defer closer()
 	sigs, err = pc.GetSignaturesWithAcceptedAuthor(img)
 	require.NoError(t, err)
 	assert.Empty(t, sigs)
 
 	// 1 invalid, 1 valid signature (in this order)
-	img = pcImageMock(t, "fixtures/dir-img-mixed", "testing/manifest:latest")
-	defer img.Close()
+	img, closer = pcImageMock(t, "fixtures/dir-img-mixed", "testing/manifest:latest")
+	defer closer()
 	sigs, err = pc.GetSignaturesWithAcceptedAuthor(img)
 	require.NoError(t, err)
 	assert.Equal(t, []*Signature{expectedSig}, sigs)
 
 	// Two sarAccepted results for one signature
-	img = pcImageMock(t, "fixtures/dir-img-valid", "testing/manifest:twoAccepts")
-	defer img.Close()
+	img, closer = pcImageMock(t, "fixtures/dir-img-valid", "testing/manifest:twoAccepts")
+	defer closer()
 	sigs, err = pc.GetSignaturesWithAcceptedAuthor(img)
 	require.NoError(t, err)
 	assert.Equal(t, []*Signature{expectedSig}, sigs)
 
 	// sarAccepted+sarRejected for a signature
-	img = pcImageMock(t, "fixtures/dir-img-valid", "testing/manifest:acceptReject")
-	defer img.Close()
+	img, closer = pcImageMock(t, "fixtures/dir-img-valid", "testing/manifest:acceptReject")
+	defer closer()
 	sigs, err = pc.GetSignaturesWithAcceptedAuthor(img)
 	require.NoError(t, err)
 	assert.Empty(t, sigs)
 
 	// sarAccepted+sarUnknown for a signature
-	img = pcImageMock(t, "fixtures/dir-img-valid", "testing/manifest:acceptUnknown")
-	defer img.Close()
+	img, closer = pcImageMock(t, "fixtures/dir-img-valid", "testing/manifest:acceptUnknown")
+	defer closer()
 	sigs, err = pc.GetSignaturesWithAcceptedAuthor(img)
 	require.NoError(t, err)
 	assert.Equal(t, []*Signature{expectedSig}, sigs)
 
 	// sarRejected+sarUnknown for a signature
-	img = pcImageMock(t, "fixtures/dir-img-valid", "testing/manifest:rejectUnknown")
-	defer img.Close()
+	img, closer = pcImageMock(t, "fixtures/dir-img-valid", "testing/manifest:rejectUnknown")
+	defer closer()
 	sigs, err = pc.GetSignaturesWithAcceptedAuthor(img)
 	require.NoError(t, err)
 	assert.Empty(t, sigs)
 
 	// sarUnknown only
-	img = pcImageMock(t, "fixtures/dir-img-valid", "testing/manifest:unknown")
-	defer img.Close()
+	img, closer = pcImageMock(t, "fixtures/dir-img-valid", "testing/manifest:unknown")
+	defer closer()
 	sigs, err = pc.GetSignaturesWithAcceptedAuthor(img)
 	require.NoError(t, err)
 	assert.Empty(t, sigs)
 
-	img = pcImageMock(t, "fixtures/dir-img-valid", "testing/manifest:unknown2")
-	defer img.Close()
+	img, closer = pcImageMock(t, "fixtures/dir-img-valid", "testing/manifest:unknown2")
+	defer closer()
 	sigs, err = pc.GetSignaturesWithAcceptedAuthor(img)
 	require.NoError(t, err)
 	assert.Empty(t, sigs)
 
 	// Empty list of requirements (invalid)
-	img = pcImageMock(t, "fixtures/dir-img-valid", "testing/manifest:invalidEmptyRequirements")
-	defer img.Close()
+	img, closer = pcImageMock(t, "fixtures/dir-img-valid", "testing/manifest:invalidEmptyRequirements")
+	defer closer()
 	sigs, err = pc.GetSignaturesWithAcceptedAuthor(img)
 	require.NoError(t, err)
 	assert.Empty(t, sigs)
@@ -345,8 +345,8 @@ func TestPolicyContextGetSignaturesWithAcceptedAuthor(t *testing.T) {
 	require.NoError(t, err)
 	err = destroyedPC.Destroy()
 	require.NoError(t, err)
-	img = pcImageMock(t, "fixtures/dir-img-valid", "testing/manifest:latest")
-	defer img.Close()
+	img, closer = pcImageMock(t, "fixtures/dir-img-valid", "testing/manifest:latest")
+	defer closer()
 	sigs, err = destroyedPC.GetSignaturesWithAcceptedAuthor(img)
 	assert.Error(t, err)
 	assert.Nil(t, sigs)
@@ -357,8 +357,8 @@ func TestPolicyContextGetSignaturesWithAcceptedAuthor(t *testing.T) {
 	// Error reading signatures.
 	invalidSigDir := createInvalidSigDir(t)
 	defer os.RemoveAll(invalidSigDir)
-	img = pcImageMock(t, invalidSigDir, "testing/manifest:latest")
-	defer img.Close()
+	img, closer = pcImageMock(t, invalidSigDir, "testing/manifest:latest")
+	defer closer()
 	sigs, err = pc.GetSignaturesWithAcceptedAuthor(img)
 	assert.Error(t, err)
 	assert.Nil(t, sigs)
@@ -394,63 +394,63 @@ func TestPolicyContextIsRunningImageAllowed(t *testing.T) {
 	defer pc.Destroy()
 
 	// Success
-	img := pcImageMock(t, "fixtures/dir-img-valid", "testing/manifest:latest")
-	defer img.Close()
+	img, closer := pcImageMock(t, "fixtures/dir-img-valid", "testing/manifest:latest")
+	defer closer()
 	res, err := pc.IsRunningImageAllowed(img)
 	assertRunningAllowed(t, res, err)
 
 	// Two signatures
 	// FIXME? Use really different signatures for this?
-	img = pcImageMock(t, "fixtures/dir-img-valid-2", "testing/manifest:latest")
-	defer img.Close()
+	img, closer = pcImageMock(t, "fixtures/dir-img-valid-2", "testing/manifest:latest")
+	defer closer()
 	res, err = pc.IsRunningImageAllowed(img)
 	assertRunningAllowed(t, res, err)
 
 	// No signatures
-	img = pcImageMock(t, "fixtures/dir-img-unsigned", "testing/manifest:latest")
-	defer img.Close()
+	img, closer = pcImageMock(t, "fixtures/dir-img-unsigned", "testing/manifest:latest")
+	defer closer()
 	res, err = pc.IsRunningImageAllowed(img)
 	assertRunningRejectedPolicyRequirement(t, res, err)
 
 	// Only invalid signatures
-	img = pcImageMock(t, "fixtures/dir-img-modified-manifest", "testing/manifest:latest")
-	defer img.Close()
+	img, closer = pcImageMock(t, "fixtures/dir-img-modified-manifest", "testing/manifest:latest")
+	defer closer()
 	res, err = pc.IsRunningImageAllowed(img)
 	assertRunningRejectedPolicyRequirement(t, res, err)
 
 	// 1 invalid, 1 valid signature (in this order)
-	img = pcImageMock(t, "fixtures/dir-img-mixed", "testing/manifest:latest")
-	defer img.Close()
+	img, closer = pcImageMock(t, "fixtures/dir-img-mixed", "testing/manifest:latest")
+	defer closer()
 	res, err = pc.IsRunningImageAllowed(img)
 	assertRunningAllowed(t, res, err)
 
 	// Two allowed results
-	img = pcImageMock(t, "fixtures/dir-img-mixed", "testing/manifest:twoAllows")
-	defer img.Close()
+	img, closer = pcImageMock(t, "fixtures/dir-img-mixed", "testing/manifest:twoAllows")
+	defer closer()
 	res, err = pc.IsRunningImageAllowed(img)
 	assertRunningAllowed(t, res, err)
 
 	// Allow + deny results
-	img = pcImageMock(t, "fixtures/dir-img-mixed", "testing/manifest:allowDeny")
-	defer img.Close()
+	img, closer = pcImageMock(t, "fixtures/dir-img-mixed", "testing/manifest:allowDeny")
+	defer closer()
 	res, err = pc.IsRunningImageAllowed(img)
 	assertRunningRejectedPolicyRequirement(t, res, err)
 
 	// prReject works
-	img = pcImageMock(t, "fixtures/dir-img-mixed", "testing/manifest:reject")
-	defer img.Close()
+	img, closer = pcImageMock(t, "fixtures/dir-img-mixed", "testing/manifest:reject")
+	defer closer()
 	res, err = pc.IsRunningImageAllowed(img)
 	assertRunningRejectedPolicyRequirement(t, res, err)
 
 	// prInsecureAcceptAnything works
-	img = pcImageMock(t, "fixtures/dir-img-mixed", "testing/manifest:acceptAnything")
-	defer img.Close()
+	img, closer = pcImageMock(t, "fixtures/dir-img-mixed", "testing/manifest:acceptAnything")
+	defer closer()
 	res, err = pc.IsRunningImageAllowed(img)
 	assertRunningAllowed(t, res, err)
 
 	// Empty list of requirements (invalid)
-	img = pcImageMock(t, "fixtures/dir-img-valid", "testing/manifest:invalidEmptyRequirements")
-	defer img.Close()
+	img, closer = pcImageMock(t, "fixtures/dir-img-valid", "testing/manifest:invalidEmptyRequirements")
+	defer closer()
 	res, err = pc.IsRunningImageAllowed(img)
 	assertRunningRejectedPolicyRequirement(t, res, err)
 
@@ -459,8 +459,8 @@ func TestPolicyContextIsRunningImageAllowed(t *testing.T) {
 	require.NoError(t, err)
 	err = destroyedPC.Destroy()
 	require.NoError(t, err)
-	img = pcImageMock(t, "fixtures/dir-img-valid", "testing/manifest:latest")
-	defer img.Close()
+	img, closer = pcImageMock(t, "fixtures/dir-img-valid", "testing/manifest:latest")
+	defer closer()
 	res, err = destroyedPC.IsRunningImageAllowed(img)
 	assertRunningRejected(t, res, err)
 	// Not testing the pcInUse->pcReady transition, that would require custom PolicyRequirement

signature/policy_reference_match_test.go

@@ -94,7 +94,7 @@ func (ref refImageReferenceMock) PolicyConfigurationIdentity() string {
 func (ref refImageReferenceMock) PolicyConfigurationNamespaces() []string {
 	panic("unexpected call to a mock function")
 }
-func (ref refImageReferenceMock) NewImage(ctx *types.SystemContext) (types.Image, error) {
+func (ref refImageReferenceMock) NewImage(ctx *types.SystemContext) (types.ImageCloser, error) {
 	panic("unexpected call to a mock function")
 }
 func (ref refImageReferenceMock) NewImageSource(ctx *types.SystemContext) (types.ImageSource, error) {

storage/storage_image.go

@@ -67,8 +67,8 @@ type storageLayerMetadata struct {
 	CompressedSize int64  `json:"compressed-size,omitempty"`
 }
 
-type storageImage struct {
-	types.Image
+type storageImageCloser struct {
+	types.ImageCloser
 	size int64
 }
 
@@ -606,12 +606,12 @@ func (s *storageImageSource) getSize() (int64, error) {
 	return sum, nil
 }
 
-func (s *storageImage) Size() (int64, error) {
+func (s *storageImageCloser) Size() (int64, error) {
 	return s.size, nil
 }
 
-// newImage creates an image that also knows its size
-func newImage(s storageReference) (types.Image, error) {
+// newImage creates an ImageCloser that also knows its size
+func newImage(s storageReference) (types.ImageCloser, error) {
 	src, err := newImageSource(s)
 	if err != nil {
 		return nil, err
@@ -624,5 +624,5 @@ func newImage(s storageReference) (types.Image, error) {
 	if err != nil {
 		return nil, err
 	}
-	return &storageImage{Image: img, size: size}, nil
+	return &storageImageCloser{ImageCloser: img, size: size}, nil
 }

storage/storage_reference.go

@@ -137,7 +137,12 @@ func (s storageReference) PolicyConfigurationNamespaces() []string {
 	return namespaces
 }
 
-func (s storageReference) NewImage(ctx *types.SystemContext) (types.Image, error) {
+// NewImage returns a types.ImageCloser for this reference, possibly specialized for this ImageTransport.
+// The caller must call .Close() on the returned ImageCloser.
+// NOTE: If any kind of signature verification should happen, build an UnparsedImage from the value returned by NewImageSource,
+// verify that UnparsedImage, and convert it into a real Image via image.FromUnparsedImage.
+// WARNING: This may not do the right thing for a manifest list, see image.FromSource for details.
+func (s storageReference) NewImage(ctx *types.SystemContext) (types.ImageCloser, error) {
 	return newImage(s)
 }
 

tarball/tarball_reference.go

@@ -61,7 +61,12 @@ func (r *tarballReference) PolicyConfigurationNamespaces() []string {
 	return nil
 }
 
-func (r *tarballReference) NewImage(ctx *types.SystemContext) (types.Image, error) {
+// NewImage returns a types.ImageCloser for this reference, possibly specialized for this ImageTransport.
+// The caller must call .Close() on the returned ImageCloser.
+// NOTE: If any kind of signature verification should happen, build an UnparsedImage from the value returned by NewImageSource,
+// verify that UnparsedImage, and convert it into a real Image via image.FromUnparsedImage.
+// WARNING: This may not do the right thing for a manifest list, see image.FromSource for details.
+func (r *tarballReference) NewImage(ctx *types.SystemContext) (types.ImageCloser, error) {
 	src, err := r.NewImageSource(ctx)
 	if err != nil {
 		return nil, err

types/types.go

@@ -73,11 +73,12 @@ type ImageReference interface {
 	// and each following element to be a prefix of the element preceding it.
 	PolicyConfigurationNamespaces() []string
 
-	// NewImage returns a types.Image for this reference, possibly specialized for this ImageTransport.
-	// The caller must call .Close() on the returned Image.
+	// NewImage returns a types.ImageCloser for this reference, possibly specialized for this ImageTransport.
+	// The caller must call .Close() on the returned ImageCloser.
 	// NOTE: If any kind of signature verification should happen, build an UnparsedImage from the value returned by NewImageSource,
 	// verify that UnparsedImage, and convert it into a real Image via image.FromUnparsedImage.
-	NewImage(ctx *SystemContext) (Image, error)
+	// WARNING: This may not do the right thing for a manifest list, see image.FromSource for details.
+	NewImage(ctx *SystemContext) (ImageCloser, error)
 	// NewImageSource returns a types.ImageSource for this reference.
 	// The caller must call .Close() on the returned ImageSource.
 	NewImageSource(ctx *SystemContext) (ImageSource, error)
@@ -201,13 +202,11 @@ func (e ManifestTypeRejectedError) Error() string {
 //
 // An UnparsedImage is a pair of (ImageSource, instance digest); it can represent either a manifest list or a single image instance.
 //
-// Each UnparsedImage should eventually be closed by calling Close().
+// The UnparsedImage must not be used after the underlying ImageSource is Close()d.
 type UnparsedImage interface {
 	// Reference returns the reference used to set up this source, _as specified by the user_
 	// (not as the image itself, or its underlying storage, claims).  This can be used e.g. to determine which public keys are trusted for this image.
 	Reference() ImageReference
-	// Close removes resources associated with an initialized UnparsedImage, if any.
-	Close() error
 	// Manifest is like ImageSource.GetManifest, but the result is cached; it is OK to call this however often you need.
 	Manifest() ([]byte, string, error)
 	// Signatures is like ImageSource.GetSignatures, but the result is cached; it is OK to call this however often you need.
@@ -216,7 +215,8 @@ type UnparsedImage interface {
 
 // Image is the primary API for inspecting properties of images.
 // An Image is based on a pair of (ImageSource, instance digest); it can represent either a manifest list or a single image instance.
-// Each Image should eventually be closed by calling Close().
+//
+// The Image must not be used after the underlying ImageSource is Close()d.
 type Image interface {
 	// Note that Reference may return nil in the return value of UpdatedImage!
 	UnparsedImage
@@ -253,6 +253,15 @@ type Image interface {
 	Size() (int64, error)
 }
 
+// ImageCloser is an Image with a Close() method which must be called by the user.
+// This is returned by ImageReference.NewImage, which transparently instantiates a types.ImageSource,
+// to ensure that the ImageSource is closed.
+type ImageCloser interface {
+	Image
+	// Close removes resources associated with an initialized ImageCloser.
+	Close() error
+}
+
 // ManifestUpdateOptions is a way to pass named optional arguments to Image.UpdatedManifest
 type ManifestUpdateOptions struct {
 	LayerInfos              []BlobInfo // Complete BlobInfos (size+digest+urls+annotations) which should replace the originals, in order (the root layer first, and then successive layered layers). BlobInfos' MediaType fields are ignored.