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

Add override for default region. #88

Merged
merged 9 commits into from
Aug 3, 2022
Merged

Add override for default region. #88

merged 9 commits into from
Aug 3, 2022

Conversation

danmancuso
Copy link
Contributor

Resolves Issue #27

This change allows overriding of the default region by either adding an environment variable or by a setting in the secretsmanager.properties file. Some small refactoring of where we set the PrivateLink DNS endpoint configuration was done in the process.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@simonmarty simonmarty enabled auto-merge (squash) June 30, 2022 22:11
@codecov-commenter
Copy link

codecov-commenter commented Jul 6, 2022

Codecov Report

Merging #88 (f666daa) into master (38b40a9) will increase coverage by 1.70%.
The diff coverage is 89.47%.

@@             Coverage Diff              @@
##             master      #88      +/-   ##
============================================
+ Coverage     72.47%   74.18%   +1.70%     
- Complexity      110      116       +6     
============================================
  Files             9       10       +1     
  Lines           327      337      +10     
  Branches         40       42       +2     
============================================
+ Hits            237      250      +13     
+ Misses           84       81       -3     
  Partials          6        6              
Impacted Files Coverage Δ
...ws/secretsmanager/sql/AWSSecretsManagerDriver.java 80.64% <50.00%> (+2.64%) ⬆️
...tsmanager/util/JDBCSecretCacheBuilderProvider.java 94.11% <94.11%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 38b40a9...f666daa. Read the comment docs.

@@ -92,3 +92,20 @@ The secret being used should be in the JSON format we use for our rotation lambd
...
}
```
### Overriding Default Regions

The default region provider chain is used if no adjustments are made. In order to override this default, use one of the following options listed below.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These provide a way to override the default region and I do like this change for other use cases (so don't revert it), but we also want a way for customers to configure a backup region in order to provide high availability using replicated secrets. There are multiple ways to accomplish this. Some potential options:

  • Allow for a "backupRegion" to be specified via configuration here in the JDBC driver. If present, we can instantiate a second SecretCache with this region. Then in the driver logic, if we can't retrieve the secret in the primary region cache, we can fallback to using the backup SecretCache. There are a couple places where the secret is retrieved from the cache so we would want to split out this logic into a helper function.
  • We can implement the fallback logic in the SecretCache itself. The cache could take a backup region in via configuration and have two different AWSecretsManager clients. If the primary region fails for whatever reason, we can fallback to the backup region.

In both approaches, if a Secret ARN is provided, we probably want to do some automatic region substitution as well.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is spot on with our need. During DR testing, we would like to be able to configure failover secret/region config to be used.

Copy link
Contributor

@willtong1234 willtong1234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm approving the file change overall, but it would be great if you could improve the commit message. Previously those who used the constructor that took in a SecretCache would get the cache overwritten if they have endpoint/region properties specified. This is no longer true. I think it's an improvement, but since it is a backward incompatible change we should at least call that out in the commit message for transparency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants