fix(api): Fix 'last' query param for <repo>/tags/list to work without…

… param 'n' (#1777) Also fix additional issues: - sorting of tags on calls without pagination parameters ('n' or 'last') - if 'n' is 0 we should return an empty list and not error Added tests accordingly Signed-off-by: Andrei Aaron <[email protected]>
project-zot · Sep 8, 2023 · 24e37eb · 24e37eb
1 parent 7b1e24c
commit 24e37eb
Show file tree

Hide file tree

Showing 3 changed files with 279 additions and 37 deletions.
diff --git a/pkg/api/controller_test.go b/pkg/api/controller_test.go
@@ -20,6 +20,7 @@ import (
 	"os"
 	"path"
 	"path/filepath"
+	"sort"
 	"strconv"
 	"strings"
 	"testing"
@@ -5817,6 +5818,20 @@ func TestRouteFailures(t *testing.T) {
 			So(resp, ShouldNotBeNil)
 			So(resp.StatusCode, ShouldEqual, http.StatusBadRequest)
 
+			request, _ = http.NewRequestWithContext(context.TODO(), http.MethodGet, baseURL, nil)
+			request = mux.SetURLVars(request, map[string]string{"name": "foo"})
+			qparm = request.URL.Query()
+			qparm.Add("n", "-1")
+			request.URL.RawQuery = qparm.Encode()
+			response = httptest.NewRecorder()
+
+			rthdlr.ListTags(response, request)
+
+			resp = response.Result()
+			defer resp.Body.Close()
+			So(resp, ShouldNotBeNil)
+			So(resp.StatusCode, ShouldEqual, http.StatusBadRequest)
+
 			request, _ = http.NewRequestWithContext(context.TODO(), http.MethodGet, baseURL, nil)
 			request = mux.SetURLVars(request, map[string]string{"name": "foo"})
 			qparm = request.URL.Query()
@@ -5890,6 +5905,20 @@ func TestRouteFailures(t *testing.T) {
 			So(resp, ShouldNotBeNil)
 			So(resp.StatusCode, ShouldEqual, http.StatusNotFound)
 
+			request, _ = http.NewRequestWithContext(context.TODO(), http.MethodGet, baseURL, nil)
+			request = mux.SetURLVars(request, map[string]string{"name": "foo"})
+			qparm = request.URL.Query()
+			qparm.Add("last", "a")
+			request.URL.RawQuery = qparm.Encode()
+			response = httptest.NewRecorder()
+
+			rthdlr.ListTags(response, request)
+
+			resp = response.Result()
+			defer resp.Body.Close()
+			So(resp, ShouldNotBeNil)
+			So(resp.StatusCode, ShouldEqual, http.StatusNotFound)
+
 			request, _ = http.NewRequestWithContext(context.TODO(), http.MethodGet, baseURL, nil)
 			request = mux.SetURLVars(request, map[string]string{"name": "foo"})
 			qparm = request.URL.Query()
@@ -6342,6 +6371,174 @@ func TestRouteFailures(t *testing.T) {
 	})
 }
 
+func TestListingTags(t *testing.T) {
+	port := test.GetFreePort()
+	baseURL := test.GetBaseURL(port)
+	conf := config.New()
+	conf.HTTP.Port = port
+
+	ctlr := makeController(conf, t.TempDir())
+	ctlr.Config.Storage.Commit = true
+
+	cm := test.NewControllerManager(ctlr)
+	cm.StartAndWait(port)
+
+	defer cm.StopServer()
+
+	rthdlr := api.NewRouteHandler(ctlr)
+
+	img := test.CreateRandomImage()
+	sigTag := fmt.Sprintf("sha256-%s.sig", img.Digest().Encoded())
+
+	repoName := "test-tags"
+	tagsList := []string{
+		"1", "2", "1.0.0", "new", "2.0.0", sigTag,
+		"2-test", "New", "2.0.0-test", "latest",
+	}
+
+	for _, tag := range tagsList {
+		err := test.UploadImage(img, baseURL, repoName, tag)
+		if err != nil {
+			panic(err)
+		}
+	}
+
+	// Note empty strings signify the query parameter is not set
+	// There are separate tests for passing the empty string as query parameter
+	testCases := []struct {
+		testCaseName string
+		pageSize     string
+		last         string
+		expectedTags []string
+	}{
+		{
+			testCaseName: "Test tag soting order with no parameters",
+			pageSize:     "",
+			last:         "",
+			expectedTags: []string{
+				"1", "1.0.0", "2", "2-test", "2.0.0", "2.0.0-test",
+				"New", "latest", "new", sigTag,
+			},
+		},
+		{
+			testCaseName: "Test with the parameter 'n' lower than total number of results",
+			pageSize:     "5",
+			last:         "",
+			expectedTags: []string{
+				"1", "1.0.0", "2", "2-test", "2.0.0",
+			},
+		},
+		{
+			testCaseName: "Test with the parameter 'n' larger than total number of results",
+			pageSize:     "50",
+			last:         "",
+			expectedTags: []string{
+				"1", "1.0.0", "2", "2-test", "2.0.0", "2.0.0-test",
+				"New", "latest", "new", sigTag,
+			},
+		},
+		{
+			testCaseName: "Test the parameter 'n' is 0",
+			pageSize:     "0",
+			last:         "",
+			expectedTags: []string{},
+		},
+		{
+			testCaseName: "Test the parameters 'n' and 'last'",
+			pageSize:     "5",
+			last:         "2-test",
+			expectedTags: []string{"2.0.0", "2.0.0-test", "New", "latest", "new"},
+		},
+		{
+			testCaseName: "Test the parameters 'n' and 'last' with `n` exceeding total number of results",
+			pageSize:     "5",
+			last:         "latest",
+			expectedTags: []string{"new", sigTag},
+		},
+		{
+			testCaseName: "Test the parameter 'n' and 'last' being second to last tag as value",
+			pageSize:     "2",
+			last:         "new",
+			expectedTags: []string{sigTag},
+		},
+		{
+			testCaseName: "Test the parameter 'last' without parameter 'n'",
+			pageSize:     "",
+			last:         "2",
+			expectedTags: []string{
+				"2-test", "2.0.0", "2.0.0-test",
+				"New", "latest", "new", sigTag,
+			},
+		},
+		{
+			testCaseName: "Test the parameter 'last' with the final tag as value",
+			pageSize:     "",
+			last:         sigTag,
+			expectedTags: []string{},
+		},
+		{
+			testCaseName: "Test the parameter 'last' with the second to last tag as value",
+			pageSize:     "",
+			last:         "new",
+			expectedTags: []string{sigTag},
+		},
+	}
+
+	for _, testCase := range testCases {
+		Convey(testCase.testCaseName, t, func() {
+			t.Log("Running " + testCase.testCaseName)
+
+			request, _ := http.NewRequestWithContext(context.TODO(), http.MethodGet, baseURL, nil)
+			request = mux.SetURLVars(request, map[string]string{"name": repoName})
+
+			if testCase.pageSize != "" || testCase.last != "" {
+				qparm := request.URL.Query()
+
+				if testCase.pageSize != "" {
+					qparm.Add("n", testCase.pageSize)
+				}
+
+				if testCase.last != "" {
+					qparm.Add("last", testCase.last)
+				}
+
+				request.URL.RawQuery = qparm.Encode()
+			}
+
+			response := httptest.NewRecorder()
+
+			rthdlr.ListTags(response, request)
+
+			resp := response.Result()
+			defer resp.Body.Close()
+			So(resp, ShouldNotBeNil)
+			So(resp.StatusCode, ShouldEqual, http.StatusOK)
+
+			var tags api.ImageTags
+			err := json.NewDecoder(resp.Body).Decode(&tags)
+			So(err, ShouldBeNil)
+			So(tags.Tags, ShouldEqual, testCase.expectedTags)
+
+			alltags := tagsList
+			sort.Strings(alltags)
+
+			actualLinkValue := resp.Header.Get("Link")
+			if testCase.pageSize == "0" || testCase.pageSize == "" { //nolint:gocritic
+				So(actualLinkValue, ShouldEqual, "")
+			} else if testCase.expectedTags[len(testCase.expectedTags)-1] == alltags[len(alltags)-1] {
+				So(actualLinkValue, ShouldEqual, "")
+			} else {
+				expectedLinkValue := fmt.Sprintf("/v2/%s/tags/list?n=%s&last=%s; rel=\"next\"",
+					repoName, testCase.pageSize, tags.Tags[len(tags.Tags)-1],
+				)
+				So(actualLinkValue, ShouldEqual, expectedLinkValue)
+			}
+
+			t.Log("Finished " + testCase.testCaseName)
+		})
+	}
+}
+
 func TestStorageCommit(t *testing.T) {
 	Convey("Make a new controller", t, func() {
 		port := test.GetFreePort()

diff --git a/pkg/api/routes.go b/pkg/api/routes.go
@@ -293,8 +293,6 @@ func (rh *RouteHandler) ListTags(response http.ResponseWriter, request *http.Req
 		return
 	}
 
-	imgStore := rh.getImageStore(name)
-
 	paginate := false
 	numTags := -1
 
@@ -319,6 +317,12 @@ func (rh *RouteHandler) ListTags(response http.ResponseWriter, request *http.Req
 
 		numTags = int(nQuery1)
 		paginate = true
+
+		if numTags < 0 {
+			response.WriteHeader(http.StatusBadRequest)
+
+			return
+		}
 	}
 
 	last := ""
@@ -334,6 +338,8 @@ func (rh *RouteHandler) ListTags(response http.ResponseWriter, request *http.Req
 		last = lastQuery[0]
 	}
 
+	imgStore := rh.getImageStore(name)
+
 	tags, err := imgStore.GetImageTags(name)
 	if err != nil {
 		e := apiErr.NewError(apiErr.NAME_UNKNOWN).AddDetail(map[string]string{"name": name})
@@ -342,56 +348,56 @@ func (rh *RouteHandler) ListTags(response http.ResponseWriter, request *http.Req
 		return
 	}
 
-	if paginate && (numTags < len(tags)) {
-		sort.Strings(tags)
+	// Tags need to be sorted regardless of pagination parameters
+	sort.Strings(tags)
 
-		pTags := ImageTags{Name: name}
+	// Determine index of first tag returned
+	startIndex := 0
 
-		if last == "" {
-			// first
-			pTags.Tags = tags[:numTags]
-		} else {
-			// next
-			var i int
-			found := false
-			for idx, tag := range tags {
-				if tag == last {
-					found = true
-					i = idx
-
-					break
-				}
-			}
+	if last != "" {
+		found := false
 
-			if !found {
-				response.WriteHeader(http.StatusNotFound)
+		for i, tag := range tags {
+			if tag == last {
+				found = true
+				startIndex = i + 1
 
-				return
+				break
 			}
+		}
 
-			if numTags >= len(tags)-i {
-				pTags.Tags = tags[i+1:]
-				zcommon.WriteJSON(response, http.StatusOK, pTags)
-
-				return
-			}
+		if !found {
+			response.WriteHeader(http.StatusNotFound)
 
-			pTags.Tags = tags[i+1 : i+1+numTags]
+			return
 		}
+	}
 
-		if len(pTags.Tags) == 0 {
-			last = ""
-		} else {
-			last = pTags.Tags[len(pTags.Tags)-1]
-		}
+	pTags := ImageTags{Name: name}
 
-		response.Header().Set("Link", fmt.Sprintf("/v2/%s/tags/list?n=%d&last=%s; rel=\"next\"", name, numTags, last))
+	if paginate && numTags == 0 {
+		pTags.Tags = []string{}
 		zcommon.WriteJSON(response, http.StatusOK, pTags)
 
 		return
 	}
 
-	zcommon.WriteJSON(response, http.StatusOK, ImageTags{Name: name, Tags: tags})
+	stopIndex := len(tags) - 1
+	if paginate && (startIndex+numTags < len(tags)) {
+		stopIndex = startIndex + numTags - 1
+		response.Header().Set(
+			"Link",
+			fmt.Sprintf("/v2/%s/tags/list?n=%d&last=%s; rel=\"next\"",
+				name,
+				numTags,
+				tags[stopIndex],
+			),
+		)
+	}
+
+	pTags.Tags = tags[startIndex : stopIndex+1]
+
+	zcommon.WriteJSON(response, http.StatusOK, pTags)
 }
 
 // CheckManifest godoc

diff --git a/test/blackbox/pushpull.bats b/test/blackbox/pushpull.bats
@@ -154,6 +154,45 @@ function teardown_file() {
     [ $(echo "$output" | jq -r ".manifests | length") -eq 2 ]
 }
 
+@test "add and list tags using oras" {
+    run skopeo --insecure-policy copy --dest-tls-verify=false \
+        oci:${TEST_DATA_DIR}/golang:1.20 \
+        docker://127.0.0.1:8080/oras-tags:1.20
+    [ "$status" -eq 0 ]
+    run oras tag --plain-http 127.0.0.1:8080/oras-tags:1.20 1 new latest
+    [ "$status" -eq 0 ]
+    run oras repo tags --plain-http 127.0.0.1:8080/oras-tags
+    [ "$status" -eq 0 ]
+    echo "$output"
+    [ $(echo "$output" | wc -l) -eq 4 ]
+    [ "${lines[-1]}" == "new" ]
+    [ "${lines[-2]}" == "latest" ]
+    [ "${lines[-3]}" == "1.20" ]
+    [ "${lines[-4]}" == "1" ]
+    run oras repo tags --plain-http --last new 127.0.0.1:8080/oras-tags
+    [ "$status" -eq 0 ]
+    echo "$output"
+    [ -z $output ]
+    run oras repo tags --plain-http --last latest 127.0.0.1:8080/oras-tags
+    [ "$status" -eq 0 ]
+    echo "$output"
+    [ $(echo "$output" | wc -l) -eq 1 ]
+    [ "${lines[-1]}" == "new" ]
+    run oras repo tags --plain-http --last "1.20" 127.0.0.1:8080/oras-tags
+    [ "$status" -eq 0 ]
+    echo "$output"
+    [ $(echo "$output" | wc -l) -eq 2 ]
+    [ "${lines[-2]}" == "latest" ]
+    [ "${lines[-1]}" == "new" ]
+    run oras repo tags --plain-http --last "1" 127.0.0.1:8080/oras-tags
+    [ "$status" -eq 0 ]
+    echo "$output"
+    [ $(echo "$output" | wc -l) -eq 3 ]
+    [ "${lines[-3]}" == "1.20" ]
+    [ "${lines[-2]}" == "latest" ]
+    [ "${lines[-1]}" == "new" ]
+}
+
 @test "push helm chart" {
     run helm package ${BATS_FILE_TMPDIR}/helm-charts/charts/zot -d ${BATS_FILE_TMPDIR}
     [ "$status" -eq 0 ]