Skip to content

feat(authz): authz v2 GetBulkDecision #2448

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 8 commits into from
Jun 17, 2025
Merged

feat(authz): authz v2 GetBulkDecision #2448

merged 8 commits into from
Jun 17, 2025

Conversation

jakedoublev
Copy link
Contributor

@jakedoublev jakedoublev commented Jun 16, 2025 •
edited
Loading

Proposed Changes

  • return NotFound instead of Internal gRPC code when FQN in GetDecision not found in policy
  • implement RPC logic under GetBulkDecision
  • add protovalidate unit tests for GetBulkDecision

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

@github-actions github-actions bot added external-contributor External Org Member comp:authorization size/s and removed external-contributor External Org Member labels Jun 16, 2025
gemini-code-assist[bot]
gemini-code-assist bot reviewed Jun 16, 2025
View reviewed changes
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary of Changes

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!

This pull request focuses on enhancing the robustness and clarity of error handling within the authorization v2 service. It refines how specific 'not found' errors are communicated to clients and improves the internal consistency and inspectability of error messages related to fully qualified names (FQNs). While the PR title suggests a new GetBulkDecision feature, the actual changes primarily address error handling for existing decision-making logic, which is crucial for any authorization system.

Highlights

  • Improved Error Handling for Authorization Decisions: The GetDecision and GetDecisionMultiResource methods in the authorization service now provide more specific error responses. Instead of a generic connect.CodeInternal, they now return connect.CodeNotFound when authorization-related errors like access.ErrFQNNotFound or access.ErrDefinitionNotFound occur. This helps clients differentiate between internal server issues and resource/definition not found scenarios.
  • Refined Error Messaging and Wrapping: The error message for access.ErrFQNNotFound has been slightly reworded for clarity. Additionally, the way this error is wrapped in pdp.go has been updated to use fmt.Errorf with the %w verb, ensuring proper error chaining and making it easier to inspect the underlying cause of an error.
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.

Authz checks the path, Errors now show clear intent, Code finds its true way.

Footnotes

  1. 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. ↩

gemini-code-assist[bot]
gemini-code-assist bot reviewed Jun 16, 2025
View reviewed changes
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request primarily refines error handling within the authorization service (v2), particularly for GetDecision and GetDecisionMultiResource methods. Key improvements include:

  • Specific Error Codes: access.ErrFQNNotFound and access.ErrDefinitionNotFound now result in connect.CodeNotFound being returned to the client, which is more informative than a generic internal server error.
  • Improved Error Messages: The message for ErrFQNNotFound has been made more generic.
  • Correct Error Wrapping: Errors are now properly wrapped in service/internal/access/v2/pdp.go, enabling robust error checking using errors.Is.

These changes enhance the API's clarity and usability for clients.

Important Considerations:

  • PR Title Mismatch: The current title, "feat(authz): authz v2 GetBulkDecision," does not seem to align with the implemented changes, which are focused on error handling in existing endpoints. Please consider updating the title to accurately reflect the scope of this PR.
  • PR Description & Checklist: The pull request description is empty, and the checklist items (unit tests, integration tests, documentation) are unchecked. It's important to:
    • Provide a clear description of the changes and their rationale.
    • Ensure that unit tests are added or updated to cover these new error handling paths, specifically verifying that connect.CodeNotFound is returned under the new conditions.
    • Update any relevant documentation if these error code changes impact API contracts or behavior.

Overall, the code changes are beneficial for error reporting and handling.

@github-actions GitHub Actions
Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 548.255008ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 351.045755ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 340.581405ms
Throughput 293.62 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 36.89914621s
Average Latency 365.5525ms
Throughput 135.50 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 26.252851219s
Average Latency 261.695053ms
Throughput 190.46 requests/second

@github-actions GitHub Actions
Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 530.699069ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 349.405632ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 346.627772ms
Throughput 288.49 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 36.398125266s
Average Latency 362.037154ms
Throughput 137.37 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 25.609575962s
Average Latency 255.099634ms
Throughput 195.24 requests/second

@github-actions GitHub Actions
Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 543.393712ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 346.596083ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 332.588407ms
Throughput 300.67 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 36.183547424s
Average Latency 359.488375ms
Throughput 138.18 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 25.191497383s
Average Latency 250.901552ms
Throughput 198.48 requests/second

@jakedoublev jakedoublev marked this pull request as ready for review June 16, 2025 21:06
@jakedoublev jakedoublev requested a review from a team as a code owner June 16, 2025 21:06
@jakedoublev jakedoublev enabled auto-merge June 16, 2025 21:13
@jakedoublev
Copy link
Contributor Author

/gemini review

gemini-code-assist[bot]
gemini-code-assist bot reviewed Jun 16, 2025
View reviewed changes
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces the GetBulkDecision RPC, enhances error reporting by using more specific gRPC codes, and updates associated unit tests. The core functionality seems well-implemented. The primary concern is the direct modification of shared test data in Test_GetDecisionBulkRequest_Succeeds, which could lead to flaky tests; this should be addressed by cloning test objects before modification. The TODO regarding performance in GetBulkDecision is noted and seems appropriate for a follow-up.

