[#166] Fix getting s3 object with the FrostFS OID key #173
Labels
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
No project
No assignees
5 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-http-gw#173
Loading…
Add table
Reference in a new issue
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?
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
andbyAttribute
methods with callers because there is no much difference between them after fixing.Signed-off-by: Nikita Zinkevich n.zinkevich@yadro.com
@ -35,0 +42,4 @@
return
}
foundOid, err := h.checkS3ObjectExist(ctx, bktInfo, oidParam)
nitpick: Perhaps you should write the acronym in the same case:
foundOid
->foundOID
.bf4182c299
to2e71755d69
New commits pushed, approval review dismissed automatically according to repository settings
[#151] Fix getting s3 object with the FrostFS OID keyto [#166] Fix getting s3 object with the FrostFS OID key@ -0,0 +23,4 @@
Versioning string
LockConfiguration *ObjectLockConfiguration
CannedACL string
OwnerKey *keys.PublicKey
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@ -0,0 +12,4 @@
BaseNodeVersion
DeleteMarker bool
IsPrefixNode bool
IsUnversioned bool
As I see we don't use these fields
@ -0,0 +31,4 @@
// Basically used for "system" object.
type BaseNodeVersion struct {
ID uint64
ParentID uint64
We don't use these fields
@ -36,0 +38,4 @@
}
_, s3checkErr := h.tree.GetSettingsNode(ctx, bktInfo)
if s3checkErr != nil && !strings.Contains(s3checkErr.Error(), "tree not found") {
We should check error like
errors.Is(s3checkErr, tree.ErrNodeNotFound)
.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
It occurs, that I used wrong error type. Fixed
@ -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)
Suggestion: Maybe we can keep
handler.byNativeAddress
method?@ -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 {
Why do we refuse this?
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
@ -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)
Why do we move
NodeResponse
to different package?Well, yes, that wasn't very necessary. Fixed
2e71755d69
to5e4a68105d
5e4a68105d
to822ab47f1e
All handler package changes seems good to me. Let's discuss two things:
@ -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)
This change is to avoid package name collision, right?
@ -0,0 +18,4 @@
}
// BucketSettings stores settings such as versioning.
type BucketSettings struct {
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
@ -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) {
nitpick: it would be nice to keep order of the functions the same, so it is easier to review:
@ -31,0 +31,4 @@
ParentID []uint64
ObjID oid.ID
TimeStamp []uint64
Size uint64
Size unused?
@ -64,0 +57,4 @@
versioningKV = "Versioning"
cannedACLKV = "cannedACL"
ownerKeyKV = "ownerKey"
lockConfigurationKV = "LockConfiguration"
cannedACLKV
ownerKeyKV
lockConfigurationKV
all unused.
822ab47f1e
toa6bc8208ae
a6bc8208ae
to4d424383b9
@ -135,1 +121,4 @@
func newNodeVersionFromTreeNode(treeNode *treeNode) *data.NodeVersion {
version := &data.NodeVersion{
BaseNodeVersion: data.BaseNodeVersion{
OID: treeNode.ObjID,
Seems that
IsDeleteMarker
is used but not set@ -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
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 implementationin client we use:
This again leads to wrong dependency direction (consumer of this interface starts rely on specific implementation)
We should:
tree.NodeResponse
somewhere indata
package for example (or just in this file)internal/api/layer/tree_service.go
that was removed in this PR4d424383b9
to4531d5ebf3
New commits pushed, approval review dismissed automatically according to repository settings
New commits pushed, approval review dismissed automatically according to repository settings
4531d5ebf3
to5eb208caaa
5eb208caaa
to6603b594f5
@ -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) {
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.6603b594f5
to1be92fa4be
New commits pushed, approval review dismissed automatically according to repository settings