-
Notifications
You must be signed in to change notification settings - Fork 318
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
feat: added regional secret support for secret-manager #3365
base: main
Are you sure you want to change the base?
Changes from all commits
b45b0fc
c75d512
9557d43
6f23874
9f6bf38
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,7 @@ | |
import com.google.cloud.spring.core.Credentials; | ||
import com.google.cloud.spring.core.CredentialsSupplier; | ||
import com.google.cloud.spring.core.GcpScope; | ||
import java.util.Optional; | ||
import org.springframework.boot.context.properties.ConfigurationProperties; | ||
import org.springframework.boot.context.properties.NestedConfigurationProperty; | ||
|
||
|
@@ -43,6 +44,13 @@ public class GcpSecretManagerProperties implements CredentialsSupplier { | |
*/ | ||
private String projectId; | ||
|
||
/** | ||
* Defines the region of the secrets when Regional Stack is used. | ||
* | ||
* <p>When not specified, the secret manager will use the Global Stack. | ||
*/ | ||
private Optional<String> location = Optional.empty(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I think your PR (or the feature you're bringing) is totally cool, I've taken a look at the code in its entirety today, not just the delta shown up here in 'Files changed' view. May I ask questions about the way you've implemented the 'location'? The first one is, why as
The other is, if the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The use of I understand your suggestion regarding variable naming, but I believe the intent is already clear from the type itself ( Let me know your thoughts, and I'm happy to make further adjustments if needed! |
||
|
||
/** | ||
* Whether the secret manager will allow a default secret value when accessing a non-existing | ||
* secret. | ||
|
@@ -71,4 +79,12 @@ public boolean isAllowDefaultSecret() { | |
public void setAllowDefaultSecret(boolean allowDefaultSecret) { | ||
this.allowDefaultSecret = allowDefaultSecret; | ||
} | ||
|
||
public Optional<String> getLocation() { | ||
return location; | ||
} | ||
|
||
public void setLocation(String location) { | ||
this.location = Optional.ofNullable(location).filter(loc -> !loc.isEmpty()); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,19 @@ | ||
package com.google.cloud.spring.autoconfigure.secretmanager; | ||
|
||
import static org.assertj.core.api.Assertions.assertThatCode; | ||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.mockito.ArgumentMatchers.anyString; | ||
import static org.mockito.Mockito.mock; | ||
import static org.mockito.Mockito.when; | ||
|
||
import com.google.cloud.spring.core.GcpProjectIdProvider; | ||
import com.google.cloud.spring.secretmanager.SecretManagerPropertySource; | ||
import com.google.cloud.spring.secretmanager.SecretManagerTemplate; | ||
import org.junit.jupiter.api.Test; | ||
import java.util.Optional; | ||
import org.junit.jupiter.params.ParameterizedTest; | ||
import org.junit.jupiter.params.provider.CsvSource; | ||
import org.springframework.boot.ConfigurableBootstrapContext; | ||
import org.springframework.boot.context.config.ConfigData; | ||
import org.springframework.boot.context.config.ConfigDataLoaderContext; | ||
import org.springframework.boot.context.config.ConfigDataLocation; | ||
|
||
|
@@ -21,18 +26,30 @@ class SecretManagerConfigDataLoaderUnitTests { | |
private final ConfigDataLoaderContext loaderContext = mock(ConfigDataLoaderContext.class); | ||
private final GcpProjectIdProvider idProvider = mock(GcpProjectIdProvider.class); | ||
private final SecretManagerTemplate template = mock(SecretManagerTemplate.class); | ||
private final GcpSecretManagerProperties properties = mock(GcpSecretManagerProperties.class); | ||
private final ConfigurableBootstrapContext bootstrapContext = mock( | ||
ConfigurableBootstrapContext.class); | ||
private final SecretManagerConfigDataLoader loader = new SecretManagerConfigDataLoader(); | ||
|
||
@Test | ||
void loadIncorrectResourceThrowsException() { | ||
@ParameterizedTest | ||
@CsvSource({ | ||
"regional-fake, us-central1", | ||
"fake, " | ||
}) | ||
void loadIncorrectResourceThrowsException(String resourceName, String location) { | ||
when(loaderContext.getBootstrapContext()).thenReturn(bootstrapContext); | ||
when(bootstrapContext.get(GcpProjectIdProvider.class)).thenReturn(idProvider); | ||
when(bootstrapContext.get(SecretManagerTemplate.class)).thenReturn(template); | ||
when(bootstrapContext.get(GcpSecretManagerProperties.class)).thenReturn(properties); | ||
when(template.secretExists(anyString(), anyString())).thenReturn(false); | ||
when(properties.getLocation()).thenReturn(Optional.ofNullable(location)); | ||
SecretManagerConfigDataResource resource = new SecretManagerConfigDataResource( | ||
ConfigDataLocation.of("fake")); | ||
assertThatCode(() -> loader.load(loaderContext, resource)).doesNotThrowAnyException(); | ||
ConfigDataLocation.of(resourceName)); | ||
assertThatCode(() -> { | ||
ConfigData configData = loader.load(loaderContext, resource); | ||
SecretManagerPropertySource propertySource = | ||
(SecretManagerPropertySource) configData.getPropertySources().get(0); | ||
assertThat(Optional.ofNullable(location)).isEqualTo(propertySource.getLocation()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. First, I'm not from Google, so my opinion is truly not more than my opinion. Google may see things different 😉 Now, in assertions you normally test assumtions/expectations on an object. Having this on mind the assertions should be more in this way:
The result is the same, but now it is in a more logical order: assertThat(propertySource)
.returns(Optional.ofNullable(location), SecretManagerPropertySource::getLocation); You could add more verifications if suitable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I followed the existing convention used throughout the codebase, where assertions are written in the same order as the current pattern. However, I understand your perspective, and if the team prefers the alternative style you suggested, I’m happy to make the adjustment. |
||
}).doesNotThrowAnyException(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,107 @@ | ||
package com.google.cloud.spring.autoconfigure.secretmanager; | ||
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
import static org.mockito.Mockito.mock; | ||
import static org.mockito.Mockito.when; | ||
|
||
import com.google.api.gax.rpc.NotFoundException; | ||
import com.google.cloud.secretmanager.v1.AccessSecretVersionResponse; | ||
import com.google.cloud.secretmanager.v1.SecretManagerServiceClient; | ||
import com.google.cloud.secretmanager.v1.SecretPayload; | ||
import com.google.cloud.secretmanager.v1.SecretVersionName; | ||
import com.google.protobuf.ByteString; | ||
import org.junit.jupiter.api.BeforeEach; | ||
import org.junit.jupiter.api.Test; | ||
import org.springframework.boot.BootstrapRegistry.InstanceSupplier; | ||
import org.springframework.boot.WebApplicationType; | ||
import org.springframework.boot.builder.SpringApplicationBuilder; | ||
import org.springframework.context.ConfigurableApplicationContext; | ||
import org.springframework.core.env.ConfigurableEnvironment; | ||
|
||
|
||
/** | ||
* Unit tests to check compatibility of Secret Manager for regional endpoints. | ||
*/ | ||
class SecretManagerRegionalCompatibilityTests { | ||
|
||
private static final String PROJECT_NAME = "regional-secret-manager-project"; | ||
private static final String LOCATION = "us-central1"; | ||
private SpringApplicationBuilder application; | ||
private SecretManagerServiceClient client; | ||
|
||
@BeforeEach | ||
void init() { | ||
application = new SpringApplicationBuilder(SecretManagerRegionalCompatibilityTests.class) | ||
.web(WebApplicationType.NONE) | ||
.properties( | ||
"spring.cloud.gcp.secretmanager.project-id=" + PROJECT_NAME, | ||
"spring.cloud.gcp.sql.enabled=false", | ||
"spring.cloud.gcp.secretmanager.location=" + LOCATION); | ||
|
||
client = mock(SecretManagerServiceClient.class); | ||
SecretVersionName secretVersionName = | ||
SecretVersionName.newProjectLocationSecretSecretVersionBuilder() | ||
.setProject(PROJECT_NAME) | ||
.setLocation(LOCATION) | ||
.setSecret("my-reg-secret") | ||
.setSecretVersion("latest") | ||
.build(); | ||
when(client.accessSecretVersion(secretVersionName)) | ||
.thenReturn( | ||
AccessSecretVersionResponse.newBuilder() | ||
.setPayload( | ||
SecretPayload.newBuilder().setData(ByteString.copyFromUtf8("newRegSecret"))) | ||
.build()); | ||
secretVersionName = | ||
SecretVersionName.newProjectLocationSecretSecretVersionBuilder() | ||
.setProject(PROJECT_NAME) | ||
.setLocation(LOCATION) | ||
.setSecret("fake-reg-secret") | ||
.setSecretVersion("latest") | ||
.build(); | ||
when(client.accessSecretVersion(secretVersionName)) | ||
.thenThrow(NotFoundException.class); | ||
} | ||
|
||
/** | ||
* Users with the new configuration (i.e., using `spring.config.import`) should get {@link | ||
* com.google.cloud.spring.secretmanager.SecretManagerTemplate} autoconfiguration and properties | ||
* resolved. | ||
*/ | ||
@Test | ||
void testRegionalConfigurationWhenDefaultSecretIsNotAllowed() { | ||
application.properties( | ||
"spring.config.import=sm://") | ||
.addBootstrapRegistryInitializer( | ||
(registry) -> registry.registerIfAbsent( | ||
SecretManagerServiceClient.class, | ||
InstanceSupplier.of(client) | ||
) | ||
); | ||
try (ConfigurableApplicationContext applicationContext = application.run()) { | ||
ConfigurableEnvironment environment = applicationContext.getEnvironment(); | ||
assertThat(environment.getProperty("sm://my-reg-secret")).isEqualTo("newRegSecret"); | ||
assertThatThrownBy(() -> environment.getProperty("sm://fake-reg-secret")) | ||
.isExactlyInstanceOf(NotFoundException.class); | ||
} | ||
} | ||
|
||
@Test | ||
void testRegionalConfigurationWhenDefaultSecretIsAllowed() { | ||
application.properties( | ||
"spring.cloud.gcp.secretmanager.allow-default-secret=true", | ||
"spring.config.import=sm://") | ||
.addBootstrapRegistryInitializer( | ||
(registry) -> registry.registerIfAbsent( | ||
SecretManagerServiceClient.class, | ||
InstanceSupplier.of(client) | ||
) | ||
); | ||
try (ConfigurableApplicationContext applicationContext = application.run()) { | ||
ConfigurableEnvironment environment = applicationContext.getEnvironment(); | ||
assertThat(environment.getProperty("sm://my-reg-secret")).isEqualTo("newRegSecret"); | ||
assertThat(environment.getProperty("sm://fake-reg-secret")).isNull(); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
= Spring Framework on Google Cloud Secret Manager Regional Sample Application | ||
|
||
This code sample demonstrates how to use the Spring Framework on Google Cloud Secret Manager integration. | ||
The sample demonstrates how one can access Secret Manager regional secrets through a `@ConfigurationProperties` class and also through `@Value` annotations on fields. | ||
|
||
== Running the Sample | ||
|
||
image:http://gstatic.com/cloudssh/images/open-btn.svg[link=https://ssh.cloud.google.com/cloudshell/editor?cloudshell_git_repo=https%3A%2F%2Fgithub.com%2FGoogleCloudPlatform%2Fspring-cloud-gcp&cloudshell_open_in_editor=spring-cloud-gcp-samples/spring-cloud-gcp-secretmanager-regional-sample/README.adoc] | ||
|
||
1. Create a Google Cloud project with https://cloud.google.com/billing/docs/how-to/modify-project#enable-billing[billing enabled], if you don't have one already. | ||
|
||
2. Enable the Secret Manager API from the "APIs & Services" menu of the Google Cloud Console. | ||
This can be done using the `gcloud` command line tool: | ||
+ | ||
[source] | ||
---- | ||
gcloud services enable secretmanager.googleapis.com | ||
---- | ||
|
||
3. Authenticate in one of two ways: | ||
|
||
a. Use the Google Cloud SDK to https://cloud.google.com/sdk/gcloud/reference/auth/application-default/login[authenticate with application default credentials]. | ||
This method is convenient but should only be used in local development. | ||
b. https://cloud.google.com/iam/docs/creating-managing-service-accounts[Create a new service account], download its private key and point the `spring.cloud.gcp.secretmanager.credentials.location` property to it. | ||
+ | ||
Such as: `spring.cloud.gcp.secretmanager.credentials.location=file:/path/to/creds.json` | ||
|
||
4. Using the https://console.cloud.google.com/security/secret-manager;regionalSecret[Secret Manager UI in Cloud Console], create a new regional secret named `application-secret` at us-central1 location and set it to any value. | ||
Instructions for using the Secret Manager UI can be found in the https://cloud.google.com/secret-manager/regional-secrets/create-regional-secret[Secret Manager documentation]. | ||
|
||
5. Make sure that the `spring.cloud.gcp.secretmanager.location` property points to the desired location for the regional secret. For this sample, we have kept the location as us-central1. | ||
+ | ||
Such as: `spring.cloud.gcp.secretmanager.location=us-central1` | ||
|
||
6. Run `$ mvn clean install` from the root directory of the project. | ||
|
||
7. Run `$ mvn spring-boot:run` command from the same directory as this sample's `pom.xml` file. | ||
|
||
8. Go to http://localhost:8080 in your browser or use the `Web Preview` button in Cloud Shell to preview the app | ||
on port 8080. Your secret value is injected into your application through the `WebController` and you will see it | ||
displayed. | ||
+ | ||
[source] | ||
---- | ||
applicationSecret: Hello regional world. | ||
---- | ||
+ | ||
You will also see some web forms that allow you to create, read, and update regional secrets in Secret Manager. | ||
This is done by using the `SecretManagerTemplate`. | ||
+ | ||
Finally, you can view all of your regional secrets using the https://console.cloud.google.com/security/secret-manager;regionalSecret[Secret Manager Cloud Console UI], which is the source of truth for all of your secrets in Secret Manager. | ||
|
||
9. Refresh the secrets without restarting the application: | ||
|
||
a. After running the application, change your secrets using https://console.cloud.google.com/security/secret-manager;regionalSecret[Secret Manager Cloud Console UI]. | ||
|
||
b. To refresh the secret, send the following command to your server from which hosting the application: | ||
+ | ||
[source] | ||
---- | ||
curl -X POST http://localhost:8080/actuator/refresh | ||
---- | ||
Note that only `@ConfigurationProperties` annotated with `@RefreshScope` got the updated value. |
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.
Is it syntactically correct if there is no closing
</p>
tag?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.
I've referred other files for the docstrings and there is no use of closing
</p>
tag. Following are some of such files:Let me know your thoughts on this.
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.
Oh, sorry. You are right. I wasn't aware that the closing tag is not mandatory.