-
Notifications
You must be signed in to change notification settings - Fork 35
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
Added support for regional secrets via Google Cloud Secret Manager #1202
Added support for regional secrets via Google Cloud Secret Manager #1202
Conversation
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.
Nice work! Just a few items to address
.../src/main/java/io/micronaut/gcp/secretmanager/client/DefaultLocationSecretManagerClient.java
Outdated
Show resolved
Hide resolved
.../src/main/java/io/micronaut/gcp/secretmanager/client/DefaultLocationSecretManagerClient.java
Outdated
Show resolved
Hide resolved
...-manager/src/main/java/io/micronaut/gcp/secretmanager/client/DefaultSecretManagerClient.java
Outdated
Show resolved
Hide resolved
.../src/main/java/io/micronaut/gcp/secretmanager/client/DefaultLocationSecretManagerClient.java
Outdated
Show resolved
Hide resolved
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.
Thanks for the contribution @abheda-crest.
I have pushed some small changes:
- Decrete the empty constructor of
SecretManagerFactory
- Keep a single DefaultSecretManagerClient
- Add a regex pattern for the locations
- Made Groovy code in tests a little bit more idiomatic
- Add some nullability and javadoc info tags.
- Small copy corrections.
I hope you don't mind. Again, thanks for the contribution.
Added support for regional secrets via Google Cloud Secret Manager.
Approach:
SecretManagerConfigurationProperties
SecretManagerConfigurationProperties
into theSecretManagerFactory
using constructor injection. This will be used to create theSecretManagerServiceClient
based on the location provided in the configuration.DefaultLocationSecretManagerClient
class that will be used to fetch regional secrets based on the location provided in the configuration. For this, we have injectedSecretManagerConfigurationProperties
.SecretManagerClient
based on the availability of location property in the configuration using@Requires
annotation. i.e. If location field is present, than a bean ofDefaultLocationSecretManagerClient
will be created, else a bean ofDefaultSecretManagerClient
will be created.VersionedSecret
.Alternative Approach and the reason we avoided it:
Rather than developing a separate implementation for the wrapper client
DefaultLocationSecretManagerClient
, we could have introduced a new field forSecretManagerConfigurationProperties
and utilised constructor injection to modify the existing constructor of theDefaultSecretManagerClient
. However, we were concerned that some users might have used this constructor to instantiate the wrapper client. As a result, we decided to pursue the approach outlined above.We've covered the following scenarios for both global as well as regional secrets to test the functionality:
gcp.secret-manager.location=
to the bootstrap configuration, the SecretManagerServiceClient was created using the global endpoint (https://secretmanager.googleapis.com/). Also,DefaultLocationSecretManagerClient
was instantiated, rather than theDefaultSecretManagerClient
itself, which depends on the SecretManagerServiceClient. Consequently, when using the methods of this wrapper location client, we cannot retrieve the regional secrets, and we won’t see any errors. This is because thegetSecret
method manages errors by returning a Mono.empty() object in case of an error.More information about regional secrets: https://cloud.google.com/secret-manager/regional-secrets/data-residency