Skip to content

Conversation

@dmihalcik-virtru
Copy link
Member

Proposed Changes

The recent change to us the ProviderConfig accidentally makes that field required for all keys already loaded from the policy db. Since most (all) existing keys won't have this set, and any keys imported via the command line tool's import don't have it set, we should keep the old behavior

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

I accidentally require all keys loaded from the policy db to have this configured.
A lot of existing keys won't have this set, and any keys imported via the command line tool's import don't have it set yet
@github-actions
Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

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

Benchmark authorization.v2.GetMultiResourceDecision Results:

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

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 367.559207ms
Throughput 272.07 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 38.634833942s
Average Latency 384.267282ms
Throughput 129.42 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 26.98740605s
Average Latency 268.918776ms
Throughput 185.27 requests/second

@github-actions
Copy link
Contributor

@dmihalcik-virtru dmihalcik-virtru marked this pull request as ready for review October 30, 2025 18:45
@dmihalcik-virtru dmihalcik-virtru requested a review from a team as a code owner October 30, 2025 18:45
sujankota
sujankota previously approved these changes Oct 30, 2025
cshamrick
cshamrick previously approved these changes Oct 30, 2025
@dmihalcik-virtru dmihalcik-virtru added this pull request to the merge queue Oct 30, 2025
@dmihalcik-virtru dmihalcik-virtru removed this pull request from the merge queue due to a manual request Oct 30, 2025
@dmihalcik-virtru dmihalcik-virtru dismissed stale reviews from cshamrick and sujankota via 709d23d October 30, 2025 19:46
@github-actions
Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

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

Benchmark authorization.v2.GetMultiResourceDecision Results:

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

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 366.016688ms
Throughput 273.21 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 38.681655129s
Average Latency 385.310881ms
Throughput 129.26 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 26.951800657s
Average Latency 268.664535ms
Throughput 185.52 requests/second

@dmihalcik-virtru dmihalcik-virtru added this pull request to the merge queue Oct 30, 2025
Merged via the queue into main with commit fb0b99d Oct 30, 2025
38 checks passed
@dmihalcik-virtru dmihalcik-virtru deleted the DSPX-1841-better-defaulting branch October 30, 2025 20:17
@opentdf-automation
Copy link
Contributor

Successfully created backport PR for release/service/v0.11:

opentdf-automation bot pushed a commit that referenced this pull request Oct 30, 2025
### Proposed Changes

The recent change to us the ProviderConfig accidentally makes that field
required for all keys already loaded from the policy db. Since most
(all) existing keys won't have this set, and any keys imported via the
command line tool's import don't have it set, we should keep the old
behavior

### 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

(cherry picked from commit fb0b99d)
opentdf-automation bot added a commit that referenced this pull request Oct 30, 2025
### Proposed Changes

The recent change to us the ProviderConfig accidentally makes that field
required for all keys already loaded from the policy db. Since most
(all) existing keys won't have this set, and any keys imported via the
command line tool's import don't have it set, we should keep the old
behavior

### 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

(cherry picked from commit fb0b99d)
dmihalcik-virtru pushed a commit that referenced this pull request Oct 30, 2025
…se/service/v0.11] (#2859)

# Description
Backport of #2858 to `release/service/v0.11`.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants