From 0ba85e177b64871b414720364260c6d204131a38 Mon Sep 17 00:00:00 2001 From: Nikita Zinkevich Date: Thu, 19 Dec 2024 12:25:10 +0300 Subject: [PATCH 1/2] [#585] Add ListBuckets pagination Signed-off-by: Nikita Zinkevich --- api/handler/bucket_list.go | 70 ++++++++++++++++++++++++++++++++++++++ api/handler/list.go | 49 -------------------------- api/handler/object_list.go | 2 ++ api/handler/response.go | 8 +++-- api/layer/container.go | 30 ++++++++++++++-- api/layer/layer.go | 32 +++++++++++++++-- 6 files changed, 136 insertions(+), 55 deletions(-) create mode 100644 api/handler/bucket_list.go delete mode 100644 api/handler/list.go diff --git a/api/handler/bucket_list.go b/api/handler/bucket_list.go new file mode 100644 index 00000000..e060b676 --- /dev/null +++ b/api/handler/bucket_list.go @@ -0,0 +1,70 @@ +package handler + +import ( + "net/http" + "strconv" + "time" + + "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/errors" + "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/layer" + "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/middleware" +) + +const maxBucketList = 10000 + +// ListBucketsHandler handles bucket listing requests. +func (h *handler) ListBucketsHandler(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() + reqInfo := middleware.GetReqInfo(ctx) + + params, err := parseListBucketParams(r) + if err != nil { + h.logAndSendError(ctx, w, "failed to parse params", reqInfo, err) + return + } + + resp, err := h.obj.ListBuckets(ctx, params) + if err != nil { + h.logAndSendError(ctx, w, "something went wrong", reqInfo, err) + return + } + + if err = middleware.EncodeToResponse(w, encodeListBuckets(reqInfo.User, resp, params)); err != nil { + h.logAndSendError(ctx, w, "something went wrong", reqInfo, err) + } +} + +func encodeListBuckets(owner string, resp layer.ListBucketsResult, params layer.ListBucketsParams) *ListBucketsResponse { + res := &ListBucketsResponse{ + Owner: Owner{ + ID: owner, + DisplayName: owner, + }, + ContinuationToken: resp.ContinuationToken, + Prefix: params.Prefix, + } + + for _, item := range resp.Containers { + res.Buckets.Buckets = append(res.Buckets.Buckets, Bucket{ + Name: item.Name, + CreationDate: item.Created.UTC().Format(time.RFC3339), + BucketRegion: item.LocationConstraint, + }) + } + return res +} + +func parseListBucketParams(r *http.Request) (prm layer.ListBucketsParams, err error) { + prm.MaxBuckets = maxBucketList + strMaxBuckets := r.URL.Query().Get(middleware.QueryMaxBuckets) + if strMaxBuckets != "" { + if prm.MaxBuckets, err = strconv.Atoi(strMaxBuckets); err != nil || prm.MaxBuckets < 0 { + return layer.ListBucketsParams{}, errors.GetAPIError(errors.ErrInvalidMaxKeys) + } + } + prm.Prefix = r.URL.Query().Get(middleware.QueryPrefix) + prm.BucketRegion = r.URL.Query().Get(middleware.QueryBucketRegion) + prm.ContinuationToken = r.URL.Query().Get(middleware.QueryContinuationToken) + + return +} diff --git a/api/handler/list.go b/api/handler/list.go deleted file mode 100644 index f6bfae3a..00000000 --- a/api/handler/list.go +++ /dev/null @@ -1,49 +0,0 @@ -package handler - -import ( - "net/http" - "time" - - "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/middleware" - "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/user" -) - -const maxObjectList = 1000 // Limit number of objects in a listObjectsResponse/listObjectsVersionsResponse. - -// ListBucketsHandler handles bucket listing requests. -func (h *handler) ListBucketsHandler(w http.ResponseWriter, r *http.Request) { - var ( - own user.ID - res *ListBucketsResponse - ctx = r.Context() - reqInfo = middleware.GetReqInfo(ctx) - ) - - list, err := h.obj.ListBuckets(ctx) - if err != nil { - h.logAndSendError(ctx, w, "something went wrong", reqInfo, err) - return - } - - if len(list) > 0 { - own = list[0].Owner - } - - res = &ListBucketsResponse{ - Owner: Owner{ - ID: own.String(), - DisplayName: own.String(), - }, - } - - for _, item := range list { - res.Buckets.Buckets = append(res.Buckets.Buckets, Bucket{ - Name: item.Name, - CreationDate: item.Created.UTC().Format(time.RFC3339), - }) - } - - if err = middleware.EncodeToResponse(w, res); err != nil { - h.logAndSendError(ctx, w, "something went wrong", reqInfo, err) - } -} diff --git a/api/handler/object_list.go b/api/handler/object_list.go index 7c20f37d..33183966 100644 --- a/api/handler/object_list.go +++ b/api/handler/object_list.go @@ -15,6 +15,8 @@ import ( oid "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object/id" ) +const maxObjectList = 1000 // Limit number of objects in a listObjectsResponse/listObjectsVersionsResponse. + // ListObjectsV1Handler handles objects listing requests for API version 1. func (h *handler) ListObjectsV1Handler(w http.ResponseWriter, r *http.Request) { ctx := r.Context() diff --git a/api/handler/response.go b/api/handler/response.go index 8654d8d9..eb21b2af 100644 --- a/api/handler/response.go +++ b/api/handler/response.go @@ -15,6 +15,9 @@ type ListBucketsResponse struct { Buckets struct { Buckets []Bucket `xml:"Bucket"` } // Buckets are nested + + ContinuationToken string `xml:"ContinuationToken,omitempty"` + Prefix string `xml:"Prefix,omitempty"` } // ListObjectsV1Response -- format for ListObjectsV1 response. @@ -51,8 +54,9 @@ type ListObjectsV2Response struct { // Bucket container for bucket metadata. type Bucket struct { - Name string - CreationDate string // time string of format "2006-01-02T15:04:05.000Z" + Name string `xml:"Name"` + CreationDate string `xml:"CreationDate"` // time string of format "2006-01-02T15:04:05.000Z" + BucketRegion string `xml:"BucketRegion,omitempty"` } // PolicyStatus contains status of bucket policy. diff --git a/api/layer/container.go b/api/layer/container.go index ebb23d76..cc9346eb 100644 --- a/api/layer/container.go +++ b/api/layer/container.go @@ -3,7 +3,9 @@ package layer import ( "context" "fmt" + "sort" "strconv" + "strings" "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api" "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/data" @@ -76,7 +78,7 @@ func (n *Layer) containerInfo(ctx context.Context, prm frostfs.PrmContainer) (*d return info, nil } -func (n *Layer) containerList(ctx context.Context) ([]*data.BucketInfo, error) { +func (n *Layer) containerList(ctx context.Context, listParams ListBucketsParams) ([]*data.BucketInfo, error) { stoken := n.SessionTokenForRead(ctx) prm := frostfs.PrmUserContainers{ @@ -102,10 +104,34 @@ func (n *Layer) containerList(ctx context.Context) ([]*data.BucketInfo, error) { continue } + if shouldSkipBucket(info, listParams) { + continue + } + list = append(list, info) } - return list, nil + sort.Slice(list, func(i, j int) bool { + return list[i].Name < list[j].Name + }) + + for i, info := range list { + if listParams.ContinuationToken != "" && info.Name != listParams.ContinuationToken { + continue + } + return list[i:], nil + } + + return nil, nil +} + +func shouldSkipBucket(info *data.BucketInfo, prm ListBucketsParams) bool { + if !strings.HasPrefix(info.Name, prm.Prefix) || + (prm.BucketRegion != "" && info.LocationConstraint != prm.BucketRegion) { + return true + } + + return false } func (n *Layer) createContainer(ctx context.Context, p *CreateBucketParams) (*data.BucketInfo, error) { diff --git a/api/layer/layer.go b/api/layer/layer.go index bd7e7d8c..7b2d8ec3 100644 --- a/api/layer/layer.go +++ b/api/layer/layer.go @@ -195,6 +195,18 @@ type ( Encode string } + ListBucketsParams struct { + MaxBuckets int + Prefix string + ContinuationToken string + BucketRegion string + } + + ListBucketsResult struct { + Containers []*data.BucketInfo + ContinuationToken string + } + // VersionedObject stores info about objects to delete. VersionedObject struct { Name string @@ -371,8 +383,24 @@ func (n *Layer) ResolveCID(ctx context.Context, name string) (cid.ID, error) { // ListBuckets returns all user containers. The name of the bucket is a container // id. Timestamp is omitted since it is not saved in frostfs container. -func (n *Layer) ListBuckets(ctx context.Context) ([]*data.BucketInfo, error) { - return n.containerList(ctx) +func (n *Layer) ListBuckets(ctx context.Context, params ListBucketsParams) (ListBucketsResult, error) { + var result ListBucketsResult + var err error + + if params.MaxBuckets == 0 { + return result, nil + } + + result.Containers, err = n.containerList(ctx, params) + if err != nil { + return ListBucketsResult{}, err + } + if len(result.Containers) > params.MaxBuckets { + result.ContinuationToken = result.Containers[params.MaxBuckets].Name + result.Containers = result.Containers[:params.MaxBuckets] + } + + return result, nil } // GetObject from storage. -- 2.45.2 From 7dd308b466bdfbb825d291dd1f49fbffc84de11f Mon Sep 17 00:00:00 2001 From: Nikita Zinkevich Date: Thu, 19 Dec 2024 17:14:22 +0300 Subject: [PATCH 2/2] [#585] Add ListBuckets handler test Modify containers field in TestFrostFS in order to get determined order of containers between test runs Signed-off-by: Nikita Zinkevich --- api/handler/acl_test.go | 37 ++++++- api/handler/bucket_list_test.go | 174 ++++++++++++++++++++++++++++++++ api/handler/handlers_test.go | 13 ++- 3 files changed, 216 insertions(+), 8 deletions(-) create mode 100644 api/handler/bucket_list_test.go diff --git a/api/handler/acl_test.go b/api/handler/acl_test.go index 14b3cf4e..2533ca56 100644 --- a/api/handler/acl_test.go +++ b/api/handler/acl_test.go @@ -7,6 +7,7 @@ import ( "encoding/xml" "net/http" "net/http/httptest" + "net/url" "testing" "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api" @@ -297,10 +298,17 @@ type createBucketInfo struct { Key *keys.PrivateKey } +type bucketPrm struct { + bktName string + query url.Values + box *accessbox.Box + createParams createBucketParams +} + func createBucket(hc *handlerContext, bktName string) *createBucketInfo { box, key := createAccessBox(hc.t) - w := createBucketBase(hc, bktName, box) + w := createBucketBase(hc, bucketPrm{bktName: bktName, box: box}) assertStatus(hc.t, w, http.StatusOK) bktInfo, err := hc.Layer().GetBucketInfo(hc.Context(), bktName) @@ -314,13 +322,32 @@ func createBucket(hc *handlerContext, bktName string) *createBucketInfo { } func createBucketAssertS3Error(hc *handlerContext, bktName string, box *accessbox.Box, code apierr.ErrorCode) { - w := createBucketBase(hc, bktName, box) + w := createBucketBase(hc, bucketPrm{bktName: bktName, box: box}) assertS3Error(hc.t, w, apierr.GetAPIError(code)) } -func createBucketBase(hc *handlerContext, bktName string, box *accessbox.Box) *httptest.ResponseRecorder { - w, r := prepareTestRequest(hc, bktName, "", nil) - ctx := middleware.SetBox(r.Context(), &middleware.Box{AccessBox: box}) +func createBucketWithConstraint(hc *handlerContext, bktName, constraint string) *createBucketInfo { + box, key := createAccessBox(hc.t) + var prm createBucketParams + if constraint != "" { + prm.LocationConstraint = constraint + } + w := createBucketBase(hc, bucketPrm{bktName: bktName, box: box, createParams: prm}) + assertStatus(hc.t, w, http.StatusOK) + + bktInfo, err := hc.Layer().GetBucketInfo(hc.Context(), bktName) + require.NoError(hc.t, err) + + return &createBucketInfo{ + BktInfo: bktInfo, + Box: box, + Key: key, + } +} + +func createBucketBase(hc *handlerContext, prm bucketPrm) *httptest.ResponseRecorder { + w, r := prepareTestFullRequest(hc, prm.bktName, "", nil, prm.createParams) + ctx := middleware.SetBox(r.Context(), &middleware.Box{AccessBox: prm.box}) r = r.WithContext(ctx) hc.Handler().CreateBucketHandler(w, r) return w diff --git a/api/handler/bucket_list_test.go b/api/handler/bucket_list_test.go new file mode 100644 index 00000000..8ac2a123 --- /dev/null +++ b/api/handler/bucket_list_test.go @@ -0,0 +1,174 @@ +package handler + +import ( + "encoding/xml" + "net/http" + "net/http/httptest" + "net/url" + "sort" + "testing" + + apierr "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/errors" + "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/middleware" + "github.com/stretchr/testify/require" +) + +func TestHandler_ListBucketsHandler(t *testing.T) { + const defaultConstraint = "default" + + region := "us-west-1" + hc := prepareHandlerContext(t) + hc.config.putLocationConstraint(region) + + props := []Bucket{ + {Name: "first"}, + {Name: "regional", BucketRegion: "us-west-1"}, + {Name: "third"}, + } + sort.Slice(props, func(i, j int) bool { + return props[i].Name < props[j].Name + }) + for _, bkt := range props { + createBucketWithConstraint(hc, bkt.Name, bkt.BucketRegion) + } + + for _, tt := range []struct { + title string + token string + prefix string + bucketRegion string + maxBuckets string + expectErr bool + expected []Bucket + expectedToken string + }{ + { + title: "no params", + expected: []Bucket{ + {Name: "first", BucketRegion: defaultConstraint}, + {Name: "regional", BucketRegion: "us-west-1"}, + {Name: "third", BucketRegion: defaultConstraint}, + }, + }, + { + title: "negative max-buckets", + maxBuckets: "-1", + expected: []Bucket{}, + expectErr: true, + }, + { + title: "zero max-buckets", + maxBuckets: "0", + expected: []Bucket{}, + }, + { + title: "prefix", + prefix: "thi", + expected: []Bucket{{Name: "third", BucketRegion: defaultConstraint}}, + }, + { + title: "wrong prefix", + prefix: "sdh", + expected: []Bucket{}, + }, + { + title: "bucket region", + bucketRegion: region, + expected: []Bucket{{Name: "regional", BucketRegion: "us-west-1"}}, + }, + { + title: "default bucket region", + bucketRegion: defaultConstraint, + expected: []Bucket{ + {Name: "first", BucketRegion: defaultConstraint}, + {Name: "third", BucketRegion: defaultConstraint}, + }, + }, + { + title: "wrong bucket region", + bucketRegion: "sj dfdlsj", + expected: []Bucket{}, + }, + } { + t.Run(tt.title, func(t *testing.T) { + if tt.expectErr { + listBucketsErr(hc, tt.prefix, tt.token, tt.bucketRegion, tt.maxBuckets, apierr.GetAPIError(apierr.ErrInvalidMaxKeys)) + return + } + + resp := listBuckets(hc, tt.prefix, tt.token, tt.bucketRegion, tt.maxBuckets) + require.Len(t, resp.Buckets.Buckets, len(tt.expected)) + require.Equal(t, tt.prefix, resp.Prefix) + require.Equal(t, hc.owner.String(), resp.Owner.ID) + if len(resp.Buckets.Buckets) > 0 { + t.Log(resp.Buckets.Buckets[0].Name) + } + for i, bkt := range resp.Buckets.Buckets { + require.Equal(t, tt.expected[i].Name, bkt.Name) + require.Equal(t, tt.expected[i].BucketRegion, bkt.BucketRegion) + } + }) + } + + t.Run("pagination", func(t *testing.T) { + t.Run("happy path", func(t *testing.T) { + resp := listBuckets(hc, "", "", "", "1") + require.Len(t, resp.Buckets.Buckets, 1) + require.Equal(t, props[0].Name, resp.Buckets.Buckets[0].Name) + require.NotEmpty(t, resp.ContinuationToken) + + resp = listBuckets(hc, "", resp.ContinuationToken, "", "1") + require.Len(t, resp.Buckets.Buckets, 1) + require.Equal(t, props[1].Name, resp.Buckets.Buckets[0].Name) + require.NotEmpty(t, resp.ContinuationToken) + + resp = listBuckets(hc, "", resp.ContinuationToken, "", "1") + require.Len(t, resp.Buckets.Buckets, 1) + require.Equal(t, props[2].Name, resp.Buckets.Buckets[0].Name) + require.Empty(t, resp.ContinuationToken) + }) + + t.Run("wrong continuation-token", func(t *testing.T) { + resp := listBuckets(hc, "", "CebuVwfRpdMqi9dvgV2SUNbrkfteGtudchKKhNabXUu9", "", "1") + require.Len(t, resp.Buckets.Buckets, 0) + require.Empty(t, resp.ContinuationToken) + }) + }) +} + +func listBuckets(hc *handlerContext, prefix, token, bucketRegion, maxBuckets string) ListBucketsResponse { + query := url.Values{ + middleware.QueryPrefix: []string{prefix}, + middleware.QueryContinuationToken: []string{token}, + middleware.QueryBucketRegion: []string{bucketRegion}, + middleware.QueryMaxBuckets: []string{maxBuckets}, + } + w := listBucketsBase(hc, bucketPrm{query: query}) + assertStatus(hc.t, w, http.StatusOK) + var resp ListBucketsResponse + err := xml.NewDecoder(w.Body).Decode(&resp) + require.NoError(hc.t, err) + + return resp +} + +func listBucketsErr(hc *handlerContext, prefix, token, bucketRegion, maxBuckets string, err apierr.Error) { + query := url.Values{ + middleware.QueryPrefix: []string{prefix}, + middleware.QueryContinuationToken: []string{token}, + middleware.QueryBucketRegion: []string{bucketRegion}, + middleware.QueryMaxBuckets: []string{maxBuckets}, + } + w := listBucketsBase(hc, bucketPrm{query: query}) + assertS3Error(hc.t, w, err) +} + +func listBucketsBase(hc *handlerContext, prm bucketPrm) *httptest.ResponseRecorder { + box, _ := createAccessBox(hc.t) + w, r := prepareTestFullRequest(hc, "", "", prm.query, nil) + ctx := middleware.SetBox(r.Context(), &middleware.Box{AccessBox: box}) + r = r.WithContext(ctx) + hc.Handler().ListBucketsHandler(w, r) + + return w +} diff --git a/api/handler/handlers_test.go b/api/handler/handlers_test.go index 7b73e519..78de4393 100644 --- a/api/handler/handlers_test.go +++ b/api/handler/handlers_test.go @@ -74,6 +74,7 @@ func (hc *handlerContextBase) Context() context.Context { type configMock struct { defaultPolicy netmap.PlacementPolicy + placementPolicies map[string]netmap.PlacementPolicy copiesNumbers map[string][]uint32 defaultCopiesNumbers []uint32 bypassContentEncodingInChunks bool @@ -86,8 +87,9 @@ func (c *configMock) DefaultPlacementPolicy(_ string) netmap.PlacementPolicy { return c.defaultPolicy } -func (c *configMock) PlacementPolicy(_, _ string) (netmap.PlacementPolicy, bool) { - return netmap.PlacementPolicy{}, false +func (c *configMock) PlacementPolicy(_, constraint string) (netmap.PlacementPolicy, bool) { + policy, ok := c.placementPolicies[constraint] + return policy, ok } func (c *configMock) CopiesNumbers(_, locationConstraint string) ([]uint32, bool) { @@ -151,6 +153,10 @@ func (c *configMock) TLSTerminationHeader() string { return c.tlsTerminationHeader } +func (c *configMock) putLocationConstraint(constraint string) { + c.placementPolicies[constraint] = c.defaultPolicy +} + func prepareHandlerContext(t *testing.T) *handlerContext { hc, err := prepareHandlerContextBase(layer.DefaultCachesConfigs(zap.NewExample())) require.NoError(t, err) @@ -217,7 +223,8 @@ func prepareHandlerContextBase(cacheCfg *layer.CachesConfig) (*handlerContextBas } cfg := &configMock{ - defaultPolicy: pp, + defaultPolicy: pp, + placementPolicies: make(map[string]netmap.PlacementPolicy), } h := &handler{ log: log, -- 2.45.2