Skip to content

Conversation

c-r33d
Copy link
Contributor

@c-r33d c-r33d commented Sep 3, 2025

Proposed Changes

1.) Implement AddObligationTrigger and RemoveObligationTrigger methods within the Obligation Service
2.) Add proto-validate logic to the above RPCs

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

Copy link
Contributor

github-actions bot commented Sep 5, 2025

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 200.201932ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 98.44725ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 352.911706ms
Throughput 283.36 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 37.587012095s
Average Latency 374.545238ms
Throughput 133.02 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 26.443909801s
Average Latency 263.183915ms
Throughput 189.08 requests/second

@c-r33d
Copy link
Contributor Author

c-r33d commented Sep 5, 2025

/gemini review

Copy link
Contributor

github-actions bot commented Sep 5, 2025

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 173.158793ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 96.008358ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 359.302331ms
Throughput 278.32 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 37.647201155s
Average Latency 375.176125ms
Throughput 132.81 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 25.750788645s
Average Latency 256.570817ms
Throughput 194.17 requests/second

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 effectively implements the AddObligationTrigger and RemoveObligationTrigger functionalities. The introduction of reusable IdFqnIdentifier and IdNameIdentifier messages in protobuf is a great improvement for consistency across the API. The database queries are well-structured and include important validation, such as ensuring entities belong to the same namespace. The addition of comprehensive integration and validation tests is also commendable. I've found one high-severity issue related to logging logic that should be addressed.

Copy link
Contributor

github-actions bot commented Sep 5, 2025

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 166.125659ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 83.166542ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 341.882652ms
Throughput 292.50 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 36.409899083s
Average Latency 362.485048ms
Throughput 137.33 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 25.540257943s
Average Latency 254.508826ms
Throughput 195.77 requests/second

Copy link
Contributor

github-actions bot commented Sep 8, 2025

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 175.549023ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 98.305462ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 525.499642ms
Throughput 190.30 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 36.536163796s
Average Latency 364.167606ms
Throughput 136.85 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 24.80156855s
Average Latency 246.588249ms
Throughput 201.60 requests/second

Copy link
Contributor

@jakedoublev jakedoublev left a comment

Choose a reason for hiding this comment

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

LGTM pending protovalidate bumps needed before this in #2698

Copy link
Contributor

github-actions bot commented Sep 8, 2025

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 170.315444ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 98.090544ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 344.477199ms
Throughput 290.29 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 36.553859999s
Average Latency 364.169299ms
Throughput 136.78 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 25.452338266s
Average Latency 253.622374ms
Throughput 196.45 requests/second

Copy link
Contributor

github-actions bot commented Sep 8, 2025

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 176.642616ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 103.25744ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 358.791347ms
Throughput 278.71 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 37.362777123s
Average Latency 371.479041ms
Throughput 133.82 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 26.803512329s
Average Latency 267.10356ms
Throughput 186.54 requests/second

jakedoublev
jakedoublev previously approved these changes Sep 8, 2025
Copy link
Contributor

github-actions bot commented Sep 8, 2025

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 170.920851ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 97.815907ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 353.205205ms
Throughput 283.12 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 36.97668867s
Average Latency 366.909463ms
Throughput 135.22 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 25.713393702s
Average Latency 256.227365ms
Throughput 194.45 requests/second

@strantalis strantalis merged commit 22d0837 into main Sep 8, 2025
30 of 31 checks passed
@strantalis strantalis deleted the DSPX-1346-obligation-triggerts branch September 8, 2025 17:37
github-merge-queue bot pushed a commit that referenced this pull request Sep 12, 2025
🤖 I have created a release *beep* *boop*
---


##
[0.9.0](protocol/go/v0.8.0...protocol/go/v0.9.0)
(2025-09-11)


### Features

* **policy:** add FQN of obligation definitions/values to protos
([#2703](#2703))
([45ded0e](45ded0e))
* **policy:** Add obligation triggers
([#2675](#2675))
([22d0837](22d0837))
* **policy:** Allow creation and update of triggers on Obligation Values
([#2691](#2691))
([b1e7ba1](b1e7ba1))
* **policy:** Allow for additional context to be added to obligation
triggers ([#2705](#2705))
([7025599](7025599))
* **policy:** obligations + values CRUD
([#2545](#2545))
([c194e35](c194e35))


### Bug Fixes

* **deps:** update protovalidate to v0.14.2 to use new buf validate
MessageOneofRule
([#2698](#2698))
([1cae18e](1cae18e))

---
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 Sep 17, 2025
🤖 I have created a release *beep* *boop*
---


##
[0.10.0](service/v0.9.0...service/v0.10.0)
(2025-09-17)


### ⚠ BREAKING CHANGES

* **policy:** Add manager column to provider configuration for
multi-instance support
([#2601](#2601))

### Features

* **authz:** add obligation policy decision point
([#2706](#2706))
([bb2a4f8](bb2a4f8))
* **core:** add service negation for op mode
([#2680](#2680))
([029db8c](029db8c))
* **core:** Bump default write timeout.
([#2671](#2671))
([6a233c1](6a233c1))
* **core:** Encapsulate&gt;Encrypt
([#2676](#2676))
([3c5a614](3c5a614))
* **core:** Lets key manager factory take context
([#2715](#2715))
([8d70993](8d70993))
* **policy:** add FQN of obligation definitions/values to protos
([#2703](#2703))
([45ded0e](45ded0e))
* **policy:** Add manager column to provider configuration for
multi-instance support
([#2601](#2601))
([a5fc994](a5fc994))
* **policy:** Add obligation triggers
([#2675](#2675))
([22d0837](22d0837))
* **policy:** add protovalidate for obligation defs + vals
([#2699](#2699))
([af5c049](af5c049))
* **policy:** Allow creation and update of triggers on Obligation Values
([#2691](#2691))
([b1e7ba1](b1e7ba1))
* **policy:** Allow for additional context to be added to obligation
triggers ([#2705](#2705))
([7025599](7025599))
* **policy:** Include Triggers in GET/LISTable reqs
([#2704](#2704))
([b4381d1](b4381d1))
* **policy:** obligations + values CRUD
([#2545](#2545))
([c194e35](c194e35))
* use public AES protected key from lib/ocrypto
([#2600](#2600))
([75d7590](75d7590))


### Bug Fixes

* **core:** remove extraneous comment
([#2741](#2741))
([ada8da6](ada8da6))
* **core:** return services in the order they were registered
([#2733](#2733))
([1d661db](1d661db))
* **deps:** bump github.com/opentdf/platform/lib/ocrypto from 0.3.0 to
0.6.0 in /service
([#2714](#2714))
([00354b3](00354b3))
* **deps:** bump github.com/opentdf/platform/protocol/go from 0.7.0 to
0.9.0 in /service
([#2726](#2726))
([9004368](9004368))
* **deps:** bump protocol/go to 0.10.0 in service
([#2734](#2734))
([11e6201](11e6201))
* **deps:** update protovalidate to v0.14.2 to use new buf validate
MessageOneofRule
([#2698](#2698))
([1cae18e](1cae18e))
* **policy:** Registered Resources should consider actions correctly
within Decision Requests
([#2681](#2681))
([cf264a2](cf264a2))
* sanitize db schema identifiers
([#2682](#2682))
([0d3dd94](0d3dd94))

---
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
Labels
comp:db DB component comp:policy Policy Configuration ( attributes, subject mappings, resource mappings, kas registry) comp:sdk A software development kit, including library, for client applications and inter-service communicati docs Documentation size/xl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants