platform
platform copied to clipboard
feat(policy): disable kas grants in favor of key mappings
Proposed Changes
Checklist
- [ ] I have added or updated unit tests
- [ ] I have added or updated integration tests (if appropriate)
- [ ] I have added or updated documentation
Testing Instructions
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.
/gemini-code-assist review
Thank you for submitting this pull request and for requesting a review. I've reviewed the changes aimed at deprecating KAS grants in favor of key mappings.
First, I'd like to acknowledge the comment from github-actions[bot] regarding the size of this PR. It is indeed a substantial change, reflecting a significant refactoring of the policy service's approach to key associations. While large PRs can sometimes be harder to review, the scope of replacing the KAS grant system with key mappings seems to necessitate a broad set of changes across multiple files, including protobuf definitions, service logic, database interactions, examples, and tests. The changes appear consistent with the stated goal.
Summary of Changes:
The core of this pull request is the deprecation of the older KeyAccessServer based grant system and its replacement with a new system based on Key mappings. This involves:
- Marking old RPCs (
AssignKeyAccessServerToAttribute,RemoveKeyAccessServerFromAttribute,AssignKeyAccessServerToValue,RemoveKeyAccessServerFromValue,AssignKeyAccessServerToNamespace,RemoveKeyAccessServerFromNamespace) as deprecated in the protobuf definitions and implementing them to returnconnect.CodeUnimplementederrors. - Introducing new RPCs for key assignments (
AssignPublicKeyToAttribute,RemovePublicKeyFromAttribute,AssignPublicKeyToValue,RemovePublicKeyFromValue,AssignPublicKeyToNamespace,RemovePublicKeyFromNamespace). - Updating the database schema and queries to store and retrieve
Keyassociations instead ofKeyAccessServergrants. - Refactoring integration tests to use the new key mapping RPCs and verify the updated behavior.
- Updating documentation comments in
.protofiles and the generateddocs/grpc/index.htmlto reflect the deprecation and point to the new key mapping fields (kas_keys). - Removing deprecated commands and variables from the example CLI (
examples/cmd/attributes.go).
Feedback:
- Correctness: The changes appear logically sound for the intended deprecation and replacement. The integration tests have been updated to reflect the new API and data model, which is crucial for verifying correctness.
- Maintainability: Deprecating the old system is a positive step for long-term maintainability, reducing confusion and technical debt. The code changes seem well-contained within the relevant service layers and database interactions.
- Testing: It's good to see that the integration tests have been updated to cover the new key mapping functionality and that tests for the deprecated grant functionality have been removed where appropriate. This helps ensure the new system works as expected.
- Documentation: The updates to the
.protofiles anddocs/grpc/index.htmlare a good start for documenting the deprecation. However, given the significant nature of this change, it would be beneficial to review other documentation files (e.g., Markdown files indocs/andexamples/docs/) to ensure they are updated to reflect the new key mapping system and the deprecation of KAS grants.
Breaking Change and Migration:
As indicated by the ! in the commit title (feat(policy)!), this is a breaking change. Users currently relying on the old KAS grant RPCs will need to update their code to use the new key mapping RPCs. It would be helpful to have a clear migration guide or notes for users upgrading to this version.
Overall, this is a significant and necessary change. The approach to deprecation seems appropriate. Please consider reviewing the broader documentation to ensure users have a smooth transition.
Let me know if you have any questions about this feedback.