Skip to content

Conversation

@everettbu
Copy link

Test 2

* remove the use of client side cache for in-proc authz client

Co-authored-by: Gabriel MABILLE <[email protected]>

* add a permission denial cache, fetch perms if not in either of the caches

Co-authored-by: Gabriel MABILLE <[email protected]>

* Clean up tests

Co-authored-by: Ieva <[email protected]>

* Cache tests

Co-authored-by: Ieva <[email protected]>

* Add test to list + cache

Co-authored-by: Ieva <[email protected]>

* Add outdated cache test

Co-authored-by: Ieva <[email protected]>

* Re-organize metrics

Co-authored-by: Ieva <[email protected]>

---------

Co-authored-by: Gabriel MABILLE <[email protected]>
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Summary

This PR enhances the AuthZ service caching mechanism by introducing a dual-caching strategy to optimize authorization performance. The main changes include:

New Permission Denial Cache: A permDenialCache has been added to the RBAC service that specifically caches denied permissions. This prevents repeated expensive database lookups when users attempt to access resources they don't have permissions for.

Restructured Permission Checking Logic: The Check method now follows a two-tier approach - first checking the denial cache for immediate rejection, then attempting to use cached permissions via the new getCachedIdentityPermissions method before falling back to database queries. This provides more granular control over caching behavior.

Client-Side Caching Optimization: In-process RBAC clients now use a NoopCache implementation instead of LocalCache, eliminating redundant caching at the client level since the service already has comprehensive caching. Remote clients retain their LocalCache with 30-second TTL for network optimization.

Enhanced Test Coverage: The test suite has been completely overhauled with new comprehensive test functions (TestService_CacheCheck and TestService_CacheList) that validate cache hits, database fallbacks, outdated cache handling, and denial cache behavior across multiple scenarios.

These changes integrate with Grafana's existing authorization infrastructure, building upon the service's existing permCache, teamCache, and basicRoleCache to create a more efficient and performant authorization system that reduces database load while maintaining security guarantees.

Confidence score: 4/5

• This PR appears safe to merge with well-structured caching improvements and comprehensive test coverage
• The score reflects solid implementation with proper separation of concerns and fallback mechanisms, though the minimal PR description limits context validation
• The service.go file needs careful review to ensure the caching logic correctly handles edge cases and maintains authorization security

4 files reviewed, no comments

Edit Code Review Bot Settings | Greptile

@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update or ping for review. Thank you for your contributions!

@github-actions
Copy link
Contributor

This pull request has been automatically closed because it has not had any further activity in the last 2 weeks. Thank you for your contributions!

@github-actions github-actions bot closed this Sep 12, 2025
@everettbu everettbu reopened this Sep 12, 2025
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants