fix/Child objects ACL #2716

carpawell · 2024-01-12T02:27:28Z

No description provided.

codecov · 2024-02-27T20:20:23Z

Codecov Report

Attention: Patch coverage is 14.28571% with 468 lines in your changes are missing coverage. Please review.

Project coverage is 22.00%. Comparing base (9c6573c) to head (d309b9d).

Files	Patch %	Lines
pkg/services/object/util/chain.go	0.00%	97 Missing ⚠️
pkg/services/object/split/verify.go	0.00%	86 Missing ⚠️
pkg/services/object/get/assembly_v2.go	21.87%	75 Missing ⚠️
cmd/neofs-node/object.go	0.00%	47 Missing ⚠️
cmd/neofs-cli/modules/object/util.go	0.00%	42 Missing ⚠️
pkg/services/object/delete/util.go	0.00%	26 Missing ⚠️
cmd/neofs-cli/modules/storagegroup/put.go	0.00%	23 Missing ⚠️
pkg/core/object/fmt.go	52.63%	17 Missing and 1 partial ⚠️
pkg/local_object_storage/metabase/put.go	30.00%	8 Missing and 6 partials ⚠️
pkg/services/object/acl/eacl/v2/headers.go	44.44%	7 Missing and 3 partials ⚠️
... and 11 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2716      +/-   ##
==========================================
- Coverage   22.04%   22.00%   -0.04%     
==========================================
  Files         787      789       +2     
  Lines       46738    47212     +474     
==========================================
+ Hits        10304    10390      +86     
- Misses      35552    35930     +378     
- Partials      882      892      +10

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

carpawell · 2024-02-29T20:30:40Z

Well, I need Go 1.20 now. Otherwise, I am ready.

carpawell · 2024-03-01T15:54:09Z

Not sure what is wrong with the tests: it creates REP 2 IN X CBF 1 SELECT 4 FROM * AS X container PUTs a complex object and HEADs its parts with TTL 1 to every container node (2 of 4 answers correct, the other 2 say 404). Need to be investigated, not sure why all 4 nodes are expected to answer. Generally can be reviewed, draft status is caused by that strange test.

roman-khimov

@cthulhu-rider, join the party. We need to merge this ASAP to unblock SDK updates.

roman-khimov · 2024-03-05T20:23:27Z

cmd/neofs-node/object.go

