From 24e37eb68baacbe46ca87657e25b4ea4956f875e Mon Sep 17 00:00:00 2001 From: Andrei Aaron Date: Fri, 8 Sep 2023 19:46:17 +0300 Subject: [PATCH] fix(api): Fix 'last' query param for /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 --- pkg/api/controller_test.go | 197 ++++++++++++++++++++++++++++++++++++ pkg/api/routes.go | 86 ++++++++-------- test/blackbox/pushpull.bats | 39 +++++++ 3 files changed, 282 insertions(+), 40 deletions(-) diff --git a/pkg/api/controller_test.go b/pkg/api/controller_test.go index 3d03ca82..4ab83d42 100644 --- 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 index c57020d0..fabfa698 100644 --- 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 + if last != "" { + found := false - break - } + for i, tag := range tags { + if tag == last { + found = true + startIndex = i + 1 + + break } - - if !found { - response.WriteHeader(http.StatusNotFound) - - return - } - - if numTags >= len(tags)-i { - pTags.Tags = tags[i+1:] - zcommon.WriteJSON(response, http.StatusOK, pTags) - - return - } - - pTags.Tags = tags[i+1 : i+1+numTags] } - if len(pTags.Tags) == 0 { - last = "" - } else { - last = pTags.Tags[len(pTags.Tags)-1] - } + if !found { + response.WriteHeader(http.StatusNotFound) - response.Header().Set("Link", fmt.Sprintf("/v2/%s/tags/list?n=%d&last=%s; rel=\"next\"", name, numTags, last)) + return + } + } + + pTags := ImageTags{Name: name} + + 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 index 73efadb3..03d7c6d1 100644 --- 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 ]