Merge pull request #1351 from fuweid/feature_allow_use_image_by_digest_id

feature: allow use image by digest id

Author: YaoZengzeng

cri/src/cri.go

@@ -9,7 +9,6 @@ import (
 	"path"
 	"path/filepath"
 	"reflect"
-	"strings"
 	"time"
 
 	apitypes "github.com/alibaba/pouch/apis/types"
@@ -649,7 +648,7 @@ func (c *CriManager) ContainerStatus(ctx context.Context, r *runtime.ContainerSt
 	labels, annotations := extractLabels(container.Config.Labels)
 
 	imageRef := container.Image
-	imageInfo, err := c.ImageMgr.GetImage(ctx, strings.TrimPrefix(imageRef, "sha256:"))
+	imageInfo, err := c.ImageMgr.GetImage(ctx, imageRef)
 	if err != nil {
 		return nil, fmt.Errorf("failed to get image %s: %v", imageRef, err)
 	}
@@ -826,7 +825,7 @@ func (c *CriManager) ListImages(ctx context.Context, r *runtime.ListImagesReques
 			continue
 		}
 		// NOTE: we should query image cache to get the correct image info.
-		imageInfo, err := c.ImageMgr.GetImage(ctx, strings.TrimPrefix(i.ID, "sha256:"))
+		imageInfo, err := c.ImageMgr.GetImage(ctx, i.ID)
 		if err != nil {
 			continue
 		}
@@ -850,7 +849,7 @@ func (c *CriManager) ImageStatus(ctx context.Context, r *runtime.ImageStatusRequ
 		return nil, err
 	}
 
-	imageInfo, err := c.ImageMgr.GetImage(ctx, strings.TrimPrefix(ref.String(), "sha256:"))
+	imageInfo, err := c.ImageMgr.GetImage(ctx, ref.String())
 	if err != nil {
 		// TODO: separate ErrImageNotFound with others.
 		// Now we just return empty if the error occurred.
@@ -894,7 +893,7 @@ func (c *CriManager) PullImage(ctx context.Context, r *runtime.PullImageRequest)
 
 // RemoveImage removes the image.
 func (c *CriManager) RemoveImage(ctx context.Context, r *runtime.RemoveImageRequest) (*runtime.RemoveImageResponse, error) {
-	imageRef := strings.TrimPrefix(r.GetImage().GetImage(), "sha256:")
+	imageRef := r.GetImage().GetImage()
 
 	if err := c.ImageMgr.RemoveImage(ctx, imageRef, false); err != nil {
 		if errtypes.IsNotfound(err) {

daemon/mgr/container.go

@@ -36,7 +36,6 @@ import (
 	"github.com/go-openapi/strfmt"
 	"github.com/imdario/mergo"
 	"github.com/magiconair/properties"
-	digest "github.com/opencontainers/go-digest"
 	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
@@ -1919,7 +1918,7 @@ func (mgr *ContainerManager) getMountPointFromImage(ctx context.Context, meta *C
 	var err error
 
 	// parse volumes from image
-	image, err := mgr.ImageMgr.GetImage(ctx, strings.TrimPrefix(meta.Image, digest.Canonical.String()+":"))
+	image, err := mgr.ImageMgr.GetImage(ctx, meta.Image)
 	if err != nil {
 		return errors.Wrapf(err, "failed to get image: %s", meta.Image)
 	}

daemon/mgr/image.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"io"
+	"strings"
 	"time"
 
 	"github.com/alibaba/pouch/apis/types"
@@ -178,8 +179,11 @@ func (mgr *ImageManager) RemoveImage(ctx context.Context, idOrRef string, force
 		return err
 	}
 
-	// should remove all the references if the reference is Named Only
-	if reference.IsNamedOnly(namedRef) {
+	// should remove all the references if the reference is ID (Named Only)
+	// or Digest ID (Tagged Named)
+	if reference.IsNamedOnly(namedRef) ||
+		strings.HasPrefix(id.String(), namedRef.String()) {
+
 		// NOTE: the user maybe use the following references to pull one image
 		//
 		//	busybox:1.25
@@ -257,9 +261,11 @@ func (mgr *ImageManager) CheckReference(ctx context.Context, idOrRef string) (ac
 		}
 	}
 
-	// NOTE: if the actualRef is short ID or ID, the primaryRef is first one of
-	// primary reference
-	if reference.IsNamedOnly(actualRef) {
+	// NOTE: if the actualRef is ID (Named Only) or Digest ID (Tagged Named)
+	// the primaryRef is first one of primary reference
+	if reference.IsNamedOnly(actualRef) ||
+		strings.HasPrefix(actualID.String(), actualRef.String()) {
+
 		refs := mgr.localStore.GetPrimaryReferences(actualID)
 		if len(refs) == 0 {
 			err = errtypes.ErrNotfound

daemon/mgr/image_store.go

@@ -1,6 +1,8 @@
 package mgr
 
 import (
+	"fmt"
+	"strings"
 	"sync"
 
 	"github.com/alibaba/pouch/pkg/errtypes"
@@ -141,34 +143,41 @@ func (store *imageStore) Search(ref reference.Named) (digest.Digest, reference.N
 
 	// if the reference is short ID or ID
 	//
-	// NOTE: by default, use the sha256 as the digest algorithm
-	id, err := store.searchIDs(digest.Canonical.String(), ref.String())
+	// NOTE: by default, use the sha256 as the digest algorithm if missing
+	// algorithm header.
+	id, err := store.searchIDs(ref.String())
 	if err != nil {
 		return "", nil, err
 	}
 	return id, ref, nil
 }
 
-func (store *imageStore) searchIDs(algo string, prefixID string) (digest.Digest, error) {
+func (store *imageStore) searchIDs(refID string) (digest.Digest, error) {
 	var ids []digest.Digest
+	var id string
+
+	id = refID
+	if !strings.HasPrefix(refID, digest.Canonical.String()) {
+		id = fmt.Sprintf("%s:%s", digest.Canonical.String(), refID)
+	}
 
 	fn := func(_ patricia.Prefix, item patricia.Item) error {
 		if got, ok := item.(digest.Digest); ok {
 			ids = append(ids, got)
 		}
 
 		if len(ids) > 1 {
-			return pkgerrors.Wrap(errtypes.ErrTooMany, "image: "+prefixID)
+			return pkgerrors.Wrap(errtypes.ErrTooMany, "image: "+refID)
 		}
 		return nil
 	}
 
-	if err := store.idSet.VisitSubtree(patricia.Prefix(algo+":"+prefixID), fn); err != nil {
+	if err := store.idSet.VisitSubtree(patricia.Prefix(id), fn); err != nil {
 		return "", err
 	}
 
 	if len(ids) == 0 {
-		return "", pkgerrors.Wrap(errtypes.ErrNotfound, "image: "+prefixID)
+		return "", pkgerrors.Wrap(errtypes.ErrNotfound, "image: "+refID)
 	}
 	return ids[0], nil
 }

daemon/mgr/image_store_test.go

@@ -112,7 +112,7 @@ func TestSearch(t *testing.T) {
 
 	// search
 	{
-		// should return id if the reference is id
+		// should return id if the reference is id without algorithm header
 		{
 			namedStr := id.Hex()
 
@@ -127,6 +127,21 @@ func TestSearch(t *testing.T) {
 			assert.Equal(t, gotRef.String(), namedRef.String())
 		}
 
+		// should return id if the reference is digest id
+		{
+			namedStr := id.String()
+
+			namedRef, err := reference.Parse(namedStr)
+			if err != nil {
+				t.Fatalf("unexpected error during parse reference %v: %v", namedStr, err)
+			}
+
+			gotID, gotRef, err := store.Search(namedRef)
+			assert.Equal(t, err, nil)
+			assert.Equal(t, gotID.String(), id.String())
+			assert.Equal(t, gotRef.String(), namedRef.String())
+		}
+
 		// should return busybox:latest if the reference is busybox
 		{
 			namedStr := "busybox"
@@ -202,7 +217,7 @@ func TestSearch(t *testing.T) {
 
 		// should return ErrTooMany if the reference is commonPart
 		{
-			namedStr := id.Hex()[:20]
+			namedStr := id.String()[:20]
 
 			namedRef, err := reference.Parse(namedStr)
 			if err != nil {

pkg/reference/parse_test.go

@@ -43,9 +43,6 @@ func TestDefaultTagIfMissing(t *testing.T) {
 	assert.Equal(t, false, strings.Contains(named.String(), "latest"))
 }
 
-func TestIsNamedOnly(t *testing.T) {
-}
-
 func TestParse(t *testing.T) {
 	type tCase struct {
 		name     string
@@ -121,6 +118,14 @@ func TestParse(t *testing.T) {
 			input:    "busybox@sha256:1669a6aa7350e1cdd28f972ddad5aceba2912f589f19a090ac",
 			expected: nil,
 			err:      errors.New("invalid checksum digest length"),
+		}, {
+			name:  "Digest ID",
+			input: "sha256:1669a6aa7350e1cdd28f972ddad5aceba2912f589f19a090ac",
+			expected: taggedReference{
+				Named: namedReference{"sha256"},
+				tag:   "1669a6aa7350e1cdd28f972ddad5aceba2912f589f19a090ac",
+			},
+			err: nil,
 		},
 	} {
 		ref, err := Parse(tc.input)

test/api_image_inspect_test.go

@@ -26,9 +26,11 @@ func (suite *APIImageInspectSuite) SetUpTest(c *check.C) {
 
 // TestImageInspectOk tests inspecting images is OK.
 func (suite *APIImageInspectSuite) TestImageInspectOk(c *check.C) {
+	repoID := environment.BusyboxID
 	repoTag, repoDigest := busyboxImage, fmt.Sprintf("%s@%s", environment.BusyboxRepo, environment.BusyboxDigest)
 
 	for _, image := range []string{
+		repoID,
 		repoTag,
 		repoDigest,
 		fmt.Sprintf("%s:whatever@%s", environment.BusyboxRepo, environment.BusyboxDigest),
@@ -43,7 +45,7 @@ func (suite *APIImageInspectSuite) TestImageInspectOk(c *check.C) {
 
 		// TODO: More specific check is needed
 		c.Assert(got.Config, check.NotNil)
-		c.Assert(got.ID, check.NotNil)
+		c.Assert(got.ID, check.Equals, repoID)
 		c.Assert(got.CreatedAt, check.NotNil)
 		c.Assert(got.Size, check.NotNil)
 		c.Assert(reflect.DeepEqual(got.RepoTags, []string{repoTag}), check.Equals, true)

test/cli_rmi_test.go

@@ -64,6 +64,22 @@ func (suite *PouchRmiSuite) TestRmiByImageID(c *check.C) {
 	}
 }
 
+// TestRmiByImageDigestID tests "pouch rmi sha256:xxx" work.
+func (suite *PouchRmiSuite) TestRmiByImageDigestID(c *check.C) {
+	command.PouchRun("pull", helloworldImage).Assert(c, icmd.Success)
+
+	res := command.PouchRun("images")
+	res.Assert(c, icmd.Success)
+	imageID := imagesListToKV(res.Combined())[helloworldImage][0]
+
+	command.PouchRun("rmi", "sha256:"+imageID).Assert(c, icmd.Success)
+
+	res = command.PouchRun("images").Assert(c, icmd.Success)
+	if out := res.Combined(); strings.Contains(out, helloworldImage) {
+		c.Fatalf("unexpected output %s: should rm image %s\n", out, helloworldImage)
+	}
+}
+
 // TestRmiByImageIDWithTwoPrimaryReferences tests "pouch rmi {ID}" work.
 func (suite *PouchRmiSuite) TestRmiByImageIDWithTwoPrimaryReferences(c *check.C) {
 	var (

test/environment/env.go

@@ -26,6 +26,9 @@ var (
 	// BusyboxRepo the repository of busybox image
 	BusyboxRepo = "registry.hub.docker.com/library/busybox"
 
+	// BusyboxID the digest ID used for busybox image
+	BusyboxID = "sha256:8ac48589692a53a9b8c2d1ceaa6b402665aa7fe667ba51ccc03002300856d8c7"
+
 	// BusyboxTag the tag used for busybox image
 	BusyboxTag = "1.28"