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

Follow up changes for recent fixes: remove len downcasts and ensure all other downcasts are validated #1780

Merged
merged 3 commits into from
Mar 6, 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
8 changes: 4 additions & 4 deletions internal/datasets/subjectset_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2598,13 +2598,13 @@ func TestIntersectConcreteWithWildcard(t *testing.T) {
// it counts in binary and "activates" input funcs that match 1s in the binary representation
// it doesn't check for overflow so don't go crazy
func allSubsets[T any](objs []T, n int) [][]T {
maxInt := uint(math.Exp2(float64(len(objs)))) - 1
maxInt := uint64(math.Exp2(float64(len(objs)))) - 1
all := make([][]T, 0)

for i := uint(0); i < maxInt; i++ {
for i := uint64(0); i < maxInt; i++ {
set := make([]T, 0, n)
for digit := uint(0); digit < uint(len(objs)); digit++ {
mask := uint(1) << digit
for digit := uint64(0); digit < uint64(len(objs)); digit++ {
mask := uint64(1) << digit
if mask&i != 0 {
set = append(set, objs[digit])
}
Expand Down
13 changes: 8 additions & 5 deletions internal/datastore/crdb/pool/balancer.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"golang.org/x/sync/semaphore"

log "github.com/authzed/spicedb/internal/logging"
"github.com/authzed/spicedb/pkg/genutil"
)

var (
Expand Down Expand Up @@ -108,18 +109,18 @@ func (p *nodeConnectionBalancer[P, C]) Prune(ctx context.Context) {
case <-p.ticker.C:
if p.sem.TryAcquire(1) {
ctx, cancel := context.WithTimeout(ctx, 10*time.Second)
p.pruneConnections(ctx)
p.mustPruneConnections(ctx)
cancel()
p.sem.Release(1)
}
}
}
}

// pruneConnections prunes connections to nodes that have more than MaxConns/(# of nodes)
// mustPruneConnections prunes connections to nodes that have more than MaxConns/(# of nodes)
// This causes the pool to reconnect, which over time will lead to a balanced number of connections
// across each node.
func (p *nodeConnectionBalancer[P, C]) pruneConnections(ctx context.Context) {
func (p *nodeConnectionBalancer[P, C]) mustPruneConnections(ctx context.Context) {
start := time.Now()
defer func() {
pruningTimeHistogram.WithLabelValues(p.pool.ID()).Observe(float64(time.Since(start).Milliseconds()))
Expand Down Expand Up @@ -224,8 +225,10 @@ func (p *nodeConnectionBalancer[P, C]) pruneConnections(ctx context.Context) {
if numToPrune > 1 {
numToPrune >>= 1
}
if uint32(len(healthyConns[node])) < numToPrune {
numToPrune = uint32(len(healthyConns[node]))

healthyNodeCount := genutil.MustEnsureUInt32(len(healthyConns[node]))
if healthyNodeCount < numToPrune {
numToPrune = healthyNodeCount
}
if numToPrune == 0 {
continue
Expand Down
2 changes: 1 addition & 1 deletion internal/datastore/crdb/pool/balancer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ func TestNodeConnectionBalancerPrune(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()

p.pruneConnections(ctx)
p.mustPruneConnections(ctx)
require.Equal(t, len(tt.expectedGC), len(pool.gc))
gcFromNodes := make([]uint32, 0, len(tt.expectedGC))
for _, n := range pool.gc {
Expand Down
4 changes: 2 additions & 2 deletions internal/services/shared/schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ func ValidateSchemaChanges(ctx context.Context, compiled *compiler.CompiledSchem
type AppliedSchemaChanges struct {
// TotalOperationCount holds the total number of "dispatch" operations performed by the schema
// being applied.
TotalOperationCount uint32
TotalOperationCount int

// NewObjectDefNames contains the names of the newly added object definitions.
NewObjectDefNames []string
Expand Down Expand Up @@ -229,7 +229,7 @@ func ApplySchemaChangesOverExisting(
Msg("completed schema update")

return &AppliedSchemaChanges{
TotalOperationCount: uint32(len(validated.compiled.ObjectDefinitions) + len(validated.compiled.CaveatDefinitions) + removedObjectDefNames.Len() + removedCaveatDefNames.Len()),
TotalOperationCount: len(validated.compiled.ObjectDefinitions) + len(validated.compiled.CaveatDefinitions) + removedObjectDefNames.Len() + removedCaveatDefNames.Len(),
NewObjectDefNames: validated.newObjectDefNames.Subtract(existingObjectDefNames).AsSlice(),
RemovedObjectDefNames: removedObjectDefNames.AsSlice(),
NewCaveatDefNames: validated.newCaveatDefNames.Subtract(existingCaveatDefNames).AsSlice(),
Expand Down
60 changes: 48 additions & 12 deletions internal/services/v1/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,52 @@ import (
"github.com/authzed/spicedb/pkg/tuple"
)

// ErrExceedsMaximumChecks occurs when too many checks are given to a call.
type ErrExceedsMaximumChecks struct {
error
checkCount uint64
maxCountAllowed uint64
}

// MarshalZerologObject implements zerolog object marshalling.
func (err ErrExceedsMaximumChecks) MarshalZerologObject(e *zerolog.Event) {
e.Err(err.error).Uint64("checkCount", err.checkCount).Uint64("maxCountAllowed", err.maxCountAllowed)
}

// GRPCStatus implements retrieving the gRPC status for the error.
func (err ErrExceedsMaximumChecks) GRPCStatus() *status.Status {
return spiceerrors.WithCodeAndDetails(
err,
codes.InvalidArgument,
spiceerrors.ForReason(
v1.ErrorReason_ERROR_REASON_UNSPECIFIED,
map[string]string{
"check_count": strconv.FormatUint(err.checkCount, 10),
"maximum_checks_allowed": strconv.FormatUint(err.maxCountAllowed, 10),
},
),
)
}

// NewExceedsMaximumChecksErr creates a new error representing that too many updates were given to a BulkCheckPermissions call.
func NewExceedsMaximumChecksErr(checkCount uint64, maxCountAllowed uint64) ErrExceedsMaximumChecks {
return ErrExceedsMaximumChecks{
error: fmt.Errorf("check count of %d is greater than maximum allowed of %d", checkCount, maxCountAllowed),
checkCount: checkCount,
maxCountAllowed: maxCountAllowed,
}
}

// ErrExceedsMaximumUpdates occurs when too many updates are given to a call.
type ErrExceedsMaximumUpdates struct {
error
updateCount uint16
maxCountAllowed uint16
updateCount uint64
maxCountAllowed uint64
}

// MarshalZerologObject implements zerolog object marshalling.
func (err ErrExceedsMaximumUpdates) MarshalZerologObject(e *zerolog.Event) {
e.Err(err.error).Uint16("updateCount", err.updateCount).Uint16("maxCountAllowed", err.maxCountAllowed)
e.Err(err.error).Uint64("updateCount", err.updateCount).Uint64("maxCountAllowed", err.maxCountAllowed)
}

// GRPCStatus implements retrieving the gRPC status for the error.
Expand All @@ -35,15 +71,15 @@ func (err ErrExceedsMaximumUpdates) GRPCStatus() *status.Status {
spiceerrors.ForReason(
v1.ErrorReason_ERROR_REASON_TOO_MANY_UPDATES_IN_REQUEST,
map[string]string{
"update_count": strconv.Itoa(int(err.updateCount)),
"maximum_updates_allowed": strconv.Itoa(int(err.maxCountAllowed)),
"update_count": strconv.FormatUint(err.updateCount, 10),
"maximum_updates_allowed": strconv.FormatUint(err.maxCountAllowed, 10),
},
),
)
}

// NewExceedsMaximumUpdatesErr creates a new error representing that too many updates were given to a WriteRelationships call.
func NewExceedsMaximumUpdatesErr(updateCount uint16, maxCountAllowed uint16) ErrExceedsMaximumUpdates {
func NewExceedsMaximumUpdatesErr(updateCount uint64, maxCountAllowed uint64) ErrExceedsMaximumUpdates {
return ErrExceedsMaximumUpdates{
error: fmt.Errorf("update count of %d is greater than maximum allowed of %d", updateCount, maxCountAllowed),
updateCount: updateCount,
Expand All @@ -54,13 +90,13 @@ func NewExceedsMaximumUpdatesErr(updateCount uint16, maxCountAllowed uint16) Err
// ErrExceedsMaximumPreconditions occurs when too many preconditions are given to a call.
type ErrExceedsMaximumPreconditions struct {
error
preconditionCount uint16
maxCountAllowed uint16
preconditionCount uint64
maxCountAllowed uint64
}

// MarshalZerologObject implements zerolog object marshalling.
func (err ErrExceedsMaximumPreconditions) MarshalZerologObject(e *zerolog.Event) {
e.Err(err.error).Uint16("preconditionCount", err.preconditionCount).Uint16("maxCountAllowed", err.maxCountAllowed)
e.Err(err.error).Uint64("preconditionCount", err.preconditionCount).Uint64("maxCountAllowed", err.maxCountAllowed)
}

// GRPCStatus implements retrieving the gRPC status for the error.
Expand All @@ -71,15 +107,15 @@ func (err ErrExceedsMaximumPreconditions) GRPCStatus() *status.Status {
spiceerrors.ForReason(
v1.ErrorReason_ERROR_REASON_TOO_MANY_PRECONDITIONS_IN_REQUEST,
map[string]string{
"precondition_count": strconv.Itoa(int(err.preconditionCount)),
"maximum_updates_allowed": strconv.Itoa(int(err.maxCountAllowed)),
"precondition_count": strconv.FormatUint(err.preconditionCount, 10),
"maximum_updates_allowed": strconv.FormatUint(err.maxCountAllowed, 10),
},
),
)
}

// NewExceedsMaximumPreconditionsErr creates a new error representing that too many preconditions were given to a call.
func NewExceedsMaximumPreconditionsErr(preconditionCount uint16, maxCountAllowed uint16) ErrExceedsMaximumPreconditions {
func NewExceedsMaximumPreconditionsErr(preconditionCount uint64, maxCountAllowed uint64) ErrExceedsMaximumPreconditions {
return ErrExceedsMaximumPreconditions{
error: fmt.Errorf(
"precondition count of %d is greater than maximum allowed of %d",
Expand Down
14 changes: 13 additions & 1 deletion internal/services/v1/experimental.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (
"github.com/authzed/spicedb/pkg/cursor"
"github.com/authzed/spicedb/pkg/datastore"
dsoptions "github.com/authzed/spicedb/pkg/datastore/options"
"github.com/authzed/spicedb/pkg/genutil"
"github.com/authzed/spicedb/pkg/genutil/mapz"
"github.com/authzed/spicedb/pkg/genutil/slicez"
"github.com/authzed/spicedb/pkg/middleware/consistency"
Expand Down Expand Up @@ -409,14 +410,25 @@ func (es *experimentalServer) BulkExportRelationships(
return nil
}

const maxBulkCheckCount = 10000
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this meant to be 1K?

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; since we didn't previously have a limit at all, I figured we'd set a super high limit for now and then lower it when BulkCheck is moved out of experimental


func (es *experimentalServer) BulkCheckPermission(ctx context.Context, req *v1.BulkCheckPermissionRequest) (*v1.BulkCheckPermissionResponse, error) {
atRevision, checkedAt, err := consistency.RevisionFromContext(ctx)
if err != nil {
return nil, es.rewriteError(ctx, err)
}

if len(req.Items) > maxBulkCheckCount {
return nil, es.rewriteError(ctx, NewExceedsMaximumChecksErr(uint64(len(req.Items)), maxBulkCheckCount))
}

// Compute a hash for each requested item and record its index(es) for the items, to be used for sorting of results.
itemIndexByHash := mapz.NewMultiMapWithCap[string, int](uint32(len(req.Items)))
itemCount, err := genutil.EnsureUInt32(len(req.Items))
if err != nil {
return nil, es.rewriteError(ctx, err)
}

itemIndexByHash := mapz.NewMultiMapWithCap[string, int](itemCount)
for index, item := range req.Items {
itemHash, err := computeBulkCheckPermissionItemHash(item)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion internal/services/v1/experimental_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ func TestBulkExportRelationships(t *testing.T) {
}

require.NoError(err)
require.LessOrEqual(uint32(len(batch.Relationships)), tc.batchSize)
require.LessOrEqual(uint64(len(batch.Relationships)), uint64(tc.batchSize))
require.NotNil(batch.AfterResultCursor)
require.NotEmpty(batch.AfterResultCursor.Token)

Expand Down
23 changes: 17 additions & 6 deletions internal/services/v1/relationships.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"github.com/authzed/spicedb/pkg/datastore"
"github.com/authzed/spicedb/pkg/datastore/options"
"github.com/authzed/spicedb/pkg/datastore/pagination"
"github.com/authzed/spicedb/pkg/genutil"
"github.com/authzed/spicedb/pkg/genutil/mapz"
"github.com/authzed/spicedb/pkg/middleware/consistency"
dispatchv1 "github.com/authzed/spicedb/pkg/proto/dispatch/v1"
Expand Down Expand Up @@ -254,14 +255,14 @@ func (ps *permissionServer) WriteRelationships(ctx context.Context, req *v1.Writ
if len(req.Updates) > int(ps.config.MaxUpdatesPerWrite) {
return nil, ps.rewriteError(
ctx,
NewExceedsMaximumUpdatesErr(uint16(len(req.Updates)), ps.config.MaxUpdatesPerWrite),
NewExceedsMaximumUpdatesErr(uint64(len(req.Updates)), uint64(ps.config.MaxUpdatesPerWrite)),
)
}

if len(req.OptionalPreconditions) > int(ps.config.MaxPreconditionsCount) {
return nil, ps.rewriteError(
ctx,
NewExceedsMaximumPreconditionsErr(uint16(len(req.OptionalPreconditions)), ps.config.MaxPreconditionsCount),
NewExceedsMaximumPreconditionsErr(uint64(len(req.OptionalPreconditions)), uint64(ps.config.MaxPreconditionsCount)),
)
}

Expand Down Expand Up @@ -302,9 +303,14 @@ func (ps *permissionServer) WriteRelationships(ctx context.Context, req *v1.Writ
return ps.rewriteError(ctx, err)
}

dispatchCount, err := genutil.EnsureUInt32(len(req.OptionalPreconditions) + 1)
if err != nil {
return ps.rewriteError(ctx, err)
}

usagemetrics.SetInContext(ctx, &dispatchv1.ResponseMeta{
// One request per precondition and one request for the actual writes.
DispatchCount: uint32(len(req.OptionalPreconditions)) + 1,
DispatchCount: dispatchCount,
})

span.AddEvent("preconditions")
Expand Down Expand Up @@ -338,7 +344,7 @@ func (ps *permissionServer) DeleteRelationships(ctx context.Context, req *v1.Del
if len(req.OptionalPreconditions) > int(ps.config.MaxPreconditionsCount) {
return nil, ps.rewriteError(
ctx,
NewExceedsMaximumPreconditionsErr(uint16(len(req.OptionalPreconditions)), ps.config.MaxPreconditionsCount),
NewExceedsMaximumPreconditionsErr(uint64(len(req.OptionalPreconditions)), uint64(ps.config.MaxPreconditionsCount)),
)
}

Expand All @@ -350,9 +356,14 @@ func (ps *permissionServer) DeleteRelationships(ctx context.Context, req *v1.Del
return err
}

dispatchCount, err := genutil.EnsureUInt32(len(req.OptionalPreconditions) + 1)
if err != nil {
return ps.rewriteError(ctx, err)
}

usagemetrics.SetInContext(ctx, &dispatchv1.ResponseMeta{
// One request per precondition and one request for the actual delete.
DispatchCount: uint32(len(req.OptionalPreconditions)) + 1,
DispatchCount: dispatchCount,
})

if err := checkPreconditions(ctx, rwt, req.OptionalPreconditions); err != nil {
Expand Down Expand Up @@ -403,7 +414,7 @@ func (ps *permissionServer) DeleteRelationships(ctx context.Context, req *v1.Del
}

// Otherwise, kick off an unlimited deletion.
_, err := rwt.DeleteRelationships(ctx, req.RelationshipFilter)
_, err = rwt.DeleteRelationships(ctx, req.RelationshipFilter)
return err
})
if err != nil {
Expand Down
16 changes: 14 additions & 2 deletions internal/services/v1/schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"github.com/authzed/spicedb/internal/middleware/usagemetrics"
"github.com/authzed/spicedb/internal/services/shared"
"github.com/authzed/spicedb/pkg/datastore"
"github.com/authzed/spicedb/pkg/genutil"
dispatchv1 "github.com/authzed/spicedb/pkg/proto/dispatch/v1"
"github.com/authzed/spicedb/pkg/schemadsl/compiler"
"github.com/authzed/spicedb/pkg/schemadsl/generator"
Expand Down Expand Up @@ -87,8 +88,13 @@ func (ss *schemaServer) ReadSchema(ctx context.Context, _ *v1.ReadSchemaRequest)
return nil, ss.rewriteError(ctx, err)
}

dispatchCount, err := genutil.EnsureUInt32(len(nsDefs) + len(caveatDefs))
if err != nil {
return nil, ss.rewriteError(ctx, err)
}

usagemetrics.SetInContext(ctx, &dispatchv1.ResponseMeta{
DispatchCount: uint32(len(nsDefs) + len(caveatDefs)),
DispatchCount: dispatchCount,
})

return &v1.ReadSchemaResponse{
Expand Down Expand Up @@ -124,8 +130,14 @@ func (ss *schemaServer) WriteSchema(ctx context.Context, in *v1.WriteSchemaReque
if err != nil {
return err
}

dispatchCount, err := genutil.EnsureUInt32(applied.TotalOperationCount)
if err != nil {
return err
}

usagemetrics.SetInContext(ctx, &dispatchv1.ResponseMeta{
DispatchCount: applied.TotalOperationCount,
DispatchCount: dispatchCount,
})
return nil
})
Expand Down
Loading
Loading