diff --git a/cmd/spicedb/servetesting_integration_test.go b/cmd/spicedb/servetesting_integration_test.go index 66d324158b..6a305c9986 100644 --- a/cmd/spicedb/servetesting_integration_test.go +++ b/cmd/spicedb/servetesting_integration_test.go @@ -17,9 +17,13 @@ import ( "github.com/ory/dockertest/v3" "github.com/stretchr/testify/require" "google.golang.org/grpc" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) func TestTestServer(t *testing.T) { + require := require.New(t) + tester, err := newTester(t, &dockertest.RunOptions{ Repository: "authzed/spicedb", @@ -28,15 +32,15 @@ func TestTestServer(t *testing.T) { ExposedPorts: []string{"50051/tcp", "50052/tcp"}, }, ) - require.NoError(t, err) + require.NoError(err) defer tester.cleanup() conn, err := grpc.Dial(fmt.Sprintf("localhost:%s", tester.port), grpc.WithInsecure()) - require.NoError(t, err) + require.NoError(err) defer conn.Close() roConn, err := grpc.Dial(fmt.Sprintf("localhost:%s", tester.readonlyPort), grpc.WithInsecure()) - require.NoError(t, err) + require.NoError(err) defer roConn.Close() v0client := v0.NewACLServiceClient(conn) @@ -67,7 +71,7 @@ func TestTestServer(t *testing.T) { }, }, }) - require.Equal(t, "rpc error: code = Unavailable desc = service read-only", err.Error()) + require.Equal("rpc error: code = Unavailable desc = service read-only", err.Error()) // Write a simple relationship. _, err = v0client.Write(context.Background(), &v0.WriteRequest{ @@ -78,7 +82,7 @@ func TestTestServer(t *testing.T) { }, }, }) - require.NoError(t, err) + require.NoError(err) // Ensure the check succeeds. checkReq := &v1.CheckPermissionRequest{ @@ -99,22 +103,24 @@ func TestTestServer(t *testing.T) { } v1Resp, err := v1client.CheckPermission(context.Background(), checkReq) - require.NoError(t, err) - require.Equal(t, v1.CheckPermissionResponse_PERMISSIONSHIP_HAS_PERMISSION, v1Resp.Permissionship) + require.NoError(err) + require.Equal(v1.CheckPermissionResponse_PERMISSIONSHIP_HAS_PERMISSION, v1Resp.Permissionship) // Ensure check against readonly works as well. v1Resp, err = rov1client.CheckPermission(context.Background(), checkReq) - require.NoError(t, err) - require.Equal(t, v1.CheckPermissionResponse_PERMISSIONSHIP_HAS_PERMISSION, v1Resp.Permissionship) + require.NoError(err) + require.Equal(v1.CheckPermissionResponse_PERMISSIONSHIP_HAS_PERMISSION, v1Resp.Permissionship) // Try a call with a different auth header and ensure it fails. authedConn, err := grpc.Dial(fmt.Sprintf("localhost:%s", tester.readonlyPort), grpc.WithInsecure(), grpcutil.WithInsecureBearerToken("someothertoken")) - require.NoError(t, err) + require.NoError(err) defer authedConn.Close() authedv1client := v1.NewPermissionsServiceClient(authedConn) - v1Resp, err = authedv1client.CheckPermission(context.Background(), checkReq) - require.Equal(t, "rpc error: code = FailedPrecondition desc = failed precondition: object definition `resource` not found", err.Error()) + _, err = authedv1client.CheckPermission(context.Background(), checkReq) + s, ok := status.FromError(err) + require.True(ok) + require.Equal(codes.FailedPrecondition, s.Code()) } type spicedbHandle struct { diff --git a/internal/namespace/caching.go b/internal/namespace/caching.go index 3d361910d6..90e35b5742 100644 --- a/internal/namespace/caching.go +++ b/internal/namespace/caching.go @@ -9,6 +9,7 @@ import ( v0 "github.com/authzed/authzed-go/proto/authzed/api/v0" "github.com/dgraph-io/ristretto" "github.com/shopspring/decimal" + "golang.org/x/sync/singleflight" "google.golang.org/protobuf/proto" "github.com/authzed/spicedb/internal/datastore" @@ -20,9 +21,10 @@ const ( ) type cachingManager struct { - delegate datastore.Datastore - expiration time.Duration - c *ristretto.Cache + delegate datastore.Datastore + expiration time.Duration + c *ristretto.Cache + readNsGroup singleflight.Group } func cacheKey(nsName string, revision decimal.Decimal) string { @@ -47,14 +49,14 @@ func NewCachingNamespaceManager( return nil, fmt.Errorf(errInitialization, err) } - return cachingManager{ + return &cachingManager{ delegate: delegate, expiration: expiration, c: cache, }, nil } -func (nsc cachingManager) ReadNamespaceAndTypes(ctx context.Context, nsName string, revision decimal.Decimal) (*v0.NamespaceDefinition, *NamespaceTypeSystem, error) { +func (nsc *cachingManager) ReadNamespaceAndTypes(ctx context.Context, nsName string, revision decimal.Decimal) (*v0.NamespaceDefinition, *NamespaceTypeSystem, error) { nsDef, err := nsc.ReadNamespace(ctx, nsName, revision) if err != nil { return nsDef, nil, err @@ -65,18 +67,34 @@ func (nsc cachingManager) ReadNamespaceAndTypes(ctx context.Context, nsName stri return nsDef, ts, terr } -func (nsc cachingManager) ReadNamespace(ctx context.Context, nsName string, revision decimal.Decimal) (*v0.NamespaceDefinition, error) { +func (nsc *cachingManager) ReadNamespace(ctx context.Context, nsName string, revision decimal.Decimal) (*v0.NamespaceDefinition, error) { ctx, span := tracer.Start(ctx, "ReadNamespace") defer span.End() // Check the cache. - value, found := nsc.c.Get(cacheKey(nsName, revision)) + nsRevisionKey := cacheKey(nsName, revision) + value, found := nsc.c.Get(nsRevisionKey) if found { return value.(*v0.NamespaceDefinition), nil } // We couldn't use the cached entry, load one - loaded, _, err := nsc.delegate.ReadNamespace(ctx, nsName, revision) + loadedRaw, err, _ := nsc.readNsGroup.Do(nsRevisionKey, func() (interface{}, error) { + span.AddEvent("Read namespace from delegate (datastore)") + loaded, _, err := nsc.delegate.ReadNamespace(ctx, nsName, revision) + if err != nil { + return nil, err + } + + // Remove user-defined metadata. + loaded = namespace.FilterUserDefinedMetadata(loaded) + + // Save it to the cache + nsc.c.Set(cacheKey(nsName, revision), loaded, int64(proto.Size(loaded))) + span.AddEvent("Saved to cache") + + return loaded, err + }) if errors.As(err, &datastore.ErrNamespaceNotFound{}) { return nil, NewNamespaceNotFoundErr(nsName) } @@ -84,18 +102,10 @@ func (nsc cachingManager) ReadNamespace(ctx context.Context, nsName string, revi return nil, err } - // Remove user-defined metadata. - loaded = namespace.FilterUserDefinedMetadata(loaded) - - // Save it to the cache - nsc.c.Set(cacheKey(nsName, revision), loaded, int64(proto.Size(loaded))) - - span.AddEvent("Saved to cache") - - return loaded, nil + return loadedRaw.(*v0.NamespaceDefinition), nil } -func (nsc cachingManager) CheckNamespaceAndRelation(ctx context.Context, namespace, relation string, allowEllipsis bool, revision decimal.Decimal) error { +func (nsc *cachingManager) CheckNamespaceAndRelation(ctx context.Context, namespace, relation string, allowEllipsis bool, revision decimal.Decimal) error { config, err := nsc.ReadNamespace(ctx, namespace, revision) if err != nil { return err @@ -114,7 +124,7 @@ func (nsc cachingManager) CheckNamespaceAndRelation(ctx context.Context, namespa return NewRelationNotFoundErr(namespace, relation) } -func (nsc cachingManager) Close() error { +func (nsc *cachingManager) Close() error { nsc.c.Close() return nil } diff --git a/internal/services/v1/permissions.go b/internal/services/v1/permissions.go index 520fd30086..0f96c067a0 100644 --- a/internal/services/v1/permissions.go +++ b/internal/services/v1/permissions.go @@ -7,6 +7,7 @@ import ( v0 "github.com/authzed/authzed-go/proto/authzed/api/v0" v1 "github.com/authzed/authzed-go/proto/authzed/api/v1" "github.com/jzelinskie/stringz" + "golang.org/x/sync/errgroup" "github.com/authzed/spicedb/internal/graph" "github.com/authzed/spicedb/internal/middleware/consistency" @@ -17,18 +18,27 @@ import ( func (ps *permissionServer) CheckPermission(ctx context.Context, req *v1.CheckPermissionRequest) (*v1.CheckPermissionResponse, error) { atRevision, checkedAt := consistency.MustRevisionFromContext(ctx) - err := ps.nsm.CheckNamespaceAndRelation(ctx, req.Resource.ObjectType, req.Permission, false, atRevision) - if err != nil { - return nil, rewritePermissionsError(ctx, err) - } - - err = ps.nsm.CheckNamespaceAndRelation(ctx, - req.Subject.Object.ObjectType, - normalizeSubjectRelation(req.Subject), - true, - atRevision, - ) - if err != nil { + // Perform our preflight checks in parallel + errG, checksCtx := errgroup.WithContext(ctx) + errG.Go(func() error { + return ps.nsm.CheckNamespaceAndRelation( + checksCtx, + req.Resource.ObjectType, + req.Permission, + false, + atRevision, + ) + }) + errG.Go(func() error { + return ps.nsm.CheckNamespaceAndRelation( + checksCtx, + req.Subject.Object.ObjectType, + normalizeSubjectRelation(req.Subject), + true, + atRevision, + ) + }) + if err := errG.Wait(); err != nil { return nil, rewritePermissionsError(ctx, err) }