Automated Test: authz-service-improve-caching-pr #332
Closed
+240
−33
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* remove the use of client side cache for in-proc authz client Co-authored-by: Gabriel MABILLE <gabriel.mabille@grafana.com> * add a permission denial cache, fetch perms if not in either of the caches Co-authored-by: Gabriel MABILLE <gabriel.mabille@grafana.com> * Clean up tests Co-authored-by: Ieva <ieva.vasiljeva@grafana.com> * Cache tests Co-authored-by: Ieva <ieva.vasiljeva@grafana.com> * Add test to list + cache Co-authored-by: Ieva <ieva.vasiljeva@grafana.com> * Add outdated cache test Co-authored-by: Ieva <ieva.vasiljeva@grafana.com> * Re-organize metrics Co-authored-by: Ieva <ieva.vasiljeva@grafana.com> --------- Co-authored-by: Gabriel MABILLE <gabriel.mabille@grafana.com>
📝 WalkthroughWalkthroughThe changes introduce a permission denial caching mechanism to the RBAC authorization service. A new NoopCache type is added for cloud deployments, and the Service struct now includes a permission denial cache. The Check and List methods are updated to consult the denial cache first and leverage cached identity permissions, with denial results cached for subsequent requests. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Check as Check Flow
participant DenialCache as Denial Cache
participant PermCache as Permission Cache
participant Store as Store
Client->>Check: Check(identity, namespace, action, resource)
Check->>DenialCache: Lookup denial entry
alt Denial Found
DenialCache-->>Check: return ErrNotFound
Check-->>Client: Deny access (cached)
else Denial Not Found
DenialCache-->>Check: cache miss
Check->>PermCache: getCachedIdentityPermissions()
alt Permissions Cached
PermCache-->>Check: return cached permissions
Check->>Check: Evaluate permission
alt Permission Granted
Check-->>Client: Allow access
else Permission Denied
Check->>DenialCache: Cache denial
DenialCache-->>Check: OK
Check-->>Client: Deny access (newly cached)
end
else Cache Miss
PermCache-->>Check: cache miss
Check->>Store: Fetch identity permissions
Store-->>Check: return permissions
Check->>Check: Evaluate permission
alt Permission Granted
Check-->>Client: Allow access
else Permission Denied
Check->>DenialCache: Cache denial
DenialCache-->>Check: OK
Check-->>Client: Deny access (newly cached)
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/services/authz/rbac/service_test.go`:
- Around line 973-995: Test comment and the permCache.Set call are misleading:
remove the misleading "Allow access..." comment and the s.permCache.Set(ctx,
userPermCacheKey("org-12", "test-uid", "dashboards:read"),
map[string]bool{"dashboards:uid:dash1": false}) line, or alternatively replace
them with a clear comment stating the test asserts permDenialCache.Set takes
priority over permCache; update references around s.permDenialCache.Set and the
Check(...) invocation so the test only documents that denial cache precedence
(Check and checkPermission semantics) is being validated.
In `@pkg/services/authz/rbac/service.go`:
- Around line 116-137: The permissionCacheUsage metric is being set to "false"
unconditionally after calling getCachedIdentityPermissions; change the logic so
that when getCachedIdentityPermissions returns no error (i.e., a cache hit), you
record permissionCacheUsage.WithLabelValues("true", checkReq.Action).Inc()
regardless of whether checkPermission allowed the request, and only record
"false" when getCachedIdentityPermissions returns an error (cache miss); update
usages around permDenialKey, s.permDenialCache.Get,
getCachedIdentityPermissions, and checkPermission to ensure requestCount labels
remain correct for the hit vs miss cases.
🧹 Nitpick comments (2)
pkg/services/authz/rbac/cache.go (1)
30-32: Potential cache key collision whennameorparentcontain underscores or are empty.The key format
namespace.perm_userUID_action_name_parentuses_as a delimiter between all components. Ifnameorparentcontain underscores or either is empty, keys for different resources could collide:
("ns", "u", "act", "a_b", "")→ns.perm_u_act_a_b_("ns", "u", "act", "a", "b_")→ns.perm_u_act_a_b_Consider using a delimiter that won't appear in these fields (e.g.,
|or:) or a structured format.💡 Suggested fix using a safer delimiter
func userPermDenialCacheKey(namespace, userUID, action, name, parent string) string { - return namespace + ".perm_" + userUID + "_" + action + "_" + name + "_" + parent + return namespace + ".perm_denial:" + userUID + ":" + action + ":" + name + ":" + parent }pkg/services/authz/rbac.go (1)
239-251: Consider unexportingNoopCacheif it's only used internally.
NoopCacheis exported but only used within this package at line 103. Since there are no external usages, consider renaming tonoopCacheto reduce the public API surface.
Comment on lines
+973
to
+995
| t.Run("Should deny on explicit cache deny entry", func(t *testing.T) { | ||
| s := setupService() | ||
|
|
||
| s.idCache.Set(ctx, userIdentifierCacheKey("org-12", "test-uid"), *userID) | ||
|
|
||
| // Explicitly deny access to the dashboard | ||
| s.permDenialCache.Set(ctx, userPermDenialCacheKey("org-12", "test-uid", "dashboards:read", "dash1", "fold1"), true) | ||
|
|
||
| // Allow access to the dashboard to prove this is not checked | ||
| s.permCache.Set(ctx, userPermCacheKey("org-12", "test-uid", "dashboards:read"), map[string]bool{"dashboards:uid:dash1": false}) | ||
|
|
||
| resp, err := s.Check(ctx, &authzv1.CheckRequest{ | ||
| Namespace: "org-12", | ||
| Subject: "user:test-uid", | ||
| Group: "dashboard.grafana.app", | ||
| Resource: "dashboards", | ||
| Verb: "get", | ||
| Name: "dash1", | ||
| Folder: "fold1", | ||
| }) | ||
| require.NoError(t, err) | ||
| assert.False(t, resp.Allowed) | ||
| }) |
There was a problem hiding this comment.
Test comment is misleading; the assertion passes due to denial cache priority, not because permCache isn't checked.
Line 982 sets permCache with value false and comments "Allow access to the dashboard to prove this is not checked." However, checkPermission checks for key existence in the map, not the boolean value. The test passes because the denial cache is checked first (line 117-121 in service.go), not because the permCache isn't being consulted.
Consider either:
- Removing the misleading
permCache.Setand comment, or - Adding a more accurate comment explaining the test validates denial cache takes priority.
💡 Suggested fix
t.Run("Should deny on explicit cache deny entry", func(t *testing.T) {
s := setupService()
s.idCache.Set(ctx, userIdentifierCacheKey("org-12", "test-uid"), *userID)
- // Explicitly deny access to the dashboard
+ // Explicitly deny access - denial cache is checked before permission cache
s.permDenialCache.Set(ctx, userPermDenialCacheKey("org-12", "test-uid", "dashboards:read", "dash1", "fold1"), true)
- // Allow access to the dashboard to prove this is not checked
- s.permCache.Set(ctx, userPermCacheKey("org-12", "test-uid", "dashboards:read"), map[string]bool{"dashboards:uid:dash1": false})
-
resp, err := s.Check(ctx, &authzv1.CheckRequest{📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| t.Run("Should deny on explicit cache deny entry", func(t *testing.T) { | |
| s := setupService() | |
| s.idCache.Set(ctx, userIdentifierCacheKey("org-12", "test-uid"), *userID) | |
| // Explicitly deny access to the dashboard | |
| s.permDenialCache.Set(ctx, userPermDenialCacheKey("org-12", "test-uid", "dashboards:read", "dash1", "fold1"), true) | |
| // Allow access to the dashboard to prove this is not checked | |
| s.permCache.Set(ctx, userPermCacheKey("org-12", "test-uid", "dashboards:read"), map[string]bool{"dashboards:uid:dash1": false}) | |
| resp, err := s.Check(ctx, &authzv1.CheckRequest{ | |
| Namespace: "org-12", | |
| Subject: "user:test-uid", | |
| Group: "dashboard.grafana.app", | |
| Resource: "dashboards", | |
| Verb: "get", | |
| Name: "dash1", | |
| Folder: "fold1", | |
| }) | |
| require.NoError(t, err) | |
| assert.False(t, resp.Allowed) | |
| }) | |
| t.Run("Should deny on explicit cache deny entry", func(t *testing.T) { | |
| s := setupService() | |
| s.idCache.Set(ctx, userIdentifierCacheKey("org-12", "test-uid"), *userID) | |
| // Explicitly deny access - denial cache is checked before permission cache | |
| s.permDenialCache.Set(ctx, userPermDenialCacheKey("org-12", "test-uid", "dashboards:read", "dash1", "fold1"), true) | |
| resp, err := s.Check(ctx, &authzv1.CheckRequest{ | |
| Namespace: "org-12", | |
| Subject: "user:test-uid", | |
| Group: "dashboard.grafana.app", | |
| Resource: "dashboards", | |
| Verb: "get", | |
| Name: "dash1", | |
| Folder: "fold1", | |
| }) | |
| require.NoError(t, err) | |
| assert.False(t, resp.Allowed) | |
| }) |
🤖 Prompt for AI Agents
In `@pkg/services/authz/rbac/service_test.go` around lines 973 - 995, Test comment
and the permCache.Set call are misleading: remove the misleading "Allow
access..." comment and the s.permCache.Set(ctx, userPermCacheKey("org-12",
"test-uid", "dashboards:read"), map[string]bool{"dashboards:uid:dash1": false})
line, or alternatively replace them with a clear comment stating the test
asserts permDenialCache.Set takes priority over permCache; update references
around s.permDenialCache.Set and the Check(...) invocation so the test only
documents that denial cache precedence (Check and checkPermission semantics) is
being validated.
Comment on lines
+116
to
+137
| permDenialKey := userPermDenialCacheKey(checkReq.Namespace.Value, checkReq.UserUID, checkReq.Action, checkReq.Name, checkReq.ParentFolder) | ||
| if _, ok := s.permDenialCache.Get(ctx, permDenialKey); ok { | ||
| s.metrics.permissionCacheUsage.WithLabelValues("true", checkReq.Action).Inc() | ||
| s.metrics.requestCount.WithLabelValues("false", "true", req.GetVerb(), req.GetGroup(), req.GetResource()).Inc() | ||
| return &authzv1.CheckResponse{Allowed: false}, nil | ||
| } | ||
|
|
||
| cachedPerms, err := s.getCachedIdentityPermissions(ctx, checkReq.Namespace, checkReq.IdentityType, checkReq.UserUID, checkReq.Action) | ||
| if err == nil { | ||
| allowed, err := s.checkPermission(ctx, cachedPerms, checkReq) | ||
| if err != nil { | ||
| ctxLogger.Error("could not check permission", "error", err) | ||
| s.metrics.requestCount.WithLabelValues("true", "true", req.GetVerb(), req.GetGroup(), req.GetResource()).Inc() | ||
| return deny, err | ||
| } | ||
| if allowed { | ||
| s.metrics.permissionCacheUsage.WithLabelValues("true", checkReq.Action).Inc() | ||
| s.metrics.requestCount.WithLabelValues("false", "true", req.GetVerb(), req.GetGroup(), req.GetResource()).Inc() | ||
| return &authzv1.CheckResponse{Allowed: allowed}, nil | ||
| } | ||
| } | ||
| s.metrics.permissionCacheUsage.WithLabelValues("false", checkReq.Action).Inc() |
There was a problem hiding this comment.
Metrics label "false" is recorded even when cached permissions were found but didn't grant access.
On Line 137, permissionCacheUsage records "false" (cache miss) regardless of whether getCachedIdentityPermissions succeeded. If cached permissions were found but the check wasn't allowed, the metric will incorrectly indicate a cache miss.
🔧 Suggested fix
cachedPerms, err := s.getCachedIdentityPermissions(ctx, checkReq.Namespace, checkReq.IdentityType, checkReq.UserUID, checkReq.Action)
if err == nil {
allowed, err := s.checkPermission(ctx, cachedPerms, checkReq)
if err != nil {
ctxLogger.Error("could not check permission", "error", err)
s.metrics.requestCount.WithLabelValues("true", "true", req.GetVerb(), req.GetGroup(), req.GetResource()).Inc()
return deny, err
}
if allowed {
s.metrics.permissionCacheUsage.WithLabelValues("true", checkReq.Action).Inc()
s.metrics.requestCount.WithLabelValues("false", "true", req.GetVerb(), req.GetGroup(), req.GetResource()).Inc()
return &authzv1.CheckResponse{Allowed: allowed}, nil
}
+ // Cache hit but permission not granted - still a cache hit for metrics
+ s.metrics.permissionCacheUsage.WithLabelValues("true", checkReq.Action).Inc()
+ } else {
+ s.metrics.permissionCacheUsage.WithLabelValues("false", checkReq.Action).Inc()
}
- s.metrics.permissionCacheUsage.WithLabelValues("false", checkReq.Action).Inc()🤖 Prompt for AI Agents
In `@pkg/services/authz/rbac/service.go` around lines 116 - 137, The
permissionCacheUsage metric is being set to "false" unconditionally after
calling getCachedIdentityPermissions; change the logic so that when
getCachedIdentityPermissions returns no error (i.e., a cache hit), you record
permissionCacheUsage.WithLabelValues("true", checkReq.Action).Inc() regardless
of whether checkPermission allowed the request, and only record "false" when
getCachedIdentityPermissions returns an error (cache miss); update usages around
permDenialKey, s.permDenialCache.Get, getCachedIdentityPermissions, and
checkPermission to ensure requestCount labels remain correct for the hit vs miss
cases.
Sign in
to join this conversation on GitHub.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
New Features
Improvements
Tests