fix(NODE-5053): enforce empty map for kmsProvider auto credentials #565
Conversation
nbbeeken
requested review from
durran,
baileympearson,
dariakp and
addaleax
as code owners
| ); | ||
| } | ||
|
|
||
| let awsCredentialProviders = null; |
There was a problem hiding this comment.
Btw, it is so nice that this package does dependency injection for the driver package – It might even be worth adapting that to this dependency as well … :)
There was a problem hiding this comment.
How come you think this is a beneficial pattern? The DI for the driver is something I've wanted to remove for a while
There was a problem hiding this comment.
Well, from a very practical point, it’s just a necessity in cases where the driver is bundled into something but this package isn’t (which would be common in bundling scenarios given that this package is a native addon package).
But also just philosophically (and I’m pretty sure you already know this), I think DI is a great design pattern that solves more issues than it creates 🙂
There was a problem hiding this comment.
Just to clarify, this thought is not directly related to the changes in this PR and should absolutely not block it :)
There was a problem hiding this comment.
We should consider this though, using it for testing too could be 🔥
For GCP we don't plan to use an SDK (and likely not for azure either) so this would only be for AWS but still, valid use case even for one dep.
Description
What is changing?
We strictly check for an existing object but with zero keys for a kmsProviders credentials before attempting auto fetching.
Is there new documentation needed for these changes?
Yes, this could be surprising to existing AWS users that were able to leave kmsProviders empty. They need to provide an 'aws' key set to
{}What is the motivation for this change?
The kmsProviders settings needs to have a key that exists but is set to an empty object in order to enable auto obtaining credentials. Otherwise future credential providers will both be obtained without actually requesting it.
Double check the following
npm run check:lintscripttype(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript