refactor(policy): Rename key context structures. #2318
Conversation
There was a problem hiding this comment.
Hello @c-r33d, 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!
Summary of Changes
Hello team,
I've put together this pull request to address the naming of the asymmetric key context structures within the policy definitions. As per the title and description, the primary goal is to remove the "Kas" prefix from KasPublicKeyCtx and KasPrivateKeyCtx to make the naming more consistent and less tied to a specific implementation detail (Key Access Server).
This change involves renaming the protobuf messages and updating all references to these types across the codebase, including the generated Go code, OpenAPI specifications for various policy services, and the relevant integration test files. I've also updated the validation rule for the private_key_ctx field in the CreateKeyRequest to be conditionally required based on the key mode, which seems more accurate.
Highlights
- Type Renaming: The protobuf message
KasPublicKeyCtxhas been renamed toPublicKeyCtxandKasPrivateKeyCtxhas been renamed toPrivateKeyCtx. - Reference Updates: All occurrences and references to the old type names (
KasPublicKeyCtx,KasPrivateKeyCtx) have been updated to the new names (PublicKeyCtx,PrivateKeyCtx) across protobuf definitions, generated Go code, OpenAPI specifications, and test files. - Validation Change: The
private_key_ctxfield in theCreateKeyRequestprotobuf message has been updated from being strictly 'Required' to 'Conditionally Required', with validation rules adjusted accordingly. - Documentation Updates: The generated documentation files (gRPC and OpenAPI) have been updated to reflect the new type names and their definitions.
Changelog
Click here to see the changelog
- docs/grpc/index.html
- Removed old
KasPrivateKeyCtxandKasPublicKeyCtxentries from the table of contents. - Added new
PrivateKeyCtxandPublicKeyCtxentries to the table of contents. - Updated references within the
AsymmetricKeydefinition to point to the newPublicKeyCtxandPrivateKeyCtxtypes.
- Removed old
- docs/openapi/policy/actions/actions.swagger.json
- Updated
$refpointers forpublicKeyCtxandprivateKeyCtxto reference the newpolicyPublicKeyCtxandpolicyPrivateKeyCtxdefinitions. - Removed the old
policyKasPrivateKeyCtxandpolicyKasPublicKeyCtxdefinitions. - Added new
policyPrivateKeyCtxandpolicyPublicKeyCtxdefinitions with the same structure as the removed ones.
- Updated
- docs/openapi/policy/attributes/attributes.swagger.json
- Updated
$refpointers forpublicKeyCtxandprivateKeyCtxto reference the newpolicyPublicKeyCtxandpolicyPrivateKeyCtxdefinitions. - Removed the old
policyKasPrivateKeyCtxandpolicyKasPublicKeyCtxdefinitions. - Added new
policyPrivateKeyCtxandpolicyPublicKeyCtxdefinitions with the same structure as the removed ones.
- Updated
- docs/openapi/policy/kasregistry/key_access_server_registry.swagger.json
- Updated
$refpointers forpublicKeyCtxandprivateKeyCtxto reference the newpolicyPublicKeyCtxandpolicyPrivateKeyCtxdefinitions. - Removed the old
policyKasPrivateKeyCtxandpolicyKasPublicKeyCtxdefinitions. - Added new
policyPrivateKeyCtxandpolicyPublicKeyCtxdefinitions with the same structure as the removed ones.
- Updated
- docs/openapi/policy/namespaces/namespaces.swagger.json
- Updated
$refpointers forpublicKeyCtxandprivateKeyCtxto reference the newpolicyPublicKeyCtxandpolicyPrivateKeyCtxdefinitions. - Removed the old
policyKasPrivateKeyCtxandpolicyKasPublicKeyCtxdefinitions. - Added new
policyPrivateKeyCtxandpolicyPublicKeyCtxdefinitions with the same structure as the removed ones.
- Updated
- docs/openapi/policy/registeredresources/registered_resources.swagger.json
- Updated
$refpointers forpublicKeyCtxandprivateKeyCtxto reference the newpolicyPublicKeyCtxandpolicyPrivateKeyCtxdefinitions. - Removed the old
policyKasPrivateKeyCtxandpolicyKasPublicKeyCtxdefinitions. - Added new
policyPrivateKeyCtxandpolicyPublicKeyCtxdefinitions with the same structure as the removed ones.
- Updated
- docs/openapi/policy/resourcemapping/resource_mapping.swagger.json
- Updated
$refpointers forpublicKeyCtxandprivateKeyCtxto reference the newpolicyPublicKeyCtxandpolicyPrivateKeyCtxdefinitions. - Removed the old
policyKasPrivateKeyCtxandpolicyKasPublicKeyCtxdefinitions. - Added new
policyPrivateKeyCtxandpolicyPublicKeyCtxdefinitions with the same structure as the removed ones.
- Updated
- docs/openapi/policy/subjectmapping/subject_mapping.swagger.json
- Updated
$refpointers forpublicKeyCtxandprivateKeyCtxto reference the newpolicyPublicKeyCtxandpolicyPrivateKeyCtxdefinitions. - Removed the old
policyKasPrivateKeyCtxandpolicyKasPublicKeyCtxdefinitions. - Added new
policyPrivateKeyCtxandpolicyPublicKeyCtxdefinitions with the same structure as the removed ones.
- Updated
- docs/openapi/policy/unsafe/unsafe.swagger.json
- Updated
$refpointers forpublicKeyCtxandprivateKeyCtxto reference the newpolicyPublicKeyCtxandpolicyPrivateKeyCtxdefinitions. - Removed the old
policyKasPrivateKeyCtxandpolicyKasPublicKeyCtxdefinitions. - Added new
policyPrivateKeyCtxandpolicyPublicKeyCtxdefinitions with the same structure as the removed ones.
- Updated
- protocol/go/policy/kasregistry/key_access_server_registry.pb.go
- Updated type references for
PublicKeyCtxandPrivateKeyCtxfields and their getter methods inCreateKeyRequestandRotateKeyRequest_NewKey. - Changed the comment for the
PrivateKeyCtxfield inCreateKeyRequestfrom 'Required' to 'Conditionally Required'. - Updated internal protobuf descriptor data to reflect the renaming.
- Updated type references for
- protocol/go/policy/objects.pb.go
- Renamed the
KasPublicKeyCtxstruct toPublicKeyCtx(line 2224). - Renamed the
KasPrivateKeyCtxstruct toPrivateKeyCtx(line 2272). - Updated method receivers and type references for the renamed structs.
- Updated type references for
PublicKeyCtxandPrivateKeyCtxfields and their getter methods in theAsymmetricKeystruct. - Updated internal protobuf descriptor data and type information to reflect the renaming.
- Renamed the
- service/integration/kas_registry_key_test.go
- Updated type references for
PublicKeyCtxandPrivateKeyCtxin test case structs and variable declarations.
- Updated type references for
- service/integration/kas_registry_test.go
- Updated type references for
PublicKeyCtxandPrivateKeyCtxin test case structs and variable declarations.
- Updated type references for
- service/kas/key_indexer_test.go
- Updated type references for
PublicKeyCtxin test case structs.
- Updated type references for
- service/policy/db/utils.go
- Updated the function signature and variable types in
unmarshalPrivatePublicKeyContextto use*policy.PublicKeyCtxand*policy.PrivateKeyCtx.
- Updated the function signature and variable types in
- service/policy/kasregistry/key_access_server_registry.proto
- Renamed the
KasPublicKeyCtxmessage type toPublicKeyCtx(line 501). - Renamed the
KasPrivateKeyCtxmessage type toPrivateKeyCtx(line 506). - Updated field types in
CreateKeyRequestandRotateKeyRequest.NewKeyto use the new message types. - Changed the requirement for the
private_key_ctxfield inCreateKeyRequestfromrequired = trueto a conditional validation rule (line 414).
- Renamed the
- service/policy/kasregistry/key_access_server_registry_keys_test.go
- Updated type references for
PublicKeyCtxandPrivateKeyCtxin variable declarations and test case structs.
- Updated type references for
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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.
A name change sweeps the code,
Old 'Kas' gives way, a lighter load.
Contexts now stand free,
For all the world to see,
A cleaner path is surely sowed.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request primarily focuses on refactoring key context structures by removing the "KAS" prefix from their names (e.g., KasPublicKeyCtx to PublicKeyCtx). This change is consistently applied across protocol definitions, generated Go code, documentation (HTML and OpenAPI), and test files, which should improve clarity and reduce redundancy in naming.
However, a significant observation is the change in the requirement for the private_key_ctx field within the CreateKeyRequest message (in service/policy/kasregistry/key_access_server_registry.proto). Its requirement has been altered from "Required" to "Conditionally Required". This is a semantic API change that goes beyond simple renaming and is not explicitly mentioned in the PR title or description. Such changes need careful consideration, clear documentation, and thorough testing.
Additionally, the PR checklist indicates that updates to unit tests, integration tests, and documentation are still pending. Given the API contract modification, ensuring these are comprehensively updated is critical before merging.
No explicit style guide was provided, so feedback is based on common best practices for Go and Protobuf.
Summary of Findings
- Semantic API Change in
CreateKeyRequest: Theprivate_key_ctxfield in theCreateKeyRequestmessage (defined inservice/policy/kasregistry/key_access_server_registry.proto) has been changed from being unconditionally "Required" to "Conditionally Required". This is a functional API change that alters the contract for clients. This change was not explicitly mentioned in the PR description, which focuses on renaming. This needs to be confirmed as intentional, justified, clearly documented in the field comments, and thoroughly tested. - Incomplete PR Checklist: The PR checklist indicates that unit tests, integration tests, and documentation updates are still pending (
[ ]). Given the semantic API change noted above, it's particularly important to ensure that all tests are updated or new tests are added to cover the new conditional logic and that documentation accurately reflects all changes.
Merge Readiness
The refactoring of key context structure names is a positive step for clarity and has been applied consistently. However, there are critical concerns that need addressing before this PR can be considered ready for merging:
- Semantic API Change: The modification of
private_key_ctxinCreateKeyRequestfrom "Required" to "Conditionally Required" is a significant API contract change. This needs to be explicitly acknowledged in the PR description, its rationale provided, and its implications fully understood and documented. The field comment in the proto definition should also be updated to reflect this conditionality clearly. - Pending Tests and Documentation: The PR checklist indicates that unit tests, integration tests, and documentation updates are not yet completed. These are essential, especially to validate the new conditional logic introduced by the API change and to ensure all documentation accurately reflects the current state of the API.
Due to these points, particularly the unconfirmed semantic change and the pending tests, I recommend that these changes be addressed. I am unable to approve the pull request in its current state. Please ensure these concerns are resolved and consider further reviews if necessary before merging.
| // Conditionally Required | ||
| PrivateKeyCtx private_key_ctx = 6; // Context or additional data specific to the private key, based on the key provider implementation |
There was a problem hiding this comment.
The requirement for private_key_ctx in CreateKeyRequest has changed from "Required" to "Conditionally Required". This is a semantic change to the API contract, not just a refactor as implied by the PR title ("Rename key context structures").
Could you please:
- Confirm if this change is intentional? If so, could you update the PR description to reflect this functional change and provide a brief justification for it?
- Consider updating the field comment for
private_key_ctx(currently "Context or additional data specific to the private key, based on the key provider implementation") to more explicitly clarify the conditions under which it's required? For example, you could reference the conditions outlined in the CEL ruleprivate_key_ctx_optionally_required(e.g., "Conditionally Required. Required if key_mode is LOCAL or PROVIDER_ROOT_KEY. Context or additional data..."). This would improve clarity in generated documentation. - Ensure that the corresponding validation logic (the CEL rule
private_key_ctx_optionally_required) accurately and comprehensively reflects this new conditional requirement across all key modes? - The PR checklist indicates tests are not yet updated. It's crucial to add or update tests to cover this new conditional logic thoroughly.
Benchmark results, click to expandBenchmark Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
🤖 I have created a release *beep* *boop* --- ## [0.3.6](protocol/go/v0.3.5...protocol/go/v0.3.6) (2025-05-27) ### Features * **policy:** Update key status's and UpdateKey rpc. ([#2315](#2315)) ([7908db9](7908db9)) * **policy** Rename key context structures. ([#2318](#2318)) ([4cb28a9](4cb28a9)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com> Co-authored-by: Chris Reed <87077975+c-r33d@users.noreply.github.com>
Proposed Changes
1.) Remove KAS naming from Asymmetric Key Context.
Checklist
Testing Instructions