Merge pull request #44426 from thaJeztah/deprecate_image_IDFromDigest

image: deprecate IDFromDigest(), and some minor fixes/cleanup

daemon/images/image.go

@@ -49,7 +49,7 @@ type manifest struct {
 func (i *ImageService) manifestMatchesPlatform(ctx context.Context, img *image.Image, platform specs.Platform) (bool, error) {
 	logger := logrus.WithField("image", img.ID).WithField("desiredPlatform", platforms.Format(platform))
 
-	ls, leaseErr := i.leases.ListResources(ctx, leases.Lease{ID: imageKey(img.ID().Digest())})
+	ls, leaseErr := i.leases.ListResources(ctx, leases.Lease{ID: imageKey(img.ID().String())})
 	if leaseErr != nil {
 		logger.WithError(leaseErr).Error("Error looking up image leases")
 		return false, leaseErr
@@ -230,17 +230,15 @@ func (i *ImageService) getImage(ctx context.Context, refOrID string, options ima
 		if !ok {
 			return nil, ErrImageDoesNotExist{ref}
 		}
-		id := image.IDFromDigest(digested.Digest())
-		if img, err := i.imageStore.Get(id); err == nil {
+		if img, err := i.imageStore.Get(image.ID(digested.Digest())); err == nil {
 			return img, nil
 		}
 		return nil, ErrImageDoesNotExist{ref}
 	}
 
-	if digest, err := i.referenceStore.Get(namedRef); err == nil {
+	if dgst, err := i.referenceStore.Get(namedRef); err == nil {
 		// Search the image stores to get the operating system, defaulting to host OS.
-		id := image.IDFromDigest(digest)
-		if img, err := i.imageStore.Get(id); err == nil {
+		if img, err := i.imageStore.Get(image.ID(dgst)); err == nil {
 			return img, nil
 		}
 	}

daemon/images/image_list.go

@@ -2,6 +2,7 @@ package images // import "github.com/docker/docker/daemon/images"
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"sort"
 	"time"
@@ -132,7 +133,7 @@ func (i *ImageService) Images(ctx context.Context, opts types.ImageListOptions)
 			if err != nil {
 				// The layer may have been deleted between the call to `Map()` or
 				// `Heads()` and the call to `Get()`, so we just ignore this error
-				if err == layer.ErrLayerDoesNotExist {
+				if errors.Is(err, layer.ErrLayerDoesNotExist) {
 					continue
 				}
 				return nil, err

daemon/images/store.go

@@ -17,8 +17,10 @@ import (
 	"github.com/sirupsen/logrus"
 )
 
-func imageKey(dgst digest.Digest) string {
-	return "moby-image-" + dgst.String()
+const imageKeyPrefix = "moby-image-"
+
+func imageKey(dgst string) string {
+	return imageKeyPrefix + dgst
 }
 
 // imageStoreWithLease wraps the configured image store with one that deletes the lease
@@ -36,7 +38,7 @@ type imageStoreWithLease struct {
 
 func (s *imageStoreWithLease) Delete(id image.ID) ([]layer.Metadata, error) {
 	ctx := namespaces.WithNamespace(context.TODO(), s.ns)
-	if err := s.leases.Delete(ctx, leases.Lease{ID: imageKey(digest.Digest(id))}); err != nil && !c8derrdefs.IsNotFound(err) {
+	if err := s.leases.Delete(ctx, leases.Lease{ID: imageKey(id.String())}); err != nil && !c8derrdefs.IsNotFound(err) {
 		return nil, errors.Wrap(err, "error deleting lease")
 	}
 	return s.Store.Delete(id)
@@ -67,7 +69,7 @@ func (s *imageStoreForPull) Get(ctx context.Context, dgst digest.Digest) ([]byte
 }
 
 func (s *imageStoreForPull) updateLease(ctx context.Context, dgst digest.Digest) error {
-	leaseID := imageKey(dgst)
+	leaseID := imageKey(dgst.String())
 	lease, err := s.leases.Create(ctx, leases.WithID(leaseID))
 	if err != nil {
 		if !c8derrdefs.IsAlreadyExists(err) {

daemon/images/store_test.go

@@ -68,7 +68,7 @@ func TestImageDelete(t *testing.T) {
 		assert.NilError(t, err)
 		defer images.Delete(id)
 
-		leaseID := imageKey(digest.Digest(id))
+		leaseID := imageKey(id.String())
 		_, err = images.leases.Create(ctx, leases.WithID(leaseID))
 		assert.NilError(t, err)
 		defer images.leases.Delete(ctx, leases.Lease{ID: leaseID})

daemon/list_test.go

@@ -37,7 +37,7 @@ func setupContainerWithName(t *testing.T, name string, daemon *Daemon) *containe
 	t.Helper()
 	var (
 		id              = uuid.New().String()
-		computedImageID = digest.FromString(id)
+		computedImageID = image.ID(digest.FromString(id))
 		cRoot           = filepath.Join(root, id)
 	)
 	if err := os.MkdirAll(cRoot, 0755); err != nil {
@@ -54,7 +54,7 @@ func setupContainerWithName(t *testing.T, name string, daemon *Daemon) *containe
 	c.HostConfig = &containertypes.HostConfig{}
 
 	// these are for passing the refreshImage reducer
-	c.ImageID = image.IDFromDigest(computedImageID)
+	c.ImageID = computedImageID
 	c.Config = &containertypes.Config{
 		Image: computedImageID.String(),
 	}

distribution/config.go

@@ -118,7 +118,7 @@ func (s *imageConfigStore) Put(_ context.Context, c []byte) (digest.Digest, erro
 }
 
 func (s *imageConfigStore) Get(_ context.Context, d digest.Digest) ([]byte, error) {
-	img, err := s.Store.Get(image.IDFromDigest(d))
+	img, err := s.Store.Get(image.ID(d))
 	if err != nil {
 		return nil, err
 	}

image/image.go

@@ -28,6 +28,8 @@ func (id ID) Digest() digest.Digest {
 }
 
 // IDFromDigest creates an ID from a digest
+//
+// Deprecated: cast to an ID using ID(digest).
 func IDFromDigest(digest digest.Digest) ID {
 	return ID(digest)
 }

image/store.go

@@ -67,22 +67,29 @@ func NewImageStore(fs StoreBackend, lss LayerGetReleaser) (Store, error) {
 }
 
 func (is *store) restore() error {
+	// As the code below is run when restoring all images (which can be "many"),
+	// constructing the "logrus.WithFields" is deliberately not "DRY", as the
+	// logger is only used for error-cases, and we don't want to do allocations
+	// if we don't need it. The "f" type alias is here is just for convenience,
+	// and to make the code _slightly_ more DRY. See the discussion on GitHub;
+	// https://github.com/moby/moby/pull/44426#discussion_r1059519071
+	type f = logrus.Fields
 	err := is.fs.Walk(func(dgst digest.Digest) error {
-		img, err := is.Get(IDFromDigest(dgst))
+		img, err := is.Get(ID(dgst))
 		if err != nil {
-			logrus.Errorf("invalid image %v, %v", dgst, err)
+			logrus.WithFields(f{"digest": dgst, "err": err}).Error("invalid image")
 			return nil
 		}
 		var l layer.Layer
 		if chainID := img.RootFS.ChainID(); chainID != "" {
 			if !system.IsOSSupported(img.OperatingSystem()) {
-				logrus.Errorf("not restoring image with unsupported operating system %v, %v, %s", dgst, chainID, img.OperatingSystem())
+				logrus.WithFields(f{"chainID": chainID, "os": img.OperatingSystem()}).Error("not restoring image with unsupported operating system")
 				return nil
 			}
 			l, err = is.lss.Get(chainID)
 			if err != nil {
-				if err == layer.ErrLayerDoesNotExist {
-					logrus.Errorf("layer does not exist, not restoring image %v, %v, %s", dgst, chainID, img.OperatingSystem())
+				if errors.Is(err, layer.ErrLayerDoesNotExist) {
+					logrus.WithFields(f{"chainID": chainID, "os": img.OperatingSystem(), "err": err}).Error("not restoring image")
 					return nil
 				}
 				return err
@@ -92,13 +99,11 @@ func (is *store) restore() error {
 			return err
 		}
 
-		imageMeta := &imageMeta{
+		is.images[ID(dgst)] = &imageMeta{
 			layer:    l,
 			children: make(map[ID]struct{}),
 		}
 
-		is.images[IDFromDigest(dgst)] = imageMeta
-
 		return nil
 	})
 	if err != nil {
@@ -141,15 +146,15 @@ func (is *store) Create(config []byte) (ID, error) {
 		return "", errdefs.InvalidParameter(errors.New("too many non-empty layers in History section"))
 	}
 
-	dgst, err := is.fs.Set(config)
+	imageDigest, err := is.fs.Set(config)
 	if err != nil {
 		return "", errdefs.InvalidParameter(err)
 	}
-	imageID := IDFromDigest(dgst)
 
 	is.Lock()
 	defer is.Unlock()
 
+	imageID := ID(imageDigest)
 	if _, exists := is.images[imageID]; exists {
 		return imageID, nil
 	}
@@ -167,13 +172,12 @@ func (is *store) Create(config []byte) (ID, error) {
 		}
 	}
 
-	imageMeta := &imageMeta{
+	is.images[imageID] = &imageMeta{
 		layer:    l,
 		children: make(map[ID]struct{}),
 	}
 
-	is.images[imageID] = imageMeta
-	if err := is.digestSet.Add(imageID.Digest()); err != nil {
+	if err = is.digestSet.Add(imageDigest); err != nil {
 		delete(is.images, imageID)
 		return "", errdefs.InvalidParameter(err)
 	}
@@ -197,7 +201,7 @@ func (is *store) Search(term string) (ID, error) {
 		}
 		return "", errors.WithStack(err)
 	}
-	return IDFromDigest(dgst), nil
+	return ID(dgst), nil
 }
 
 func (is *store) Get(id ID) (*Image, error) {
@@ -226,16 +230,16 @@ func (is *store) Delete(id ID) ([]layer.Metadata, error) {
 	is.Lock()
 	defer is.Unlock()
 
-	imageMeta := is.images[id]
-	if imageMeta == nil {
+	imgMeta := is.images[id]
+	if imgMeta == nil {
 		return nil, errdefs.NotFound(fmt.Errorf("unrecognized image ID %s", id.String()))
 	}
 	_, err := is.Get(id)
 	if err != nil {
 		return nil, errdefs.NotFound(fmt.Errorf("unrecognized image %s, %v", id.String(), err))
 	}
-	for id := range imageMeta.children {
-		is.fs.DeleteMetadata(id.Digest(), "parent")
+	for cID := range imgMeta.children {
+		is.fs.DeleteMetadata(cID.Digest(), "parent")
 	}
 	if parent, err := is.GetParent(id); err == nil && is.images[parent] != nil {
 		delete(is.images[parent].children, id)
@@ -247,24 +251,24 @@ func (is *store) Delete(id ID) ([]layer.Metadata, error) {
 	delete(is.images, id)
 	is.fs.Delete(id.Digest())
 
-	if imageMeta.layer != nil {
-		return is.lss.Release(imageMeta.layer)
+	if imgMeta.layer != nil {
+		return is.lss.Release(imgMeta.layer)
 	}
 	return nil, nil
 }
 
-func (is *store) SetParent(id, parent ID) error {
+func (is *store) SetParent(id, parentID ID) error {
 	is.Lock()
 	defer is.Unlock()
-	parentMeta := is.images[parent]
+	parentMeta := is.images[parentID]
 	if parentMeta == nil {
-		return errdefs.NotFound(fmt.Errorf("unknown parent image ID %s", parent.String()))
+		return errdefs.NotFound(fmt.Errorf("unknown parent image ID %s", parentID.String()))
 	}
 	if parent, err := is.GetParent(id); err == nil && is.images[parent] != nil {
 		delete(is.images[parent].children, id)
 	}
 	parentMeta.children[id] = struct{}{}
-	return is.fs.SetMetadata(id.Digest(), "parent", []byte(parent))
+	return is.fs.SetMetadata(id.Digest(), "parent", []byte(parentID))
 }
 
 func (is *store) GetParent(id ID) (ID, error) {

image/tarexport/save.go

@@ -94,8 +94,7 @@ func (l *tarexporter) parseNames(names []string) (desc map[image.ID]*imageDescri
 		if !ok {
 			// Check if digest ID reference
 			if digested, ok := ref.(reference.Digested); ok {
-				id := image.IDFromDigest(digested.Digest())
-				if err := addAssoc(id, nil); err != nil {
+				if err := addAssoc(image.ID(digested.Digest()), nil); err != nil {
 					return nil, err
 				}
 				continue
@@ -116,7 +115,7 @@ func (l *tarexporter) parseNames(names []string) (desc map[image.ID]*imageDescri
 		if reference.IsNameOnly(namedRef) {
 			assocs := l.rs.ReferencesByName(namedRef)
 			for _, assoc := range assocs {
-				if err := addAssoc(image.IDFromDigest(assoc.ID), assoc.Ref); err != nil {
+				if err := addAssoc(image.ID(assoc.ID), assoc.Ref); err != nil {
 					return nil, err
 				}
 			}
@@ -135,7 +134,7 @@ func (l *tarexporter) parseNames(names []string) (desc map[image.ID]*imageDescri
 		if err != nil {
 			return nil, err
 		}
-		if err := addAssoc(image.IDFromDigest(id), namedRef); err != nil {
+		if err := addAssoc(image.ID(id), namedRef); err != nil {
 			return nil, err
 		}
 	}

layer/layer_test.go

@@ -2,6 +2,7 @@ package layer // import "github.com/docker/docker/layer"
 
 import (
 	"bytes"
+	"errors"
 	"io"
 	"os"
 	"path/filepath"
@@ -397,7 +398,7 @@ func TestStoreRestore(t *testing.T) {
 	// Create again with same name, should return error
 	if _, err := ls2.CreateRWLayer("some-mount_name", layer3b.ChainID(), nil); err == nil {
 		t.Fatal("Expected error creating mount with same name")
-	} else if err != ErrMountNameConflict {
+	} else if !errors.Is(err, ErrMountNameConflict) {
 		t.Fatal(err)
 	}