From 0426c76142760cac80083ee9f821f51be6da6b60 Mon Sep 17 00:00:00 2001
From: Cory Snider <csnider@mirantis.com>
Date: Mon, 21 Nov 2022 16:44:57 -0500
Subject: [PATCH] List images with multiple since/before filters

The List Images API endpoint has accepted multiple values for the
`since` and `before` filter predicates, but thanks to Go's randomizing
of map iteration order, it would pick an arbitrary image to compare
created timestamps against. In other words, the behaviour was undefined.
Change these filter predicates to have well-defined semantics: the
logical AND of all values for each of the respective predicates. As
timestamps are a totally-ordered relation, this is exactly equivalent to
applying the newest and oldest creation timestamps for the `since` and
`before` predicates, respectively.

Signed-off-by: Cory Snider <csnider@mirantis.com>
---
 daemon/images/image_list.go    | 40 ++++++++++++++++++++------------
 integration/image/list_test.go | 42 ++++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+), 14 deletions(-)

diff --git a/daemon/images/image_list.go b/daemon/images/image_list.go
index b116f8b11a..2686b3f8f5 100644
--- a/daemon/images/image_list.go
+++ b/daemon/images/image_list.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"sort"
+	"time"
 
 	"github.com/docker/distribution/reference"
 	"github.com/docker/docker/api/types"
@@ -46,20 +47,36 @@ func (i *ImageService) Images(ctx context.Context, opts types.ImageListOptions)
 	}
 
 	var (
-		beforeFilter, sinceFilter *image.Image
+		beforeFilter, sinceFilter time.Time
 		err                       error
 	)
 	err = opts.Filters.WalkValues("before", func(value string) error {
-		beforeFilter, err = i.GetImage(ctx, value, imagetypes.GetImageOpts{})
-		return err
+		img, err := i.GetImage(ctx, value, imagetypes.GetImageOpts{})
+		if err != nil {
+			return err
+		}
+		// Resolve multiple values to the oldest image,
+		// equivalent to ANDing all the values together.
+		if beforeFilter.IsZero() || beforeFilter.After(img.Created) {
+			beforeFilter = img.Created
+		}
+		return nil
 	})
 	if err != nil {
 		return nil, err
 	}
 
 	err = opts.Filters.WalkValues("since", func(value string) error {
-		sinceFilter, err = i.GetImage(ctx, value, imagetypes.GetImageOpts{})
-		return err
+		img, err := i.GetImage(ctx, value, imagetypes.GetImageOpts{})
+		if err != nil {
+			return err
+		}
+		// Resolve multiple values to the newest image,
+		// equivalent to ANDing all the values together.
+		if sinceFilter.Before(img.Created) {
+			sinceFilter = img.Created
+		}
+		return nil
 	})
 	if err != nil {
 		return nil, err
@@ -84,16 +101,11 @@ func (i *ImageService) Images(ctx context.Context, opts types.ImageListOptions)
 		default:
 		}
 
-		if beforeFilter != nil {
-			if img.Created.Equal(beforeFilter.Created) || img.Created.After(beforeFilter.Created) {
-				continue
-			}
+		if !beforeFilter.IsZero() && !img.Created.Before(beforeFilter) {
+			continue
 		}
-
-		if sinceFilter != nil {
-			if img.Created.Equal(sinceFilter.Created) || img.Created.Before(sinceFilter.Created) {
-				continue
-			}
+		if !sinceFilter.IsZero() && !img.Created.After(sinceFilter) {
+			continue
 		}
 
 		if opts.Filters.Contains("label") {
diff --git a/integration/image/list_test.go b/integration/image/list_test.go
index 68dd8d6741..a63277335e 100644
--- a/integration/image/list_test.go
+++ b/integration/image/list_test.go
@@ -2,12 +2,16 @@ package image // import "github.com/docker/docker/integration/image"
 
 import (
 	"context"
+	"fmt"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/api/types/filters"
 	"github.com/docker/docker/api/types/versions"
+	"github.com/docker/docker/integration/internal/container"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"gotest.tools/v3/assert"
 	is "gotest.tools/v3/assert/cmp"
 	"gotest.tools/v3/skip"
@@ -51,3 +55,41 @@ func TestImagesFilterMultiReference(t *testing.T) {
 		}
 	}
 }
+
+func TestImagesFilterBeforeSince(t *testing.T) {
+	defer setupTest(t)()
+	client := testEnv.APIClient()
+	ctx := context.Background()
+
+	name := strings.ToLower(t.Name())
+	ctr := container.Create(ctx, t, client, container.WithName(name))
+
+	imgs := make([]string, 5)
+	for i := range imgs {
+		if i > 0 {
+			// Make really really sure each image has a distinct timestamp.
+			time.Sleep(time.Millisecond)
+		}
+		id, err := client.ContainerCommit(ctx, ctr, types.ContainerCommitOptions{Reference: fmt.Sprintf("%s:v%d", name, i)})
+		assert.NilError(t, err)
+		imgs[i] = id.ID
+	}
+
+	filter := filters.NewArgs(
+		filters.Arg("since", imgs[0]),
+		filters.Arg("before", imgs[len(imgs)-1]),
+	)
+	list, err := client.ImageList(ctx, types.ImageListOptions{Filters: filter})
+	assert.NilError(t, err)
+
+	var listedIDs []string
+	for _, i := range list {
+		t.Logf("ImageList: ID=%v RepoTags=%v", i.ID, i.RepoTags)
+		listedIDs = append(listedIDs, i.ID)
+	}
+	// The ImageList API sorts the list by created timestamp... truncated to
+	// 1-second precision. Since all the images were created within
+	// milliseconds of each other, listedIDs is effectively unordered and
+	// 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 }))
+}