Skip to content

Conversation

dmihalcik-virtru
Copy link
Member

Proposed Changes

  • Some of our newer KMS implementations do some health checks and other things during their init phase.
  • This was always intended as an option for the init, so we always should have allowed passing in a context
  • I made this an explicit parameter to satisfy the golang linter that prevents context from being a parameter (in KeyManagerFactoryOptions
  • This means we need to have new types for a lot of things
  • and to avoid breaking changes while downstream service implementations pick this up, this implements bidirectional compatibility, upgrading factories that don't take context to ignore it, and downgrading services that don't set it to use Background context

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

- Some of our newer KMS implementations do some health checks and other things during their init phase.
- This was always intended as an option for the init, so we always should have allowed passing in a context
- I made this an explicit parameter to satisfy the golang linter that prevents context from being a parameter (in `KeyManagerFactoryOptions`
- This means we need to have new types for a lot of things
- and to avoid breaking changes while downstream service implementations pick this up, this implements bidirectional compatibility, upgrading factories that don't take context to ignore it, and downgrading services that don't set it to use `Background` context
Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

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

Benchmark authorization.v2.GetMultiResourceDecision Results:

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

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 362.561514ms
Throughput 275.82 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 37.903096114s
Average Latency 377.577669ms
Throughput 131.92 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 26.30867758s
Average Latency 262.087075ms
Throughput 190.05 requests/second

@dmihalcik-virtru dmihalcik-virtru marked this pull request as ready for review September 12, 2025 12:49
@dmihalcik-virtru dmihalcik-virtru requested a review from a team as a code owner September 12, 2025 12:49
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds context support to key manager factories to enable health checks and other initialization operations that require context. The changes implement bidirectional compatibility by introducing new context-aware interfaces while maintaining backward compatibility with existing implementations.

Key changes:

  • Introduces KeyManagerFactoryCtx type and NamedKeyManagerCtxFactory struct for context-aware key manager creation
  • Updates DelegatingKeyService to use context-aware factories internally while providing compatibility shims
  • Modifies service registration and startup flow to pass both legacy and context-aware factory types

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
service/trust/key_manager.go Adds new NamedKeyManagerCtxFactory struct for context-aware factories
service/trust/delegating_key_service.go Updates core delegation logic to use context-aware factories with backward compatibility
service/pkg/serviceregistry/serviceregistry.go Adds KeyManagerCtxFactories field to registration parameters
service/pkg/server/start_test.go Updates test to use new context-aware factory parameter names
service/pkg/server/start.go Updates server startup to pass context-aware factories
service/pkg/server/services_test.go Removes unused import and updates test parameter structure
service/pkg/server/services.go Implements bidirectional compatibility between legacy and context-aware factories
service/pkg/server/options.go Adds new option functions for context-aware factories with legacy compatibility

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

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

Benchmark authorization.v2.GetMultiResourceDecision Results:

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

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 353.922974ms
Throughput 282.55 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 37.409766216s
Average Latency 372.411712ms
Throughput 133.65 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 26.490312322s
Average Latency 263.941364ms
Throughput 188.75 requests/second

@jrschumacher
Copy link
Member

@dmihalcik-virtru I talked with @strantalis and this is currently experimental so we don't need to deprecate. Can we add an // Experimental comment and remove the deprecation?

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.473606ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

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

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 347.756601ms
Throughput 287.56 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 36.804767845s
Average Latency 365.702495ms
Throughput 135.85 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 25.90737268s
Average Latency 258.379238ms
Throughput 193.00 requests/second

Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

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

Benchmark authorization.v2.GetMultiResourceDecision Results:

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

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 339.91916ms
Throughput 294.19 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 37.317888558s
Average Latency 371.559469ms
Throughput 133.98 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 25.898667603s
Average Latency 257.748781ms
Throughput 193.06 requests/second

@dmihalcik-virtru dmihalcik-virtru added this pull request to the merge queue Sep 16, 2025
Merged via the queue into main with commit 8d70993 Sep 16, 2025
32 checks passed
@dmihalcik-virtru dmihalcik-virtru deleted the DSPX-1668-ctx branch September 16, 2025 16:51
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>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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants