[#232] Use contract to get container info #234

Merged
dkirillov merged 1 commit from nzinkevich/frostfs-http-gw:feature/get_cnr_from_contract into master 2025-04-29 07:42:38 +00:00
Member

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

Signed-off-by: Nikita Zinkevich <n.zinkevich@yadro.com>
nzinkevich self-assigned this 2025-04-25 07:03:33 +00:00
nzinkevich added 1 commit 2025-04-25 07:03:34 +00:00
[#XX] Use contract to get container info
Some checks failed
/ DCO (pull_request) Failing after 29s
/ Vulncheck (pull_request) Successful in 55s
/ Builds (pull_request) Successful in 1m11s
/ OCI image (pull_request) Successful in 1m21s
/ Lint (pull_request) Successful in 2m22s
/ Tests (pull_request) Successful in 1m7s
/ Integration tests (pull_request) Failing after 5m57s
be8cfb47bf
Signed-off-by: Nikita Zinkevich <n.zinkevich@yadro.com>
requested reviews from storage-services-developers, storage-services-committers 2025-04-25 07:03:34 +00:00
nzinkevich changed title from [#XX] Use contract to get container info to [#232] Use contract to get container info 2025-04-25 07:07:04 +00:00
nzinkevich force-pushed feature/get_cnr_from_contract from be8cfb47bf to 7c1fa45ac7 2025-04-25 07:07:29 +00:00 Compare
nzinkevich force-pushed feature/get_cnr_from_contract from 7c1fa45ac7 to c84b7aa27c 2025-04-25 07:32:48 +00:00 Compare
dkirillov reviewed 2025-04-25 09:37:01 +00:00
@ -200,0 +201,4 @@
contracts:
container:
# Container contract hash (LE) or name in NNS.
name: container.frostfs
Member

We should mention this in docs/gate-configuration.md

We should mention this in `docs/gate-configuration.md`
dkirillov marked this conversation as resolved
@ -12,0 +15,4 @@
Owner user.ID
Created time.Time
LocationConstraint string
ObjectLockEnabled bool
Member

Actually these 4 fields are not used. Let's add them later (if needed)

Actually these 4 fields are not used. Let's add them later (if needed)
dkirillov marked this conversation as resolved
@ -0,0 +21,4 @@
)
func (h *Handler) containerInfo(ctx context.Context, cnrID cid.ID) (*data.BucketInfo, error) {
var (
Member

We can write just (considering previous comment)

	info := &data.BucketInfo{
		CID:  cnrID,
		Name: cnrID.EncodeToString(),
	}

	res, err := h.cnrContract.GetContainerByID(cnrID)
We can write just (considering previous comment) ```golang info := &data.BucketInfo{ CID: cnrID, Name: cnrID.EncodeToString(), } res, err := h.cnrContract.GetContainerByID(cnrID) ```
dkirillov marked this conversation as resolved
@ -0,0 +34,4 @@
res, err = h.cnrContract.GetContainerByID(cnrID)
if err != nil {
if strings.Contains(err.Error(), containerContract.NotFoundError) {
Member

Let's make GetContainerByID return ErrContainerNotFound.
And don't handle any special here

Let's make `GetContainerByID` return [ErrContainerNotFound](https://git.frostfs.info/nzinkevich/frostfs-http-gw/src/commit/c84b7aa27c27858d29c9cc6d667ec44a3b3b7cf7/internal/handler/handler.go#L147). And don't handle any special here
dkirillov marked this conversation as resolved
@ -0,0 +68,4 @@
return nil, fmt.Errorf("get namespace: %w", err)
}
zone := h.config.FormContainerZone(ns)
if zone != info.Zone {
Member

I'm not sure if we need this. We can access raw containers (not bucket) via http-gw too. It seems on that containers check will be failed. We don't want that

I'm not sure if we need this. We can access raw containers (not bucket) via http-gw too. It seems on that containers check will be failed. We don't want that
dkirillov marked this conversation as resolved
@ -0,0 +1,72 @@
package container
Member

It's better to move this file to service/contracts/container dir

It's better to move this file to `service/contracts/container` dir
Member

Now its path is service/contracts/frostfs/container. It seems frostfs is redundant because we have all contracts in frostfs

Now its path is `service/contracts/frostfs/container`. It seems `frostfs` is redundant because we have all contracts in frostfs
dkirillov marked this conversation as resolved
@ -0,0 +25,4 @@
}
func New(cfg Config) (*Client, error) {
contractHash, err := frostfsutil.ResolveContractHash(cfg.Contract, cfg.RPCAddress)
Member

Can we accept resolving hash as field in Config struct? To avoid duplicating in RPCAddress and RPCClient

Can we accept resolving hash as field in `Config` struct? To avoid duplicating in `RPCAddress` and `RPCClient`
dkirillov marked this conversation as resolved
nzinkevich force-pushed feature/get_cnr_from_contract from c84b7aa27c to 39f2fc4064 2025-04-25 12:20:32 +00:00 Compare
nzinkevich force-pushed feature/get_cnr_from_contract from 39f2fc4064 to b6002c633a 2025-04-25 12:50:49 +00:00 Compare
nzinkevich force-pushed feature/get_cnr_from_contract from b6002c633a to 3f12b85a2d 2025-04-25 12:52:28 +00:00 Compare
nzinkevich force-pushed feature/get_cnr_from_contract from 3f12b85a2d to 34e986ba91 2025-04-28 07:18:46 +00:00 Compare
dkirillov reviewed 2025-04-28 07:24:35 +00:00
@ -409,3 +420,3 @@
namespaces := s.defaultNamespaces
s.mu.RUnlock()
if slices.Contains(namespaces, ns) {
if len(ns) == 0 || slices.Contains(namespaces, ns) {
Member

Why do we need this?

Why do we need this?
Author
Member

To handle empty namespace header as root rather than custom ns

To handle empty namespace header as `root` rather than custom ns
Member

By default we have this

v.SetDefault(cfgResolveDefaultNamespaces, []string{"", "root"})

By default we have this https://git.frostfs.info/TrueCloudLab/frostfs-http-gw/src/commit/ee628617a36f3a7512a29414dabf7af173a06e5d/cmd/http-gw/settings.go#L398
dkirillov marked this conversation as resolved
@ -0,0 +47,4 @@
func (c *Client) GetContainerByID(cnrID cid.ID) (*container.Container, error) {
items, err := c.contract.Get(cnrID[:])
if err != nil {
return nil, handler.ErrContainerNotFound
Member

We should not always return handler.ErrContainerNotFound.
We need something:

diff --git a/internal/service/contracts/frostfs/container/client.go b/internal/service/contracts/frostfs/container/client.go
index cbb483f..ea50496 100644
--- a/internal/service/contracts/frostfs/container/client.go
+++ b/internal/service/contracts/frostfs/container/client.go
@@ -2,8 +2,10 @@ package container
 
 import (
 	"fmt"
+	"strings"
 
-	containerContract "git.frostfs.info/TrueCloudLab/frostfs-contract/rpcclient/container"
+	containercontract "git.frostfs.info/TrueCloudLab/frostfs-contract/container"
+	containerclient "git.frostfs.info/TrueCloudLab/frostfs-contract/rpcclient/container"
 	"git.frostfs.info/TrueCloudLab/frostfs-http-gw/internal/handler"
 	"git.frostfs.info/TrueCloudLab/frostfs-sdk-go/container"
 	cid "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/container/id"
@@ -15,7 +17,7 @@ import (
 )
 
 type Client struct {
-	contract *containerContract.Contract
+	contract *containerclient.Contract
 }
 
 type Config struct {
@@ -40,14 +42,18 @@ func New(cfg Config) (*Client, error) {
 	}
 
 	return &Client{
-		contract: containerContract.New(act, cfg.ContractHash),
+		contract: containerclient.New(act, cfg.ContractHash),
 	}, nil
 }
 
 func (c *Client) GetContainerByID(cnrID cid.ID) (*container.Container, error) {
 	items, err := c.contract.Get(cnrID[:])
 	if err != nil {
-		return nil, handler.ErrContainerNotFound
+		if strings.Contains(err.Error(), containercontract.NotFoundError) {
+			return nil, fmt.Errorf("%w: %s", handler.ErrContainerNotFound, err.Error())
+		}
+
+		return nil, err
 	}
 
 	if len(items) != 4 {

We should not always return `handler.ErrContainerNotFound`. We need something: ```diff diff --git a/internal/service/contracts/frostfs/container/client.go b/internal/service/contracts/frostfs/container/client.go index cbb483f..ea50496 100644 --- a/internal/service/contracts/frostfs/container/client.go +++ b/internal/service/contracts/frostfs/container/client.go @@ -2,8 +2,10 @@ package container import ( "fmt" + "strings" - containerContract "git.frostfs.info/TrueCloudLab/frostfs-contract/rpcclient/container" + containercontract "git.frostfs.info/TrueCloudLab/frostfs-contract/container" + containerclient "git.frostfs.info/TrueCloudLab/frostfs-contract/rpcclient/container" "git.frostfs.info/TrueCloudLab/frostfs-http-gw/internal/handler" "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/container" cid "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/container/id" @@ -15,7 +17,7 @@ import ( ) type Client struct { - contract *containerContract.Contract + contract *containerclient.Contract } type Config struct { @@ -40,14 +42,18 @@ func New(cfg Config) (*Client, error) { } return &Client{ - contract: containerContract.New(act, cfg.ContractHash), + contract: containerclient.New(act, cfg.ContractHash), }, nil } func (c *Client) GetContainerByID(cnrID cid.ID) (*container.Container, error) { items, err := c.contract.Get(cnrID[:]) if err != nil { - return nil, handler.ErrContainerNotFound + if strings.Contains(err.Error(), containercontract.NotFoundError) { + return nil, fmt.Errorf("%w: %s", handler.ErrContainerNotFound, err.Error()) + } + + return nil, err } if len(items) != 4 { ```
dkirillov marked this conversation as resolved
nzinkevich force-pushed feature/get_cnr_from_contract from 34e986ba91 to f2b3c50a69 2025-04-28 08:05:13 +00:00 Compare
dkirillov approved these changes 2025-04-28 12:06:05 +00:00
Dismissed
dkirillov left a comment
Member

LGTM, but see comment #234 (comment)

LGTM, but see comment https://git.frostfs.info/TrueCloudLab/frostfs-http-gw/pulls/234#issuecomment-74241
nzinkevich force-pushed feature/get_cnr_from_contract from f2b3c50a69 to 58a5b34af0 2025-04-28 12:46:52 +00:00 Compare
nzinkevich dismissed dkirillov's review 2025-04-28 12:46:53 +00:00
Reason:

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

nzinkevich force-pushed feature/get_cnr_from_contract from 58a5b34af0 to 904576bb70 2025-04-28 12:48:09 +00:00 Compare
dkirillov approved these changes 2025-04-28 13:32:09 +00:00
Dismissed
r.loginov approved these changes 2025-04-28 13:35:50 +00:00
Dismissed
pogpp approved these changes 2025-04-28 13:45:22 +00:00
Dismissed
mbiryukova approved these changes 2025-04-28 13:50:29 +00:00
Dismissed
@ -130,0 +130,4 @@
EmptyAccessControlRequestMethodHeader = "empty Access-Control-Request-Method request header"
CORSRuleWasNotMatched = "cors rule was not matched"
CouldntCacheCors = "couldn't cache cors"
CouldNotParseContainerObjectLockEnabledAttribute = "could not parse container object lock enabled attribute"
Member

Seems to be unused and the next one too

Seems to be unused and the next one too
mbiryukova marked this conversation as resolved
nzinkevich force-pushed feature/get_cnr_from_contract from 904576bb70 to 96a22d98f2 2025-04-28 13:58:56 +00:00 Compare
nzinkevich dismissed dkirillov's review 2025-04-28 13:58:56 +00:00
Reason:

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

nzinkevich dismissed r.loginov's review 2025-04-28 13:58:56 +00:00
Reason:

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

nzinkevich dismissed pogpp's review 2025-04-28 13:58:56 +00:00
Reason:

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

nzinkevich dismissed mbiryukova's review 2025-04-28 13:58:56 +00:00
Reason:

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

dkirillov approved these changes 2025-04-28 15:30:26 +00:00
pogpp approved these changes 2025-04-29 06:45:44 +00:00
r.loginov approved these changes 2025-04-29 07:11:33 +00:00
dkirillov merged commit 96a22d98f2 into master 2025-04-29 07:42:38 +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#234
No description provided.