-
Notifications
You must be signed in to change notification settings - Fork 139
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
feat(parameters): SecretsProvider support #1206
feat(parameters): SecretsProvider support #1206
Conversation
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.
Great work!
Should we have tests for the transformation in providers or having them in BaseProvider
is enough?
I would add JSDoc for class and methods with a couple of examples (from the PR description).
It looks good to me.
const sdkOptions: GetSecretValueCommandInput = { | ||
SecretId: name, | ||
}; | ||
if (options?.sdkOptions) { | ||
this.removeNonOverridableOptions(options.sdkOptions as GetSecretValueCommandInput); | ||
Object.assign(sdkOptions, options.sdkOptions); | ||
} |
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.
Explicit arguments passed to the constructor will take precedence over ones passed to the method.
I looked at the comment above the removeNonOverridableOptions
function and instead of explicitly deleting properties, spread options.sdkOptions
first (if any) and then overwrite them with name
would work:
const sdkOptions: GetSecretValueCommandInput = {
...(options?.sdkOptions || {}),
SecretId: name,
};
It may be less readable, but I like it more than duplicating removeNonOverridableOptions
in every provider. Am I missing something?
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.
Good point, I have applied your suggestion. Thank you!
Thank you for the review!
I think testing in the
Definitely, we have a dedicated issue for it (#1043). When doing the planning I put it in a separate one because I thought it'd make more sense to do it once the code is more or less stable. I expect to do this sometime towards the end of the integration tests & before the beta release. |
Description of your changes
This PR introduces the
SecretsProvider
class which will allow the future Parameters utility to support retrieving secrets from AWS Secrets Manager.The
SecretsProvider
supports two retrieval modes, both for retrieving one secret at the time. This is in line with the current functionalities supported by the Parameters utility in Powertools for Python.get
this class method allows customers to retrieve a single secret by name and allows to specify aspects like the transformation to apply (none, binary, json, automatic) and max age (aka time to live for the item in the cache).The class method will be used as such:
getSecret
utility function that behaves the same as the the method, but doesn't require instantiating a class.In both cases users can configure how long the secret should be cached in the Lambda's execution environment:
Secrets Manger allows to store secrets both as
SecretString
andSecretBinary
. Both of them can be base64-encoded.Using the provider or helper function, users can optionally specify a transform (
json
,binary
, orauto
) and SecretsProvider will try to transform the string or binary accordingly. When usingauto
, the name of the secret must have a suffix that specifies the transform to use (i.e.my-secret.json
will result in ajson
transform). If no transform is specified, the secret will be returned as-is (akastring
orUint8Array
).Examples:
Finally, users can also customize the behavior of the underlying AWS SDK client calls by passing an optional
sdkOptions
of typeGetSecretValueCommandInput
in the second argument:Or, for class-based usage only, they can pass their own SDK client config (of type
SecretsManagerClient
) altogether while initialising the instance:How to verify this change
See the newly added unit test cases & optionally compare also the API surface with the implementation found in Powertools for Python.
Related issues, RFCs
Issue number: #1175
PR status
Is this ready for review?: YES
Is it a breaking change?: NO
Checklist
Breaking change checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.