+}
+
+func (h headerSource) Head(address oid.Address) (*objectSDK.Object, error) {
+	if h.cache != nil {


Why would it be nil?

cacheSize may be zero so no caching is done then. hardcoded for now but may be useful in the future

Seems very artificial to me (likely cache can have zero size and still be instantiated), but OK.

we have a config param that explicitly says "no caching, please" (but for morph values mostly)

cacheSize may be zero

i meant you can change one const and it is not caching anymore, you may make it configurable and just init it with a value you read and zero value is already supported. i agree, it may look strange without a real use-case and config param but since it is already done, why not?

roman-khimov · 2024-03-05T20:27:39Z

pkg/local_object_storage/metabase/VERSION.md

@@ -57,6 +57,10 @@ Numbers stand for a single byte value.
  - Name: container ID + `9`
  - Key: object ID
  - Value: marshaled object
+- Buckets containing object or LINK type
+  - Name: container ID + `18`


I thought they're sorted here.

they have a logical title (Unique index buckets) so i decided them to be together

pkg/local_object_storage/metabase/put.go

pkg/services/object/split/verify.go

roman-khimov · 2024-03-05T20:42:30Z

pkg/services/object/split/verify.go

+			}
+
+			var err error
+			receivedObjects[iCopy], err = v.verifySinglePart(ctx, cnr, shouldHaveFirstObject, childrenFromLink[iCopy])


1M * 16K for headers?

how child objects may be 16K? optimizations may be presented here but not sure they changes smth for now (64mb are required for the most optimal part of the object storing if NO get-check-drop is implemented and all the objects are fetched before any validation is being done)

Fetch and verify group by group, headers are headers and can be up to 16KB.

This is still a problem, but can be left for another issue.

in fact just missed that comment. done in this PR

cthulhu-rider · 2024-03-06T08:40:51Z

@roman-khimov affirmative

@carpawell wt readiness? PR is still draft

carpawell · 2024-03-06T17:29:51Z

@carpawell wt readiness? PR is still draft

Basically can be reviewed since the push that allowed building. Some fixes are done (and will be done but are based on my work with int test cases) and after the review (when the base code is fixed), I would add some missing unit tests.

cthulhu-rider

finished small commits, will return to the main one

pkg/local_object_storage/metabase/select.go

cmd/neofs-cli/modules/object/head.go

cmd/neofs-cli/modules/object/util.go

roman-khimov · 2024-03-12T16:22:20Z

cmd/neofs-node/object.go

+}
+
+func (h headerSource) Head(address oid.Address) (*objectSDK.Object, error) {
+	if h.cache != nil {


Seems very artificial to me (likely cache can have zero size and still be instantiated), but OK.

roman-khimov · 2024-03-12T18:26:21Z

pkg/core/object/fmt.go

+		if splitID != nil {
+			// V1 split
+			if firstSet {
+				return errors.New("v1 split: first ID object is set")


First object ID?

roman-khimov · 2024-03-12T20:10:52Z

pkg/services/object/util/chain.go

 	}

-	for i := len(leaves) - 1; i >= 0; i-- {
-		if h(leaves[i]) {
+	return errors.New("nor link, nor last object ID is found")


neither nor

roman-khimov · 2024-03-12T20:12:17Z

pkg/services/object/split/verify.go

+			}
+
+			var err error
+			receivedObjects[iCopy], err = v.verifySinglePart(ctx, cnr, shouldHaveFirstObject, childrenFromLink[iCopy])


This is still a problem, but can be left for another issue.

cthulhu-rider

huge amount of work has been done, such that I am not able to closely examine some of its parts. These are my final comments, lets merge after them

pkg/services/object/util/chain.go

cthulhu-rider · 2024-03-14T10:44:01Z

pkg/services/object/util/chain.go

+	for i := len(idBuff) - 1; i >= 0; i-- {
+		addr.SetObject(idBuff[i])
+
+		childObj, err := headFromInformer(r, addr)


didnt we do this in the previous loop?

agree, looks ugly. but what if we have 1M 16K children as @roman-khimov loves? saving 1M 32-bytes IDs would be better than storing so many headers. not sure what is worse: duplicating RPCs or having 16GB memory for simple SG object put

also, this case is not expected since link object are always preferable

also, we are not expecting IDs to be in the right order in SGs as i remember

also, we are not expecting to have ID in the right order in SG as i remember

woops, no: https://github.com/nspcc-dev/neofs-api/blob/5c3535423c564fe63991812cb84d8334db1c553a/storagegroup/types.proto#L32

pkg/services/object/util/chain.go

pkg/local_object_storage/metabase/util.go

pkg/services/object/get/assembly_v2.go

pkg/services/object/get/assemble.go

For the updated object split API mainly. Signed-off-by: Pavel Karpy <[email protected]>

This buckets may be missing on if no objects of a certain type have been put yet. Signed-off-by: Pavel Karpy <[email protected]>

Signed-off-by: Pavel Karpy <[email protected]>

On reading: Support both split chain versions a new and an old one. On writing: Split objects only with the V2 version: 1. Link objects do not have child objects in their headers, payload is used instead 2. Link objects contain child objects' sizes along with the IDs 3. SplitID is not attached to the objects anymore 4. The First part's ID of a big object is attached to every split part (a link has it too, but obviously the first part itself does not have its ID in its split header) 5. Receiving link object triggers verification of its children: they should have the same first object and have the same payload size as the link declares 6. The First object part now has an original object's paylaod (without payload-dependent fields set). Closes #2667. Signed-off-by: Pavel Karpy <[email protected]>

Search for child in link object's payload, not its header. Signed-off-by: Pavel Karpy <[email protected]>

They are used for session tokens. Sessions may be reconsidered in the future, but for now big objects need to be sessioned part by part. Signed-off-by: Pavel Karpy <[email protected]>

…nough Signed-off-by: Pavel Karpy <[email protected]>

It was easier to implement it from scratch than to try to understand callbacks and recursion. Signed-off-by: Pavel Karpy <[email protected]>

carpawell self-assigned this Jan 12, 2024

carpawell force-pushed the fix/child-object-acl branch 2 times, most recently from 9a60f81 to 960fe98 Compare February 27, 2024 20:16

carpawell force-pushed the fix/child-object-acl branch 4 times, most recently from daa352f to e20f436 Compare February 29, 2024 20:12

carpawell force-pushed the fix/child-object-acl branch from e20f436 to 770824a Compare March 1, 2024 13:58

carpawell requested review from roman-khimov and cthulhu-rider March 1, 2024 15:55

carpawell force-pushed the fix/child-object-acl branch from 770824a to c7e7137 Compare March 1, 2024 17:19

cthulhu-rider mentioned this pull request Mar 5, 2024

Use new API for background replication #2674

Merged

roman-khimov reviewed Mar 5, 2024

View reviewed changes

carpawell force-pushed the fix/child-object-acl branch from c7e7137 to d674736 Compare March 6, 2024 16:57

carpawell force-pushed the fix/child-object-acl branch 2 times, most recently from 98e3aa3 to 4278283 Compare March 6, 2024 20:32

cthulhu-rider requested changes Mar 7, 2024

View reviewed changes

carpawell force-pushed the fix/child-object-acl branch 4 times, most recently from 3edac98 to 40ffda7 Compare March 12, 2024 00:13

carpawell marked this pull request as ready for review March 12, 2024 00:29

carpawell requested review from roman-khimov and cthulhu-rider March 12, 2024 00:29

roman-khimov reviewed Mar 12, 2024

View reviewed changes

roman-khimov approved these changes Mar 12, 2024

View reviewed changes

cthulhu-rider reviewed Mar 14, 2024

View reviewed changes

carpawell force-pushed the fix/child-object-acl branch 2 times, most recently from 9d0c2dd to 37e3949 Compare March 14, 2024 23:52

cthulhu-rider approved these changes Mar 18, 2024

View reviewed changes

carpawell added 3 commits March 20, 2024 19:37

go.mod: Update SDK to its latest version

285c22f

For the updated object split API mainly. Signed-off-by: Pavel Karpy <[email protected]>

meta: Do not stop selecting objects on bucket absence

f6bab60

This buckets may be missing on if no objects of a certain type have been put yet. Signed-off-by: Pavel Karpy <[email protected]>

cli: Add first child ID to the output if set

c2ec641

Signed-off-by: Pavel Karpy <[email protected]>

carpawell force-pushed the fix/child-object-acl branch 2 times, most recently from 0d489a2 to fb9b4e7 Compare March 20, 2024 17:38

carpawell added 5 commits March 20, 2024 21:25

deleteSvc: Support V2 split object removal

61c079b

Search for child in link object's payload, not its header. Signed-off-by: Pavel Karpy <[email protected]>

cli: Collect child ID objects according to V2 split too

73b22dc

They are used for session tokens. Sessions may be reconsidered in the future, but for now big objects need to be sessioned part by part. Signed-off-by: Pavel Karpy <[email protected]>

node: Do not search children by SplitID if link or last objects are e…

06f8768

…nough Signed-off-by: Pavel Karpy <[email protected]>

cli: Collect storagegroup members with new V2 split

d309b9d

It was easier to implement it from scratch than to try to understand callbacks and recursion. Signed-off-by: Pavel Karpy <[email protected]>

carpawell force-pushed the fix/child-object-acl branch from fb9b4e7 to d309b9d Compare March 20, 2024 18:32

roman-khimov merged commit 9ca1fbd into master Mar 20, 2024
13 of 16 checks passed

roman-khimov deleted the fix/child-object-acl branch March 20, 2024 19:06

carpawell mentioned this pull request Mar 21, 2024

Storage group (for complex object) test cannot live without children in the link's payload (split V2) nspcc-dev/neofs-testcases#755

Closed

carpawell mentioned this pull request Apr 23, 2024

Test attribute-based eACL for big objects nspcc-dev/neofs-testcases#786

Closed

carpawell mentioned this pull request May 3, 2024

GC object parts test nspcc-dev/neofs-testcases#794

Closed

carpawell mentioned this pull request May 31, 2024

GC has not collected an expired object #2858

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix/Child objects ACL #2716

fix/Child objects ACL #2716

carpawell commented Jan 12, 2024

codecov bot commented Feb 27, 2024 •

edited

Loading

carpawell commented Feb 29, 2024

carpawell commented Mar 1, 2024

roman-khimov left a comment

roman-khimov Mar 5, 2024

carpawell Mar 6, 2024 •

edited

Loading

roman-khimov Mar 12, 2024

carpawell Mar 14, 2024

roman-khimov Mar 5, 2024

carpawell Mar 6, 2024

roman-khimov Mar 5, 2024

carpawell Mar 6, 2024

roman-khimov Mar 6, 2024

roman-khimov Mar 12, 2024

carpawell Mar 20, 2024

cthulhu-rider commented Mar 6, 2024

carpawell commented Mar 6, 2024 •

edited

Loading

cthulhu-rider left a comment

roman-khimov Mar 12, 2024

roman-khimov Mar 12, 2024

roman-khimov Mar 12, 2024

roman-khimov Mar 12, 2024

cthulhu-rider left a comment

cthulhu-rider Mar 14, 2024

carpawell Mar 14, 2024 •

edited

Loading

carpawell Mar 14, 2024

fix/Child objects ACL #2716

fix/Child objects ACL #2716

Conversation

carpawell commented Jan 12, 2024

codecov bot commented Feb 27, 2024 • edited Loading

Codecov Report

carpawell commented Feb 29, 2024

carpawell commented Mar 1, 2024

roman-khimov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carpawell Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cthulhu-rider commented Mar 6, 2024

carpawell commented Mar 6, 2024 • edited Loading

cthulhu-rider left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cthulhu-rider left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carpawell Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 27, 2024 •

edited

Loading

carpawell Mar 6, 2024 •

edited

Loading

carpawell commented Mar 6, 2024 •

edited

Loading

carpawell Mar 14, 2024 •

edited

Loading