-
Notifications
You must be signed in to change notification settings - Fork 146
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): SSMProvider support #1187
Conversation
642b9bb
to
815ef5c
Compare
815ef5c
to
f84c05d
Compare
7f24e91
to
0ad79de
Compare
public async get(name: string, options?: SSMGetOptionsInterface): Promise<undefined | string | Record<string, unknown>>; | ||
public async get(name: string, options?: GetOptionsInterface): Promise<undefined | string | Record<string, unknown>> { |
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.
Is this function overload? I believe there should be an implementation signature.
Won't parameters with union types work here?
public async get(name: string, options?: GetOptionsInterface | SSMGetOptionsInterface): Promise<undefined | string | Record<string, unknown>>
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.
Yes, it's a function overload which as far as I have been reading, it's purely a TypeScript construct. Since JavaScript proper doesn't really have function overload you are allowed to specify only one implementation (not sure if this answers your question).
I tried union types initially but I ended up with overloading both because of the inputs but also because of the outputs. In this specific case SSMProvider.get
never returns an Uint8Array
while other providers do (i.e. AppConfigProvider
).
If I use the union type the type-checker complains like this:
I'm very open to alternatives though. I have to admit that this is very close to the limits of my TS knowledge so if there's a better / more elegant way that allows users to get the right types when using the library I'm open to consider it.
batch: {}, | ||
decrypt: {}, |
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.
I would suggest renaming these to something more descriptive, like parametersToFetchInBatch
and parametersToDecrypt
since we also have a boolean decrypt
argument it is confusing.
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 callout, I have updated the implementation to use your suggestions.
} | ||
} | ||
|
||
return results; |
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.
I see the consistency of creating a results object at the top of every function which is great. But the name of the variable is generic and from the reader's perspective, I often get to the top of the function and look at what results
object consists of. Since we see the return type in the function signature, I would suggest returning a new object in functions where it is appropriate like this return { cached, toFetch }
, return { response, errors }
and etc.
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 changed all the functions that allowed it according to your suggestion and used more explanatory names for the others that didn't.
I have also re-formatted some of the signatures to be on multiple lines since they were very long.
type SSMGetParametersByNameFromCacheOutputType = { | ||
cached: Record<string, string | Record<string, unknown>> | ||
toFetch: Record<string, SSMGetParametersByNameOptionsInterface> | ||
} & { [key: string]: SSMGetParametersByNameOptionsInterface }; |
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.
I think this intersection may be redundant. Can a single property be in the return object?
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.
Same question for the SSMSplitBatchAndDecryptParametersOutputType
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.
It was, I have removed them.
if (Object.keys(decrypt).length !== Object.keys(parameters).length) { | ||
const { response: decryptResponse, errors: decryptErrors } = await this.getParametersByNameWithDecryptOption(decrypt, configs.throwOnError); | ||
const { response: batchResponse, errors: batchErrors } = await this.getParametersBatchByName(batch, configs.throwOnError, false); | ||
|
||
response = { ...decryptResponse, ...batchResponse }; | ||
// Fail-fast disabled, let's aggregate errors under "_errors" key so they can handle gracefully | ||
if (!configs.throwOnError) { | ||
response[this.errorsKey] = [ ...decryptErrors, ...batchErrors ]; | ||
} | ||
} else { | ||
const { response: batchResponse, errors: batchErrors } = await this.getParametersBatchByName(decrypt, configs.throwOnError, true); | ||
|
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.
I have a question about logic.
What if getParametersBatchByName
gets parameters object with only one parameter? Does it handle it properly by _getParametersByName
? If so, why do we need getParametersByNameWithDecryptOption
to get parameters one by one if they are already filtered as a batch with a decrypt option?
My train of thoughts:
- We split parameters into two groups. The sum of their properties is equal to the
parameters
length anyways. - Call
this.getParametersBatchByName()
withtrue
andfalse
for both groups accordingly. Optionally check for emptiness to make only one call. - Merge and return.
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.
My understanding is that decryption introduces an additional variable in the equation and that this forces us to have to handle parameters that require decryption separately and one-by-one.
The discussion under the PR that introduced the change in Python (link) explains how they settled on this implementation and I think this message, specifically the parts related to KMS, are the reason why we need to treat these ops as separate.
The TL;DR; is that users might try to retrieve params that have been encrypted with different keys that their function might or might not have rights to use. In order to handle this gracefully we need to isolate these calls.
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.
And to answer your first question about the method being called with a single parameter:
- If the parameter doesn't need encryption, the
if
block will be executed and:this.getParametersByNameWithDecryptOption
will be called with an empty object, which will cause an immediate return (also empty)this.getParametersBatchByName
with `false will be called
- If the parameter needs encryption, the
else
block will be executed andthis.getParametersBatchByName
withtrue
will be called
1f4c91e
to
cdb7dbd
Compare
Description of your changes
This PR implements the
SSMProvider
class which allows the future Parameters utility to support retrieving parameters from SSM.The
SSMProvider
supports the following retrieval modes:get
this class method allows customers to retrieve a single parameter by name and allows to specify transform, max age, and decrypt.getMultiple
this class method allows customers to retrieve multiple parameters by path and allows to specify a shared config object that changes the behavior around transform, max age, and decrypt.getParametersByName
this class method allows customers to retrieve multiple parameters by name. It also allows to specify configs around transform, max age, and decrypt both for every single parameter or using a shared config object. Since the configs can vary, the underlying SDK calls must adapt based on the logic described below.getParameter
this is a standalone function that customers can import & call directly without instantiating the class. Aside from that it's an alias to the class method described above.getParameters
this is a standalone function that customers can import & call directly without instantiating the class. Aside from that it's an alias to the class method described above.getParametersByName
this is a standalone function that customers can import & call directly without instantiating the class. Aside from that it's an alias to the class method described above.At the time of writing, the implementation in this PR should have feature parity with the implementation found in Powertools for Python.
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: #1176
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.