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
  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
DCO action / DCO (pull_request) Successful in 1m30s
Pre-commit hooks / Pre-commit (pull_request) Successful in 2m13s
Vulncheck / Vulncheck (pull_request) Successful in 2m11s
Build / Build Components (pull_request) Successful in 2m35s
Tests and linters / Staticcheck (pull_request) Successful in 2m51s
Tests and linters / gopls check (pull_request) Successful in 2m54s
Tests and linters / Lint (pull_request) Successful in 3m41s
Tests and linters / Tests (pull_request) Successful in 4m19s
Tests and linters / Tests with -race (pull_request) Successful in 5m53s
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
@ -123,3 +126,3 @@
}
}
header = c.fillHeaderWithECParent(ctx, prm, header)
header, err := c.fillHeaderWithECParent(ctx, prm, header)
Author
Member

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
@ -61,2 +63,4 @@
}
restoreTokens := e.CommonPrm.ForgetTokens()
defer restoreTokens()
Author
Member

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
Owner

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

Author
Member

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:

    cn, err := b.containerCache.ContainerNodes(nm, cnr, p)

  2. BuildPlacement returns original slice (from cache):

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

    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
@ -695,6 +695,7 @@ func TestPutECChunk(t *testing.T) {
nm := &netmapStub{
currentEpoch: 100,
netmaps: map[uint64]*netmapSDK.NetMap{
99: netmap,
Owner

Why this change is necessary?

Why this change is necessary?
Author
Member

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
@ -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) {
Owner

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?
Author
Member
  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
Owner

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?
Author
Member

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.
Owner

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.
Author
Member

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
@ -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)
Member

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
Author
Member

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.
Member

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
Author
Member

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.
Member

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
@ -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]))
Owner

slices.Clone?

`slices.Clone`?
Author
Member

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
fyrchik referenced this pull request from a commit 2024-11-06 08:18:13 +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-node#1451
No description provided.