feat(policy): implement narrow attribute read APIs#3697
Conversation
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughImplements ChangesFQN key mapping and entitlement lookups
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AttributesService
participant PolicyDBClient
participant resolveEffectiveKasKeys
AttributesService->>PolicyDBClient: GetKeyMappingsByFqns(fqns)
PolicyDBClient->>PolicyDBClient: GetAttributesByValueFqns(fqns)
PolicyDBClient->>resolveEffectiveKasKeys: resolve value/definition/namespace keys
resolveEffectiveKasKeys-->>PolicyDBClient: effective KAS keys
PolicyDBClient-->>AttributesService: AttributeKeyMapping per FQN
sequenceDiagram
participant AttributesService
participant PolicyDBClient
participant SubjectMappingsQuery
AttributesService->>PolicyDBClient: GetEntitleableAttributesByFqns(fqns)
PolicyDBClient->>PolicyDBClient: GetAttributesByValueFqns(fqns)
PolicyDBClient->>PolicyDBClient: expand hierarchy sibling FQNs
PolicyDBClient->>SubjectMappingsQuery: getSubjectMappingsByValueFqns(valueFqns)
SubjectMappingsQuery-->>PolicyDBClient: subject mapping rows
PolicyDBClient->>PolicyDBClient: hydrateSubjectMappingForEntitlement(rows)
PolicyDBClient-->>AttributesService: entitleable definitions and per-FQN values
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
github-actions
Bot
added
comp:db
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 completes the service-level implementation for the narrow attribute read APIs, following the previously merged protocol and generated code changes. It focuses on providing efficient, performant data retrieval for key mappings and entitleable attributes by optimizing database queries and enforcing correct FQN resolution logic. These changes are part of a multi-stage rollout to ensure release safety. 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. 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. The FQNs dance in a line, To make the policy design, With keys resolved and mappings set, The best performance we shall get. 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 implements the server-side database and service logic for the GetKeyMappingsByFqns and GetEntitleableAttributesByFqns endpoints in the policy service, along with corresponding integration and unit tests. The review feedback suggests minor improvements to these database methods, specifically adding early returns when the input FQN slice is empty to avoid redundant database queries, and normalizing keys to lowercase during subject mapping lookups to ensure case-insensitive robustness.
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.
alkalescent
force-pushed
the
DSPX-2541-attribute-read-apis
branch
from
10d625d to
a40a66d
Compare
alkalescent
force-pushed
the
DSPX-2541-attribute-read-apis-service
branch
from
5d4d9cc to
1ebed0f
Compare
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:
|
![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 1673-1678: The GetKeyMappingsByFqns negative-path subtest
currently only checks that an error is returned, but it should also verify the
specific contract for missing mappings. Update the “errors when a requested fqn
does not exist” case in attributes_test.go to assert the error is db.ErrNotFound
using ErrorIs on the err returned from PolicyClient.GetKeyMappingsByFqns, and
apply the same pattern to any other non-existent FQN subtests in this area.
🪄 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: 4728e400-250b-4050-addf-126cc5ba6902
📒 Files selected for processing (6)
service/integration/attributes_test.goservice/policy/attributes/attributes.goservice/policy/db/attribute_fqn.goservice/policy/db/attribute_fqn_test.goservice/policy/db/queries/subject_mappings.sqlservice/policy/db/subject_mappings.sql.go
alkalescent
force-pushed
the
DSPX-2541-attribute-read-apis
branch
from
a40a66d to
3239ed4
Compare
alkalescent
force-pushed
the
DSPX-2541-attribute-read-apis-service
branch
2 times, most recently
from
abaa330 to
39680c6
Compare
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:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
…#3634) ## Proposed Changes Proto and generated-code portion of the narrow attribute read APIs, split out so the heavy service implementation lands separately and `protocol/go` releases cleanly. Adds two RPCs to `AttributesService`: - **`GetKeyMappingsByFqns`** — per value FQN: the attribute rule and effective KAS keys (value > definition > namespace precedence), for client-side key splits. - **`GetEntitleableAttributesByFqns`** — per value FQN: rule, value id, the definition's ordered value FQNs (hierarchy), and value-level subject mappings, for server-side decisioning. Includes the `.proto`, regenerated `protocol/go`, gRPC/OpenAPI docs, and the generated `sdkconnect` client wrapper. The wrapper and the `AttributesServiceClient` test mocks ship here because CI regenerates them from the proto (`make connect-wrapper-generate`) and requires a clean tree. Server methods are `Unimplemented` stubs. ## PR stack Replaces the previous combined PR per review (separate proto/generated from service logic). Merge order: 1. **this PR** — proto + generated + docs + sdkconnect wrapper 2. #3697 — service implementation (DB, SQL, real handlers, tests) Follow-up feature branch: switch the SDK granter to consume `GetKeyMappingsByFqns` and add an xtest proving key-split parity with the previous SDK. Signed-off-by: Krish Suchak <[email protected]>
Implement GetKeyMappingsByFqns and GetEntitleableAttributesByFqns in the attributes service: DB methods with value > definition > namespace key resolution, a lean subject-mapping-by-FQN query, value-FQN guards (reject allow_traversal definition-only fallbacks), real handlers replacing the proto PR's stubs, integration/unit tests, and updated AttributesServiceClient mocks. Signed-off-by: Krish Suchak <[email protected]>
- early-return on empty fqns in GetKeyMappingsByFqns and GetEntitleableAttributesByFqns to avoid a needless DB round-trip - assert db.ErrNotFound in the non-existent-FQN integration subtests Signed-off-by: Krish Suchak <[email protected]>
Extend resolveEffectiveKasKeys to fall through to grant-derived keys per level (value > definition > namespace, mapped keys preferred within a level), so the RPC returns the effective key set for grant-configured policy instead of an empty set. Grants are converted to SimpleKasKeys from their cached public keys; grants without a cached key are skipped. This lets the SDK stop resolving grants and use the RPC as a complete replacement for the split path. Signed-off-by: Krish Suchak <[email protected]>
Return the subject mapping's own namespace from getSubjectMappingsByValueFqns and hydrate SubjectMapping.Namespace in GetEntitleableAttributesByFqns, so the API carries what the PDP's strict namespaced-entitlements filter requires. Signed-off-by: Krish Suchak <[email protected]>
alkalescent
force-pushed
the
DSPX-2541-attribute-read-apis-service
branch
from
27563e7 to
3f48978
Compare
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
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/policy/db/attribute_fqn.go`:
- Around line 218-238: The value-FQN normalization and validation logic is
duplicated in GetKeyMappingsByFqns and GetEntitleableAttributesByFqns, so
extract it into a shared helper on PolicyDBClient (for example, a
resolveValueFqns-style method) that lowercases the input FQNs, calls
GetAttributesByValueFqns, and rejects any result whose AttributeAndValue has a
nil Value. Update both call sites to use the helper so the
allow_traversal/value-FQN contract lives in one place and cannot drift.
- Around line 208-213: The docstring for GetKeyMappingsByFqns contradicts the
real fallback behavior in resolveEffectiveKasKeys and the legacy-grants test.
Update the comment to say that effective KAS keys are resolved with value >
definition > namespace precedence and that legacy grant-only rules can still
produce keys via grantsToSimpleKasKeys when cached key material is available,
while grants without usable kid/pem are skipped. Keep the wording aligned with
the behavior of GetKeyMappingsByFqns, resolveEffectiveKasKeys, and
grantsToSimpleKasKeys.
🪄 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: 6eabce9c-509b-41dc-af52-8c5ddc95c8ba
📒 Files selected for processing (7)
service/integration/attributes_test.goservice/policy/attributes/attributes.goservice/policy/db/attribute_fqn.goservice/policy/db/attribute_fqn_test.goservice/policy/db/grant_mappings.goservice/policy/db/queries/subject_mappings.sqlservice/policy/db/subject_mappings.sql.go
- correct GetKeyMappingsByFqns docstring to reflect grant fallback (grants resolve per level; only grants without a cached kid/pem are skipped) - extract shared resolveValueFqns helper used by GetKeyMappingsByFqns and GetEntitleableAttributesByFqns (normalize, resolve, reject nil-value) Signed-off-by: Krish Suchak <[email protected]>
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
grant.GetUri() is nil-safe and returns "" for a nil grant, so the grant == nil clause is redundant; the empty-URI check already skips nil grants. Signed-off-by: Krish Suchak <[email protected]>
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
1 participant
Proposed Changes
Service implementation of the narrow attribute read APIs. Stacked on #3634 (proto + generated code).
GetKeyMappingsByFqns/GetEntitleableAttributesByFqnsDB methods with value > definition > namespace key resolution (mirrorssdk/granter.go).getSubjectMappingsByValueFqnsquery (single round-trip; action aggregation restricted to the requested FQNs).allow_traversaldefinition-only fallbacks.Stack
Base is #3634; rebase onto
mainonce it merges.Summary by CodeRabbit
New Features
Bug Fixes