Refactor secrets cache to sdk client #278
Conversation
Signed-off-by: Tanner Lewis <[email protected]>
lewijacn
requested review from
chelma,
gregschohn,
kartg,
mikaylathompson,
okhasawn and
sumobrian
as code owners
Codecov Report
@@ Coverage Diff @@
## main #278 +/- ##
============================================
+ Coverage 61.93% 62.05% +0.12%
Complexity 617 617
============================================
Files 82 82
Lines 3129 3139 +10
Branches 292 292
============================================
+ Hits 1938 1948 +10
Misses 1014 1014
Partials 177 177
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| public String getBasicAuthHeaderFromSecret(String username, String secretId) { | ||
| String authHeaderString = username + ":" + getSecret(secretId); | ||
| public String getBasicAuthHeaderFromSecret(String username, String secretId) throws ExecutionException, InterruptedException { | ||
| String secretValue = getSecret(secretId).get().secretString(); |
There was a problem hiding this comment.
I don't like having the synchronous method alongside the async one and then being opinionated on exceptions that can come out of the synchronous method that would only happen when dealing with something asynchronous.
You could also return a CompletableFuture by applying the getSecret Output through a finalizer that does everything else. That let's the caller determine how to handle the exceptions and keeps these methods more consistent.
| private static IAuthTransformerFactory buildAuthTransformerFactory(Parameters params) | ||
| throws ExecutionException, InterruptedException { |
There was a problem hiding this comment.
I can't imagine a case where I'd need to make these factories asynchronously. I could see needing to create the AuthTransformers themselves asynchronously, or signing the requests - but this seems like it would always be part of bootstrapping at the top level.
There was a problem hiding this comment.
I guess I am inclined to agree with you. Initially when I set this up I thought this might be used in the Netty pipeline so having async would be useful, but for our current use case I really only see synchronous and supporting the off case where someone's secrets has changed in the middle of a migration doesn't seem immediate. I've refactored to use a synchronous client now
There was a problem hiding this comment.
I also included the fix for the failing test case since I don't plan to merge this to the release branch
Signed-off-by: Tanner Lewis <[email protected]>
* main: [Fetch Migration] Monitoring script for Data Prepper (opensearch-project#264) Refactor secrets cache to sdk client (opensearch-project#278) Update humanReadableLogs script Update primary->source and shadow->target and documentation Add shadowRequest and connectionId to tuple Update tests for CRLF Use CRLF in NettyJsonToByteBufHandler Signed-off-by: Greg Schohn <[email protected]> # Conflicts: # TrafficCapture/trafficCaptureProxyServer/src/test/java/org/opensearch/migrations/trafficcapture/proxyserver/netty/ExpiringSubstitutableItemPoolTest.java # TrafficCapture/trafficReplayer/build.gradle # TrafficCapture/trafficReplayer/src/main/java/org/opensearch/migrations/replay/SourceTargetCaptureTuple.java # TrafficCapture/trafficReplayer/src/main/java/org/opensearch/migrations/replay/TrafficReplayer.java
Description
This reworks the SecretCache to be a SecretsManager async client that retrieves the secret. This will be helpful in the future as needed with integrating with Netty, for now it has a synchronous method for constructing an auth header from a secret.
Issues Resolved
None
Testing
Unit tests
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.