From d74756a0fb211d6ee96c0ce0f9717e3e9af6b0e1 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Wed, 27 Nov 2024 13:14:02 -0500 Subject: [PATCH 01/39] Add missing translation in struct for rel expiration --- pkg/tuple/structs.go | 16 +++-- pkg/tuple/structs_test.go | 128 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 140 insertions(+), 4 deletions(-) diff --git a/pkg/tuple/structs.go b/pkg/tuple/structs.go index 208e0ddf63..edb8041c30 100644 --- a/pkg/tuple/structs.go +++ b/pkg/tuple/structs.go @@ -5,6 +5,8 @@ import ( "fmt" "time" + "google.golang.org/protobuf/types/known/timestamppb" + core "github.com/authzed/spicedb/pkg/proto/core/v1" "github.com/authzed/spicedb/pkg/spiceerrors" ) @@ -77,11 +79,17 @@ type Relationship struct { // ToCoreTuple converts the Relationship to a core.RelationTuple. func (r Relationship) ToCoreTuple() *core.RelationTuple { + var expirationTime *timestamppb.Timestamp + if r.OptionalExpiration != nil { + expirationTime = timestamppb.New(*r.OptionalExpiration) + } + return &core.RelationTuple{ - ResourceAndRelation: r.Resource.ToCoreONR(), - Subject: r.Subject.ToCoreONR(), - Caveat: r.OptionalCaveat, - Integrity: r.OptionalIntegrity, + ResourceAndRelation: r.Resource.ToCoreONR(), + Subject: r.Subject.ToCoreONR(), + Caveat: r.OptionalCaveat, + Integrity: r.OptionalIntegrity, + OptionalExpirationTime: expirationTime, } } diff --git a/pkg/tuple/structs_test.go b/pkg/tuple/structs_test.go index cf3be66a44..cb884becb5 100644 --- a/pkg/tuple/structs_test.go +++ b/pkg/tuple/structs_test.go @@ -2,9 +2,15 @@ package tuple import ( "testing" + "time" "unsafe" "github.com/stretchr/testify/require" + "google.golang.org/protobuf/types/known/structpb" + "google.golang.org/protobuf/types/known/timestamppb" + + core "github.com/authzed/spicedb/pkg/proto/core/v1" + "github.com/authzed/spicedb/pkg/testutil" ) func TestONRStructSize(t *testing.T) { @@ -16,3 +22,125 @@ func TestRelationshipStructSize(t *testing.T) { size := int(unsafe.Sizeof(Relationship{})) require.Equal(t, relStructSize, size) } + +func TestToCoreTuple(t *testing.T) { + tcs := []struct { + name string + rel Relationship + exp *core.RelationTuple + }{ + { + name: "simple", + rel: MustParse("type:object#relation@subjecttype:subject"), + exp: &core.RelationTuple{ + ResourceAndRelation: &core.ObjectAndRelation{ + Namespace: "type", + ObjectId: "object", + Relation: "relation", + }, + Subject: &core.ObjectAndRelation{ + Namespace: "subjecttype", + ObjectId: "subject", + Relation: Ellipsis, + }, + }, + }, + { + name: "public wildcard", + rel: MustParse("type:object#relation@subjecttype:*"), + exp: &core.RelationTuple{ + ResourceAndRelation: &core.ObjectAndRelation{ + Namespace: "type", + ObjectId: "object", + Relation: "relation", + }, + Subject: &core.ObjectAndRelation{ + Namespace: "subjecttype", + ObjectId: PublicWildcard, + Relation: Ellipsis, + }, + }, + }, + { + name: "subject relation", + rel: MustParse("type:object#relation@subjecttype:subject#subjectrelation"), + exp: &core.RelationTuple{ + ResourceAndRelation: &core.ObjectAndRelation{ + Namespace: "type", + ObjectId: "object", + Relation: "relation", + }, + Subject: &core.ObjectAndRelation{ + Namespace: "subjecttype", + ObjectId: "subject", + Relation: "subjectrelation", + }, + }, + }, + { + name: "caveated with no context", + rel: MustParse("type:object#relation@subjecttype:subject[somecaveat]"), + exp: &core.RelationTuple{ + ResourceAndRelation: &core.ObjectAndRelation{ + Namespace: "type", + ObjectId: "object", + Relation: "relation", + }, + Subject: &core.ObjectAndRelation{ + Namespace: "subjecttype", + ObjectId: "subject", + Relation: Ellipsis, + }, + Caveat: &core.ContextualizedCaveat{ + CaveatName: "somecaveat", + }, + }, + }, + { + name: "caveated with context", + rel: MustParse("type:object#relation@subjecttype:subject[somecaveat:{\"foo\": 42}]"), + exp: &core.RelationTuple{ + ResourceAndRelation: &core.ObjectAndRelation{ + Namespace: "type", + ObjectId: "object", + Relation: "relation", + }, + Subject: &core.ObjectAndRelation{ + Namespace: "subjecttype", + ObjectId: "subject", + Relation: Ellipsis, + }, + Caveat: &core.ContextualizedCaveat{ + CaveatName: "somecaveat", + Context: (func() *structpb.Struct { + s, _ := structpb.NewStruct(map[string]any{"foo": 42}) + return s + })(), + }, + }, + }, + { + name: "with expiration", + rel: MustParse("type:object#relation@subjecttype:subject[expiration:2021-01-01T00:00:00Z]"), + exp: &core.RelationTuple{ + ResourceAndRelation: &core.ObjectAndRelation{ + Namespace: "type", + ObjectId: "object", + Relation: "relation", + }, + Subject: &core.ObjectAndRelation{ + Namespace: "subjecttype", + ObjectId: "subject", + Relation: Ellipsis, + }, + OptionalExpirationTime: timestamppb.New(time.Date(2021, 1, 1, 0, 0, 0, 0, time.UTC)), + }, + }, + } + + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + testutil.RequireProtoEqual(t, tc.exp, tc.rel.ToCoreTuple(), "mismatch") + }) + } +} From 5966b555c5602e93055ce0f980805c440d73c513 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Wed, 27 Nov 2024 13:15:07 -0500 Subject: [PATCH 02/39] Add support for rel expiration in memdb and add basic datastore tests --- internal/datastore/memdb/readonly.go | 18 ++++ internal/datastore/memdb/readwrite.go | 1 + internal/datastore/memdb/schema.go | 6 +- internal/testfixtures/datastore.go | 4 + pkg/datastore/test/datastore.go | 1 + pkg/datastore/test/relationships.go | 116 ++++++++++++++++++++++++++ 6 files changed, 144 insertions(+), 2 deletions(-) diff --git a/internal/datastore/memdb/readonly.go b/internal/datastore/memdb/readonly.go index e3734d3184..427ea0ea6a 100644 --- a/internal/datastore/memdb/readonly.go +++ b/internal/datastore/memdb/readonly.go @@ -6,6 +6,7 @@ import ( "slices" "sort" "strings" + "time" "github.com/hashicorp/go-memdb" @@ -471,12 +472,17 @@ func newSubjectSortedIterator(it memdb.ResultIterator, limit *uint64) (datastore results := make([]tuple.Relationship, 0) // Coalesce all of the results into memory + now := time.Now() for foundRaw := it.Next(); foundRaw != nil; foundRaw = it.Next() { rt, err := foundRaw.(*relationship).Relationship() if err != nil { return nil, err } + if rt.OptionalExpiration != nil && rt.OptionalExpiration.Before(now) { + continue + } + results = append(results, rt) } @@ -516,6 +522,7 @@ func eq(lhsNamespace, lhsObjectID, lhsRelation string, rhs tuple.ObjectAndRelati func newMemdbTupleIterator(it memdb.ResultIterator, limit *uint64) datastore.RelationshipIterator { var count uint64 return func(yield func(tuple.Relationship, error) bool) { + now := time.Now() for { foundRaw := it.Next() if foundRaw == nil { @@ -527,6 +534,17 @@ func newMemdbTupleIterator(it memdb.ResultIterator, limit *uint64) datastore.Rel } rt, err := foundRaw.(*relationship).Relationship() + if err != nil { + if !yield(tuple.Relationship{}, err) { + return + } + continue + } + + if rt.OptionalExpiration != nil && rt.OptionalExpiration.Before(now) { + continue + } + if !yield(rt, err) { return } diff --git a/internal/datastore/memdb/readwrite.go b/internal/datastore/memdb/readwrite.go index 2f97c92766..885a406553 100644 --- a/internal/datastore/memdb/readwrite.go +++ b/internal/datastore/memdb/readwrite.go @@ -59,6 +59,7 @@ func (rwt *memdbReadWriteTx) write(tx *memdb.Txn, mutations ...tuple.Relationshi mutation.Relationship.Subject.Relation, rwt.toCaveatReference(mutation), rwt.toIntegrity(mutation), + mutation.Relationship.OptionalExpiration, } found, err := tx.First( diff --git a/internal/datastore/memdb/schema.go b/internal/datastore/memdb/schema.go index 2caf054a0a..9328137228 100644 --- a/internal/datastore/memdb/schema.go +++ b/internal/datastore/memdb/schema.go @@ -56,6 +56,7 @@ type relationship struct { subjectRelation string caveat *contextualizedCaveat integrity *relationshipIntegrity + expiration *time.Time } type relationshipIntegrity struct { @@ -149,8 +150,9 @@ func (r relationship) Relationship() (tuple.Relationship, error) { Relation: r.subjectRelation, }, }, - OptionalCaveat: cr, - OptionalIntegrity: ig, + OptionalCaveat: cr, + OptionalIntegrity: ig, + OptionalExpiration: r.expiration, }, nil } diff --git a/internal/testfixtures/datastore.go b/internal/testfixtures/datastore.go index 230bae950f..53d0b2e8d4 100644 --- a/internal/testfixtures/datastore.go +++ b/internal/testfixtures/datastore.go @@ -54,6 +54,10 @@ var DocumentNS = ns.Namespace( nil, ns.AllowedRelationWithCaveat("user", "...", ns.AllowedCaveat("test")), ), + ns.MustRelation("expiring_viewer", + nil, + ns.AllowedRelationWithExpiration("user", "..."), + ), ns.MustRelation("parent", nil, ns.AllowedRelation("folder", "...")), ns.MustRelation("edit", ns.Union( diff --git a/pkg/datastore/test/datastore.go b/pkg/datastore/test/datastore.go index 83e3c5bb1d..741b4b8bd7 100644 --- a/pkg/datastore/test/datastore.go +++ b/pkg/datastore/test/datastore.go @@ -134,6 +134,7 @@ func AllWithExceptions(t *testing.T, tester DatastoreTester, except Categories, t.Run("TestDeleteRelationshipsWithVariousFilters", runner(tester, DeleteRelationshipsWithVariousFiltersTest)) t.Run("TestTouchTypedAlreadyExistingWithoutCaveat", runner(tester, TypedTouchAlreadyExistingTest)) t.Run("TestTouchTypedAlreadyExistingWithCaveat", runner(tester, TypedTouchAlreadyExistingWithCaveatTest)) + t.Run("TestRelationshipExpiration", runner(tester, RelationshipExpirationTest)) t.Run("TestMultipleReadsInRWT", runner(tester, MultipleReadsInRWTTest)) t.Run("TestConcurrentWriteSerialization", runner(tester, ConcurrentWriteSerializationTest)) diff --git a/pkg/datastore/test/relationships.go b/pkg/datastore/test/relationships.go index 1d7dd31557..3109a30f9b 100644 --- a/pkg/datastore/test/relationships.go +++ b/pkg/datastore/test/relationships.go @@ -1352,6 +1352,21 @@ func QueryRelationshipsWithVariousFiltersTest(t *testing.T, tester DatastoreTest "folder:someotherfolder#viewer@user:tom", }, }, + { + name: "relationship expiration", + filter: datastore.RelationshipsFilter{ + OptionalResourceType: "document", + }, + relationships: []string{ + "document:first#viewer@user:tom", + "document:first#expiring_viewer@user:fred[expiration:2021-01-01T00:00:00Z]", + "document:first#expiring_viewer@user:sarah[expiration:2900-01-02T01:02:03Z]", + }, + expected: []string{ + "document:first#viewer@user:tom", + "document:first#expiring_viewer@user:sarah[expiration:2900-01-02T01:02:03Z]", + }, + }, } for _, tc := range tcs { @@ -1411,6 +1426,61 @@ func TypedTouchAlreadyExistingTest(t *testing.T, tester DatastoreTester) { ensureRelationships(ctx, require, ds, tpl1) } +// RelationshipExpirationTest tests expiration on relationships. +func RelationshipExpirationTest(t *testing.T, tester DatastoreTester) { + require := require.New(t) + + rawDS, err := tester.New(0, veryLargeGCInterval, veryLargeGCWindow, 1) + require.NoError(err) + + ds, _ := testfixtures.StandardDatastoreWithData(rawDS, require) + ctx := context.Background() + + // Create a relationship that expires in the future. + rel1, err := tuple.Parse("document:foo#expiring_viewer@user:tom[expiration:2900-01-01T00:00:00Z]") + require.NoError(err) + + _, err = common.WriteRelationships(ctx, ds, tuple.UpdateOperationCreate, rel1) + require.NoError(err) + ensureRelationships(ctx, require, ds, rel1) + ensureReverseRelationships(ctx, require, ds, rel1) + + // Touch the relationship to a different expiration. + rel2, err := tuple.Parse("document:foo#expiring_viewer@user:tom[expiration:2800-01-01T00:00:00Z]") + require.NoError(err) + + _, err = common.WriteRelationships(ctx, ds, tuple.UpdateOperationTouch, rel2) + require.NoError(err) + ensureRelationships(ctx, require, ds, rel2) + ensureReverseRelationships(ctx, require, ds, rel2) + + // Touch the relationship to a time in the past. + rel3, err := tuple.Parse("document:foo#expiring_viewer@user:tom[expiration:2000-01-01T00:00:00Z]") + require.NoError(err) + + _, err = common.WriteRelationships(ctx, ds, tuple.UpdateOperationTouch, rel3) + require.NoError(err) + ensureNotRelationships(ctx, require, ds, rel3) + ensureNotReverseRelationships(ctx, require, ds, rel3) + + // Try to recreate the relationship, which should fail even though the expiration has past. + relrecreate, err := tuple.Parse("document:foo#expiring_viewer@user:tom[expiration:3000-01-01T00:00:00Z]") + require.NoError(err) + + _, err = common.WriteRelationships(ctx, ds, tuple.UpdateOperationCreate, relrecreate) + require.Error(err) + require.ErrorContains(err, "already existed") + + // Touch the relationship back to the future. + rel4, err := tuple.Parse("document:foo#expiring_viewer@user:tom[expiration:3000-01-01T00:00:00Z]") + require.NoError(err) + + _, err = common.WriteRelationships(ctx, ds, tuple.UpdateOperationTouch, rel4) + require.NoError(err) + ensureRelationships(ctx, require, ds, rel4) + ensureReverseRelationships(ctx, require, ds, rel4) +} + // TypedTouchAlreadyExistingWithCaveatTest tests touching a relationship twice, when valid type information is provided. func TypedTouchAlreadyExistingWithCaveatTest(t *testing.T, tester DatastoreTester) { require := require.New(t) @@ -1658,6 +1728,52 @@ func onrToSubjectsFilter(onr tuple.ObjectAndRelation) datastore.SubjectsFilter { } } +func ensureReverseRelationships(ctx context.Context, require *require.Assertions, ds datastore.Datastore, rels ...tuple.Relationship) { + ensureReverseRelationshipsStatus(ctx, require, ds, rels, true) +} + +func ensureNotReverseRelationships(ctx context.Context, require *require.Assertions, ds datastore.Datastore, rels ...tuple.Relationship) { + ensureReverseRelationshipsStatus(ctx, require, ds, rels, false) +} + +func ensureReverseRelationshipsStatus(ctx context.Context, require *require.Assertions, ds datastore.Datastore, rels []tuple.Relationship, mustExist bool) { + headRev, err := ds.HeadRevision(ctx) + require.NoError(err) + + reader := ds.SnapshotReader(headRev) + + for _, rel := range rels { + filter := datastore.SubjectRelationFilter{ + NonEllipsisRelation: rel.Subject.Relation, + } + if rel.Subject.Relation == tuple.Ellipsis { + filter.IncludeEllipsisRelation = true + filter.NonEllipsisRelation = "" + } + + iter, err := reader.ReverseQueryRelationships(ctx, datastore.SubjectsFilter{ + SubjectType: rel.Subject.ObjectType, + OptionalSubjectIds: []string{rel.Subject.ObjectID}, + RelationFilter: filter, + }) + require.NoError(err) + + found, err := datastore.IteratorToSlice(iter) + require.NoError(err) + + if mustExist { + require.NotEmpty(found, "expected relationship %s", tuple.MustString(rel)) + } else { + require.Empty(found, "expected relationship %s to not exist", tuple.MustString(rel)) + } + + if mustExist { + require.Equal(1, len(found)) + require.Equal(tuple.MustString(rel), tuple.MustString(found[0])) + } + } +} + func ensureRelationships(ctx context.Context, require *require.Assertions, ds datastore.Datastore, rels ...tuple.Relationship) { ensureRelationshipsStatus(ctx, require, ds, rels, true) } From 698a2021a0310b35b523feaea2338d0be5962c15 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Wed, 27 Nov 2024 13:21:07 -0500 Subject: [PATCH 03/39] Flip flag on the schema compiler to allow `use expiration` by default --- pkg/schemadsl/compiler/compiler.go | 17 ++++++++++++++--- pkg/schemadsl/compiler/compiler_test.go | 2 +- pkg/schemadsl/generator/generator_test.go | 2 +- pkg/typesystem/typesystem_test.go | 2 +- 4 files changed, 17 insertions(+), 6 deletions(-) diff --git a/pkg/schemadsl/compiler/compiler.go b/pkg/schemadsl/compiler/compiler.go index 6565b1196e..435f9e22fb 100644 --- a/pkg/schemadsl/compiler/compiler.go +++ b/pkg/schemadsl/compiler/compiler.go @@ -5,6 +5,7 @@ import ( "fmt" "google.golang.org/protobuf/proto" + "k8s.io/utils/strings/slices" core "github.com/authzed/spicedb/pkg/proto/core/v1" "github.com/authzed/spicedb/pkg/schemadsl/dslshape" @@ -69,8 +70,12 @@ func AllowUnprefixedObjectType() ObjectPrefixOption { return func(cfg *config) { cfg.objectTypePrefix = new(string) } } -func AllowExpirationUseFlag() Option { - return func(cfg *config) { cfg.allowedFlags = append(cfg.allowedFlags, "expiration") } +func DisallowExpirationFlag() ObjectPrefixOption { + return func(cfg *config) { + cfg.allowedFlags = slices.Filter([]string{}, cfg.allowedFlags, func(s string) bool { + return s != "expiration" + }) + } } type Option func(*config) @@ -79,7 +84,13 @@ type ObjectPrefixOption func(*config) // Compile compilers the input schema into a set of namespace definition protos. func Compile(schema InputSchema, prefix ObjectPrefixOption, opts ...Option) (*CompiledSchema, error) { - cfg := &config{} + cfg := &config{ + allowedFlags: make([]string, 0, 1), + } + + // Enable `expiration` flag by default. + cfg.allowedFlags = append(cfg.allowedFlags, "expiration") + prefix(cfg) // required option for _, fn := range opts { diff --git a/pkg/schemadsl/compiler/compiler_test.go b/pkg/schemadsl/compiler/compiler_test.go index 78fa8a579d..114b8ffef4 100644 --- a/pkg/schemadsl/compiler/compiler_test.go +++ b/pkg/schemadsl/compiler/compiler_test.go @@ -1032,7 +1032,7 @@ func TestCompile(t *testing.T) { require := require.New(t) compiled, err := Compile(InputSchema{ input.Source(test.name), test.input, - }, test.objectPrefix, AllowExpirationUseFlag()) + }, test.objectPrefix) if test.expectedError != "" { require.Error(err) diff --git a/pkg/schemadsl/generator/generator_test.go b/pkg/schemadsl/generator/generator_test.go index 82bf4fb78e..a9cbc89495 100644 --- a/pkg/schemadsl/generator/generator_test.go +++ b/pkg/schemadsl/generator/generator_test.go @@ -405,7 +405,7 @@ definition document { compiled, err := compiler.Compile(compiler.InputSchema{ Source: input.Source(test.name), SchemaString: test.input, - }, compiler.AllowUnprefixedObjectType(), compiler.AllowExpirationUseFlag()) + }, compiler.AllowUnprefixedObjectType()) require.NoError(err) source, _, err := GenerateSchema(compiled.OrderedDefinitions) diff --git a/pkg/typesystem/typesystem_test.go b/pkg/typesystem/typesystem_test.go index c85352438d..4b00ae13aa 100644 --- a/pkg/typesystem/typesystem_test.go +++ b/pkg/typesystem/typesystem_test.go @@ -945,7 +945,7 @@ func TestTypeSystemAccessors(t *testing.T) { compiled, err := compiler.Compile(compiler.InputSchema{ Source: input.Source("schema"), SchemaString: tc.schema, - }, compiler.AllowUnprefixedObjectType(), compiler.AllowExpirationUseFlag()) + }, compiler.AllowUnprefixedObjectType()) require.NoError(err) lastRevision, err := ds.HeadRevision(context.Background()) From 08d0e6551a48cc1aede3f2feabf4e9c61f1d3251 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Wed, 27 Nov 2024 13:21:14 -0500 Subject: [PATCH 04/39] Add support for expiration on rels in validation --- internal/relationships/validation.go | 4 ++++ internal/relationships/validation_test.go | 28 +++++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/internal/relationships/validation.go b/internal/relationships/validation.go index 069106f88b..3de0b3fab2 100644 --- a/internal/relationships/validation.go +++ b/internal/relationships/validation.go @@ -198,6 +198,10 @@ func ValidateOneRelationship( caveat) } + if rel.OptionalExpiration != nil { + relationToCheck = ns.WithExpiration(relationToCheck) + } + switch { case rule == ValidateRelationshipForCreateOrTouch || caveat != nil: // For writing or when the caveat was specified, the caveat must be a direct match. diff --git a/internal/relationships/validation_test.go b/internal/relationships/validation_test.go index f487fd006e..69da383047 100644 --- a/internal/relationships/validation_test.go +++ b/internal/relationships/validation_test.go @@ -239,6 +239,34 @@ func TestValidateRelationshipOperations(t *testing.T) { core.RelationTupleUpdate_CREATE, "subjects of type `user` are not allowed on relation `resource#viewer`; did you mean `user#member`?", }, + { + "expiration fail test", + ` + use expiration + + definition user {} + + definition resource { + relation viewer: user with expiration + }`, + "resource:foo#viewer@user:tom", + core.RelationTupleUpdate_CREATE, + "subjects of type `user` are not allowed on relation `resource#viewer`; did you mean `user with expiration`?", + }, + { + "expiration success test", + ` + use expiration + + definition user {} + + definition resource { + relation viewer: user with expiration + }`, + "resource:foo#viewer@user:tom[expiration:2021-01-01T00:00:00Z]", + core.RelationTupleUpdate_CREATE, + "", + }, } for _, tc := range tcs { From beb63697f63db12d1c68dce051c54310549d0bd4 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Wed, 27 Nov 2024 13:21:42 -0500 Subject: [PATCH 05/39] Get relationship expiration working in consistency tests end-to-end --- .../integrationtesting/consistency_test.go | 11 ++++-- .../consistencytestutil/servicetester.go | 20 +++++++---- ...{expiration.yaml => caveatexpiration.yaml} | 0 .../testconfigs/expirationwithcaveat.yaml | 33 ++++++++++++++++++ .../testconfigs/indirectrelexpiration.yaml | 34 +++++++++++++++++++ .../testconfigs/relexpiration.yaml | 20 +++++++++++ internal/services/v1/relationships.go | 8 ----- pkg/namespace/builder.go | 10 ++++++ 8 files changed, 119 insertions(+), 17 deletions(-) rename internal/services/integrationtesting/testconfigs/{expiration.yaml => caveatexpiration.yaml} (100%) create mode 100644 internal/services/integrationtesting/testconfigs/expirationwithcaveat.yaml create mode 100644 internal/services/integrationtesting/testconfigs/indirectrelexpiration.yaml create mode 100644 internal/services/integrationtesting/testconfigs/relexpiration.yaml diff --git a/internal/services/integrationtesting/consistency_test.go b/internal/services/integrationtesting/consistency_test.go index 81203f8d43..9692ea30b8 100644 --- a/internal/services/integrationtesting/consistency_test.go +++ b/internal/services/integrationtesting/consistency_test.go @@ -260,7 +260,11 @@ func validateRelationshipReads(t *testing.T, vctx validationContext) { foundRelationshipsSet.Insert(tuple.MustString(rel)) } - require.True(t, foundRelationshipsSet.Has(tuple.MustString(relationship)), "missing expected relationship %s in read results: %s", tuple.MustString(relationship), foundRelationshipsSet.AsSlice()) + if relationship.OptionalExpiration != nil && relationship.OptionalExpiration.Before(time.Now()) { + require.False(t, foundRelationshipsSet.Has(tuple.MustString(relationship)), "found unexpected expired relationship %s in read results: %s", tuple.MustString(relationship), foundRelationshipsSet.AsSlice()) + } else { + require.True(t, foundRelationshipsSet.Has(tuple.MustString(relationship)), "missing expected relationship %s in read results: %s", tuple.MustString(relationship), foundRelationshipsSet.AsSlice()) + } }) } @@ -767,8 +771,10 @@ func validateDevelopment(t *testing.T, vctx validationContext) { Relationships: rels, } - devContext, _, err := development.NewDevContext(context.Background(), reqContext) + devContext, devErr, err := development.NewDevContext(context.Background(), reqContext) require.NoError(t, err) + require.Nil(t, devErr, "dev error: %v", devErr) + require.NotNil(t, devContext) // Validate checks. validateDevelopmentChecks(t, devContext, vctx) @@ -788,6 +794,7 @@ func validateDevelopmentChecks(t *testing.T, devContext *development.DevContext, for _, subject := range vctx.accessibilitySet.AllSubjectsNoWildcards() { subject := subject t.Run(tuple.StringONR(subject), func(t *testing.T) { + require.NotNil(t, devContext) cr, err := development.RunCheck(devContext, resource, subject, nil) require.NoError(t, err, "Got unexpected error from development check") diff --git a/internal/services/integrationtesting/consistencytestutil/servicetester.go b/internal/services/integrationtesting/consistencytestutil/servicetester.go index f9b31debd3..f4783ae8ea 100644 --- a/internal/services/integrationtesting/consistencytestutil/servicetester.go +++ b/internal/services/integrationtesting/consistencytestutil/servicetester.go @@ -109,14 +109,20 @@ func (v1st v1ServiceTester) Expand(ctx context.Context, resource tuple.ObjectAnd } func (v1st v1ServiceTester) Write(ctx context.Context, relationship tuple.Relationship) error { - _, err := v1st.permClient.WriteRelationships(ctx, &v1.WriteRelationshipsRequest{ - OptionalPreconditions: []*v1.Precondition{ - { - Operation: v1.Precondition_OPERATION_MUST_MATCH, - Filter: tuple.ToV1Filter(relationship), - }, + preconditions := []*v1.Precondition{ + { + Operation: v1.Precondition_OPERATION_MUST_MATCH, + Filter: tuple.ToV1Filter(relationship), }, - Updates: []*v1.RelationshipUpdate{tuple.MustUpdateToV1RelationshipUpdate(tuple.Touch(relationship))}, + } + + if relationship.OptionalExpiration != nil { + preconditions = nil + } + + _, err := v1st.permClient.WriteRelationships(ctx, &v1.WriteRelationshipsRequest{ + OptionalPreconditions: preconditions, + Updates: []*v1.RelationshipUpdate{tuple.MustUpdateToV1RelationshipUpdate(tuple.Touch(relationship))}, }) return err } diff --git a/internal/services/integrationtesting/testconfigs/expiration.yaml b/internal/services/integrationtesting/testconfigs/caveatexpiration.yaml similarity index 100% rename from internal/services/integrationtesting/testconfigs/expiration.yaml rename to internal/services/integrationtesting/testconfigs/caveatexpiration.yaml diff --git a/internal/services/integrationtesting/testconfigs/expirationwithcaveat.yaml b/internal/services/integrationtesting/testconfigs/expirationwithcaveat.yaml new file mode 100644 index 0000000000..ae707060a5 --- /dev/null +++ b/internal/services/integrationtesting/testconfigs/expirationwithcaveat.yaml @@ -0,0 +1,33 @@ +--- +schema: |+ + use expiration + + caveat somecaveat(somecondition int) { + somecondition == 42 + } + + definition user {} + + definition document { + relation viewer: user with somecaveat and expiration + permission view = viewer + } + +relationships: >- + document:firstdoc#viewer@user:tom[somecaveat][expiration:2023-12-01T00:00:00Z] + + document:firstdoc#viewer@user:fred[somecaveat][expiration:2300-12-01T00:00:00Z] + + document:firstdoc#viewer@user:sarah[somecaveat:{"somecondition":42}][expiration:2300-12-01T00:00:00Z] + + document:firstdoc#viewer@user:tracy[somecaveat:{"somecondition":41}][expiration:2300-12-01T00:00:00Z] +assertions: + assertTrue: + - 'document:firstdoc#view@user:fred with {"somecondition": 42}' + - "document:firstdoc#view@user:sarah" + assertCaveated: + - "document:firstdoc#view@user:fred" + assertFalse: + - "document:firstdoc#view@user:tom" + - 'document:firstdoc#view@user:fred with {"somecondition": 41}' + - "document:firstdoc#viewer@user:tracy" diff --git a/internal/services/integrationtesting/testconfigs/indirectrelexpiration.yaml b/internal/services/integrationtesting/testconfigs/indirectrelexpiration.yaml new file mode 100644 index 0000000000..4155452287 --- /dev/null +++ b/internal/services/integrationtesting/testconfigs/indirectrelexpiration.yaml @@ -0,0 +1,34 @@ +--- +schema: |+ + use expiration + + definition user {} + + definition team { + relation member: user with expiration + } + + definition document { + relation viewer: team#member with expiration + permission view = viewer + } + +relationships: >- + team:firstteam#member@user:tracy[expiration:2023-12-01T00:00:00Z] + + team:firstteam#member@user:tom[expiration:2300-12-01T00:00:00Z] + + team:secondteam#member@user:fred[expiration:2023-12-01T00:00:00Z] + + team:secondteam#member@user:sarah[expiration:2300-12-01T00:00:00Z] + + document:firstdoc#viewer@team:firstteam#member[expiration:2023-12-01T00:00:00Z] + + document:firstdoc#viewer@team:secondteam#member[expiration:2300-12-01T00:00:00Z] +assertions: + assertTrue: + - "document:firstdoc#view@user:sarah" + assertFalse: + - "document:firstdoc#view@user:tom" + - "document:firstdoc#view@user:fred" + - "document:firstdoc#view@user:tracy" diff --git a/internal/services/integrationtesting/testconfigs/relexpiration.yaml b/internal/services/integrationtesting/testconfigs/relexpiration.yaml new file mode 100644 index 0000000000..515e6fa7a5 --- /dev/null +++ b/internal/services/integrationtesting/testconfigs/relexpiration.yaml @@ -0,0 +1,20 @@ +--- +schema: |+ + use expiration + + definition user {} + + definition document { + relation viewer: user with expiration + permission view = viewer + } + +relationships: >- + document:firstdoc#viewer@user:tracy[expiration:2023-12-01T00:00:00Z] + + document:firstdoc#viewer@user:tom[expiration:2323-12-01T00:00:00Z] +assertions: + assertTrue: + - "document:firstdoc#view@user:tom" + assertFalse: + - "document:firstdoc#view@user:tracy" diff --git a/internal/services/v1/relationships.go b/internal/services/v1/relationships.go index 1850a58099..191191e917 100644 --- a/internal/services/v1/relationships.go +++ b/internal/services/v1/relationships.go @@ -295,14 +295,6 @@ func (ps *permissionServer) WriteRelationships(ctx context.Context, req *v1.Writ // Check for duplicate updates and create the set of caveat names to load. updateRelationshipSet := mapz.NewSet[string]() for _, update := range req.Updates { - // TODO(jschorr): Remove this once expiration is supported. - if update.Relationship.OptionalExpiresAt != nil { - return nil, ps.rewriteError( - ctx, - fmt.Errorf("expiration is not supported in this version of the API"), - ) - } - // TODO(jschorr): Change to struct-based keys. tupleStr := tuple.V1StringRelationshipWithoutCaveatOrExpiration(update.Relationship) if !updateRelationshipSet.Add(tupleStr) { diff --git a/pkg/namespace/builder.go b/pkg/namespace/builder.go index 687259af8f..cf4c745b4f 100644 --- a/pkg/namespace/builder.go +++ b/pkg/namespace/builder.go @@ -94,6 +94,16 @@ func AllowedRelationWithCaveat(namespaceName string, relationName string, withCa } } +// WithExpiration adds the expiration trait to the allowed relation. +func WithExpiration(allowedRelation *core.AllowedRelation) *core.AllowedRelation { + return &core.AllowedRelation{ + Namespace: allowedRelation.Namespace, + RelationOrWildcard: allowedRelation.RelationOrWildcard, + RequiredCaveat: allowedRelation.RequiredCaveat, + RequiredExpiration: &core.ExpirationTrait{}, + } +} + // AllowedRelationWithExpiration creates a relation reference to an allowed relation. func AllowedRelationWithExpiration(namespaceName string, relationName string) *core.AllowedRelation { return &core.AllowedRelation{ From 1165f53d694e069af571c500f866adf3055caff6 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Wed, 27 Nov 2024 13:53:58 -0500 Subject: [PATCH 06/39] Add write rels test for expiring relationship --- internal/services/v1/relationships_test.go | 24 ++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/internal/services/v1/relationships_test.go b/internal/services/v1/relationships_test.go index d770c703f7..360fb92218 100644 --- a/internal/services/v1/relationships_test.go +++ b/internal/services/v1/relationships_test.go @@ -480,6 +480,30 @@ func TestDeleteRelationshipViaWriteNoop(t *testing.T) { require.NoError(err) } +func TestWriteExpiringRelationshinps(t *testing.T) { + req := require.New(t) + + conn, cleanup, _, _ := testserver.NewTestServer(req, 0, memdb.DisableGC, true, tf.StandardDatastoreWithData) + client := v1.NewPermissionsServiceClient(conn) + t.Cleanup(cleanup) + + toWrite := tuple.MustParse("document:companyplan#expiring_viewer@user:johndoe#...[expiration:2300-01-01T00:00:00Z]") + relWritten := tuple.ToV1Relationship(toWrite) + writeReq := &v1.WriteRelationshipsRequest{ + Updates: []*v1.RelationshipUpdate{{ + Operation: v1.RelationshipUpdate_OPERATION_CREATE, + Relationship: relWritten, + }}, + } + + resp, err := client.WriteRelationships(context.Background(), writeReq) + req.NoError(err) + + // read relationship back + relRead := readFirst(req, client, resp.WrittenAt, relWritten) + req.True(proto.Equal(relWritten, relRead)) +} + func TestWriteCaveatedRelationships(t *testing.T) { for _, deleteWithCaveat := range []bool{true, false} { t.Run(fmt.Sprintf("with-caveat-%v", deleteWithCaveat), func(t *testing.T) { From 741af9750ac2bdd31cf88a5212baad0e36cfbe24 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Wed, 27 Nov 2024 14:09:55 -0500 Subject: [PATCH 07/39] Add support in schema change checking for expiration --- internal/datastore/memdb/readonly.go | 7 ++ internal/services/shared/schema.go | 10 ++- internal/services/v1/schema_test.go | 117 +++++++++++++++++++++++++++ pkg/datastore/datastore.go | 19 +++++ pkg/datastore/test/relationships.go | 30 +++++++ 5 files changed, 182 insertions(+), 1 deletion(-) diff --git a/internal/datastore/memdb/readonly.go b/internal/datastore/memdb/readonly.go index 427ea0ea6a..29c96dc7d8 100644 --- a/internal/datastore/memdb/readonly.go +++ b/internal/datastore/memdb/readonly.go @@ -140,6 +140,7 @@ func (r *memdbReader) QueryRelationships( filter.OptionalResourceRelation, filter.OptionalSubjectsSelectors, filter.OptionalCaveatName, + filter.OptionalExpirationOption, makeCursorFilterFn(queryOpts.After, queryOpts.Sort), ) filteredIterator := memdb.NewFilterIterator(bestIterator, matchingRelationshipsFilterFunc) @@ -202,6 +203,7 @@ func (r *memdbReader) ReverseQueryRelationships( filterRelation, []datastore.SubjectsSelector{subjectsFilter.AsSelector()}, "", + datastore.ExpirationFilterOptionNone, makeCursorFilterFn(queryOpts.AfterForReverse, queryOpts.SortForReverse), ) filteredIterator := memdb.NewFilterIterator(iterator, matchingRelationshipsFilterFunc) @@ -386,6 +388,7 @@ func filterFuncForFilters( optionalRelation string, optionalSubjectsSelectors []datastore.SubjectsSelector, optionalCaveatFilter string, + optionalExpirationFilter datastore.ExpirationFilterOption, cursorFilter func(*relationship) bool, ) memdb.FilterFunc { return func(tupleRaw interface{}) bool { @@ -402,6 +405,10 @@ func filterFuncForFilters( return true case optionalCaveatFilter != "" && (tuple.caveat == nil || tuple.caveat.caveatName != optionalCaveatFilter): return true + case optionalExpirationFilter == datastore.ExpirationFilterOptionHasExpiration && tuple.expiration == nil: + return true + case optionalExpirationFilter == datastore.ExpirationFilterOptionNoExpiration && tuple.expiration != nil: + return true } applySubjectSelector := func(selector datastore.SubjectsSelector) bool { diff --git a/internal/services/shared/schema.go b/internal/services/shared/schema.go index f4b7592e16..b2f58d08a5 100644 --- a/internal/services/shared/schema.go +++ b/internal/services/shared/schema.go @@ -388,6 +388,7 @@ func sanityCheckNamespaceChanges( var optionalSubjectIds []string var relationFilter datastore.SubjectRelationFilter optionalCaveatName := "" + expirationOption := datastore.ExpirationFilterOptionNoExpiration if delta.AllowedType.GetPublicWildcard() != nil { optionalSubjectIds = []string{tuple.PublicWildcard} @@ -401,6 +402,12 @@ func sanityCheckNamespaceChanges( optionalCaveatName = delta.AllowedType.GetRequiredCaveat().CaveatName } + if delta.AllowedType.RequiredExpiration != nil { + expirationOption = datastore.ExpirationFilterOptionHasExpiration + } else { + expirationOption = datastore.ExpirationFilterOptionNoExpiration + } + qyr, qyrErr := rwt.QueryRelationships( ctx, datastore.RelationshipsFilter{ @@ -413,7 +420,8 @@ func sanityCheckNamespaceChanges( RelationFilter: relationFilter, }, }, - OptionalCaveatName: optionalCaveatName, + OptionalCaveatName: optionalCaveatName, + OptionalExpirationOption: expirationOption, }, options.WithLimit(options.LimitOne), ) diff --git a/internal/services/v1/schema_test.go b/internal/services/v1/schema_test.go index c3df95e77c..b7df58e217 100644 --- a/internal/services/v1/schema_test.go +++ b/internal/services/v1/schema_test.go @@ -585,3 +585,120 @@ func TestSchemaInvalid(t *testing.T) { grpcutil.RequireStatus(t, codes.InvalidArgument, err) require.ErrorContains(t, err, "found token TokenTypeStar") } + +func TestSchemaChangeExpiration(t *testing.T) { + conn, cleanup, _, _ := testserver.NewTestServer(require.New(t), 0, memdb.DisableGC, true, tf.EmptyDatastore) + t.Cleanup(cleanup) + client := v1.NewSchemaServiceClient(conn) + v1client := v1.NewPermissionsServiceClient(conn) + + // Write a basic schema with expiration. + originalSchema := ` + use expiration + + definition user {} + + definition document { + relation somerelation: user with expiration + }` + _, err := client.WriteSchema(context.Background(), &v1.WriteSchemaRequest{ + Schema: originalSchema, + }) + require.NoError(t, err) + + // Write the relationship referencing the expiration. + toWrite := tuple.MustParse("document:somedoc#somerelation@user:tom[expiration:2300-01-01T00:00:00Z]") + _, err = v1client.WriteRelationships(context.Background(), &v1.WriteRelationshipsRequest{ + Updates: []*v1.RelationshipUpdate{tuple.MustUpdateToV1RelationshipUpdate(tuple.Create( + toWrite, + ))}, + }) + require.Nil(t, err) + + newSchema := "definition document {\n\trelation somerelation: user\n}\n\ndefinition user {}" + + // Attempt to change the relation type, which should fail. + _, err = client.WriteSchema(context.Background(), &v1.WriteSchemaRequest{ + Schema: newSchema, + }) + grpcutil.RequireStatus(t, codes.InvalidArgument, err) + require.Equal(t, "rpc error: code = InvalidArgument desc = cannot remove allowed type `user with expiration` from relation `somerelation` in object definition `document`, as a relationship exists with it", err.Error()) + + // Delete the relationship. + _, err = v1client.WriteRelationships(context.Background(), &v1.WriteRelationshipsRequest{ + Updates: []*v1.RelationshipUpdate{tuple.MustUpdateToV1RelationshipUpdate(tuple.Delete( + toWrite, + ))}, + }) + require.Nil(t, err) + + // Attempt to delete the relation type, which should work now. + _, err = client.WriteSchema(context.Background(), &v1.WriteSchemaRequest{ + Schema: newSchema, + }) + require.Nil(t, err) + + // Ensure it was deleted. + readback, err := client.ReadSchema(context.Background(), &v1.ReadSchemaRequest{}) + require.NoError(t, err) + require.Equal(t, newSchema, readback.SchemaText) + + // Add the relationship back without expiration. + toWriteWithoutExp := tuple.MustParse("document:somedoc#somerelation@user:tom") + _, err = v1client.WriteRelationships(context.Background(), &v1.WriteRelationshipsRequest{ + Updates: []*v1.RelationshipUpdate{tuple.MustUpdateToV1RelationshipUpdate(tuple.Create( + toWriteWithoutExp, + ))}, + }) + require.Nil(t, err) + + // Attempt to change the relation type back to including expiration, which should fail. + _, err = client.WriteSchema(context.Background(), &v1.WriteSchemaRequest{ + Schema: originalSchema, + }) + grpcutil.RequireStatus(t, codes.InvalidArgument, err) + require.Equal(t, "rpc error: code = InvalidArgument desc = cannot remove allowed type `user` from relation `somerelation` in object definition `document`, as a relationship exists with it", err.Error()) +} + +func TestSchemaChangeExpirationAllowed(t *testing.T) { + conn, cleanup, _, _ := testserver.NewTestServer(require.New(t), 0, memdb.DisableGC, true, tf.EmptyDatastore) + t.Cleanup(cleanup) + client := v1.NewSchemaServiceClient(conn) + v1client := v1.NewPermissionsServiceClient(conn) + + // Write a basic schema with expiration. + originalSchema := ` + use expiration + + definition user {} + + definition document { + relation somerelation: user | user with expiration + }` + _, err := client.WriteSchema(context.Background(), &v1.WriteSchemaRequest{ + Schema: originalSchema, + }) + require.NoError(t, err) + + // Write the relationship without referencing the expiration. + toWrite := tuple.MustParse("document:somedoc#somerelation@user:tom") + _, err = v1client.WriteRelationships(context.Background(), &v1.WriteRelationshipsRequest{ + Updates: []*v1.RelationshipUpdate{tuple.MustUpdateToV1RelationshipUpdate(tuple.Create( + toWrite, + ))}, + }) + require.Nil(t, err) + + newSchema := "definition document {\n\trelation somerelation: user\n}\n\ndefinition user {}" + + // Attempt to change the schema to remove the expiration, which should work. + _, err = client.WriteSchema(context.Background(), &v1.WriteSchemaRequest{ + Schema: newSchema, + }) + require.Nil(t, err) + + // Ensure it was deleted. + readback, err := client.ReadSchema(context.Background(), &v1.ReadSchemaRequest{}) + require.NoError(t, err) + require.Equal(t, newSchema, readback.SchemaText) +} diff --git a/pkg/datastore/datastore.go b/pkg/datastore/datastore.go index 0174f8dadc..e78420c71f 100644 --- a/pkg/datastore/datastore.go +++ b/pkg/datastore/datastore.go @@ -83,6 +83,14 @@ func (rc *RevisionChanges) MarshalZerologObject(e *zerolog.Event) { e.Int("num-changed-relationships", len(rc.RelationshipChanges)) } +type ExpirationFilterOption int + +const ( + ExpirationFilterOptionNone ExpirationFilterOption = iota + ExpirationFilterOptionHasExpiration + ExpirationFilterOptionNoExpiration +) + // RelationshipsFilter is a filter for relationships. type RelationshipsFilter struct { // OptionalResourceType is the namespace/type for the resources to be found. @@ -106,6 +114,9 @@ type RelationshipsFilter struct { // OptionalCaveatName is the filter to use for caveated relationships, filtering by a specific caveat name. // If nil, all caveated and non-caveated relationships are allowed OptionalCaveatName string + + // OptionalExpirationOption is the filter to use for relationships with or without an expiration. + OptionalExpirationOption ExpirationFilterOption } // Test returns true iff the given relationship is matched by this filter. @@ -141,6 +152,14 @@ func (rf RelationshipsFilter) Test(relationship tuple.Relationship) bool { } } + if rf.OptionalExpirationOption == ExpirationFilterOptionHasExpiration && relationship.OptionalExpiration == nil { + return false + } + + if rf.OptionalExpirationOption == ExpirationFilterOptionNoExpiration && relationship.OptionalExpiration != nil { + return false + } + return true } diff --git a/pkg/datastore/test/relationships.go b/pkg/datastore/test/relationships.go index 3109a30f9b..a2b77056b4 100644 --- a/pkg/datastore/test/relationships.go +++ b/pkg/datastore/test/relationships.go @@ -1367,6 +1367,36 @@ func QueryRelationshipsWithVariousFiltersTest(t *testing.T, tester DatastoreTest "document:first#expiring_viewer@user:sarah[expiration:2900-01-02T01:02:03Z]", }, }, + { + name: "relationship expiration filtered by expiration required", + filter: datastore.RelationshipsFilter{ + OptionalResourceType: "document", + OptionalExpirationOption: datastore.ExpirationFilterOptionHasExpiration, + }, + relationships: []string{ + "document:first#viewer@user:tom", + "document:first#expiring_viewer@user:fred[expiration:2021-01-01T00:00:00Z]", + "document:first#expiring_viewer@user:sarah[expiration:2900-01-02T01:02:03Z]", + }, + expected: []string{ + "document:first#expiring_viewer@user:sarah[expiration:2900-01-02T01:02:03Z]", + }, + }, + { + name: "relationship expiration filtered by expiration disallowed", + filter: datastore.RelationshipsFilter{ + OptionalResourceType: "document", + OptionalExpirationOption: datastore.ExpirationFilterOptionNoExpiration, + }, + relationships: []string{ + "document:first#viewer@user:tom", + "document:first#expiring_viewer@user:fred[expiration:2021-01-01T00:00:00Z]", + "document:first#expiring_viewer@user:sarah[expiration:2900-01-02T01:02:03Z]", + }, + expected: []string{ + "document:first#viewer@user:tom", + }, + }, } for _, tc := range tcs { From 7c2ea03fb979077147c0f120b081916ac01fa60e Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Wed, 27 Nov 2024 15:27:56 -0500 Subject: [PATCH 08/39] Further improvement to caveat validation errors for relationship expiration --- internal/relationships/errors.go | 3 +- internal/relationships/validation_test.go | 36 +++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/internal/relationships/errors.go b/internal/relationships/errors.go index 5597ce8574..d51fa7c5af 100644 --- a/internal/relationships/errors.go +++ b/internal/relationships/errors.go @@ -48,7 +48,8 @@ func NewInvalidSubjectTypeError( if allowedType.RequiredCaveat != nil && allowedType.RequiredCaveat.CaveatName != "" && allowedType.Namespace == relationship.Subject.ObjectType && - allowedType.GetRelation() == relationship.Subject.Relation { + allowedType.GetRelation() == relationship.Subject.Relation && + (allowedType.RequiredExpiration != nil) == (relationship.OptionalExpiration != nil) { allowedCaveatsForSubject.Add(allowedType.RequiredCaveat.CaveatName) } } diff --git a/internal/relationships/validation_test.go b/internal/relationships/validation_test.go index 69da383047..ef645c50fe 100644 --- a/internal/relationships/validation_test.go +++ b/internal/relationships/validation_test.go @@ -267,6 +267,42 @@ func TestValidateRelationshipOperations(t *testing.T) { core.RelationTupleUpdate_CREATE, "", }, + { + "expiration missing caveat test", + ` + use expiration + + definition user {} + + caveat somecaveat(somecondition int) { + somecondition == 42 + } + + definition resource { + relation viewer: user with somecaveat and expiration + }`, + "resource:foo#viewer@user:tom[expiration:2021-01-01T00:00:00Z]", + core.RelationTupleUpdate_CREATE, + "subjects of type `user with expiration` are not allowed on relation `resource#viewer` without one of the following caveats: somecaveat", + }, + { + "expiration invalid caveat test", + ` + use expiration + + definition user {} + + caveat anothercaveat(somecondition int) { + somecondition == 42 + } + + definition resource { + relation viewer: user with anothercaveat and expiration + }`, + "resource:foo#viewer@user:tom[somecaveat][expiration:2021-01-01T00:00:00Z]", + core.RelationTupleUpdate_CREATE, + "subjects of type `user with somecaveat and expiration` are not allowed on relation `resource#viewer`", + }, } for _, tc := range tcs { From c89355be9b5d80dec63af64279441116696bbabd Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Wed, 27 Nov 2024 15:49:37 -0500 Subject: [PATCH 09/39] Enable expiration on bulk import of relationships --- internal/services/v1/experimental.go | 7 +++++++ internal/services/v1/permissions.go | 7 +++++++ internal/services/v1/permissions_test.go | 22 +++++++++++++++++----- internal/services/v1/relationships_test.go | 19 +++++++++++++++++++ 4 files changed, 50 insertions(+), 5 deletions(-) diff --git a/internal/services/v1/experimental.go b/internal/services/v1/experimental.go index d2dddb4115..fe892a9b5e 100644 --- a/internal/services/v1/experimental.go +++ b/internal/services/v1/experimental.go @@ -196,6 +196,13 @@ func (a *bulkLoadAdapter) Next(_ context.Context) (*tuple.Relationship, error) { a.current.Subject.ObjectID = a.currentBatch[a.numSent].Subject.Object.ObjectId a.current.Subject.Relation = stringz.DefaultEmpty(a.currentBatch[a.numSent].Subject.OptionalRelation, tuple.Ellipsis) + if a.currentBatch[a.numSent].OptionalExpiresAt != nil { + t := a.currentBatch[a.numSent].OptionalExpiresAt.AsTime() + a.current.OptionalExpiration = &t + } else { + a.current.OptionalExpiration = nil + } + if err := relationships.ValidateOneRelationship( a.referencedNamespaceMap, a.referencedCaveatMap, diff --git a/internal/services/v1/permissions.go b/internal/services/v1/permissions.go index c5a471770f..8ab94a5580 100644 --- a/internal/services/v1/permissions.go +++ b/internal/services/v1/permissions.go @@ -785,6 +785,13 @@ func (a *loadBulkAdapter) Next(_ context.Context) (*tuple.Relationship, error) { a.current.Subject.ObjectID = a.currentBatch[a.numSent].Subject.Object.ObjectId a.current.Subject.Relation = stringz.DefaultEmpty(a.currentBatch[a.numSent].Subject.OptionalRelation, tuple.Ellipsis) + if a.currentBatch[a.numSent].OptionalExpiresAt != nil { + t := a.currentBatch[a.numSent].OptionalExpiresAt.AsTime() + a.current.OptionalExpiration = &t + } else { + a.current.OptionalExpiration = nil + } + if err := relationships.ValidateOneRelationship( a.referencedNamespaceMap, a.referencedCaveatMap, diff --git a/internal/services/v1/permissions_test.go b/internal/services/v1/permissions_test.go index 12b150dc9e..2815209dae 100644 --- a/internal/services/v1/permissions_test.go +++ b/internal/services/v1/permissions_test.go @@ -2066,9 +2066,9 @@ func TestImportBulkRelationships(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - for _, withCaveats := range []bool{true, false} { - withCaveats := withCaveats - t.Run(fmt.Sprintf("withCaveats=%t", withCaveats), func(t *testing.T) { + for _, withTrait := range []string{"", "caveated_viewer", "expiring_viewer"} { + withTrait := withTrait + t.Run(fmt.Sprintf("withTrait=%s", withTrait), func(t *testing.T) { require := require.New(t) conn, cleanup, _, _ := testserver.NewTestServer(require, 0, memdb.DisableGC, true, tf.StandardDatastoreWithSchema) @@ -2086,7 +2086,7 @@ func TestImportBulkRelationships(t *testing.T) { batch := make([]*v1.Relationship, 0, batchSize) for i := uint64(0); i < batchSize; i++ { - if withCaveats { + if withTrait == "caveated_viewer" { batch = append(batch, relWithCaveat( tf.DocumentNS.Name, strconv.Itoa(batchNum)+"_"+strconv.FormatUint(i, 10), @@ -2096,6 +2096,16 @@ func TestImportBulkRelationships(t *testing.T) { "", "test", )) + } else if withTrait == "expiring_viewer" { + batch = append(batch, relWithExpiration( + tf.DocumentNS.Name, + strconv.Itoa(batchNum)+"_"+strconv.FormatUint(i, 10), + "expiring_viewer", + tf.UserNS.Name, + strconv.FormatUint(i, 10), + "", + time.Date(2300, 1, 1, 0, 0, 0, 0, time.UTC), + )) } else { batch = append(batch, rel( tf.DocumentNS.Name, @@ -2139,9 +2149,11 @@ func TestImportBulkRelationships(t *testing.T) { continue } - if withCaveats { + if withTrait == "caveated_viewer" { require.NotNil(res.Relationship.OptionalCaveat) require.Equal("test", res.Relationship.OptionalCaveat.CaveatName) + } else if withTrait == "expiring_viewer" { + require.NotNil(res.Relationship.OptionalExpiresAt) } else { require.Nil(res.Relationship.OptionalCaveat) } diff --git a/internal/services/v1/relationships_test.go b/internal/services/v1/relationships_test.go index 360fb92218..2a2e2f0302 100644 --- a/internal/services/v1/relationships_test.go +++ b/internal/services/v1/relationships_test.go @@ -19,6 +19,7 @@ import ( "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/types/known/structpb" + "google.golang.org/protobuf/types/known/timestamppb" "github.com/authzed/spicedb/internal/datastore/memdb" tf "github.com/authzed/spicedb/internal/testfixtures" @@ -633,6 +634,24 @@ func rel(resType, resID, relation, subType, subID, subRel string) *v1.Relationsh } } +func relWithExpiration(resType, resID, relation, subType, subID, subRel string, expiration time.Time) *v1.Relationship { + return &v1.Relationship{ + Resource: &v1.ObjectReference{ + ObjectType: resType, + ObjectId: resID, + }, + Relation: relation, + Subject: &v1.SubjectReference{ + Object: &v1.ObjectReference{ + ObjectType: subType, + ObjectId: subID, + }, + OptionalRelation: subRel, + }, + OptionalExpiresAt: timestamppb.New(expiration), + } +} + func relWithCaveat(resType, resID, relation, subType, subID, subRel, caveatName string) *v1.Relationship { return &v1.Relationship{ Resource: &v1.ObjectReference{ From 4a001b4443f40d7a28ce3a40e61cce3e46a86fff Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Mon, 2 Dec 2024 15:30:14 -0500 Subject: [PATCH 10/39] Add support in Postgres driver for expiration --- internal/datastore/common/sql.go | 17 ++++++++ internal/datastore/common/sql_test.go | 2 + internal/datastore/postgres/common/bulk.go | 1 + internal/datastore/postgres/common/pgx.go | 8 +++- ...z_migration.0021_add_expiration_support.go | 37 +++++++++++++++++ internal/datastore/postgres/postgres.go | 1 + .../postgres/postgres_shared_test.go | 1 + internal/datastore/postgres/reader.go | 2 + internal/datastore/postgres/readwrite.go | 12 ++++-- internal/datastore/postgres/watch.go | 6 +++ pkg/typesystem/typesystem.go | 40 +++++++++++++++++++ 11 files changed, 121 insertions(+), 6 deletions(-) create mode 100644 internal/datastore/postgres/migrations/zz_migration.0021_add_expiration_support.go diff --git a/internal/datastore/common/sql.go b/internal/datastore/common/sql.go index 536de9385f..1ceef9e99a 100644 --- a/internal/datastore/common/sql.go +++ b/internal/datastore/common/sql.go @@ -71,6 +71,7 @@ type SchemaInformation struct { colUsersetObjectID string colUsersetRelation string colCaveatName string + colExpiration string paginationFilterType PaginationFilterType } @@ -82,6 +83,7 @@ func NewSchemaInformation( colUsersetObjectID, colUsersetRelation, colCaveatName string, + colExpiration string, paginationFilterType PaginationFilterType, ) SchemaInformation { return SchemaInformation{ @@ -92,6 +94,7 @@ func NewSchemaInformation( colUsersetObjectID, colUsersetRelation, colCaveatName, + colExpiration, paginationFilterType, } } @@ -404,6 +407,12 @@ func (sqf SchemaQueryFilterer) FilterWithRelationshipsFilter(filter datastore.Re csqf = csqf.FilterWithCaveatName(filter.OptionalCaveatName) } + if filter.OptionalExpirationOption == datastore.ExpirationFilterOptionHasExpiration { + csqf.queryBuilder = csqf.queryBuilder.Where(sq.NotEq{csqf.schema.colExpiration: nil}) + } else if filter.OptionalExpirationOption == datastore.ExpirationFilterOptionNoExpiration { + csqf.queryBuilder = csqf.queryBuilder.Where(sq.Eq{csqf.schema.colExpiration: nil}) + } + return csqf, nil } @@ -563,6 +572,14 @@ func (tqs QueryExecutor) ExecuteQuery( } toExecute := query.limit(limit) + + // Filter out any expired relationships. + toExecute.queryBuilder = toExecute.queryBuilder.Where(sq.Or{ + sq.Eq{query.schema.colExpiration: nil}, + sq.Expr(query.schema.colExpiration + " > NOW()"), + }) + + // Run the query. sql, args, err := toExecute.queryBuilder.ToSql() if err != nil { return nil, err diff --git a/internal/datastore/common/sql_test.go b/internal/datastore/common/sql_test.go index e61339f2f6..0fd67f67eb 100644 --- a/internal/datastore/common/sql_test.go +++ b/internal/datastore/common/sql_test.go @@ -669,6 +669,7 @@ func TestSchemaQueryFilterer(t *testing.T) { "subject_object_id", "subject_relation", "caveat", + "expiration", TupleComparison, ) filterer := NewSchemaQueryFilterer(schema, base, 100) @@ -693,6 +694,7 @@ func BenchmarkSchemaFilterer(b *testing.B) { "resource_id", "resource_relation", "caveat_name", + "expiration", TupleComparison, ) sqf := NewSchemaQueryFilterer(si, sq.Select("*"), 100) diff --git a/internal/datastore/postgres/common/bulk.go b/internal/datastore/postgres/common/bulk.go index 1360fe288a..bfa1b4e636 100644 --- a/internal/datastore/postgres/common/bulk.go +++ b/internal/datastore/postgres/common/bulk.go @@ -46,6 +46,7 @@ func (tg *tupleSourceAdapter) Values() ([]any, error) { tg.valuesBuffer[5] = tg.current.Subject.Relation tg.valuesBuffer[6] = caveatName tg.valuesBuffer[7] = caveatContext + tg.valuesBuffer[8] = tg.current.OptionalExpiration if len(tg.colNames) > 8 && tg.current.OptionalIntegrity != nil { tg.valuesBuffer[8] = tg.current.OptionalIntegrity.KeyId diff --git a/internal/datastore/postgres/common/pgx.go b/internal/datastore/postgres/common/pgx.go index 546fe98945..9e58944f2e 100644 --- a/internal/datastore/postgres/common/pgx.go +++ b/internal/datastore/postgres/common/pgx.go @@ -58,6 +58,7 @@ func queryRels(ctx context.Context, sqlStatement string, args []any, span trace. var subjectRelation string var caveatName sql.NullString var caveatCtx map[string]any + var expiration *time.Time relCount := 0 for rows.Next() { @@ -77,6 +78,7 @@ func queryRels(ctx context.Context, sqlStatement string, args []any, span trace. &subjectRelation, &caveatName, &caveatCtx, + &expiration, &integrityKeyID, &integrityHash, ×tamp, @@ -99,6 +101,7 @@ func queryRels(ctx context.Context, sqlStatement string, args []any, span trace. &subjectRelation, &caveatName, &caveatCtx, + &expiration, ); err != nil { return fmt.Errorf(errUnableToQueryTuples, fmt.Errorf("scan err: %w", err)) } @@ -127,8 +130,9 @@ func queryRels(ctx context.Context, sqlStatement string, args []any, span trace. Relation: subjectRelation, }, }, - OptionalCaveat: caveat, - OptionalIntegrity: integrity, + OptionalCaveat: caveat, + OptionalIntegrity: integrity, + OptionalExpiration: expiration, }, nil) { return nil } diff --git a/internal/datastore/postgres/migrations/zz_migration.0021_add_expiration_support.go b/internal/datastore/postgres/migrations/zz_migration.0021_add_expiration_support.go new file mode 100644 index 0000000000..3f6e148dfa --- /dev/null +++ b/internal/datastore/postgres/migrations/zz_migration.0021_add_expiration_support.go @@ -0,0 +1,37 @@ +package migrations + +import ( + "context" + "fmt" + + "github.com/jackc/pgx/v5" +) + +const addExpirationColumn = ` + ALTER TABLE relation_tuple + ADD COLUMN expiration TIMESTAMPTZ DEFAULT NULL; +` + +const addWithExpirationCoveringIndex = `CREATE INDEX CONCURRENTLY + IF NOT EXISTS ix_relation_tuple_with_expiration + ON relation_tuple (namespace, relation, object_id, created_xid, expiration) + INCLUDE (userset_namespace, userset_object_id, userset_relation, caveat_name, caveat_context) + WHERE deleted_xid = '9223372036854775807'::xid8;` + +func init() { + if err := DatabaseMigrations.Register("add-expiration-support", "add-watch-api-index-to-relation-tuple-table", + func(ctx context.Context, conn *pgx.Conn) error { + if _, err := conn.Exec(ctx, addExpirationColumn); err != nil { + return fmt.Errorf("failed to add expiration column to relation tuple table: %w", err) + } + + if _, err := conn.Exec(ctx, addWithExpirationCoveringIndex); err != nil { + return fmt.Errorf("failed to add expiration column to relation tuple table: %w", err) + } + + return nil + }, + noTxMigration); err != nil { + panic("failed to register migration: " + err.Error()) + } +} diff --git a/internal/datastore/postgres/postgres.go b/internal/datastore/postgres/postgres.go index 996419fbd4..dec442c9b8 100644 --- a/internal/datastore/postgres/postgres.go +++ b/internal/datastore/postgres/postgres.go @@ -64,6 +64,7 @@ const ( colCaveatDefinition = "definition" colCaveatContextName = "caveat_name" colCaveatContext = "caveat_context" + colExpiration = "expiration" colCounterName = "name" colCounterFilter = "serialized_filter" diff --git a/internal/datastore/postgres/postgres_shared_test.go b/internal/datastore/postgres/postgres_shared_test.go index 14cf25bcc2..7e9573c77f 100644 --- a/internal/datastore/postgres/postgres_shared_test.go +++ b/internal/datastore/postgres/postgres_shared_test.go @@ -1513,6 +1513,7 @@ func NullCaveatWatchTest(t *testing.T, ds datastore.Datastore) { "...", nil, // set explicitly to null nil, // set explicitly to null + nil, // no expiration } query := createInserts.Values(valuesToWrite...) diff --git a/internal/datastore/postgres/reader.go b/internal/datastore/postgres/reader.go index 75c6cd7afb..cdf76ce63f 100644 --- a/internal/datastore/postgres/reader.go +++ b/internal/datastore/postgres/reader.go @@ -34,6 +34,7 @@ var ( colUsersetRelation, colCaveatContextName, colCaveatContext, + colExpiration, ).From(tableTuple) countRels = psql.Select("COUNT(*)").From(tableTuple) @@ -46,6 +47,7 @@ var ( colUsersetObjectID, colUsersetRelation, colCaveatContextName, + colExpiration, common.TupleComparison, ) diff --git a/internal/datastore/postgres/readwrite.go b/internal/datastore/postgres/readwrite.go index 01dbcd37b2..7e8b58f2c1 100644 --- a/internal/datastore/postgres/readwrite.go +++ b/internal/datastore/postgres/readwrite.go @@ -53,6 +53,7 @@ var ( colUsersetRelation, colCaveatContextName, colCaveatContext, + colExpiration, ) deleteTuple = psql.Update(tableTuple).Where(sq.Eq{colDeletedXid: liveDeletedTxnID}) @@ -100,7 +101,8 @@ func appendForInsertion(builder sq.InsertBuilder, tpl tuple.Relationship) sq.Ins tpl.Subject.ObjectID, tpl.Subject.Relation, caveatName, - caveatContext, // PGX driver serializes map[string]any to JSONB type columns + caveatContext, // PGX driver serializes map[string]any to JSONB type columns, + tpl.OptionalExpiration, } return builder.Values(valuesToWrite...) @@ -152,7 +154,7 @@ func (rwt *pgReadWriteTXN) collectSimplifiedTouchTypes(ctx context.Context, muta return nil, handleWriteError(err) } - notAllowed, err := vts.RelationDoesNotAllowCaveatsForSubject(rel.Resource.Relation, rel.Subject.ObjectType) + notAllowed, err := vts.RelationDoesNotAllowCaveatsOrTraitsForSubject(rel.Resource.Relation, rel.Subject.ObjectType) if err != nil { // Ignore errors and just fallback to the less efficient path. continue @@ -302,7 +304,7 @@ func (rwt *pgReadWriteTXN) WriteRelationships(ctx context.Context, mutations []t continue } - deleteClauses = append(deleteClauses, exactRelationshipDifferentCaveatClause(mut.Relationship)) + deleteClauses = append(deleteClauses, exactRelationshipDifferentCaveatAndExpirationClause(mut.Relationship)) } } @@ -756,7 +758,7 @@ func exactRelationshipClause(r tuple.Relationship) sq.Eq { } } -func exactRelationshipDifferentCaveatClause(r tuple.Relationship) sq.And { +func exactRelationshipDifferentCaveatAndExpirationClause(r tuple.Relationship) sq.And { var caveatName string var caveatContext map[string]any if r.OptionalCaveat != nil { @@ -764,6 +766,7 @@ func exactRelationshipDifferentCaveatClause(r tuple.Relationship) sq.And { caveatContext = r.OptionalCaveat.Context.AsMap() } + expiration := r.OptionalExpiration return sq.And{ sq.Eq{ colNamespace: r.Resource.ObjectType, @@ -775,6 +778,7 @@ func exactRelationshipDifferentCaveatClause(r tuple.Relationship) sq.And { }, sq.Or{ sq.Expr(fmt.Sprintf(`%s IS DISTINCT FROM ?`, colCaveatContextName), caveatName), + sq.Expr(fmt.Sprintf(`%s IS DISTINCT FROM ?`, colExpiration), expiration), sq.NotEq{ colCaveatContext: caveatContext, }, diff --git a/internal/datastore/postgres/watch.go b/internal/datastore/postgres/watch.go index d8a8253bdc..45b3bb64dd 100644 --- a/internal/datastore/postgres/watch.go +++ b/internal/datastore/postgres/watch.go @@ -42,6 +42,7 @@ var ( colUsersetRelation, colCaveatContextName, colCaveatContext, + colExpiration, colCreatedXid, colDeletedXid, ).From(tableTuple) @@ -358,6 +359,9 @@ func (pgd *pgDatastore) loadRelationshipChanges(ctx context.Context, xmin uint64 var createdXID, deletedXID xid8 var caveatName *string var caveatContext map[string]any + + var expiration *time.Time + if err := changes.Scan( &resourceObjectType, &resourceObjectID, @@ -367,6 +371,7 @@ func (pgd *pgDatastore) loadRelationshipChanges(ctx context.Context, xmin uint64 &subjectRelation, &caveatName, &caveatContext, + &expiration, &createdXID, &deletedXID, ); err != nil { @@ -386,6 +391,7 @@ func (pgd *pgDatastore) loadRelationshipChanges(ctx context.Context, xmin uint64 Relation: subjectRelation, }, }, + OptionalExpiration: expiration, } if caveatName != nil && *caveatName != "" { diff --git a/pkg/typesystem/typesystem.go b/pkg/typesystem/typesystem.go index 337b2d364d..481c55efd9 100644 --- a/pkg/typesystem/typesystem.go +++ b/pkg/typesystem/typesystem.go @@ -691,6 +691,46 @@ func (nts *TypeSystem) TypeSystemForNamespace(ctx context.Context, namespaceName return NewNamespaceTypeSystem(nsDef, nts.resolver) } +// RelationDoesNotAllowCaveatsOrTraitsForSubject returns true if and only if it can be conclusively determined that +// the given subject type does not accept any caveats or traits on the given relation. If the relation does not have type information, +// returns an error. +func (nts *TypeSystem) RelationDoesNotAllowCaveatsOrTraitsForSubject(relationName string, subjectTypeName string) (bool, error) { + relation, ok := nts.relationMap[relationName] + if !ok { + return false, NewRelationNotFoundErr(nts.nsDef.Name, relationName) + } + + typeInfo := relation.GetTypeInformation() + if typeInfo == nil { + return false, NewTypeWithSourceError( + fmt.Errorf("relation `%s` does not have type information", relationName), + relation, relationName, + ) + } + + foundSubjectType := false + for _, allowedRelation := range typeInfo.GetAllowedDirectRelations() { + if allowedRelation.GetNamespace() == subjectTypeName { + foundSubjectType = true + if allowedRelation.GetRequiredCaveat() != nil && allowedRelation.GetRequiredCaveat().CaveatName != "" { + return false, nil + } + if allowedRelation.GetRequiredExpiration() != nil { + return false, nil + } + } + } + + if !foundSubjectType { + return false, NewTypeWithSourceError( + fmt.Errorf("relation `%s` does not allow subject type `%s`", relationName, subjectTypeName), + relation, relationName, + ) + } + + return true, nil +} + // RelationDoesNotAllowCaveatsForSubject returns true if and only if it can be conclusively determined that // the given subject type does not accept any caveats on the given relation. If the relation does not have type information, // returns an error. From a730b7ed3d235426214e8ae6cc5fc6048f6e1089 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Mon, 2 Dec 2024 15:45:57 -0500 Subject: [PATCH 11/39] Add support in MySQL driver for expiration --- internal/datastore/mysql/datastore.go | 6 +++++- .../zz_migration.0010_add_expiration.go | 18 ++++++++++++++++++ internal/datastore/mysql/query_builder.go | 4 ++++ internal/datastore/mysql/reader.go | 1 + internal/datastore/mysql/readwrite.go | 9 +++++++-- internal/datastore/mysql/watch.go | 3 +++ 6 files changed, 38 insertions(+), 3 deletions(-) create mode 100644 internal/datastore/mysql/migrations/zz_migration.0010_add_expiration.go diff --git a/internal/datastore/mysql/datastore.go b/internal/datastore/mysql/datastore.go index 9c4e3a1285..e70ae293f7 100644 --- a/internal/datastore/mysql/datastore.go +++ b/internal/datastore/mysql/datastore.go @@ -51,6 +51,7 @@ const ( colCaveatDefinition = "definition" colCaveatName = "caveat_name" colCaveatContext = "caveat_context" + colExpiration = "expiration" colCounterName = "name" colCounterSerializedFilter = "serialized_filter" @@ -460,6 +461,7 @@ func newMySQLExecutor(tx querier) common.ExecuteQueryFunc { var subjectRelation string var caveatName string var caveatContext structpbWrapper + var expiration *time.Time err := rows.Scan( &resourceObjectType, &resourceObjectID, @@ -469,6 +471,7 @@ func newMySQLExecutor(tx querier) common.ExecuteQueryFunc { &subjectRelation, &caveatName, &caveatContext, + &expiration, ) if err != nil { yield(tuple.Relationship{}, fmt.Errorf(errUnableToQueryTuples, err)) @@ -495,7 +498,8 @@ func newMySQLExecutor(tx querier) common.ExecuteQueryFunc { Relation: subjectRelation, }, }, - OptionalCaveat: caveat, + OptionalCaveat: caveat, + OptionalExpiration: expiration, }, nil) { return } diff --git a/internal/datastore/mysql/migrations/zz_migration.0010_add_expiration.go b/internal/datastore/mysql/migrations/zz_migration.0010_add_expiration.go new file mode 100644 index 0000000000..f7057e4729 --- /dev/null +++ b/internal/datastore/mysql/migrations/zz_migration.0010_add_expiration.go @@ -0,0 +1,18 @@ +package migrations + +import "fmt" + +func addExpirationToRelationTupleTable(t *tables) string { + return fmt.Sprintf(`ALTER TABLE %s + ADD COLUMN expiration DATETIME NULL DEFAULT NULL;`, + t.RelationTuple(), + ) +} + +func init() { + mustRegisterMigration("add_expiration_to_relation_tuple", "add_metadata_to_transaction_table", noNonatomicMigration, + newStatementBatch( + addExpirationToRelationTupleTable, + ).execute, + ) +} diff --git a/internal/datastore/mysql/query_builder.go b/internal/datastore/mysql/query_builder.go index 32a21c6830..0ff69a91b8 100644 --- a/internal/datastore/mysql/query_builder.go +++ b/internal/datastore/mysql/query_builder.go @@ -161,6 +161,7 @@ func queryRelationshipsWithIds(tableTuple string) sq.SelectBuilder { colUsersetRelation, colCaveatName, colCaveatContext, + colExpiration, ).From(tableTuple) } @@ -174,6 +175,7 @@ func queryRelationships(tableTuple string) sq.SelectBuilder { colUsersetRelation, colCaveatName, colCaveatContext, + colExpiration, ).From(tableTuple) } @@ -201,6 +203,7 @@ func writeRelationship(tableTuple string) sq.InsertBuilder { colUsersetRelation, colCaveatName, colCaveatContext, + colExpiration, colCreatedTxn, ) } @@ -215,6 +218,7 @@ func queryChanged(tableTuple string) sq.SelectBuilder { colUsersetRelation, colCaveatName, colCaveatContext, + colExpiration, colCreatedTxn, colDeletedTxn, ).From(tableTuple) diff --git a/internal/datastore/mysql/reader.go b/internal/datastore/mysql/reader.go index fe278ba078..0cee5c466d 100644 --- a/internal/datastore/mysql/reader.go +++ b/internal/datastore/mysql/reader.go @@ -47,6 +47,7 @@ var schema = common.NewSchemaInformation( colUsersetObjectID, colUsersetRelation, colCaveatName, + colExpiration, common.ExpandedLogicComparison, ) diff --git a/internal/datastore/mysql/readwrite.go b/internal/datastore/mysql/readwrite.go index 9d3cae399c..cdc93c454e 100644 --- a/internal/datastore/mysql/readwrite.go +++ b/internal/datastore/mysql/readwrite.go @@ -10,6 +10,7 @@ import ( "fmt" "regexp" "strings" + "time" sq "github.com/Masterminds/squirrel" v1 "github.com/authzed/authzed-go/proto/authzed/api/v1" @@ -223,6 +224,7 @@ func (rwt *mysqlReadWriteTXN) WriteRelationships(ctx context.Context, mutations var subjectRelation string var caveatName string var caveatContext structpbWrapper + var expiration *time.Time relIdsToDelete := make([]int64, 0, len(clauses)) for rows.Next() { @@ -237,6 +239,7 @@ func (rwt *mysqlReadWriteTXN) WriteRelationships(ctx context.Context, mutations &subjectRelation, &caveatName, &caveatContext, + &expiration, ); err != nil { return fmt.Errorf(errUnableToWriteRelationships, err) } @@ -312,6 +315,7 @@ func (rwt *mysqlReadWriteTXN) WriteRelationships(ctx context.Context, mutations rel.Subject.Relation, caveatName, &caveatContext, + rel.OptionalExpiration, rwt.newTxnID, ) bulkWriteHasValues = true @@ -496,7 +500,7 @@ func (rwt *mysqlReadWriteTXN) DeleteNamespaces(ctx context.Context, nsNames ...s func (rwt *mysqlReadWriteTXN) BulkLoad(ctx context.Context, iter datastore.BulkWriteRelationshipSource) (uint64, error) { var sqlStmt bytes.Buffer - sql, _, err := rwt.WriteRelsQuery.Values(1, 2, 3, 4, 5, 6, 7, 8, 9).ToSql() + sql, _, err := rwt.WriteRelsQuery.Values(1, 2, 3, 4, 5, 6, 7, 8, 9, 10).ToSql() if err != nil { return 0, err } @@ -515,7 +519,7 @@ func (rwt *mysqlReadWriteTXN) BulkLoad(ctx context.Context, iter datastore.BulkW for ; rel != nil && err == nil && batchLen < bulkInsertRowsLimit; rel, err = iter.Next(ctx) { if batchLen != 0 { - sqlStmt.WriteString(",(?,?,?,?,?,?,?,?,?)") + sqlStmt.WriteString(",(?,?,?,?,?,?,?,?,?,?)") } var caveatName string @@ -533,6 +537,7 @@ func (rwt *mysqlReadWriteTXN) BulkLoad(ctx context.Context, iter datastore.BulkW rel.Subject.Relation, caveatName, &caveatContext, + rel.OptionalExpiration, rwt.newTxnID, ) batchLen++ diff --git a/internal/datastore/mysql/watch.go b/internal/datastore/mysql/watch.go index 4e70ed3531..8d03a34472 100644 --- a/internal/datastore/mysql/watch.go +++ b/internal/datastore/mysql/watch.go @@ -211,6 +211,7 @@ func (mds *Datastore) loadChanges( var deletedTxn uint64 var caveatName string var caveatContext structpbWrapper + var expiration *time.Time err = rows.Scan( &resourceObjectType, &resourceObjectID, @@ -220,6 +221,7 @@ func (mds *Datastore) loadChanges( &subjectRelation, &caveatName, &caveatContext, + &expiration, &createdTxn, &deletedTxn, ) @@ -240,6 +242,7 @@ func (mds *Datastore) loadChanges( Relation: subjectRelation, }, }, + OptionalExpiration: expiration, } relationship.OptionalCaveat, err = common.ContextualizedCaveatFrom(caveatName, caveatContext) From 9fe7f49a4c7dc3f89331777d8acc29c9fdaf9a75 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Mon, 2 Dec 2024 16:38:30 -0500 Subject: [PATCH 12/39] Add support in CRDB driver for expiration --- internal/datastore/crdb/crdb.go | 1 + ...z_migration.0009_add_expiration_support.go | 75 +++++++++++++++++++ internal/datastore/crdb/reader.go | 3 + internal/datastore/crdb/readwrite.go | 17 ++++- internal/datastore/postgres/common/bulk.go | 6 +- internal/datastore/postgres/common/pgx.go | 7 ++ 6 files changed, 104 insertions(+), 5 deletions(-) create mode 100644 internal/datastore/crdb/migrations/zz_migration.0009_add_expiration_support.go diff --git a/internal/datastore/crdb/crdb.go b/internal/datastore/crdb/crdb.go index 01c2bbe017..498c8ebd82 100644 --- a/internal/datastore/crdb/crdb.go +++ b/internal/datastore/crdb/crdb.go @@ -72,6 +72,7 @@ const ( colCaveatDefinition = "definition" colCaveatContextName = "caveat_name" colCaveatContext = "caveat_context" + colExpiration = "expires_at" colIntegrityHash = "integrity_hash" colIntegrityKeyID = "integrity_key_id" diff --git a/internal/datastore/crdb/migrations/zz_migration.0009_add_expiration_support.go b/internal/datastore/crdb/migrations/zz_migration.0009_add_expiration_support.go new file mode 100644 index 0000000000..2bfea2a2e9 --- /dev/null +++ b/internal/datastore/crdb/migrations/zz_migration.0009_add_expiration_support.go @@ -0,0 +1,75 @@ +package migrations + +import ( + "context" + "fmt" + "regexp" + + "github.com/Masterminds/semver" + "github.com/jackc/pgx/v5" +) + +const ( + addExpirationColumnToRelationTuple = ` + ALTER TABLE relation_tuple + ADD COLUMN IF NOT EXISTS expires_at TIMESTAMPTZ DEFAULT NULL; + + ALTER TABLE relation_tuple_with_integrity + ADD COLUMN IF NOT EXISTS expires_at TIMESTAMPTZ DEFAULT NULL; + ` + + // ttl_expiration_expression support was added in CRDB v22.2. + addExpirationPolicy = ` + ALTER TABLE relation_tuple SET (ttl_expiration_expression = 'expires_at', ttl_job_cron = '@daily'); + + ALTER TABLE relation_tuple_with_integrity SET (ttl_expiration_expression = 'expires_at', ttl_job_cron = '@daily'); + ` + + createExpirationTupleIndex = `CREATE INDEX ix_relation_tuple_with_expiration ON relation_tuple (namespace, relation, object_id, expires_at, userset_namespace, userset_object_id, userset_relation) STORING (caveat_name, caveat_context);` +) + +func init() { + err := CRDBMigrations.Register("add-expiration-support", "add-transaction-metadata-table", addExpirationSupport, noAtomicMigration) + if err != nil { + panic("failed to register migration: " + err.Error()) + } +} + +func addExpirationSupport(ctx context.Context, conn *pgx.Conn) error { + // Add the expires_at column to relation_tuple. + _, err := conn.Exec(ctx, addExpirationColumnToRelationTuple) + if err != nil { + return err + } + + // Add the expiration index. + _, err = conn.Exec(ctx, createExpirationTupleIndex) + if err != nil { + return err + } + + // Add the TTL policy to relation_tuple, if supported. + row := conn.QueryRow(ctx, "select version()") + var fullVersionString string + if err := row.Scan(&fullVersionString); err != nil { + return err + } + + re, err := regexp.Compile(semver.SemVerRegex) + if err != nil { + return fmt.Errorf("failed to compile regex: %w", err) + } + + version := re.FindString(fullVersionString) + v, err := semver.NewVersion(version) + if err != nil { + return fmt.Errorf("failed to parse version %q: %w", version, err) + } + + if v.Major() < 22 || (v.Major() == 22 && v.Minor() < 2) { + return nil + } + + _, err = conn.Exec(ctx, addExpirationPolicy) + return err +} diff --git a/internal/datastore/crdb/reader.go b/internal/datastore/crdb/reader.go index 8ae6cc2946..4876c08b80 100644 --- a/internal/datastore/crdb/reader.go +++ b/internal/datastore/crdb/reader.go @@ -37,6 +37,7 @@ var ( colUsersetObjectID, colUsersetRelation, colCaveatContextName, + colExpiration, common.ExpandedLogicComparison, ) @@ -194,6 +195,7 @@ func (cr *crdbReader) queryTuples() sq.SelectBuilder { colUsersetRelation, colCaveatContextName, colCaveatContext, + colExpiration, colIntegrityKeyID, colIntegrityHash, colTimestamp, @@ -209,6 +211,7 @@ func (cr *crdbReader) queryTuples() sq.SelectBuilder { colUsersetRelation, colCaveatContextName, colCaveatContext, + colExpiration, ) } diff --git a/internal/datastore/crdb/readwrite.go b/internal/datastore/crdb/readwrite.go index 2b70659acf..53a25cc586 100644 --- a/internal/datastore/crdb/readwrite.go +++ b/internal/datastore/crdb/readwrite.go @@ -53,7 +53,7 @@ type crdbReadWriteTXN struct { var ( upsertTupleSuffixWithoutIntegrity = fmt.Sprintf( - "ON CONFLICT (%s,%s,%s,%s,%s,%s) DO UPDATE SET %s = now(), %s = excluded.%s, %s = excluded.%s WHERE (relation_tuple.%s <> excluded.%s OR relation_tuple.%s <> excluded.%s)", + "ON CONFLICT (%s,%s,%s,%s,%s,%s) DO UPDATE SET %s = now(), %s = excluded.%s, %s = excluded.%s, %s = excluded.%s WHERE (relation_tuple.%s <> excluded.%s OR relation_tuple.%s <> excluded.%s OR relation_tuple.%s <> excluded.%s)", colNamespace, colObjectID, colRelation, @@ -65,14 +65,18 @@ var ( colCaveatContextName, colCaveatContext, colCaveatContext, + colExpiration, + colExpiration, colCaveatContextName, colCaveatContextName, colCaveatContext, colCaveatContext, + colExpiration, + colExpiration, ) upsertTupleSuffixWithIntegrity = fmt.Sprintf( - "ON CONFLICT (%s,%s,%s,%s,%s,%s) DO UPDATE SET %s = now(), %s = excluded.%s, %s = excluded.%s, %s = excluded.%s, %s = excluded.%s WHERE (relation_tuple_with_integrity.%s <> excluded.%s OR relation_tuple_with_integrity.%s <> excluded.%s)", + "ON CONFLICT (%s,%s,%s,%s,%s,%s) DO UPDATE SET %s = now(), %s = excluded.%s, %s = excluded.%s, %s = excluded.%s, %s = excluded.%s, %s = excluded.%s WHERE (relation_tuple_with_integrity.%s <> excluded.%s OR relation_tuple_with_integrity.%s <> excluded.%s OR relation_tuple_with_integrity.%s <> excluded.%s)", colNamespace, colObjectID, colRelation, @@ -88,10 +92,14 @@ var ( colIntegrityKeyID, colIntegrityHash, colIntegrityHash, + colExpiration, + colExpiration, colCaveatContextName, colCaveatContextName, colCaveatContext, colCaveatContext, + colExpiration, + colExpiration, ) queryTouchTransaction = fmt.Sprintf( @@ -133,6 +141,7 @@ func (rwt *crdbReadWriteTXN) queryWriteTuple() sq.InsertBuilder { colUsersetRelation, colCaveatContextName, colCaveatContext, + colExpiration, colIntegrityKeyID, colIntegrityHash, ) @@ -147,6 +156,7 @@ func (rwt *crdbReadWriteTXN) queryWriteTuple() sq.InsertBuilder { colUsersetRelation, colCaveatContextName, colCaveatContext, + colExpiration, ) } @@ -299,6 +309,7 @@ func (rwt *crdbReadWriteTXN) WriteRelationships(ctx context.Context, mutations [ rel.Subject.Relation, caveatName, caveatContext, + rel.OptionalExpiration, } if rwt.withIntegrity { @@ -524,6 +535,7 @@ var copyCols = []string{ colUsersetRelation, colCaveatContextName, colCaveatContext, + colExpiration, } var copyColsWithIntegrity = []string{ @@ -535,6 +547,7 @@ var copyColsWithIntegrity = []string{ colUsersetRelation, colCaveatContextName, colCaveatContext, + colExpiration, colIntegrityKeyID, colIntegrityHash, colTimestamp, diff --git a/internal/datastore/postgres/common/bulk.go b/internal/datastore/postgres/common/bulk.go index bfa1b4e636..8690e916bc 100644 --- a/internal/datastore/postgres/common/bulk.go +++ b/internal/datastore/postgres/common/bulk.go @@ -49,9 +49,9 @@ func (tg *tupleSourceAdapter) Values() ([]any, error) { tg.valuesBuffer[8] = tg.current.OptionalExpiration if len(tg.colNames) > 8 && tg.current.OptionalIntegrity != nil { - tg.valuesBuffer[8] = tg.current.OptionalIntegrity.KeyId - tg.valuesBuffer[9] = tg.current.OptionalIntegrity.Hash - tg.valuesBuffer[10] = tg.current.OptionalIntegrity.HashedAt.AsTime() + tg.valuesBuffer[9] = tg.current.OptionalIntegrity.KeyId + tg.valuesBuffer[10] = tg.current.OptionalIntegrity.Hash + tg.valuesBuffer[11] = tg.current.OptionalIntegrity.HashedAt.AsTime() } return tg.valuesBuffer, nil diff --git a/internal/datastore/postgres/common/pgx.go b/internal/datastore/postgres/common/pgx.go index 9e58944f2e..8b778d2258 100644 --- a/internal/datastore/postgres/common/pgx.go +++ b/internal/datastore/postgres/common/pgx.go @@ -116,6 +116,13 @@ func queryRels(ctx context.Context, sqlStatement string, args []any, span trace. } } + if expiration != nil { + // Ensure the returned expiration is always in UTC, as some datastores (like CRDB) + // convert to the local timezone when reading. + utc := expiration.UTC() + expiration = &utc + } + relCount++ if !yield(tuple.Relationship{ RelationshipReference: tuple.RelationshipReference{ From c05222b244eddcf035583d522beef75dfd2e40a4 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Mon, 2 Dec 2024 17:08:25 -0500 Subject: [PATCH 13/39] Add support in Spanner driver for expiration --- internal/datastore/common/sql.go | 5 ++- internal/datastore/common/sql_test.go | 2 ++ internal/datastore/crdb/reader.go | 1 + internal/datastore/mysql/reader.go | 1 + internal/datastore/postgres/reader.go | 1 + ...z_migration.0011_add_expiration_support.go | 35 +++++++++++++++++++ internal/datastore/spanner/reader.go | 14 +++++++- internal/datastore/spanner/readwrite.go | 6 ++++ internal/datastore/spanner/schema.go | 2 ++ internal/services/shared/schema.go | 4 +-- pkg/datastore/test/relationships.go | 5 +-- 11 files changed, 67 insertions(+), 9 deletions(-) create mode 100644 internal/datastore/spanner/migrations/zz_migration.0011_add_expiration_support.go diff --git a/internal/datastore/common/sql.go b/internal/datastore/common/sql.go index 1ceef9e99a..d99c846af5 100644 --- a/internal/datastore/common/sql.go +++ b/internal/datastore/common/sql.go @@ -73,6 +73,7 @@ type SchemaInformation struct { colCaveatName string colExpiration string paginationFilterType PaginationFilterType + nowFunction string } func NewSchemaInformation( @@ -85,6 +86,7 @@ func NewSchemaInformation( colCaveatName string, colExpiration string, paginationFilterType PaginationFilterType, + nowFunction string, ) SchemaInformation { return SchemaInformation{ colNamespace, @@ -96,6 +98,7 @@ func NewSchemaInformation( colCaveatName, colExpiration, paginationFilterType, + nowFunction, } } @@ -576,7 +579,7 @@ func (tqs QueryExecutor) ExecuteQuery( // Filter out any expired relationships. toExecute.queryBuilder = toExecute.queryBuilder.Where(sq.Or{ sq.Eq{query.schema.colExpiration: nil}, - sq.Expr(query.schema.colExpiration + " > NOW()"), + sq.Expr(query.schema.colExpiration + " > " + query.schema.nowFunction + "()"), }) // Run the query. diff --git a/internal/datastore/common/sql_test.go b/internal/datastore/common/sql_test.go index 0fd67f67eb..b7ade62d37 100644 --- a/internal/datastore/common/sql_test.go +++ b/internal/datastore/common/sql_test.go @@ -671,6 +671,7 @@ func TestSchemaQueryFilterer(t *testing.T) { "caveat", "expiration", TupleComparison, + "NOW", ) filterer := NewSchemaQueryFilterer(schema, base, 100) @@ -696,6 +697,7 @@ func BenchmarkSchemaFilterer(b *testing.B) { "caveat_name", "expiration", TupleComparison, + "NOW", ) sqf := NewSchemaQueryFilterer(si, sq.Select("*"), 100) var names []string diff --git a/internal/datastore/crdb/reader.go b/internal/datastore/crdb/reader.go index 4876c08b80..70002a8965 100644 --- a/internal/datastore/crdb/reader.go +++ b/internal/datastore/crdb/reader.go @@ -39,6 +39,7 @@ var ( colCaveatContextName, colExpiration, common.ExpandedLogicComparison, + "NOW", ) queryCounters = psql.Select( diff --git a/internal/datastore/mysql/reader.go b/internal/datastore/mysql/reader.go index 0cee5c466d..55eaaf4ea7 100644 --- a/internal/datastore/mysql/reader.go +++ b/internal/datastore/mysql/reader.go @@ -49,6 +49,7 @@ var schema = common.NewSchemaInformation( colCaveatName, colExpiration, common.ExpandedLogicComparison, + "NOW", ) func (mr *mysqlReader) CountRelationships(ctx context.Context, name string) (int, error) { diff --git a/internal/datastore/postgres/reader.go b/internal/datastore/postgres/reader.go index cdf76ce63f..14a1b99a0e 100644 --- a/internal/datastore/postgres/reader.go +++ b/internal/datastore/postgres/reader.go @@ -49,6 +49,7 @@ var ( colCaveatContextName, colExpiration, common.TupleComparison, + "NOW", ) readNamespace = psql. diff --git a/internal/datastore/spanner/migrations/zz_migration.0011_add_expiration_support.go b/internal/datastore/spanner/migrations/zz_migration.0011_add_expiration_support.go new file mode 100644 index 0000000000..2f7105ab8e --- /dev/null +++ b/internal/datastore/spanner/migrations/zz_migration.0011_add_expiration_support.go @@ -0,0 +1,35 @@ +package migrations + +import ( + "context" + + "cloud.google.com/go/spanner/admin/database/apiv1/databasepb" +) + +const ( + addExpirationColumn = ` + ALTER TABLE relation_tuple ADD COLUMN expires_at TIMESTAMP + ` + + addExpirationSupport = ` + ALTER TABLE relation_tuple ADD ROW DELETION POLICY (OLDER_THAN(expires_at, INTERVAL 1 DAY)) + ` +) + +func init() { + if err := SpannerMigrations.Register("add-expiration-support", "add-transaction-metadata-table", func(ctx context.Context, w Wrapper) error { + updateOp, err := w.adminClient.UpdateDatabaseDdl(ctx, &databasepb.UpdateDatabaseDdlRequest{ + Database: w.client.DatabaseName(), + Statements: []string{ + addExpirationColumn, + addExpirationSupport, + }, + }) + if err != nil { + return err + } + return updateOp.Wait(ctx) + }, nil); err != nil { + panic("failed to register migration: " + err.Error()) + } +} diff --git a/internal/datastore/spanner/reader.go b/internal/datastore/spanner/reader.go index 821de67035..c165dc7699 100644 --- a/internal/datastore/spanner/reader.go +++ b/internal/datastore/spanner/reader.go @@ -194,6 +194,7 @@ func queryExecutor(txSource txFactory) common.ExecuteQueryFunc { var subjectRelation string var caveatName spanner.NullString var caveatCtx spanner.NullJSON + var expirationOrNull spanner.NullTime err := row.Columns( &resourceObjectType, &resourceObjectID, @@ -203,6 +204,7 @@ func queryExecutor(txSource txFactory) common.ExecuteQueryFunc { &subjectRelation, &caveatName, &caveatCtx, + &expirationOrNull, ) if err != nil { return err @@ -214,6 +216,12 @@ func queryExecutor(txSource txFactory) common.ExecuteQueryFunc { } relCount++ + + var expiration *time.Time + if expirationOrNull.Valid { + expiration = &expirationOrNull.Time + } + if !yield(tuple.Relationship{ RelationshipReference: tuple.RelationshipReference{ Resource: tuple.ObjectAndRelation{ @@ -227,7 +235,8 @@ func queryExecutor(txSource txFactory) common.ExecuteQueryFunc { Relation: subjectRelation, }, }, - OptionalCaveat: caveat, + OptionalCaveat: caveat, + OptionalExpiration: expiration, }, nil) { return errStopIterator } @@ -355,6 +364,7 @@ var queryTuples = sql.Select( colUsersetRelation, colCaveatName, colCaveatContext, + colExpiration, ).From(tableRelationship) var countRels = sql.Select("COUNT(*)").From(tableRelationship) @@ -376,7 +386,9 @@ var schema = common.NewSchemaInformation( colUsersetObjectID, colUsersetRelation, colCaveatName, + colExpiration, common.ExpandedLogicComparison, + "CURRENT_TIMESTAMP", ) var _ datastore.Reader = spannerReader{} diff --git a/internal/datastore/spanner/readwrite.go b/internal/datastore/spanner/readwrite.go index afc0b2c510..74411a0829 100644 --- a/internal/datastore/spanner/readwrite.go +++ b/internal/datastore/spanner/readwrite.go @@ -320,6 +320,12 @@ func upsertVals(r tuple.Relationship) []any { key := keyFromRelationship(r) key = append(key, spanner.CommitTimestamp) key = append(key, caveatVals(r)...) + + if r.OptionalExpiration != nil { + key = append(key, spanner.NullTime{Time: *r.OptionalExpiration, Valid: true}) + } else { + key = append(key, nil) + } return key } diff --git a/internal/datastore/spanner/schema.go b/internal/datastore/spanner/schema.go index 3cde345a2a..8b5dabc39b 100644 --- a/internal/datastore/spanner/schema.go +++ b/internal/datastore/spanner/schema.go @@ -16,6 +16,7 @@ const ( colTimestamp = "timestamp" colCaveatName = "caveat_name" colCaveatContext = "caveat_context" + colExpiration = "expires_at" tableCaveat = "caveat" colName = "name" @@ -46,4 +47,5 @@ var allRelationshipCols = []string{ colTimestamp, colCaveatName, colCaveatContext, + colExpiration, } diff --git a/internal/services/shared/schema.go b/internal/services/shared/schema.go index b2f58d08a5..2befaf69c0 100644 --- a/internal/services/shared/schema.go +++ b/internal/services/shared/schema.go @@ -388,7 +388,6 @@ func sanityCheckNamespaceChanges( var optionalSubjectIds []string var relationFilter datastore.SubjectRelationFilter optionalCaveatName := "" - expirationOption := datastore.ExpirationFilterOptionNoExpiration if delta.AllowedType.GetPublicWildcard() != nil { optionalSubjectIds = []string{tuple.PublicWildcard} @@ -402,10 +401,9 @@ func sanityCheckNamespaceChanges( optionalCaveatName = delta.AllowedType.GetRequiredCaveat().CaveatName } + expirationOption := datastore.ExpirationFilterOptionNoExpiration if delta.AllowedType.RequiredExpiration != nil { expirationOption = datastore.ExpirationFilterOptionHasExpiration - } else { - expirationOption = datastore.ExpirationFilterOptionNoExpiration } qyr, qyrErr := rwt.QueryRelationships( diff --git a/pkg/datastore/test/relationships.go b/pkg/datastore/test/relationships.go index a2b77056b4..4f1784f6ed 100644 --- a/pkg/datastore/test/relationships.go +++ b/pkg/datastore/test/relationships.go @@ -1673,10 +1673,7 @@ func ConcurrentWriteSerializationTest(t *testing.T, tester DatastoreTester) { <-waitToFinish return err }) - if err != nil { - panic(err) - } - return nil + return err }) <-waitToStart From 2fe0bb5d7a39f187f347fed09ef7e0e6afcfd4a4 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Mon, 2 Dec 2024 17:35:23 -0500 Subject: [PATCH 14/39] Add watch test for relationship with expiration --- pkg/datastore/test/datastore.go | 1 + pkg/datastore/test/watch.go | 36 +++++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/pkg/datastore/test/datastore.go b/pkg/datastore/test/datastore.go index 741b4b8bd7..6e4bcac7bc 100644 --- a/pkg/datastore/test/datastore.go +++ b/pkg/datastore/test/datastore.go @@ -181,6 +181,7 @@ func AllWithExceptions(t *testing.T, tester DatastoreTester, except Categories, t.Run("TestWatchWithTouch", runner(tester, WatchWithTouchTest)) t.Run("TestWatchWithDelete", runner(tester, WatchWithDeleteTest)) t.Run("TestWatchWithMetadata", runner(tester, WatchWithMetadataTest)) + t.Run("TestWatchWithExpiration", runner(tester, WatchWithExpirationTest)) t.Run("TestWatchEmissionStrategy", runner(tester, WatchEmissionStrategyTest)) } diff --git a/pkg/datastore/test/watch.go b/pkg/datastore/test/watch.go index 40b9e7aa58..f8b0bc37b2 100644 --- a/pkg/datastore/test/watch.go +++ b/pkg/datastore/test/watch.go @@ -384,6 +384,42 @@ func WatchWithTouchTest(t *testing.T, tester DatastoreTester) { ) } +func WatchWithExpirationTest(t *testing.T, tester DatastoreTester) { + require := require.New(t) + + ds, err := tester.New(0, veryLargeGCInterval, veryLargeGCWindow, 16) + require.NoError(err) + + setupDatastore(ds, require) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + lowestRevision, err := ds.HeadRevision(ctx) + require.NoError(err) + + changes, errchan := ds.Watch(ctx, lowestRevision, datastore.WatchJustRelationships()) + require.Zero(len(errchan)) + + metadata, err := structpb.NewStruct(map[string]any{"somekey": "somevalue"}) + require.NoError(err) + + _, err = ds.ReadWriteTx(ctx, func(ctx context.Context, rwt datastore.ReadWriteTransaction) error { + return rwt.WriteRelationships(ctx, []tuple.RelationshipUpdate{ + tuple.Create(tuple.MustParse("document:firstdoc#viewer@user:tom[expiration:2321-01-01T00:00:00Z]")), + }) + }, options.WithMetadata(metadata)) + require.NoError(err) + + VerifyUpdates(require, [][]tuple.RelationshipUpdate{ + {tuple.Touch(tuple.MustParse("document:firstdoc#viewer@user:tom[expiration:2321-01-01T00:00:00Z]"))}, + }, + changes, + errchan, + false, + ) +} + type updateWithMetadata struct { updates []tuple.RelationshipUpdate metadata map[string]any From 4518735b6d89472d059c5cb71ab2f6fb289eb079 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Mon, 2 Dec 2024 17:45:32 -0500 Subject: [PATCH 15/39] Add bulk upload with expiration test --- internal/datastore/postgres/common/bulk.go | 2 +- internal/datastore/postgres/readwrite.go | 1 + internal/testfixtures/generator.go | 20 ++++++++--- pkg/datastore/test/bulk.go | 39 ++++++++++++++++++++++ pkg/datastore/test/datastore.go | 1 + 5 files changed, 57 insertions(+), 6 deletions(-) diff --git a/internal/datastore/postgres/common/bulk.go b/internal/datastore/postgres/common/bulk.go index 8690e916bc..4673432a8e 100644 --- a/internal/datastore/postgres/common/bulk.go +++ b/internal/datastore/postgres/common/bulk.go @@ -48,7 +48,7 @@ func (tg *tupleSourceAdapter) Values() ([]any, error) { tg.valuesBuffer[7] = caveatContext tg.valuesBuffer[8] = tg.current.OptionalExpiration - if len(tg.colNames) > 8 && tg.current.OptionalIntegrity != nil { + if len(tg.colNames) > 9 && tg.current.OptionalIntegrity != nil { tg.valuesBuffer[9] = tg.current.OptionalIntegrity.KeyId tg.valuesBuffer[10] = tg.current.OptionalIntegrity.Hash tg.valuesBuffer[11] = tg.current.OptionalIntegrity.HashedAt.AsTime() diff --git a/internal/datastore/postgres/readwrite.go b/internal/datastore/postgres/readwrite.go index 7e8b58f2c1..6bc3ead8c4 100644 --- a/internal/datastore/postgres/readwrite.go +++ b/internal/datastore/postgres/readwrite.go @@ -741,6 +741,7 @@ var copyCols = []string{ colUsersetRelation, colCaveatContextName, colCaveatContext, + colExpiration, } func (rwt *pgReadWriteTXN) BulkLoad(ctx context.Context, iter datastore.BulkWriteRelationshipSource) (uint64, error) { diff --git a/internal/testfixtures/generator.go b/internal/testfixtures/generator.go index 4b06938c3d..4e1f578558 100644 --- a/internal/testfixtures/generator.go +++ b/internal/testfixtures/generator.go @@ -5,6 +5,7 @@ import ( "math/rand" "strconv" "testing" + "time" "github.com/authzed/spicedb/pkg/datastore" "github.com/authzed/spicedb/pkg/tuple" @@ -37,15 +38,17 @@ func NewBulkRelationshipGenerator(objectType, relation, subjectType string, coun objectType, relation, subjectType, + false, } } type BulkRelationshipGenerator struct { - remaining int - t *testing.T - objectType string - relation string - subjectType string + remaining int + t *testing.T + objectType string + relation string + subjectType string + WithExpiration bool } func (btg *BulkRelationshipGenerator) Next(_ context.Context) (*tuple.Relationship, error) { @@ -54,6 +57,12 @@ func (btg *BulkRelationshipGenerator) Next(_ context.Context) (*tuple.Relationsh } btg.remaining-- + var expiration *time.Time + if btg.WithExpiration { + exp := time.Now().Add(24 * time.Hour) + expiration = &exp + } + return &tuple.Relationship{ RelationshipReference: tuple.RelationshipReference{ Resource: tuple.ObjectAndRelation{ @@ -67,6 +76,7 @@ func (btg *BulkRelationshipGenerator) Next(_ context.Context) (*tuple.Relationsh Relation: datastore.Ellipsis, }, }, + OptionalExpiration: expiration, }, nil } diff --git a/pkg/datastore/test/bulk.go b/pkg/datastore/test/bulk.go index 151be63337..c9500e5cf9 100644 --- a/pkg/datastore/test/bulk.go +++ b/pkg/datastore/test/bulk.go @@ -122,6 +122,45 @@ func BulkUploadAlreadyExistsSameCallErrorTest(t *testing.T, tester DatastoreTest grpcutil.RequireStatus(t, codes.AlreadyExists, err) } +func BulkUploadWithExpiration(t *testing.T, tester DatastoreTester) { + tc := 10 + require := require.New(t) + ctx := context.Background() + + rawDS, err := tester.New(0, veryLargeGCInterval, veryLargeGCWindow, 1) + require.NoError(err) + + ds, _ := testfixtures.StandardDatastoreWithSchema(rawDS, require) + bulkSource := testfixtures.NewBulkRelationshipGenerator( + testfixtures.DocumentNS.Name, + "expired_viewer", + testfixtures.UserNS.Name, + tc, + t, + ) + bulkSource.WithExpiration = true + + // This is statically defined so we can cast straight. + uintTc, _ := safecast.ToUint64(tc) + lastRevision, err := ds.ReadWriteTx(ctx, func(ctx context.Context, rwt datastore.ReadWriteTransaction) error { + loaded, err := rwt.BulkLoad(ctx, bulkSource) + require.NoError(err) + require.Equal(uintTc, loaded) + return err + }) + require.NoError(err) + + iter, err := ds.SnapshotReader(lastRevision).QueryRelationships(ctx, datastore.RelationshipsFilter{ + OptionalResourceType: testfixtures.DocumentNS.Name, + }) + require.NoError(err) + + for found, err := range iter { + require.NoError(err) + require.NotNil(found.OptionalExpiration) + } +} + func BulkUploadEditCaveat(t *testing.T, tester DatastoreTester) { tc := 10 require := require.New(t) diff --git a/pkg/datastore/test/datastore.go b/pkg/datastore/test/datastore.go index 6e4bcac7bc..d6421b5f35 100644 --- a/pkg/datastore/test/datastore.go +++ b/pkg/datastore/test/datastore.go @@ -161,6 +161,7 @@ func AllWithExceptions(t *testing.T, tester DatastoreTester, except Categories, t.Run("TestBulkUploadAlreadyExistsError", runner(tester, BulkUploadAlreadyExistsErrorTest)) t.Run("TestBulkUploadAlreadyExistsSameCallError", runner(tester, BulkUploadAlreadyExistsSameCallErrorTest)) t.Run("BulkUploadEditCaveat", runner(tester, BulkUploadEditCaveat)) + t.Run("BulkUploadWithExpiration", runner(tester, BulkUploadWithExpiration)) if !except.Stats() { t.Run("TestStats", runner(tester, StatsTest)) From d53a4df96f1cf9e131d23e597758dad15fda0fc0 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Tue, 3 Dec 2024 12:16:16 -0500 Subject: [PATCH 16/39] Add watch filtering for TTL deletes in Spanner --- internal/datastore/spanner/watch.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/internal/datastore/spanner/watch.go b/internal/datastore/spanner/watch.go index 0cee6df022..c312430c99 100644 --- a/internal/datastore/spanner/watch.go +++ b/internal/datastore/spanner/watch.go @@ -189,6 +189,18 @@ func (sd *spannerDatastore) watch( changeRevision := revisions.NewForTime(dcr.CommitTimestamp) modType := dcr.ModType // options are INSERT, UPDATE, DELETE + // See: https://cloud.google.com/spanner/docs/ttl + // > TTL supports auditing its deletions through change streams. Change + // > streams data records that track TTL changes to a database have the + // > transaction_tag field set to RowDeletionPolicy and the + // > is_system_transaction field set to true. + if modType == "DELETE" && dcr.TransactionTag == "RowDeletionPolicy" && dcr.IsSystemTransaction { + // Skip deletions that are performed by TTL policy. + // TODO(jschorr): once we decide to emit events for GCed expired rels, change to emit those + // events instead. + continue + } + if len(dcr.TransactionTag) > 0 { if err := addMetadataForTransactionTag(ctx, tracked, changeRevision, dcr.TransactionTag); err != nil { return err From ebc23358b6bdbe744ac59890c370e28219659fde Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Tue, 3 Dec 2024 12:53:44 -0500 Subject: [PATCH 17/39] Add filtering for TTL deletes in CRDB --- internal/datastore/crdb/crdb.go | 40 ++++++++++++++++------------ internal/datastore/crdb/watch.go | 45 +++++++++++++++++++++++++++++++- 2 files changed, 67 insertions(+), 18 deletions(-) diff --git a/internal/datastore/crdb/crdb.go b/internal/datastore/crdb/crdb.go index 498c8ebd82..76ff5c8dda 100644 --- a/internal/datastore/crdb/crdb.go +++ b/internal/datastore/crdb/crdb.go @@ -91,6 +91,8 @@ const ( queryTransactionNowPreV23 = querySelectNow queryTransactionNow = "SHOW COMMIT TIMESTAMP" queryShowZoneConfig = "SHOW ZONE CONFIGURATION FOR RANGE default;" + + spicedbTransactionKey = "$spicedb_transaction_key" ) var livingTupleConstraints = []string{"pk_relation_tuple"} @@ -345,21 +347,25 @@ func (cds *crdbDatastore) ReadWriteTx( Executor: pgxcommon.NewPGXExecutorWithIntegrityOption(querier, cds.supportsIntegrity), } - // If metadata is to be attached, write that row now. - if config.Metadata != nil && len(config.Metadata.GetFields()) > 0 { - expiresAt := time.Now().Add(cds.gcWindow).Add(1 * time.Minute) - insertTransactionMetadata := psql.Insert(tableTransactionMetadata). - Columns(colExpiresAt, colMetadata). - Values(expiresAt, config.Metadata.AsMap()) + // Write metadata onto the transaction. + metadata := config.Metadata.AsMap() - sql, args, err := insertTransactionMetadata.ToSql() - if err != nil { - return fmt.Errorf("error building metadata insert: %w", err) - } + // Mark the transaction as coming from SpiceDB. See the comment in watch.go + // for why this is necessary. + metadata[spicedbTransactionKey] = true - if _, err := tx.Exec(ctx, sql, args...); err != nil { - return fmt.Errorf("error writing metadata: %w", err) - } + expiresAt := time.Now().Add(cds.gcWindow).Add(1 * time.Minute) + insertTransactionMetadata := psql.Insert(tableTransactionMetadata). + Columns(colExpiresAt, colMetadata). + Values(expiresAt, config.Metadata.AsMap()) + + sql, args, err := insertTransactionMetadata.ToSql() + if err != nil { + return fmt.Errorf("error building metadata insert: %w", err) + } + + if _, err := tx.Exec(ctx, sql, args...); err != nil { + return fmt.Errorf("error writing metadata: %w", err) } rwt := &crdbReadWriteTXN{ @@ -391,10 +397,10 @@ func (cds *crdbDatastore) ReadWriteTx( } } - var err error - commitTimestamp, err = cds.readTransactionCommitRev(ctx, querier) - if err != nil { - return fmt.Errorf("error getting commit timestamp: %w", err) + var cerr error + commitTimestamp, cerr = cds.readTransactionCommitRev(ctx, querier) + if cerr != nil { + return fmt.Errorf("error getting commit timestamp: %w", cerr) } return nil }) diff --git a/internal/datastore/crdb/watch.go b/internal/datastore/crdb/watch.go index e000038e93..da6da149ad 100644 --- a/internal/datastore/crdb/watch.go +++ b/internal/datastore/crdb/watch.go @@ -361,6 +361,47 @@ func (cds *crdbDatastore) processChanges(ctx context.Context, changes pgx.Rows, for _, revChange := range filtered { revChange := revChange + + // TODO(jschorr): Change this to a new event type if/when we decide to report these + // row GCs. + + // Filter out any DELETEs that occurred due to a relationship being expired and CRDB + // performing GC of the row. + // As CRDB does not (currently) provide a means for differentiating rows deleted by a user + // and rows deleted by the system, we use a set of heuristics to determine when to filter: + // + // 1) Rows deleted by the TTL cleanup in CRDB will not have any metadata associated with the + // transaction. + // 2) No other operations besides DELETE will be performed by the TTL cleanup. + // 3) Only filter rows that had an expires_at that are before the current time. + if _, ok := revChange.Metadata.AsMap()[spicedbTransactionKey]; !ok { + if len(revChange.ChangedDefinitions) == 0 && len(revChange.DeletedNamespaces) == 0 && + len(revChange.DeletedCaveats) == 0 { + hasNonTTLEvent := false + for _, relChange := range revChange.RelationshipChanges { + if relChange.Operation != tuple.UpdateOperationDelete { + hasNonTTLEvent = true + break + } + + if relChange.Relationship.OptionalExpiration == nil { + hasNonTTLEvent = true + break + } + + if relChange.Relationship.OptionalExpiration.After(time.Now()) { + hasNonTTLEvent = true + break + } + } + + if !hasNonTTLEvent { + // Skip this event. + continue + } + } + } + if err := sendChange(&revChange); err != nil { sendError(err) return @@ -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 + delete(adjustedMetadata, spicedbTransactionKey) + if err := tracked.SetRevisionMetadata(ctx, rev, adjustedMetadata); err != nil { sendError(err) return } From ab2a502f2294512fe0d00b096ce7bea31ea4def7 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Tue, 3 Dec 2024 13:29:54 -0500 Subject: [PATCH 18/39] Add expired relationship GC to PG and MySQL drivers --- internal/datastore/common/gc.go | 16 ++++++++++++++++ internal/datastore/common/gc_test.go | 25 ++++++++++++++++++++++--- internal/datastore/mysql/gc.go | 8 ++++++++ internal/datastore/postgres/gc.go | 9 +++++++++ 4 files changed, 55 insertions(+), 3 deletions(-) diff --git a/internal/datastore/common/gc.go b/internal/datastore/common/gc.go index f6ba7fc4c9..d3c90647ab 100644 --- a/internal/datastore/common/gc.go +++ b/internal/datastore/common/gc.go @@ -29,6 +29,13 @@ var ( Help: "The number of stale relationships deleted by the datastore garbage collection.", }) + gcExpiredRelationshipsCounter = prometheus.NewCounter(prometheus.CounterOpts{ + Namespace: "spicedb", + Subsystem: "datastore", + Name: "gc_expired_relationships_total", + Help: "The number of expired relationships deleted by the datastore garbage collection.", + }) + gcTransactionsCounter = prometheus.NewCounter(prometheus.CounterOpts{ Namespace: "spicedb", Subsystem: "datastore", @@ -81,6 +88,7 @@ type GarbageCollector interface { Now(context.Context) (time.Time, error) TxIDBefore(context.Context, time.Time) (datastore.Revision, error) DeleteBeforeTx(ctx context.Context, txID datastore.Revision) (DeletionCounts, error) + DeleteExpiredRels(ctx context.Context) (int64, error) } // DeletionCounts tracks the amount of deletions that occurred when calling @@ -185,11 +193,14 @@ func RunGarbageCollection(gc GarbageCollector, window, timeout time.Duration) er collected, err := gc.DeleteBeforeTx(ctx, watermark) + expiredRelationshipsCount, eerr := gc.DeleteExpiredRels(ctx) + // 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)) collectionDuration := time.Since(startTime) gcDurationHistogram.Observe(collectionDuration.Seconds()) @@ -197,11 +208,16 @@ func RunGarbageCollection(gc GarbageCollector, window, timeout time.Duration) er return fmt.Errorf("error deleting in gc: %w", err) } + if eerr != nil { + return fmt.Errorf("error deleting expired relationships in gc: %w", eerr) + } + log.Ctx(ctx).Info(). Stringer("highestTxID", watermark). Dur("duration", collectionDuration). Time("nowTime", now). Interface("collected", collected). + Int64("expiredRelationships", expiredRelationshipsCount). Msg("datastore garbage collection completed successfully") gc.MarkGCCompleted() diff --git a/internal/datastore/common/gc_test.go b/internal/datastore/common/gc_test.go index da8a09202f..aa42bc9062 100644 --- a/internal/datastore/common/gc_test.go +++ b/internal/datastore/common/gc_test.go @@ -26,9 +26,10 @@ type fakeGC struct { } type gcMetrics struct { - deleteBeforeTxCount int - markedCompleteCount int - resetGCCompletedCount int + deleteBeforeTxCount int + markedCompleteCount int + resetGCCompletedCount int + deleteExpiredRelsCount int } func newFakeGC(deleter gcDeleter) fakeGC { @@ -71,6 +72,15 @@ func (gc *fakeGC) DeleteBeforeTx(_ context.Context, rev datastore.Revision) (Del return gc.deleter.DeleteBeforeTx(revInt) } +func (gc *fakeGC) DeleteExpiredRels(_ context.Context) (int64, error) { + gc.lock.Lock() + defer gc.lock.Unlock() + + gc.metrics.deleteExpiredRelsCount++ + + return gc.deleter.DeleteExpiredRels() +} + func (gc *fakeGC) HasGCRun() bool { gc.lock.Lock() defer gc.lock.Unlock() @@ -102,6 +112,7 @@ func (gc *fakeGC) GetMetrics() gcMetrics { // Allows specifying different deletion behaviors for tests type gcDeleter interface { DeleteBeforeTx(revision uint64) (DeletionCounts, error) + DeleteExpiredRels() (int64, error) } // Always error trying to perform a delete @@ -111,6 +122,10 @@ func (alwaysErrorDeleter) DeleteBeforeTx(_ uint64) (DeletionCounts, error) { return DeletionCounts{}, fmt.Errorf("delete error") } +func (alwaysErrorDeleter) DeleteExpiredRels() (int64, error) { + return 0, fmt.Errorf("delete error") +} + // Only error on specific revisions type revisionErrorDeleter struct { errorOnRevisions []uint64 @@ -124,6 +139,10 @@ func (d revisionErrorDeleter) DeleteBeforeTx(revision uint64) (DeletionCounts, e return DeletionCounts{}, nil } +func (d revisionErrorDeleter) DeleteExpiredRels() (int64, error) { + return 0, nil +} + func TestGCFailureBackoff(t *testing.T) { localCounter := prometheus.NewCounter(gcFailureCounterConfig) reg := prometheus.NewRegistry() diff --git a/internal/datastore/mysql/gc.go b/internal/datastore/mysql/gc.go index bde07fe40a..88346139c4 100644 --- a/internal/datastore/mysql/gc.go +++ b/internal/datastore/mysql/gc.go @@ -99,6 +99,14 @@ func (mds *Datastore) DeleteBeforeTx( return } +func (mds *Datastore) DeleteExpiredRels(ctx context.Context) (int64, error) { + return mds.batchDelete( + ctx, + mds.driver.RelationTuple(), + sq.Lt{colExpiration: time.Now().Add(-1 * mds.gcWindow)}, + ) +} + // - query was reworked to make it compatible with Vitess // - API differences with PSQL driver func (mds *Datastore) batchDelete(ctx context.Context, tableName string, filter sqlFilter) (int64, error) { diff --git a/internal/datastore/postgres/gc.go b/internal/datastore/postgres/gc.go index 873e3e077f..9c68c59a7a 100644 --- a/internal/datastore/postgres/gc.go +++ b/internal/datastore/postgres/gc.go @@ -68,6 +68,15 @@ func (pgd *pgDatastore) TxIDBefore(ctx context.Context, before time.Time) (datas return postgresRevision{snapshot: snapshot, optionalTxID: value}, nil } +func (pgd *pgDatastore) DeleteExpiredRels(ctx context.Context) (int64, error) { + return pgd.batchDelete( + ctx, + tableTuple, + gcPKCols, + sq.Lt{colExpiration: time.Now().Add(-1 * pgd.gcWindow)}, + ) +} + func (pgd *pgDatastore) DeleteBeforeTx(ctx context.Context, txID datastore.Revision) (common.DeletionCounts, error) { revision := txID.(postgresRevision) From 7701ec912fc34f3c5956fad1057adc5475952481 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Tue, 3 Dec 2024 13:59:59 -0500 Subject: [PATCH 19/39] Add experimental flag for enabling rel expiration --- internal/services/server.go | 2 +- internal/services/v1/relationships.go | 11 +++++++++++ internal/services/v1/schema.go | 15 +++++++++++---- internal/testserver/server.go | 1 + pkg/cmd/serve.go | 1 + pkg/cmd/server/server.go | 26 ++++++++++++++------------ pkg/cmd/server/zz_generated.options.go | 9 +++++++++ pkg/cmd/testserver/testserver.go | 1 + pkg/development/devcontext.go | 11 ++++++----- pkg/schemadsl/compiler/compiler.go | 2 +- 10 files changed, 56 insertions(+), 23 deletions(-) diff --git a/internal/services/server.go b/internal/services/server.go index c58ce3a4e7..b53177327b 100644 --- a/internal/services/server.go +++ b/internal/services/server.go @@ -68,7 +68,7 @@ func RegisterGrpcServices( } if schemaServiceOption == V1SchemaServiceEnabled || schemaServiceOption == V1SchemaServiceAdditiveOnly { - v1.RegisterSchemaServiceServer(srv, v1svc.NewSchemaServer(schemaServiceOption == V1SchemaServiceAdditiveOnly)) + v1.RegisterSchemaServiceServer(srv, v1svc.NewSchemaServer(schemaServiceOption == V1SchemaServiceAdditiveOnly, permSysConfig.ExpiringRelationshipsEnabled)) healthManager.RegisterReportedService(v1.SchemaService_ServiceDesc.ServiceName) } diff --git a/internal/services/v1/relationships.go b/internal/services/v1/relationships.go index 191191e917..c5fb52f354 100644 --- a/internal/services/v1/relationships.go +++ b/internal/services/v1/relationships.go @@ -95,6 +95,9 @@ type PermissionsServerConfig struct { // MaxBulkExportRelationshipsLimit defines the maximum number of relationships that can be // exported in a single BulkExportRelationships call. MaxBulkExportRelationshipsLimit uint32 + + // ExpiringRelationshipsEnabled defines whether or not expiring relationships are enabled. + ExpiringRelationshipsEnabled bool } // NewPermissionsServer creates a PermissionsServiceServer instance. @@ -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, } return &permissionServer{ @@ -309,6 +313,13 @@ func (ps *permissionServer) WriteRelationships(ctx context.Context, req *v1.Writ NewMaxRelationshipContextError(update, ps.config.MaxRelationshipContextSize), ) } + + if !ps.config.ExpiringRelationshipsEnabled && update.Relationship.OptionalExpiresAt != nil { + return nil, ps.rewriteError( + ctx, + fmt.Errorf("support for expiring relationships is not enabled"), + ) + } } // Execute the write operation(s). diff --git a/internal/services/v1/schema.go b/internal/services/v1/schema.go index 25cfc72ba9..5f11849042 100644 --- a/internal/services/v1/schema.go +++ b/internal/services/v1/schema.go @@ -23,7 +23,7 @@ import ( ) // NewSchemaServer creates a SchemaServiceServer instance. -func NewSchemaServer(additiveOnly bool) v1.SchemaServiceServer { +func NewSchemaServer(additiveOnly bool, expiringRelsEnabled bool) v1.SchemaServiceServer { return &schemaServer{ WithServiceSpecificInterceptors: shared.WithServiceSpecificInterceptors{ Unary: middleware.ChainUnaryServer( @@ -35,7 +35,8 @@ func NewSchemaServer(additiveOnly bool) v1.SchemaServiceServer { usagemetrics.StreamServerInterceptor(), ), }, - additiveOnly: additiveOnly, + additiveOnly: additiveOnly, + expiringRelsEnabled: expiringRelsEnabled, } } @@ -43,7 +44,8 @@ type schemaServer struct { v1.UnimplementedSchemaServiceServer shared.WithServiceSpecificInterceptors - additiveOnly bool + additiveOnly bool + expiringRelsEnabled bool } func (ss *schemaServer) rewriteError(ctx context.Context, err error) error { @@ -109,10 +111,15 @@ func (ss *schemaServer) WriteSchema(ctx context.Context, in *v1.WriteSchemaReque ds := datastoremw.MustFromContext(ctx) // Compile the schema into the namespace definitions. + opts := []compiler.Option{} + if !ss.expiringRelsEnabled { + opts = append(opts, compiler.DisallowExpirationFlag()) + } + compiled, err := compiler.Compile(compiler.InputSchema{ Source: input.Source("schema"), SchemaString: in.GetSchema(), - }, compiler.AllowUnprefixedObjectType()) + }, compiler.AllowUnprefixedObjectType(), opts...) if err != nil { return nil, ss.rewriteError(ctx, err) } diff --git a/internal/testserver/server.go b/internal/testserver/server.go index 2599c74cca..1b8fd6d991 100644 --- a/internal/testserver/server.go +++ b/internal/testserver/server.go @@ -71,6 +71,7 @@ func NewTestServerWithConfigAndDatastore(require *require.Assertions, ds, revision := dsInitFunc(emptyDS, require) ctx, cancel := context.WithCancel(context.Background()) srv, err := server.NewConfigWithOptionsAndDefaults( + server.WithEnableExperimentalRelationshipExpiration(true), server.WithDatastore(ds), server.WithDispatcher(graph.NewLocalOnlyDispatcher(10, 100)), server.WithDispatchMaxDepth(50), diff --git a/pkg/cmd/serve.go b/pkg/cmd/serve.go index 7f91cb4a2e..97d5a613cb 100644 --- a/pkg/cmd/serve.go +++ b/pkg/cmd/serve.go @@ -169,6 +169,7 @@ func RegisterServeFlags(cmd *cobra.Command, config *server.Config) error { Msg("The old implementation of LookupResources is no longer available, and a `false` value is no longer valid. Please remove this flag.") } + experimentalFlags.BoolVar(&config.EnableExperimentalRelationshipExpiration, "enable-experimental-relationship-expiration", false, "enables experimental support for first-class relationship expiration") experimentalFlags.BoolVar(&config.EnableExperimentalWatchableSchemaCache, "enable-experimental-watchable-schema-cache", false, "enables the experimental schema cache which makes use of the Watch API for automatic updates") // TODO: these two could reasonably be put in either the Dispatch group or the Experimental group. Is there a preference? experimentalFlags.StringToStringVar(&config.DispatchSecondaryUpstreamAddrs, "experimental-dispatch-secondary-upstream-addrs", nil, "secondary upstream addresses for dispatches, each with a name") diff --git a/pkg/cmd/server/server.go b/pkg/cmd/server/server.go index def099b86a..5f2ca4a37b 100644 --- a/pkg/cmd/server/server.go +++ b/pkg/cmd/server/server.go @@ -108,18 +108,19 @@ type Config struct { ClusterDispatchCacheConfig CacheConfig `debugmap:"visible"` // API Behavior - DisableV1SchemaAPI bool `debugmap:"visible"` - V1SchemaAdditiveOnly bool `debugmap:"visible"` - MaximumUpdatesPerWrite uint16 `debugmap:"visible"` - MaximumPreconditionCount uint16 `debugmap:"visible"` - MaxDatastoreReadPageSize uint64 `debugmap:"visible"` - StreamingAPITimeout time.Duration `debugmap:"visible"` - WatchHeartbeat time.Duration `debugmap:"visible"` - MaxReadRelationshipsLimit uint32 `debugmap:"visible"` - MaxDeleteRelationshipsLimit uint32 `debugmap:"visible"` - MaxLookupResourcesLimit uint32 `debugmap:"visible"` - MaxBulkExportRelationshipsLimit uint32 `debugmap:"visible"` - EnableExperimentalLookupResources bool `debugmap:"visible"` + DisableV1SchemaAPI bool `debugmap:"visible"` + V1SchemaAdditiveOnly bool `debugmap:"visible"` + MaximumUpdatesPerWrite uint16 `debugmap:"visible"` + MaximumPreconditionCount uint16 `debugmap:"visible"` + MaxDatastoreReadPageSize uint64 `debugmap:"visible"` + StreamingAPITimeout time.Duration `debugmap:"visible"` + WatchHeartbeat time.Duration `debugmap:"visible"` + MaxReadRelationshipsLimit uint32 `debugmap:"visible"` + MaxDeleteRelationshipsLimit uint32 `debugmap:"visible"` + MaxLookupResourcesLimit uint32 `debugmap:"visible"` + MaxBulkExportRelationshipsLimit uint32 `debugmap:"visible"` + EnableExperimentalLookupResources bool `debugmap:"visible"` + EnableExperimentalRelationshipExpiration bool `debugmap:"visible"` // Additional Services MetricsAPI util.HTTPServerConfig `debugmap:"visible"` @@ -437,6 +438,7 @@ func (c *Config) Complete(ctx context.Context) (RunnableServer, error) { MaxLookupResourcesLimit: c.MaxLookupResourcesLimit, MaxBulkExportRelationshipsLimit: c.MaxBulkExportRelationshipsLimit, DispatchChunkSize: c.DispatchChunkSize, + ExpiringRelationshipsEnabled: c.EnableExperimentalRelationshipExpiration, } healthManager := health.NewHealthManager(dispatcher, ds) diff --git a/pkg/cmd/server/zz_generated.options.go b/pkg/cmd/server/zz_generated.options.go index 5e4e8f3991..8e2dfaddef 100644 --- a/pkg/cmd/server/zz_generated.options.go +++ b/pkg/cmd/server/zz_generated.options.go @@ -88,6 +88,7 @@ func (c *Config) ToOption() ConfigOption { to.MaxLookupResourcesLimit = c.MaxLookupResourcesLimit to.MaxBulkExportRelationshipsLimit = c.MaxBulkExportRelationshipsLimit to.EnableExperimentalLookupResources = c.EnableExperimentalLookupResources + to.EnableExperimentalRelationshipExpiration = c.EnableExperimentalRelationshipExpiration to.MetricsAPI = c.MetricsAPI to.UnaryMiddlewareModification = c.UnaryMiddlewareModification to.StreamingMiddlewareModification = c.StreamingMiddlewareModification @@ -156,6 +157,7 @@ func (c Config) DebugMap() map[string]any { debugMap["MaxLookupResourcesLimit"] = helpers.DebugValue(c.MaxLookupResourcesLimit, false) debugMap["MaxBulkExportRelationshipsLimit"] = helpers.DebugValue(c.MaxBulkExportRelationshipsLimit, false) debugMap["EnableExperimentalLookupResources"] = helpers.DebugValue(c.EnableExperimentalLookupResources, false) + debugMap["EnableExperimentalRelationshipExpiration"] = helpers.DebugValue(c.EnableExperimentalRelationshipExpiration, false) debugMap["MetricsAPI"] = helpers.DebugValue(c.MetricsAPI, false) debugMap["SilentlyDisableTelemetry"] = helpers.DebugValue(c.SilentlyDisableTelemetry, false) debugMap["TelemetryCAOverridePath"] = helpers.DebugValue(c.TelemetryCAOverridePath, false) @@ -561,6 +563,13 @@ func WithEnableExperimentalLookupResources(enableExperimentalLookupResources boo } } +// WithEnableExperimentalRelationshipExpiration returns an option that can set EnableExperimentalRelationshipExpiration on a Config +func WithEnableExperimentalRelationshipExpiration(enableExperimentalRelationshipExpiration bool) ConfigOption { + return func(c *Config) { + c.EnableExperimentalRelationshipExpiration = enableExperimentalRelationshipExpiration + } +} + // WithMetricsAPI returns an option that can set MetricsAPI on a Config func WithMetricsAPI(metricsAPI util.HTTPServerConfig) ConfigOption { return func(c *Config) { diff --git a/pkg/cmd/testserver/testserver.go b/pkg/cmd/testserver/testserver.go index 1af2b14f5f..a80b396c0c 100644 --- a/pkg/cmd/testserver/testserver.go +++ b/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, }, 1*time.Second, ) diff --git a/pkg/development/devcontext.go b/pkg/development/devcontext.go index f15357f917..59738b0551 100644 --- a/pkg/development/devcontext.go +++ b/pkg/development/devcontext.go @@ -166,12 +166,13 @@ func (dc *DevContext) RunV1InMemoryService() (*grpc.ClientConn, func(), error) { ), ) ps := v1svc.NewPermissionsServer(dc.Dispatcher, v1svc.PermissionsServerConfig{ - MaxUpdatesPerWrite: 50, - MaxPreconditionsCount: 50, - MaximumAPIDepth: 50, - MaxCaveatContextSize: 0, + MaxUpdatesPerWrite: 50, + MaxPreconditionsCount: 50, + MaximumAPIDepth: 50, + MaxCaveatContextSize: 0, + ExpiringRelationshipsEnabled: true, }) - ss := v1svc.NewSchemaServer(false) + ss := v1svc.NewSchemaServer(false, true) v1.RegisterPermissionsServiceServer(s, ps) v1.RegisterSchemaServiceServer(s, ss) diff --git a/pkg/schemadsl/compiler/compiler.go b/pkg/schemadsl/compiler/compiler.go index 435f9e22fb..99af7af50a 100644 --- a/pkg/schemadsl/compiler/compiler.go +++ b/pkg/schemadsl/compiler/compiler.go @@ -70,7 +70,7 @@ func AllowUnprefixedObjectType() ObjectPrefixOption { return func(cfg *config) { cfg.objectTypePrefix = new(string) } } -func DisallowExpirationFlag() ObjectPrefixOption { +func DisallowExpirationFlag() Option { return func(cfg *config) { cfg.allowedFlags = slices.Filter([]string{}, cfg.allowedFlags, func(s string) bool { return s != "expiration" From 436b3f3616a1dfbebd92f04722f5b2b936d9dde1 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Tue, 3 Dec 2024 14:26:54 -0500 Subject: [PATCH 20/39] Ensure the canonical representation of expiring rels always uses UTC --- pkg/tuple/hashing.go | 2 +- pkg/tuple/parsing_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/tuple/hashing.go b/pkg/tuple/hashing.go index 7e7bec6732..42fd1f4bf9 100644 --- a/pkg/tuple/hashing.go +++ b/pkg/tuple/hashing.go @@ -42,7 +42,7 @@ func CanonicalBytes(rel Relationship) ([]byte, error) { if rel.OptionalExpiration != nil { sb.WriteString(" with $expiration:") - sb.WriteString(rel.OptionalExpiration.Format(expirationFormat)) + sb.WriteString(rel.OptionalExpiration.UTC().Format(expirationFormat)) } return sb.Bytes(), nil diff --git a/pkg/tuple/parsing_test.go b/pkg/tuple/parsing_test.go index 2cf0588666..d44294072d 100644 --- a/pkg/tuple/parsing_test.go +++ b/pkg/tuple/parsing_test.go @@ -571,7 +571,7 @@ var testCases = []struct { time.Date(2020, 1, 1, 0, 0, 1, 0, time.FixedZone("UTC-4", -4*60*60)), ), v1Format: ev1rel("document", "foo", "viewer", "user", "tom", "", time.Date(2020, 1, 1, 4, 0, 1, 0, time.UTC)), - stableCanonicalization: "ZG9jdW1lbnQ6Zm9vI3ZpZXdlckB1c2VyOnRvbSMuLi4gd2l0aCAkZXhwaXJhdGlvbjoyMDIwLTAxLTAxVDAwOjAwOjAxLTA0OjAw", + stableCanonicalization: "ZG9jdW1lbnQ6Zm9vI3ZpZXdlckB1c2VyOnRvbSMuLi4gd2l0aCAkZXhwaXJhdGlvbjoyMDIwLTAxLTAxVDA0OjAwOjAxWg==", }, } From b18f05b6dd1b794b400263dbbbcbbae7414af86c Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Tue, 3 Dec 2024 14:36:04 -0500 Subject: [PATCH 21/39] Add indexes for relationship expiration GC --- .../migrations/zz_migration.0010_add_expiration.go | 8 ++++++++ .../zz_migration.0021_add_expiration_support.go | 10 ++++++++++ 2 files changed, 18 insertions(+) diff --git a/internal/datastore/mysql/migrations/zz_migration.0010_add_expiration.go b/internal/datastore/mysql/migrations/zz_migration.0010_add_expiration.go index f7057e4729..4a98fa41ec 100644 --- a/internal/datastore/mysql/migrations/zz_migration.0010_add_expiration.go +++ b/internal/datastore/mysql/migrations/zz_migration.0010_add_expiration.go @@ -9,10 +9,18 @@ func addExpirationToRelationTupleTable(t *tables) string { ) } +func addExpiredRelationshipsIndex(t *tables) string { + return fmt.Sprintf(`CREATE INDEX ix_%s_expired ON %s (expiration);`, + t.RelationTuple(), + t.RelationTuple(), + ) +} + func init() { mustRegisterMigration("add_expiration_to_relation_tuple", "add_metadata_to_transaction_table", noNonatomicMigration, newStatementBatch( addExpirationToRelationTupleTable, + addExpiredRelationshipsIndex, ).execute, ) } diff --git a/internal/datastore/postgres/migrations/zz_migration.0021_add_expiration_support.go b/internal/datastore/postgres/migrations/zz_migration.0021_add_expiration_support.go index 3f6e148dfa..8f6967c2f2 100644 --- a/internal/datastore/postgres/migrations/zz_migration.0021_add_expiration_support.go +++ b/internal/datastore/postgres/migrations/zz_migration.0021_add_expiration_support.go @@ -18,6 +18,12 @@ const addWithExpirationCoveringIndex = `CREATE INDEX CONCURRENTLY INCLUDE (userset_namespace, userset_object_id, userset_relation, caveat_name, caveat_context) WHERE deleted_xid = '9223372036854775807'::xid8;` +const addExpiredRelationshipsIndex = `CREATE INDEX CONCURRENTLY + IF NOT EXISTS ix_relation_tuple_expired + ON relation_tuple (expiration) + WHERE expiration IS NOT NULL; +` + func init() { if err := DatabaseMigrations.Register("add-expiration-support", "add-watch-api-index-to-relation-tuple-table", func(ctx context.Context, conn *pgx.Conn) error { @@ -29,6 +35,10 @@ func init() { return fmt.Errorf("failed to add expiration column to relation tuple table: %w", err) } + if _, err := conn.Exec(ctx, addExpiredRelationshipsIndex); err != nil { + return fmt.Errorf("failed to add expiration column to relation tuple table: %w", err) + } + return nil }, noTxMigration); err != nil { From cbe215a6fe3c514cff986ae39d1a88e226279900 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Tue, 3 Dec 2024 14:47:55 -0500 Subject: [PATCH 22/39] Truncate times to seconds when generating hashes --- pkg/tuple/hashing.go | 4 +++- pkg/tuple/parsing_test.go | 10 +++++----- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/pkg/tuple/hashing.go b/pkg/tuple/hashing.go index 42fd1f4bf9..1f733d46b0 100644 --- a/pkg/tuple/hashing.go +++ b/pkg/tuple/hashing.go @@ -4,6 +4,7 @@ import ( "bytes" "fmt" "sort" + "time" "google.golang.org/protobuf/types/known/structpb" @@ -42,7 +43,8 @@ func CanonicalBytes(rel Relationship) ([]byte, error) { if rel.OptionalExpiration != nil { sb.WriteString(" with $expiration:") - sb.WriteString(rel.OptionalExpiration.UTC().Format(expirationFormat)) + truncated := rel.OptionalExpiration.UTC().Truncate(time.Second) + sb.WriteString(truncated.Format(expirationFormat)) } return sb.Bytes(), nil diff --git a/pkg/tuple/parsing_test.go b/pkg/tuple/parsing_test.go index d44294072d..2ff55502f7 100644 --- a/pkg/tuple/parsing_test.go +++ b/pkg/tuple/parsing_test.go @@ -531,17 +531,17 @@ var testCases = []struct { stableCanonicalization: "ZG9jdW1lbnQ6Zm9vI3ZpZXdlckB1c2VyOnRvbSMuLi4gd2l0aCBzb21lY2F2ZWF0Ontmb286NDIuMDAwMDAwfSB3aXRoICRleHBpcmF0aW9uOjIwMjAtMDEtMDFUMDA6MDA6MDBa", }, { - input: `document:foo#viewer@user:tom[expiration:2020-01-01T00:00:01.542Z]`, - expectedOutput: `document:foo#viewer@user:tom[expiration:2020-01-01T00:00:01.542Z]`, + input: `document:foo#viewer@user:tom[expiration:2020-01-01T00:00:02.542Z]`, + expectedOutput: `document:foo#viewer@user:tom[expiration:2020-01-01T00:00:02.542Z]`, relFormat: MustWithExpiration( makeRel( StringToONR("document", "foo", "viewer"), StringToONR("user", "tom", "..."), ), - time.Date(2020, 1, 1, 0, 0, 1, 542000000, time.UTC), + time.Date(2020, 1, 1, 0, 0, 2, 542000000, time.UTC), ), - v1Format: ev1rel("document", "foo", "viewer", "user", "tom", "", time.Date(2020, 1, 1, 0, 0, 1, 542000000, time.UTC)), - stableCanonicalization: "ZG9jdW1lbnQ6Zm9vI3ZpZXdlckB1c2VyOnRvbSMuLi4gd2l0aCAkZXhwaXJhdGlvbjoyMDIwLTAxLTAxVDAwOjAwOjAxLjU0Mlo=", + v1Format: ev1rel("document", "foo", "viewer", "user", "tom", "", time.Date(2020, 1, 1, 0, 0, 2, 542000000, time.UTC)), + stableCanonicalization: "ZG9jdW1lbnQ6Zm9vI3ZpZXdlckB1c2VyOnRvbSMuLi4gd2l0aCAkZXhwaXJhdGlvbjoyMDIwLTAxLTAxVDAwOjAwOjAyWg==", }, { input: `document:foo#viewer@user:tom[expiration:2020-01-01T00:00:01Z]`, From 6b7937cf563065e3e53d51c4b50ea08376127e05 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Tue, 3 Dec 2024 16:35:46 -0500 Subject: [PATCH 23/39] Fix metadata map reference in CRDB --- internal/datastore/crdb/crdb.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/datastore/crdb/crdb.go b/internal/datastore/crdb/crdb.go index 76ff5c8dda..4613c3a089 100644 --- a/internal/datastore/crdb/crdb.go +++ b/internal/datastore/crdb/crdb.go @@ -357,7 +357,7 @@ func (cds *crdbDatastore) ReadWriteTx( expiresAt := time.Now().Add(cds.gcWindow).Add(1 * time.Minute) insertTransactionMetadata := psql.Insert(tableTransactionMetadata). Columns(colExpiresAt, colMetadata). - Values(expiresAt, config.Metadata.AsMap()) + Values(expiresAt, metadata) sql, args, err := insertTransactionMetadata.ToSql() if err != nil { From 2c725b6ced6d2d0121ab1341f82ab9cc827cbd0c Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Tue, 3 Dec 2024 16:36:39 -0500 Subject: [PATCH 24/39] Fix typo --- internal/services/v1/relationships_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/services/v1/relationships_test.go b/internal/services/v1/relationships_test.go index 2a2e2f0302..fe9443dead 100644 --- a/internal/services/v1/relationships_test.go +++ b/internal/services/v1/relationships_test.go @@ -481,7 +481,7 @@ func TestDeleteRelationshipViaWriteNoop(t *testing.T) { require.NoError(err) } -func TestWriteExpiringRelationshinps(t *testing.T) { +func TestWriteExpiringRelationships(t *testing.T) { req := require.New(t) conn, cleanup, _, _ := testserver.NewTestServer(req, 0, memdb.DisableGC, true, tf.StandardDatastoreWithData) From 8278eca6abc4b3d22eb7964d939d1ed11c37a01d Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Tue, 3 Dec 2024 17:44:08 -0500 Subject: [PATCH 25/39] Add expiration into CRDB watch --- internal/datastore/crdb/watch.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/internal/datastore/crdb/watch.go b/internal/datastore/crdb/watch.go index da6da149ad..de341112f7 100644 --- a/internal/datastore/crdb/watch.go +++ b/internal/datastore/crdb/watch.go @@ -55,6 +55,8 @@ type changeDetails struct { RelationshipCaveatContext map[string]any `json:"caveat_context"` RelationshipCaveatName string `json:"caveat_name"` + RelationshipExpiration *time.Time `json:"expires_at"` + IntegrityKeyID *string `json:"integrity_key_id"` IntegrityHashAsHex *string `json:"integrity_hash"` TimestampAsString *string `json:"timestamp"` @@ -444,8 +446,12 @@ func (cds *crdbDatastore) processChanges(ctx context.Context, changes pgx.Rows, return } - var integrity *core.RelationshipIntegrity + var expiration *time.Time + if details.After != nil { + expiration = details.After.RelationshipExpiration + } + var integrity *core.RelationshipIntegrity if details.After != nil && details.After.IntegrityKeyID != nil && details.After.IntegrityHashAsHex != nil && details.After.TimestampAsString != nil { hexString := *details.After.IntegrityHashAsHex hashBytes, err := hex.DecodeString(hexString[2:]) // drop the \x @@ -481,8 +487,9 @@ func (cds *crdbDatastore) processChanges(ctx context.Context, changes pgx.Rows, Relation: pkValues[5], }, }, - OptionalCaveat: ctxCaveat, - OptionalIntegrity: integrity, + OptionalCaveat: ctxCaveat, + OptionalIntegrity: integrity, + OptionalExpiration: expiration, } rev, err := revisions.HLCRevisionFromString(details.Updated) From 14189f55a3e24e42e3e62d747a98c8c1b3423631 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Wed, 4 Dec 2024 12:06:07 -0500 Subject: [PATCH 26/39] Lower watch buffer size to ensure timeout is hit CRDB is batching up events differently now for watch and its causing the larger buffer to not hit the timeout --- pkg/datastore/test/watch.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/datastore/test/watch.go b/pkg/datastore/test/watch.go index f8b0bc37b2..54cf39f3a5 100644 --- a/pkg/datastore/test/watch.go +++ b/pkg/datastore/test/watch.go @@ -74,7 +74,7 @@ func WatchTest(t *testing.T, tester DatastoreTester) { opts := datastore.WatchOptions{ Content: datastore.WatchRelationships, - WatchBufferLength: 128, + WatchBufferLength: 50, WatchBufferWriteTimeout: tc.bufferTimeout, } changes, errchan := ds.Watch(ctx, lowestRevision, opts) From cceb01a34fb3ac96d61b573af03a20a6d5d07516 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Wed, 4 Dec 2024 13:03:08 -0500 Subject: [PATCH 27/39] Add additional rel conversion tests --- pkg/tuple/v1_test.go | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/pkg/tuple/v1_test.go b/pkg/tuple/v1_test.go index 948160e408..8db3e1cd6d 100644 --- a/pkg/tuple/v1_test.go +++ b/pkg/tuple/v1_test.go @@ -5,6 +5,8 @@ import ( v1 "github.com/authzed/authzed-go/proto/authzed/api/v1" "github.com/stretchr/testify/require" + + "github.com/authzed/spicedb/pkg/testutil" ) func objRef(typ, id string) *v1.ObjectReference { @@ -44,3 +46,26 @@ func TestJoinSubjectRef(t *testing.T) { require.Equal(t, tt.expected, V1StringSubjectRef(tt.ref)) } } + +func TestBackAndForth(t *testing.T) { + for _, tc := range testCases { + tc := tc + if tc.stableCanonicalization == "" { + continue + } + + t.Run("relationship/"+tc.input, func(t *testing.T) { + v1rel := ToV1Relationship(tc.relFormat) + testutil.RequireProtoEqual(t, tc.v1Format, v1rel, "mismatch in v1 proto") + adjusted := tc.relFormat + + // Converting to V1 removes the timezones. + if adjusted.OptionalExpiration != nil { + t := adjusted.OptionalExpiration.UTC() + adjusted.OptionalExpiration = &t + } + + require.Equal(t, adjusted, FromV1Relationship(v1rel)) + }) + } +} From ff686e2529a8c066c687d963d8a1e4208cfd3a05 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Wed, 4 Dec 2024 13:03:14 -0500 Subject: [PATCH 28/39] Remove unused method in memdb --- internal/datastore/memdb/schema.go | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/internal/datastore/memdb/schema.go b/internal/datastore/memdb/schema.go index 9328137228..b68c48a21f 100644 --- a/internal/datastore/memdb/schema.go +++ b/internal/datastore/memdb/schema.go @@ -3,9 +3,7 @@ package memdb import ( "time" - v1 "github.com/authzed/authzed-go/proto/authzed/api/v1" "github.com/hashicorp/go-memdb" - "github.com/jzelinskie/stringz" "github.com/rs/zerolog" "google.golang.org/protobuf/types/known/structpb" "google.golang.org/protobuf/types/known/timestamppb" @@ -109,23 +107,6 @@ func (r relationship) MarshalZerologObject(e *zerolog.Event) { e.Str("rel", r.String()) } -func (r relationship) V1Relationship() *v1.Relationship { - return &v1.Relationship{ - Resource: &v1.ObjectReference{ - ObjectType: r.namespace, - ObjectId: r.resourceID, - }, - Relation: r.relation, - Subject: &v1.SubjectReference{ - Object: &v1.ObjectReference{ - ObjectType: r.subjectNamespace, - ObjectId: r.subjectObjectID, - }, - OptionalRelation: stringz.Default(r.subjectRelation, "", datastore.Ellipsis), - }, - } -} - func (r relationship) Relationship() (tuple.Relationship, error) { cr, err := r.caveat.ContextualizedCaveat() if err != nil { From c5d6ce09df0d06683f354291c1e46a1a4e8190cf Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Wed, 4 Dec 2024 13:11:54 -0500 Subject: [PATCH 29/39] Add expiration to memdb relationship debug string --- internal/datastore/memdb/schema.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/internal/datastore/memdb/schema.go b/internal/datastore/memdb/schema.go index b68c48a21f..7905d4854f 100644 --- a/internal/datastore/memdb/schema.go +++ b/internal/datastore/memdb/schema.go @@ -100,7 +100,12 @@ func (r relationship) String() string { caveat = "[" + r.caveat.caveatName + "]" } - return r.namespace + ":" + r.resourceID + "#" + r.relation + "@" + r.subjectNamespace + ":" + r.subjectObjectID + "#" + r.subjectRelation + caveat + expiration := "" + if r.expiration != nil { + expiration = "[expiration:" + r.expiration.Format(time.RFC3339Nano) + "]" + } + + return r.namespace + ":" + r.resourceID + "#" + r.relation + "@" + r.subjectNamespace + ":" + r.subjectObjectID + "#" + r.subjectRelation + caveat + expiration } func (r relationship) MarshalZerologObject(e *zerolog.Event) { From 1f5b1ca27503cb9e40994a1ba833237fb1cbc16f Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Wed, 4 Dec 2024 13:19:00 -0500 Subject: [PATCH 30/39] Add counter test over expiring and expired relationships --- pkg/datastore/test/counters.go | 53 +++++++++++++++++++++++++++++++++ pkg/datastore/test/datastore.go | 1 + 2 files changed, 54 insertions(+) diff --git a/pkg/datastore/test/counters.go b/pkg/datastore/test/counters.go index 0f82379900..c3341cb8a6 100644 --- a/pkg/datastore/test/counters.go +++ b/pkg/datastore/test/counters.go @@ -3,6 +3,7 @@ package test import ( "context" "testing" + "time" "github.com/stretchr/testify/require" @@ -10,8 +11,60 @@ import ( "github.com/authzed/spicedb/pkg/datastore" core "github.com/authzed/spicedb/pkg/proto/core/v1" + "github.com/authzed/spicedb/pkg/tuple" ) +func RelationshipCounterOverExpiredTest(t *testing.T, tester DatastoreTester) { + rawDS, err := tester.New(0, veryLargeGCInterval, veryLargeGCWindow, 1) + require.NoError(t, err) + + ds, _ := testfixtures.StandardDatastoreWithData(rawDS, require.New(t)) + + // Register the filter. + _, err = ds.ReadWriteTx(context.Background(), func(ctx context.Context, tx datastore.ReadWriteTransaction) error { + err := tx.RegisterCounter(ctx, "document", &core.RelationshipFilter{ + ResourceType: testfixtures.DocumentNS.Name, + }) + require.NoError(t, err) + return nil + }) + require.NoError(t, err) + + // Add some expiring and expired relationships. + updatedRev, err := ds.ReadWriteTx(context.Background(), func(ctx context.Context, tx datastore.ReadWriteTransaction) error { + return tx.WriteRelationships(ctx, []tuple.RelationshipUpdate{ + tuple.Touch(tuple.MustParse("document:somedoc#expiring_viewer@user:tom[expiration:2020-01-01T00:00:00Z]")), + tuple.Touch(tuple.MustParse("document:somedoc#expiring_viewer@user:fred[expiration:2320-01-01T00:00:00Z]")), + }) + }) + require.NoError(t, err) + + // Check the count using the filter. + reader := ds.SnapshotReader(updatedRev) + + expectedCount := 0 + iter, err := reader.QueryRelationships(context.Background(), datastore.RelationshipsFilter{ + OptionalResourceType: testfixtures.DocumentNS.Name, + }) + require.NoError(t, err) + + foundExpiringRel := false + for rel, err := range iter { + expectedCount++ + require.NoError(t, err) + foundExpiringRel = foundExpiringRel || rel.OptionalExpiration != nil + if rel.OptionalExpiration != nil { + require.True(t, rel.OptionalExpiration.After(time.Now())) + } + } + + require.True(t, foundExpiringRel) + + count, err := reader.CountRelationships(context.Background(), "document") + require.NoError(t, err) + require.Equal(t, expectedCount, count) +} + func RelationshipCountersTest(t *testing.T, tester DatastoreTester) { rawDS, err := tester.New(0, veryLargeGCInterval, veryLargeGCWindow, 1) require.NoError(t, err) diff --git a/pkg/datastore/test/datastore.go b/pkg/datastore/test/datastore.go index d6421b5f35..40b34c01a3 100644 --- a/pkg/datastore/test/datastore.go +++ b/pkg/datastore/test/datastore.go @@ -198,6 +198,7 @@ func AllWithExceptions(t *testing.T, tester DatastoreTester, except Categories, t.Run("TestRelationshipCounters", runner(tester, RelationshipCountersTest)) t.Run("TestUpdateRelationshipCounter", runner(tester, UpdateRelationshipCounterTest)) t.Run("TestDeleteAllData", runner(tester, DeleteAllDataTest)) + t.Run("TestRelationshipCounterOverExpired", runner(tester, RelationshipCounterOverExpiredTest)) } // All runs all generic datastore tests on a DatastoreTester. From 94ba9240b0d007627c9c044b4c8fd6e4a6f98631 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Wed, 4 Dec 2024 13:20:59 -0500 Subject: [PATCH 31/39] Add doc comments on expiration filter options --- pkg/datastore/datastore.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/pkg/datastore/datastore.go b/pkg/datastore/datastore.go index e78420c71f..c3fcbf63bf 100644 --- a/pkg/datastore/datastore.go +++ b/pkg/datastore/datastore.go @@ -83,11 +83,20 @@ func (rc *RevisionChanges) MarshalZerologObject(e *zerolog.Event) { e.Int("num-changed-relationships", len(rc.RelationshipChanges)) } +// ExpirationFilterOption is the filter option for the expiration field on relationships. type ExpirationFilterOption int const ( + // ExpirationFilterOptionNone indicates that the expiration filter should not be used: + // relationships both with and without expiration will be returned. ExpirationFilterOptionNone ExpirationFilterOption = iota + + // ExpirationFilterOptionHasExpiration indicates that the expiration filter should only + // return relationships with an expiration. ExpirationFilterOptionHasExpiration + + // ExpirationFilterOptionNoExpiration indicates that the expiration filter should only + // return relationships without an expiration. ExpirationFilterOptionNoExpiration ) From eb7bfe1e9ef7ce54a97a2b184470c2c17592ecb9 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Wed, 4 Dec 2024 13:26:58 -0500 Subject: [PATCH 32/39] Change to use a single `now` for the entire reader in memdb --- internal/datastore/memdb/memdb.go | 10 +++++----- internal/datastore/memdb/readonly.go | 15 +++++++-------- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/internal/datastore/memdb/memdb.go b/internal/datastore/memdb/memdb.go index b8f416ea2c..796b1b2bc2 100644 --- a/internal/datastore/memdb/memdb.go +++ b/internal/datastore/memdb/memdb.go @@ -105,11 +105,11 @@ func (mdb *memdbDatastore) SnapshotReader(dr datastore.Revision) datastore.Reade defer mdb.RUnlock() if len(mdb.revisions) == 0 { - return &memdbReader{nil, nil, fmt.Errorf("memdb datastore is not ready")} + return &memdbReader{nil, nil, fmt.Errorf("memdb datastore is not ready"), time.Now()} } if err := mdb.checkRevisionLocalCallerMustLock(dr); err != nil { - return &memdbReader{nil, nil, err} + return &memdbReader{nil, nil, err, time.Now()} } revIndex := sort.Search(len(mdb.revisions), func(i int) bool { @@ -123,7 +123,7 @@ func (mdb *memdbDatastore) SnapshotReader(dr datastore.Revision) datastore.Reade rev := mdb.revisions[revIndex] if rev.db == nil { - return &memdbReader{nil, nil, fmt.Errorf("memdb datastore is already closed")} + return &memdbReader{nil, nil, fmt.Errorf("memdb datastore is already closed"), time.Now()} } roTxn := rev.db.Txn(false) @@ -131,7 +131,7 @@ func (mdb *memdbDatastore) SnapshotReader(dr datastore.Revision) datastore.Reade return roTxn, nil } - return &memdbReader{noopTryLocker{}, txSrc, nil} + return &memdbReader{noopTryLocker{}, txSrc, nil, time.Now()} } func (mdb *memdbDatastore) SupportsIntegrity() bool { @@ -177,7 +177,7 @@ func (mdb *memdbDatastore) ReadWriteTx( } newRevision := mdb.newRevisionID() - rwt := &memdbReadWriteTx{memdbReader{&sync.Mutex{}, txSrc, nil}, newRevision} + rwt := &memdbReadWriteTx{memdbReader{&sync.Mutex{}, txSrc, nil, time.Now()}, newRevision} if err := f(ctx, rwt); err != nil { mdb.Lock() if tx != nil { diff --git a/internal/datastore/memdb/readonly.go b/internal/datastore/memdb/readonly.go index 29c96dc7d8..87ef405c1f 100644 --- a/internal/datastore/memdb/readonly.go +++ b/internal/datastore/memdb/readonly.go @@ -24,6 +24,7 @@ type memdbReader struct { TryLocker txSource txFactory initErr error + now time.Time } func (r *memdbReader) CountRelationships(ctx context.Context, name string) (int, error) { @@ -150,11 +151,11 @@ func (r *memdbReader) QueryRelationships( fallthrough case options.ByResource: - iter := newMemdbTupleIterator(filteredIterator, queryOpts.Limit) + iter := newMemdbTupleIterator(r.now, filteredIterator, queryOpts.Limit) return iter, nil case options.BySubject: - return newSubjectSortedIterator(filteredIterator, queryOpts.Limit) + return newSubjectSortedIterator(r.now, filteredIterator, queryOpts.Limit) default: return nil, spiceerrors.MustBugf("unsupported sort order: %v", queryOpts.Sort) @@ -213,11 +214,11 @@ func (r *memdbReader) ReverseQueryRelationships( fallthrough case options.ByResource: - iter := newMemdbTupleIterator(filteredIterator, queryOpts.LimitForReverse) + iter := newMemdbTupleIterator(r.now, filteredIterator, queryOpts.LimitForReverse) return iter, nil case options.BySubject: - return newSubjectSortedIterator(filteredIterator, queryOpts.LimitForReverse) + return newSubjectSortedIterator(r.now, filteredIterator, queryOpts.LimitForReverse) default: return nil, spiceerrors.MustBugf("unsupported sort order: %v", queryOpts.SortForReverse) @@ -475,11 +476,10 @@ func makeCursorFilterFn(after options.Cursor, order options.SortOrder) func(tpl return noopCursorFilter } -func newSubjectSortedIterator(it memdb.ResultIterator, limit *uint64) (datastore.RelationshipIterator, error) { +func newSubjectSortedIterator(now time.Time, it memdb.ResultIterator, limit *uint64) (datastore.RelationshipIterator, error) { results := make([]tuple.Relationship, 0) // Coalesce all of the results into memory - now := time.Now() for foundRaw := it.Next(); foundRaw != nil; foundRaw = it.Next() { rt, err := foundRaw.(*relationship).Relationship() if err != nil { @@ -526,10 +526,9 @@ func eq(lhsNamespace, lhsObjectID, lhsRelation string, rhs tuple.ObjectAndRelati return lhsNamespace == rhs.ObjectType && lhsObjectID == rhs.ObjectID && lhsRelation == rhs.Relation } -func newMemdbTupleIterator(it memdb.ResultIterator, limit *uint64) datastore.RelationshipIterator { +func newMemdbTupleIterator(now time.Time, it memdb.ResultIterator, limit *uint64) datastore.RelationshipIterator { var count uint64 return func(yield func(tuple.Relationship, error) bool) { - now := time.Now() for { foundRaw := it.Next() if foundRaw == nil { From 20b67a8710bad2a4b306d4174a3def45cf9b6ad1 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Wed, 4 Dec 2024 13:59:51 -0500 Subject: [PATCH 33/39] Make sure *all* queries used in the query builder are filtered by rel expiration --- internal/datastore/common/sql.go | 12 ++-- internal/datastore/common/sql_test.go | 86 +++++++++++++-------------- 2 files changed, 49 insertions(+), 49 deletions(-) diff --git a/internal/datastore/common/sql.go b/internal/datastore/common/sql.go index d99c846af5..d886927931 100644 --- a/internal/datastore/common/sql.go +++ b/internal/datastore/common/sql.go @@ -118,6 +118,12 @@ func NewSchemaQueryFilterer(schema SchemaInformation, initialQuery sq.SelectBuil log.Warn().Msg("SchemaQueryFilterer: filterMaximumIDCount not set, defaulting to 100") } + // Filter out any expired relationships. + initialQuery = initialQuery.Where(sq.Or{ + sq.Eq{schema.colExpiration: nil}, + sq.Expr(schema.colExpiration + " > " + schema.nowFunction + "()"), + }) + return SchemaQueryFilterer{ schema: schema, queryBuilder: initialQuery, @@ -576,12 +582,6 @@ func (tqs QueryExecutor) ExecuteQuery( toExecute := query.limit(limit) - // Filter out any expired relationships. - toExecute.queryBuilder = toExecute.queryBuilder.Where(sq.Or{ - sq.Eq{query.schema.colExpiration: nil}, - sq.Expr(query.schema.colExpiration + " > " + query.schema.nowFunction + "()"), - }) - // Run the query. sql, args, err := toExecute.queryBuilder.ToSql() if err != nil { diff --git a/internal/datastore/common/sql_test.go b/internal/datastore/common/sql_test.go index b7ade62d37..11043512b9 100644 --- a/internal/datastore/common/sql_test.go +++ b/internal/datastore/common/sql_test.go @@ -31,7 +31,7 @@ func TestSchemaQueryFilterer(t *testing.T) { func(filterer SchemaQueryFilterer) SchemaQueryFilterer { return filterer.FilterToRelation("somerelation") }, - "SELECT * WHERE relation = ?", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND relation = ?", []any{"somerelation"}, map[string]int{ "relation": 1, @@ -42,7 +42,7 @@ func TestSchemaQueryFilterer(t *testing.T) { func(filterer SchemaQueryFilterer) SchemaQueryFilterer { return filterer.FilterToResourceID("someresourceid") }, - "SELECT * WHERE object_id = ?", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND object_id = ?", []any{"someresourceid"}, map[string]int{ "object_id": 1, @@ -53,7 +53,7 @@ func TestSchemaQueryFilterer(t *testing.T) { func(filterer SchemaQueryFilterer) SchemaQueryFilterer { return filterer.MustFilterWithResourceIDPrefix("someprefix") }, - "SELECT * WHERE object_id LIKE ?", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND object_id LIKE ?", []any{"someprefix%"}, map[string]int{}, // object_id is not statically used, so not present in the map }, @@ -62,7 +62,7 @@ func TestSchemaQueryFilterer(t *testing.T) { func(filterer SchemaQueryFilterer) SchemaQueryFilterer { return filterer.MustFilterToResourceIDs([]string{"someresourceid", "anotherresourceid"}) }, - "SELECT * WHERE object_id IN (?,?)", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND object_id IN (?,?)", []any{"someresourceid", "anotherresourceid"}, map[string]int{ "object_id": 2, @@ -73,7 +73,7 @@ func TestSchemaQueryFilterer(t *testing.T) { func(filterer SchemaQueryFilterer) SchemaQueryFilterer { return filterer.FilterToResourceType("sometype") }, - "SELECT * WHERE ns = ?", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND ns = ?", []any{"sometype"}, map[string]int{ "ns": 1, @@ -84,7 +84,7 @@ func TestSchemaQueryFilterer(t *testing.T) { func(filterer SchemaQueryFilterer) SchemaQueryFilterer { return filterer.FilterToResourceType("sometype").FilterToResourceID("someobj").FilterToRelation("somerel") }, - "SELECT * WHERE ns = ? AND object_id = ? AND relation = ?", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND ns = ? AND object_id = ? AND relation = ?", []any{"sometype", "someobj", "somerel"}, map[string]int{ "ns": 1, @@ -99,7 +99,7 @@ func TestSchemaQueryFilterer(t *testing.T) { OptionalResourceType: "sometype", }) }, - "SELECT * WHERE ns = ?", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND ns = ?", []any{"sometype"}, map[string]int{ "ns": 1, @@ -113,7 +113,7 @@ func TestSchemaQueryFilterer(t *testing.T) { OptionalResourceIds: []string{"someid"}, }) }, - "SELECT * WHERE ns = ? AND object_id IN (?)", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND ns = ? AND object_id IN (?)", []any{"sometype", "someid"}, map[string]int{ "ns": 1, @@ -128,7 +128,7 @@ func TestSchemaQueryFilterer(t *testing.T) { OptionalResourceIds: []string{}, }) }, - "SELECT * WHERE ns = ?", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND ns = ?", []any{"sometype"}, map[string]int{ "ns": 1, @@ -142,7 +142,7 @@ func TestSchemaQueryFilterer(t *testing.T) { OptionalResourceIds: []string{"someid", "anotherid"}, }) }, - "SELECT * WHERE ns = ? AND object_id IN (?,?)", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND ns = ? AND object_id IN (?,?)", []any{"sometype", "someid", "anotherid"}, map[string]int{ "ns": 1, @@ -156,7 +156,7 @@ func TestSchemaQueryFilterer(t *testing.T) { OptionalSubjectType: "somesubjectype", }) }, - "SELECT * WHERE ((subject_ns = ?))", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND ((subject_ns = ?))", []any{"somesubjectype"}, map[string]int{ "subject_ns": 1, @@ -171,7 +171,7 @@ func TestSchemaQueryFilterer(t *testing.T) { OptionalSubjectType: "anothersubjectype", }) }, - "SELECT * WHERE ((subject_ns = ?) OR (subject_ns = ?))", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND ((subject_ns = ?) OR (subject_ns = ?))", []any{"somesubjectype", "anothersubjectype"}, map[string]int{ "subject_ns": 2, @@ -185,7 +185,7 @@ func TestSchemaQueryFilterer(t *testing.T) { OptionalSubjectIds: []string{"somesubjectid"}, }) }, - "SELECT * WHERE ((subject_ns = ? AND subject_object_id IN (?)))", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND ((subject_ns = ? AND subject_object_id IN (?)))", []any{"somesubjectype", "somesubjectid"}, map[string]int{ "subject_ns": 1, @@ -199,7 +199,7 @@ func TestSchemaQueryFilterer(t *testing.T) { OptionalSubjectIds: []string{"somesubjectid"}, }) }, - "SELECT * WHERE ((subject_object_id IN (?)))", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND ((subject_object_id IN (?)))", []any{"somesubjectid"}, map[string]int{ "subject_object_id": 1, @@ -210,7 +210,7 @@ func TestSchemaQueryFilterer(t *testing.T) { func(filterer SchemaQueryFilterer) SchemaQueryFilterer { return filterer.MustFilterWithSubjectsSelectors(datastore.SubjectsSelector{}) }, - "SELECT * WHERE ((1=1))", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND ((1=1))", nil, map[string]int{}, }, @@ -222,7 +222,7 @@ func TestSchemaQueryFilterer(t *testing.T) { OptionalSubjectIds: []string{"somesubjectid", "anothersubjectid"}, }) }, - "SELECT * WHERE ((subject_ns = ? AND subject_object_id IN (?,?)))", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND ((subject_ns = ? AND subject_object_id IN (?,?)))", []any{"somesubjectype", "somesubjectid", "anothersubjectid"}, map[string]int{ "subject_ns": 1, @@ -237,7 +237,7 @@ func TestSchemaQueryFilterer(t *testing.T) { RelationFilter: datastore.SubjectRelationFilter{}.WithEllipsisRelation(), }) }, - "SELECT * WHERE ((subject_ns = ? AND subject_relation = ?))", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND ((subject_ns = ? AND subject_relation = ?))", []any{"somesubjectype", "..."}, map[string]int{ "subject_ns": 1, @@ -252,7 +252,7 @@ func TestSchemaQueryFilterer(t *testing.T) { RelationFilter: datastore.SubjectRelationFilter{}.WithNonEllipsisRelation("somesubrel"), }) }, - "SELECT * WHERE ((subject_ns = ? AND subject_relation = ?))", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND ((subject_ns = ? AND subject_relation = ?))", []any{"somesubjectype", "somesubrel"}, map[string]int{ "subject_ns": 1, @@ -267,7 +267,7 @@ func TestSchemaQueryFilterer(t *testing.T) { RelationFilter: datastore.SubjectRelationFilter{}.WithOnlyNonEllipsisRelations(), }) }, - "SELECT * WHERE ((subject_ns = ? AND subject_relation <> ?))", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND ((subject_ns = ? AND subject_relation <> ?))", []any{"somesubjectype", "..."}, map[string]int{ "subject_ns": 1, @@ -282,7 +282,7 @@ func TestSchemaQueryFilterer(t *testing.T) { RelationFilter: datastore.SubjectRelationFilter{}.WithNonEllipsisRelation("somesubrel").WithEllipsisRelation(), }) }, - "SELECT * WHERE ((subject_ns = ? AND (subject_relation = ? OR subject_relation = ?)))", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND ((subject_ns = ? AND (subject_relation = ? OR subject_relation = ?)))", []any{"somesubjectype", "...", "somesubrel"}, map[string]int{ "subject_ns": 1, @@ -298,7 +298,7 @@ func TestSchemaQueryFilterer(t *testing.T) { RelationFilter: datastore.SubjectRelationFilter{}.WithNonEllipsisRelation("somesubrel").WithEllipsisRelation(), }) }, - "SELECT * WHERE ((subject_ns = ? AND subject_object_id IN (?,?) AND (subject_relation = ? OR subject_relation = ?)))", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND ((subject_ns = ? AND subject_object_id IN (?,?) AND (subject_relation = ? OR subject_relation = ?)))", []any{"somesubjectype", "somesubjectid", "anothersubjectid", "...", "somesubrel"}, map[string]int{ "subject_ns": 1, @@ -326,7 +326,7 @@ func TestSchemaQueryFilterer(t *testing.T) { }, ) }, - "SELECT * WHERE ((subject_ns = ? AND subject_object_id IN (?,?) AND (subject_relation = ? OR subject_relation = ?)) OR (subject_ns = ? AND subject_object_id IN (?,?) AND (subject_relation = ? OR subject_relation = ?)) OR (subject_ns = ? AND subject_relation <> ?))", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND ((subject_ns = ? AND subject_object_id IN (?,?) AND (subject_relation = ? OR subject_relation = ?)) OR (subject_ns = ? AND subject_object_id IN (?,?) AND (subject_relation = ? OR subject_relation = ?)) OR (subject_ns = ? AND subject_relation <> ?))", []any{"somesubjectype", "a", "b", "...", "somesubrel", "anothersubjecttype", "b", "c", "...", "anotherrel", "thirdsubjectype", "..."}, map[string]int{ "subject_ns": 3, @@ -341,7 +341,7 @@ func TestSchemaQueryFilterer(t *testing.T) { SubjectType: "subns", }) }, - "SELECT * WHERE subject_ns = ?", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND subject_ns = ?", []any{"subns"}, map[string]int{ "subject_ns": 1, @@ -355,7 +355,7 @@ func TestSchemaQueryFilterer(t *testing.T) { OptionalSubjectId: "subid", }) }, - "SELECT * WHERE subject_ns = ? AND subject_object_id = ?", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND subject_ns = ? AND subject_object_id = ?", []any{"subns", "subid"}, map[string]int{ "subject_ns": 1, @@ -372,7 +372,7 @@ func TestSchemaQueryFilterer(t *testing.T) { }, }) }, - "SELECT * WHERE subject_ns = ? AND subject_relation = ?", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND subject_ns = ? AND subject_relation = ?", []any{"subns", "subrel"}, map[string]int{ "subject_ns": 1, @@ -389,7 +389,7 @@ func TestSchemaQueryFilterer(t *testing.T) { }, }) }, - "SELECT * WHERE subject_ns = ? AND subject_relation = ?", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND subject_ns = ? AND subject_relation = ?", []any{"subns", "..."}, map[string]int{ "subject_ns": 1, @@ -407,7 +407,7 @@ func TestSchemaQueryFilterer(t *testing.T) { }, }) }, - "SELECT * WHERE subject_ns = ? AND subject_object_id = ? AND subject_relation = ?", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND subject_ns = ? AND subject_object_id = ? AND subject_relation = ?", []any{"subns", "subid", "somerel"}, map[string]int{ "subject_ns": 1, @@ -420,7 +420,7 @@ func TestSchemaQueryFilterer(t *testing.T) { func(filterer SchemaQueryFilterer) SchemaQueryFilterer { return filterer.limit(100) }, - "SELECT * LIMIT 100", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) LIMIT 100", nil, map[string]int{}, }, @@ -442,7 +442,7 @@ func TestSchemaQueryFilterer(t *testing.T) { }, ) }, - "SELECT * WHERE ns = ? AND relation = ? AND object_id IN (?,?) AND ((subject_ns = ? AND subject_object_id IN (?,?) AND (subject_relation = ? OR subject_relation = ?)))", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND ns = ? AND relation = ? AND object_id IN (?,?) AND ((subject_ns = ? AND subject_object_id IN (?,?) AND (subject_relation = ? OR subject_relation = ?)))", []any{"someresourcetype", "somerelation", "someid", "anotherid", "somesubjectype", "somesubjectid", "anothersubjectid", "...", "somesubrel"}, map[string]int{ "ns": 1, @@ -462,7 +462,7 @@ func TestSchemaQueryFilterer(t *testing.T) { }, ).TupleOrder(options.ByResource) }, - "SELECT * WHERE ns = ? ORDER BY ns, object_id, relation, subject_ns, subject_object_id, subject_relation", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND ns = ? ORDER BY ns, object_id, relation, subject_ns, subject_object_id, subject_relation", []any{"someresourcetype"}, map[string]int{ "ns": 1, @@ -477,7 +477,7 @@ func TestSchemaQueryFilterer(t *testing.T) { }, ).After(toCursor(tuple.MustParse("someresourcetype:foo#viewer@user:bar")), options.ByResource) }, - "SELECT * WHERE ns = ? AND (object_id,relation,subject_ns,subject_object_id,subject_relation) > (?,?,?,?,?)", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND ns = ? AND (object_id,relation,subject_ns,subject_object_id,subject_relation) > (?,?,?,?,?)", []any{"someresourcetype", "foo", "viewer", "user", "bar", "..."}, map[string]int{ "ns": 1, @@ -492,7 +492,7 @@ func TestSchemaQueryFilterer(t *testing.T) { }, ).After(toCursor(tuple.MustParse("someresourcetype:foo#viewer@user:bar")), options.ByResource) }, - "SELECT * WHERE relation = ? AND (ns,object_id,subject_ns,subject_object_id,subject_relation) > (?,?,?,?,?)", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND relation = ? AND (ns,object_id,subject_ns,subject_object_id,subject_relation) > (?,?,?,?,?)", []any{"somerelation", "someresourcetype", "foo", "user", "bar", "..."}, map[string]int{ "relation": 1, @@ -508,7 +508,7 @@ func TestSchemaQueryFilterer(t *testing.T) { }, ).After(toCursor(tuple.MustParse("someresourcetype:foo#viewer@user:bar")), options.ByResource) }, - "SELECT * WHERE ns = ? AND object_id IN (?) AND (relation,subject_ns,subject_object_id,subject_relation) > (?,?,?,?)", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND ns = ? AND object_id IN (?) AND (relation,subject_ns,subject_object_id,subject_relation) > (?,?,?,?)", []any{"someresourcetype", "one", "viewer", "user", "bar", "..."}, map[string]int{ "ns": 1, @@ -524,7 +524,7 @@ func TestSchemaQueryFilterer(t *testing.T) { }, ).After(toCursor(tuple.MustParse("someresourcetype:foo#viewer@user:bar")), options.ByResource) }, - "SELECT * WHERE object_id IN (?) AND (ns,relation,subject_ns,subject_object_id,subject_relation) > (?,?,?,?,?)", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND object_id IN (?) AND (ns,relation,subject_ns,subject_object_id,subject_relation) > (?,?,?,?,?)", []any{"one", "someresourcetype", "viewer", "user", "bar", "..."}, map[string]int{ "object_id": 1, @@ -540,7 +540,7 @@ func TestSchemaQueryFilterer(t *testing.T) { }, ).After(toCursor(tuple.MustParse("someresourcetype:foo#viewer@user:bar")), options.ByResource) }, - "SELECT * WHERE ns = ? AND object_id IN (?,?) AND (object_id,relation,subject_ns,subject_object_id,subject_relation) > (?,?,?,?,?)", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND ns = ? AND object_id IN (?,?) AND (object_id,relation,subject_ns,subject_object_id,subject_relation) > (?,?,?,?,?)", []any{"someresourcetype", "one", "two", "foo", "viewer", "user", "bar", "..."}, map[string]int{ "ns": 1, @@ -557,7 +557,7 @@ func TestSchemaQueryFilterer(t *testing.T) { }, ).After(toCursor(tuple.MustParse("someresourcetype:foo#viewer@user:bar")), options.ByResource) }, - "SELECT * WHERE ns = ? AND relation = ? AND (object_id,subject_ns,subject_object_id,subject_relation) > (?,?,?,?)", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND ns = ? AND relation = ? AND (object_id,subject_ns,subject_object_id,subject_relation) > (?,?,?,?)", []any{"someresourcetype", "somerelation", "foo", "user", "bar", "..."}, map[string]int{ "ns": 1, @@ -571,7 +571,7 @@ func TestSchemaQueryFilterer(t *testing.T) { OptionalSubjectType: "somesubjectype", }).After(toCursor(tuple.MustParse("someresourcetype:foo#viewer@user:bar")), options.ByResource) }, - "SELECT * WHERE ((subject_ns = ?)) AND (ns,object_id,relation,subject_object_id,subject_relation) > (?,?,?,?,?)", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND ((subject_ns = ?)) AND (ns,object_id,relation,subject_object_id,subject_relation) > (?,?,?,?,?)", []any{"somesubjectype", "someresourcetype", "foo", "viewer", "bar", "..."}, map[string]int{ "subject_ns": 1, @@ -588,7 +588,7 @@ func TestSchemaQueryFilterer(t *testing.T) { OptionalSubjectType: "anothersubjectype", }).After(toCursor(tuple.MustParse("someresourcetype:foo#viewer@user:bar")), options.ByResource) }, - "SELECT * WHERE ((subject_ns = ?)) AND ((subject_ns = ?)) AND (ns,object_id,relation,subject_ns,subject_object_id,subject_relation) > (?,?,?,?,?,?)", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND ((subject_ns = ?)) AND ((subject_ns = ?)) AND (ns,object_id,relation,subject_ns,subject_object_id,subject_relation) > (?,?,?,?,?,?)", []any{"somesubjectype", "anothersubjectype", "someresourcetype", "foo", "viewer", "user", "bar", "..."}, map[string]int{ "subject_ns": 2, @@ -599,7 +599,7 @@ func TestSchemaQueryFilterer(t *testing.T) { func(filterer SchemaQueryFilterer) SchemaQueryFilterer { return filterer.MustFilterWithResourceIDPrefix("someprefix").After(toCursor(tuple.MustParse("someresourcetype:foo#viewer@user:bar")), options.ByResource) }, - "SELECT * WHERE object_id LIKE ? AND (ns,object_id,relation,subject_ns,subject_object_id,subject_relation) > (?,?,?,?,?,?)", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND object_id LIKE ? AND (ns,object_id,relation,subject_ns,subject_object_id,subject_relation) > (?,?,?,?,?,?)", []any{"someprefix%", "someresourcetype", "foo", "viewer", "user", "bar", "..."}, map[string]int{}, }, @@ -612,7 +612,7 @@ func TestSchemaQueryFilterer(t *testing.T) { }, ).TupleOrder(options.BySubject) }, - "SELECT * WHERE ns = ? ORDER BY subject_ns, subject_object_id, subject_relation, ns, object_id, relation", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND ns = ? ORDER BY subject_ns, subject_object_id, subject_relation, ns, object_id, relation", []any{"someresourcetype"}, map[string]int{ "ns": 1, @@ -625,7 +625,7 @@ func TestSchemaQueryFilterer(t *testing.T) { OptionalSubjectType: "somesubjectype", }).After(toCursor(tuple.MustParse("someresourcetype:foo#viewer@user:bar")), options.BySubject) }, - "SELECT * WHERE ((subject_ns = ?)) AND (subject_object_id,ns,object_id,relation,subject_relation) > (?,?,?,?,?)", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND ((subject_ns = ?)) AND (subject_object_id,ns,object_id,relation,subject_relation) > (?,?,?,?,?)", []any{"somesubjectype", "bar", "someresourcetype", "foo", "viewer", "..."}, map[string]int{ "subject_ns": 1, @@ -639,7 +639,7 @@ func TestSchemaQueryFilterer(t *testing.T) { OptionalSubjectIds: []string{"foo"}, }).After(toCursor(tuple.MustParse("someresourcetype:someresource#viewer@user:bar")), options.BySubject) }, - "SELECT * WHERE ((subject_ns = ? AND subject_object_id IN (?))) AND (ns,object_id,relation,subject_relation) > (?,?,?,?)", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND ((subject_ns = ? AND subject_object_id IN (?))) AND (ns,object_id,relation,subject_relation) > (?,?,?,?)", []any{"somesubjectype", "foo", "someresourcetype", "someresource", "viewer", "..."}, map[string]int{"subject_ns": 1, "subject_object_id": 1}, }, @@ -651,7 +651,7 @@ func TestSchemaQueryFilterer(t *testing.T) { OptionalSubjectIds: []string{"foo", "bar"}, }).After(toCursor(tuple.MustParse("someresourcetype:someresource#viewer@user:next")), options.BySubject) }, - "SELECT * WHERE ((subject_ns = ? AND subject_object_id IN (?,?))) AND (subject_object_id,ns,object_id,relation,subject_relation) > (?,?,?,?,?)", + "SELECT * WHERE (expiration IS NULL OR expiration > NOW()) AND ((subject_ns = ? AND subject_object_id IN (?,?))) AND (subject_object_id,ns,object_id,relation,subject_relation) > (?,?,?,?,?)", []any{"somesubjectype", "foo", "bar", "next", "someresourcetype", "someresource", "viewer", "..."}, map[string]int{"subject_ns": 1, "subject_object_id": 2}, }, From d269224c94f657d62f4d4a2649fdf3616b700417 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Wed, 4 Dec 2024 14:00:52 -0500 Subject: [PATCH 34/39] Move constant into const --- pkg/schemadsl/compiler/compiler.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/schemadsl/compiler/compiler.go b/pkg/schemadsl/compiler/compiler.go index 99af7af50a..618f7deabb 100644 --- a/pkg/schemadsl/compiler/compiler.go +++ b/pkg/schemadsl/compiler/compiler.go @@ -70,10 +70,12 @@ func AllowUnprefixedObjectType() ObjectPrefixOption { return func(cfg *config) { cfg.objectTypePrefix = new(string) } } +const expirationFlag = "expiration" + func DisallowExpirationFlag() Option { return func(cfg *config) { cfg.allowedFlags = slices.Filter([]string{}, cfg.allowedFlags, func(s string) bool { - return s != "expiration" + return s != expirationFlag }) } } @@ -89,7 +91,7 @@ func Compile(schema InputSchema, prefix ObjectPrefixOption, opts ...Option) (*Co } // Enable `expiration` flag by default. - cfg.allowedFlags = append(cfg.allowedFlags, "expiration") + cfg.allowedFlags = append(cfg.allowedFlags, expirationFlag) prefix(cfg) // required option From 9866059b01bb82d3482bc1fcc4702f72df274413 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Wed, 4 Dec 2024 14:06:26 -0500 Subject: [PATCH 35/39] Add arrow expiration consistency test --- .../testconfigs/arrowoverexpiration.yaml | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 internal/services/integrationtesting/testconfigs/arrowoverexpiration.yaml diff --git a/internal/services/integrationtesting/testconfigs/arrowoverexpiration.yaml b/internal/services/integrationtesting/testconfigs/arrowoverexpiration.yaml new file mode 100644 index 0000000000..7e4cd98966 --- /dev/null +++ b/internal/services/integrationtesting/testconfigs/arrowoverexpiration.yaml @@ -0,0 +1,35 @@ +--- +schema: |+ + use expiration + + definition user {} + + definition folder { + relation viewer: user with expiration + permission view = viewer + } + + definition document { + relation parent: folder with expiration + permission view = parent->view + } + +relationships: >- + document:firstdoc#parent@folder:expired[expiration:2023-12-01T00:00:00Z] + + document:firstdoc#parent@folder:notexpired[expiration:2323-12-01T00:00:00Z] + + folder:expired#viewer@user:tom[expiration:2023-12-01T00:00:00Z] + + folder:expired#viewer@user:fred[expiration:2223-12-01T00:00:00Z] + + folder:notexpired#viewer@user:sarah[expiration:2023-12-01T00:00:00Z] + + folder:notexpired#viewer@user:tracy[expiration:2223-12-01T00:00:00Z] +assertions: + assertTrue: + - "document:firstdoc#view@user:tracy" + assertFalse: + - "document:firstdoc#view@user:tom" + - "document:firstdoc#view@user:fred" + - "document:firstdoc#view@user:sarah" From 18c06956f2768e4f0726274ef44c9f14afaf5abb Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Wed, 4 Dec 2024 14:18:32 -0500 Subject: [PATCH 36/39] Remove new indexes, as per internal discussion to revisit later --- .../zz_migration.0009_add_expiration_support.go | 8 -------- .../migrations/zz_migration.0010_add_expiration.go | 1 + .../zz_migration.0021_add_expiration_support.go | 11 +---------- 3 files changed, 2 insertions(+), 18 deletions(-) diff --git a/internal/datastore/crdb/migrations/zz_migration.0009_add_expiration_support.go b/internal/datastore/crdb/migrations/zz_migration.0009_add_expiration_support.go index 2bfea2a2e9..3b668652a5 100644 --- a/internal/datastore/crdb/migrations/zz_migration.0009_add_expiration_support.go +++ b/internal/datastore/crdb/migrations/zz_migration.0009_add_expiration_support.go @@ -24,8 +24,6 @@ const ( ALTER TABLE relation_tuple_with_integrity SET (ttl_expiration_expression = 'expires_at', ttl_job_cron = '@daily'); ` - - createExpirationTupleIndex = `CREATE INDEX ix_relation_tuple_with_expiration ON relation_tuple (namespace, relation, object_id, expires_at, userset_namespace, userset_object_id, userset_relation) STORING (caveat_name, caveat_context);` ) func init() { @@ -42,12 +40,6 @@ func addExpirationSupport(ctx context.Context, conn *pgx.Conn) error { return err } - // Add the expiration index. - _, err = conn.Exec(ctx, createExpirationTupleIndex) - if err != nil { - return err - } - // Add the TTL policy to relation_tuple, if supported. row := conn.QueryRow(ctx, "select version()") var fullVersionString string diff --git a/internal/datastore/mysql/migrations/zz_migration.0010_add_expiration.go b/internal/datastore/mysql/migrations/zz_migration.0010_add_expiration.go index 4a98fa41ec..32f30583ba 100644 --- a/internal/datastore/mysql/migrations/zz_migration.0010_add_expiration.go +++ b/internal/datastore/mysql/migrations/zz_migration.0010_add_expiration.go @@ -9,6 +9,7 @@ func addExpirationToRelationTupleTable(t *tables) string { ) } +// Used for cleaning up expired relationships. func addExpiredRelationshipsIndex(t *tables) string { return fmt.Sprintf(`CREATE INDEX ix_%s_expired ON %s (expiration);`, t.RelationTuple(), diff --git a/internal/datastore/postgres/migrations/zz_migration.0021_add_expiration_support.go b/internal/datastore/postgres/migrations/zz_migration.0021_add_expiration_support.go index 8f6967c2f2..eacdd734ba 100644 --- a/internal/datastore/postgres/migrations/zz_migration.0021_add_expiration_support.go +++ b/internal/datastore/postgres/migrations/zz_migration.0021_add_expiration_support.go @@ -12,12 +12,7 @@ const addExpirationColumn = ` ADD COLUMN expiration TIMESTAMPTZ DEFAULT NULL; ` -const addWithExpirationCoveringIndex = `CREATE INDEX CONCURRENTLY - IF NOT EXISTS ix_relation_tuple_with_expiration - ON relation_tuple (namespace, relation, object_id, created_xid, expiration) - INCLUDE (userset_namespace, userset_object_id, userset_relation, caveat_name, caveat_context) - WHERE deleted_xid = '9223372036854775807'::xid8;` - +// Used for cleaning up expired relationships. const addExpiredRelationshipsIndex = `CREATE INDEX CONCURRENTLY IF NOT EXISTS ix_relation_tuple_expired ON relation_tuple (expiration) @@ -31,10 +26,6 @@ func init() { return fmt.Errorf("failed to add expiration column to relation tuple table: %w", err) } - if _, err := conn.Exec(ctx, addWithExpirationCoveringIndex); err != nil { - return fmt.Errorf("failed to add expiration column to relation tuple table: %w", err) - } - if _, err := conn.Exec(ctx, addExpiredRelationshipsIndex); err != nil { return fmt.Errorf("failed to add expiration column to relation tuple table: %w", err) } From ca3e11486663b156aebd9cc2cc2cc4cd61a62553 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Thu, 5 Dec 2024 11:49:26 -0500 Subject: [PATCH 37/39] Remove RelationDoesNotAllowCaveatsForSubject as it is unused now --- pkg/typesystem/typesystem.go | 37 ------------------------------- pkg/typesystem/typesystem_test.go | 34 ++++++++++++++-------------- 2 files changed, 17 insertions(+), 54 deletions(-) diff --git a/pkg/typesystem/typesystem.go b/pkg/typesystem/typesystem.go index 481c55efd9..23792b9f61 100644 --- a/pkg/typesystem/typesystem.go +++ b/pkg/typesystem/typesystem.go @@ -731,43 +731,6 @@ func (nts *TypeSystem) RelationDoesNotAllowCaveatsOrTraitsForSubject(relationNam return true, nil } -// RelationDoesNotAllowCaveatsForSubject returns true if and only if it can be conclusively determined that -// the given subject type does not accept any caveats on the given relation. If the relation does not have type information, -// returns an error. -func (nts *TypeSystem) RelationDoesNotAllowCaveatsForSubject(relationName string, subjectTypeName string) (bool, error) { - relation, ok := nts.relationMap[relationName] - if !ok { - return false, NewRelationNotFoundErr(nts.nsDef.Name, relationName) - } - - typeInfo := relation.GetTypeInformation() - if typeInfo == nil { - return false, NewTypeWithSourceError( - fmt.Errorf("relation `%s` does not have type information", relationName), - relation, relationName, - ) - } - - foundSubjectType := false - for _, allowedRelation := range typeInfo.GetAllowedDirectRelations() { - if allowedRelation.GetNamespace() == subjectTypeName { - foundSubjectType = true - if allowedRelation.GetRequiredCaveat() != nil && allowedRelation.GetRequiredCaveat().CaveatName != "" { - return false, nil - } - } - } - - if !foundSubjectType { - return false, NewTypeWithSourceError( - fmt.Errorf("relation `%s` does not allow subject type `%s`", relationName, subjectTypeName), - relation, relationName, - ) - } - - return true, nil -} - // ValidatedNamespaceTypeSystem is validated type system for a namespace. type ValidatedNamespaceTypeSystem struct { *TypeSystem diff --git a/pkg/typesystem/typesystem_test.go b/pkg/typesystem/typesystem_test.go index 4b00ae13aa..3f576b38da 100644 --- a/pkg/typesystem/typesystem_test.go +++ b/pkg/typesystem/typesystem_test.go @@ -528,12 +528,12 @@ func TestTypeSystemAccessors(t *testing.T) { require.True(t, vts.IsPermission("edit")) }) - t.Run("RelationDoesNotAllowCaveatsForSubject", func(t *testing.T) { - ok, err := vts.RelationDoesNotAllowCaveatsForSubject("viewer", "user") + t.Run("RelationDoesNotAllowCaveatsOrTraitsForSubject", func(t *testing.T) { + ok, err := vts.RelationDoesNotAllowCaveatsOrTraitsForSubject("viewer", "user") require.NoError(t, err) require.True(t, ok) - ok, err = vts.RelationDoesNotAllowCaveatsForSubject("editor", "user") + ok, err = vts.RelationDoesNotAllowCaveatsOrTraitsForSubject("editor", "user") require.NoError(t, err) require.True(t, ok) }) @@ -620,12 +620,12 @@ func TestTypeSystemAccessors(t *testing.T) { require.True(t, vts.IsPermission("view")) }) - t.Run("RelationDoesNotAllowCaveatsForSubject", func(t *testing.T) { - ok, err := vts.RelationDoesNotAllowCaveatsForSubject("viewer", "user") + t.Run("RelationDoesNotAllowCaveatsOrTraitsForSubject", func(t *testing.T) { + ok, err := vts.RelationDoesNotAllowCaveatsOrTraitsForSubject("viewer", "user") require.NoError(t, err) require.True(t, ok) - ok, err = vts.RelationDoesNotAllowCaveatsForSubject("editor", "user") + ok, err = vts.RelationDoesNotAllowCaveatsOrTraitsForSubject("editor", "user") require.NoError(t, err) require.True(t, ok) }) @@ -697,12 +697,12 @@ func TestTypeSystemAccessors(t *testing.T) { require.False(t, vts.IsPermission("member")) }) - t.Run("RelationDoesNotAllowCaveatsForSubject", func(t *testing.T) { - ok, err := vts.RelationDoesNotAllowCaveatsForSubject("member", "user") + t.Run("RelationDoesNotAllowCaveatsOrTraitsForSubject", func(t *testing.T) { + ok, err := vts.RelationDoesNotAllowCaveatsOrTraitsForSubject("member", "user") require.NoError(t, err) require.True(t, ok) - ok, err = vts.RelationDoesNotAllowCaveatsForSubject("member", "group") + ok, err = vts.RelationDoesNotAllowCaveatsOrTraitsForSubject("member", "group") require.NoError(t, err) require.True(t, ok) }) @@ -776,16 +776,16 @@ func TestTypeSystemAccessors(t *testing.T) { require.False(t, vts.IsPermission("onlycaveated")) }) - t.Run("RelationDoesNotAllowCaveatsForSubject", func(t *testing.T) { - ok, err := vts.RelationDoesNotAllowCaveatsForSubject("viewer", "user") + t.Run("RelationDoesNotAllowCaveatsOrTraitsForSubject", func(t *testing.T) { + ok, err := vts.RelationDoesNotAllowCaveatsOrTraitsForSubject("viewer", "user") require.NoError(t, err) require.False(t, ok) - ok, err = vts.RelationDoesNotAllowCaveatsForSubject("editor", "user") + ok, err = vts.RelationDoesNotAllowCaveatsOrTraitsForSubject("editor", "user") require.NoError(t, err) require.True(t, ok) - ok, err = vts.RelationDoesNotAllowCaveatsForSubject("onlycaveated", "user") + ok, err = vts.RelationDoesNotAllowCaveatsOrTraitsForSubject("onlycaveated", "user") require.NoError(t, err) require.False(t, ok) }) @@ -869,12 +869,12 @@ func TestTypeSystemAccessors(t *testing.T) { require.False(t, vts.IsPermission("viewer")) }) - t.Run("RelationDoesNotAllowCaveatsForSubject", func(t *testing.T) { - ok, err := vts.RelationDoesNotAllowCaveatsForSubject("editor", "user") + t.Run("RelationDoesNotAllowCaveatsOrTraitsForSubject", func(t *testing.T) { + ok, err := vts.RelationDoesNotAllowCaveatsOrTraitsForSubject("editor", "user") require.NoError(t, err) - require.True(t, ok) + require.False(t, ok) - ok, err = vts.RelationDoesNotAllowCaveatsForSubject("viewer", "user") + ok, err = vts.RelationDoesNotAllowCaveatsOrTraitsForSubject("viewer", "user") require.NoError(t, err) require.False(t, ok) }) From 1f5368d2ece1f775a08232db8da8afbb120caf8e Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Thu, 5 Dec 2024 11:57:35 -0500 Subject: [PATCH 38/39] Switch rel expiration to use the datastore's clock --- internal/datastore/mysql/gc.go | 7 ++++++- internal/datastore/postgres/gc.go | 7 ++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/internal/datastore/mysql/gc.go b/internal/datastore/mysql/gc.go index 88346139c4..75d7b68213 100644 --- a/internal/datastore/mysql/gc.go +++ b/internal/datastore/mysql/gc.go @@ -100,10 +100,15 @@ func (mds *Datastore) DeleteBeforeTx( } func (mds *Datastore) DeleteExpiredRels(ctx context.Context) (int64, error) { + now, err := mds.Now(ctx) + if err != nil { + return -1, err + } + return mds.batchDelete( ctx, mds.driver.RelationTuple(), - sq.Lt{colExpiration: time.Now().Add(-1 * mds.gcWindow)}, + sq.Lt{colExpiration: now.Add(-1 * mds.gcWindow)}, ) } diff --git a/internal/datastore/postgres/gc.go b/internal/datastore/postgres/gc.go index 9c68c59a7a..c1e1846923 100644 --- a/internal/datastore/postgres/gc.go +++ b/internal/datastore/postgres/gc.go @@ -69,11 +69,16 @@ func (pgd *pgDatastore) TxIDBefore(ctx context.Context, before time.Time) (datas } func (pgd *pgDatastore) DeleteExpiredRels(ctx context.Context) (int64, error) { + now, err := pgd.Now(ctx) + if err != nil { + return -1, err + } + return pgd.batchDelete( ctx, tableTuple, gcPKCols, - sq.Lt{colExpiration: time.Now().Add(-1 * pgd.gcWindow)}, + sq.Lt{colExpiration: now.Add(-1 * pgd.gcWindow)}, ) } From d749ed4797e38b721e2ee75a5ca5316338751b26 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Thu, 5 Dec 2024 12:16:25 -0500 Subject: [PATCH 39/39] Switch number of rels returned on error --- internal/datastore/mysql/gc.go | 2 +- internal/datastore/postgres/gc.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/datastore/mysql/gc.go b/internal/datastore/mysql/gc.go index 75d7b68213..bf2c04e993 100644 --- a/internal/datastore/mysql/gc.go +++ b/internal/datastore/mysql/gc.go @@ -102,7 +102,7 @@ func (mds *Datastore) DeleteBeforeTx( func (mds *Datastore) DeleteExpiredRels(ctx context.Context) (int64, error) { now, err := mds.Now(ctx) if err != nil { - return -1, err + return 0, err } return mds.batchDelete( diff --git a/internal/datastore/postgres/gc.go b/internal/datastore/postgres/gc.go index c1e1846923..769859646e 100644 --- a/internal/datastore/postgres/gc.go +++ b/internal/datastore/postgres/gc.go @@ -71,7 +71,7 @@ func (pgd *pgDatastore) TxIDBefore(ctx context.Context, before time.Time) (datas func (pgd *pgDatastore) DeleteExpiredRels(ctx context.Context) (int64, error) { now, err := pgd.Now(ctx) if err != nil { - return -1, err + return 0, err } return pgd.batchDelete(