feat: allow endpoint override for secrets manager plugin #707
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sergiyvamz
approved these changes
Oct 27, 2023
crystall-bitquill
force-pushed
the
issue-630
branch
from
67e3c9a
to
53c4969
Compare
brunos-bq
reviewed
Oct 27, 2023
| .endpointOverride(endpointURI) | ||
| .region(region) | ||
| .build(); | ||
| } catch (URISyntaxException e) { |
There was a problem hiding this comment.
is this the only exception we can have from the endpoint override?
What happens if the syntax is ok but the endpoint doesn't lead anywhere?
is there a way to see all the exceptions this call can throw?
| @@ -36,6 +36,7 @@ AwsCredentialsManager.nullProvider=The configured AwsCredentialsProvider was nul | |||
| AwsSdk.unsupportedRegion=Unsupported AWS region ''{0}''. For supported regions please read https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/Concepts.RegionsAndAvailabilityZones.html | |||
|
|
|||
| # AWS Secrets Manager Connection Plugin | |||
| AwsSecretsManagerConnectionPlugin.endpointOverrideMisconfigured=The provided endpoint could not be used to create a URI: ``{0}``. | |||
There was a problem hiding this comment.
NIT: is it worth using a scarier word like incorrect or invalid?
| @@ -72,6 +74,9 @@ public class AwsSecretsManagerConnectionPlugin extends AbstractConnectionPlugin | |||
| public static final AwsWrapperProperty REGION_PROPERTY = new AwsWrapperProperty( | |||
| "secretsManagerRegion", "us-east-1", | |||
| "The region of the secret to retrieve."); | |||
| public static final AwsWrapperProperty ENDPOINT_PROPERTY = new AwsWrapperProperty( | |||
There was a problem hiding this comment.
NIT: should we add SECRETS_MANAGER prefix to both properties?
There was a problem hiding this comment.
I think it's ok without the prefix, since these properties are only used in the plugin and there isn't any ambiguity on what the they are.
| |--------------------------|:------:|:--------------------------------------------------------:|:-------------------------------------------------------------------------|:------------------------|---------------| | ||
| | `secretsManagerSecretId` | String | Yes | Set this value to be the secret name or the secret ARN. | `secretId` | `null` | | ||
| | `secretsManagerRegion` | String | Yes unless the `secretsManagerSecretId` is a Secret ARN. | Set this value to be the region your secret is in. | `us-east-2` | `us-east-1` | | ||
| | `secretsManagerEndpoint` | String | No | Set this value to be the endpoint override to retrieve your secret from. | `http://localhost:1234` | `null` | |
There was a problem hiding this comment.
We need to indicate that it is a URL in the description, and also indicate which formats are OK or not (ex: port mandatory or not)
crystall-bitquill
force-pushed
the
issue-630
branch
3 times, most recently
from
e869b09
to
0e838c9
Compare
crystall-bitquill
force-pushed
the
issue-630
branch
from
0e838c9
to
d7e26bd
Compare
brunos-bq
approved these changes
Oct 31, 2023
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Allow endpoint override for secrets manager plugin
Description
secretsManagerEndpointto allow users to set the endpoint override for secret retrieval in the secrets manager pluginAddresses #630
Additional Reviewers
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.