-
Notifications
You must be signed in to change notification settings - Fork 24
feat(core): Actually use KeyManager ProviderConfig #2837
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.
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
Conversation
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
- Looking up a key now loads the `manager` field in the key provider - This is required, as the manager should be the factory key, not the name - This allows us to load the same manager multiple times with different configs. HOWEVER the config is only looked at on the first load; we should update this so it evicts and reloads the provider if the config changes. This will come in a follow-up.
f000ac8 to
02094df
Compare
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
|
/gemini review |
There was a problem hiding this 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 refactors the key manager loading mechanism to use ProviderConfig, allowing for more flexible configurations. The changes are well-contained, and the removal of the legacy KeyManagerFactory without context is a good cleanup. The updates to use KeyManagerCtxFactories across the services are consistent. My review includes a couple of suggestions to improve error message clarity for better debuggability.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
|
/backport |
### Proposed Changes - Looking up a key now loads the `manager` field in the key provider - This is required, as the manager should be the factory key, not the name - This allows us to load the same manager multiple times with different configs. HOWEVER the config is only looked at on the first load; we should update this so it evicts and reloads the provider if the config changes. This hopefully will come in a follow-up. Similarly, we don't have much in the way of integration tests for these, since we don't include a key manager that takes a config. I'll look into starting the Vault sample plugin back up and running. While I'm here, since our downstream deps no longer create them, I've removed support for the `KeyManagerFactory` that does *not* take a context object. ### Checklist - [x] 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 65ba2e0)
|
Successfully created backport PR for |
### Proposed Changes - Looking up a key now loads the `manager` field in the key provider - This is required, as the manager should be the factory key, not the name - This allows us to load the same manager multiple times with different configs. HOWEVER the config is only looked at on the first load; we should update this so it evicts and reloads the provider if the config changes. This hopefully will come in a follow-up. Similarly, we don't have much in the way of integration tests for these, since we don't include a key manager that takes a config. I'll look into starting the Vault sample plugin back up and running. While I'm here, since our downstream deps no longer create them, I've removed support for the `KeyManagerFactory` that does *not* take a context object. ### Checklist - [x] 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 65ba2e0)
Proposed Changes
managerfield in the key providerHOWEVER the config is only looked at on the first load; we should update this so it evicts and reloads the provider if the config changes. This hopefully will come in a follow-up.
Similarly, we don't have much in the way of integration tests for these, since we don't include a key manager that takes a config. I'll look into starting the Vault sample plugin back up and running.
While I'm here, since our downstream deps no longer create them, I've removed support for the
KeyManagerFactorythat does not take a context object.Checklist
Testing Instructions