feat: P2P ACP #3317

fredcarle · 2024-12-11T18:47:32Z

Relevant issue(s)

Resolves #3179

Description

This PR brings ACP functionality to the P2P system. This ensures that permissioned collections do not share blocks of registered documents without ensuring the requesting node has access. This is bypassed on collection replication because the node sending the blocks is sending to peers explicitly.

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

Some new and updated integration tests.

Specify the platform(s) on which this was tested:

MacOS

codecov · 2024-12-11T19:04:42Z

Codecov Report

Attention: Patch coverage is 69.52381% with 96 lines in your changes missing coverage. Please review.

Project coverage is 78.22%. Comparing base (442bc57) to head (99be9d1).
Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
net/server.go	57.14%	40 Missing and 17 partials ⚠️
net/grpc.go	45.45%	11 Missing and 1 partial ⚠️
net/client.go	52.63%	7 Missing and 2 partials ⚠️
acp/identity/identity.go	63.64%	6 Missing and 2 partials ⚠️
internal/db/db.go	82.61%	4 Missing ⚠️
net/errors.go	0.00%	2 Missing ⚠️
internal/db/p2p_replicator.go	91.67%	1 Missing ⚠️
internal/db/request.go	50.00%	1 Missing ⚠️
net/sync_dag.go	80.00%	0 Missing and 1 partial ⚠️
node/node.go	92.86%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3317      +/-   ##
===========================================
- Coverage    78.41%   78.22%   -0.19%     
===========================================
  Files          392      392              
  Lines        35716    35905     +189     
===========================================
+ Hits         28006    28086      +80     
- Misses        6072     6150      +78     
- Partials      1638     1669      +31

