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.

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

Add API support for transaction metadata on WriteRels and DeleteRels #2084

Merged
merged 1 commit into from
Oct 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion e2e/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ module github.com/authzed/spicedb/e2e
go 1.22.7

require (
github.com/authzed/authzed-go v0.16.0
github.com/authzed/authzed-go v0.16.1-0.20241001202507-27cc182a7b92
github.com/authzed/grpcutil v0.0.0-20240123092924-129dc0a6a6e1
github.com/authzed/spicedb v1.29.5
github.com/brianvoe/gofakeit/v6 v6.23.2
Expand Down
4 changes: 2 additions & 2 deletions e2e/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ github.com/Masterminds/squirrel v1.5.4 h1:uUcX/aBc8O7Fg9kaISIUsHXdKuqehiXAMQTYX8
github.com/Masterminds/squirrel v1.5.4/go.mod h1:NNaOrjSoIDfDA40n7sr2tPNZRfjzjA400rg+riTZj10=
github.com/antlr4-go/antlr/v4 v4.13.0 h1:lxCg3LAv+EUK6t1i0y1V6/SLeUi0eKEKdhQAlS8TVTI=
github.com/antlr4-go/antlr/v4 v4.13.0/go.mod h1:pfChB/xh/Unjila75QW7+VU4TSnWnnk9UTnmpPaOR2g=
github.com/authzed/authzed-go v0.16.0 h1:QzIduF472qZcHeEraNPSpnLFFV25ZQv6u7oDufvBf3o=
github.com/authzed/authzed-go v0.16.0/go.mod h1:Cx1DQKMX38u2fFVLZiGUuZnbTo2J7LCZEXmVak+TMak=
github.com/authzed/authzed-go v0.16.1-0.20241001202507-27cc182a7b92 h1:9BQQO4ga8naS5bl2aWeRoyF54Bxp1H28WMr+/XU7rLE=
github.com/authzed/authzed-go v0.16.1-0.20241001202507-27cc182a7b92/go.mod h1:Cx1DQKMX38u2fFVLZiGUuZnbTo2J7LCZEXmVak+TMak=
github.com/authzed/cel-go v0.20.2 h1:GlmLecGry7Z8HU0k+hmaHHUV05ZHrsFxduXHtIePvck=
github.com/authzed/cel-go v0.20.2/go.mod h1:pJHVFWbqUHV1J+klQoZubdKswlbxcsbojda3mye9kiU=
github.com/authzed/grpcutil v0.0.0-20240123092924-129dc0a6a6e1 h1:zBfQzia6Hz45pJBeURTrv1b6HezmejB6UmiGuBilHZM=
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ require (
github.com/IBM/pgxpoolprometheus v1.1.1
github.com/KimMachineGun/automemlimit v0.6.1
github.com/Masterminds/squirrel v1.5.4
github.com/authzed/authzed-go v0.16.0
github.com/authzed/authzed-go v0.16.1-0.20241001202507-27cc182a7b92

// NOTE: We are using a *copy* of `cel-go` here to ensure there isn't a conflict
// with the version used in Kubernetes. This is a temporary measure until we can
Expand Down
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -706,8 +706,8 @@ github.com/ashanbrown/forbidigo v1.6.0 h1:D3aewfM37Yb3pxHujIPSpTf6oQk9sc9WZi8ger
github.com/ashanbrown/forbidigo v1.6.0/go.mod h1:Y8j9jy9ZYAEHXdu723cUlraTqbzjKF1MUyfOKL+AjcU=
github.com/ashanbrown/makezero v1.1.1 h1:iCQ87C0V0vSyO+M9E/FZYbu65auqH0lnsOkf5FcB28s=
github.com/ashanbrown/makezero v1.1.1/go.mod h1:i1bJLCRSCHOcOa9Y6MyF2FTfMZMFdHvxKHxgO5Z1axI=
github.com/authzed/authzed-go v0.16.0 h1:QzIduF472qZcHeEraNPSpnLFFV25ZQv6u7oDufvBf3o=
github.com/authzed/authzed-go v0.16.0/go.mod h1:Cx1DQKMX38u2fFVLZiGUuZnbTo2J7LCZEXmVak+TMak=
github.com/authzed/authzed-go v0.16.1-0.20241001202507-27cc182a7b92 h1:9BQQO4ga8naS5bl2aWeRoyF54Bxp1H28WMr+/XU7rLE=
github.com/authzed/authzed-go v0.16.1-0.20241001202507-27cc182a7b92/go.mod h1:Cx1DQKMX38u2fFVLZiGUuZnbTo2J7LCZEXmVak+TMak=
github.com/authzed/cel-go v0.20.2 h1:GlmLecGry7Z8HU0k+hmaHHUV05ZHrsFxduXHtIePvck=
github.com/authzed/cel-go v0.20.2/go.mod h1:pJHVFWbqUHV1J+klQoZubdKswlbxcsbojda3mye9kiU=
github.com/authzed/consistent v0.1.0 h1:tlh1wvKoRbjRhMm2P+X5WQQyR54SRoS4MyjLOg17Mp8=
Expand Down
34 changes: 34 additions & 0 deletions internal/services/v1/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -475,3 +475,37 @@ func defaultIfZero[T comparable](value T, defaultValue T) T {
}
return value
}

// ErrTransactionMetadataTooLarge indicates that the metadata for a transaction is too large.
type ErrTransactionMetadataTooLarge struct {
error
metadataSize int
maxSize int
}

// NewTransactionMetadataTooLargeErr constructs a new transaction metadata too large error.
func NewTransactionMetadataTooLargeErr(metadataSize int, maxSize int) ErrTransactionMetadataTooLarge {
return ErrTransactionMetadataTooLarge{
error: fmt.Errorf("metadata size of %d is greater than maximum allowed of %d", metadataSize, maxSize),
metadataSize: metadataSize,
maxSize: maxSize,
}
}

func (err ErrTransactionMetadataTooLarge) MarshalZerologObject(e *zerolog.Event) {
e.Err(err.error).Int("metadataSize", err.metadataSize).Int("maxSize", err.maxSize)
}

func (err ErrTransactionMetadataTooLarge) GRPCStatus() *status.Status {
return spiceerrors.WithCodeAndDetails(
err,
codes.InvalidArgument,
spiceerrors.ForReason(
v1.ErrorReason_ERROR_REASON_TRANSACTION_METADATA_TOO_LARGE,
map[string]string{
"metadata_byte_size": strconv.Itoa(err.metadataSize),
"maximum_allowed_metadata_byte_size": strconv.Itoa(err.maxSize),
},
),
)
}
32 changes: 30 additions & 2 deletions internal/services/v1/relationships.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/prometheus/client_golang/prometheus/promauto"
"go.opentelemetry.io/otel/trace"
"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/types/known/structpb"

"github.com/authzed/spicedb/internal/dispatch"
"github.com/authzed/spicedb/internal/middleware"
Expand Down Expand Up @@ -42,6 +43,8 @@ var writeUpdateCounter = promauto.NewHistogramVec(prometheus.HistogramOpts{
Buckets: []float64{0, 1, 2, 5, 10, 15, 25, 50, 100, 250, 500, 1000},
}, []string{"kind"})

const MaximumTransactionMetadataSize = 65000 // bytes. Limited by the BLOB size used in MySQL driver
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth making this datastore-specific? Or will we burn that bridge when we get to it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather set it to the low amount and reevaluate in the future if need be. "65K should be enough for anyone!"


// PermissionsServerConfig is configuration for the permissions server.
type PermissionsServerConfig struct {
// MaxUpdatesPerWrite holds the maximum number of updates allowed per
Expand Down Expand Up @@ -273,6 +276,10 @@ func (ps *permissionServer) ReadRelationships(req *v1.ReadRelationshipsRequest,
}

func (ps *permissionServer) WriteRelationships(ctx context.Context, req *v1.WriteRelationshipsRequest) (*v1.WriteRelationshipsResponse, error) {
if err := ps.validateTransactionMetadata(req.OptionalTransactionMetadata); err != nil {
return nil, ps.rewriteError(ctx, err)
}

ds := datastoremw.MustFromContext(ctx)

span := trace.SpanFromContext(ctx)
Expand Down Expand Up @@ -347,7 +354,7 @@ func (ps *permissionServer) WriteRelationships(ctx context.Context, req *v1.Writ

span.AddEvent("write relationships")
return rwt.WriteRelationships(ctx, tupleUpdates)
})
}, options.WithMetadata(req.OptionalTransactionMetadata))
if err != nil {
return nil, ps.rewriteError(ctx, err)
}
Expand All @@ -367,7 +374,28 @@ func (ps *permissionServer) WriteRelationships(ctx context.Context, req *v1.Writ
}, nil
}

