Merge pull request #46034 from rumpl/c8d-image-list

c8d: Various images/json API fixes

daemon/containerd/image_list.go

@@ -3,6 +3,7 @@ package containerd
 import (
 	"context"
 	"encoding/json"
+	"sort"
 	"strings"
 	"time"
 
@@ -14,6 +15,7 @@ import (
 	"github.com/docker/distribution/reference"
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/api/types/filters"
+	"github.com/docker/docker/api/types/image"
 	timetypes "github.com/docker/docker/api/types/time"
 	"github.com/opencontainers/go-digest"
 	"github.com/opencontainers/image-spec/identity"
@@ -22,6 +24,13 @@ import (
 	"github.com/sirupsen/logrus"
 )
 
+// Subset of ocispec.Image that only contains Labels
+type configLabels struct {
+	Config struct {
+		Labels map[string]string `json:"Labels,omitempty"`
+	} `json:"config,omitempty"`
+}
+
 var acceptedImageFilterTags = map[string]bool{
 	"dangling":  true,
 	"label":     true,
@@ -32,11 +41,17 @@ var acceptedImageFilterTags = map[string]bool{
 	"until":     true,
 }
 
+// byCreated is a temporary type used to sort a list of images by creation
+// time.
+type byCreated []*types.ImageSummary
+
+func (r byCreated) Len() int           { return len(r) }
+func (r byCreated) Swap(i, j int)      { r[i], r[j] = r[j], r[i] }
+func (r byCreated) Less(i, j int) bool { return r[i].Created < r[j].Created }
+
 // Images returns a filtered list of images.
 //
-// TODO(thaJeztah): sort the results by created (descending); see https://github.com/moby/moby/issues/43848
 // TODO(thaJeztah): implement opts.ContainerCount (used for docker system df); see https://github.com/moby/moby/issues/43853
-// TODO(thaJeztah): add labels to results; see https://github.com/moby/moby/issues/43852
 // TODO(thaJeztah): verify behavior of `RepoDigests` and `RepoTags` for images without (untagged) or multiple tags; see https://github.com/moby/moby/issues/43861
 // TODO(thaJeztah): verify "Size" vs "VirtualSize" in images; see https://github.com/moby/moby/issues/43862
 func (i *ImageService) Images(ctx context.Context, opts types.ImageListOptions) ([]*types.ImageSummary, error) {
@@ -44,12 +59,12 @@ func (i *ImageService) Images(ctx context.Context, opts types.ImageListOptions)
 		return nil, err
 	}
 
-	listFilters, filter, err := i.setupFilters(ctx, opts.Filters)
+	filter, err := i.setupFilters(ctx, opts.Filters)
 	if err != nil {
 		return nil, err
 	}
 
-	imgs, err := i.client.ImageService().List(ctx, listFilters...)
+	imgs, err := i.client.ImageService().List(ctx)
 	if err != nil {
 		return nil, err
 	}
@@ -80,11 +95,29 @@ func (i *ImageService) Images(ctx context.Context, opts types.ImageListOptions)
 	}
 
 	contentStore := i.client.ContentStore()
+	uniqueImages := map[digest.Digest]images.Image{}
+	tagsByDigest := map[digest.Digest][]string{}
+
 	for _, img := range imgs {
 		if !filter(img) {
 			continue
 		}
 
+		dgst := img.Target.Digest
+		uniqueImages[dgst] = img
+
+		if isDanglingImage(img) {
+			continue
+		}
+
+		ref, err := reference.ParseNormalizedNamed(img.Name)
+		if err != nil {
+			continue
+		}
+		tagsByDigest[dgst] = append(tagsByDigest[dgst], reference.FamiliarString(ref))
+	}
+
+	for _, img := range uniqueImages {
 		err := i.walkImageManifests(ctx, img, func(img *ImageManifest) error {
 			if isPseudo, err := img.IsPseudoImage(ctx); isPseudo || err != nil {
 				return err
@@ -104,7 +137,7 @@ func (i *ImageService) Images(ctx context.Context, opts types.ImageListOptions)
 				return nil
 			}
 
-			image, chainIDs, err := i.singlePlatformImage(ctx, contentStore, img)
+			image, chainIDs, err := i.singlePlatformImage(ctx, contentStore, tagsByDigest[img.RealTarget.Digest], img)
 			if err != nil {
 				return err
 			}
@@ -136,10 +169,12 @@ func (i *ImageService) Images(ctx context.Context, opts types.ImageListOptions)
 		}
 	}
 
+	sort.Sort(sort.Reverse(byCreated(summaries)))
+
 	return summaries, nil
 }
 
-func (i *ImageService) singlePlatformImage(ctx context.Context, contentStore content.Store, image *ImageManifest) (*types.ImageSummary, []digest.Digest, error) {
+func (i *ImageService) singlePlatformImage(ctx context.Context, contentStore content.Store, repoTags []string, image *ImageManifest) (*types.ImageSummary, []digest.Digest, error) {
 	diffIDs, err := image.RootFS(ctx)
 	if err != nil {
 		return nil, nil, err
@@ -178,7 +213,7 @@ func (i *ImageService) singlePlatformImage(ctx context.Context, contentStore con
 	// (unpacked layers) combined.
 	totalSize := size + snapshotSize
 
-	var repoTags, repoDigests []string
+	var repoDigests []string
 	rawImg := image.Metadata()
 	target := rawImg.Target.Digest
 
@@ -199,8 +234,6 @@ func (i *ImageService) singlePlatformImage(ctx context.Context, contentStore con
 			repoTags = append(repoTags, rawImg.Name)
 		}
 	} else {
-		repoTags = append(repoTags, reference.TagNameOnly(ref).String())
-
 		digested, err := reference.WithDigest(reference.TrimNamed(ref), target)
 		if err != nil {
 			logger.WithError(err).Error("failed to create digested reference")
@@ -209,6 +242,15 @@ func (i *ImageService) singlePlatformImage(ctx context.Context, contentStore con
 		}
 	}
 
+	cfgDesc, err := image.Image.Config(ctx)
+	if err != nil {
+		return nil, nil, err
+	}
+	var cfg configLabels
+	if err := readConfig(ctx, contentStore, cfgDesc, &cfg); err != nil {
+		return nil, nil, err
+	}
+
 	summary := &types.ImageSummary{
 		ParentID:    "",
 		ID:          target.String(),
@@ -216,6 +258,7 @@ func (i *ImageService) singlePlatformImage(ctx context.Context, contentStore con
 		RepoDigests: repoDigests,
 		RepoTags:    repoTags,
 		Size:        totalSize,
+		Labels:      cfg.Config.Labels,
 		// -1 indicates that the value has not been set (avoids ambiguity
 		// between 0 (default) and "not set". We cannot use a pointer (nil)
 		// for this, as the JSON representation uses "omitempty", which would
@@ -231,47 +274,48 @@ type imageFilterFunc func(image images.Image) bool
 
 // setupFilters constructs an imageFilterFunc from the given imageFilters.
 //
-// containerdListFilters is a slice of filters which should be passed to ImageService.List()
 // filterFunc is a function that checks whether given image matches the filters.
-// TODO(thaJeztah): reimplement filters using containerd filters: see https://github.com/moby/moby/issues/43845
-func (i *ImageService) setupFilters(ctx context.Context, imageFilters filters.Args) (containerdListFilters []string, filterFunc imageFilterFunc, outErr error) {
+// TODO(thaJeztah): reimplement filters using containerd filters if possible: see https://github.com/moby/moby/issues/43845
+func (i *ImageService) setupFilters(ctx context.Context, imageFilters filters.Args) (filterFunc imageFilterFunc, outErr error) {
 	var fltrs []imageFilterFunc
 	err := imageFilters.WalkValues("before", func(value string) error {
-		ref, err := reference.ParseDockerRef(value)
+		img, err := i.GetImage(ctx, value, image.GetImageOpts{})
 		if err != nil {
 			return err
 		}
-		img, err := i.client.GetImage(ctx, ref.String())
-		if img != nil {
-			t := img.Metadata().CreatedAt
-			fltrs = append(fltrs, func(image images.Image) bool {
-				created := image.CreatedAt
-				return created.Equal(t) || created.After(t)
+		if img != nil && img.Created != nil {
+			fltrs = append(fltrs, func(candidate images.Image) bool {
+				cand, err := i.GetImage(ctx, candidate.Name, image.GetImageOpts{})
+				if err != nil {
+					return false
+				}
+				return cand.Created != nil && cand.Created.Before(*img.Created)
 			})
 		}
-		return err
+		return nil
 	})
 	if err != nil {
-		return nil, nil, err
+		return nil, err
 	}
 
 	err = imageFilters.WalkValues("since", func(value string) error {
-		ref, err := reference.ParseDockerRef(value)
+		img, err := i.GetImage(ctx, value, image.GetImageOpts{})
 		if err != nil {
 			return err
 		}
-		img, err := i.client.GetImage(ctx, ref.String())
-		if img != nil {
-			t := img.Metadata().CreatedAt
-			fltrs = append(fltrs, func(image images.Image) bool {
-				created := image.CreatedAt
-				return created.Equal(t) || created.Before(t)
+		if img != nil && img.Created != nil {
+			fltrs = append(fltrs, func(candidate images.Image) bool {
+				cand, err := i.GetImage(ctx, candidate.Name, image.GetImageOpts{})
+				if err != nil {
+					return false
+				}
+				return cand.Created != nil && cand.Created.After(*img.Created)
 			})
 		}
-		return err
+		return nil
 	})
 	if err != nil {
-		return nil, nil, err
+		return nil, err
 	}
 
 	err = imageFilters.WalkValues("until", func(value string) error {
@@ -292,12 +336,12 @@ func (i *ImageService) setupFilters(ctx context.Context, imageFilters filters.Ar
 		return err
 	})
 	if err != nil {
-		return nil, nil, err
+		return nil, err
 	}
 
 	labelFn, err := setupLabelFilter(i.client.ContentStore(), imageFilters)
 	if err != nil {
-		return nil, nil, err
+		return nil, err
 	}
 	if labelFn != nil {
 		fltrs = append(fltrs, labelFn)
@@ -306,28 +350,33 @@ func (i *ImageService) setupFilters(ctx context.Context, imageFilters filters.Ar
 	if imageFilters.Contains("dangling") {
 		danglingValue, err := imageFilters.GetBoolOrDefault("dangling", false)
 		if err != nil {
-			return nil, nil, err
+			return nil, err
 		}
 		fltrs = append(fltrs, func(image images.Image) bool {
 			return danglingValue == isDanglingImage(image)
 		})
 	}
 
-	var listFilters []string
-	err = imageFilters.WalkValues("reference", func(value string) error {
-		ref, err := reference.ParseNormalizedNamed(value)
-		if err != nil {
-			return err
-		}
-		ref = reference.TagNameOnly(ref)
-		listFilters = append(listFilters, "name=="+ref.String())
-		return nil
-	})
-	if err != nil {
-		return nil, nil, err
+	if refs := imageFilters.Get("reference"); len(refs) != 0 {
+		fltrs = append(fltrs, func(image images.Image) bool {
+			ref, err := reference.ParseNormalizedNamed(image.Name)
+			if err != nil {
+				return false
+			}
+			for _, value := range refs {
+				found, err := reference.FamiliarMatch(value, ref)
+				if err != nil {
+					return false
+				}
+				if found {
+					return found
+				}
+			}
+			return false
+		})
 	}
 
-	return listFilters, func(image images.Image) bool {
+	return func(image images.Image) bool {
 		for _, filter := range fltrs {
 			if !filter(image) {
 				return false
@@ -385,12 +434,7 @@ func setupLabelFilter(store content.Store, fltrs filters.Args) (func(image image
 			if !images.IsConfigType(desc.MediaType) {
 				return nil, nil
 			}
-			// Subset of ocispec.Image that only contains Labels
-			var cfg struct {
-				Config struct {
-					Labels map[string]string `json:"Labels,omitempty"`
-				} `json:"Config,omitempty"`
-			}
+			var cfg configLabels
 			if err := readConfig(ctx, store, desc, &cfg); err != nil {
 				return nil, err
 			}

daemon/containerd/image_prune.go

@@ -53,7 +53,7 @@ func (i *ImageService) ImagesPrune(ctx context.Context, fltrs filters.Args) (*ty
 		fltrs.Del("dangling", v)
 	}
 
-	_, filterFunc, err := i.setupFilters(ctx, fltrs)
+	filterFunc, err := i.setupFilters(ctx, fltrs)
 	if err != nil {
 		return nil, err
 	}

integration-cli/docker_api_images_test.go

@@ -8,7 +8,6 @@ import (
 	"testing"
 
 	"github.com/docker/docker/api/types"
-	"github.com/docker/docker/api/types/filters"
 	"github.com/docker/docker/client"
 	"github.com/docker/docker/integration-cli/cli"
 	"github.com/docker/docker/integration-cli/cli/build"
@@ -16,42 +15,6 @@ import (
 	"gotest.tools/v3/assert"
 )
 
-func (s *DockerAPISuite) TestAPIImagesFilter(c *testing.T) {
-	apiClient, err := client.NewClientWithOpts(client.FromEnv)
-	assert.NilError(c, err)
-	defer apiClient.Close()
-
-	name := "utest:tag1"
-	name2 := "utest/docker:tag2"
-	name3 := "utest:5000/docker:tag3"
-	for _, n := range []string{name, name2, name3} {
-		dockerCmd(c, "tag", "busybox", n)
-	}
-	getImages := func(filter string) []types.ImageSummary {
-		options := types.ImageListOptions{
-			All:     false,
-			Filters: filters.NewArgs(filters.Arg("reference", filter)),
-		}
-		images, err := apiClient.ImageList(context.Background(), options)
-		assert.NilError(c, err)
-
-		return images
-	}
-
-	// incorrect number of matches returned
-	images := getImages("utest*/*")
-	assert.Equal(c, len(images[0].RepoTags), 2)
-
-	images = getImages("utest")
-	assert.Equal(c, len(images[0].RepoTags), 1)
-
-	images = getImages("utest*")
-	assert.Equal(c, len(images[0].RepoTags), 1)
-
-	images = getImages("*5000*/*")
-	assert.Equal(c, len(images[0].RepoTags), 1)
-}
-
 func (s *DockerAPISuite) TestAPIImagesSaveAndLoad(c *testing.T) {
 	testRequires(c, Network)
 	buildImageSuccessfully(c, "saveandload", build.WithDockerfile("FROM busybox\nENV FOO bar"))

integration/image/list_test.go

@@ -20,7 +20,7 @@ import (
 // Regression : #38171
 func TestImagesFilterMultiReference(t *testing.T) {
 	skip.If(t, versions.LessThan(testEnv.DaemonAPIVersion(), "1.40"), "broken in earlier versions")
-	defer setupTest(t)()
+	t.Cleanup(setupTest(t))
 	client := testEnv.APIClient()
 	ctx := context.Background()
 
@@ -42,13 +42,13 @@ func TestImagesFilterMultiReference(t *testing.T) {
 	filter.Add("reference", repoTags[1])
 	filter.Add("reference", repoTags[2])
 	options := types.ImageListOptions{
-		All:     false,
 		Filters: filter,
 	}
 	images, err := client.ImageList(ctx, options)
 	assert.NilError(t, err)
 
-	assert.Check(t, is.Equal(len(images[0].RepoTags), 3))
+	assert.Assert(t, is.Len(images, 1))
+	assert.Check(t, is.Len(images[0].RepoTags, 3))
 	for _, repoTag := range images[0].RepoTags {
 		if repoTag != repoTags[0] && repoTag != repoTags[1] && repoTag != repoTags[2] {
 			t.Errorf("list images doesn't match any repoTag we expected, repoTag: %s", repoTag)
@@ -57,7 +57,7 @@ func TestImagesFilterMultiReference(t *testing.T) {
 }
 
 func TestImagesFilterBeforeSince(t *testing.T) {
-	defer setupTest(t)()
+	t.Cleanup(setupTest(t))
 	client := testEnv.APIClient()
 	ctx := context.Background()
 
@@ -93,3 +93,59 @@ func TestImagesFilterBeforeSince(t *testing.T) {
 	// the assertion must therefore be order-independent.
 	assert.DeepEqual(t, listedIDs, imgs[1:len(imgs)-1], cmpopts.SortSlices(func(a, b string) bool { return a < b }))
 }
+
+func TestAPIImagesFilters(t *testing.T) {
+	t.Cleanup(setupTest(t))
+	client := testEnv.APIClient()
+	ctx := context.Background()
+
+	for _, n := range []string{"utest:tag1", "utest/docker:tag2", "utest:5000/docker:tag3"} {
+		err := client.ImageTag(ctx, "busybox:latest", n)
+		assert.NilError(t, err)
+	}
+
+	testcases := []struct {
+		name             string
+		filters          []filters.KeyValuePair
+		expectedImages   int
+		expectedRepoTags int
+	}{
+		{
+			name:             "repository regex",
+			filters:          []filters.KeyValuePair{filters.Arg("reference", "utest*/*")},
+			expectedImages:   1,
+			expectedRepoTags: 2,
+		},
+		{
+			name:             "image name regex",
+			filters:          []filters.KeyValuePair{filters.Arg("reference", "utest*")},
+			expectedImages:   1,
+			expectedRepoTags: 1,
+		},
+		{
+			name:             "image name without a tag",
+			filters:          []filters.KeyValuePair{filters.Arg("reference", "utest")},
+			expectedImages:   1,
+			expectedRepoTags: 1,
+		},
+		{
+			name:             "registry port regex",
+			filters:          []filters.KeyValuePair{filters.Arg("reference", "*5000*/*")},
+			expectedImages:   1,
+			expectedRepoTags: 1,
+		},
+	}
+
+	for _, tc := range testcases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			images, err := client.ImageList(context.Background(), types.ImageListOptions{
+				Filters: filters.NewArgs(tc.filters...),
+			})
+			assert.Check(t, err)
+			assert.Assert(t, is.Len(images, tc.expectedImages))
+			assert.Check(t, is.Len(images[0].RepoTags, tc.expectedRepoTags))
+		})
+	}
+}