feat(policy): DSPX-2541 add GetKeyMappingsByFqns API#3632
Conversation
Add a narrow client-side key-split read API to the attributes service: proto, regenerated protocol/go, handler, DB projection, and tests in one PR. Per requested attribute value FQN it returns the attribute rule and the effective KAS keys, resolved with value > definition > namespace precedence over the SimpleKasKey model (mirroring sdk/granter.go). Replaces the full attribute+value graph returned by GetAttributeValuesByFqns for the encrypt/splits path. Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 adds a new, optimized API endpoint, 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. New API for keys so bright, Resolving splits with all our might. From value down to namespace deep, The secrets safe we vow to keep. 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 a new GetKeyMappingsByFqns RPC endpoint to the AttributesService to support client-side key split construction by returning only the governing attribute rule and effective KAS keys. The implementation includes protobuf updates, database resolution logic with value-definition-namespace precedence, and corresponding unit and integration tests. Feedback suggests improving structured logging in the service handler by using slog.Any instead of string formatting, and adding explicit nil checks in the key resolution logic for defensive programming and readability.
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.
|
|
||
| mappings, err := s.dbClient.GetKeyMappingsByFqns(ctx, req.Msg) | ||
| if err != nil { | ||
| return nil, db.StatusifyError(ctx, s.logger, err, db.ErrTextGetRetrievalFailed, slog.String("fqns", fmt.Sprintf("%v", req.Msg.GetFqns()))) |
There was a problem hiding this comment.
Using fmt.Sprintf("%v", ...) to log a slice of strings in slog is inefficient and prevents structured log formatters (like JSON) from outputting it as a proper array. Use slog.Any instead to log the slice directly.
| return nil, db.StatusifyError(ctx, s.logger, err, db.ErrTextGetRetrievalFailed, slog.String("fqns", fmt.Sprintf("%v", req.Msg.GetFqns()))) | |
| return nil, db.StatusifyError(ctx, s.logger, err, db.ErrTextGetRetrievalFailed, slog.Any("fqns", req.Msg.GetFqns())) |
| func resolveEffectiveKasKeys(value *policy.Value, attr *policy.Attribute) []*policy.SimpleKasKey { | ||
| if keys := value.GetKasKeys(); len(keys) > 0 { | ||
| return keys | ||
| } | ||
| if keys := attr.GetKasKeys(); len(keys) > 0 { | ||
| return keys | ||
| } | ||
| if keys := attr.GetNamespace().GetKasKeys(); len(keys) > 0 { | ||
| return keys | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
While protobuf generated getters are nil-safe, explicitly checking for nil values before accessing their properties is highly recommended for defensive programming and readability. This is especially important here because value can be nil when allow_traversal is enabled, and explicit checks make this business logic clear to future maintainers who might otherwise refactor the code to use direct field access (which would panic on nil).
func resolveEffectiveKasKeys(value *policy.Value, attr *policy.Attribute) []*policy.SimpleKasKey {
if value != nil {
if keys := value.GetKasKeys(); len(keys) > 0 {
return keys
}
}
if attr != nil {
if keys := attr.GetKasKeys(); len(keys) > 0 {
return keys
}
if ns := attr.GetNamespace(); ns != nil {
if keys := ns.GetKasKeys(); len(keys) > 0 {
return keys
}
}
}
return nil
}
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
…ingsByFqns make connect-wrapper-generate (go run ./sdk/codegen) regenerates sdk/sdkconnect from protocol/go; the new RPC adds a wrapper method that CI's generated-code freshness check requires to be committed. Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
|
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
GetKeyMappingsByFqns, a narrow client-side key-split read API on the attributes service: proto, regeneratedprotocol/go, handler, DB projection, and tests, all in one PR (per repo convention for adding an RPC, e.g. 4c3d53d).ruleand the effective KAS keys, resolved server-side with value > definition > namespace precedence over theSimpleKasKeymodel (mirroringsdk/granter.gonewGranterFromService). Replaces the full attribute+value graph thatGetAttributeValuesByFqnsreturns for the encrypt/splits path.Part of DSPX-2541. The companion
GetEntitleableAttributesByFqnsAPI is a stacked follow-up. DeprecatingGetAttributeValuesByFqnsand the SDK/decisioning migrations are tracked separately.Limitation: values configured only with legacy
KeyAccessServergrants (nokas_keys) return an empty key set;kas_keysis the forward model.Checklist
Testing Instructions
go test ./service/policy/db/ -run TestResolveEffectiveKasKeysgo test ./service/integration/ -run TestAttributesSuite/Test_GetKeyMappingsByFqnsgolangci-lint run ./policy/db/... ./policy/attributes/...— 0 issues.Related