func (ps *permissionServer) validateTransactionMetadata(metadata *structpb.Struct) error {
if metadata == nil {
return nil
}

b, err := metadata.MarshalJSON()
if err != nil {
return err
}

if len(b) > MaximumTransactionMetadataSize {
return NewTransactionMetadataTooLargeErr(len(b), MaximumTransactionMetadataSize)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we guaranteed that the client encoding will match the JSON encoding? i.e. will the size that that the client sees on their machine match what we're validating against here?

Copy link
Member Author

Choose a reason for hiding this comment

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

No; this is a rough estimate but since the drivers store JSON too, it should be sufficient

}

return nil
}

func (ps *permissionServer) DeleteRelationships(ctx context.Context, req *v1.DeleteRelationshipsRequest) (*v1.DeleteRelationshipsResponse, error) {
if err := ps.validateTransactionMetadata(req.OptionalTransactionMetadata); err != nil {
return nil, ps.rewriteError(ctx, err)
}

if len(req.OptionalPreconditions) > int(ps.config.MaxPreconditionsCount) {
return nil, ps.rewriteError(
ctx,
Expand Down Expand Up @@ -456,7 +484,7 @@ func (ps *permissionServer) DeleteRelationships(ctx context.Context, req *v1.Del
// Otherwise, kick off an unlimited deletion.
_, err = rwt.DeleteRelationships(ctx, req.RelationshipFilter)
return err
})
}, options.WithMetadata(req.OptionalTransactionMetadata))
if err != nil {
return nil, ps.rewriteError(ctx, err)
}
Expand Down
108 changes: 108 additions & 0 deletions internal/services/v1/relationships_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"fmt"
"io"
"maps"
"strings"
"testing"
"time"

