feat: Handle P2P with SourceHub ACP #2848

AndrewSisley · 2024-07-18T20:49:46Z

Relevant issue(s)

Resolves #2366

Description

Handles P2P with SourceHub ACP.

Local ACP remains blocked off, as documents synced would essentially become public unless encrypted.

The testing assumes that a distributed SourceHub chain would be tested in the SourceHub repo, and so it doesn't bother spinning up a chain per node and dealing with SourceHub sync stuff. Long term we'll want some Defra-based testing around this, but I think we can get away without it. Feel very free to argue against this :)

Instead of once per node

codecov · 2024-07-18T21:10:40Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.29%. Comparing base (517333c) to head (5af21e6).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2848      +/-   ##
===========================================
- Coverage    79.40%   79.29%   -0.11%     
===========================================
  Files          323      323              
  Lines        24691    24693       +2     
===========================================
- Hits         19604    19578      -26     
- Misses        3684     3698      +14     
- Partials      1403     1417      +14

Flag	Coverage Δ
all-tests	`79.29% <100.00%> (-0.11%)`	⬇️

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

Files	Coverage Δ
acp/source_hub_client.go	`88.19% <100.00%> (+0.25%)`	⬆️
internal/db/errors.go	`65.68% <ø> (ø)`
internal/db/p2p_replicator.go	`74.88% <100.00%> (+0.72%)`	⬆️
internal/db/p2p_schema_root.go	`77.87% <100.00%> (+0.18%)`	⬆️

... and 13 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 517333c...5af21e6. Read the comment docs.

islamaliev

LGTM! Left a few comments

islamaliev · 2024-07-18T20:58:21Z

tests/integration/acp.go

+		if acpType == SourceHubACPType {
+			addedToSourceHub = true
+		}


suggestion: why not to put in the end of the loop body this:

if acpType == SourceHubACPType { break }

todo: and please leave a short comment reminding why we do it

why not to put in the end of the loop body this

That would be much nicer lol, thanks 😁

Simplify addedToSourceHub stuff

islamaliev · 2024-07-18T21:03:07Z

tests/integration/acp/p2p/replicator_test.go

+			testUtils.AddPolicy{
+				Identity: immutable.Some(1),
+				Policy: `
+                    name: test


suggestion: this seems like a regular policy and doesn't really improve testing readability. Would be nice to extract it to a const.

Going to shrug this one off as personal preference, as I prefer it inline - partly so that I can see that it is just a regular policy. I could store it in a var called totallyNormalAndUnimportantUserPolicy, but that would make me even more suspicious and would cause me to read it even more carefully than if it was inline (and I'd have to bounce around out of the test to do so) :)

Happy as is

nasdf

Changes look good! One potential issue to resolve.

nasdf · 2024-07-18T23:50:33Z

internal/db/p2p_replicator.go

-			// TODO-ACP: remove this check/loop after https://github.com/sourcenetwork/defradb/issues/2366
-			if col.Description().Policy.HasValue() {
-				return ErrReplicatorSomeColsHavePolicy
+		if db.acp.HasValue() && !db.acp.Value().SupportsP2P() {


todo: the switch case above this also checks for a policy (unsure why there is not a todo there). It might be cleaner to combine both checks outside of the switch case

Nice spot! Yeah I'll pull them both out of the switch and remove one of the error types. Thanks Keenan :)

replicator error stuff

shahzadlone

Lovely work, finally ties a lot of pieces together.

Just left some todos and questions. Please resolve before merge.

shahzadlone · 2024-07-19T09:55:59Z

acp/README.md

suggestion: Would be nice to add a complete p2p flow example using sourcehub acp below under a new heading called Sourcehub P2P or something, similar to how we have other examples in this README.md

I think that is too lengthy to include in this document and it will help drown out the information currently contained within it.

shahzadlone · 2024-07-19T10:03:02Z

internal/db/p2p_schema_root_test.go

-// This test documents that we don't allow adding p2p collections that have a policy
-// until the following is implemented:


todo: Still makes sense to have the comment that this test documents that we don't allow adding p2p collections that have a policy for local acp.

I will delete the test

Delete test

shahzadlone · 2024-07-19T10:04:24Z

tests/integration/acp.go

@@ -110,6 +111,12 @@ func addPolicyACP(

 		expectedErrorRaised := AssertError(s.t, s.testCase.Description, err, action.ExpectedError)
 		assertExpectedErrorRaised(s.t, s.testCase.Description, action.ExpectedError, expectedErrorRaised)
+
+		// The policy should only be added to a SourceHub chain once - there is no need to loop through


question: What happens if a node tries to re-add the same exact policy ? Guessing it will output a new policyID and still add the policy?

That has not changed, the test framework will still attempt to re-add the policy.

shahzadlone · 2024-07-19T10:05:56Z

tests/integration/acp/p2p/replicator_test.go

-// until the following is implemented:
-// TODO-ACP: ACP <> P2P https://github.com/sourcenetwork/defradb/issues/2366
-func TestACP_P2POneToOneReplicatorWithPermissionedCollection_Error(t *testing.T) {
+func TestACP_P2POneToOneReplicatorWithPermissionedCollection_LocalACP(t *testing.T) {


todo: I like the documentation on what the test does (can just add that its for local acp case)

I dont think it is worth the eye/brain-space here. This one-liner should have been in the description, and I have a large dislike for the description prop as they are very often misleading.

It will save us time in the long run to let the test definition define itself.

shahzadlone · 2024-07-19T10:07:01Z

tests/integration/acp/p2p/replicator_test.go

+			testUtils.AddPolicy{
+				Identity: immutable.Some(1),
+				Policy: `
+                    name: test


Happy as is

AndrewSisley added feature New feature or request area/auth Related to the authorization and authentication of data labels Jul 18, 2024

AndrewSisley added this to the DefraDB v0.13 milestone Jul 18, 2024

AndrewSisley requested a review from a team July 18, 2024 20:49

AndrewSisley self-assigned this Jul 18, 2024

AndrewSisley added 3 commits July 18, 2024 16:50

Setup sourcehub once per test

313f06e

Instead of once per node

Only define policy once if testing sourcehub

92522f4

Handle ACP and P2P

103a658

AndrewSisley force-pushed the 2366-p2p-acp branch from fb4e41f to 103a658 Compare July 18, 2024 20:50

AndrewSisley changed the title ~~feat: Handle P2P with SourceHub ACP~~ feature: Handle P2P with SourceHub ACP Jul 18, 2024

AndrewSisley changed the title ~~feature: Handle P2P with SourceHub ACP~~ feature: Handle P2P with SourceHub ACP Jul 18, 2024

AndrewSisley changed the title ~~feature: Handle P2P with SourceHub ACP~~ feat: Handle P2P with SourceHub ACP Jul 18, 2024

islamaliev approved these changes Jul 18, 2024

View reviewed changes

PR FIXUP - Simplify addPolicy test loop

ba694b9

nasdf requested changes Jul 18, 2024

View reviewed changes

shahzadlone approved these changes Jul 19, 2024

View reviewed changes

AndrewSisley added 2 commits July 19, 2024 11:09

PR FIXUP - Fix replicator schema branch

1f4c22f

PR FIXUP - Delete integration test written outside test framework

5af21e6

AndrewSisley requested a review from nasdf July 19, 2024 15:15

nasdf approved these changes Jul 19, 2024

View reviewed changes

AndrewSisley merged commit 25a3063 into sourcenetwork:develop Jul 19, 2024
40 of 42 checks passed

AndrewSisley deleted the 2366-p2p-acp branch July 19, 2024 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Handle P2P with SourceHub ACP #2848

feat: Handle P2P with SourceHub ACP #2848

AndrewSisley commented Jul 18, 2024 •

edited

Loading

codecov bot commented Jul 18, 2024 •

edited

Loading

islamaliev left a comment

islamaliev Jul 18, 2024

AndrewSisley Jul 18, 2024 •

edited

Loading

islamaliev Jul 18, 2024

AndrewSisley Jul 18, 2024

shahzadlone Jul 19, 2024

nasdf left a comment

nasdf Jul 18, 2024

AndrewSisley Jul 19, 2024 •

edited

Loading

shahzadlone left a comment

shahzadlone Jul 19, 2024

AndrewSisley Jul 19, 2024

shahzadlone Jul 19, 2024

AndrewSisley Jul 19, 2024 •

edited

Loading

shahzadlone Jul 19, 2024

AndrewSisley Jul 19, 2024

shahzadlone Jul 19, 2024

AndrewSisley Jul 19, 2024

shahzadlone Jul 19, 2024

		// This test documents that we don't allow adding p2p collections that have a policy
		// until the following is implemented:

feat: Handle P2P with SourceHub ACP #2848

feat: Handle P2P with SourceHub ACP #2848

Conversation

AndrewSisley commented Jul 18, 2024 • edited Loading

Relevant issue(s)

Description

codecov bot commented Jul 18, 2024 • edited Loading

Codecov Report

islamaliev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Jul 18, 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

nasdf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

shahzadlone 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

AndrewSisley Jul 19, 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

AndrewSisley commented Jul 18, 2024 •

edited

Loading

codecov bot commented Jul 18, 2024 •

edited

Loading

AndrewSisley Jul 18, 2024 •

edited

Loading

AndrewSisley Jul 19, 2024 •

edited

Loading

AndrewSisley Jul 19, 2024 •

edited

Loading