diff --git a/internal/dispatch/graph/check_test.go b/internal/dispatch/graph/check_test.go index c8f2b28405..31d05d0a5a 100644 --- a/internal/dispatch/graph/check_test.go +++ b/internal/dispatch/graph/check_test.go @@ -5,7 +5,6 @@ import ( "fmt" "os" "testing" - "time" v0 "github.com/authzed/authzed-go/proto/authzed/api/v0" v1_api "github.com/authzed/authzed-go/proto/authzed/api/v1" @@ -181,7 +180,7 @@ func TestMaxDepth(t *testing.T) { require.NoError(err) require.True(revision.GreaterThan(decimal.Zero)) - nsm, err := namespace.NewCachingNamespaceManager(1*time.Second, testCacheConfig) + nsm, err := namespace.NewCachingNamespaceManager(testCacheConfig) require.NoError(err) dispatch := NewLocalOnlyDispatcher(nsm) @@ -299,7 +298,7 @@ func newLocalDispatcher(require *require.Assertions) (context.Context, dispatch. ds, revision := testfixtures.StandardDatastoreWithData(rawDS, require) - nsm, err := namespace.NewCachingNamespaceManager(1*time.Second, testCacheConfig) + nsm, err := namespace.NewCachingNamespaceManager(testCacheConfig) require.NoError(err) dispatch := NewLocalOnlyDispatcher(nsm) diff --git a/internal/dispatch/graph/expand_test.go b/internal/dispatch/graph/expand_test.go index 57405a29ae..dae0174d84 100644 --- a/internal/dispatch/graph/expand_test.go +++ b/internal/dispatch/graph/expand_test.go @@ -8,7 +8,6 @@ import ( "go/token" "os" "testing" - "time" v0 "github.com/authzed/authzed-go/proto/authzed/api/v0" v1_api "github.com/authzed/authzed-go/proto/authzed/api/v1" @@ -280,7 +279,7 @@ func TestMaxDepthExpand(t *testing.T) { require.True(revision.GreaterThan(decimal.Zero)) require.NoError(datastoremw.SetInContext(ctx, ds)) - nsm, err := namespace.NewCachingNamespaceManager(1*time.Second, testCacheConfig) + nsm, err := namespace.NewCachingNamespaceManager(testCacheConfig) require.NoError(err) dispatch := NewLocalOnlyDispatcher(nsm) diff --git a/internal/dispatch/graph/lookup_test.go b/internal/dispatch/graph/lookup_test.go index 98b31b6359..ab68f3c981 100644 --- a/internal/dispatch/graph/lookup_test.go +++ b/internal/dispatch/graph/lookup_test.go @@ -166,7 +166,7 @@ func TestMaxDepthLookup(t *testing.T) { ds, revision := testfixtures.StandardDatastoreWithData(rawDS, require) - nsm, err := namespace.NewCachingNamespaceManager(1*time.Second, testCacheConfig) + nsm, err := namespace.NewCachingNamespaceManager(testCacheConfig) require.NoError(err) dispatch := NewLocalOnlyDispatcher(nsm) diff --git a/internal/namespace/caching.go b/internal/namespace/caching.go index f8ab200fae..d2c65c09c3 100644 --- a/internal/namespace/caching.go +++ b/internal/namespace/caching.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "time" v0 "github.com/authzed/authzed-go/proto/authzed/api/v0" "github.com/dgraph-io/ristretto" @@ -21,14 +20,20 @@ const ( errInitialization = "unable to initialize namespace manager: %w" ) +// nsCache holds the subset of the underlying ristretto.Cache interface that the +// manager needs +type nsCache interface { + Get(key interface{}) (interface{}, bool) + Set(key, value interface{}, cost int64) bool + Close() +} + type cachingManager struct { - expiration time.Duration - c *ristretto.Cache + c nsCache readNsGroup singleflight.Group } func NewCachingNamespaceManager( - expiration time.Duration, cacheConfig *ristretto.Config, ) (Manager, error) { if cacheConfig == nil { @@ -45,11 +50,15 @@ func NewCachingNamespaceManager( } return &cachingManager{ - expiration: expiration, - c: cache, + c: cache, }, nil } +// NewNonCachingNamespaceManager returns a namespace manager that doesn't cache +func NewNonCachingNamespaceManager() Manager { + return &cachingManager{c: noCache{}} +} + 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 { @@ -67,7 +76,7 @@ func (nsc *cachingManager) ReadNamespace(ctx context.Context, nsName string, rev ds := datastoremw.MustFromContext(ctx) - // Check the cache. + // Check the nsCache. nsRevisionKey, err := ds.NamespaceCacheKey(nsName, revision) if err != nil { return nil, err @@ -88,14 +97,9 @@ func (nsc *cachingManager) ReadNamespace(ctx context.Context, nsName string, rev // Remove user-defined metadata. loaded = namespace.FilterUserDefinedMetadata(loaded) - cacheKey, err := ds.NamespaceCacheKey(nsName, revision) - if err != nil { - return nil, err - } - - // Save it to the cache - nsc.c.Set(cacheKey, loaded, int64(proto.Size(loaded))) - span.AddEvent("Saved to cache") + // Save it to the nsCache + nsc.c.Set(nsRevisionKey, loaded, int64(proto.Size(loaded))) + span.AddEvent("Saved to nsCache") return loaded, err }) @@ -132,3 +136,18 @@ func (nsc *cachingManager) Close() error { nsc.c.Close() return nil } + +// noCache is an implementation of nsCache that doesn't cache +type noCache struct{} + +var _ nsCache = noCache{} + +func (n noCache) Get(key interface{}) (interface{}, bool) { + return nil, false +} + +func (n noCache) Set(key, value interface{}, cost int64) bool { + return false +} + +func (n noCache) Close() {} diff --git a/internal/namespace/caching_test.go b/internal/namespace/caching_test.go index 2db9e19ae4..220fedbbd9 100644 --- a/internal/namespace/caching_test.go +++ b/internal/namespace/caching_test.go @@ -3,9 +3,9 @@ package namespace import ( "context" "testing" - "time" v0 "github.com/authzed/authzed-go/proto/authzed/api/v0" + "github.com/dgraph-io/ristretto" "github.com/stretchr/testify/require" "github.com/authzed/spicedb/internal/datastore/memdb" @@ -14,7 +14,7 @@ import ( ) func TestDisjointCacheKeys(t *testing.T) { - cache, err := NewCachingNamespaceManager(10*time.Second, nil) + cache, err := NewCachingNamespaceManager(nil) require.NoError(t, err) ds, err := memdb.NewMemdbDatastore(0, 0, memdb.DisableGC, 0) @@ -37,7 +37,7 @@ func TestDisjointCacheKeys(t *testing.T) { keyA, err := dsA.NamespaceCacheKey("test/user", rev) require.NoError(t, err) - rCache := cache.(*cachingManager).c + rCache := cache.(*cachingManager).c.(*ristretto.Cache) rCache.Wait() nsA, ok := rCache.Get(keyA) @@ -62,3 +62,25 @@ func TestDisjointCacheKeys(t *testing.T) { // namespaces are different require.NotEmpty(t, nsA, nsB) } + +func TestNoCache(t *testing.T) { + cache := NewNonCachingNamespaceManager() + ds, err := memdb.NewMemdbDatastore(0, 0, memdb.DisableGC, 0) + require.NoError(t, err) + + ctx := datastoremw.ContextWithDatastore(context.Background(), ds) + + // write a namespace to the "A" store + rev, err := ds.WriteNamespace(ctx, &v0.NamespaceDefinition{Name: "test/user"}) + require.NoError(t, err) + + def, err := cache.ReadNamespace(ctx, "test/user", rev) + require.NoError(t, err) + require.Equal(t, "test/user", def.Name) + + defB, err := cache.ReadNamespace(ctx, "test/user", rev) + require.NoError(t, err) + require.Equal(t, "test/user", defB.Name) + + require.Equal(t, def, defB) +} diff --git a/internal/namespace/manager.go b/internal/namespace/manager.go index 235144abda..3f497340a4 100644 --- a/internal/namespace/manager.go +++ b/internal/namespace/manager.go @@ -10,7 +10,7 @@ import ( var tracer = otel.Tracer("spicedb/internal/namespace") -// Manager is a subset of the datastore interface that can read (and possibly cache) namespaces. +// Manager is a subset of the datastore interface that can read (and possibly nsCache) namespaces. type Manager interface { // ReadNamespace reads a namespace definition and version and returns it if found. // diff --git a/internal/namespace/typesystem_test.go b/internal/namespace/typesystem_test.go index 853c03d1d8..70f6cd9cb4 100644 --- a/internal/namespace/typesystem_test.go +++ b/internal/namespace/typesystem_test.go @@ -3,7 +3,6 @@ package namespace import ( "context" "testing" - "time" v0 "github.com/authzed/authzed-go/proto/authzed/api/v0" "github.com/shopspring/decimal" @@ -223,7 +222,7 @@ func TestTypeSystem(t *testing.T) { require.NoError(err) ctx := datastoremw.ContextWithDatastore(context.Background(), ds) - nsm, err := NewCachingNamespaceManager(0*time.Second, nil) + nsm, err := NewCachingNamespaceManager(nil) require.NoError(err) var lastRevision decimal.Decimal diff --git a/internal/services/consistency_test.go b/internal/services/consistency_test.go index e16e0fc4d4..1e2680cb21 100644 --- a/internal/services/consistency_test.go +++ b/internal/services/consistency_test.go @@ -77,7 +77,7 @@ func TestConsistency(t *testing.T) { }) t.Cleanup(cleanup) - ns, err := namespace.NewCachingNamespaceManager(1*time.Second, nil) + ns, err := namespace.NewCachingNamespaceManager(nil) lrequire.NoError(err) dsCtx := datastoremw.ContextWithHandle(context.Background()) diff --git a/internal/services/v0/namespace.go b/internal/services/v0/namespace.go index 2a20102e78..909d64e8d3 100644 --- a/internal/services/v0/namespace.go +++ b/internal/services/v0/namespace.go @@ -3,7 +3,6 @@ package v0 import ( "context" "errors" - "time" v0 "github.com/authzed/authzed-go/proto/authzed/api/v0" v1 "github.com/authzed/authzed-go/proto/authzed/api/v1" @@ -43,10 +42,7 @@ func NewNamespaceServer() v0.NamespaceServiceServer { func (nss *nsServer) WriteConfig(ctx context.Context, req *v0.WriteConfigRequest) (*v0.WriteConfigResponse, error) { ds := datastoremw.MustFromContext(ctx) - nsm, err := namespace.NewCachingNamespaceManager(0*time.Second, nil) - if err != nil { - return nil, rewriteNamespaceError(ctx, err) - } + nsm := namespace.NewNonCachingNamespaceManager() readRevision, _ := consistency.MustRevisionFromContext(ctx) diff --git a/internal/services/v1alpha1/schema.go b/internal/services/v1alpha1/schema.go index 00f6434f9a..0985a3d623 100644 --- a/internal/services/v1alpha1/schema.go +++ b/internal/services/v1alpha1/schema.go @@ -98,11 +98,7 @@ func (ss *schemaServiceServer) ReadSchema(ctx context.Context, in *v1alpha1.Read func (ss *schemaServiceServer) WriteSchema(ctx context.Context, in *v1alpha1.WriteSchemaRequest) (*v1alpha1.WriteSchemaResponse, error) { log.Ctx(ctx).Trace().Str("schema", in.GetSchema()).Msg("requested Schema to be written") ds := datastoremw.MustFromContext(ctx) - - nsm, err := namespace.NewCachingNamespaceManager(0, nil) // non-caching manager - if err != nil { - return nil, rewriteError(ctx, err) - } + nsm := namespace.NewNonCachingNamespaceManager() inputSchema := compiler.InputSchema{ Source: input.Source("schema"), diff --git a/internal/testserver/server.go b/internal/testserver/server.go index 916130e9a4..c52c26091f 100644 --- a/internal/testserver/server.go +++ b/internal/testserver/server.go @@ -29,11 +29,10 @@ func NewTestServer(require *require.Assertions, emptyDS, err := memdb.NewMemdbDatastore(0, revisionFuzzingTimedelta, gcWindow, simulatedLatency) require.NoError(err) ds, revision := dsInitFunc(emptyDS, require) - ns, err := namespace.NewCachingNamespaceManager(1*time.Second, nil) + ns, err := namespace.NewCachingNamespaceManager(nil) require.NoError(err) srv, err := server.NewConfigWithOptions( server.WithDatastore(ds), - server.WithNamespaceCacheExpiration(1*time.Second), server.WithDispatcher(graph.NewLocalOnlyDispatcher(ns)), server.WithDispatchMaxDepth(50), server.WithGRPCServer(util.GRPCServerConfig{ @@ -41,7 +40,6 @@ func NewTestServer(require *require.Assertions, Enabled: true, }), server.WithSchemaPrefixesRequired(schemaPrefixRequired), - server.WithNamespaceCacheExpiration(1*time.Second), server.WithGRPCAuthFunc(func(ctx context.Context) (context.Context, error) { return ctx, nil }), diff --git a/pkg/cmd/serve.go b/pkg/cmd/serve.go index 3d565ff92b..111df706e9 100644 --- a/pkg/cmd/serve.go +++ b/pkg/cmd/serve.go @@ -26,7 +26,10 @@ func RegisterServeFlags(cmd *cobra.Command, config *server.Config) { datastore.RegisterDatastoreFlags(cmd, &config.DatastoreConfig) // Flags for the namespace manager - cmd.Flags().DurationVar(&config.NamespaceCacheExpiration, "ns-cache-expiration", 1*time.Minute, "amount of time a namespace entry should remain cached") + cmd.Flags().Duration("ns-cache-expiration", 1*time.Minute, "amount of time a namespace entry should remain cached") + if err := cmd.Flags().MarkHidden("ns-cache-expiration"); err != nil { + panic("failed to mark flag hidden: " + err.Error()) + } // Flags for parsing and validating schemas. cmd.Flags().BoolVar(&config.SchemaPrefixesRequired, "schema-prefixes-required", false, "require prefixes on all object definitions in schemas") diff --git a/pkg/cmd/server/server.go b/pkg/cmd/server/server.go index 4e7ad9a075..65b8925f50 100644 --- a/pkg/cmd/server/server.go +++ b/pkg/cmd/server/server.go @@ -48,9 +48,6 @@ type Config struct { DatastoreConfig datastorecfg.Config Datastore datastore.Datastore - // Namespace cache - NamespaceCacheExpiration time.Duration - // Schema options SchemaPrefixesRequired bool @@ -99,7 +96,7 @@ func (c *Config) Complete() (RunnableServer, error) { } } - nsm, err := namespace.NewCachingNamespaceManager(c.NamespaceCacheExpiration, nil) + nsm, err := namespace.NewCachingNamespaceManager(nil) if err != nil { return nil, fmt.Errorf("failed to create namespace manager: %w", err) } diff --git a/pkg/cmd/server/zz_generated.options.go b/pkg/cmd/server/zz_generated.options.go index e910b328b5..0cc16089e0 100644 --- a/pkg/cmd/server/zz_generated.options.go +++ b/pkg/cmd/server/zz_generated.options.go @@ -36,7 +36,6 @@ func (c *Config) ToOption() ConfigOption { to.HTTPGatewayCorsAllowedOrigins = c.HTTPGatewayCorsAllowedOrigins to.DatastoreConfig = c.DatastoreConfig to.Datastore = c.Datastore - to.NamespaceCacheExpiration = c.NamespaceCacheExpiration to.SchemaPrefixesRequired = c.SchemaPrefixesRequired to.DispatchServer = c.DispatchServer to.DispatchMaxDepth = c.DispatchMaxDepth @@ -146,13 +145,6 @@ func WithDatastore(datastore datastore1.Datastore) ConfigOption { } } -// WithNamespaceCacheExpiration returns an option that can set NamespaceCacheExpiration on a Config -func WithNamespaceCacheExpiration(namespaceCacheExpiration time.Duration) ConfigOption { - return func(c *Config) { - c.NamespaceCacheExpiration = namespaceCacheExpiration - } -} - // WithSchemaPrefixesRequired returns an option that can set SchemaPrefixesRequired on a Config func WithSchemaPrefixesRequired(schemaPrefixesRequired bool) ConfigOption { return func(c *Config) { diff --git a/pkg/cmd/testserver/testserver.go b/pkg/cmd/testserver/testserver.go index 2869307cff..ed6d99eb6b 100644 --- a/pkg/cmd/testserver/testserver.go +++ b/pkg/cmd/testserver/testserver.go @@ -3,7 +3,6 @@ package testserver import ( "context" "fmt" - "time" "github.com/rs/zerolog" "github.com/rs/zerolog/log" @@ -22,10 +21,7 @@ import ( "github.com/authzed/spicedb/pkg/cmd/util" ) -const ( - nsCacheExpiration = 0 * time.Minute // No caching - maxDepth = 50 -) +const maxDepth = 50 //go:generate go run github.com/ecordell/optgen -output zz_generated.options.go . Config type Config struct { @@ -41,7 +37,7 @@ type RunnableTestServer interface { } func (c *Config) Complete() (RunnableTestServer, error) { - nsm, err := namespace.NewCachingNamespaceManager(nsCacheExpiration, nil) + nsm, err := namespace.NewCachingNamespaceManager(nil) if err != nil { return nil, fmt.Errorf("failed to initialize namespace manager: %w", err) } diff --git a/pkg/development/devcontext.go b/pkg/development/devcontext.go index 4d7bb1abd5..763a7e1855 100644 --- a/pkg/development/devcontext.go +++ b/pkg/development/devcontext.go @@ -54,10 +54,7 @@ func NewDevContext(ctx context.Context, developerRequestContext *v0.RequestConte ctx = datastoremw.ContextWithDatastore(ctx, ds) // Instantiate the namespace manager with *no caching*. - nsm, err := namespace.NewCachingNamespaceManager(0*time.Second, nil) - if err != nil { - return nil, nil, err - } + nsm := namespace.NewNonCachingNamespaceManager() dctx, devErrs, nerr := newDevContextWithDatastore(ctx, developerRequestContext, ds, nsm) if nerr != nil || devErrs != nil {