#30 add object name resolving #43

Merged
alexvanin merged 1 commit from pogpp/frostfs-http-gw:feature/tree_object_name_resolving into master 2023-05-15 14:56:04 +00:00
Member

Signed-off-by: Pavel Pogodaev p.pogodaev@yadro.com

Signed-off-by: Pavel Pogodaev <p.pogodaev@yadro.com>
dkirillov reviewed 2023-04-28 13:37:51 +00:00
app.go Outdated
@ -162,6 +169,7 @@ func newApp(ctx context.Context, opt ...Option) App {
if err != nil {
a.log.Fatal("failed to dial pool", zap.Error(err))
}
a.key = getWalletKey(a.log, a.cfg)
Member

Actually, you have already got this key on line 128. Probably you have to change some methods signature a little, but still

Actually, you have already got this key on line 128. Probably you have to change some methods signature a little, but still
pogpp marked this conversation as resolved
dkirillov reviewed 2023-04-28 13:45:59 +00:00
@ -0,0 +1,61 @@
package wallet
Member

Can be dropped

Can be dropped
pogpp marked this conversation as resolved
dkirillov reviewed 2023-04-28 13:47:16 +00:00
app.go Outdated
@ -44,6 +50,7 @@ type (
services []*metrics.Service
settings *appSettings
servers []Server
treeSvc *tree.Tree
Member

Currently this is unused

Currently this is unused
pogpp marked this conversation as resolved
dkirillov reviewed 2023-04-28 13:51:02 +00:00
@ -0,0 +1,28 @@
package accessbox
Member

Can be dropped along with creds/accessbox/accessbox.pb.go

Can be dropped along with `creds/accessbox/accessbox.pb.go`
pogpp marked this conversation as resolved
dkirillov reviewed 2023-04-28 13:59:47 +00:00
@ -0,0 +292,4 @@
}
func getBearer(ctx context.Context, bktInfo *data.BucketInfo) []byte {
if bd, ok := ctx.Value(api.BoxData).(*accessbox.Box); ok && bd != nil && bd.Gate != nil {
Member

You should set bearer token to context there using something like that and use something like this here to get token back

You should set bearer token to context [there](https://git.frostfs.info/pogpp/frostfs-http-gw/src/commit/713da1ac4826f25c29c8166f577f57e82c65d9cc/downloader/download.go#L312) using something like [that](https://git.frostfs.info/pogpp/frostfs-http-gw/src/commit/713da1ac4826f25c29c8166f577f57e82c65d9cc/downloader/download.go#L100-L108) and use something like [this](https://git.frostfs.info/pogpp/frostfs-http-gw/src/commit/959213520e2ddbab081c1cdffa0c98303535b46b/tokens/bearer-token.go#L65) here to get token back
pogpp marked this conversation as resolved
dkirillov reviewed 2023-04-28 14:08:14 +00:00
@ -0,0 +1,174 @@
package layer
Member

For now can be droped

For now can be droped
pogpp marked this conversation as resolved
dkirillov reviewed 2023-04-28 14:08:41 +00:00
@ -0,0 +8,4 @@
)
// TreeService provide interface to interact with tree service using s3 data models.
type TreeService interface {
Member

We can simplify to:

type TreeService interface {
	GetLatestVersion(ctx context.Context, cnrID cid.ID, objectName string) (*data.NodeVersion, error)
}
We can simplify to: ``` type TreeService interface { GetLatestVersion(ctx context.Context, cnrID cid.ID, objectName string) (*data.NodeVersion, error) } ```
pogpp marked this conversation as resolved
dkirillov reviewed 2023-04-28 14:08:59 +00:00
api/user_auth.go Outdated
@ -0,0 +1,7 @@
package api
Member

Can be dropped

Can be dropped
pogpp marked this conversation as resolved
dkirillov reviewed 2023-04-28 14:09:18 +00:00
@ -0,0 +1,39 @@
package data
Member

Can be dropped

Can be dropped
pogpp marked this conversation as resolved
dkirillov reviewed 2023-04-28 14:09:24 +00:00
@ -0,0 +1,50 @@
package data
Member

Can be dropped

Can be dropped
pogpp marked this conversation as resolved
dkirillov reviewed 2023-04-28 14:09:43 +00:00
api/data/info.go Outdated
@ -0,0 +1,127 @@
package data
Member

Can be dropped

Can be dropped
pogpp marked this conversation as resolved
dkirillov reviewed 2023-04-28 14:10:41 +00:00
@ -211,6 +213,7 @@ type Downloader struct {
pool *pool.Pool
containerResolver *resolver.ContainerResolver
settings *Settings
tree *tree.Tree
Member

We should use interface TreeService here

We should use interface `TreeService` here
Member

Or we can postpone this, if we don't want to add tree mock now

Or we can postpone this, if we don't want to add tree mock now
pogpp marked this conversation as resolved
pogpp force-pushed feature/tree_object_name_resolving from 713da1ac48 to fcbcf8ec8a 2023-04-28 14:10:49 +00:00 Compare
dkirillov reviewed 2023-04-28 14:18:16 +00:00
tree/tree.go Outdated
@ -0,0 +21,4 @@
// ServiceClient is a client to interact with tree service.
// Each method must return ErrNodeNotFound or ErrNodeAccessDenied if relevant.
ServiceClient interface {
Member

Can be simplified to:

	ServiceClient interface {
		GetNodes(ctx context.Context, p *GetNodesParams) ([]NodeResponse, error)
	}
Can be simplified to: ``` ServiceClient interface { GetNodes(ctx context.Context, p *GetNodesParams) ([]NodeResponse, error) } ```
pogpp marked this conversation as resolved
dkirillov reviewed 2023-04-28 14:18:40 +00:00
tree/tree.go Outdated
@ -0,0 +40,4 @@
}
GetNodesParams struct {
BktInfo *data.BucketInfo
Member

Use CnrID cid.ID

Use `CnrID cid.ID`
pogpp marked this conversation as resolved
dkirillov reviewed 2023-04-28 14:19:07 +00:00
@ -0,0 +256,4 @@
return nil
}
func (c *ServiceClientGRPC) RemoveNode(ctx context.Context, bktInfo *data.BucketInfo, treeID string, nodeID uint64) error {
Member

Drop this method and other unused

Drop this method and other unused
pogpp marked this conversation as resolved
dkirillov reviewed 2023-04-28 14:20:00 +00:00
api/data/tree.go Outdated
@ -0,0 +65,4 @@
}
// MultipartInfo is multipart upload information.
type MultipartInfo struct {
Member

Drop this and other unused

Drop this and other unused
pogpp marked this conversation as resolved
pogpp force-pushed feature/tree_object_name_resolving from fcbcf8ec8a to 187b3b3044 2023-05-02 14:09:49 +00:00 Compare
dkirillov reviewed 2023-05-03 08:18:54 +00:00
app.go Outdated
@ -119,3 +124,3 @@
a.webServer.StreamRequestBody = a.cfg.GetBool(cfgWebStreamRequestBody)
// -- -- -- -- -- -- -- -- -- -- -- -- -- --
key, err = getFrostFSKey(a)
pvtKey, err := getFrostFSKey(a)
Member

We can simply write:

a.key, err = getFrostFSKey(a)
We can simply write: ``` a.key, err = getFrostFSKey(a) ```
pogpp marked this conversation as resolved
dkirillov reviewed 2023-05-03 08:20:08 +00:00
app.go Outdated
@ -120,2 +125,3 @@
// -- -- -- -- -- -- -- -- -- -- -- -- -- --
key, err = getFrostFSKey(a)
pvtKey, err := getFrostFSKey(a)
key = &pvtKey.PrivateKey
Member

This cause panic if err != nil. Let's drop this line and below (line 136-137) write:

var prm pool.InitParameters
prm.SetKey(&pvtKey.PrivateKey)
This cause panic if `err != nil`. Let's drop this line and below (line 136-137) write: ``` var prm pool.InitParameters prm.SetKey(&pvtKey.PrivateKey) ```
pogpp marked this conversation as resolved
dkirillov reviewed 2023-05-03 08:24:51 +00:00
app.go Outdated
@ -469,3 +477,3 @@
r.GET("/zip/{cid}/{prefix:*}", a.logger(downloadRoutes.DownloadZipped))
a.log.Info("added path /zip/{cid}/{prefix}")
r.GET("/s3get/{bucketname}/{key}", a.logger(downloadRoutes.DownloadByBucketName))
Member

Probably we should use existing route /get/{cid}/{oid}

Probably we should use existing route `/get/{cid}/{oid}`
pogpp marked this conversation as resolved
dkirillov reviewed 2023-05-03 08:29:57 +00:00
@ -283,0 +314,4 @@
addr.SetContainer(*cnrID)
// TODO where i've to take UID?
foundOid, err := d.tree.GetLatestVersion(d.appCtx, cnrID, user.ID{}, key)
Member

You don't have to use user.ID. You can just drop if statement here

Also you save bearer token to c but futher try to get it from d.appCtx so this won't work.

You don't have to use `user.ID`. You can just drop `if` statement [here](https://git.frostfs.info/TrueCloudLab/frostfs-http-gw/src/commit/187b3b30449c336447441fd09604b575b2570145/internal/frostfs/services/tree_client_grpc.go#L130) Also you save bearer token to `c` but futher try to get it from `d.appCtx` so this won't work.
pogpp marked this conversation as resolved
dkirillov reviewed 2023-05-03 08:38:40 +00:00
@ -283,0 +315,4 @@
// TODO where i've to take UID?
foundOid, err := d.tree.GetLatestVersion(d.appCtx, cnrID, user.ID{}, key)
if err != nil {
Member

You should also check if foundOid.IsDeleteMarker() is false otherwise returns 404

You should also check if `foundOid.IsDeleteMarker()` is false otherwise returns `404`
pogpp marked this conversation as resolved
dkirillov reviewed 2023-05-03 08:39:40 +00:00
@ -283,0 +292,4 @@
// prepares request and object address to it.
func (d *Downloader) byBucketname(c *fasthttp.RequestCtx, f func(request, *pool.Pool, oid.Address)) {
var (
bucketname, _ = c.UserValue("bucketname").(string)
Member

Actually we can omit , _ and write:

bucketname = c.UserValue("bucketname").(string)
Actually we can omit `, _` and write: ``` bucketname = c.UserValue("bucketname").(string) ```
pogpp marked this conversation as resolved
Owner

@pogpp @dkirillov What you think about removing gRPC generated code from the repo and having the same script as in S3 Gateway?

  1. https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/src/branch/master/syncTree.sh
@pogpp @dkirillov What you think about removing gRPC generated code from the repo and having the same script as in S3 Gateway? 1) https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/src/commit/69d8779dafaef2af88cad4045630bd9a98cba18d/Makefile#L46 2) https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/src/branch/master/syncTree.sh
alexvanin reviewed 2023-05-03 10:53:24 +00:00
api/tree.go Outdated
@ -0,0 +30,4 @@
type BaseNodeVersion struct {
ID uint64
ParenID uint64
OID oid.ID
Owner

As far as I understand, none of these attributes in the tree are not used except OID in the code.

While S3 Gateway requires data about etag, size, timestamp, etc., HTTP gateway requires only OID to make GET request to the storage node.

As far as I understand, none of these attributes in the tree are not used except `OID` in the code. While S3 Gateway requires data about etag, size, timestamp, etc., HTTP gateway requires only OID to make GET request to the storage node.
pogpp marked this conversation as resolved
tree/tree.go Outdated
@ -0,0 +180,4 @@
}
func (c *Tree) GetLatestVersion(ctx context.Context, cnrID *cid.ID, UID user.ID, objectName string) (*api.NodeVersion, error) {
meta := []string{oidKV, isUnversionedKV, isDeleteMarkerKV, etagKV, sizeKV}
Owner

Can we simplify it by removing etagKV and sizeKV? I don't think this is required to fetch OID from the tree.

Can we simplify it by removing `etagKV` and `sizeKV`? I don't think this is required to fetch OID from the tree.
pogpp marked this conversation as resolved
pogpp force-pushed feature/tree_object_name_resolving from 187b3b3044 to ae98ef7daf 2023-05-04 10:37:38 +00:00 Compare
pogpp force-pushed feature/tree_object_name_resolving from ae98ef7daf to 43e2b44045 2023-05-04 12:40:57 +00:00 Compare
dkirillov reviewed 2023-05-04 12:59:55 +00:00
api/tree.go Outdated
@ -0,0 +10,4 @@
// NodeVersion represent node from tree service.
type NodeVersion struct {
BaseNodeVersion
DeleteMarker *DeleteMarkerInfo
Member

Actually, we can simplify this to bool field.

Actually, we can simplify this to `bool` field.
pogpp marked this conversation as resolved
dkirillov reviewed 2023-05-04 13:00:29 +00:00
api/tree.go Outdated
@ -0,0 +11,4 @@
type NodeVersion struct {
BaseNodeVersion
DeleteMarker *DeleteMarkerInfo
IsUnversioned bool
Member

It seems we can drop this field

It seems we can drop this field
pogpp marked this conversation as resolved
dkirillov reviewed 2023-05-04 13:01:17 +00:00
app.go Outdated
@ -468,4 +472,3 @@
a.log.Info("added path /get_by_attribute/{cid}/{attr_key}/{attr_val:*}")
r.GET("/zip/{cid}/{prefix:*}", a.logger(downloadRoutes.DownloadZipped))
a.log.Info("added path /zip/{cid}/{prefix}")
Member

Unnecessary change

Unnecessary change
pogpp marked this conversation as resolved
dkirillov reviewed 2023-05-04 13:04:36 +00:00
tree/tree.go Outdated
@ -0,0 +31,4 @@
ObjID oid.ID
TimeStamp uint64
Size int64
Meta map[string]string
Member

It seems we can keep only ObjID and Meta fields

It seems we can keep only `ObjID` and `Meta` fields
pogpp marked this conversation as resolved
dkirillov reviewed 2023-05-04 13:06:27 +00:00
tree/tree.go Outdated
@ -0,0 +121,4 @@
return value, ok
}
func newNodeVersion(filePath string, node NodeResponse) (*api.NodeVersion, error) {
Member

The first arg filePath string isn't used`

The first arg `filePath string` isn't used`
pogpp marked this conversation as resolved
dkirillov reviewed 2023-05-04 13:08:58 +00:00
@ -113,1 +112,3 @@
d.byAddress(c, request.headObject)
// HeadByAddressOrBucketName handles head requests using simple cid/oid or bucketname/key format.
func (d *Downloader) HeadByAddressOrBucketName(c *fasthttp.RequestCtx) {
test, _ := c.UserValue("cid").(string)
Member

Actually we should check for oid here because both d.byBucketname and d.byAddress can handle any cid just fine

Actually we should check for `oid` here because both `d.byBucketname` and `d.byAddress` can handle any `cid` just fine
pogpp marked this conversation as resolved
pogpp force-pushed feature/tree_object_name_resolving from 43e2b44045 to c23c1ea730 2023-05-04 13:11:10 +00:00 Compare
pogpp force-pushed feature/tree_object_name_resolving from c23c1ea730 to f196297d51 2023-05-04 14:49:26 +00:00 Compare
pogpp force-pushed feature/tree_object_name_resolving from f196297d51 to 5d991ce249 2023-05-04 19:00:43 +00:00 Compare
dkirillov reviewed 2023-05-05 06:49:32 +00:00
api/tree.go Outdated
@ -0,0 +10,4 @@
DeleteMarker bool
}
func (v NodeVersion) IsDeleteMarker() bool {
Member

Can be dropped

Can be dropped
pogpp marked this conversation as resolved
dkirillov reviewed 2023-05-05 06:51:00 +00:00
@ -127,2 +134,4 @@
}
}
Member

This will be fixed by go fmt, so let's drop this change

This will be fixed by `go fmt`, so let's drop this change
pogpp marked this conversation as resolved
dkirillov reviewed 2023-05-05 06:53:22 +00:00
tree/tree.go Outdated
@ -0,0 +129,4 @@
},
}
if isDeleteMarker {
Member

We can just write:

version := &api.NodeVersion{
	BaseNodeVersion: api.BaseNodeVersion{
		OID: treeNode.ObjID,
	},
	DeleteMarker = isDeleteMarker,
}
We can just write: ```golang version := &api.NodeVersion{ BaseNodeVersion: api.BaseNodeVersion{ OID: treeNode.ObjID, }, DeleteMarker = isDeleteMarker, } ```
pogpp marked this conversation as resolved
dkirillov reviewed 2023-05-05 06:54:04 +00:00
tree/tree.go Outdated
@ -0,0 +57,4 @@
// keys for delete marker nodes.
isDeleteMarkerKV = "IsDeleteMarker"
ownerKV = "Owner"
createdKV = "Created"
Member

ownerKV and createdKV can be dropped now

`ownerKV` and `createdKV` can be dropped now
pogpp marked this conversation as resolved
dkirillov reviewed 2023-05-05 06:54:43 +00:00
tree/tree.go Outdated
@ -0,0 +136,4 @@
}
func (c *Tree) GetLatestVersion(ctx context.Context, cnrID *cid.ID, objectName string) (*api.NodeVersion, error) {
meta := []string{oidKV, isUnversionedKV, isDeleteMarkerKV}
Member

Actually, we don't use isUnversionedKV anymore

Actually, we don't use `isUnversionedKV` anymore
pogpp marked this conversation as resolved
dkirillov reviewed 2023-05-05 06:56:34 +00:00
@ -0,0 +1,520 @@
//*
Member

We shouldn't commit these files. Let's add directory internal/frostfs/services/tree to .gitignore

We shouldn't commit these files. Let's add directory `internal/frostfs/services/tree` to `.gitignore`
pogpp marked this conversation as resolved
dkirillov reviewed 2023-05-05 06:58:31 +00:00
@ -293,0 +329,4 @@
foundOid, err := d.tree.GetLatestVersion(d.appCtx, cnrID, key)
if err != nil || foundOid.IsDeleteMarker() {
log.Error("object wasn't found", zap.Error(err))
Member

I wouldn't log nil error (error is nil in case of foundID is delete marker). But I don't insist

I wouldn't log `nil` error (error is `nil` in case of foundID is delete marker). But I don't insist
pogpp marked this conversation as resolved
dkirillov reviewed 2023-05-05 06:59:36 +00:00
@ -293,0 +322,4 @@
response.Error(c, "could not fetch and store bearer token: "+err.Error(), fasthttp.StatusBadRequest)
return
}
d.appCtx = ctx
Member

Why we do this? Let's just pass ctx futher

Why we do this? Let's just pass `ctx` futher
pogpp marked this conversation as resolved
pogpp force-pushed feature/tree_object_name_resolving from 5d991ce249 to 6c4fc75bbc 2023-05-05 08:20:08 +00:00 Compare
dkirillov reviewed 2023-05-05 09:23:49 +00:00
@ -293,0 +333,4 @@
return
}
if foundOid.DeleteMarker {
response.Error(c, "object deleted", fasthttp.StatusNotFound)
Member

Why don't we log such situation?

Why don't we log such situation?
pogpp marked this conversation as resolved
dkirillov reviewed 2023-05-05 09:26:07 +00:00
tree/tree.go Outdated
@ -0,0 +75,4 @@
GetMeta() []Meta
GetNodeID() uint64
GetParentID() uint64
GetTimestamp() uint64
Member

Actually, we use only GetMeta from this interface. So other methods can be dropped

Actually, we use only `GetMeta` from this interface. So other methods can be dropped
pogpp marked this conversation as resolved
dkirillov reviewed 2023-05-05 09:29:57 +00:00
@ -293,0 +324,4 @@
}
var addr oid.Address
addr.SetContainer(*cnrID)
Member

Is there any reason to define addr variable here and not after successful getting last version (right before 6c4fc75bbc/downloader/download.go (L340))

Is there any reason to define `addr` variable here and not after successful getting last version (right before https://git.frostfs.info/pogpp/frostfs-http-gw/src/commit/6c4fc75bbccf98baace6d066d844ab9aad8b1017/downloader/download.go#L340)
pogpp marked this conversation as resolved
pogpp force-pushed feature/tree_object_name_resolving from 6c4fc75bbc to 2e31a0794d 2023-05-05 11:34:48 +00:00 Compare
dkirillov reviewed 2023-05-05 15:17:23 +00:00
Makefile Outdated
@ -40,2 +39,4 @@
@mkdir -p $@
# Synchronize tree service
sync-tree:
Member

Let's also clean this newly created directory on make clean. See s3

Let's also clean this newly created directory on `make clean`. See s3 https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/src/commit/b366e75366e1253422833bbea817af7ec30ba915/Makefile#L137
pogpp marked this conversation as resolved
dkirillov reviewed 2023-05-05 15:27:38 +00:00
app.go Outdated
@ -478,2 +482,2 @@
r.GET("/get/{cid}/{oid}", a.logger(downloadRoutes.DownloadByAddress))
r.HEAD("/get/{cid}/{oid}", a.logger(downloadRoutes.HeadByAddress))
r.GET("/get/{cid}/{oid}", a.logger(downloadRoutes.DownloadByAddressOrBucketName))
r.HEAD("/get/{cid}/{oid}", a.logger(downloadRoutes.HeadByAddressOrBucketName))
Member

Actually, we should uses the following path (for GET and HEAD):

"/get/{cid}/{oid:*}"
Actually, we should uses the following path (for `GET` and `HEAD`): ``` "/get/{cid}/{oid:*}" ````
pogpp marked this conversation as resolved
dkirillov reviewed 2023-05-05 15:29:38 +00:00
@ -293,0 +337,4 @@
var addr oid.Address
addr.SetContainer(*cnrID)
addr.SetObject(foundOid.OID)
Member

It's a matter of taste but can we write:


	var addr oid.Address
	addr.SetContainer(*cnrID)
	addr.SetObject(foundOid.OID)

instead of:

	var addr oid.Address
	addr.SetContainer(*cnrID)

	addr.SetObject(foundOid.OID)
It's a matter of taste but can we write: ``` var addr oid.Address addr.SetContainer(*cnrID) addr.SetObject(foundOid.OID) ``` instead of: ``` var addr oid.Address addr.SetContainer(*cnrID) addr.SetObject(foundOid.OID) ```
pogpp marked this conversation as resolved
pogpp force-pushed feature/tree_object_name_resolving from 2e31a0794d to 30653bdca5 2023-05-12 09:51:44 +00:00 Compare
pogpp force-pushed feature/tree_object_name_resolving from 30653bdca5 to 8c3c3782f5 2023-05-12 09:53:53 +00:00 Compare
ironbee approved these changes 2023-05-12 10:26:30 +00:00
dkirillov approved these changes 2023-05-15 09:57:39 +00:00
alexvanin merged commit 8c3c3782f5 into master 2023-05-15 14:56:04 +00:00
alexvanin deleted branch feature/tree_object_name_resolving 2023-05-15 14:56:05 +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-http-gw#43
No description provided.