Allow for a list and no searching for keys

Kralizek / AWSSecretsManagerConfigurationExtensions

This repository contains a provider for Microsoft.Extensions.Configuration that retrieves secrets stored in AWS Secrets Manager.

MIT License

219 stars 43 forks source link

Allow for a list and no searching for keys #46

Closed scott-phillips-ah closed 3 years ago

scott-phillips-ah commented 3 years ago

Searching for keys makes the secrets manager configuration require the ListSecrets permission, which some organizations don't like to grant to services. This PR allows users to define a list of secret ARNs to load, and it only loads those. Filtering still works, but I don't see a good reason to use a filter with a defined secrets list.

Test created and passed. Not sure the best way to develop an example

Kralizek commented 3 years ago

Hi @scott-phillips-ah

Thank you for your pull request.

I have two observations:

can we avoid having a list of arns and a flag to set in the options? Basically instead of checking the flag we could check if the list is non empty. Otherwise the user could be misled into an erroneous path by either adding desired arns to the list or setting the flag but not both.
i noticed you changed the styling of some files, please revert those changes.

Thanks.

scott-phillips-ah commented 3 years ago

Addressed all feedback, thanks for the quick review!

Kralizek commented 3 years ago

Hi @scott-phillips-ah, sorry I disappeared.

I'm not totally happy with the name of DefinedSecretArns. Can you suggest something else? I was more inclined towards AcceptedSecretArns but I welcome any other idea.

Also, would you be so kind to update the README file with this addition?

scott-phillips-ah commented 3 years ago

Hey, no worries. Good call on that. I changed it to what you specified, the alternative I'd use is RequiredSecretArns (which would indicate that the following secrets are required for the application to operate).

Kralizek commented 3 years ago

Would the application fail to start if a secret with one of the specified Arns were to be missing? If not, i don't think Required describes the behavior we are introducing.

scott-phillips-ah commented 3 years ago

In our case, it would. But I wouldn't say that is true in every case. We can leave it as it is.

scott-phillips-ah commented 3 years ago

Feel free to merge whenever.

killianhale-earnin commented 3 years ago

Any updates on this? I'd be happy to test or do whatever is needed to get this merged. I'm very excited for the feature ^_^

Kralizek commented 3 years ago

Hi @killianhale-earnin, could you take care of updating the readme? Also if you could try the feature in the wild, using one of the nightly builds, it would be great.

scott-phillips-ah commented 3 years ago

@killianhale-earnin @Kralizek the README has been updated per your request.

Kralizek commented 3 years ago

@scott-phillips-ah @killianhale-earnin

I'll release a new version once #47 is merged in :)