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

Merged
alexvanin merged 1 commit from nzinkevich/frostfs-http-gw:fix_path_collision into master 2024-12-17 13:02:37 +00:00
Member

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
/ Vulncheck (pull_request) Successful in 1m49s
/ Builds (pull_request) Successful in 2m2s
/ Lint (pull_request) Successful in 2m36s
/ Tests (pull_request) Successful in 2m2s
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>
requested reviews from alexvanin, dkirillov 2024-12-02 08:48:51 +00:00
requested reviews from storage-services-committers, storage-services-developers 2024-12-02 08:50:21 +00:00
r.loginov reviewed 2024-12-02 10:54:33 +00:00
@ -35,0 +42,4 @@
return
}
foundOid, err := h.checkS3ObjectExist(ctx, bktInfo, oidParam)
Member

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`.
r.loginov marked this conversation as resolved
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
@ -0,0 +23,4 @@
Versioning string
LockConfiguration *ObjectLockConfiguration
CannedACL string
OwnerKey *keys.PublicKey
Member

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
@ -0,0 +12,4 @@
BaseNodeVersion
DeleteMarker bool
IsPrefixNode bool
IsUnversioned bool
Member

As I see we don't use these fields

As I see we don't use these fields
dkirillov marked this conversation as resolved
@ -0,0 +31,4 @@
// Basically used for "system" object.
type BaseNodeVersion struct {
ID uint64
ParentID uint64
Member

We don't use these fields

We don't use these fields
dkirillov marked this conversation as resolved
@ -36,0 +38,4 @@
}
_, s3checkErr := h.tree.GetSettingsNode(ctx, bktInfo)
if s3checkErr != nil && !strings.Contains(s3checkErr.Error(), "tree not found") {
Member

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

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

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
Author
Member

It occurs, that I used wrong error type. Fixed

It occurs, that I used wrong error type. Fixed
dkirillov marked this conversation as resolved
@ -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)
Member

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 ```
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 {
Member

Why do we refuse this?

Why do we refuse this?
Author
Member

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)
Member

Why do we move NodeResponse to different package?

Why do we move `NodeResponse` to different package?
Author
Member

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

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?
@ -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)
Owner

This change is to avoid package name collision, right?

This change is to avoid package name collision, right?
alexvanin marked this conversation as resolved
@ -0,0 +18,4 @@
}
// BucketSettings stores settings such as versioning.
type BucketSettings struct {
Owner

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
nzinkevich marked this conversation as resolved
@ -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) {
Owner

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
Owner

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"
Owner

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
alexvanin approved these changes 2024-12-12 13:49:36 +00:00
Dismissed
r.loginov approved these changes 2024-12-13 03:08:43 +00:00
Dismissed
mbiryukova reviewed 2024-12-13 06:43:25 +00:00
@ -135,1 +121,4 @@
func newNodeVersionFromTreeNode(treeNode *treeNode) *data.NodeVersion {
version := &data.NodeVersion{
BaseNodeVersion: data.BaseNodeVersion{
OID: treeNode.ObjID,
Member

Seems that IsDeleteMarker is used but not set

Seems that `IsDeleteMarker` is used but not set
dkirillov reviewed 2024-12-13 07:50:28 +00:00
@ -0,0 +12,4 @@
type TreeService interface {
GetLatestVersion(ctx context.Context, cnrID *cid.ID, objectName string) (*data.NodeVersion, error)
GetSubTreeByPrefix(ctx context.Context, bktInfo *data.BucketInfo, prefix string, latestOnly bool) ([]tree.NodeResponse, string, error)
CheckSettingsNodeExists(ctx context.Context, bktInfo *data.BucketInfo) error
Member

There are some problems with this TreeService interface:

  • GetSubTreeByPrefix return type from package that contains implementation of this interface (so we've got wrong dependency direction)
  • CheckSettingsNodeExists returns error specific type (that should be mention in comments) from implementation
    in client we use:
import "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/pool/tree"
//...
errors.Is(checkS3Err, tree.ErrNodeNotFound)

This again leads to wrong dependency direction (consumer of this interface starts rely on specific implementation)

We should:

  • Define type similar to tree.NodeResponse somewhere in data package for example (or just in this file)
  • Define error that we expect implementation will return. The same way that was in internal/api/layer/tree_service.go that was removed in this PR
var (		
	// ErrNodeNotFound is returned from Tree service in case of not found error.
	ErrNodeNotFound = errors.New("not found")		

	// ErrNodeAccessDenied is returned from Tree service in case of access denied error.
	ErrNodeAccessDenied = errors.New("access denied")		
)
There are some problems with this `TreeService` interface: * `GetSubTreeByPrefix` return type from package that contains implementation of this interface (so we've got wrong dependency direction) * `CheckSettingsNodeExists` returns error specific type (that should be mention in comments) from implementation in client we use: ```golang import "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/pool/tree" //... errors.Is(checkS3Err, tree.ErrNodeNotFound) ``` This again leads to wrong dependency direction (consumer of this interface starts rely on specific implementation) We should: * Define type similar to `tree.NodeResponse` somewhere in `data` package for example (or just in this file) * Define error that we expect implementation will return. The same way that was in `internal/api/layer/tree_service.go` that was removed in this PR ```golang var ( // ErrNodeNotFound is returned from Tree service in case of not found error. ErrNodeNotFound = errors.New("not found") // ErrNodeAccessDenied is returned from Tree service in case of access denied error. ErrNodeAccessDenied = errors.New("access denied") ) ```
dkirillov marked this conversation as resolved
nzinkevich force-pushed fix_path_collision from 4d424383b9 to 4531d5ebf3 2024-12-13 12:26:16 +00:00 Compare
nzinkevich dismissed alexvanin's review 2024-12-13 12:26:16 +00:00
Reason:

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

nzinkevich dismissed r.loginov's review 2024-12-13 12:26:16 +00:00
Reason:

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

nzinkevich force-pushed fix_path_collision from 4531d5ebf3 to 5eb208caaa 2024-12-16 09:24:55 +00:00 Compare
nzinkevich force-pushed fix_path_collision from 5eb208caaa to 6603b594f5 2024-12-16 11:40:03 +00:00 Compare
alexvanin approved these changes 2024-12-16 13:44:14 +00:00
Dismissed
dkirillov reviewed 2024-12-16 14:06:27 +00:00
@ -354,3 +311,3 @@
if err != nil {
cnrID, err = h.containerResolver.Resolve(ctx, containerID)
if err != nil && strings.Contains(err.Error(), "not found") {
if err != nil && !errors.Is(err, layer.ErrNodeNotFound) {
Member

I don't think we should change this. The reason is the layer.ErrNodeNotFound error from tree service, but resolvers (dns or nns) don't use tree service and can not get such error.

I don't think we should change this. The reason is the `layer.ErrNodeNotFound` error from tree service, but resolvers (dns or nns) don't use tree service and can not get such error.
dkirillov marked this conversation as resolved
nzinkevich force-pushed fix_path_collision from 6603b594f5 to 1be92fa4be 2024-12-17 07:33:37 +00:00 Compare
nzinkevich dismissed alexvanin's review 2024-12-17 07:33:38 +00:00
Reason:

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

dkirillov approved these changes 2024-12-17 08:21:58 +00:00
alexvanin approved these changes 2024-12-17 13:02:31 +00:00
alexvanin merged commit 1be92fa4be into master 2024-12-17 13:02:37 +00:00
alexvanin deleted branch fix_path_collision 2024-12-17 13:02:41 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
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.