mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
perf(coderd): batch authcheck permissions via rbac.Filter (#27309)
`POST /api/v2/authcheck` evaluated every check with a full policy evaluation in a serial loop. A subject in many organizations (100+) produced hundreds of full evaluations, taking seconds on a cold cache (DEVEX-608). Group the checks by `(action, resource type)` and authorize each group with the existing `rbac.Filter`, which amortizes a single partial evaluation across the group once it is large enough. Each check is wrapped in a small value struct that carries its response key, so `Filter`'s returned subset maps back to keys by reading a field rather than relying on element identity. `Filter` now takes an explicit `prepareThreshold`; existing callers pass the new `rbac.DefaultFilterThreshold` (10), and `checkAuthorization` passes 50, above the ~35-group crossover measured for this workload, so subjects with few objects of a given type keep the per-object path and cannot regress. ## Stacking This is stacked on top of #27244. `Filter` runs `Prepare` (partial evaluation), and those residuals are only compact once #27244's set-membership residuals land. On plain `main` the existing O(N) residual fanout means batching can regress at high org counts, so this change should land with or after #27244. <details> <summary>Decision log</summary> ### Bottleneck - `site/src/modules/permissions/organizations.ts` defines ~14 permission checks per org; `organizationsPermissions()` flattens them across all orgs into one `POST /api/v2/authcheck`. A 100-org request is ~1400 checks. - `checkAuthorization` looped serially, calling `Authorizer.Authorize` (full eval) once per check. - The endpoint's `maxFetch = 10` only caps checks that carry a `resource_id`, not total checks, so it does not bound this workload. ### Approach - Group checks by `(action, resource type)` and run each group through `rbac.Filter`, which does one partial evaluation (`Prepare`) and reuses it across the group. - Carry the response key as data in a small value struct implementing `RBACObject()`, so allowed results map back to keys without pointer identity: ```go type authorizeCheck struct { key string object rbac.Object } func (c authorizeCheck) RBACObject() rbac.Object { return c.object } ``` - `Filter` takes a required `prepareThreshold int` (no functional options). Generic callers pass `rbac.DefaultFilterThreshold = 10`; `/authcheck` passes 50 because the measured crossover for this workload is ~35 groups. ### Alternatives rejected - **Bounded `errgroup` parallelism**: reduced wall time at high org counts but not aggregate work (allocations flat). Discarded in favor of reducing work via partial evaluation. - **Symmetric-deny Rego simplification** (on the #27244 branch): replacing the known-org deny-fold with symmetric `org := -1` / `scope_org := -1` rules failed existing SQL-compile tests. A `-1` known-org vote gated by `not org = -1` produces a negated membership test over the unknown org id, which OPA emits as an unconvertible support rule. #27244's fold (`member_allow - org_deny`, a positive set-difference membership test) is therefore load-bearing, not incidental. </details> --- Authored with Coder Agents. --------- Co-authored-by: Steven Masley <Emyrk@users.noreply.github.com>
This commit is contained in:
co-authored by
Steven Masley
parent
7e708b24ce
commit
51a9aa1bfc
+23
-6
@@ -210,13 +210,32 @@ type PreparedAuthorized interface {
|
||||
CompileToSQL(ctx context.Context, cfg regosql.ConvertConfig) (string, error)
|
||||
}
|
||||
|
||||
// DefaultFilterThreshold is the object count at or above which Filter switches
|
||||
// from a full evaluation per object to a single partial evaluation (Prepare)
|
||||
// reused across the set. Benchmarks show Authorize is faster than the Prepare
|
||||
// overhead below ~10 objects. Callers whose Prepare cost grows with the input
|
||||
// size (for example a subject carrying one role per object) should pass a
|
||||
// higher threshold.
|
||||
const DefaultFilterThreshold = 10
|
||||
|
||||
// Filter takes in a list of objects, and will filter the list removing all
|
||||
// the elements the subject does not have permission for. All objects must be
|
||||
// of the same type.
|
||||
//
|
||||
// prepareThreshold is the object count at or above which Filter uses a single
|
||||
// partial evaluation reused across the set instead of a full evaluation per
|
||||
// object. Pass DefaultFilterThreshold unless the caller has a reason to tune
|
||||
// it.
|
||||
//
|
||||
// Ideally the 'CompileToSQL' is used instead for large sets. This cost scales
|
||||
// linearly with the number of objects passed in.
|
||||
func Filter[O Objecter](ctx context.Context, auth Authorizer, subject Subject, action policy.Action, objects []O) ([]O, error) {
|
||||
func Filter[O Objecter](ctx context.Context, auth Authorizer, subject Subject, action policy.Action, objects []O, prepareThreshold int) ([]O, error) {
|
||||
if prepareThreshold <= 0 {
|
||||
// A non-positive threshold would force the Prepare path for every
|
||||
// non-empty input, the opposite of what a caller passing 0 as a stand-in
|
||||
// for "default" expects. Fail loudly on an authorization function.
|
||||
return nil, xerrors.New("prepareThreshold must be positive; pass DefaultFilterThreshold")
|
||||
}
|
||||
if len(objects) == 0 {
|
||||
// Nothing to filter
|
||||
return objects, nil
|
||||
@@ -238,11 +257,9 @@ func Filter[O Objecter](ctx context.Context, auth Authorizer, subject Subject, a
|
||||
)
|
||||
defer span.End()
|
||||
|
||||
// Running benchmarks on this function, it is **always** faster to call
|
||||
// auth.Authorize on <10 objects. This is because the overhead of
|
||||
// 'Prepare'. Once we cross 10 objects, then it starts to become
|
||||
// faster
|
||||
if len(objects) < 10 {
|
||||
// Below the threshold, a full evaluation per object is faster than paying
|
||||
// the Prepare overhead once and reusing it.
|
||||
if len(objects) < prepareThreshold {
|
||||
for _, o := range objects {
|
||||
rbacObj := o.RBACObject()
|
||||
if rbacObj.Type != objectType {
|
||||
|
||||
@@ -61,10 +61,27 @@ func TestFilterError(t *testing.T) {
|
||||
Scope: ScopeAll,
|
||||
}
|
||||
|
||||
_, err := Filter(context.Background(), auth, subject, policy.ActionRead, []Object{ResourceUser, ResourceWorkspace})
|
||||
_, err := Filter(context.Background(), auth, subject, policy.ActionRead, []Object{ResourceUser, ResourceWorkspace}, DefaultFilterThreshold)
|
||||
require.ErrorContains(t, err, "object types must be uniform")
|
||||
})
|
||||
|
||||
t.Run("NonPositiveThreshold", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
auth := NewAuthorizer(prometheus.NewRegistry())
|
||||
subject := Subject{
|
||||
ID: uuid.NewString(),
|
||||
Roles: RoleIdentifiers{},
|
||||
Groups: []string{},
|
||||
Scope: ScopeAll,
|
||||
}
|
||||
|
||||
for _, threshold := range []int{0, -1} {
|
||||
_, err := Filter(context.Background(), auth, subject, policy.ActionRead, []Object{ResourceWorkspace}, threshold)
|
||||
require.ErrorContains(t, err, "prepareThreshold must be positive")
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("CancelledContext", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
@@ -99,7 +116,7 @@ func TestFilterError(t *testing.T) {
|
||||
ResourceUser,
|
||||
}
|
||||
|
||||
_, err := Filter(ctx, auth, subject, policy.ActionRead, objects)
|
||||
_, err := Filter(ctx, auth, subject, policy.ActionRead, objects, DefaultFilterThreshold)
|
||||
require.ErrorIs(t, err, context.Canceled)
|
||||
})
|
||||
|
||||
@@ -119,7 +136,7 @@ func TestFilterError(t *testing.T) {
|
||||
bomb: cancel,
|
||||
}
|
||||
|
||||
_, err := Filter(ctx, auth, subject, policy.ActionRead, objects)
|
||||
_, err := Filter(ctx, auth, subject, policy.ActionRead, objects, DefaultFilterThreshold)
|
||||
require.ErrorIs(t, err, context.Canceled)
|
||||
})
|
||||
})
|
||||
@@ -267,7 +284,7 @@ func TestFilter(t *testing.T) {
|
||||
}
|
||||
|
||||
// Run by filter
|
||||
list, err := Filter(ctx, auth, actor, tc.Action, localObjects)
|
||||
list, err := Filter(ctx, auth, actor, tc.Action, localObjects, DefaultFilterThreshold)
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, allowedCount, len(list), "expected number of allowed")
|
||||
for _, obj := range list {
|
||||
|
||||
@@ -255,7 +255,7 @@ func BenchmarkRBACFilter(b *testing.B) {
|
||||
b.Run(c.Name, func(b *testing.B) {
|
||||
objects := benchmarkSetup(orgs, users, b.N)
|
||||
b.ResetTimer()
|
||||
allowed, err := rbac.Filter(context.Background(), authorizer, c.Actor, policy.ActionRead, objects)
|
||||
allowed, err := rbac.Filter(context.Background(), authorizer, c.Actor, policy.ActionRead, objects, rbac.DefaultFilterThreshold)
|
||||
require.NoError(b, err)
|
||||
_ = allowed
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user