feat(policy): add GetRegisteredResources batch RPC proto - #3460
feat(policy): add GetRegisteredResources batch RPC proto#3460ronelliott wants to merge 4 commits into
Conversation
## Summary - Add GetRegisteredResourcesRequest and GetRegisteredResourcesResponse messages to the registered resources proto - Reuse GetRegisteredResourceRequest as the identifier element type to avoid duplicating field definitions - Add GetRegisteredResources RPC to RegisteredResourcesService for batch lookup by id or name - Regenerate protocol/go types, connect handlers, SDK wrappers, and API docs Signed-off-by: Ron Elliott <ron.elliott@virtru.com>
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 introduces a batch lookup capability for registered resources by adding a new RPC endpoint. This change is intended to facilitate more efficient ABAC authorization checks during bulk operations by allowing multiple resources to be retrieved in a single request, while maintaining compatibility with existing identifier definitions. 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. A batch of requests we send, / To fetch the resources we tend. / With proto in hand, / Across all the land, / The latency comes to an end. Footnotes
|
github-actions
Bot
added
comp:policy
|
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 ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds a bulk GetRegisteredResources RPC to RegisteredResourcesService with protobuf message definitions, generated Connect-Go client/server code, an SDK wrapper, OpenAPI and gRPC documentation updates, and a server handler stub that returns Unimplemented. ChangesGetRegisteredResources Bulk RPC
Sequence Diagram(s)sequenceDiagram
participant Caller
participant SDK as SDK Wrapper
participant ConnectClient
participant Server as RegisteredResourcesService
Caller->>SDK: GetRegisteredResources(request)
SDK->>ConnectClient: connect.NewRequest(request) -> Call GetRegisteredResources
ConnectClient->>Server: HTTP/Connect unary RPC GetRegisteredResources
Server-->>ConnectClient: 200 OK / GetRegisteredResourcesResponse
ConnectClient-->>SDK: Connect response
SDK-->>Caller: returns GetRegisteredResourcesResponse
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![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 introduces a new batch retrieval method, GetRegisteredResources, to the RegisteredResourcesService, including updates to the Protobuf definitions, Go SDK, and documentation. Feedback suggests improving the Protobuf design by creating a dedicated identifier message instead of reusing the single-resource request message as a field. Additionally, it is recommended to refine the response structure to allow clients to easily correlate returned resources with their requested identifiers, similar to other batch operations in the service.
There was a problem hiding this comment.
Pull request overview
Adds a new batch RPC to the Policy RegisteredResources API to support bulk lookups (intended for ABAC prechecks), along with regenerated Go/Connect/gRPC bindings and OpenAPI/HTML docs.
Changes:
- Added
GetRegisteredResourcesRequest/Responsemessages andGetRegisteredResourcesRPC toRegisteredResourcesService. - Regenerated Go protobuf types plus Connect and gRPC client/server stubs to expose the new RPC.
- Updated OpenAPI and gRPC HTML documentation outputs to include the new endpoint/messages.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| service/policy/registeredresources/registered_resources.proto | Defines new batch request/response messages and adds the batch RPC to the service. |
| sdk/sdkconnect/registeredresources.go | Exposes the new batch RPC on the SDK Connect wrapper interface and implementation. |
| protocol/go/policy/registeredresources/registeredresourcesconnect/registered_resources.connect.go | Regenerated Connect transport client/handler code to include the new RPC. |
| protocol/go/policy/registeredresources/registered_resources.pb.go | Regenerated Go protobuf types for the new messages and updated descriptors. |
| protocol/go/policy/registeredresources/registered_resources_grpc.pb.go | Regenerated gRPC client/server stubs and service descriptor to include the new RPC. |
| protocol/go/policy/objects.pb.go | Regenerated output with minor comment formatting change. |
| protocol/go/authorization/authorization.pb.go | Regenerated output with minor comment formatting changes. |
| docs/openapi/policy/subjectmapping/subject_mapping.openapi.yaml | Regenerated docs with minor formatting change. |
| docs/openapi/policy/registeredresources/registered_resources.openapi.yaml | Adds OpenAPI path + schemas for the new batch RPC. |
| docs/openapi/policy/objects.openapi.yaml | Regenerated docs with minor formatting change. |
| docs/openapi/authorization/authorization.openapi.yaml | Regenerated docs with minor formatting changes. |
| docs/grpc/index.html | Regenerated gRPC HTML docs to include the new messages and RPC. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/grpc/index.html`:
- Around line 14950-14952: The documentation for the required identifiers field
is missing the maximum batch size; update the identifiers field description to
mention the request cap (max 250 identifiers) and clarify that each identifier
supports the same oneof (id or name) and optional namespace context as
GetRegisteredResourceRequest; locate the identifiers description near the
existing GetRegisteredResourceRequest mention and append a short sentence like
"Maximum 250 identifiers per request" so clients know the limit.
🪄 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: 992a6c44-244e-4a3d-b014-5d27ea36bdfc
⛔ Files ignored due to path filters (4)
protocol/go/authorization/authorization.pb.gois excluded by!**/*.pb.goprotocol/go/policy/objects.pb.gois excluded by!**/*.pb.goprotocol/go/policy/registeredresources/registered_resources.pb.gois excluded by!**/*.pb.goprotocol/go/policy/registeredresources/registered_resources_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (8)
docs/grpc/index.htmldocs/openapi/authorization/authorization.openapi.yamldocs/openapi/policy/objects.openapi.yamldocs/openapi/policy/registeredresources/registered_resources.openapi.yamldocs/openapi/policy/subjectmapping/subject_mapping.openapi.yamlprotocol/go/policy/registeredresources/registeredresourcesconnect/registered_resources.connect.gosdk/sdkconnect/registeredresources.goservice/policy/registeredresources/registered_resources.proto
Remove regeneration drift in authorization.pb.go and objects.pb.go caused by a local buf version difference. Add unimplemented stub for GetRegisteredResources so the service compiles against the updated interface. Signed-off-by: Ron Elliott <ron.elliott@virtru.com>
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Remove the RPC from the service definition and revert all generated files that changed as a result (connect handler interface, grpc stubs, SDK wrapper). Only the message types ship in this release of protocol/go so the service module release job is unaffected. The RPC and service implementation will follow in a subsequent PR after protocol/go is released and service/go.mod is bumped. Signed-off-by: Ron Elliott <ron.elliott@virtru.com>
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
…okup Replace reuse of GetRegisteredResourceRequest with a dedicated RegisteredResourceIdentifier message in GetRegisteredResourcesRequest. Produces cleaner Go oneof wrapper names (RegisteredResourceIdentifier_Id vs GetRegisteredResourceRequest_Id) at the call site. Signed-off-by: Ron Elliott <ron.elliott@virtru.com>
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
c-r33d
left a comment
c-r33d
left a comment
There was a problem hiding this comment.
I would make sure to add tests to:
If you haven't already. Just to make sure your request structure works as expected. You can comment them out to avoid any dep issues with not having released the protos first. Once the protos are released, uncomment.
|
|
||
| message GetRegisteredResourcesRequest { | ||
| // Required - maximum 250 identifiers per request | ||
| repeated RegisteredResourceIdentifier identifiers = 1 [ |
There was a problem hiding this comment.
Is there any concern with uniqueness? As mentioned in the gemini comment?
There was a problem hiding this comment.
for my use case no, but others perhaps. should this be updated to a map structure?
c-r33d
left a comment
c-r33d
left a comment
There was a problem hiding this comment.
Approved. Spoke with Ron about using the GetRegisteredResourceValuesByFQNsRequest, but in their use case they only have the name of the RR, not FQN of the value.
marythought
left a comment
marythought
left a comment
There was a problem hiding this comment.
Thanks for splitting this out so protocol/go can be released ahead of the service-layer implementation — that's the right sequencing.
A couple of consistency issues caught my eye that I think want to be resolved before this merges, plus some ergonomics follow-ups (one of which c-r33d already raised):
Consistency
- The
GetRegisteredResourcesRPC isn't actually declared in theservice RegisteredResourcesService { ... }block inservice/policy/registeredresources/registered_resources.proto. The messages exist and.pb.gois regenerated for them, but there's norpc GetRegisteredResources(...)entry, so the PR title ("add … batch RPC") and body ("AddGetRegisteredResourcesRPC toRegisteredResourcesService") don't match what lands. If the intent is messages-only-now / RPC-later, I'd suggest adjusting the title + description to reflect that ("add proto types for the batch lookup RPC") so future readers and release notes aren't confused. If messages-only is the intent: would also recommend dropping the OpenAPI path additions in this PR so the docs don't promise an endpoint that the proto hasn't registered. docs/openapi/policy/registeredresources/registered_resources.openapi.yamlis out of sync with the proto — it still advertises aPOST .../GetRegisteredResourcesendpoint and itsidentifiers.itemsstill$refsGetRegisteredResourceRequestinstead of the newRegisteredResourceIdentifier. The gencode CI check only validates.pb.go, which is why this slipped past. Worth regenerating, or dropping the OpenAPI changes from this PR if the RPC is intentionally deferred.- The PR body says "Reuses
GetRegisteredResourceRequestas the identifier element type" — the latest revision uses a dedicatedRegisteredResourceIdentifierinstead, so that line is stale.
Ergonomics — inline comments below on:
- Uniqueness on
identifiers(following c-r33d's open question) - Response shape doesn't help callers correlate to inputs or detect partial misses — matters specifically for the ABAC pre-check use case this PR justifies
RegisteredResourceIdentifierduplicatesGetRegisteredResourceRequest's validation byte-for-byte; the two will drift
Happy to chat through any of these if it's easier than going back and forth in comments.
|
|
||
| message GetRegisteredResourcesRequest { | ||
| // Required - maximum 250 identifiers per request | ||
| repeated RegisteredResourceIdentifier identifiers = 1 [ |
There was a problem hiding this comment.
+1 to c-r33d's question on uniqueness. The sibling GetRegisteredResourceValuesByFQNsRequest.fqns already enforces unique: true, and without it here, callers can submit duplicates (or the same resource referenced once by id and once by name+namespace) — wasted work and ambiguous response interpretation. Recommend unique: true unless duplicates are intentionally allowed, in which case a comment explaining why would help.
| ]; | ||
| } | ||
| message GetRegisteredResourcesResponse { | ||
| repeated policy.RegisteredResource resources = 1; |
There was a problem hiding this comment.
Following up on the earlier suggestion about correlation — "callers can correlate using names or ids" works for the happy path, but the use case this PR calls out is ABAC pre-checks before bulk operations, and for that the important question is which of my N inputs were misses. With repeated policy.RegisteredResource resources and no preserved order or "not found" markers, the caller has to diff the request against the response and re-map (which gets messier when a batch mixes id-keyed and name+namespace-keyed identifiers).
Two paths that preserve ergonomics without a big design change:
- Document the contract in the proto comment: "order preserved; missing identifiers are omitted" (or whatever you actually want). Right now it's undefined, which is the part I'd most want to fix before this ships.
- Return paired results — e.g.
repeated GetRegisteredResourceResultwhere each entry carries the input identifier alongside an optional resource. Cheap now, much harder to add later without breaking callers.
Even (1) alone would be a meaningful improvement.
|
|
||
| // Identifier for a single registered resource lookup within a batch request. | ||
| // Mirrors the identifier fields of GetRegisteredResourceRequest. | ||
| message RegisteredResourceIdentifier { |
There was a problem hiding this comment.
Now that this is a dedicated message (which I think is the right call), it's a byte-for-byte duplicate of the identifier fields on GetRegisteredResourceRequest — same oneof, same CEL rule, same namespace context. These will drift the next time someone updates one and forgets the other.
Two options:
- Extract a shared inner message that both wrap (e.g.,
GetRegisteredResourceRequestbecomes{ RegisteredResourceIdentifier identifier = 1; }). One source of truth for validation. - Accept the duplication and add a
// Keep in sync with GetRegisteredResourceRequestcomment so the next person knows.
Either is fine — leaving it implicit is the risky path.
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/policy.registeredresources.GetRegisteredResourceResponse' | ||
| /policy.registeredresources.RegisteredResourcesService/GetRegisteredResources: |
There was a problem hiding this comment.
This yaml is out of sync with the proto on the current branch:
- It registers this
POST .../GetRegisteredResourcesendpoint, but the proto's service block has no such RPC. - Down in the schema definition,
identifiers.items$refsGetRegisteredResourceRequest, but the proto now usesRegisteredResourceIdentifier.
CI's gencode check only validates .pb.go, so the OpenAPI drift slipped through. Re-generate, or drop these OpenAPI changes from this PR if the RPC is being deferred.
This is a big one. Is it better to correlate with guaranteed order from input to output or respond with a map from input name to output We also have a pretty solidified |
I agree with the renaming of the RPC. After speaking with Ron, he mentioned that S4 only has the |
|
@jakedoublev @c-r33d should we maybe change it to GetRegisteredResourcesByName, returning the map structure similar to the ByFQN methods and drop the by ID lookup? this more closely matches our specific use case and broadens the API surface a bit less. |
|
per the discussion we will go a different route so I'm closing this PR |
Summary
GetRegisteredResourcesRequestandGetRegisteredResourcesResponsemessages to the registered resources protoGetRegisteredResourceRequestas the identifier element type (supportsoneofid or name with optional namespace context) to avoid duplicating field definitions; max 250 identifiers per requestGetRegisteredResourcesRPC toRegisteredResourcesServicefor batch lookup intended to support ABAC authorization checks before bulk operationsprotocol/gotypes, connect handlers, SDK wrappers, and API docsNotes
protocol/gomust be released first before bumping inservice/go.modand adding the implementationTest plan
buf lint servicepasses cleanprotocol/go/policy/registeredresources/Summary by CodeRabbit
New Features
Documentation