feat(policy): DSPX-2998 optionally namespace resource mappings#3567
feat(policy): DSPX-2998 optionally namespace resource mappings#3567alkalescent wants to merge 2 commits into
Conversation
Implement the service side of optional namespacing for resource mappings: - Add a migration adding a nullable namespace_id (FK to attribute_namespaces, ON DELETE CASCADE) plus an index to resource_mappings. - Resolve a mapping's owning namespace from namespace_id/namespace_fqn or its group; a grouped mapping must share the group's namespace. - Remove the constraint forcing the mapped attribute value into the group's namespace, allowing mappings to cross namespaces to the values they map. - Hydrate the namespace on get/list responses and add namespace_id/namespace_fqn filters to ListResourceMappings, plus namespace_fqn to ListResourceMappingGroups. - Enforce a namespace on create when namespaced_policy is enabled (namespace_id, namespace_fqn, or group_id satisfies it). - Update/extend integration tests for the new behavior. Stacked on the proto PR (#3565). 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 implements optional namespacing for resource mappings, enabling better multi-tenant isolation and management. It allows resource mappings to be explicitly associated with a namespace or inherited from a parent group, while decoupling the mapping's owning namespace from the namespace of the attribute values it maps. These changes include database schema migrations, updated query logic, and enhanced API support for filtering and retrieving namespace information. 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 mapping now finds its own home, Across namespaces it's free to roam. With group or with ID, It's clear as can be, No longer in shadows to gloam. 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 nullable namespace_id column to the resource_mappings table, allowing resource mappings to be optionally namespaced independently of the mapped attribute value's namespace. It updates the database schema, models, and SQL queries to support this field, including new filtering capabilities by namespace ID or FQN when listing resource mappings and groups. Additionally, it implements namespace resolution and validation logic during resource mapping creation and updates, and enforces namespace requirements when the NamespacedPolicy configuration is enabled. Feedback on the changes highlights a potential issue in resolveResourceMappingNamespaceID where providing both namespaceID and namespaceFqn in a request results in the FQN being silently ignored without consistency validation.
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.
| switch { | ||
| case namespaceID != "": | ||
| if !pgtypeUUID(namespaceID).Valid { | ||
| return "", db.ErrUUIDInvalid | ||
| } | ||
| case namespaceFqn != "": | ||
| ns, err := c.GetNamespace(ctx, &namespaces.GetNamespaceRequest_Fqn{Fqn: namespaceFqn}) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| namespaceID = ns.GetId() | ||
| } |
There was a problem hiding this comment.
If both namespaceID and namespaceFqn are provided in the request, the switch statement will only execute the first case (namespaceID != "") and silently ignore namespaceFqn without validating that they match. Consider validating that they are consistent if both are provided, or returning an error to prevent unexpected behavior.
if namespaceID != "" {
if !pgtypeUUID(namespaceID).Valid {
return "", db.ErrUUIDInvalid
}
}
if namespaceFqn != "" {
ns, err := c.GetNamespace(ctx, &namespaces.GetNamespaceRequest_Fqn{Fqn: namespaceFqn})
if err != nil {
return "", err
}
if namespaceID != "" && namespaceID != ns.GetId() {
return "", db.ErrNamespaceMismatch
}
namespaceID = ns.GetId()
}
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Add a migration that backfills resource_mappings.namespace_id from the owning group for existing grouped mappings created before the column existed, so namespace filtering works on legacy data. Ungrouped mappings remain global. Idempotent (only fills NULL rows). Adds an integration test for the backfill. 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
Second PR in the stacked series for DSPX-2998. Implements the service side (AC1 + AC2). Stacked on #3565 (proto) — review/merge that first.
namespace_id(FK toattribute_namespaces,ON DELETE CASCADE) + index toresource_mappings.namespace_id/namespace_fqnor its group; a grouped mapping must share its group's namespace (elseErrNamespaceMismatch).namespaceon get/list responses; addnamespace_id/namespace_fqnfilters toListResourceMappingsandnamespace_fqntoListResourceMappingGroups.namespaced_policyis enabled (namespace_id,namespace_fqn, orgroup_idsatisfies it), matching the RR/Actions/SM pattern.Checklist
Testing Instructions
cd service && go test ./integration/ -run TestResourceMappingsSuite(requires Docker for testcontainers).Related