feat(policy): DSPX-2541 add narrow attribute read APIs#3634
feat(policy): DSPX-2541 add narrow attribute read APIs#3634alkalescent wants to merge 4 commits into
Conversation
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 two purpose-built read RPCs to the attributes service, replacing the broader 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. Two new paths for data to flow, Narrow reads to make it go. Keys and rules in tidy sets, Optimized for what it gets. Footnotes
|
github-actions
Bot
added
comp:db
![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 two new narrow read APIs to the AttributesService: GetKeyMappingsByFqns (for client-side key split construction) and GetEntitleableAttributesByFqns (for server-side entitlement resolution). These APIs allow fetching targeted policy data without loading the entire policy. The changes include new Protobuf definitions, database query implementations, and comprehensive tests. The review feedback suggests optimizing the SQL query getSubjectMappingsByValueFqns by casting standard_actions and custom_actions to ::jsonb. This enables sqlc to generate these fields as []byte instead of interface{}, allowing the Go code to pass the raw bytes directly to unmarshalAllActionsProto and completely avoid the overhead of json.Marshal.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Warning Review limit reached
More reviews will be available in 37 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (12)
📝 WalkthroughWalkthroughTwo new narrow read-path RPCs — ChangesFQN-based Narrow Read RPCs
Sequence Diagram(s)sequenceDiagram
participant Client
participant AttributesService
participant PolicyDBClient
participant GetAttributesByValueFqns
participant getSubjectMappingsByValueFqns
participant resolveEffectiveKasKeys
rect rgba(70, 130, 180, 0.5)
Note over Client, resolveEffectiveKasKeys: GetKeyMappingsByFqns
Client->>AttributesService: GetKeyMappingsByFqns(fqns)
AttributesService->>PolicyDBClient: GetKeyMappingsByFqns(req)
PolicyDBClient->>GetAttributesByValueFqns: resolve attributes/values by FQN
GetAttributesByValueFqns-->>PolicyDBClient: map[fqn]→Attribute+Value
loop per FQN
PolicyDBClient->>resolveEffectiveKasKeys: value > definition > namespace keys
resolveEffectiveKasKeys-->>PolicyDBClient: []SimpleKasKey
end
PolicyDBClient-->>AttributesService: map[fqn]→AttributeKeyMapping
AttributesService-->>Client: GetKeyMappingsByFqnsResponse
end
rect rgba(60, 179, 113, 0.5)
Note over Client, getSubjectMappingsByValueFqns: GetEntitleableAttributesByFqns
Client->>AttributesService: GetEntitleableAttributesByFqns(fqns)
AttributesService->>PolicyDBClient: GetEntitleableAttributesByFqns(req)
PolicyDBClient->>GetAttributesByValueFqns: resolve attributes/values by FQN
PolicyDBClient->>getSubjectMappingsByValueFqns: fetch value-level subject mappings
getSubjectMappingsByValueFqns-->>PolicyDBClient: []row with value_fqn + mappings
PolicyDBClient-->>AttributesService: map[fqn]→EntitleableAttribute
AttributesService-->>Client: GetEntitleableAttributesByFqnsResponse
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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 |
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:
|
|
@dmihalcik-virtru Would you mind taking a look as the author of the |
| AttributeRuleTypeEnum rule = 2; | ||
| // effective KAS keys resolved for this value (value > definition > namespace | ||
| // precedence, key mappings preferred over legacy grants); may be empty | ||
| repeated policy.SimpleKasKey keys = 3; |
There was a problem hiding this comment.
I think we may need to clarify in the response whether there are keys assigned at the attribute value level, definition level, and namespace level for each of the requested attribute value FQNs. If I have a key on only some of the attribute values under a single definition, it seems like it wouldn't be possible to easily tell from this API response if the specific keys I got in this list were for the value, definition, or namespace, unless it's more of a tuple with 3 indexes than a list?
There was a problem hiding this comment.
Dave will definitely be able to tell you, but the granter does need to be able to tell what split to make; which is derived from where the key lives. (value, def, namespace)
There was a problem hiding this comment.
In the key assignment algorithm, the most specific key is picked - for example, if there is a key on a value, that is preferred to keys on the attribute or namespace. IIRC if assigned two values from one attribute, and one has a key and the other does not, I think then both keys from the attr (or namespace) and value are used. For example if we have assignments:
a -> k1
a/x -> k2
a/y -> none
If a is an and, you'll get a split over k1 and k2, and if a is an or you'll get a share over k1 and k2.
There was a problem hiding this comment.
Thanks, Dave. I'll keep the flat repeated SimpleKasKey keys since resolveEffectiveKasKeys returns the most specific keys per value (value > definition > namespace), and each entry carries the attribute rule.
| }]; | ||
| } | ||
| message GetEntitleableAttributesByFqnsResponse { | ||
| message EntitleableAttribute { |
There was a problem hiding this comment.
Some ideas about this response:
- Would it be an over-optimization if we only provide the entire list of values back for hierarchy attributes? If we have an any_of or all_of attribute, when decisioning in the PDP, we only need to know the rule and compare against the actual attributes in the decision request (on the TDF), such as /attr/project/value/abc and attr/project/value/xyz. If an entity is entitled to attr/project/value/jkl, it doesn't matter to the decision for any_of and all_of attributes, but that logic doesn't hold up for hierarchy attributes where entitlement to one of the values that is not present on the resource in the decision request could result in entitlement to the resource.
- Should we restructure the response so that we only return all of the values for each definition once? This way, if we have an attribute definition with 10k values and have to make a decision on a request with anywhere from 1-10k attribute values under that single definition, we only get the definition with all of its values 1 time instead of
ntimes in the response back?
There was a problem hiding this comment.
These are some good ideas to fine-tune the entitlements response; I've incorporated both into the changes.
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: 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 `@service/integration/attributes_test.go`:
- Around line 1672-1688: The test currently validates that lowFqn exists in
resp.GetFqnEntitleableAttributes() with correct properties, but does not assert
that the map contains only the requested value and no other FQNs. Add an
assertion to verify the length of resp.GetFqnEntitleableAttributes() equals 1 or
add explicit checks that highFqn and midFqn are not present in the map, ensuring
the response does not inadvertently over-return entitleable attributes beyond
what was requested.
🪄 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: e9a97052-0f55-4a9a-9338-b3656bc84e4c
⛔ Files ignored due to path filters (1)
protocol/go/policy/attributes/attributes.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (6)
docs/grpc/index.htmldocs/openapi/policy/attributes/attributes.openapi.yamlservice/integration/attributes_test.goservice/policy/attributes/attributes.goservice/policy/attributes/attributes.protoservice/policy/db/attribute_fqn.go
Add two purpose-built read RPCs to the attributes service, replacing the 'get everything' GetAttributeValuesByFqns for two distinct callers: - GetKeyMappingsByFqns (client-side key splits): per value FQN returns the attribute rule and effective KAS keys, resolved value > definition > namespace over the SimpleKasKey model (mirrors sdk/granter.go). - GetEntitleableAttributesByFqns (server-side decisioning): per value FQN returns the rule, value id, ordered definition value FQNs (for hierarchy), and value-level subject mappings via a new lean getSubjectMappingsByValueFqns query. Includes proto, regenerated protocol/go + docs, the sdk connect wrapper, handlers, DB methods/SQL, and tests. Updates the hand-written AttributesServiceClient mocks (myAttributesClient, paginatedMockAttributesClient) for the new interface methods. Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
…tion Cast standard_actions/custom_actions to ::jsonb in getSubjectMappingsByValueFqns so sqlc generates []byte; pass the raw bytes directly to unmarshalAllActionsProto, removing the json.Marshal round-trip (addresses review feedback; aligns with the CPU-reduction goal). Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
alkalescent
force-pushed
the
DSPX-2541-attribute-read-apis
branch
from
f16cd0d to
6adaa3e
Compare
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
4 participants
Proposed Changes
Add two purpose-built read RPCs to the attributes service, replacing the "get everything"
GetAttributeValuesByFqnsfor two distinct callers:GetKeyMappingsByFqns(client-side key splits) — per value FQN returns the attributeruleand the effective KAS keys, resolved server-side with value > definition > namespace precedence over theSimpleKasKeymodel (mirrorssdk/granter.go). Limitation: legacy-grant-only values (nokas_keys) return an empty key set.GetEntitleableAttributesByFqns(server-side decisioning) — per value FQN returns therule, value id, the definition's ordered value FQNs (for hierarchy rule logic), and value-level subject mappings, fetched via a new leangetSubjectMappingsByValueFqnsquery (single roundtrip, not the full-policy load).Both responses are flat FQN-keyed maps. Includes proto, regenerated
protocol/go+ docs, the sdk connect wrapper, handlers, DB methods/SQL, tests, and updates to the hand-writtenAttributesServiceClientmocks for the new interface methods.Checklist
Testing Instructions
go test ./service/policy/db/ -run TestResolveEffectiveKasKeysgo test ./service/integration/ -run 'TestAttributesSuite/Test_GetKeyMappingsByFqns|TestAttributesSuite/Test_GetEntitleableAttributesByFqns'go test ./service/authorization/...golangci-lint run ./policy/... ./authorization/...— 0 issues.Related
Summary by CodeRabbit
GetKeyMappingsByFqns(returns effective key-mapping data for key-split construction) andGetEntitleableAttributesByFqns(returns entitlement-relevant attribute data for access decisioning). Both accept attribute-value identifiers and return mapping/definition/value data without side effects.