Skip to content

Commit

Permalink
Merge pull request #342 from authzed/optimized-read-ns
Browse files Browse the repository at this point in the history 
optimize reading of namespaces
  • Loading branch information
@jakedt
jakedt committed Dec 17, 2021
2 parents 1d4a393 + e378b66 commit 4d8d76b
Show file tree
Hide file tree
Showing 3 changed files with 69 additions and 43 deletions.
30 changes: 18 additions & 12 deletions cmd/spicedb/servetesting_integration_test.go
View file
Expand Up @@ -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",
Expand All @@ -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)
Expand Down Expand Up @@ -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{
Expand All @@ -78,7 +82,7 @@ func TestTestServer(t *testing.T) {
},
},
})
require.NoError(t, err)
require.NoError(err)

// Ensure the check succeeds.
checkReq := &v1.CheckPermissionRequest{
Expand All @@ -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 {
Expand Down
48 changes: 29 additions & 19 deletions internal/namespace/caching.go
View file
Expand Up @@ -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"
Expand All @@ -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 {
Expand All @@ -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
Expand All @@ -65,37 +67,45 @@ 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)
}
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, 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
Expand All @@ -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
}
34 changes: 22 additions & 12 deletions internal/services/v1/permissions.go
View file
Expand Up @@ -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"
Expand All @@ -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)
}

Expand Down

0 comments on commit 4d8d76b

Please sign in to comment.