TrueCloudLab/frostfs-node
20
1
Fork 27
Code Issues 105 Pull requests 11 Projects Releases 2 Activity Actions

Strict APE check for EC & fix sign EC part put requests #1451

Merged
fyrchik merged 5 commits from dstepanov-yadro/frostfs-node:fix/ec_ape_strict into master 2024-11-06 08:18:11 +00:00
Conversation 2 Commits 5 Files changed 7 +30 -15
2 changed files with 30 additions and 15 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit de3a89a0ba - Show all commits

1
pkg/services/object/ape/checker_test.go
View file

@ -695,6 +695,7 @@ func TestPutECChunk(t *testing.T) {
nm := &netmapStub{ nm := &netmapStub{
currentEpoch: 100, currentEpoch: 100,
netmaps: map[uint64]*netmapSDK.NetMap{ netmaps: map[uint64]*netmapSDK.NetMap{
99: netmap,
fyrchik commented 2024-10-30 06:59:12 +00:00
Outdated
Review
Copy link

Why this change is necessary?

Why this change is necessary?
dstepanov-yadro commented 2024-10-30 08:09:38 +00:00
Outdated
Review
Copy link

previously, checking that the node is a container node ignored the error that netmap is not defined for previous epoch

previously, checking that the node is a container node ignored the error that netmap is not defined for previous epoch
100: netmap, 100: netmap,
}, },
} }

44
pkg/services/object/ape/request.go
View file

@ -3,6 +3,7 @@ package ape
import ( import (
"context" "context"
"crypto/sha256" "crypto/sha256"
"errors"
"fmt" "fmt"
"net" "net"
"strconv" "strconv"
@ -11,6 +12,7 @@ import (
aperequest "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/ape/request" aperequest "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/ape/request"
"git.frostfs.info/TrueCloudLab/frostfs-node/pkg/core/netmap" "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/core/netmap"
"git.frostfs.info/TrueCloudLab/frostfs-node/pkg/core/object" "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/core/object"
apistatus "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/client/status"
"git.frostfs.info/TrueCloudLab/frostfs-sdk-go/container/acl" "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/container/acl"
cid "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/container/id" cid "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/container/id"
objectSDK "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object" objectSDK "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object"
@ -24,6 +26,8 @@ import (
var defaultRequest = aperequest.Request{} var defaultRequest = aperequest.Request{}
var errECMissingParentObjectID = errors.New("missing EC parent object ID")
func nativeSchemaRole(role acl.Role) string { func nativeSchemaRole(role acl.Role) string {
switch role { switch role {
case acl.RoleOwner: case acl.RoleOwner:
@ -122,7 +126,10 @@ func (c *checkerImpl) newAPERequest(ctx context.Context, prm Prm) (aperequest.Re
header = headerObjSDK.ToV2().GetHeader() header = headerObjSDK.ToV2().GetHeader()
} }
} }
dstepanov-yadro commented 2024-10-28 13:21:57 +00:00
Outdated
Review
Copy link

It looks even too strict if you look at the line above:


	} else if prm.Object != nil {
		headerObjSDK, err := c.headerProvider.GetHeader(ctx, prm.Container, *prm.Object, true)
		if err == nil {
			header = headerObjSDK.ToV2().GetHeader()
		}
	}
It looks even too strict if you look at the line above: ``` } else if prm.Object != nil { headerObjSDK, err := c.headerProvider.GetHeader(ctx, prm.Container, *prm.Object, true) if err == nil { header = headerObjSDK.ToV2().GetHeader() } } ```
header = c.fillHeaderWithECParent(ctx, prm, header) header, err := c.fillHeaderWithECParent(ctx, prm, header)
if err != nil {
return defaultRequest, fmt.Errorf("get EC parent header: %w", err)
}
reqProps := map[string]string{ reqProps := map[string]string{
nativeschema.PropertyKeyActorPublicKey: prm.SenderKey, nativeschema.PropertyKeyActorPublicKey: prm.SenderKey,
nativeschema.PropertyKeyActorRole: prm.Role, nativeschema.PropertyKeyActorRole: prm.Role,
@ -133,7 +140,6 @@ func (c *checkerImpl) newAPERequest(ctx context.Context, prm Prm) (aperequest.Re
reqProps[xheadKey] = xhead.GetValue() reqProps[xheadKey] = xhead.GetValue()
} }
var err error
reqProps, err = c.fillWithUserClaimTags(reqProps, prm) reqProps, err = c.fillWithUserClaimTags(reqProps, prm)
if err != nil { if err != nil {
return defaultRequest, err return defaultRequest, err
@ -155,35 +161,43 @@ func (c *checkerImpl) newAPERequest(ctx context.Context, prm Prm) (aperequest.Re
), nil ), nil
} }
func (c *checkerImpl) fillHeaderWithECParent(ctx context.Context, prm Prm, header *objectV2.Header) *objectV2.Header { func (c *checkerImpl) fillHeaderWithECParent(ctx context.Context, prm Prm, header *objectV2.Header) (*objectV2.Header, error) {
fyrchik commented 2024-10-30 07:01:47 +00:00
Outdated
Review
Copy link

I do not completely understand this function.
In what circumstances do we return header and not parent header and, more importantly, why?

I do not completely understand this function. In what circumstances do we return `header` and not parent header and, more importantly, why?
dstepanov-yadro commented 2024-10-30 08:08:37 +00:00
Outdated
Review
Copy link
  1. if it is not EC chunk
  2. if current node is IR or container node: APE skips check for inside-container requests; not sure about the same for IR, maybe should delete it
  3. if parent object does not exist
  4. if current node is not container node: in this case node has no permissions to collect EC object
1. if it is not EC chunk 2. if current node is IR or container node: APE skips check for inside-container requests; not sure about the same for IR, maybe should delete it 3. if parent object does not exist 4. if current node is not container node: in this case node has no permissions to collect EC object
fyrchik commented 2024-10-30 08:20:30 +00:00
Outdated
Review
Copy link

If ape skips checks for in-container requests, why do we call this function at all?

If ape skips checks for in-container requests, why do we _call_ this function at all?
dstepanov-yadro commented 2024-10-30 08:27:02 +00:00
Outdated
Review
Copy link

Oh, mistake. It must be:

2. if request is from IR or container node: APE skips check for inside-container requests; not sure about the same for IR, maybe

Then container node tries to collect headers if request is done be owner or other user.

Oh, mistake. It must be: ``` 2. if request is from IR or container node: APE skips check for inside-container requests; not sure about the same for IR, maybe ``` Then container node tries to collect headers if request is done be owner or other user.
fyrchik commented 2024-10-30 08:34:31 +00:00
Outdated
Review
Copy link

Again, if APE skips check, why do we call this function?
It doesn't seem we signify the "skip" part with our return value in any way.

Again, if APE skips check, why do we call this function? It doesn't seem we signify the "skip" part with our return value in any way.
dstepanov-yadro commented 2024-10-30 08:54:24 +00:00
Outdated
Review
Copy link

Removed pt.2 as it is checker before this function call.

Removed pt.2 as it is checker before this function call.
if header == nil { if header == nil {
return header return header, nil
} }
if header.GetEC() == nil { if header.GetEC() == nil {
return header return header, nil
}
if prm.Role == nativeschema.PropertyValueContainerRoleContainer ||
prm.Role == nativeschema.PropertyValueContainerRoleIR {
return header
} }
parentObjRefID := header.GetEC().Parent parentObjRefID := header.GetEC().Parent
if parentObjRefID == nil { if parentObjRefID == nil {
return header return nil, errECMissingParentObjectID
} }
var parentObjID oid.ID var parentObjID oid.ID
if err := parentObjID.ReadFromV2(*parentObjRefID); err != nil { if err := parentObjID.ReadFromV2(*parentObjRefID); err != nil {
return header return nil, fmt.Errorf("EC parent object ID format error: %w", err)
} }
// only container node have access to collect parent object // only container node have access to collect parent object
contNode, err := c.currentNodeIsContainerNode(prm.Container) contNode, err := c.currentNodeIsContainerNode(prm.Container)
if err != nil || !contNode { if err != nil {
return header return nil, fmt.Errorf("check container node status: %w", err)
}
if !contNode {
return header, nil
} }
parentObj, err := c.headerProvider.GetHeader(ctx, prm.Container, parentObjID, false) parentObj, err := c.headerProvider.GetHeader(ctx, prm.Container, parentObjID, false)
if err != nil { if err != nil {
return header if isLogicalError(err) {
return header, nil
} }
return parentObj.ToV2().GetHeader() return nil, fmt.Errorf("EC parent header request: %w", err)
}
return parentObj.ToV2().GetHeader(), nil
}
func isLogicalError(err error) bool {
var errObjRemoved *apistatus.ObjectAlreadyRemoved
var errObjNotFound *apistatus.ObjectNotFound
return errors.As(err, &errObjRemoved) || errors.As(err, &errObjNotFound)
} }
func (c *checkerImpl) currentNodeIsContainerNode(cnrID cid.ID) (bool, error) { func (c *checkerImpl) currentNodeIsContainerNode(cnrID cid.ID) (bool, error) {