Expand Down Expand Up @@ -1344,6 +1345,113 @@ func TestDeleteRelationshipsPreconditionsOverLimit(t *testing.T) {
require.Contains(err.Error(), "precondition count of 2 is greater than maximum allowed of 1")
}

func TestWriteRelationshipsWithMetadata(t *testing.T) {
require := require.New(t)
conn, cleanup, _, beforeWriteRev := testserver.NewTestServerWithConfig(
require,
testTimedeltas[0],
memdb.DisableGC,
true,
testserver.ServerConfig{
MaxPreconditionsCount: 10,
MaxUpdatesPerWrite: 10,
},
tf.StandardDatastoreWithData,
)
t.Cleanup(cleanup)

client := v1.NewPermissionsServiceClient(conn)

metadata, err := structpb.NewStruct(map[string]any{
"foo": "123546",
})
require.NoError(err)

_, err = client.WriteRelationships(context.Background(), &v1.WriteRelationshipsRequest{
OptionalTransactionMetadata: metadata,
Updates: []*v1.RelationshipUpdate{
{
Operation: v1.RelationshipUpdate_OPERATION_TOUCH,
Relationship: rel("document", "newdoc", "parent", "folder", "afolder", ""),
},
},
})

require.NoError(err)

beforeWriteToken := zedtoken.MustNewFromRevision(beforeWriteRev)
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()

watchClient := v1.NewWatchServiceClient(conn)

stream, err := watchClient.Watch(ctx, &v1.WatchRequest{OptionalStartCursor: beforeWriteToken})
require.NoError(err)

resp, err := stream.Recv()
require.NoError(err)
require.Equal(metadata, resp.OptionalTransactionMetadata)
}

func TestWriteRelationshipsMetadataOverLimit(t *testing.T) {
require := require.New(t)
conn, cleanup, _, _ := testserver.NewTestServerWithConfig(
require,
testTimedeltas[0],
memdb.DisableGC,
true,
testserver.ServerConfig{
MaxPreconditionsCount: 1,
MaxUpdatesPerWrite: 1,
},
tf.StandardDatastoreWithData,
)
client := v1.NewPermissionsServiceClient(conn)
t.Cleanup(cleanup)

metadata, err := structpb.NewStruct(map[string]any{
"foo": strings.Repeat("hithere", 65000),
})
require.NoError(err)

_, err = client.WriteRelationships(context.Background(), &v1.WriteRelationshipsRequest{
OptionalTransactionMetadata: metadata,
})

require.Error(err)
require.Contains(err.Error(), "metadata size of 455010 is greater than maximum allowed of 65000")
}

func TestDeleteRelationshipsMetadataOverLimit(t *testing.T) {
require := require.New(t)
conn, cleanup, _, _ := testserver.NewTestServerWithConfig(
require,
testTimedeltas[0],
memdb.DisableGC,
true,
testserver.ServerConfig{
MaxPreconditionsCount: 1,
MaxUpdatesPerWrite: 1,
},
tf.StandardDatastoreWithData,
)
client := v1.NewPermissionsServiceClient(conn)
t.Cleanup(cleanup)

metadata, err := structpb.NewStruct(map[string]any{
"foo": strings.Repeat("hithere", 65000),
})
require.NoError(err)

_, err = client.DeleteRelationships(context.Background(), &v1.DeleteRelationshipsRequest{
OptionalTransactionMetadata: metadata,
RelationshipFilter: &v1.RelationshipFilter{},
})

require.Error(err)
require.Contains(err.Error(), "metadata size of 455010 is greater than maximum allowed of 65000")
}

func TestWriteRelationshipsPreconditionsOverLimit(t *testing.T) {
require := require.New(t)
conn, cleanup, _, _ := testserver.NewTestServerWithConfig(
Expand Down
5 changes: 3 additions & 2 deletions internal/services/v1/watch.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,9 @@ func (ws *watchServer) Watch(req *v1.WatchRequest, stream v1.WatchService_WatchS
filtered := filterUpdates(objectTypes, filters, update.RelationshipChanges)
if len(filtered) > 0 {
if err := stream.Send(&v1.WatchResponse{
Updates: filtered,
ChangesThrough: zedtoken.MustNewFromRevision(update.Revision),
Updates: filtered,
ChangesThrough: zedtoken.MustNewFromRevision(update.Revision),
OptionalTransactionMetadata: update.Metadata,
}); err != nil {
return status.Errorf(codes.Canceled, "watch canceled by user: %s", err)
}
Expand Down
Loading