Skip to content

Commit 4cbbd13

Browse files
EmyrkEmyrk
authored
feat: Turn on rbac check caching (#6202)
* chore: Turn on rbac check caching. Should not affect much unless authz_querier experiment is enabled
1 parent fac7c02 commit 4cbbd13

File tree

9 files changed

+61
-9
lines changed

9 files changed

+61
-9
lines changed

‎coderd/coderd.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,7 @@ func New(options *Options) *API {
187187
options.PrometheusRegistry = prometheus.NewRegistry()
188188
}
189189
if options.Authorizer == nil {
190-
options.Authorizer = rbac.NewAuthorizer(options.PrometheusRegistry)
190+
options.Authorizer = rbac.NewCachingAuthorizer(options.PrometheusRegistry)
191191
}
192192
if options.TailnetCoordinator == nil {
193193
options.TailnetCoordinator = tailnet.NewCoordinator()
@@ -289,6 +289,7 @@ func New(options *Options) *API {
289289
tracing.StatusWriterMiddleware,
290290
tracing.Middleware(api.TracerProvider),
291291
httpmw.AttachRequestID,
292+
httpmw.AttachAuthzCache,
292293
httpmw.ExtractRealIP(api.RealIPConfig),
293294
httpmw.Logger(api.Logger),
294295
httpmw.Prometheus(options.PrometheusRegistry),

‎coderd/coderdtest/coderdtest.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -184,7 +184,7 @@ func NewOptions(t *testing.T, options *Options) (func(http.Handler), context.Can
184184
if strings.Contains(os.Getenv("CODER_EXPERIMENTS_TEST"), string(codersdk.ExperimentAuthzQuerier)) {
185185
if options.Authorizer == nil {
186186
options.Authorizer = &RecordingAuthorizer{
187-
Wrapped: rbac.NewAuthorizer(prometheus.NewRegistry()),
187+
Wrapped: rbac.NewCachingAuthorizer(prometheus.NewRegistry()),
188188
}
189189
}
190190
options.Database = dbauthz.New(options.Database, options.Authorizer, slogtest.Make(t, nil).Leveled(slog.LevelDebug))

‎coderd/httpmw/authz.go

+16-2
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,10 @@ package httpmw
33
import (
44
"net/http"
55

6-
"github.com/coder/coder/coderd/database/dbauthz"
7-
86
"github.com/go-chi/chi/v5"
7+
8+
"github.com/coder/coder/coderd/database/dbauthz"
9+
"github.com/coder/coder/coderd/rbac"
910
)
1011

1112
// AsAuthzSystem is a chained handler that temporarily sets the dbauthz context
@@ -35,3 +36,16 @@ func AsAuthzSystem(mws ...func(http.Handler) http.Handler) func(http.Handler) ht
3536
})
3637
}
3738
}
39+
40+
// AttachAuthzCache enables the authz cache for the authorizer. All rbac checks will
41+
// run against the cache, meaning duplicate checks will not be performed.
42+
//
43+
// Note the cache is safe for multiple actors. So mixing user and system checks
44+
// is ok.
45+
func AttachAuthzCache(next http.Handler) http.Handler {
46+
return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
47+
ctx := rbac.WithCacheCtx(r.Context())
48+
49+
next.ServeHTTP(rw, r.WithContext(ctx))
50+
})
51+
}

‎coderd/rbac/authz.go

+7
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,13 @@ var (
158158
partialQuery rego.PreparedPartialQuery
159159
)
160160

