TrueCloudLab/frostfs-http-gw
17
1
Fork 17
Code Issues 11 Pull requests 7 Projects Releases 9 Activity Actions

[#166] Fix getting s3 object with the FrostFS OID key #173

Open
nzinkevich wants to merge 1 commit from nzinkevich/frostfs-http-gw:fix_path_collision into master
pull from: nzinkevich/frostfs-http-gw:fix_path_collision
merge into: TrueCloudLab:master
Conversation 0 Commits 1 Files changed 13 +161 -188
nzinkevich commented 2024-12-02 08:48:50 +00:00
Member
Copy link

Prioritize getting s3 object with the key, which equals to valid FrostFS OID, rather than getting (potentially non-existent) object with OID via native protocol for GET and HEAD requests.
Also merged byS3Path, byNativeAddress and byAttribute methods with callers because there is no much difference between them after fixing.

Signed-off-by: Nikita Zinkevich n.zinkevich@yadro.com

Prioritize getting s3 object with the key, which equals to valid FrostFS OID, rather than getting (potentially non-existent) object with OID via native protocol for GET and HEAD requests. Also merged `byS3Path`, `byNativeAddress` and `byAttribute` methods with callers because there is no much difference between them after fixing. Signed-off-by: Nikita Zinkevich <n.zinkevich@yadro.com>
nzinkevich self-assigned this 2024-12-02 08:48:50 +00:00
nzinkevich added 1 commit 2024-12-02 08:48:51 +00:00
[#151] Fix getting s3 object with the FrostFS OID name
All checks were successful
/ DCO (pull_request) Successful in 1m33s
Details
/ Vulncheck (pull_request) Successful in 1m49s
Details
/ Builds (pull_request) Successful in 2m2s
Details
/ Lint (pull_request) Successful in 2m36s
Details
/ Tests (pull_request) Successful in 2m2s
Details
bf4182c299 
Prioritize getting s3 object with the key, which equals to valid FrostFS OID, rather than getting non-existent object with OID via native protocol for GET and HEAD requests

Signed-off-by: Nikita Zinkevich <n.zinkevich@yadro.com>
nzinkevich requested review from alexvanin 2024-12-02 08:48:51 +00:00
nzinkevich requested review from dkirillov 2024-12-02 08:48:51 +00:00
nzinkevich requested review from storage-services-committers 2024-12-02 08:50:17 +00:00
nzinkevich requested review from storage-services-developers 2024-12-02 08:50:21 +00:00
r.loginov reviewed 2024-12-02 10:54:33 +00:00
internal/handler/download.go Outdated
@ -35,0 +42,4 @@
return
}
foundOid, err := h.checkS3ObjectExist(ctx, bktInfo, oidParam)
r.loginov commented 2024-12-02 10:54:13 +00:00
Member
Copy link

nitpick: Perhaps you should write the acronym in the same case: foundOid -> foundOID.

nitpick: Perhaps you should write the acronym in the same case: `foundOid` -> `foundOID`.
👍 2
mbiryukova approved these changes 2024-12-02 12:27:23 +00:00
Dismissed
nzinkevich force-pushed fix_path_collision from bf4182c299 to 2e71755d69 2024-12-04 12:12:09 +00:00 Compare
nzinkevich dismissed mbiryukova's review 2024-12-04 12:12:09 +00:00
Reason:

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

nzinkevich changed title from [#151] Fix getting s3 object with the FrostFS OID key to [#166] Fix getting s3 object with the FrostFS OID key 2024-12-04 12:14:36 +00:00
dkirillov reviewed 2024-12-06 13:11:39 +00:00
internal/data/info.go Outdated
@ -0,0 +23,4 @@
Versioning string
LockConfiguration *ObjectLockConfiguration
CannedACL string
OwnerKey *keys.PublicKey
dkirillov commented 2024-12-06 12:09:57 +00:00
Member
Copy link

Why do we need all these fields? We don't use them. And probably we can determine is it correct bucket settings node just by having Versioning KV in tree node

Why do we need all these fields? We don't use them. And probably we can determine is it correct bucket settings node just by having `Versioning` KV in tree node
dkirillov marked this conversation as resolved
internal/data/tree.go Outdated
@ -0,0 +12,4 @@
BaseNodeVersion
DeleteMarker bool
IsPrefixNode bool
IsUnversioned bool
dkirillov commented 2024-12-06 12:54:30 +00:00
Member
Copy link

As I see we don't use these fields

As I see we don't use these fields
dkirillov marked this conversation as resolved
internal/data/tree.go Outdated
@ -0,0 +31,4 @@
// Basically used for "system" object.
type BaseNodeVersion struct {
ID uint64
ParentID uint64
dkirillov commented 2024-12-06 12:56:25 +00:00
Member
Copy link

We don't use these fields

We don't use these fields
dkirillov marked this conversation as resolved
internal/handler/download.go Outdated
@ -36,0 +38,4 @@
}
_, s3checkErr := h.tree.GetSettingsNode(ctx, bktInfo)
if s3checkErr != nil && !strings.Contains(s3checkErr.Error(), "tree not found") {
dkirillov commented 2024-12-06 12:13:31 +00:00
Member
Copy link

We should check error like errors.Is(s3checkErr, tree.ErrNodeNotFound).

We should check error like `errors.Is(s3checkErr, tree.ErrNodeNotFound)`.
nzinkevich commented 2024-12-06 14:03:35 +00:00
Author
Member
Copy link

The problem is that the returned error is not ErrNodeNotFound, but "tree not found", which has not error type in sdk? I will look why it's happening

The problem is that the returned error is not ErrNodeNotFound, but "tree not found", which has not error type in sdk? I will look why it's happening
nzinkevich commented 2024-12-09 10:40:57 +00:00
Author
Member
Copy link

It occurs, that I used wrong error type. Fixed

It occurs, that I used wrong error type. Fixed
dkirillov marked this conversation as resolved
internal/handler/head.go Outdated
@ -111,0 +130,4 @@
} else if err = objID.DecodeString(oidParam); err == nil {
// Head object via native protocol
addr := newAddress(bktInfo.CID, objID)
h.headObject(ctx, h.newRequest(c, log), addr)
dkirillov commented 2024-12-06 13:11:32 +00:00
Member
Copy link

Suggestion: Maybe we can keep handler.byNativeAddress method?

diff --git a/internal/handler/download.go b/internal/handler/download.go
index ce58bf1..27f633d 100644
--- a/internal/handler/download.go
+++ b/internal/handler/download.go
@@ -29,7 +29,10 @@ func (h *Handler) DownloadByAddressOrBucketName(c *fasthttp.RequestCtx) {
 	downloadParam := c.QueryArgs().GetBool("download")
 
 	ctx := utils.GetContextFromRequest(c)
-	log := utils.GetReqLogOrDefault(ctx, h.log)
+	log := utils.GetReqLogOrDefault(ctx, h.log).With(
+		zap.String("cid", cidParam),
+		zap.String("oid", oidParam),
+	)
 
 	bktInfo, err := h.getBucketInfo(ctx, cidParam, log)
 	if err != nil {
@@ -43,14 +46,13 @@ func (h *Handler) DownloadByAddressOrBucketName(c *fasthttp.RequestCtx) {
 		return
 	}
 
+	req := h.newRequest(c, log)
+
 	var objID oid.ID
 	if s3checkErr == nil && shouldDownload(oidParam, downloadParam) {
-		// Receive file via S3
-		h.byS3Path(c, bktInfo.CID, h.receiveFile)
+		h.byS3Path(ctx, req, bktInfo.CID, oidParam, h.receiveFile)
 	} else if err = objID.DecodeString(oidParam); err == nil {
-		// Receive file via native protocol
-		addr := newAddress(bktInfo.CID, objID)
-		h.receiveFile(ctx, h.newRequest(c, log), addr)
+		h.byNativeAddress(ctx, req, bktInfo.CID, objID, h.receiveFile)
 	} else {
 		h.browseIndex(c, s3checkErr != nil)
 	}
diff --git a/internal/handler/handler.go b/internal/handler/handler.go
index acc053e..6f24ff9 100644
--- a/internal/handler/handler.go
+++ b/internal/handler/handler.go
@@ -204,27 +204,26 @@ func (h *Handler) resolveContainer(ctx context.Context, containerID string) (*ci
 	return cnrID, err
 }
 
-func (h *Handler) byS3Path(c *fasthttp.RequestCtx, cnrID cid.ID, handler func(context.Context, request, oid.Address)) {
-	oidParam := c.UserValue("oid").(string)
-	ctx := utils.GetContextFromRequest(c)
-	log := utils.GetReqLogOrDefault(ctx, h.log).With(
-		zap.String("cid", cnrID.EncodeToString()),
-		zap.String("oid", oidParam),
-	)
-
-	foundOID, err := h.tree.GetLatestVersion(ctx, &cnrID, oidParam)
+func (h *Handler) byS3Path(ctx context.Context, req request, cnrID cid.ID, path string, handler func(context.Context, request, oid.Address)) {
+	log := req.log
+	foundOID, err := h.tree.GetLatestVersion(ctx, &cnrID, path)
 	if err != nil {
-		logAndSendBucketError(c, log, err)
+		logAndSendBucketError(req.RequestCtx, log, err)
 		return
 	}
 	if foundOID.IsDeleteMarker {
 		log.Error(logs.ObjectWasDeleted)
-		response.Error(c, "object deleted", fasthttp.StatusNotFound)
+		response.Error(req.RequestCtx, "object deleted", fasthttp.StatusNotFound)
 		return
 	}
 
 	addr := newAddress(cnrID, foundOID.OID)
-	handler(ctx, h.newRequest(c, log), addr)
+	handler(ctx, req, addr)
+}
+
+func (h *Handler) byNativeAddress(ctx context.Context, req request, cnrID cid.ID, objID oid.ID, handler func(context.Context, request, oid.Address)) {
+	addr := newAddress(cnrID, objID)
+	handler(ctx, req, addr)
 }
 
 func (h *Handler) byAttribute(c *fasthttp.RequestCtx, handler func(context.Context, request, oid.Address)) {
diff --git a/internal/handler/head.go b/internal/handler/head.go
index 9d00b20..19bbf89 100644
--- a/internal/handler/head.go
+++ b/internal/handler/head.go
@@ -123,14 +123,13 @@ func (h *Handler) HeadByAddressOrBucketName(c *fasthttp.RequestCtx) {
 		return
 	}
 
+	req := h.newRequest(c, log)
+
 	var objID oid.ID
 	if checkErr == nil {
-		// Head object via s3 protocol
-		h.byS3Path(c, bktInfo.CID, h.headObject)
+		h.byS3Path(ctx, req, bktInfo.CID, oidParam, h.headObject)
 	} else if err = objID.DecodeString(oidParam); err == nil {
-		// Head object via native protocol
-		addr := newAddress(bktInfo.CID, objID)
-		h.headObject(ctx, h.newRequest(c, log), addr)
+		h.byNativeAddress(ctx, req, bktInfo.CID, objID, h.headObject)
 	} else {
 		logAndSendBucketError(c, log, checkErr)
 		return
Suggestion: Maybe we can keep `handler.byNativeAddress` method? ```diff diff --git a/internal/handler/download.go b/internal/handler/download.go index ce58bf1..27f633d 100644 --- a/internal/handler/download.go +++ b/internal/handler/download.go @@ -29,7 +29,10 @@ func (h *Handler) DownloadByAddressOrBucketName(c *fasthttp.RequestCtx) { downloadParam := c.QueryArgs().GetBool("download") ctx := utils.GetContextFromRequest(c) - log := utils.GetReqLogOrDefault(ctx, h.log) + log := utils.GetReqLogOrDefault(ctx, h.log).With( + zap.String("cid", cidParam), + zap.String("oid", oidParam), + ) bktInfo, err := h.getBucketInfo(ctx, cidParam, log) if err != nil { @@ -43,14 +46,13 @@ func (h *Handler) DownloadByAddressOrBucketName(c *fasthttp.RequestCtx) { return } + req := h.newRequest(c, log) + var objID oid.ID if s3checkErr == nil && shouldDownload(oidParam, downloadParam) { - // Receive file via S3 - h.byS3Path(c, bktInfo.CID, h.receiveFile) + h.byS3Path(ctx, req, bktInfo.CID, oidParam, h.receiveFile) } else if err = objID.DecodeString(oidParam); err == nil { - // Receive file via native protocol - addr := newAddress(bktInfo.CID, objID) - h.receiveFile(ctx, h.newRequest(c, log), addr) + h.byNativeAddress(ctx, req, bktInfo.CID, objID, h.receiveFile) } else { h.browseIndex(c, s3checkErr != nil) } diff --git a/internal/handler/handler.go b/internal/handler/handler.go index acc053e..6f24ff9 100644 --- a/internal/handler/handler.go +++ b/internal/handler/handler.go @@ -204,27 +204,26 @@ func (h *Handler) resolveContainer(ctx context.Context, containerID string) (*ci return cnrID, err } -func (h *Handler) byS3Path(c *fasthttp.RequestCtx, cnrID cid.ID, handler func(context.Context, request, oid.Address)) { - oidParam := c.UserValue("oid").(string) - ctx := utils.GetContextFromRequest(c) - log := utils.GetReqLogOrDefault(ctx, h.log).With( - zap.String("cid", cnrID.EncodeToString()), - zap.String("oid", oidParam), - ) - - foundOID, err := h.tree.GetLatestVersion(ctx, &cnrID, oidParam) +func (h *Handler) byS3Path(ctx context.Context, req request, cnrID cid.ID, path string, handler func(context.Context, request, oid.Address)) { + log := req.log + foundOID, err := h.tree.GetLatestVersion(ctx, &cnrID, path) if err != nil { - logAndSendBucketError(c, log, err) + logAndSendBucketError(req.RequestCtx, log, err) return } if foundOID.IsDeleteMarker { log.Error(logs.ObjectWasDeleted) - response.Error(c, "object deleted", fasthttp.StatusNotFound) + response.Error(req.RequestCtx, "object deleted", fasthttp.StatusNotFound) return } addr := newAddress(cnrID, foundOID.OID) - handler(ctx, h.newRequest(c, log), addr) + handler(ctx, req, addr) +} + +func (h *Handler) byNativeAddress(ctx context.Context, req request, cnrID cid.ID, objID oid.ID, handler func(context.Context, request, oid.Address)) { + addr := newAddress(cnrID, objID) + handler(ctx, req, addr) } func (h *Handler) byAttribute(c *fasthttp.RequestCtx, handler func(context.Context, request, oid.Address)) { diff --git a/internal/handler/head.go b/internal/handler/head.go index 9d00b20..19bbf89 100644 --- a/internal/handler/head.go +++ b/internal/handler/head.go @@ -123,14 +123,13 @@ func (h *Handler) HeadByAddressOrBucketName(c *fasthttp.RequestCtx) { return } + req := h.newRequest(c, log) + var objID oid.ID if checkErr == nil { - // Head object via s3 protocol - h.byS3Path(c, bktInfo.CID, h.headObject) + h.byS3Path(ctx, req, bktInfo.CID, oidParam, h.headObject) } else if err = objID.DecodeString(oidParam); err == nil { - // Head object via native protocol - addr := newAddress(bktInfo.CID, objID) - h.headObject(ctx, h.newRequest(c, log), addr) + h.byNativeAddress(ctx, req, bktInfo.CID, objID, h.headObject) } else { logAndSendBucketError(c, log, checkErr) return ```
👍 1
dkirillov marked this conversation as resolved
tree/tree.go Outdated
@ -210,15 +240,6 @@ func (c *Tree) GetVersions(ctx context.Context, cnrID *cid.ID, objectName string
return c.service.GetNodes(ctx, p)
}
func (c *Tree) CheckSettingsNodeExist(ctx context.Context, bktInfo *data.BucketInfo) error {
dkirillov commented 2024-12-06 12:44:10 +00:00
Member
Copy link

Why do we refuse this?

Why do we refuse this?
nzinkevich commented 2024-12-06 14:07:04 +00:00
Author
Member
Copy link

I decided to have similar methods and implementations as s3-gw has. Generally, it would be nice to have some kind of tree service wrappers in sdk in order to avoid copy-pasting similar methods

I decided to have similar methods and implementations as s3-gw has. Generally, it would be nice to have some kind of tree service wrappers in sdk in order to avoid copy-pasting similar methods
dkirillov marked this conversation as resolved
tree/tree.go Outdated
@ -23,3 +27,2 @@
ServiceClient interface {
GetNodes(ctx context.Context, p *GetNodesParams) ([]NodeResponse, error)
GetSubTree(ctx context.Context, bktInfo *data.BucketInfo, treeID string, rootID []uint64, depth uint32, sort bool) ([]NodeResponse, error)
GetNodes(ctx context.Context, p *GetNodesParams) ([]data.NodeResponse, error)
dkirillov commented 2024-12-06 12:49:49 +00:00
Member
Copy link

Why do we move NodeResponse to different package?

Why do we move `NodeResponse` to different package?
nzinkevich commented 2024-12-09 10:42:01 +00:00
Author
Member
Copy link

Well, yes, that wasn't very necessary. Fixed

Well, yes, that wasn't very necessary. Fixed
dkirillov marked this conversation as resolved
nzinkevich force-pushed fix_path_collision from 2e71755d69 to 5e4a68105d 2024-12-09 10:39:18 +00:00 Compare
nzinkevich force-pushed fix_path_collision from 5e4a68105d to 822ab47f1e 2024-12-09 10:42:25 +00:00 Compare
alexvanin reviewed 2024-12-10 12:24:08 +00:00
alexvanin left a comment
Owner
Copy link

All handler package changes seems good to me. Let's discuss two things:

  • GetSettingsNode instead of CheckSettingsNode
  • Do we need s3 mock similar to s3-gw?
All handler package changes seems good to me. Let's discuss two things: - GetSettingsNode instead of CheckSettingsNode - Do we need s3 mock similar to s3-gw?
cmd/http-gw/app.go Outdated
@ -500,3 +500,3 @@
}()
handler := handler.New(a.AppParams(), a.settings, tree.NewTree(frostfs.NewPoolWrapper(a.treePool)), workerPool)
handle := handler.New(a.AppParams(), a.settings, tree.NewTree(frostfs.NewPoolWrapper(a.treePool), a.log), workerPool)
alexvanin commented 2024-12-10 11:31:49 +00:00
Owner
Copy link

This change is to avoid package name collision, right?

This change is to avoid package name collision, right?
👍 1
alexvanin marked this conversation as resolved
internal/data/info.go Outdated
@ -0,0 +18,4 @@
}
// BucketSettings stores settings such as versioning.
type BucketSettings struct {
alexvanin commented 2024-12-10 11:54:20 +00:00
Owner
Copy link

This struct isn't used (as well as VersioningEnabled and VersioningSuspended constants).

I understand that it is introduced to reuse tree service mock from s3-gateway. But I would like to avoid complications in codebase with practically unused structures. Let's discuss it more closely.

/cc @dkirillov

This struct isn't used (as well as VersioningEnabled and VersioningSuspended constants). I understand that it is introduced to reuse tree service mock from s3-gateway. But I would like to avoid complications in codebase with practically unused structures. Let's discuss it more closely. /cc @dkirillov
👍 1
nzinkevich marked this conversation as resolved
internal/handler/handler.go Outdated
@ -324,4 +288,0 @@
// resolveContainer decode container id, if it's not a valid container id
// then trey to resolve name using provided resolver.
func (h *Handler) resolveContainer(ctx context.Context, containerID string) (*cid.ID, error) {
alexvanin commented 2024-12-10 11:44:11 +00:00
Owner
Copy link

nitpick: it would be nice to keep order of the functions the same, so it is easier to review:

  1. byNativeAddress
  2. byS3Path
  3. byAttribute
  4. resolveContainer
nitpick: it would be nice to keep order of the functions the same, so it is easier to review: 1) byNativeAddress 2) byS3Path 3) byAttribute 4) resolveContainer
nzinkevich marked this conversation as resolved
tree/tree.go Outdated
@ -31,0 +31,4 @@
ParentID []uint64
ObjID oid.ID
TimeStamp []uint64
Size uint64
alexvanin commented 2024-12-10 11:37:36 +00:00
Owner
Copy link

Size unused?

Size unused?
nzinkevich marked this conversation as resolved
tree/tree.go Outdated
@ -64,0 +57,4 @@
versioningKV = "Versioning"
cannedACLKV = "cannedACL"
ownerKeyKV = "ownerKey"
lockConfigurationKV = "LockConfiguration"
alexvanin commented 2024-12-10 11:38:36 +00:00
Owner
Copy link

cannedACLKV
ownerKeyKV
lockConfigurationKV

all unused.

cannedACLKV ownerKeyKV lockConfigurationKV all unused.
nzinkevich marked this conversation as resolved
nzinkevich force-pushed fix_path_collision from 822ab47f1e to a6bc8208ae 2024-12-11 08:59:00 +00:00 Compare
nzinkevich force-pushed fix_path_collision from a6bc8208ae to 4d424383b9 2024-12-11 09:05:29 +00:00 Compare
alexvanin added this to the v0.32.0 milestone 2024-12-12 11:57:33 +00:00
All checks were successful
/ DCO (pull_request) Successful in 2m56s
Required
Details
/ Vulncheck (pull_request) Successful in 3m6s
Required
Details
/ Builds (pull_request) Successful in 1m55s
Required
Details
/ Lint (pull_request) Successful in 2m56s
Required
Details
/ Tests (pull_request) Successful in 1m44s
Required
Details
This pull request has changes conflicting with the target branch.
  • internal/service/frostfs/pool_wrapper.go
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u fix_path_collision:nzinkevich-fix_path_collision
git checkout nzinkevich-fix_path_collision
Sign in to join this conversation.
Reviewers
No reviewers
alexvanin
dkirillov
TrueCloudLab/storage-services-committers
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
good first issue

Good for newcomers

  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff

  
blocked

The issue or PR is blocked by something

  
bug

Something is not working

  
config

Configuration format update or breaking change

  
discussion

Talking about something in order to reach a decision or to exchange ideas

  
documentation

Documentation related

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
go

Language features adoption

  
help wanted

Extra attention is needed

  
internal

   
invalid

Something is wrong

  
kludge

This is a kludge and we know it

  
observability

Related to metrics, tracing and logs

  
perfomance

Perfomance related

  
question

More information is needed

  
refactoring

Refactoring

  
wontfix

This won't be fixed

No labels
P0
P1
P2
P3
good first issue
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
Milestone
Clear milestone
No items
No milestone
v0.32.0
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
nzinkevich
5 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-http-gw#173
No description provided.
Delete branch "nzinkevich/frostfs-http-gw:fix_path_collision"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?