fix(policy): remove gRPC gateway in policy except where needed #2382
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
external-contributor
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hello @jakedoublev, 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, gemini-code-assist here to provide a summary of this pull request. This PR focuses on removing the gRPC gateway HTTP bindings from various services within the policy component, with the explicit exception of the GetAttributeValuesByFqns method in the AttributesService. This change primarily affects the AttributesService, KeyAccessServerRegistryService, NamespaceService, ResourceMappingService, and UnsafeService, removing their RESTful HTTP interfaces generated by the gRPC gateway. Corresponding documentation has also been updated to reflect these changes.
Highlights
- gRPC Gateway Removal: Most gRPC gateway HTTP bindings have been removed from the policy service's RPCs.
- Exception for GetAttributeValuesByFqns: The
GetAttributeValuesByFqnsmethod in theAttributesServiceretains its gRPC gateway binding. - Documentation Updates: The
docs/grpc/index.htmlfile has been updated to remove the documentation for the removed HTTP bindings. - Service Registration Changes: The registration of the gRPC gateway handlers has been removed for the affected services in their respective Go files.
Changelog
Click here to see the changelog
- docs/grpc/index.html
- Removed documentation for HTTP bindings for most methods in AttributesService, KeyAccessServerRegistryService, NamespaceService, ResourceMappingService, and UnsafeService.
- Retained documentation for the HTTP binding of
GetAttributeValuesByFqns. - Added
GetAttributeValueto the list of methods withNO_SIDE_EFFECTSidempotency level.
- service/policy/attributes/attributes.proto
- Removed
google.api.httpoptions from most RPCs. - Retained
google.api.httpoption forGetAttributeValuesByFqns. - Added
idempotency_level = NO_SIDE_EFFECTS;option toGetAttributeValue.
- Removed
- service/policy/kasregistry/key_access_server_registry.go
- Removed the registration of the gRPC gateway handler (
GRPCGatewayFunc).
- Removed the registration of the gRPC gateway handler (
- service/policy/kasregistry/key_access_server_registry.proto
- Removed the import of
google/api/annotations.proto. - Removed
google.api.httpoptions from all RPCs.
- Removed the import of
- service/policy/namespaces/namespaces.go
- Removed the registration of the gRPC gateway handler (
GRPCGatewayFunc).
- Removed the registration of the gRPC gateway handler (
- service/policy/namespaces/namespaces.proto
- Removed the import of
google/api/annotations.proto. - Removed
google.api.httpoptions from all RPCs.
- Removed the import of
- service/policy/resourcemapping/resource_mapping.go
- Removed the registration of the gRPC gateway handler (
GRPCGatewayFunc).
- Removed the registration of the gRPC gateway handler (
- service/policy/resourcemapping/resource_mapping.proto
- Removed the import of
google/api/annotations.proto. - Removed
google.api.httpoptions from all RPCs.
- Removed the import of
- service/policy/unsafe/unsafe.proto
- Removed the import of
google/api/annotations.proto. - Removed
google.api.httpoptions from all RPCs. - Removed comment indicating the service was 'IN FLIGHT AND NOT YET IMPLEMENTED!'.
- Removed the import of
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.
HTTP bindings stripped away,
Except for FQNs, they say.
Protobufs now reign,
Less RESTful pain,
In policy's gRPC day.
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. ↩
Contributor
There was a problem hiding this comment.
Code Review
This pull request aims to remove gRPC gateway bindings for most policy services, with a specific exception for the GetAttributeValuesByFqns method in the AttributesService. The changes made to the .proto files, Go service registration files, and the auto-generated HTML documentation appear to correctly implement this intent.
However, there are significant concerns regarding testing and the completeness of the pull request description that need to be addressed.
Summary of Findings
- Missing Tests: The pull request description indicates that no unit or integration tests have been added or updated. Removing API endpoints (gRPC gateway) is a significant change that can impact clients. It's crucial to have tests to verify that:
- The intended endpoints are indeed removed and no longer accessible via HTTP.
- The
GetAttributeValuesByFqnsmethod inAttributesServiceremains accessible via HTTP as intended. - Other services/methods do not inadvertently become accessible or inaccessible.
Without tests, there's a risk of regressions or incorrect behavior.
- Pull Request Description and Documentation: The pull request description is very sparse, with only an asterisk under "Proposed Changes". A more detailed description explaining the motivation, the scope of changes, and potential impact on consumers would be highly beneficial. While the auto-generated gRPC documentation (
docs/grpc/index.html) has been updated, the checklist also mentions general documentation. It's unclear if other forms of documentation (e.g., API guides, client migration notes) require updates due to these endpoint removals. - Code Changes Correctness: The core changes to
.protofiles (removinggoogle.api.httpoptions) and Go service registrations (removingGRPCGatewayFunc) appear to correctly implement the goal of disabling the gRPC gateway for the specified services, while preserving it forGetAttributeValuesByFqns.
Merge Readiness
While the code changes for removing gRPC gateway functionality seem technically correct according to the PR's intent, I strongly recommend against merging this pull request in its current state. The absence of any new or updated tests for such a significant API change is a critical issue. Additionally, the pull request description should be expanded to provide better context for reviewers and future reference.
Specifically, the following should be addressed before this PR is considered for merging:
- Add comprehensive tests: Unit and integration tests are needed to confirm the correct removal of HTTP endpoints and the continued functioning of the
GetAttributeValuesByFqnsendpoint via the gateway. - Expand PR Description: Provide details on the motivation, changes, and any potential impact on clients.
- Confirm Documentation Status: Clarify if any other user-facing documentation needs updates beyond the auto-generated gRPC docs.
As a reviewer, I am not authorized to approve pull requests. Please ensure these concerns are addressed and further reviews are conducted before merging.
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
jakedoublev
changed the title
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
jrschumacher
approved these changes
Jun 4, 2025
Member
|
LGTM breaking change is of no consequence |
3 tasks
This was referenced Jun 4, 2025
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jun 5, 2025
🤖 I have created a release *beep* *boop* --- ## [0.4.0](protocol/go/v0.3.6...protocol/go/v0.4.0) (2025-06-05) ### Features * **authz:** improve v2 request proto validation ([#2357](#2357)) ([f927b99](f927b99)) * **policy:** cache SubjectConditionSet selectors in dedicated column maintained via trigger ([#2320](#2320)) ([215791f](215791f)) * **policy:** Return Simple Kas Keys from non-Key RPCs ([#2387](#2387)) ([5113e0e](5113e0e)) * **policy:** Update simple kas key ([#2378](#2378)) ([09d8239](09d8239)) ### Bug Fixes * **policy:** protovalidate deprecated action types and removal of gRPC gateway in subject mappings svc ([#2377](#2377)) ([54a6de0](54a6de0)) * **policy:** remove gRPC gateway in policy except where needed ([#2382](#2382)) ([1937acb](1937acb)) --- 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>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jun 6, 2025
🤖 I have created a release *beep* *boop* --- ## [0.6.0](service/v0.5.5...service/v0.6.0) (2025-06-06) ### Features * **authz:** DSPX-894 auth svc registered resource GetEntitlement support ([#2358](#2358)) ([a199aa7](a199aa7)) * **authz:** improve v2 request proto validation ([#2357](#2357)) ([f927b99](f927b99)) * **core:** DSPX-608 - Deprecate public_client_id ([#2185](#2185)) ([0f58efa](0f58efa)) * **policy:** Return Simple Kas Keys from non-Key RPCs ([#2387](#2387)) ([5113e0e](5113e0e)) * **policy:** Unique name for the key provider. ([#2391](#2391)) ([bb58b78](bb58b78)) * **policy:** Update simple kas key ([#2378](#2378)) ([09d8239](09d8239)) ### Bug Fixes * **deps:** bump github.com/opentdf/platform/protocol/go from 0.3.6 to 0.4.0 in /service ([#2399](#2399)) ([1c6fa75](1c6fa75)) * **deps:** bump the external group across 1 directory with 21 updates ([#2401](#2401)) ([3d0d4d1](3d0d4d1)) * **policy:** move action sub queries to CTE in sm list and match sql ([#2369](#2369)) ([0fd6feb](0fd6feb)) * **policy:** protovalidate deprecated action types and removal of gRPC gateway in subject mappings svc ([#2377](#2377)) ([54a6de0](54a6de0)) * **policy:** remove gRPC gateway in policy except where needed ([#2382](#2382)) ([1937acb](1937acb)) * **policy:** remove support for creation/updation of SubjectMappings with deprecated proto actions ([#2373](#2373)) ([3660200](3660200)) --- 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>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Aug 9, 2025
🤖 I have created a release *beep* *boop* --- ## [0.7.0](protocol/go/v0.6.2...protocol/go/v0.7.0) (2025-08-08) ### ⚠ BREAKING CHANGES * **policy:** disable kas grants in favor of key mappings ([#2220](#2220)) * **core:** Require go 1.23+ ([#1979](#1979)) ### Features * add ability to retrieve policy resources by id or name ([#1901](#1901)) ([deb4455](deb4455)) * **authz:** authz v2, ers v2 protos and gencode for ABAC with actions & registered resource ([#2124](#2124)) ([ea7992a](ea7992a)) * **authz:** improve v2 request proto validation ([#2357](#2357)) ([f927b99](f927b99)) * **authz:** sensible request limit upper bounds ([#2526](#2526)) ([b3093cc](b3093cc)) * **core:** adds bulk rewrap to sdk and service ([#1835](#1835)) ([11698ae](11698ae)) * **core:** EXPERIMENTAL: EC-wrapped key support ([#1902](#1902)) ([652266f](652266f)) * **core:** Require go 1.23+ ([#1979](#1979)) ([164c922](164c922)) * **core:** v2 ERS with proto updates ([#2210](#2210)) ([a161ef8](a161ef8)) * **policy:** add enhanced standard/custom actions protos ([#2020](#2020)) ([bbac53f](bbac53f)) * **policy:** Add legacy keys. ([#2613](#2613)) ([57370b0](57370b0)) * **policy:** Add list key mappings rpc. ([#2533](#2533)) ([fbc2724](fbc2724)) * **policy:** add obligation protos ([#2579](#2579)) ([50882e1](50882e1)) * **policy:** Add validation to delete keys ([#2576](#2576)) ([cc169d9](cc169d9)) * **policy:** add values to CreateObligationRequest ([#2614](#2614)) ([94535cc](94535cc)) * **policy:** adds new public keys table ([#1836](#1836)) ([cad5048](cad5048)) * **policy:** Allow the deletion of a key. ([#2575](#2575)) ([82b96f0](82b96f0)) * **policy:** cache SubjectConditionSet selectors in dedicated column maintained via trigger ([#2320](#2320)) ([215791f](215791f)) * **policy:** Change return type for delete key proto. ([#2566](#2566)) ([c1ae924](c1ae924)) * **policy:** Default Platform Keys ([#2254](#2254)) ([d7447fe](d7447fe)) * **policy:** disable kas grants in favor of key mappings ([#2220](#2220)) ([30f8cf5](30f8cf5)) * **policy:** DSPX-1018 NDR retrieval by FQN support ([#2131](#2131)) ([0001041](0001041)) * **policy:** DSPX-1057 registered resource action attribute values (protos only) ([#2217](#2217)) ([6375596](6375596)) * **policy:** DSPX-893 NDR define crud protos ([#2056](#2056)) ([55a5c27](55a5c27)) * **policy:** DSPX-902 NDR service crud protos only (1/2) ([#2092](#2092)) ([24b6cb5](24b6cb5)) * **policy:** Finish resource mapping groups ([#2224](#2224)) ([5ff754e](5ff754e)) * **policy:** key management crud ([#2110](#2110)) ([4c3d53d](4c3d53d)) * **policy:** Key management proto ([#2115](#2115)) ([561f853](561f853)) * **policy:** Modify get request to search for keys by kasid with keyid. ([#2147](#2147)) ([780d2e4](780d2e4)) * **policy:** Return KAS Key structure ([#2172](#2172)) ([7f97b99](7f97b99)) * **policy:** Return Simple Kas Keys from non-Key RPCs ([#2387](#2387)) ([5113e0e](5113e0e)) * **policy:** rotate keys rpc ([#2180](#2180)) ([0d00743](0d00743)) * **policy:** Update key status's and UpdateKey rpc. ([#2315](#2315)) ([7908db9](7908db9)) * **policy:** Update simple kas key ([#2378](#2378)) ([09d8239](09d8239)) ### Bug Fixes * add pagination to list public key mappings response ([#1889](#1889)) ([9898fbd](9898fbd)) * **core:** Allow 521 curve to be used ([#2485](#2485)) ([aaf43dc](aaf43dc)) * **core:** Fixes protoJSON parse bug on ec rewrap ([#1943](#1943)) ([9bebfd0](9bebfd0)) * **core:** Update fixtures and flattening in sdk and service ([#1827](#1827)) ([d6d6a7a](d6d6a7a)) * **deps:** bump toolchain in /lib/fixtures and /examples to resolve CVE GO-2025-3563 ([#2061](#2061)) ([9c16843](9c16843)) * **policy:** protovalidate deprecated action types and removal of gRPC gateway in subject mappings svc ([#2377](#2377)) ([54a6de0](54a6de0)) * **policy:** remove gRPC gateway in policy except where needed ([#2382](#2382)) ([1937acb](1937acb)) * **policy:** remove new public keys rpc's ([#1962](#1962)) ([5049bab](5049bab)) * **policy:** remove predefined rules in actions protos ([#2069](#2069)) ([060f059](060f059)) * **policy:** return kas uri on keys for definition, namespace and values ([#2186](#2186)) ([6c55fb8](6c55fb8)) * **sdk:** Fix compatibility between bulk and non-bulk rewrap ([#1914](#1914)) ([74abbb6](74abbb6)) * update key_mode to provide more context ([#2226](#2226)) ([44d0805](44d0805)) --- 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: Krish Suchak <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed Changes
Checklist
Testing Instructions