161+
// NewCachingAuthorizer returns a new RegoAuthorizer that supports context based
162+
// caching. To utilize the caching, the context passed to Authorize() must be
163+
// created with 'WithCacheCtx(ctx)'.
164+
func NewCachingAuthorizer(registry prometheus.Registerer) Authorizer {
165+
return Cacher(NewAuthorizer(registry))
166+
}
167+
161168
func NewAuthorizer(registry prometheus.Registerer) *RegoAuthorizer {
162169
queryOnce.Do(func() {
163170
var err error

‎coderd/rbac/authz_test.go

+3-3
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ func BenchmarkRBACAuthorize(b *testing.B) {
109109
uuid.MustParse("0632b012-49e0-4d70-a5b3-f4398f1dcd52"),
110110
uuid.MustParse("70dbaa7a-ea9c-4f68-a781-97b08af8461d"),
111111
)
112-
authorizer := rbac.NewAuthorizer(prometheus.NewRegistry())
112+
authorizer := rbac.NewCachingAuthorizer(prometheus.NewRegistry())
113113
// This benchmarks all the simple cases using just user permissions. Groups
114114
// are added as noise, but do not do anything.
115115
for _, c := range benchCases {
@@ -136,7 +136,7 @@ func BenchmarkRBACAuthorizeGroups(b *testing.B) {
136136
uuid.MustParse("0632b012-49e0-4d70-a5b3-f4398f1dcd52"),
137137
uuid.MustParse("70dbaa7a-ea9c-4f68-a781-97b08af8461d"),
138138
)
139-
authorizer := rbac.NewAuthorizer(prometheus.NewRegistry())
139+
authorizer := rbac.NewCachingAuthorizer(prometheus.NewRegistry())
140140

141141
// Same benchmark cases, but this time groups will be used to match.
142142
// Some '*' permissions will still match, but using a fake action reduces
@@ -188,7 +188,7 @@ func BenchmarkRBACFilter(b *testing.B) {
188188
uuid.MustParse("70dbaa7a-ea9c-4f68-a781-97b08af8461d"),
189189
)
190190

191-
authorizer := rbac.NewAuthorizer(prometheus.NewRegistry())
191+
authorizer := rbac.NewCachingAuthorizer(prometheus.NewRegistry())
192192

193193
for _, c := range benchCases {
194194
b.Run("PrepareOnly-"+c.Name, func(b *testing.B) {

‎coderd/rbac/builtin_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ type authSubject struct {
2323
func TestRolePermissions(t *testing.T) {
2424
t.Parallel()
2525

26-
auth := rbac.NewAuthorizer(prometheus.NewRegistry())
26+
auth := rbac.NewCachingAuthorizer(prometheus.NewRegistry())
2727

2828
// currentUser is anything that references "me", "mine", or "my".
2929
currentUser := uuid.New()

‎coderd/rbac/cache.go

+2
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@ type cachedCalls struct {
2020
// multiple calls are made to the Authorizer for the same subject, action, and
2121
// object. The cache is on each `ctx` and is not shared between requests.
2222
// If no cache is found on the context, the Authorizer is called as normal.
23+
//
24+
// Cacher is safe for multiple actors.
2325
func Cacher(authz Authorizer) Authorizer {
2426
return &cachedCalls{authz: authz}
2527
}

‎coderd/rbac/cache_test.go

+28
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package rbac_test
22

33
import (
44
"context"
5+
"fmt"
56
"testing"
67

78
"github.com/stretchr/testify/require"
@@ -10,6 +11,33 @@ import (
1011
"github.com/coder/coder/coderd/rbac"
1112
)
1213

14+
// BenchmarkCacher benchmarks the performance of the cacher with a given
15+
// cache size. The expected cache size in prod will usually be 1-2. In Filter
16+
// cases it can get as high as 10.
17+
func BenchmarkCacher(b *testing.B) {
18+
b.ResetTimer()
19+
// Size of the cache.
20+
sizes := []int{1, 10, 100, 1000}
21+
for _, size := range sizes {
22+
b.Run(fmt.Sprintf("Size%d", size), func(b *testing.B) {
23+
ctx := rbac.WithCacheCtx(context.Background())
24+
authz := rbac.Cacher(&coderdtest.FakeAuthorizer{AlwaysReturn: nil})
25+
for i := 0; i < size; i++ {
26+
// Preload the cache of a given size
27+
subj, obj, action := coderdtest.RandomRBACSubject(), coderdtest.RandomRBACObject(), coderdtest.RandomRBACAction()
28+
_ = authz.Authorize(ctx, subj, action, obj)
29+
}
30+
31+
// Cache is loaded as a slice, so this cache hit is always the last element.
32+
subj, obj, action := coderdtest.RandomRBACSubject(), coderdtest.RandomRBACObject(), coderdtest.RandomRBACAction()
33+
b.ResetTimer()
34+
for i := 0; i < b.N; i++ {
35+
_ = authz.Authorize(ctx, subj, action, obj)
36+
}
37+
})
38+
}
39+
}
40+
1341
func TestCacher(t *testing.T) {
1442
t.Parallel()
1543

‎enterprise/coderd/coderd.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ func New(ctx context.Context, options *Options) (*API, error) {
4747
options.PrometheusRegistry = prometheus.NewRegistry()
4848
}
4949
if options.Options.Authorizer == nil {
50-
options.Options.Authorizer = rbac.NewAuthorizer(options.PrometheusRegistry)
50+
options.Options.Authorizer = rbac.NewCachingAuthorizer(options.PrometheusRegistry)
5151
}
5252
ctx, cancelFunc := context.WithCancel(ctx)
5353
api := &API{

0 commit comments

Comments
 (0)