Flag	Coverage Δ
all-tests	`78.22% <69.52%> (-0.19%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
http/auth.go	`57.89% <100.00%> (-7.73%)`	⬇️
internal/db/backup.go	`59.43% <100.00%> (ø)`
internal/db/collection.go	`69.87% <100.00%> (ø)`
internal/db/collection_define.go	`72.24% <ø> (ø)`
internal/db/collection_id.go	`79.17% <100.00%> (ø)`
internal/db/collection_index.go	`87.50% <ø> (ø)`
internal/db/definition_validation.go	`94.77% <ø> (ø)`
internal/db/lens.go	`70.80% <100.00%> (ø)`
internal/db/merge.go	`75.99% <100.00%> (-0.79%)`	⬇️
internal/db/messages.go	`100.00% <100.00%> (ø)`
... and 18 more

... and 11 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 442bc57...99be9d1. Read the comment docs.

islamaliev

Look good overall. Good job, Fred!

I left some comments and suggestions.

In general I think it would be also nice to have it written down somewhere how the new bitswap-level ACP works together with the pubsub KMS. Otherwise it's not clear when to use what.

internal/db/permission/check.go

net/acp.go

net/server.go

tests/integration/acp.go

net/acp.go

tests/integration/acp/p2p/create_test.go

net/server.go

net/peer.go

internal/db/permission/check.go

internal/db/db.go

shahzadlone · 2024-12-17T17:03:30Z

internal/db/db.go

@@ -339,13 +339,20 @@ func (db *db) DeleteDocActorRelationship(
 	return client.DeleteDocActorRelationshipResult{RecordFound: recordFound}, nil
 }

-func (db *db) GetNodeIdentity(context.Context) (immutable.Option[identity.PublicRawIdentity], error) {
+func (db *db) GetNodeIdentity(_ context.Context) (immutable.Option[identity.PublicRawIdentity], error) {


question: What is with the underscore _? Why was this added?

Because just context.Context seems like a mistake. The underscore says that the function parameters are this way because we're following some interface pattern or something like that but we aren't using this parameter within the function.

This is a known go idiom?

It is the same as the other areas we use _ to ignore a returned argument. https://go.dev/ref/spec#Blank_identifier

The only reason why just using context.Context worked is because it was the only parameter in the method. If there was more than one, it would be a syntax error.

Unused function parameters are allowed in Go and that's why we often have ctx context.Context as parameter with ctx never being used. I feel that's misleading. Doing _ context.Context at least tells the reader that this parameter needed to be there but we aren't using it.

shahzadlone · 2024-12-17T17:26:00Z

internal/db/db.go

+func (db *db) GetIdentityToken(_ context.Context, audience immutable.Option[string]) ([]byte, error) {
+	if db.nodeIdentity.HasValue() {
+		return db.nodeIdentity.Value().NewToken(time.Hour*24, audience, immutable.None[string]())
+	}
+	return nil, nil
+}


suggestion: Remove the context.Context arg? doesn't seem to be needed anywhere?

It's a pattern we decided to keep after a discussion we had several months back about removing context. Happy to bring that up for discussion but for now leaving as is.

shahzadlone · 2024-12-17T17:36:02Z

internal/db/permission/check.go

+// - Document is public (unregistered), whether signatured request or not doesn't matter.
+func CheckDocAccessWithIdentityFunc(
+	ctx context.Context,
+	identityFunc func() immutable.Option[acpIdentity.Identity],


question: Why did we need to do this function thing? instead of just immutable.Option[acpIdentity.Identity]

Because in some cases in doesn't make sense to do an extra network call for the identity before knowing if the document is even registered.

Can you explain a bit more? Which document is registered check are we talking about? and which network call for identity?

In the net package, when we want to check if a peer has access to a doc we have to request the identity from the node requesting. That's a network call. If we request it before checking that the document is registered, then we might have done that network call for nothing. So the order of priority should be checking that the doc is registered and then requesting the identity to check if it has access.

Is the identity not included from the node requesting from the get go? Maybe I am missing something, why does it need to be requested again?

I'd also like clarification here :)

net/grpc.go

node/node.go

internal/db/permission/check.go

net/server.go

nasdf

Implementation looks solid. One blocking issue to resolve.

net/peer.go

net/server.go

shahzadlone

LGTM, thanks for answering all the questions, just left some minor suggestions. Please do resolve the on going discussions before merge from others.

todo: Please make the PR title more descriptive right now it only says P2P ACP which is quite vague for example it can be confused with the initial P2P acp work that has been done for #2366

AndrewSisley · 2025-02-19T18:12:04Z

Bug bash result: No bugs found.

fredcarle added the feature New feature or request label Dec 11, 2024

fredcarle requested a review from a team December 11, 2024 18:47

fredcarle self-assigned this Dec 11, 2024

fredcarle force-pushed the fredcarle/feat/3179-p2p-acp branch from 25d14f1 to 3de2aaf Compare December 11, 2024 20:40

islamaliev reviewed Dec 12, 2024

View reviewed changes

shahzadlone reviewed Dec 12, 2024

View reviewed changes

internal/db/permission/check.go Outdated Show resolved Hide resolved

fredcarle requested review from shahzadlone and islamaliev December 12, 2024 22:44

fredcarle force-pushed the fredcarle/feat/3179-p2p-acp branch 3 times, most recently from d35d257 to eae46e0 Compare December 13, 2024 02:36

shahzadlone reviewed Dec 17, 2024

View reviewed changes

node/node.go Show resolved Hide resolved

shahzadlone reviewed Dec 17, 2024

View reviewed changes

internal/db/permission/check.go Outdated Show resolved Hide resolved

shahzadlone reviewed Dec 17, 2024

View reviewed changes

net/server.go Show resolved Hide resolved

net/server.go Show resolved Hide resolved

net/server.go Outdated Show resolved Hide resolved

net/server.go Show resolved Hide resolved

net/server.go Outdated Show resolved Hide resolved

net/server.go Show resolved Hide resolved

AndrewSisley reviewed Dec 18, 2024

View reviewed changes

net/server.go Outdated Show resolved Hide resolved

AndrewSisley reviewed Dec 18, 2024

View reviewed changes

net/server.go Outdated Show resolved Hide resolved

fredcarle force-pushed the fredcarle/feat/3179-p2p-acp branch from eae46e0 to 137e97e Compare December 20, 2024 20:33

nasdf requested changes Dec 30, 2024

View reviewed changes

net/peer.go Show resolved Hide resolved

net/server.go Outdated Show resolved Hide resolved

net/server.go Show resolved Hide resolved

shahzadlone approved these changes Dec 30, 2024

View reviewed changes

fredcarle force-pushed the fredcarle/feat/3179-p2p-acp branch from 137e97e to 242ba5e Compare January 17, 2025 21:00

fredcarle added 8 commits January 17, 2025 16:03

add P2P ACP

37dc9c9

remove unused function

8dc9f92

apply feedback

9a4911a

change db to internal public DB

229f48a

clean up for net.DB interface

105ffed

apply feedback

1065ada

add DB client.DB implementation compile time check

349740f

fix thead safety

196ce95

fredcarle force-pushed the fredcarle/feat/3179-p2p-acp branch from 242ba5e to 196ce95 Compare January 17, 2025 21:03

fredcarle added 2 commits January 17, 2025 21:09

fix net tests

c1e8b3d

change to schema root for trySelfHasAccess

99be9d1

fredcarle requested a review from nasdf January 20, 2025 16:42

nasdf approved these changes Jan 20, 2025

View reviewed changes

fredcarle merged commit bcf91d3 into sourcenetwork:develop Jan 20, 2025
41 of 43 checks passed

fredcarle deleted the fredcarle/feat/3179-p2p-acp branch January 20, 2025 17:58

fredcarle added this to the DefraDB v0.16 milestone Feb 18, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: P2P ACP #3317

feat: P2P ACP #3317

fredcarle commented Dec 11, 2024

codecov bot commented Dec 11, 2024 •

edited

Loading

islamaliev left a comment

shahzadlone Dec 17, 2024

fredcarle Dec 17, 2024

shahzadlone Dec 19, 2024

fredcarle Dec 19, 2024

shahzadlone Dec 17, 2024

fredcarle Dec 17, 2024

shahzadlone Dec 17, 2024

fredcarle Dec 17, 2024

shahzadlone Dec 19, 2024

fredcarle Dec 20, 2024

shahzadlone Dec 30, 2024

jsimnz Dec 30, 2024

nasdf left a comment

shahzadlone left a comment

AndrewSisley commented Feb 19, 2025

feat: P2P ACP #3317

feat: P2P ACP #3317

Conversation

fredcarle commented Dec 11, 2024

Relevant issue(s)

Description

Tasks

How has this been tested?

codecov bot commented Dec 11, 2024 • edited Loading

Codecov Report

islamaliev 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

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

nasdf left a comment

Choose a reason for hiding this comment

shahzadlone left a comment

Choose a reason for hiding this comment

AndrewSisley commented Feb 19, 2025

codecov bot commented Dec 11, 2024 •

edited

Loading