TrueCloudLab/frostfs-node
20
1
Fork 27
Code Issues 100 Pull requests 4 Projects Releases 1 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 +89 -33
dstepanov-yadro commented 2024-10-28 12:58:15 +00:00
Member
Copy link
  1. Sign put EC parts requests with key of current node. This will fix adding HEAD requests from APE on PutSingle of EC part.
  2. Fail APE in case of error on EC headers collect.
1. Sign put EC parts requests with key of current node. This will fix adding HEAD requests from APE on PutSingle of EC part. 2. Fail APE in case of error on EC headers collect.
dstepanov-yadro added 2 commits 2024-10-28 12:58:15 +00:00 
As EC put request may be processed only by container node, so sign requests
with current node private to not to perform APE checks.

Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>
[#9999] ape: Perform strict APE checks for EC parts
All checks were successful
Tests and linters / Run gofumpt (pull_request) Successful in 1m17s
Details
DCO action / DCO (pull_request) Successful in 1m30s
Details
Pre-commit hooks / Pre-commit (pull_request) Successful in 2m13s
Details
Vulncheck / Vulncheck (pull_request) Successful in 2m11s
Details
Build / Build Components (pull_request) Successful in 2m35s
Details
Tests and linters / Staticcheck (pull_request) Successful in 2m51s
Details
Tests and linters / gopls check (pull_request) Successful in 2m54s
Details
Tests and linters / Lint (pull_request) Successful in 3m41s
Details
Tests and linters / Tests (pull_request) Successful in 4m19s
Details
Tests and linters / Tests with -race (pull_request) Successful in 5m53s
Details
2a3bc3a2c8 
Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>
dstepanov-yadro force-pushed fix/ec_ape_strict from 2a3bc3a2c8 to 196bb755c6 2024-10-28 13:19:52 +00:00 Compare
dstepanov-yadro reviewed 2024-10-28 13:21:57 +00:00
pkg/services/object/ape/request.go Outdated
@ -123,3 +126,3 @@
}
}
header = c.fillHeaderWithECParent(ctx, prm, header)
header, err := c.fillHeaderWithECParent(ctx, prm, header)
dstepanov-yadro commented 2024-10-28 13:21:57 +00:00
Author
Member
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() } } ```
dstepanov-yadro changed title from WIP: Strict APE check for EC & fix sign EC part put requests to Strict APE check for EC & fix sign EC part put requests 2024-10-28 13:41:01 +00:00
dstepanov-yadro requested review from storage-core-committers 2024-10-28 13:41:11 +00:00
dstepanov-yadro requested review from storage-core-developers 2024-10-28 13:41:12 +00:00
dstepanov-yadro changed title from Strict APE check for EC & fix sign EC part put requests to WIP: Strict APE check for EC & fix sign EC part put requests 2024-10-28 13:41:23 +00:00
dstepanov-yadro reviewed 2024-10-28 15:44:31 +00:00
pkg/services/object/common/writer/ec.go Outdated
@ -61,2 +63,4 @@
}
restoreTokens := e.CommonPrm.ForgetTokens()
defer restoreTokens()
dstepanov-yadro commented 2024-10-28 15:44:31 +00:00
Author
Member
Copy link

Restore tokens as in case of complex object use session token to store linking object (as it was before)

Restore tokens as in case of complex object use session token to store linking object (as it was before)
dstepanov-yadro changed title from WIP: Strict APE check for EC & fix sign EC part put requests to Strict APE check for EC & fix sign EC part put requests 2024-10-28 15:53:00 +00:00
acid-ant approved these changes 2024-10-29 05:53:46 +00:00
Dismissed
fyrchik commented 2024-10-29 08:02:29 +00:00
Owner
Copy link

What happens now if we PUT EC object via non-container node?

What happens now if we PUT EC object via non-container node?
dstepanov-yadro force-pushed fix/ec_ape_strict from 196bb755c6 to 5c5dfaaf35 2024-10-29 09:47:43 +00:00 Compare
dstepanov-yadro dismissed acid-ant's review 2024-10-29 09:47:43 +00:00
Reason:

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

dstepanov-yadro commented 2024-10-29 09:55:29 +00:00
Author
Member
Copy link

What happens now if we PUT EC object via non-container node?

It will fail. Fixed.

Also found a bug in traverser:

  1. Create traverser for container without object ID. Container node cache is empty, so netmap requested, stored in cache and returned to caller:

    Line 48 in 81f4cdb
    cn, err := b.containerCache.ContainerNodes(nm, cnr, p)

  2. BuildPlacement returns original slice (from cache):

    Line 58 in 81f4cdb
    return cnrNodes, nil

  3. Traverser changes t.vectors which links to cache's slice:

    Line 284 in 81f4cdb
    t.vectors[0] = t.vectors[0][count:]

  4. Create one more traverser for the same container but with object id. Container node cache returns empty slice, so traverser returns no placement nodes.

Also added check if all EC parts were processed by writer.

> What happens now if we PUT EC object via non-container node? It will fail. Fixed. Also found a bug in traverser: 1. Create traverser for container without object ID. Container node cache is empty, so netmap requested, stored in cache and returned to caller: https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/81f4cdbb91589ddab47ecde57992ef8de10e9ec0/pkg/services/object_manager/placement/netmap.go#L48 2. `BuildPlacement` returns original slice (from cache): https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/81f4cdbb91589ddab47ecde57992ef8de10e9ec0/pkg/services/object_manager/placement/netmap.go#L58 3. `Traverser` changes `t.vectors` which links to cache's slice: https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/81f4cdbb91589ddab47ecde57992ef8de10e9ec0/pkg/services/object_manager/placement/traverser.go#L284 4. Create one more traverser for the same container but with object id. Container node cache returns empty slice, so traverser returns no placement nodes. Also added check if all EC parts were processed by writer.
dstepanov-yadro force-pushed fix/ec_ape_strict from 5c5dfaaf35 to 37645a18b4 2024-10-29 10:01:01 +00:00 Compare
dstepanov-yadro force-pushed fix/ec_ape_strict from 37645a18b4 to 5ea53276c5 2024-10-29 10:04:50 +00:00 Compare
dstepanov-yadro changed title from Strict APE check for EC & fix sign EC part put requests to WIP: Strict APE check for EC & fix sign EC part put requests 2024-10-29 10:06:31 +00:00
dstepanov-yadro force-pushed fix/ec_ape_strict from 5ea53276c5 to 94c2b36ae0 2024-10-29 10:34:24 +00:00 Compare
dstepanov-yadro force-pushed fix/ec_ape_strict from 94c2b36ae0 to 9014cba4cd 2024-10-29 10:37:15 +00:00 Compare
dstepanov-yadro changed title from WIP: Strict APE check for EC & fix sign EC part put requests to Strict APE check for EC & fix sign EC part put requests 2024-10-29 10:38:50 +00:00
dstepanov-yadro force-pushed fix/ec_ape_strict from 9014cba4cd to 84bab017b8 2024-10-29 11:11:08 +00:00 Compare
acid-ant approved these changes 2024-10-29 12:32:36 +00:00
Dismissed
aarifullin approved these changes 2024-10-29 14:15:55 +00:00
Dismissed
dstepanov-yadro force-pushed fix/ec_ape_strict from 84bab017b8 to 177843aeb0 2024-10-30 06:54:12 +00:00 Compare
fyrchik reviewed 2024-10-30 07:01:49 +00:00
pkg/services/object/ape/checker_test.go Outdated
@ -695,6 +695,7 @@ func TestPutECChunk(t *testing.T) {
nm := &netmapStub{
currentEpoch: 100,
netmaps: map[uint64]*netmapSDK.NetMap{
99: netmap,
fyrchik commented 2024-10-30 06:59:12 +00:00
Owner
Copy link

Why this change is necessary?

Why this change is necessary?
dstepanov-yadro commented 2024-10-30 08:09:38 +00:00
Author
Member
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
pkg/services/object/ape/request.go Outdated
@ -156,3 +162,3 @@
}
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
Owner
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
Author
Member
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
Owner
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
Author
Member
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
Owner
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
Author
Member
Copy link

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

Removed pt.2 as it is checker before this function call.
dstepanov-yadro force-pushed fix/ec_ape_strict from 177843aeb0 to 5afbb6202b 2024-10-30 08:53:31 +00:00 Compare
dstepanov-yadro dismissed acid-ant's review 2024-10-30 08:53:31 +00:00
Reason:

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

dstepanov-yadro dismissed aarifullin's review 2024-10-30 08:53:31 +00:00
Reason:

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

aarifullin reviewed 2024-10-31 11:27:08 +00:00
pkg/services/object/common/writer/ec.go Outdated
@ -63,0 +69,4 @@
restoreTokens := e.CommonPrm.ForgetTokens()
defer restoreTokens()
// As request executed on container node, so sign request with container key.
e.remoteRequestSignKey, err = e.Config.KeyStorage.GetKey(nil)
aarifullin commented 2024-10-31 11:27:08 +00:00
Member
Copy link

Let's consider a situation:
ec-container: s01, s03, s04. We're sending a request signed by wallet.json. We pass through APE-check for s01, resign the request by s01 key and send writeECPart to s03, s04.
Imagine that at this time interval (30sec before) a local override has been added to s03 or s04 to restrict putting object to this container - the local override is totally ignored due to this actor role change.
I don't consider a situation when an ape chain is present within policy contract because it'd reject the request before this point and that's OK

Let's consider a situation: `ec-container: s01, s03, s04`. We're sending a request signed by `wallet.json`. We pass through APE-check for `s01`, _resign_ the request by `s01` key and send `writeECPart` to `s03, s04`. Imagine that at this time interval (30sec before) a local override has been added to `s03` or `s04` to restrict putting object to this container - the local override is totally ignored due to this actor role change. I don't consider a situation when an ape chain is present within `policy` contract because it'd reject the request before this point and that's OK
dstepanov-yadro commented 2024-10-31 12:49:19 +00:00
Author
Member
Copy link

I think it is ok as frostfs-storage is not strongly consistent system.
Also it depends on local override: if local override restricts put objects with owner, it will still work.

I think it is ok as frostfs-storage is not strongly consistent system. Also it depends on local override: if local override restricts put objects with owner, it will still work.
aarifullin commented 2024-10-31 13:31:47 +00:00
Member
Copy link

I think it is ok as frostfs-storage is not strongly consistent system.

It seems to be so

if local override restricts put objects with owner, it will still work

We won't get to the check at all: look at these lines

Anyway, currently I am not requesting to change this. We just need to keep this in mind

> I think it is ok as frostfs-storage is not strongly consistent system. It seems to be so > if local override restricts put objects with owner, it will still work We won't get to the check at all: [look at these lines](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/services/object/ape/checker.go#L85-L86) Anyway, currently I am not requesting to change this. We just need to keep this in mind
dstepanov-yadro commented 2024-10-31 13:41:14 +00:00
Author
Member
Copy link

Oh, right.
But as we discussed with @fyrchik we should try to check user's request once (on first container node now) to reduce unnecessary operations count.

Oh, right. But as we discussed with @fyrchik we should try to check user's request once (on first container node now) to reduce unnecessary operations count.
aarifullin commented 2024-10-31 15:04:42 +00:00
Member
Copy link

OK. I'll keep this comment open to not forget about this point in the future if we refer back to this PR

OK. I'll keep this comment open to not forget about this point in the future if we refer back to this PR
dstepanov-yadro force-pushed fix/ec_ape_strict from 5afbb6202b to 78858c629e 2024-10-31 12:52:25 +00:00 Compare
fyrchik reviewed 2024-10-31 12:57:58 +00:00
pkg/services/object_manager/placement/cache.go Outdated
@ -69,0 +71,4 @@
func (c *ContainerNodesCache) cloneResult(nodes [][]netmapSDK.NodeInfo) [][]netmapSDK.NodeInfo {
result := make([][]netmapSDK.NodeInfo, len(nodes))
for repIdx := range nodes {
result[repIdx] = make([]netmapSDK.NodeInfo, 0, len(nodes[repIdx]))
fyrchik commented 2024-10-31 12:57:47 +00:00
Owner
Copy link

slices.Clone?

`slices.Clone`?
dstepanov-yadro commented 2024-10-31 13:20:55 +00:00
Author
Member
Copy link

fixed

fixed
dstepanov-yadro force-pushed fix/ec_ape_strict from 78858c629e to 765c81ad2a 2024-10-31 13:20:49 +00:00 Compare
aarifullin approved these changes 2024-10-31 15:04:53 +00:00
acid-ant approved these changes 2024-10-31 16:37:47 +00:00
dstepanov-yadro force-pushed fix/ec_ape_strict from 765c81ad2a to b4adf43557 2024-11-06 07:42:23 +00:00 Compare
fyrchik approved these changes 2024-11-06 08:17:58 +00:00
fyrchik merged commit 7edec9193c into master 2024-11-06 08:18:11 +00:00
fyrchik referenced this pull request from a commit 2024-11-06 08:18:13 +00:00
[#1451] ape: Drop unused
fyrchik referenced this pull request from a commit 2024-11-06 08:18:13 +00:00
[#1451] ec: Check all parts are saved
Sign in to join this conversation.
Reviewers
No reviewers
aarifullin
acid-ant
fyrchik
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
badger

Badger DB related issues

  
frostfs-adm

   
frostfs-cli

   
frostfs-ir

   
frostfs-lens

   
frostfs-node

   
good first issue

Good for newcomers

  
triage
  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff

  
blocked

The issue or PR is blocked by something

  
bug

Something is not working

  
config

Configuration format update or breaking change

  
discussion

Talking about something in order to reach a decision or to exchange ideas

  
documentation

Documentation related

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
go

Language features adoption

  
help wanted

Extra attention is needed

  
internal

   
invalid

Something is wrong

  
kludge

This is a kludge and we know it

  
observability

Related to metrics, tracing and logs

  
perfomance

Perfomance related

  
question

More information is needed

  
refactoring

Refactoring

  
wontfix

This won't be fixed

No labels
P0
P1
P2
P3
badger
frostfs-adm
frostfs-cli
frostfs-ir
frostfs-lens
frostfs-node
good first issue
triage
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
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-node#1451
No description provided.
Delete branch "dstepanov-yadro/frostfs-node:fix/ec_ape_strict"

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?