End to end support for experimental first-class relationship expiration feature #2152

josephschorr · 2024-12-03T19:13:41Z

Fixes #1262 by superseding it

When the flag is enabled, relationship expiration can be used in schema via the use expiration directive and the expiration keyword on relation's:

use expiration

definition user {}

definition resource {
  relation viewer: user with expiration
}

When a relationship reaches its expiration, it is immediately filtered from all relationship queries, making it appear as-if it was immediately deleted.

Subsequently (typically 24 hours later), the relationship will be actually removed from the underlying datastore to free space.

Warning

Currently, relationships are filtered using the wall clock found within the datastore, which has the following consequences:

For multi-master datastores such as CRDB and Spanner, there may be some slight skew between nodes on clocks, which could lead to different results if queries are called very close to the expiration time for relationship(s) to be returned
Caches are not updated if a relationship expires within the caching window, e.g. if a permission answer is computed at time N, a relationship used as part of that computation expires at time N+1 and the cache entry is valid until N+10, the caller will see the cached answer even if the relationship has expired - this is in line with caches reflecting the state of the world when they were computed, but is a change from current behavior when using caveats for expiration
at_exact_snapshot calls, as they are made to the datastore, will reflect the expiration as of "now", which means they can return different results depending on when they are invoked. This is a change from current behavior when using caveats for expiration. Note that this may be changed in the future to instead use the timestamp of the revision itself; this change is currently under discussion

Note

This PR does not yet implement returning of the expires_at in CheckPermissionResponse and CheckPermission debug information. That will be done in a followup PR.

tstirrat15

See comments, otherwise LGTM

tstirrat15 · 2024-12-03T19:16:25Z

internal/datastore/common/gc.go

 	// even if an error happened, garbage would have been collected. This makes sure these are reflected even if the
 	// worker eventually fails or times out.
 	gcRelationshipsCounter.Add(float64(collected.Relationships))
 	gcTransactionsCounter.Add(float64(collected.Transactions))
 	gcNamespacesCounter.Add(float64(collected.Namespaces))
+	gcExpiredRelationshipsCounter.Add(float64(expiredRelationshipsCount))


Is this being a float just because that's how Prometheus implements counters?

tstirrat15 · 2024-12-03T20:20:53Z

internal/datastore/crdb/crdb.go

-				Columns(colExpiresAt, colMetadata).
-				Values(expiresAt, config.Metadata.AsMap())
+		// Write metadata onto the transaction.
+		metadata := config.Metadata.AsMap()


Does the creation here also attach it to the transaction? Otherwise it looks like it's being constructed and modified but not attached.

Line 362 executes the insert statement for it; I fixed the reference

tstirrat15 · 2024-12-03T20:27:40Z

internal/datastore/crdb/watch.go

@@ -549,7 +590,9 @@ func (cds *crdbDatastore) processChanges(ctx context.Context, changes pgx.Rows,
 					return
 				}

-				if err := tracked.SetRevisionMetadata(ctx, rev, details.After.Metadata); err != nil {
+				adjustedMetadata := details.After.Metadata


For my own edification - when you set something equal to a struct rather than a pointer to that struct, is it functionally creating a copy of it, which is what lets us safely delete the key?

In this case, its safe because we're not reusing it anywhere else

tstirrat15 · 2024-12-03T20:55:32Z

internal/services/v1/relationships.go

@@ -115,6 +118,7 @@ func NewPermissionsServer(
 		MaxLookupResourcesLimit:         defaultIfZero(config.MaxLookupResourcesLimit, 1_000),
 		MaxBulkExportRelationshipsLimit: defaultIfZero(config.MaxBulkExportRelationshipsLimit, 100_000),
 		DispatchChunkSize:               defaultIfZero(config.DispatchChunkSize, 100),
+		ExpiringRelationshipsEnabled:    true,


Meaning it defaults to enabled? Or is this something that should look like the above?

Yes, for testing. We then explicitly disable it when starting SpiceDB, as its default is false

tstirrat15 · 2024-12-03T20:58:44Z

internal/services/v1/relationships_test.go

@@ -480,6 +481,30 @@ func TestDeleteRelationshipViaWriteNoop(t *testing.T) {
 	require.NoError(err)
 }

+func TestWriteExpiringRelationshinps(t *testing.T) {


Suggested change

func TestWriteExpiringRelationshinps(t *testing.T) {

func TestWriteExpiringRelationships(t *testing.T) {

tstirrat15 · 2024-12-03T21:06:04Z

pkg/cmd/testserver/testserver.go

@@ -84,6 +84,7 @@ func (c *Config) Complete() (RunnableTestServer, error) {
 				MaxLookupResourcesLimit:         c.MaxLookupResourcesLimit,
 				MaxBulkExportRelationshipsLimit: c.MaxBulkExportRelationshipsLimit,
 				DispatchChunkSize:               defaultMaxChunkSize,
+				ExpiringRelationshipsEnabled:    true,


Same here - should this reference the config?

No; I think its fine to enable by default in the test server

tstirrat15 · 2024-12-03T21:08:12Z

pkg/development/devcontext.go

+		MaxPreconditionsCount:        50,
+		MaximumAPIDepth:              50,
+		MaxCaveatContextSize:         0,
+		ExpiringRelationshipsEnabled: true,


Here as well

…ration

tstirrat15

LGTM

josephschorr requested review from vroldanbet and a team as code owners December 3, 2024 19:13

github-actions bot added area/CLI Affects the command line area/schema Affects the Schema Language area/api v1 Affects the v1 API area/datastore Affects the storage system area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) labels Dec 3, 2024

This was referenced Dec 3, 2024

GC of relationships expired via caveats #1262

Open

Relationships with expiry / TTL #282

Closed

tstirrat15 previously approved these changes Dec 3, 2024

View reviewed changes

josephschorr dismissed tstirrat15’s stale review via 122c597 December 3, 2024 21:35

josephschorr added 19 commits December 3, 2024 16:47

Add missing translation in struct for rel expiration

4f8b7a4

Add support for rel expiration in memdb and add basic datastore tests

6d335fc

Flip flag on the schema compiler to allow use expiration by default

ad867ec

Add support for expiration on rels in validation

e3be93a

Get relationship expiration working in consistency tests end-to-end

f8b2ff1

Add write rels test for expiring relationship

6113aa5

Add support in schema change checking for expiration

ad047b2

Further improvement to caveat validation errors for relationship expi…

06a7364

…ration

Enable expiration on bulk import of relationships

ad3edfe

Add support in Postgres driver for expiration

f44213c

Add support in MySQL driver for expiration

2206b77

Add support in CRDB driver for expiration

96a7644

Add support in Spanner driver for expiration

18c8df5

Add watch test for relationship with expiration

6eb757e

Add bulk upload with expiration test

d813cf9

Add watch filtering for TTL deletes in Spanner

90a07a3

Add filtering for TTL deletes in CRDB

e881965

Add expired relationship GC to PG and MySQL drivers

a70bf7f

Add experimental flag for enabling rel expiration

04abed2

josephschorr added 5 commits December 3, 2024 16:47

Ensure the canonical representation of expiring rels always uses UTC

9fa2621

Add indexes for relationship expiration GC

5700100

Truncate times to seconds when generating hashes

f009fd2

Fix metadata map reference in CRDB

34850c4

Fix typo

2de262c

tstirrat15 previously approved these changes Dec 3, 2024

View reviewed changes

josephschorr force-pushed the rel-expiration-e2e branch from 4f70c9b to 2de262c Compare December 3, 2024 22:16

Add expiration into CRDB watch

cccd06e

josephschorr dismissed tstirrat15’s stale review via 677802a December 3, 2024 23:01

josephschorr force-pushed the rel-expiration-e2e branch from 677802a to cccd06e Compare December 4, 2024 03:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

End to end support for experimental first-class relationship expiration feature #2152

End to end support for experimental first-class relationship expiration feature #2152

josephschorr commented Dec 3, 2024

tstirrat15 left a comment

tstirrat15 Dec 3, 2024

josephschorr Dec 3, 2024

tstirrat15 Dec 3, 2024

josephschorr Dec 3, 2024

tstirrat15 Dec 3, 2024

josephschorr Dec 3, 2024

tstirrat15 Dec 3, 2024

josephschorr Dec 3, 2024

tstirrat15 Dec 3, 2024

tstirrat15 Dec 3, 2024

josephschorr Dec 3, 2024

tstirrat15 Dec 3, 2024

tstirrat15 left a comment

	func TestWriteExpiringRelationshinps(t *testing.T) {
	func TestWriteExpiringRelationships(t *testing.T) {

End to end support for experimental first-class relationship expiration feature #2152

Are you sure you want to change the base?

End to end support for experimental first-class relationship expiration feature #2152

Conversation

josephschorr commented Dec 3, 2024

tstirrat15 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

tstirrat15 left a comment

Choose a reason for hiding this comment