Skip to content
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

Support batch secrets retrieval in Parameters module #1530

Open
jeromevdl opened this issue Nov 27, 2023 · 6 comments
Open

Support batch secrets retrieval in Parameters module #1530

jeromevdl opened this issue Nov 27, 2023 · 6 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers v2 Version 2
Milestone

Comments

@jeromevdl
Copy link
Contributor

Is your feature request related to a problem? Please describe.
N/A

Describe the solution you'd like
Add support for https://aws.amazon.com/about-aws/whats-new/2023/11/aws-secrets-manager-batch-retrieval-secrets/ in Parameters module.

@jeromevdl jeromevdl added enhancement New feature or request v2 Version 2 labels Nov 27, 2023
@scottgerring
Copy link
Contributor

This can be done in the v2 branch once #1403 is merged. It will be a good first issue at that point.

@scottgerring scottgerring added the good first issue Good for newcomers label Nov 30, 2023
@scottgerring scottgerring added this to the v2 milestone Dec 21, 2023
@jreijn
Copy link
Contributor

jreijn commented Jan 31, 2024

@scottgerring @jeromevdl I would like to pick this up if possible. It seems #1403 has been merged, so good to go?

@jreijn
Copy link
Contributor

jreijn commented Jan 31, 2024

I took a look at the code base and started with some minor improvements. I noticed that the getValue and the existing getMultipleValues exists, however the later on is now based on path now. I'm thinking of renaming the getMultipleValues to getMultipleValuesByPath and introduce a new method on the BaseProvider called getMultipleValuesByKey(List keys). That way we can support both use cases.
For the new method, I'll see if I can implement a solution in all the providers.

@scottgerring
Copy link
Contributor

Hey @jreijn happy for you to pick this up - I will assign it to you.
I'll take some time tomorrow morning to look at the current ...multipleValues interface and write back!

@scottgerring
Copy link
Contributor

v2 getMultipleValues impls

1. AppConfigProvider - unsupported

protected Map<String, String> getMultipleValues(String path) {
// Retrieving multiple values is not supported with the AppConfig provider.
throw new RuntimeException(
"Retrieving multiple parameter values is not supported with the AWS App Config Provider");
}

2. DynamoDbProvider

Retrieves all values that share the same partition key.

protected Map<String, String> getMultipleValues(String path) {
QueryResponse resp = client.query(QueryRequest.builder()
.tableName(tableName)
.keyConditionExpression("id = :v_id")
.expressionAttributeValues(Collections.singletonMap(":v_id", AttributeValue.fromS(path)))
.build());

3. SSMProvider

Retrieves either everything at the given level of the /parameter/hierarchy, or everything at the given level plus /parameter/hierarchy/nested/levels when using withRecursive.

private Map<String, String> getMultipleBis(String path, String nextToken) {
GetParametersByPathRequest request = GetParametersByPathRequest.builder()
.path(path)
.withDecryption(decrypt)
.recursive(recursive)
.nextToken(nextToken)
.build();

Both the existing implementations - DDB and SSM provider - already vary a little - SSM is path-based, DDB is more like, common key. I'm a bit hesitant to change the existing method name because 1/ its not path-based in both cases and 2/ it's a breaking change against v1 which wouldn't add much. What do you think?

Maybe rather than changing the existing interface, we could add getMultipleValuesByKey(keys) on the BaseProvider, provide a default impl that simply maps getValue over the keys, and then override in SSMProvider ?

Thoughts @jreijn / @jeromevdl ?

@jreijn
Copy link
Contributor

jreijn commented Feb 8, 2024

@scottgerring sounds reasonable. I also noticed that lambda powertools in other languages do something similar. I'll make some small commits while I go along, so I can get some feedback along the way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers v2 Version 2
Projects
None yet
Development

No branches or pull requests

3 participants