Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

optimize reading of namespaces #342

Merged
merged 3 commits into from Dec 17, 2021
Merged

optimize reading of namespaces #342

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3b8904f
perform CheckPermission pre-flight checks in parallel
jakedt Dec 17, 2021
0905aea
deduplicate simultaneous requests for the same namespace
jakedt Dec 17, 2021
e378b66
fix testserver test to not depend on error message
jakedt Dec 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 18 additions & 12 deletions cmd/spicedb/servetesting_integration_test.go
View file
Open in desktop
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
Open in desktop
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a span here indicating the read started?

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
Open in desktop
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