feat(authz): make v2 request limits configurable#3508
Conversation
Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis pull request implements a configurable request limits system for authorization v2 RPC endpoints. Proto validation rules are relaxed to remove upper-bound constraints, shifting enforcement to the service layer where limits are configured and validated against incoming requests. ChangesRequest Limits Configuration and Enforcement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request migrates request limit validation for Authorization v2 from static Protocol Buffer CEL expressions to a configurable service-side implementation. This change enhances flexibility by allowing administrators to adjust request limits via configuration rather than requiring code changes, while maintaining strict validation of incoming requests. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. CEL was strict and hard to change, Now limits have a wider range. Configured now with ease and grace, The service runs at better pace. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request transitions the authorization service from hardcoded request limits to configurable ones. Changes include removing CEL-based size constraints in the Protobuf definitions, adding a new request_limits configuration section, and implementing a dedicated validation layer to enforce these limits. Documentation and tests have been updated to reflect the shift from static to dynamic limits. Review feedback identified potential nil pointer dereferences in the new validation functions for entity identifiers and resources, which could lead to runtime panics if not addressed.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
jakedoublev
changed the title
Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com>
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Make DecisionRequestFulfillableObligationFqnsMax require > 0 consistent with all other request limits. Add boundary tests verifying exactly-at-limit values pass validation. Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com>
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Add ErrInvalidRequestLimitConfig sentinel error and extract requestLimitConfigError helper to reduce duplication. Also replace context.Background() with t.Context() in validation tests. Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com>
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@service/authorization/v2/authorization_test.go`:
- Around line 661-677: Add a parallel test case to cover
GetDecisionMultiResourceRequest with 51 obligations: create a test entry
mirroring the existing single-request case (name like "multi-entity: registered
resource, action: create, resource: registered, obligations - 51") that builds a
&authzV2.GetDecisionMultiResourceRequest using the same
sampleRegisteredResourceFQN for the entity and resource, sampleActionCreate for
Action, and fiftyOneObligations for FulfillableObligationFqns; place it
alongside the other table-driven cases in authorization_test.go so the
multi-resource proto path (GetDecisionMultiResourceRequest and its
FulfillableObligationFqns) is exercised and the 51-obligation success behavior
is validated.
In `@service/authorization/v2/validation_test.go`:
- Around line 60-67: The tests in
Test_validateGetDecisionMultiResourceRequest_DefaultRequestLimit (and the other
ranges) only assert resource count limits for GetDecisionMultiResourceRequest
and miss exercising the FulfillableObligationFqns branch, so add assertions and
test cases that validate obligation FQNs are checked against the configured
limit and that the branch is bound to the correct config key; update the test
helper newDecisionMultiResourceRequestWithResourceCount (or add new fixtures) to
produce requests with non-empty FulfillableObligationFqns and add cases for
valid/invalid counts, and verify validateGetDecisionMultiResourceRequest returns
connect.CodeInvalidArgument with the expected error message when obligation FQNs
exceed the limit and passes when within limits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5d29f2ca-57fb-488c-b7b6-98ec86fa7fb5
⛔ Files ignored due to path filters (1)
protocol/go/authorization/v2/authorization.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (11)
docs/Configuring.mddocs/grpc/index.htmldocs/openapi/authorization/v2/authorization.openapi.yamldocs/openapi/entityresolution/v2/entity_resolution.openapi.yamlservice/authorization/v2/authorization.goservice/authorization/v2/authorization.protoservice/authorization/v2/authorization_test.goservice/authorization/v2/config.goservice/authorization/v2/config_test.goservice/authorization/v2/validation.goservice/authorization/v2/validation_test.go
Add proto and service-side tests for FulfillableObligationFqns on GetDecisionMultiResourceRequest, which was previously untested. Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com>
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
2 participants
Make Authorization v2 request limits configurable by moving max-count validation from proto CEL to service-side config.
Summary by CodeRabbit
New Features
Documentation
Tests