feat: Purge ACP state on DB purge #3363

jsimnz · 2025-01-08T15:51:30Z

Relevant issue(s)

Resolves #3358

Description

Crude implementation to purge the ACP state for LocalACP implementation (SourceHub ACP returns error at the moment). Basically it deletes the entire underlying KVStore and reinitializes it.

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?

Manually, also added unit tests. No current integration purge tests (afaik)

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

(modify the list accordingly)
Arch Linux
Debian Linux
MacOS
Windows

shahzadlone · 2025-01-08T15:58:11Z

acp/acp.go

+	// DropAll purges the entire ACP state.
+	DropAll(ctx context.Context) error


suggestion: As mentioned on discord :P prefer ResetState over DropAll

Suggested change

// DropAll purges the entire ACP state.

DropAll(ctx context.Context) error

// ResetState resets the entire ACP state.

ResetState(ctx context.Context) error

Would Reset work, or do you prefer the full ResetState?

Reset is fine too

shahzadlone · 2025-01-08T16:03:53Z

acp/acp_local.go

@@ -105,6 +107,40 @@ func (l *ACPLocal) Close() error {
 	return l.manager.Terminate()
 }

+func (l *ACPLocal) DropAll(ctx context.Context) error {
+	err := l.Close()


question: l.manager.Terminate() is safe to call close on even if it was already closed before?

Yea there might be an issue here, thanks for flagging

Resolved this, can you do another quick review on the last 2 commits. Added a flag to track if we're already closed so we don't duplicate the termination stuff. @shahzadlone

Re-reviewed. Just left one suggestion

acp/acp_local.go

shahzadlone · 2025-01-08T16:12:10Z

acp/acp_local_test.go

+func Test_LocalACP_InMemory_AddPolicy_CreatingSamePolicyAfterDropAllReturnsSameID(t *testing.T) {
+	ctx := context.Background()
+	localACP := NewLocalACP()
+
+	localACP.Init(ctx, "")
+	errStart := localACP.Start(ctx)
+	require.Nil(t, errStart)
+
+	policyID, errAddPolicy := localACP.AddPolicy(
+		ctx,
+		identity1,
+		validPolicy,
+	)
+	require.Nil(t, errAddPolicy)
+
+	require.Equal(
+		t,
+		validPolicyID,
+		policyID,
+	)
+
+	errDrop := localACP.DropAll(ctx)
+	require.Nil(t, errDrop)
+
+	// Since nothing is persisted should allow adding same policy again with same ID
+
+	policyID, errAddPolicy = localACP.AddPolicy(
+		ctx,
+		identity1,
+		validPolicy,
+	)
+	require.Nil(t, errAddPolicy)
+	require.Equal(
+		t,
+		validPolicyID,
+		policyID,
+	)
+
+	errClose := localACP.Close()
+	require.Nil(t, errClose)
+}
+
+func Test_LocalACP_Persistent_AddPolicy_CreatingSamePolicyAfterDropAllReturnsSameID(t *testing.T) {
+	ctx := context.Background()
+	localACP := NewLocalACP()
+
+	acpPath := t.TempDir()
+	localACP.Init(ctx, acpPath)
+	errStart := localACP.Start(ctx)
+	require.Nil(t, errStart)
+
+	policyID, errAddPolicy := localACP.AddPolicy(
+		ctx,
+		identity1,
+		validPolicy,
+	)
+	require.Nil(t, errAddPolicy)
+
+	require.Equal(
+		t,
+		validPolicyID,
+		policyID,
+	)
+
+	errDrop := localACP.DropAll(ctx)
+	require.Nil(t, errDrop)
+
+	// Since nothing is persisted should allow adding same policy again with same ID
+
+	policyID, errAddPolicy = localACP.AddPolicy(
+		ctx,
+		identity1,
+		validPolicy,
+	)
+	require.Nil(t, errAddPolicy)
+	require.Equal(
+		t,
+		validPolicyID,
+		policyID,
+	)
+
+	errClose := localACP.Close()
+	require.Nil(t, errClose)
+}


praise: Thanks for adding these!

fredcarle

LGTM.

node/node.go

shahzadlone · 2025-01-08T16:29:26Z

acp/acp_local_test.go

+		policyID,
+	)
+
+	errDrop := localACP.DropAll(ctx)


nitpick: Might be interesting to also test if calling this when acp is already closed (not started) is safe or errors/panics.

Thinking this through, should we only allow dropping if it isn't closed yet?

Since we will soon have option to turn acp off (which can be done only by authorized admin), as long as we also plan to (which I believe we do) gate the purge command also with authorized admin (AAC will use it's own separate local acp logic not mixed with this document access control logic), so it makes sense that the purge resets even if it's closed.

However happy to discuss further. We could also have a --force flag with purge that does the behavior of purging on close and otherwise a "safer" one that will error if acp is off (or is closed for whatever reason).

Let's not forget that the purge command is a dev only feature so protecting it with Admin ACP doesn't really make sense.

Let's not forget that the purge command is a dev only feature so protecting it with Admin ACP doesn't really make sense.

Kind of forgot about that. Do we still want dev only features when aac is implemented?

I think so yes. I would never want purge to be accessible by anyone on a production DB.

Thinking this through, should we only allow dropping if it isn't closed yet?

Following up here. Decided we do want to make sure we're only "resetting" state after the ACP instance is closed, at the very least because it simplifies things, and produces less edge cases to cover compared to if we were resetting the state while the ACP engine is in use.

shahzadlone

While I still don't fancy the acp var on the node :P it is a minimal change.
Other nitpick highlighted is the renaming to Reset everywhere that "drop all" is used.

Rest all good :) cheers for sorting quickly

jsimnz · 2025-01-08T20:43:59Z

@shahzadlone

While I still don't fancy the acp var on the node :P it is a minimal change.

Agreed, in the spirit of the velocity experiment, Im going to power through and mark this as a follow up issue, that we can get some more feedback on how we might want to make this change more permanent, but for now its a minor change that can easily be fixed.

acp/acp_local.go

acp/acp.go

node/node.go

shahzadlone · 2025-01-08T20:56:45Z

acp/acp_local_test.go

+func Test_LocalACP_InMemory_ResetStateAfterClose(t *testing.T) {
+	ctx := context.Background()
+	localACP := NewLocalACP()
+
+	localACP.Init(ctx, "")
+	errStart := localACP.Start(ctx)
+	require.Nil(t, errStart)
+
+	errClose := localACP.Close()
+	require.NoError(t, errClose)
+
+	errReset := localACP.ResetState(ctx)
+	require.NoError(t, errReset)
+}
+
+func Test_LocalACP_Persistent_ResetStateAfterClose(t *testing.T) {
+	acpPath := t.TempDir()
+	require.NotEqual(t, "", acpPath)
+
+	ctx := context.Background()
+	localACP := NewLocalACP()
+
+	localACP.Init(ctx, acpPath)
+	errStart := localACP.Start(ctx)
+	require.Nil(t, errStart)
+
+	errClose := localACP.Close()
+	require.NoError(t, errClose)
+
+	errReset := localACP.ResetState(ctx)
+	require.NoError(t, errReset)
+}
+


prase: thanks

acp/acp_local_test.go

node/node.go

codecov · 2025-01-08T21:46:03Z

Codecov Report

Attention: Patch coverage is 44.23077% with 29 lines in your changes missing coverage. Please review.

Project coverage is 78.30%. Comparing base (d0f5a54) to head (2abd202).
Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
acp/acp_local.go	42.42%	14 Missing and 5 partials ⚠️
node/node.go	46.67%	6 Missing and 2 partials ⚠️
acp/acp_source_hub.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3363      +/-   ##
===========================================
- Coverage    78.51%   78.30%   -0.21%     
===========================================
  Files          392      392              
  Lines        35640    35688      +48     
===========================================
- Hits         27982    27943      -39     
- Misses        6033     6095      +62     
- Partials      1625     1650      +25

Flag	Coverage Δ
all-tests	`78.30% <44.23%> (-0.21%)`	⬇️

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

Files with missing lines	Coverage Δ
acp/errors.go	`90.15% <ø> (ø)`
acp/source_hub_client.go	`93.99% <100.00%> (+0.04%)`	⬆️
acp/acp_source_hub.go	`72.91% <0.00%> (-0.59%)`	⬇️
node/node.go	`55.32% <46.67%> (-2.05%)`	⬇️
acp/acp_local.go	`90.30% <42.42%> (-7.75%)`	⬇️

... and 15 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 d0f5a54...2abd202. Read the comment docs.

shahzadlone · 2025-02-21T17:59:48Z

Bug bash: works

jsimnz added 3 commits January 8, 2025 07:30

Implememted ACP DropAll method

d670507

Added new acp.DropAll method to node.PurgeAndRestart

b94621c

Changed error handling to logging for acp purge

93a639c

jsimnz added feature New feature or request priority/high area/acp Related to the acp (access control) system labels Jan 8, 2025

jsimnz added this to the DefraDB v0.16 milestone Jan 8, 2025

fixed linter errors

c781935

jsimnz force-pushed the jsimnz/feat/acp-purge branch from f56117e to c781935 Compare January 8, 2025 15:59