feat(policy): ADR namespaces for all policy primitives #3008
Conversation
github-actions
bot
added
comp:policy
Summary of ChangesHello @jrschumacher, 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 a new Architectural Decision Record (ADR-0005) that proposes and details the implementation of namespace scoping for key policy constructs, including Registered Resources, Subject Mappings, Subject Condition Sets, and Actions. The primary goal is to enhance support for multi-authority federation, prevent naming collisions, and provide clearer organizational boundaries. The ADR outlines a comprehensive plan covering schema changes, API modifications, and a phased migration strategy, while also identifying an important open decision regarding cross-namespace query behavior for entitlement decisions. Highlights
🧠 New Feature in Public Preview: You can now enable Memory 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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Namespaces now divide, Policies in order reside, Federation's tide. 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 introduces ADR-0005, a comprehensive proposal to add namespace scoping to several key policy constructs like Registered Resources, Subject Mappings, and Actions. The ADR is exceptionally well-detailed, covering schema changes, API impact, a phased migration strategy, and a thorough breakdown of the work involved.
My review focuses on ensuring the technical proposals are robust. I've identified a potential issue with the proposed unique constraint on the actions table that could fail to enforce global uniqueness for standard actions and have suggested an alternative using partial unique indexes. I also recommended a minor clarification regarding how Registered Resource Values are namespaced to improve the document's clarity.
Overall, this is an excellent and well-thought-out ADR that lays a solid foundation for this important feature.
| ALTER TABLE actions | ||
| DROP CONSTRAINT actions_name_key; | ||
| ALTER TABLE actions | ||
| ADD CONSTRAINT actions_namespace_name_unique | ||
| UNIQUE(namespace_id, name); |
There was a problem hiding this comment.
The proposed unique constraint UNIQUE(namespace_id, name) for the actions table might not correctly enforce global uniqueness for standard actions.
In PostgreSQL, NULL values are not considered equal in unique constraints. This means if namespace_id is NULL for standard actions, you could insert multiple rows with the same name (e.g., multiple 'read' actions with namespace_id = NULL), which would violate the goal of having globally unique standard actions.
To correctly enforce the desired constraints, I suggest using two separate partial unique indexes:
- One for standard actions to ensure
nameis unique whennamespace_idisNULL. - One for custom actions to ensure
(namespace_id, name)is unique whennamespace_idisNOT NULL.
This approach would correctly implement the uniqueness rules described in the ADR for both standard and custom actions.
| ALTER TABLE actions | |
| DROP CONSTRAINT actions_name_key; | |
| ALTER TABLE actions | |
| ADD CONSTRAINT actions_namespace_name_unique | |
| UNIQUE(namespace_id, name); | |
| ALTER TABLE actions | |
| DROP CONSTRAINT actions_name_key; | |
| -- For standard actions (global uniqueness on name) | |
| CREATE UNIQUE INDEX actions_standard_name_unique | |
| ON actions(name) | |
| WHERE namespace_id IS NULL; | |
| -- For custom actions (uniqueness on namespace_id + name) | |
| CREATE UNIQUE INDEX actions_custom_namespace_name_unique | |
| ON actions(namespace_id, name) | |
| WHERE namespace_id IS NOT NULL; |
| Currently, namespaces in OpenTDF primarily partition **Attribute Definitions** (and by extension, Attribute Values). However, several policy constructs remain globally scoped: | ||
|
|
||
| - **Registered Resources** | ||
| - **Registered Resource Values** |
There was a problem hiding this comment.
This list correctly identifies Registered Resource Values as a policy construct that will become namespaced. However, the "Schema Changes" section below does not explicitly mention any changes to the registered_resource_values table.
While it's implied that these values become namespaced through their foreign key relationship with the now-namespaced registered_resources table, the ADR would be clearer if this was explicitly stated.
Consider adding a brief note in the "Schema Changes" section under "Registered Resources" to confirm that registered_resource_values are implicitly namespaced and no direct schema change is needed for them. This would help avoid any ambiguity for the engineers implementing this ADR.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
jrschumacher
changed the title
Adds ADR-0005 proposing namespace scoping for all policy constructs: Registered Resources, Subject Mappings, Subject Condition Sets, and Actions.
Key Decisions
Open Decision
Estimated LOE
~18-24 days including migrations, service layer, CLI, SDKs, testing, and documentation.
Related