diff --git a/pkg/server/container_status_test.go b/pkg/server/container_status_test.go index 5f69ce2b3..63b6fb5c5 100644 --- a/pkg/server/container_status_test.go +++ b/pkg/server/container_status_test.go @@ -30,6 +30,7 @@ import ( func getContainerStatusTestData() (*containerstore.Metadata, *containerstore.Status, *imagestore.Image, *runtime.ContainerStatus) { + imageID := "sha256:1123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" testID := "test-id" config := &runtime.ContainerConfig{ Metadata: &runtime.ContainerMetadata{ @@ -53,7 +54,7 @@ func getContainerStatusTestData() (*containerstore.Metadata, *containerstore.Sta Name: "test-long-name", SandboxID: "test-sandbox-id", Config: config, - ImageRef: "test-image-id", + ImageRef: imageID, LogPath: "test-log-path", } status := &containerstore.Status{ @@ -62,7 +63,7 @@ func getContainerStatusTestData() (*containerstore.Metadata, *containerstore.Sta StartedAt: startedAt, } image := &imagestore.Image{ - ID: "test-image-id", + ID: imageID, RepoTags: []string{"test-image-repo-tag"}, RepoDigests: []string{"test-image-repo-digest"}, } diff --git a/pkg/server/helpers.go b/pkg/server/helpers.go index cbf3d887e..7e956e1dd 100644 --- a/pkg/server/helpers.go +++ b/pkg/server/helpers.go @@ -28,7 +28,6 @@ import ( "github.com/containerd/cgroups" "github.com/containerd/containerd" "github.com/containerd/containerd/content" - "github.com/containerd/containerd/errdefs" "github.com/docker/distribution/reference" imagedigest "github.com/opencontainers/go-digest" "github.com/opencontainers/image-spec/identity" @@ -205,34 +204,41 @@ func getRepoDigestAndTag(namedRef reference.Named, digest imagedigest.Digest, sc // localResolve resolves image reference locally and returns corresponding image metadata. It returns // nil without error if the reference doesn't exist. -func (c *criContainerdService) localResolve(ctx context.Context, ref string) (*imagestore.Image, error) { - _, err := imagedigest.Parse(ref) - if err != nil { - // ref is not image id, try to resolve it locally. - normalized, err := util.NormalizeImageRef(ref) - if err != nil { - return nil, fmt.Errorf("invalid image reference %q: %v", ref, err) +func (c *criContainerdService) localResolve(ctx context.Context, refOrID string) (*imagestore.Image, error) { + getImageID := func(refOrId string) string { + if _, err := imagedigest.Parse(refOrID); err == nil { + return refOrID } - image, err := c.client.GetImage(ctx, normalized.String()) - if err != nil { - if errdefs.IsNotFound(err) { - return nil, nil + + return func(ref string) string { + // ref is not image id, try to resolve it locally. + normalized, err := util.NormalizeImageRef(ref) + if err != nil { + return "" } - return nil, fmt.Errorf("failed to get image from containerd: %v", err) - } - desc, err := image.Config(ctx) - if err != nil { - return nil, fmt.Errorf("failed to get image config descriptor: %v", err) - } - ref = desc.Digest.String() + image, err := c.client.GetImage(ctx, normalized.String()) + if err != nil { + return "" + } + desc, err := image.Config(ctx) + if err != nil { + return "" + } + return desc.Digest.String() + }(refOrID) + } + + imageID := getImageID(refOrID) + if imageID == "" { + // Try to treat ref as imageID + imageID = refOrID } - imageID := ref image, err := c.imageStore.Get(imageID) if err != nil { if err == store.ErrNotExist { return nil, nil } - return nil, fmt.Errorf("failed to get image %q metadata: %v", imageID, err) + return nil, fmt.Errorf("failed to get image %q : %v", imageID, err) } return &image, nil } diff --git a/pkg/server/image_list_test.go b/pkg/server/image_list_test.go index e8606642c..631aad6a5 100644 --- a/pkg/server/image_list_test.go +++ b/pkg/server/image_list_test.go @@ -32,7 +32,7 @@ func TestListImages(t *testing.T) { c := newTestCRIContainerdService() imagesInStore := []imagestore.Image{ { - ID: "test-id-1", + ID: "sha256:1123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef", ChainID: "test-chainid-1", RepoTags: []string{"tag-a-1", "tag-b-1"}, RepoDigests: []string{"digest-a-1", "digest-b-1"}, @@ -42,7 +42,7 @@ func TestListImages(t *testing.T) { }, }, { - ID: "test-id-2", + ID: "sha256:2123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef", ChainID: "test-chainid-2", RepoTags: []string{"tag-a-2", "tag-b-2"}, RepoDigests: []string{"digest-a-2", "digest-b-2"}, @@ -52,7 +52,7 @@ func TestListImages(t *testing.T) { }, }, { - ID: "test-id-3", + ID: "sha256:3123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef", ChainID: "test-chainid-3", RepoTags: []string{"tag-a-3", "tag-b-3"}, RepoDigests: []string{"digest-a-3", "digest-b-3"}, @@ -64,21 +64,21 @@ func TestListImages(t *testing.T) { } expect := []*runtime.Image{ { - Id: "test-id-1", + Id: "sha256:1123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef", RepoTags: []string{"tag-a-1", "tag-b-1"}, RepoDigests: []string{"digest-a-1", "digest-b-1"}, Size_: uint64(1000), Username: "root", }, { - Id: "test-id-2", + Id: "sha256:2123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef", RepoTags: []string{"tag-a-2", "tag-b-2"}, RepoDigests: []string{"digest-a-2", "digest-b-2"}, Size_: uint64(2000), Uid: &runtime.Int64Value{Value: 1234}, }, { - Id: "test-id-3", + Id: "sha256:3123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef", RepoTags: []string{"tag-a-3", "tag-b-3"}, RepoDigests: []string{"digest-a-3", "digest-b-3"}, Size_: uint64(3000), diff --git a/pkg/store/image/image.go b/pkg/store/image/image.go index e6942e096..093a42fa3 100644 --- a/pkg/store/image/image.go +++ b/pkg/store/image/image.go @@ -20,7 +20,8 @@ import ( "sync" "github.com/containerd/containerd" - "github.com/docker/docker/pkg/truncindex" + "github.com/docker/distribution/digestset" + godigest "github.com/opencontainers/go-digest" imagespec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/kubernetes-incubator/cri-containerd/pkg/store" @@ -47,16 +48,16 @@ type Image struct { // Store stores all images. type Store struct { - lock sync.RWMutex - images map[string]Image - idIndex *truncindex.TruncIndex + lock sync.RWMutex + images map[string]Image + digestSet *digestset.Set } // NewStore creates an image store. func NewStore() *Store { return &Store{ - images: make(map[string]Image), - idIndex: truncindex.NewTruncIndex([]string{}), + images: make(map[string]Image), + digestSet: digestset.NewSet(), } } @@ -64,11 +65,11 @@ func NewStore() *Store { func (s *Store) Add(img Image) error { s.lock.Lock() defer s.lock.Unlock() - if _, err := s.idIndex.Get(img.ID); err != nil { - if err != truncindex.ErrNotExist { + if _, err := s.digestSet.Lookup(img.ID); err != nil { + if err != digestset.ErrDigestNotFound { return err } - if err := s.idIndex.Add(img.ID); err != nil { + if err := s.digestSet.Add(godigest.Digest(img.ID)); err != nil { return err } } @@ -91,14 +92,14 @@ func (s *Store) Add(img Image) error { func (s *Store) Get(id string) (Image, error) { s.lock.RLock() defer s.lock.RUnlock() - id, err := s.idIndex.Get(id) + digest, err := s.digestSet.Lookup(id) if err != nil { - if err == truncindex.ErrNotExist { + if err == digestset.ErrDigestNotFound { err = store.ErrNotExist } return Image{}, err } - if i, ok := s.images[id]; ok { + if i, ok := s.images[digest.String()]; ok { return i, nil } return Image{}, store.ErrNotExist @@ -119,14 +120,14 @@ func (s *Store) List() []Image { func (s *Store) Delete(id string) { s.lock.Lock() defer s.lock.Unlock() - id, err := s.idIndex.Get(id) + digest, err := s.digestSet.Lookup(id) if err != nil { // Note: The idIndex.Delete and delete doesn't handle truncated index. // So we need to return if there are error. return } - s.idIndex.Delete(id) // nolint: errcheck - delete(s.images, id) + s.digestSet.Remove(digest) // nolint: errcheck + delete(s.images, digest.String()) } // mergeStringSlices merges 2 string slices into one and remove duplicated elements. diff --git a/pkg/store/image/image_test.go b/pkg/store/image/image_test.go index 04241ac51..3502a3340 100644 --- a/pkg/store/image/image_test.go +++ b/pkg/store/image/image_test.go @@ -17,6 +17,7 @@ limitations under the License. package image import ( + "strings" "testing" imagespec "github.com/opencontainers/image-spec/specs-go/v1" @@ -26,33 +27,33 @@ import ( ) func TestImageStore(t *testing.T) { - images := map[string]Image{ - "1": { - ID: "1", + images := []Image{ + { + ID: "sha256:1123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef", ChainID: "test-chain-id-1", RepoTags: []string{"tag-1"}, RepoDigests: []string{"digest-1"}, Size: 10, Config: &imagespec.ImageConfig{}, }, - "2abcd": { - ID: "2abcd", + { + ID: "sha256:2123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef", ChainID: "test-chain-id-2abcd", RepoTags: []string{"tag-2abcd"}, RepoDigests: []string{"digest-2abcd"}, Size: 20, Config: &imagespec.ImageConfig{}, }, - "4a333": { - ID: "4a333", + { + ID: "sha256:3123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef", RepoTags: []string{"tag-4a333"}, RepoDigests: []string{"digest-4a333"}, ChainID: "test-chain-id-4a333", Size: 30, Config: &imagespec.ImageConfig{}, }, - "4abcd": { - ID: "4abcd", + { + ID: "sha256:4123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef", RepoTags: []string{"tag-4abcd"}, RepoDigests: []string{"digest-4abcd"}, ChainID: "test-chain-id-4abcd", @@ -61,6 +62,7 @@ func TestImageStore(t *testing.T) { }, } assert := assertlib.New(t) + genTruncIndex := func(normalName string) string { return normalName[:(len(normalName)+1)/2] } s := NewStore() @@ -71,11 +73,26 @@ func TestImageStore(t *testing.T) { } t.Logf("should be able to get image") - genTruncIndex := func(normalName string) string { return normalName[:(len(normalName)+1)/2] } - for id, img := range images { - got, err := s.Get(genTruncIndex(id)) - assert.NoError(err) - assert.Equal(img, got) + for _, v := range images { + truncID := genTruncIndex(v.ID) + got, err := s.Get(truncID) + assert.NoError(err, "truncID:%s, fullID:%s", truncID, v.ID) + assert.Equal(v, got) + } + + t.Logf("should be able to get image by truncated imageId without algorithm") + for _, v := range images { + truncID := genTruncIndex(v.ID[strings.Index(v.ID, ":")+1:]) + got, err := s.Get(truncID) + assert.NoError(err, "truncID:%s, fullID:%s", truncID, v.ID) + assert.Equal(v, got) + } + + t.Logf("should not be able to get image by ambiguous prefix") + ambiguousPrefixs := []string{"sha256", "sha256:"} + for _, v := range ambiguousPrefixs { + _, err := s.Get(v) + assert.NotEqual(nil, err) } t.Logf("should be able to list images") @@ -83,8 +100,8 @@ func TestImageStore(t *testing.T) { assert.Len(imgs, len(images)) imageNum := len(images) - for testID, v := range images { - truncID := genTruncIndex(testID) + for _, v := range images { + truncID := genTruncIndex(v.ID) oldRepoTag := v.RepoTags[0] oldRepoDigest := v.RepoDigests[0] newRepoTag := oldRepoTag + "new"