From 6619b7bf3dd42767a60a278b90f3dfe9e46b5632 Mon Sep 17 00:00:00 2001 From: Eusebiu Petu Date: Wed, 13 Nov 2024 23:12:41 +0200 Subject: [PATCH] feat(api): added /v2/_catalog pagination, fixes #2715 Signed-off-by: Eusebiu Petu --- pkg/api/controller_test.go | 139 +++++++++++++++++++++ pkg/api/routes.go | 177 ++++++++++----------------- pkg/api/routes_test.go | 20 +-- pkg/storage/imagestore/imagestore.go | 8 +- 4 files changed, 222 insertions(+), 122 deletions(-) diff --git a/pkg/api/controller_test.go b/pkg/api/controller_test.go index 66da66873..26ebf031b 100644 --- a/pkg/api/controller_test.go +++ b/pkg/api/controller_test.go @@ -8145,6 +8145,145 @@ func TestRouteFailures(t *testing.T) { }) } +func TestPagedRepositories(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 := CreateRandomImage() + + tag := "0.1" + repoName := "alpine" + repoNames := []string{ + "alpine1", "alpine2", "alpine3", + "alpine4", "alpine5", "alpine6", + "alpine7", "alpine8", "alpine9", + } + + for _, repo := range repoNames { + err := UploadImage(img, baseURL, repo, 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 + expectedRepos []string + }{ + { + testCaseName: "no parameters", + pageSize: "", + last: "", + expectedRepos: repoNames, + }, + { + testCaseName: "first 5", + pageSize: "5", + last: "", + expectedRepos: repoNames[:5], + }, + { + testCaseName: "next 5", + pageSize: "5", + last: repoName + "5", + expectedRepos: repoNames[5:9], + }, + { + testCaseName: "0", + pageSize: "0", + last: "", + expectedRepos: []string{}, + }, + { + testCaseName: "Test the parameter 'last' without parameter 'n'", + pageSize: "", + last: repoName + "2", + expectedRepos: repoNames[2:9], + }, + { + testCaseName: "Test the parameter 'last' with the final repo as value", + pageSize: "", + last: repoName + "9", + expectedRepos: []string{}, + }, + } + + for _, testCase := range testCases { + Convey(testCase.testCaseName, t, func() { + t.Log("Running " + testCase.testCaseName) + + request, _ := http.NewRequestWithContext(context.TODO(), http.MethodGet, + baseURL+constants.RoutePrefix+constants.ExtCatalogPrefix, nil) + + 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.ListRepositories(response, request) + + resp := response.Result() + defer resp.Body.Close() + So(resp, ShouldNotBeNil) + So(resp.StatusCode, ShouldEqual, http.StatusOK) + + catalog := struct { + Repositories []string `json:"repositories"` + }{} + + body, err := io.ReadAll(resp.Body) + So(err, ShouldBeNil) + + err = json.Unmarshal(body, &catalog) + So(err, ShouldBeNil) + + So(catalog.Repositories, ShouldEqual, testCase.expectedRepos) + + actualLinkValue := resp.Header.Get("Link") + if testCase.pageSize == "0" || testCase.pageSize == "" { //nolint:gocritic + So(actualLinkValue, ShouldEqual, "") + } else if testCase.expectedRepos[len(testCase.expectedRepos)-1] == catalog.Repositories[len(catalog.Repositories)-1] && + testCase.expectedRepos[len(testCase.expectedRepos)-1] == repoNames[len(repoNames)-1] { + So(actualLinkValue, ShouldEqual, "") + } else { + expectedLinkValue := fmt.Sprintf("; rel=\"next\"", + testCase.pageSize, catalog.Repositories[len(catalog.Repositories)-1], + ) + So(actualLinkValue, ShouldEqual, expectedLinkValue) + } + + t.Log("Finished " + testCase.testCaseName) + }) + } +} + func TestListingTags(t *testing.T) { port := test.GetFreePort() baseURL := test.GetBaseURL(port) diff --git a/pkg/api/routes.go b/pkg/api/routes.go index 27deeb71d..6663a656c 100644 --- a/pkg/api/routes.go +++ b/pkg/api/routes.go @@ -1764,84 +1764,51 @@ type RepositoryList struct { Repositories []string `json:"repositories"` } -// ListRepositories godoc -// @Summary List image repositories -// @Description List all image repositories -// @Accept json -// @Produce json -// @Success 200 {object} api.RepositoryList -// @Failure 500 {string} string "internal server error" -// @Router /v2/_catalog [get]. -func (rh *RouteHandler) ListRepositories(response http.ResponseWriter, request *http.Request) { - if request.Method == http.MethodOptions { - return - } - - q := request.URL.Query() +func (rh *RouteHandler) listStorageRepositories(lastEntry string, maxEntries int) ([]string, bool, error) { + var remainder int - lastEntry := q.Get("last") - maxEntries, err := strconv.Atoi(q.Get("n")) - if err != nil { - maxEntries = -1 - } - - if lastEntry != "" || maxEntries > 0 { - storePath := rh.c.StoreController.GetStorePath(lastEntry) + var moreEntries bool - combineRepoList := make([]string, 0) + var repos []string - var moreEntries bool = false + var err error - var remainder int + combineRepoList := make([]string, 0) + storePath := rh.c.StoreController.GetStorePath(lastEntry) + if storePath == storage.DefaultStorePath { singleStore := rh.c.StoreController.DefaultStore - if singleStore != nil && storePath == storage.DefaultStorePath { // route is default - var err error - - var repos []string - repos, moreEntries, err = singleStore.GetNextRepositories(lastEntry, maxEntries) - if err != nil { - response.WriteHeader(http.StatusInternalServerError) - - return - } - - // compute remainder - remainder = maxEntries - len(repos) - - if moreEntries { - // maxEntries has been hit - lastEntry = repos[len(repos)-1] - } else { - // reset for the next substores - lastEntry = "" - } - - combineRepoList = append(combineRepoList, repos...) + repos, moreEntries, err = singleStore.GetNextRepositories(lastEntry, maxEntries) + if err != nil { + return repos, false, err } - subStore := rh.c.StoreController.SubStore - for subPath, imgStore := range subStore { - if subPath != storePath || remainder <= 0 { - continue - } + remainder = maxEntries - len(repos) - var err error + if moreEntries && remainder <= 0 && len(repos) > 0 { + // maxEntries has been hit + lastEntry = repos[len(repos)-1] + } else { + // reset for the next substores + lastEntry = "" + } - var repos []string + combineRepoList = append(combineRepoList, repos...) + } + subStore := rh.c.StoreController.SubStore + for subPath, imgStore := range subStore { + if (remainder > 0 || maxEntries == -1) || subPath == storePath { repos, moreEntries, err = imgStore.GetNextRepositories(lastEntry, remainder) if err != nil { - response.WriteHeader(http.StatusInternalServerError) - - return + return combineRepoList, false, err } // compute remainder remainder = maxEntries - len(repos) - if moreEntries { + if moreEntries && remainder <= 0 && len(repos) > 0 { // maxEntries has been hit lastEntry = repos[len(repos)-1] } else { @@ -1851,65 +1818,39 @@ func (rh *RouteHandler) ListRepositories(response http.ResponseWriter, request * combineRepoList = append(combineRepoList, repos...) } + } - repos := make([]string, 0) - // authz context - userAc, err := reqCtx.UserAcFromContext(request.Context()) - if err != nil { - response.WriteHeader(http.StatusInternalServerError) - - return - } - - if userAc != nil { - for _, r := range combineRepoList { - if userAc.Can(constants.ReadPermission, r) { - repos = append(repos, r) - } - } - } else { - repos = combineRepoList - } - - is := RepositoryList{Repositories: repos} - - zcommon.WriteJSON(response, http.StatusOK, is) + return combineRepoList, moreEntries, nil +} +// ListRepositories godoc +// @Summary List image repositories +// @Description List all image repositories +// @Accept json +// @Produce json +// @Success 200 {object} api.RepositoryList +// @Failure 500 {string} string "internal server error" +// @Router /v2/_catalog [get]. +func (rh *RouteHandler) ListRepositories(response http.ResponseWriter, request *http.Request) { + if request.Method == http.MethodOptions { return } - combineRepoList := make([]string, 0) - - singleStore := rh.c.StoreController.DefaultStore - if singleStore != nil { // route is default - var err error - - var repos []string - - repos, err = singleStore.GetRepositories() - if err != nil { - response.WriteHeader(http.StatusInternalServerError) - - return - } + q := request.URL.Query() - combineRepoList = append(combineRepoList, repos...) + lastEntry := q.Get("last") + maxEntries, err := strconv.Atoi(q.Get("n")) + if err != nil { + maxEntries = -1 } - subStore := rh.c.StoreController.SubStore - for _, imgStore := range subStore { - repos, err := imgStore.GetRepositories() - if err != nil { - response.WriteHeader(http.StatusInternalServerError) - - return - } - - combineRepoList = append(combineRepoList, repos...) + repos, moreEntries, err := rh.listStorageRepositories(lastEntry, maxEntries) + if err != nil { + response.WriteHeader(http.StatusInternalServerError) + return } - repos := make([]string, 0) // authz context userAc, err := reqCtx.UserAcFromContext(request.Context()) if err != nil { @@ -1918,17 +1859,31 @@ func (rh *RouteHandler) ListRepositories(response http.ResponseWriter, request * return } + filteredRepos := make([]string, 0) + if userAc != nil { - for _, r := range combineRepoList { + for _, r := range repos { if userAc.Can(constants.ReadPermission, r) { - repos = append(repos, r) + filteredRepos = append(filteredRepos, r) } } } else { - repos = combineRepoList + filteredRepos = repos + } + + if moreEntries && len(filteredRepos) > 0 { + lastRepo := filteredRepos[len(filteredRepos)-1] + + response.Header().Set( + "Link", + fmt.Sprintf("; rel=\"next\"", + maxEntries, + lastRepo, + ), + ) } - is := RepositoryList{Repositories: repos} + is := RepositoryList{Repositories: filteredRepos} zcommon.WriteJSON(response, http.StatusOK, is) } diff --git a/pkg/api/routes_test.go b/pkg/api/routes_test.go index bc1554db5..483494aed 100644 --- a/pkg/api/routes_test.go +++ b/pkg/api/routes_test.go @@ -1359,8 +1359,8 @@ func TestRoutes(t *testing.T) { "session_id": "test", }, &mocks.MockedImageStore{ - GetRepositoriesFn: func() ([]string, error) { - return []string{}, ErrUnexpectedError + GetNextRepositoriesFn: func(lastRepo string, maxEntries int) ([]string, bool, error) { + return []string{}, false, ErrUnexpectedError }, }, ) @@ -1374,8 +1374,8 @@ func TestRoutes(t *testing.T) { "session_id": "test", }, &mocks.MockedImageStore{ - GetRepositoriesFn: func() ([]string, error) { - return []string{}, ErrUnexpectedError + GetNextRepositoriesFn: func(lastRepo string, maxEntries int) ([]string, bool, error) { + return []string{}, false, ErrUnexpectedError }, }, ) @@ -1384,19 +1384,19 @@ func TestRoutes(t *testing.T) { Convey("ListRepositories with Authz", func() { ctlr.StoreController.DefaultStore = &mocks.MockedImageStore{ - GetRepositoriesFn: func() ([]string, error) { - return []string{"repo"}, nil + GetNextRepositoriesFn: func(lastRepo string, maxEntries int) ([]string, bool, error) { + return []string{"repo"}, false, nil }, } ctlr.StoreController.SubStore = map[string]storageTypes.ImageStore{ "test1": &mocks.MockedImageStore{ - GetRepositoriesFn: func() ([]string, error) { - return []string{"repo1"}, nil + GetNextRepositoriesFn: func(lastRepo string, maxEntries int) ([]string, bool, error) { + return []string{"repo1"}, false, nil }, }, "test2": &mocks.MockedImageStore{ - GetRepositoriesFn: func() ([]string, error) { - return []string{"repo2"}, nil + GetNextRepositoriesFn: func(lastRepo string, maxEntries int) ([]string, bool, error) { + return []string{"repo2"}, false, nil }, }, } diff --git a/pkg/storage/imagestore/imagestore.go b/pkg/storage/imagestore/imagestore.go index 73c4b5dbe..4be0ccddf 100644 --- a/pkg/storage/imagestore/imagestore.go +++ b/pkg/storage/imagestore/imagestore.go @@ -305,7 +305,13 @@ func (is *ImageStore) GetNextRepositories(lastRepo string, maxEntries int) ([]st return nil //nolint:nilerr // ignore invalid repos } - if lastRepo == "" || lastRepo == rel { + if lastRepo == rel { + found = true + + return nil + } + + if lastRepo == "" { found = true }