@github-actions GitHub Actions
Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 529.397855ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 337.124813ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 344.379972ms
Throughput 290.38 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 36.375865313s
Average Latency 361.337876ms
Throughput 137.45 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 25.374373094s
Average Latency 252.54362ms
Throughput 197.05 requests/second

@github-actions GitHub Actions
Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 539.95752ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 332.617466ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 343.016041ms
Throughput 291.53 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 36.665539252s
Average Latency 364.545256ms
Throughput 136.37 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 26.110134823s
Average Latency 260.076543ms
Throughput 191.50 requests/second

ryanulit
ryanulit previously approved these changes Jun 17, 2025
View reviewed changes
@github-actions GitHub Actions
Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 548.323886ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 358.2169ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 344.729857ms
Throughput 290.08 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 36.464030831s
Average Latency 362.132686ms
Throughput 137.12 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 26.034589779s
Average Latency 259.532974ms
Throughput 192.05 requests/second

@github-actions GitHub Actions
Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 543.267171ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 354.526851ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 353.553574ms
Throughput 282.84 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 37.990446421s
Average Latency 378.240267ms
Throughput 131.61 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 26.464455752s
Average Latency 263.581088ms
Throughput 188.93 requests/second

@jakedoublev jakedoublev added this pull request to the merge queue Jun 17, 2025
Merged via the queue into main with commit 0da3363 Jun 17, 2025
29 checks passed
@jakedoublev jakedoublev deleted the feat/DSPX-1207-bulk branch June 17, 2025 15:43
@opentdf-automation opentdf-automation bot mentioned this pull request Jun 17, 2025
Merged
github-merge-queue bot pushed a commit that referenced this pull request Jun 24, 2025
@opentdf-automation
chore(main): release service 0.7.0 (#2406)
71be9d1 
🤖 I have created a release *beep* *boop*
---


##
[0.7.0](service/v0.6.0...service/v0.7.0)
(2025-06-24)


### ⚠ BREAKING CHANGES

* **policy:** disable kas grants in favor of key mappings
([#2220](#2220))

### Features

* **authz:** Add caching to keycloak ERS
([#2466](#2466))
([f5b0a06](f5b0a06))
* **authz:** auth svc registered resource GetDecision support
([#2392](#2392))
([5405674](5405674))
* **authz:** authz v2 GetBulkDecision
([#2448](#2448))
([0da3363](0da3363))
* **authz:** cache entitlement policy within authorization service
([#2457](#2457))
([c16361c](c16361c))
* **authz:** ensure logging parity between authz v2 and v1
([#2443](#2443))
([ef68586](ef68586))
* **core:** add cache manager
([#2449](#2449))
([2b062c5](2b062c5))
* **core:** consume RPC interceptor request context metadata in logging
([#2442](#2442))
([2769c48](2769c48))
* **core:** DSPX-609 - add cli-client to keycloak provisioning
([#2396](#2396))
([48e7489](48e7489))
* **core:** ERS cache setup, fix cache initialization
([#2458](#2458))
([d0c6938](d0c6938))
* inject logger and cache manager to key managers
([#2461](#2461))
([9292162](9292162))
* **kas:** expose provider config from key details.
([#2459](#2459))
([0e7d39a](0e7d39a))
* **main:** Add Close() method to cache manager
([#2465](#2465))
([32630d6](32630d6))
* **policy:** disable kas grants in favor of key mappings
([#2220](#2220))
([30f8cf5](30f8cf5))
* **policy:** Restrict deletion of pc with used key.
([#2414](#2414))
([3b40a46](3b40a46))
* **sdk:** allow Connect-Protocol-Version RPC header for cors
([#2437](#2437))
([4bf241e](4bf241e))


### Bug Fixes

* **core:** remove generics on new platform cache manager and client
([#2456](#2456))
([98c3c16](98c3c16))
* **core:** replace opentdf-public client with cli-client
([#2422](#2422))
([fb18525](fb18525))
* **deps:** bump github.com/casbin/casbin/v2 from 2.106.0 to 2.107.0 in
/service in the external group
([#2416](#2416))
([43afd48](43afd48))
* **deps:** bump github.com/opentdf/platform/protocol/go from 0.4.0 to
0.5.0 in /service
([#2470](#2470))
([3a73fc9](3a73fc9))
* **deps:** bump github.com/opentdf/platform/sdk from 0.4.7 to 0.5.0 in
/service ([#2473](#2473))
([ad37476](ad37476))
* **deps:** bump the external group across 1 directory with 2 updates
([#2450](#2450))
([9d8d1f1](9d8d1f1))
* **deps:** bump the external group across 1 directory with 2 updates
([#2472](#2472))
([d45b3c8](d45b3c8))
* only request a token when near expiration
([#2370](#2370))
([556d95e](556d95e))
* **policy:** fix casing bug and get provider config on update.
([#2403](#2403))
([a52b8f9](a52b8f9))
* **policy:** properly formatted pem in test fixtures
([#2409](#2409))
([54ffd23](54ffd23))

---
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryanulit ryanulit ryanulit approved these changes

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
comp:authorization size/s
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jakedoublev @ryanulit