feat(policy): DSPX-2998 optionally namespace resource mappings#3567
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (9)
💤 Files with no reviewable changes (9)
📝 WalkthroughWalkthroughTwo new Goose SQL migrations add a nullable ChangesResourceMapping Namespace Ownership
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as resourcemapping.go
participant DBLayer as resource_mapping.go
participant NsResolver as resolveOwningNamespace helper
participant NamespacesSvc as namespaces package
participant DB as PostgreSQL
Client->>Handler: CreateResourceMapping(namespace_fqn, group_id, ...)
Handler->>Handler: NamespacedPolicy check (reject if no ns/group)
Handler->>DBLayer: CreateResourceMapping(req)
DBLayer->>NsResolver: resolveOwningNamespaceForMapping(namespaceFqn, groupId)
NsResolver->>NamespacesSvc: GetNamespaceByFqn(fqn)
NamespacesSvc->>DB: SELECT id FROM attribute_namespaces WHERE fqn=?
DB-->>NsResolver: namespace UUID
NsResolver->>DB: SELECT namespace_id FROM resource_mapping_groups WHERE id=groupId
DB-->>NsResolver: group namespace UUID
NsResolver-->>DBLayer: resolved namespaceID (or ErrNamespaceMismatch)
DBLayer->>DB: INSERT resource_mappings(..., namespace_id=$5)
DB-->>DBLayer: row with namespace JSON
DBLayer-->>Handler: ResourceMapping{Namespace: {...}}
Handler-->>Client: CreateResourceMappingResponse
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 docstrings
🧪 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.
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:
|
6f5d979 to
7080723
Compare
- Add --namespace-id and --namespace-fqn to resource-mappings create and update, and as filters on resource-mappings list. - Add --namespace-id and --namespace-fqn filters to resource-mapping-groups list. - Surface the owning namespace in command output. Stacked on the service PR (#3567). 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:
|
a8fa30f to
b8879a8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/openapi/policy/resourcemapping/resource_mapping.openapi.yaml (1)
1394-1432:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDocument the optional namespace mutual-exclusion contract in OpenAPI.
At Line 1394 (and similarly at Line 1518, Line 1587, Line 1643, and Line 1686), the schemas allow both
namespaceIdandnamespaceFqnsimultaneously, but the proto message-level oneof inservice/policy/resourcemapping/resource_mapping.protoenforces at-most-one. This creates client/server contract drift (OpenAPI-valid payloads can fail server validation).Suggested schema pattern for each affected request
policy.resourcemapping.CreateResourceMappingRequest: type: object + not: + required: [namespaceId, namespaceFqn] properties: namespaceId: type: string format: uuid namespaceFqn: type: string minLength: 1 format: uriAlso applies to: 1518-1532, 1587-1610, 1643-1662, 1686-1728
🤖 Prompt for 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. In `@docs/openapi/policy/resourcemapping/resource_mapping.openapi.yaml` around lines 1394 - 1432, The OpenAPI schemas for CreateResourceMappingRequest and related request types do not enforce the mutual-exclusion constraint between namespaceId and namespaceFqn that exists in the proto message definition (via oneof). Update the schema definitions at docs/openapi/policy/resourcemapping/resource_mapping.openapi.yaml lines 1394-1432 (CreateResourceMappingRequest), 1518-1532, 1587-1610, 1643-1662, and 1686-1728 to use OpenAPI's oneOf pattern to enforce that only one of namespaceId or namespaceFqn can be provided at a time, ensuring the OpenAPI contract matches the protobuf oneof constraint and prevents clients from sending invalid payloads that would fail server validation.
🤖 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.
Outside diff comments:
In `@docs/openapi/policy/resourcemapping/resource_mapping.openapi.yaml`:
- Around line 1394-1432: The OpenAPI schemas for CreateResourceMappingRequest
and related request types do not enforce the mutual-exclusion constraint between
namespaceId and namespaceFqn that exists in the proto message definition (via
oneof). Update the schema definitions at
docs/openapi/policy/resourcemapping/resource_mapping.openapi.yaml lines
1394-1432 (CreateResourceMappingRequest), 1518-1532, 1587-1610, 1643-1662, and
1686-1728 to use OpenAPI's oneOf pattern to enforce that only one of namespaceId
or namespaceFqn can be provided at a time, ensuring the OpenAPI contract matches
the protobuf oneof constraint and prevents clients from sending invalid payloads
that would fail server validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 65d44bc1-bb3d-428c-97ec-aa58b9fd822e
⛔ Files ignored due to path filters (2)
protocol/go/policy/objects.pb.gois excluded by!**/*.pb.goprotocol/go/policy/resourcemapping/resource_mapping.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (21)
docs/grpc/index.htmldocs/openapi/authorization/authorization.openapi.yamldocs/openapi/policy/actions/actions.openapi.yamldocs/openapi/policy/attributes/attributes.openapi.yamldocs/openapi/policy/objects.openapi.yamldocs/openapi/policy/obligations/obligations.openapi.yamldocs/openapi/policy/registeredresources/registered_resources.openapi.yamldocs/openapi/policy/resourcemapping/resource_mapping.openapi.yamldocs/openapi/policy/subjectmapping/subject_mapping.openapi.yamldocs/openapi/policy/unsafe/unsafe.openapi.yamlservice/integration/resource_mappings_test.goservice/policy/db/migrations/20260604000000_add_namespace_to_resource_mappings.sqlservice/policy/db/migrations/20260605000000_backfill_resource_mapping_namespace.sqlservice/policy/db/models.goservice/policy/db/queries/resource_mapping.sqlservice/policy/db/resource_mapping.goservice/policy/db/resource_mapping.sql.goservice/policy/objects.protoservice/policy/resourcemapping/resource_mapping.goservice/policy/resourcemapping/resource_mapping.protoservice/policy/resourcemapping/resource_mapping_test.go
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>
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>
Wire the new namespace_fqn option for resource mapping groups: extract a resolveNamespaceID helper (id or fqn -> namespace UUID) and use it in CreateResourceMappingGroup and UpdateResourceMappingGroup. Add integration tests for creating and updating a group by namespace FQN. Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
7080723 to
011c840
Compare
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
…igration suite - ListResourceMappings namespace filter tests now create mappings in two namespaces and assert only the filtered namespace's mapping is returned. - Move the backfill test into migration_test.go's goose harness as TestMigrationData_ResourceMappingNamespaceBackfill, exercising the real 20260605000000 migration (grouped mapping backfilled from its group, ungrouped mapping stays global). Addresses review feedback from c-r33d. 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:
|
|
- Add --namespace-id and --namespace-fqn to resource-mappings create and update, and as filters on resource-mappings list. - Add --namespace-id and --namespace-fqn filters to resource-mapping-groups list. - Surface the owning namespace in command output. Stacked on the service PR (#3567). Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
2 participants
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
Summary by CodeRabbit
New Features
Chores