Merge pull request #2099 from authzed/remove-lookup-resources-v1

Remove LookupResources v1 implementation

internal/services/integrationtesting/consistency_test.go

@@ -60,15 +60,10 @@ func TestConsistency(t *testing.T) {
 				dispatcherKind := dispatcherKind
 
 				t.Run(dispatcherKind, func(t *testing.T) {
-					for _, useLRV2 := range []bool{false, true} {
-						useLRV2 := useLRV2
-						t.Run(fmt.Sprintf("lrv2-%t", useLRV2), func(t *testing.T) {
-							for _, chunkSize := range []uint16{5, 10} {
-								t.Run(fmt.Sprintf("lrv2-%t", useLRV2), func(t *testing.T) {
-									t.Parallel()
-									runConsistencyTestSuiteForFile(t, filePath, dispatcherKind == "caching", chunkSize, useLRV2)
-								})
-							}
+					for _, chunkSize := range []uint16{5, 10} {
+						t.Run(fmt.Sprintf("chunk-size-%d", chunkSize), func(t *testing.T) {
+							t.Parallel()
+							runConsistencyTestSuiteForFile(t, filePath, dispatcherKind == "caching", chunkSize)
 						})
 					}
 				})
@@ -77,11 +72,8 @@ func TestConsistency(t *testing.T) {
 	}
 }
 
-func runConsistencyTestSuiteForFile(t *testing.T, filePath string, useCachingDispatcher bool, chunkSize uint16, useLRV2 bool) {
+func runConsistencyTestSuiteForFile(t *testing.T, filePath string, useCachingDispatcher bool, chunkSize uint16) {
 	options := []server.ConfigOption{server.WithDispatchChunkSize(chunkSize)}
-	if useLRV2 {
-		options = append(options, server.WithEnableExperimentalLookupResources(true))
-	}
 
 	cad := consistencytestutil.LoadDataAndCreateClusterForTesting(t, filePath, testTimedelta, options...)
 

internal/services/v1/permissions.go

@@ -396,155 +396,15 @@ func TranslateExpansionTree(node *core.RelationTupleTreeNode) *v1.PermissionRela
 const lrv2CursorFlag = "lrv2"
 
 func (ps *permissionServer) LookupResources(req *v1.LookupResourcesRequest, resp v1.PermissionsService_LookupResourcesServer) error {
-	// If the cursor specifies that this is a LookupResources2 request, then that implementation must
-	// be used.
+	// NOTE: LRv2 is the only valid option, and we'll expect that all cursors include that flag.
+	// This is to preserve backward-compatibility in the meantime.
 	if req.OptionalCursor != nil {
-		_, ok, err := cursor.GetCursorFlag(req.OptionalCursor, lrv2CursorFlag)
+		_, _, err := cursor.GetCursorFlag(req.OptionalCursor, lrv2CursorFlag)
 		if err != nil {
 			return ps.rewriteError(resp.Context(), err)
 		}
-
-		if ok {
-			return ps.lookupResources2(req, resp)
-		}
-	}
-
-	if ps.config.UseExperimentalLookupResources2 {
-		return ps.lookupResources2(req, resp)
-	}
-
-	return ps.lookupResources1(req, resp)
-}
-
-func (ps *permissionServer) lookupResources1(req *v1.LookupResourcesRequest, resp v1.PermissionsService_LookupResourcesServer) error {
-	if req.OptionalLimit > 0 && req.OptionalLimit > ps.config.MaxLookupResourcesLimit {
-		return ps.rewriteError(resp.Context(), NewExceedsMaximumLimitErr(uint64(req.OptionalLimit), uint64(ps.config.MaxLookupResourcesLimit)))
-	}
-
-	ctx := resp.Context()
-
-	atRevision, revisionReadAt, err := consistency.RevisionFromContext(ctx)
-	if err != nil {
-		return ps.rewriteError(ctx, err)
-	}
-
-	ds := datastoremw.MustFromContext(ctx).SnapshotReader(atRevision)
-
-	if err := namespace.CheckNamespaceAndRelations(ctx,
-		[]namespace.TypeAndRelationToCheck{
-			{
-				NamespaceName: req.ResourceObjectType,
-				RelationName:  req.Permission,
-				AllowEllipsis: false,
-			},
-			{
-				NamespaceName: req.Subject.Object.ObjectType,
-				RelationName:  normalizeSubjectRelation(req.Subject),
-				AllowEllipsis: true,
-			},
-		}, ds); err != nil {
-		return ps.rewriteError(ctx, err)
-	}
-
-	respMetadata := &dispatch.ResponseMeta{
-		DispatchCount:       1,
-		CachedDispatchCount: 0,
-		DepthRequired:       1,
-		DebugInfo:           nil,
-	}
-	usagemetrics.SetInContext(ctx, respMetadata)
-
-	var currentCursor *dispatch.Cursor
-
-	lrRequestHash, err := computeLRRequestHash(req)
-	if err != nil {
-		return ps.rewriteError(ctx, err)
-	}
-
-	if req.OptionalCursor != nil {
-		decodedCursor, _, err := cursor.DecodeToDispatchCursor(req.OptionalCursor, lrRequestHash)
-		if err != nil {
-			return ps.rewriteError(ctx, err)
-		}
-		currentCursor = decodedCursor
-	}
-
-	alreadyPublishedPermissionedResourceIds := map[string]struct{}{}
-
-	stream := dispatchpkg.NewHandlingDispatchStream(ctx, func(result *dispatch.DispatchLookupResourcesResponse) error {
-		found := result.ResolvedResource
-
-		dispatchpkg.AddResponseMetadata(respMetadata, result.Metadata)
-		currentCursor = result.AfterResponseCursor
-
-		var partial *v1.PartialCaveatInfo
-		permissionship := v1.LookupPermissionship_LOOKUP_PERMISSIONSHIP_HAS_PERMISSION
-		if found.Permissionship == dispatch.ResolvedResource_CONDITIONALLY_HAS_PERMISSION {
-			permissionship = v1.LookupPermissionship_LOOKUP_PERMISSIONSHIP_CONDITIONAL_PERMISSION
-			partial = &v1.PartialCaveatInfo{
-				MissingRequiredContext: found.MissingRequiredContext,
-			}
-		} else if req.OptionalLimit == 0 {
-			if _, ok := alreadyPublishedPermissionedResourceIds[found.ResourceId]; ok {
-				// Skip publishing the duplicate.
-				return nil
-			}
-
-			alreadyPublishedPermissionedResourceIds[found.ResourceId] = struct{}{}
-		}
-
-		encodedCursor, err := cursor.EncodeFromDispatchCursor(result.AfterResponseCursor, lrRequestHash, atRevision, nil)
-		if err != nil {
-			return ps.rewriteError(ctx, err)
-		}
-
-		err = resp.Send(&v1.LookupResourcesResponse{
-			LookedUpAt:        revisionReadAt,
-			ResourceObjectId:  found.ResourceId,
-			Permissionship:    permissionship,
-			PartialCaveatInfo: partial,
-			AfterResultCursor: encodedCursor,
-		})
-		if err != nil {
-			return err
-		}
-		return nil
-	})
-
-	bf, err := dispatch.NewTraversalBloomFilter(uint(ps.config.MaximumAPIDepth))
-	if err != nil {
-		return err
 	}
 
-	err = ps.dispatch.DispatchLookupResources(
-		&dispatch.DispatchLookupResourcesRequest{
-			Metadata: &dispatch.ResolverMeta{
-				AtRevision:     atRevision.String(),
-				DepthRemaining: ps.config.MaximumAPIDepth,
-				TraversalBloom: bf,
-			},
-			ObjectRelation: &core.RelationReference{
-				Namespace: req.ResourceObjectType,
-				Relation:  req.Permission,
-			},
-			Subject: &core.ObjectAndRelation{
-				Namespace: req.Subject.Object.ObjectType,
-				ObjectId:  req.Subject.Object.ObjectId,
-				Relation:  normalizeSubjectRelation(req.Subject),
-			},
-			Context:        req.Context,
-			OptionalCursor: currentCursor,
-			OptionalLimit:  req.OptionalLimit,
-		},
-		stream)
-	if err != nil {
-		return ps.rewriteError(ctx, err)
-	}
-
-	return nil
-}
-
-func (ps *permissionServer) lookupResources2(req *v1.LookupResourcesRequest, resp v1.PermissionsService_LookupResourcesServer) error {
 	if req.OptionalLimit > 0 && req.OptionalLimit > ps.config.MaxLookupResourcesLimit {
 		return ps.rewriteError(resp.Context(), NewExceedsMaximumLimitErr(uint64(req.OptionalLimit), uint64(ps.config.MaxLookupResourcesLimit)))
 	}

internal/services/v1/permissions_test.go

@@ -637,11 +637,10 @@ func TestLookupResources(t *testing.T) {
 								memdb.DisableGC,
 								true,
 								testserver.ServerConfig{
-									MaxUpdatesPerWrite:              1000,
-									MaxPreconditionsCount:           1000,
-									StreamingAPITimeout:             30 * time.Second,
-									MaxRelationshipContextSize:      25000,
-									UseExperimentalLookupResources2: useV2,
+									MaxUpdatesPerWrite:         1000,
+									MaxPreconditionsCount:      1000,
+									StreamingAPITimeout:        30 * time.Second,
+									MaxRelationshipContextSize: 25000,
 								},
 								tf.StandardDatastoreWithData,
 							)

internal/services/v1/relationships.go

@@ -95,9 +95,6 @@ type PermissionsServerConfig struct {
 	// MaxBulkExportRelationshipsLimit defines the maximum number of relationships that can be
 	// exported in a single BulkExportRelationships call.
 	MaxBulkExportRelationshipsLimit uint32
-
-	// UseExperimentalLookupResources2 enables the experimental LookupResources2 API.
-	UseExperimentalLookupResources2 bool
 }
 
 // NewPermissionsServer creates a PermissionsServiceServer instance.
@@ -117,7 +114,6 @@ func NewPermissionsServer(
 		MaxDeleteRelationshipsLimit:     defaultIfZero(config.MaxDeleteRelationshipsLimit, 1_000),
 		MaxLookupResourcesLimit:         defaultIfZero(config.MaxLookupResourcesLimit, 1_000),
 		MaxBulkExportRelationshipsLimit: defaultIfZero(config.MaxBulkExportRelationshipsLimit, 100_000),
-		UseExperimentalLookupResources2: config.UseExperimentalLookupResources2,
 		DispatchChunkSize:               defaultIfZero(config.DispatchChunkSize, 100),
 	}
 

internal/testserver/server.go

@@ -21,19 +21,17 @@ import (
 
 // ServerConfig is configuration for the test server.
 type ServerConfig struct {
-	MaxUpdatesPerWrite              uint16
-	MaxPreconditionsCount           uint16
-	MaxRelationshipContextSize      int
-	StreamingAPITimeout             time.Duration
-	UseExperimentalLookupResources2 bool
+	MaxUpdatesPerWrite         uint16
+	MaxPreconditionsCount      uint16
+	MaxRelationshipContextSize int
+	StreamingAPITimeout        time.Duration
 }
 
 var DefaultTestServerConfig = ServerConfig{
-	MaxUpdatesPerWrite:              1000,
-	MaxPreconditionsCount:           1000,
-	StreamingAPITimeout:             30 * time.Second,
-	MaxRelationshipContextSize:      25000,
-	UseExperimentalLookupResources2: true,
+	MaxUpdatesPerWrite:         1000,
+	MaxPreconditionsCount:      1000,
+	StreamingAPITimeout:        30 * time.Second,
+	MaxRelationshipContextSize: 25000,
 }
 
 // NewTestServer creates a new test server, using defaults for the config.
@@ -92,7 +90,6 @@ func NewTestServerWithConfigAndDatastore(require *require.Assertions,
 		server.WithHTTPGateway(util.HTTPServerConfig{HTTPEnabled: false}),
 		server.WithMetricsAPI(util.HTTPServerConfig{HTTPEnabled: false}),
 		server.WithDispatchServer(util.GRPCServerConfig{Enabled: false}),
-		server.WithEnableExperimentalLookupResources(config.UseExperimentalLookupResources2),
 		server.SetUnaryMiddlewareModification([]server.MiddlewareModification[grpc.UnaryServerInterceptor]{
 			{
 				Operation: server.OperationReplaceAllUnsafe,

pkg/cmd/serve.go

@@ -8,6 +8,7 @@ import (
 	"github.com/fatih/color"
 	"github.com/jzelinskie/cobrautil/v2"
 	"github.com/jzelinskie/cobrautil/v2/cobraotel"
+	"github.com/rs/zerolog/log"
 	"github.com/spf13/cobra"
 
 	"github.com/authzed/spicedb/internal/telemetry"
@@ -159,6 +160,15 @@ func RegisterServeFlags(cmd *cobra.Command, config *server.Config) error {
 	experimentalFlags := nfs.FlagSet(BoldBlue("Experimental"))
 	// Flags for experimental features
 	experimentalFlags.BoolVar(&config.EnableExperimentalLookupResources, "enable-experimental-lookup-resources", true, "enables the experimental version of the lookup resources API")
+	if err := experimentalFlags.MarkDeprecated("enable-experimental-lookup-resources", "The old implementation of LookupResources has been removed and this flag is a no-op. This flag will be removed in the future."); err != nil {
+		return fmt.Errorf("failed to mark flag as deprecated: %w", err)
+	}
+	if !config.EnableExperimentalLookupResources {
+		log.Warn().
+			Bool("value", false).
+			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.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")

pkg/cmd/server/server.go

@@ -429,7 +429,6 @@ func (c *Config) Complete(ctx context.Context) (RunnableServer, error) {
 		MaxDeleteRelationshipsLimit:     c.MaxDeleteRelationshipsLimit,
 		MaxLookupResourcesLimit:         c.MaxLookupResourcesLimit,
 		MaxBulkExportRelationshipsLimit: c.MaxBulkExportRelationshipsLimit,
-		UseExperimentalLookupResources2: c.EnableExperimentalLookupResources,
 		DispatchChunkSize:               c.DispatchChunkSize,
 	}
 

pkg/cmd/testing.go

@@ -29,7 +29,6 @@ func RegisterTestingFlags(cmd *cobra.Command, config *testserver.Config) {
 	cmd.Flags().Uint32Var(&config.MaxDeleteRelationshipsLimit, "max-delete-relationships-limit", 1000, "maximum number of relationships that can be deleted in a single request")
 	cmd.Flags().Uint32Var(&config.MaxLookupResourcesLimit, "max-lookup-resources-limit", 1000, "maximum number of resources that can be looked up in a single request")
 	cmd.Flags().Uint32Var(&config.MaxBulkExportRelationshipsLimit, "max-bulk-export-relationships-limit", 10_000, "maximum number of relationships that can be exported in a single request")
-	cmd.Flags().BoolVar(&config.EnableExperimentalLookupResources, "enable-experimental-lookup-resources", false, "enables the experimental version of the lookup resources API")
 	util.RegisterCommonFlags(cmd)
 }
 

pkg/cmd/testserver/testserver.go

@@ -31,20 +31,19 @@ const (
 
 //go:generate go run github.com/ecordell/optgen -output zz_generated.options.go . Config
 type Config struct {
-	GRPCServer                        util.GRPCServerConfig `debugmap:"visible"`
-	ReadOnlyGRPCServer                util.GRPCServerConfig `debugmap:"visible"`
-	HTTPGateway                       util.HTTPServerConfig `debugmap:"visible"`
-	ReadOnlyHTTPGateway               util.HTTPServerConfig `debugmap:"visible"`
-	LoadConfigs                       []string              `debugmap:"visible"`
-	MaximumUpdatesPerWrite            uint16                `debugmap:"visible"`
-	MaximumPreconditionCount          uint16                `debugmap:"visible"`
-	MaxCaveatContextSize              int                   `debugmap:"visible"`
-	MaxRelationshipContextSize        int                   `debugmap:"visible"`
-	MaxReadRelationshipsLimit         uint32                `debugmap:"visible"`
-	MaxDeleteRelationshipsLimit       uint32                `debugmap:"visible"`
-	MaxLookupResourcesLimit           uint32                `debugmap:"visible"`
-	MaxBulkExportRelationshipsLimit   uint32                `debugmap:"visible"`
-	EnableExperimentalLookupResources bool                  `debugmap:"visible"`
+	GRPCServer                      util.GRPCServerConfig `debugmap:"visible"`
+	ReadOnlyGRPCServer              util.GRPCServerConfig `debugmap:"visible"`
+	HTTPGateway                     util.HTTPServerConfig `debugmap:"visible"`
+	ReadOnlyHTTPGateway             util.HTTPServerConfig `debugmap:"visible"`
+	LoadConfigs                     []string              `debugmap:"visible"`
+	MaximumUpdatesPerWrite          uint16                `debugmap:"visible"`
+	MaximumPreconditionCount        uint16                `debugmap:"visible"`
+	MaxCaveatContextSize            int                   `debugmap:"visible"`
+	MaxRelationshipContextSize      int                   `debugmap:"visible"`
+	MaxReadRelationshipsLimit       uint32                `debugmap:"visible"`
+	MaxDeleteRelationshipsLimit     uint32                `debugmap:"visible"`
+	MaxLookupResourcesLimit         uint32                `debugmap:"visible"`
+	MaxBulkExportRelationshipsLimit uint32                `debugmap:"visible"`
 }
 
 type RunnableTestServer interface {