bugfix/return_another_error_in_case_name_conflict_and_list_buckets_vhs_master #582

Merged
alexvanin merged 3 commits from r.loginov/frostfs-s3-gw:fix/return_another_error_in_case_name_conflict into master 2024-12-17 12:39:10 +00:00
Member

close #583

close #583
r.loginov self-assigned this 2024-12-16 06:27:43 +00:00
r.loginov added 2 commits 2024-12-16 06:27:43 +00:00
A situation may occur when the global domain is already occupied when creating the container. The error comes from the nns smart contract. This error actually means that the bucket has already been created.

Signed-off-by: Roman Loginov <r.loginov@yadro.com>
[##] Fix list-buckets vhs routing
Some checks failed
/ DCO (pull_request) Failing after 3m45s
/ Vulncheck (pull_request) Successful in 4m9s
/ Builds (pull_request) Successful in 2m8s
/ Lint (pull_request) Successful in 3m10s
/ Tests (pull_request) Successful in 2m12s
78d9a3d235
Signed-off-by: Roman Loginov <r.loginov@yadro.com>
r.loginov force-pushed fix/return_another_error_in_case_name_conflict from 78d9a3d235 to e6f1e38b04 2024-12-16 06:48:27 +00:00 Compare
r.loginov force-pushed fix/return_another_error_in_case_name_conflict from e6f1e38b04 to f7092d2532 2024-12-16 07:58:13 +00:00 Compare
r.loginov changed title from WIP: fix/return_another_error_in_case_name_conflict_and_list_buckets_vhs to bugfix/return_another_error_in_case_name_conflict_and_list_buckets_vhs_master 2024-12-16 07:58:46 +00:00
r.loginov requested review from storage-services-developers 2024-12-16 07:58:47 +00:00
r.loginov requested review from storage-services-committers 2024-12-16 07:58:48 +00:00
r.loginov force-pushed fix/return_another_error_in_case_name_conflict from f7092d2532 to ef1dfe247f 2024-12-16 08:12:58 +00:00 Compare
pogpp approved these changes 2024-12-16 10:27:07 +00:00
Dismissed
Member

Please, fix typo in commit Return BucketAlradyExist when global domain taken

Please, fix typo in commit `Return BucketAlradyExist when global domain taken`
dkirillov reviewed 2024-12-16 11:49:59 +00:00
@ -145,3 +147,3 @@
}
if len(filter.allowedQueries) > 0 {
queries := r.URL.Query()
if len(queries) == 0 {
Member

Is it possible to add some tests? Perfectly with checking listing routing (or at least some unit test).

By the way let's move this check into previous if

if filter.noQueries && (len(queries) > 0 || len(filter.allowedQueries) >0) {
	continue
}

Otherwise, as I understand we get some incorrect behavior for regular listing-v1 (because we don't mark it as NoQuery, add some AllowedQueries and want to match if len(queries)==0) but now it won't match

Is it possible to add some tests? Perfectly with checking listing routing (or at least some unit test). By the way let's move this check into previous `if` ```golang if filter.noQueries && (len(queries) > 0 || len(filter.allowedQueries) >0) { continue } ``` Otherwise, as I understand we get some incorrect behavior for regular listing-v1 (because we don't mark it as `NoQuery`, add some `AllowedQueries` and want to match if `len(queries)==0`) but now it won't match
Author
Member

You are right, however, it turned out that my solution does not cover cases. The problem is that list-buckets can also have query parameters according to the specification, and more importantly, list-objects-v1 and list-buckets have one common query parameter, which can be both there and there. Therefore, if there is only this query parameter when requesting list-buckets, we will also fail in list-objects-v1. Ultimately, if we are talking about VHS, then we can distinguish list-buckets from list-objects-v1 only by the presence of a bucket in the URL. Now I will post a new version of the fixes that takes this into account and also takes into account your scenario (listing-v1 query with len(queries)==0) and as a result, an additional if is not needed.

You are right, however, it turned out that my solution does not cover cases. The problem is that list-buckets can also have query parameters according to the specification, and more importantly, list-objects-v1 and list-buckets have one common query parameter, which can be both there and there. Therefore, if there is only this query parameter when requesting list-buckets, we will also fail in list-objects-v1. Ultimately, if we are talking about VHS, then we can distinguish list-buckets from list-objects-v1 only by the presence of a bucket in the URL. Now I will post a new version of the fixes that takes this into account and also takes into account your scenario (listing-v1 query with `len(queries)==0`) and as a result, an additional `if` is not needed.
Member

Created #585 to support query parameters in ListBuckets

Created https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/issues/585 to support query parameters in `ListBuckets`
r.loginov changed title from bugfix/return_another_error_in_case_name_conflict_and_list_buckets_vhs_master to WIP: bugfix/return_another_error_in_case_name_conflict_and_list_buckets_vhs_master 2024-12-16 12:16:07 +00:00
alexvanin added this to the v0.32.0 milestone 2024-12-16 13:36:40 +00:00
r.loginov force-pushed fix/return_another_error_in_case_name_conflict from ef1dfe247f to 9a8407e8cf 2024-12-16 14:20:42 +00:00 Compare
r.loginov dismissed pogpp's review 2024-12-16 14:20:42 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

r.loginov force-pushed fix/return_another_error_in_case_name_conflict from 9a8407e8cf to fe696ebade 2024-12-16 14:34:37 +00:00 Compare
r.loginov changed title from WIP: bugfix/return_another_error_in_case_name_conflict_and_list_buckets_vhs_master to bugfix/return_another_error_in_case_name_conflict_and_list_buckets_vhs_master 2024-12-16 14:48:51 +00:00
r.loginov requested review from storage-services-developers 2024-12-16 14:48:52 +00:00
r.loginov requested review from storage-services-committers 2024-12-16 14:48:52 +00:00
r.loginov changed title from bugfix/return_another_error_in_case_name_conflict_and_list_buckets_vhs_master to WIP: bugfix/return_another_error_in_case_name_conflict_and_list_buckets_vhs_master 2024-12-17 07:44:51 +00:00
dkirillov reviewed 2024-12-17 08:18:47 +00:00
api/router.go Outdated
@ -341,2 +339,2 @@
Add(NewFilter().
NoQueries().
s3middleware.QueryMarker, s3middleware.QueryEncodingType, s3middleware.QueryBucketRegion,
s3middleware.QueryContinuationToken, s3middleware.QueryMaxBuckets).
Member

Can we consider something like this? I think we should keep "bucket" router only bucket related operations. ListBuckets doesn't belong to it

diff --git a/api/router.go b/api/router.go
index e3a744ed..9046b5ec 100644
--- a/api/router.go
+++ b/api/router.go
@@ -159,7 +159,7 @@ func NewRouter(cfg Config) *chi.Mux {
 	defaultRouter.Get("/", named(s3middleware.ListBucketsOperation, cfg.Handler.ListBucketsHandler))
 	attachErrorHandler(defaultRouter)
 
-	vhsRouter := bucketRouter(cfg.Handler)
+	vhsRouter := newDomainRouter(cfg.Handler)
 	router := newGlobalRouter(defaultRouter, vhsRouter)
 
 	api.Mount("/", router)
@@ -169,12 +169,43 @@ func NewRouter(cfg Config) *chi.Mux {
 	return api
 }
 
+type domainRouter struct {
+	bucketRouter  chi.Router
+	defaultRouter chi.Router
+}
+
+func newDomainRouter(handler Handler) *domainRouter {
+	defaultRouter := chi.NewRouter()
+	defaultRouter.Group(func(r chi.Router) {
+		r.Method(http.MethodGet, "/", NewHandlerFilter().
+			Add(NewFilter().
+				AllowedQueries(s3middleware.QueryMaxBuckets, s3middleware.QueryPrefix,
+					s3middleware.QueryContinuationToken, s3middleware.QueryBucketRegion).
+				Handler(named(s3middleware.ListBucketsOperation, handler.ListBucketsHandler))).
+			DefaultHandler(notSupportedHandler()))
+	})
+	attachErrorHandler(defaultRouter)
+
+	return &domainRouter{
+		bucketRouter:  bucketRouter(handler),
+		defaultRouter: defaultRouter,
+	}
+}
+
+func (g *domainRouter) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+	if reqInfo := s3middleware.GetReqInfo(r.Context()); reqInfo.BucketName != "" {
+		g.bucketRouter.ServeHTTP(w, r)
+	} else {
+		g.defaultRouter.ServeHTTP(w, r)
+	}
+}
+
 type globalRouter struct {
 	pathStyleRouter chi.Router
-	vhsRouter       chi.Router
+	vhsRouter       *domainRouter
 }
 
-func newGlobalRouter(pathStyleRouter, vhsRouter chi.Router) *globalRouter {
+func newGlobalRouter(pathStyleRouter chi.Router, vhsRouter *domainRouter) *globalRouter {
 	return &globalRouter{
 		pathStyleRouter: pathStyleRouter,
 		vhsRouter:       vhsRouter,
@@ -182,12 +213,11 @@ func newGlobalRouter(pathStyleRouter, vhsRouter chi.Router) *globalRouter {
 }
 
 func (g *globalRouter) ServeHTTP(w http.ResponseWriter, r *http.Request) {
-	router := g.pathStyleRouter
 	if reqInfo := s3middleware.GetReqInfo(r.Context()); reqInfo.RequestVHSEnabled {
-		router = g.vhsRouter
+		g.vhsRouter.ServeHTTP(w, r)
+	} else {
+		g.pathStyleRouter.ServeHTTP(w, r)
 	}
-
-	router.ServeHTTP(w, r)
 }
 
 func named(name string, handlerFunc http.HandlerFunc) http.HandlerFunc {
@@ -336,9 +366,8 @@ func bucketRouter(h Handler) chi.Router {
 				Handler(named(s3middleware.ListBucketObjectVersionsOperation, h.ListBucketObjectVersionsHandler))).
 			Add(NewFilter().
 				AllowedQueries(s3middleware.QueryDelimiter, s3middleware.QueryMaxKeys, s3middleware.QueryPrefix,
-					s3middleware.QueryMarker, s3middleware.QueryEncodingType, s3middleware.QueryBucketRegion,
-					s3middleware.QueryContinuationToken, s3middleware.QueryMaxBuckets).
-				Handler(listWrapper(h))).
+					s3middleware.QueryMarker, s3middleware.QueryEncodingType).
+				Handler(named(s3middleware.ListObjectsV1Operation, h.ListObjectsV1Handler))).
 			DefaultHandler(notSupportedHandler()))
 	})
 

Can we consider something like this? I think we should keep "bucket" router only bucket related operations. `ListBuckets` doesn't belong to it ```diff diff --git a/api/router.go b/api/router.go index e3a744ed..9046b5ec 100644 --- a/api/router.go +++ b/api/router.go @@ -159,7 +159,7 @@ func NewRouter(cfg Config) *chi.Mux { defaultRouter.Get("/", named(s3middleware.ListBucketsOperation, cfg.Handler.ListBucketsHandler)) attachErrorHandler(defaultRouter) - vhsRouter := bucketRouter(cfg.Handler) + vhsRouter := newDomainRouter(cfg.Handler) router := newGlobalRouter(defaultRouter, vhsRouter) api.Mount("/", router) @@ -169,12 +169,43 @@ func NewRouter(cfg Config) *chi.Mux { return api } +type domainRouter struct { + bucketRouter chi.Router + defaultRouter chi.Router +} + +func newDomainRouter(handler Handler) *domainRouter { + defaultRouter := chi.NewRouter() + defaultRouter.Group(func(r chi.Router) { + r.Method(http.MethodGet, "/", NewHandlerFilter(). + Add(NewFilter(). + AllowedQueries(s3middleware.QueryMaxBuckets, s3middleware.QueryPrefix, + s3middleware.QueryContinuationToken, s3middleware.QueryBucketRegion). + Handler(named(s3middleware.ListBucketsOperation, handler.ListBucketsHandler))). + DefaultHandler(notSupportedHandler())) + }) + attachErrorHandler(defaultRouter) + + return &domainRouter{ + bucketRouter: bucketRouter(handler), + defaultRouter: defaultRouter, + } +} + +func (g *domainRouter) ServeHTTP(w http.ResponseWriter, r *http.Request) { + if reqInfo := s3middleware.GetReqInfo(r.Context()); reqInfo.BucketName != "" { + g.bucketRouter.ServeHTTP(w, r) + } else { + g.defaultRouter.ServeHTTP(w, r) + } +} + type globalRouter struct { pathStyleRouter chi.Router - vhsRouter chi.Router + vhsRouter *domainRouter } -func newGlobalRouter(pathStyleRouter, vhsRouter chi.Router) *globalRouter { +func newGlobalRouter(pathStyleRouter chi.Router, vhsRouter *domainRouter) *globalRouter { return &globalRouter{ pathStyleRouter: pathStyleRouter, vhsRouter: vhsRouter, @@ -182,12 +213,11 @@ func newGlobalRouter(pathStyleRouter, vhsRouter chi.Router) *globalRouter { } func (g *globalRouter) ServeHTTP(w http.ResponseWriter, r *http.Request) { - router := g.pathStyleRouter if reqInfo := s3middleware.GetReqInfo(r.Context()); reqInfo.RequestVHSEnabled { - router = g.vhsRouter + g.vhsRouter.ServeHTTP(w, r) + } else { + g.pathStyleRouter.ServeHTTP(w, r) } - - router.ServeHTTP(w, r) } func named(name string, handlerFunc http.HandlerFunc) http.HandlerFunc { @@ -336,9 +366,8 @@ func bucketRouter(h Handler) chi.Router { Handler(named(s3middleware.ListBucketObjectVersionsOperation, h.ListBucketObjectVersionsHandler))). Add(NewFilter(). AllowedQueries(s3middleware.QueryDelimiter, s3middleware.QueryMaxKeys, s3middleware.QueryPrefix, - s3middleware.QueryMarker, s3middleware.QueryEncodingType, s3middleware.QueryBucketRegion, - s3middleware.QueryContinuationToken, s3middleware.QueryMaxBuckets). - Handler(listWrapper(h))). + s3middleware.QueryMarker, s3middleware.QueryEncodingType). + Handler(named(s3middleware.ListObjectsV1Operation, h.ListObjectsV1Handler))). DefaultHandler(notSupportedHandler())) }) ```
r.loginov force-pushed fix/return_another_error_in_case_name_conflict from fe696ebade to 4ef22aa46d 2024-12-17 10:54:18 +00:00 Compare
r.loginov changed title from WIP: bugfix/return_another_error_in_case_name_conflict_and_list_buckets_vhs_master to bugfix/return_another_error_in_case_name_conflict_and_list_buckets_vhs_master 2024-12-17 11:03:30 +00:00
alexvanin approved these changes 2024-12-17 12:38:53 +00:00
alexvanin merged commit e0ce59fd32 into master 2024-12-17 12:39:10 +00:00
alexvanin deleted branch fix/return_another_error_in_case_name_conflict 2024-12-17 12:39:11 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-s3-gw#582
No description provided.