Add initial secret manager property source implementation #2168

dzou · 2020-02-02T23:28:37Z

This is the initial scaffolding for setting up Secret Manager as a bootstrap property source.

In progress #2115.

codecov · 2020-02-02T23:39:48Z

Codecov Report

Merging #2168 into master will decrease coverage by 0.26%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master    #2168      +/-   ##
============================================
- Coverage     72.62%   72.35%   -0.27%     
- Complexity     1911     1954      +43     
============================================
  Files           245      251       +6     
  Lines          7038     7167     +129     
  Branches        725      735      +10     
============================================
+ Hits           5111     5186      +75     
- Misses         1589     1645      +56     
+ Partials        338      336       -2

Flag	Coverage Δ	Complexity Δ
#integration	`?`	`?`
#unittests	`72.35% <0%> (-0.27%)`	`1954 <0> (+43)`

Impacted Files	Coverage Δ	Complexity Δ
...retmanager/SecretManagerPropertySourceLocator.java	`0% <0%> (ø)`	`0 <0> (?)`
...ure/secretmanager/SecretManagerPropertySource.java	`0% <0%> (ø)`	`0 <0> (?)`
...anager/GcpSecretManagerBootstrapConfiguration.java	`0% <0%> (ø)`	`0 <0> (?)`
...gure/secretmanager/GcpSecretManagerProperties.java	`0% <0%> (ø)`	`0 <0> (?)`
...nner/core/mapping/SpannerCompositeKeyProperty.java	`80.88% <0%> (-1.21%)`	`37% <0%> (ø)`
...nner/core/convert/StructPropertyValueProvider.java	`87.87% <0%> (-0.7%)`	`12% <0%> (-1%)`
...oud/gcp/data/datastore/core/DatastoreTemplate.java	`87.25% <0%> (-0.63%)`	`120% <0%> (ø)`
...anner/core/admin/SpannerDatabaseAdminTemplate.java	`82.81% <0%> (-0.27%)`	`20% <0%> (ø)`
...data/spanner/repository/query/SqlSpannerQuery.java	`84.8% <0%> (-0.24%)`	`29% <0%> (ø)`
...t/ConverterAwareMappingSpannerEntityProcessor.java	`73.58% <0%> (ø)`	`22% <0%> (ø)`	⬇️
... and 16 more

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 26e9adf...45d9f2b. Read the comment docs.

...a/org/springframework/cloud/gcp/autoconfigure/secretmanager/SecretManagerPropertySource.java

sethvargo · 2020-02-03T18:21:07Z

...a/org/springframework/cloud/gcp/autoconfigure/secretmanager/SecretManagerPropertySource.java

+				.build();
+
+		AccessSecretVersionResponse response = client.accessSecretVersion(secretVersionName);
+		return response.getPayload().getData().toStringUtf8();


Should probably document it in SpringBoot that only "string" secrets are accepted. Secret Manager accepts a wide character set for payload values.

public Object getProperty(String name)
So, I don't think we really need to convert all values to Strings. @dzou WDYT?

Ah good point. I could just leave it as the SecretPayload which would be more flexible. Users would then be injecting/reading SecretPayload from the environment properties (instead of us forcing strings) and can convert the values to string or their preferred payload.

Do you feel this is a reasonable approach?

Well, I think there might be problems with mapping to @ConfigurationProperties classes that have Strings and other basic Java types. It's something that we should test.

You wouldn't want SecretPayload leaking into user code.

Spring does the conversion automatically. If there isn't a SecretPayload encode/decoder it would fail.

Ohh ok, this SecretPayload object can really just be converted to two options: String or byte[].

@spencergibb - How do I hook into this encode/decoder concept? (like which interfaces do I implement/register?) Is it possible for me to override getProperty(String propertyName, Class<T> targetType) such that if targetType is String I decode to string, and if the target type is byte[] I decode to byte[]?

String and byte[] properties are now supported through registering custom SecretPayload converters. See: https://github.com/spring-cloud/spring-cloud-gcp/pull/2168/files#diff-04e47cc423dee80f0eb313f527e9f5b2R74-R90

meltsufin

This is a great start!
The obvious things that are missing, but could be in different PRs: docs, sample app.

...gframework/cloud/gcp/autoconfigure/secretmanager/GcpSecretManagerBootstrapConfiguration.java

meltsufin · 2020-02-03T19:51:17Z

...gframework/cloud/gcp/autoconfigure/secretmanager/GcpSecretManagerBootstrapConfiguration.java

+
+@Configuration
+@EnableConfigurationProperties(GcpSecretManagerProperties.class)
+@ConditionalOnProperty("spring.cloud.gcp.secretmanager.enabled")


havingValue = "true"?

I think this should be disabled by default like our config management; otherwise all applications would be making RPCs to secret manager to list the secrets even if none exist.

Are there any required properties in GcpSecretManagerProperties?

There are no required properties.

Added matchIfMIssing= true to enable by default and then also added @ConditionalOnClass(SecretManagerServiceClient.class) to make conditional on the addition of secret manager class to classpath.

...a/org/springframework/cloud/gcp/autoconfigure/secretmanager/SecretManagerPropertySource.java

...pringframework/cloud/gcp/autoconfigure/secretmanager/SecretManagerPropertySourceLocator.java

meltsufin · 2020-02-03T20:23:31Z

...a/org/springframework/cloud/gcp/autoconfigure/secretmanager/SecretManagerPropertySource.java

+
+		super(propertySourceName, client);
+
+		Map<String, Object> propertiesMap = initializePropertiesMap(client, projectIdProvider.getProjectId());


You're loading properties in the constructor. Would it make sense to allow users to reload the properties by providing a public "refresh" method to do it?

More generally, test behavior of a@RefreshScope property in the sample. Ideally, triggering a refresh event should get those properties to expire.

Regarding the refresh functionality, can I verify this in a separate PR? This would be something to verify through the addition of the sample app and a Properties class.

meltsufin · 2020-02-03T20:24:41Z

...a/org/springframework/cloud/gcp/autoconfigure/secretmanager/SecretManagerPropertySource.java

+				.build();
+
+		AccessSecretVersionResponse response = client.accessSecretVersion(secretVersionName);
+		return response.getPayload().getData().toStringUtf8();


public Object getProperty(String name)
So, I don't think we really need to convert all values to Strings. @dzou WDYT?

.../springframework/cloud/gcp/autoconfigure/secretmanager/it/SecretManagerIntegrationTests.java

meltsufin · 2020-02-03T20:30:44Z

.../springframework/cloud/gcp/autoconfigure/secretmanager/it/SecretManagerIntegrationTests.java

+	/**
+	 * Creates the secret with the specified payload if the secret does not already exist.
+	 */
+	private void createSecret(String secretId, String payload) {


This reminds me, do we want to create a SecretManagerTemplate? You would also put the secret reading in there.

+1, and this would also go into the new module.

elefeint

Can you make sure this property source supports injecting and refreshing @RefreshScope annotated properties?
https://cloud.spring.io/spring-cloud-commons/multi/multi__spring_cloud_context_application_context_services.html#refresh-scope

elefeint · 2020-02-04T14:34:56Z

...pringframework/cloud/gcp/autoconfigure/secretmanager/SecretManagerPropertySourceLocator.java

+import org.springframework.core.env.Environment;
+import org.springframework.core.env.PropertySource;
+
+public class SecretManagerPropertySourceLocator implements PropertySourceLocator {


I wonder if it makes sense to put this class into a new spring-cloud-gcp-secretmanager module. Then we could give people a single-dependency starter instead of having them bring in the autoconfiguration module combined with explicitly setting a property.

Shouldn't it just be conditional on class of the secret manager api rather than setting a property?

Sounds good, we can discuss moving this in the next PR with the addition of secret template which will necessitate creating a the new secretmanager module. For now I am keeping the door open if we decide not to add a template class then this will mirror how our spring config support is organized (in which all the relevant classes are kept in autoconfigure/config).

Can we add a tracking issue to add a template?

Yes, this is tracked in #2176

elefeint · 2020-02-04T15:12:42Z

...a/org/springframework/cloud/gcp/autoconfigure/secretmanager/SecretManagerPropertySource.java

+
+		super(propertySourceName, client);
+
+		Map<String, Object> propertiesMap = initializePropertiesMap(client, projectIdProvider.getProjectId());


More generally, test behavior of a@RefreshScope property in the sample. Ideally, triggering a refresh event should get those properties to expire.

elefeint · 2020-02-04T15:16:19Z

.../springframework/cloud/gcp/autoconfigure/secretmanager/it/SecretManagerIntegrationTests.java

+	/**
+	 * Creates the secret with the specified payload if the secret does not already exist.
+	 */
+	private void createSecret(String secretId, String payload) {


+1, and this would also go into the new module.

sethvargo · 2020-02-05T16:40:03Z

...a/org/springframework/cloud/gcp/autoconfigure/secretmanager/SecretManagerPropertySource.java

+		HashMap<String, Object> secretsMap = new HashMap<>();
+		for (Secret secret : response.iterateAll()) {
+			String secretId = extractSecretId(secret);
+			ByteString secretPayload = getSecretPayload(client, projectId, secretId);


Non-blocking, but a potential future optimization would be to parallelize this.

Noted, added to #2176 to track.

sethvargo · 2020-02-05T16:40:33Z

...a/org/springframework/cloud/gcp/autoconfigure/secretmanager/SecretManagerPropertySource.java

+		SecretVersionName secretVersionName = SecretVersionName.newBuilder()
+				.setProject(projectId)
+				.setSecret(secretId)
+				.setSecretVersion(LATEST_VERSION_STRING)


I think people will want to access specific secret versions. Defaulting to "latest" is not a production best practice.

Ah good point, we'll definitely need an answer for this. I think we'll have to discuss a bit more in-depth with our team what is the best way to let users specify which secret versions they want to load into the environment; I guess we wouldn't want to load all versions of all secrets by default.

I added this point to #2176 to track; we will definitely implement this before releasing to users. I just want this PR to setup the initial scaffolding.

...gframework/cloud/gcp/autoconfigure/secretmanager/GcpSecretManagerBootstrapConfiguration.java

...a/org/springframework/cloud/gcp/autoconfigure/secretmanager/SecretManagerPropertySource.java

meltsufin · 2020-02-05T22:02:01Z

...pringframework/cloud/gcp/autoconfigure/secretmanager/SecretManagerPropertySourceLocator.java

+import org.springframework.core.env.Environment;
+import org.springframework.core.env.PropertySource;
+
+public class SecretManagerPropertySourceLocator implements PropertySourceLocator {


Can we add a tracking issue to add a template?

.../springframework/cloud/gcp/autoconfigure/secretmanager/it/SecretManagerIntegrationTests.java

sonarcloud · 2020-02-05T22:49:49Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
3 Code Smells

0.0% Coverage
0.0% Duplication

dzou · 2020-02-06T22:07:57Z

@elefeint @meltsufin - PTAL when you guys get a chance :)

Add Secret Manager as bootstrap property source

9c6bbd6

dzou requested review from meltsufin and elefeint February 2, 2020 23:28

fix indents

d713b8c

dzou mentioned this pull request Feb 3, 2020

Add support for Google Cloud Secret Manager #2115

Closed

sethvargo suggested changes Feb 3, 2020

View reviewed changes

meltsufin suggested changes Feb 3, 2020

View reviewed changes

elefeint suggested changes Feb 4, 2020

View reviewed changes

dzou added 2 commits February 5, 2020 11:14

PR Comments

afe5e3f

cleanup

eedb338

dzou requested review from meltsufin and elefeint February 5, 2020 16:30

sethvargo suggested changes Feb 5, 2020

View reviewed changes

spencergibb reviewed Feb 5, 2020

View reviewed changes

...gframework/cloud/gcp/autoconfigure/secretmanager/GcpSecretManagerBootstrapConfiguration.java Outdated Show resolved Hide resolved

dzou mentioned this pull request Feb 5, 2020

Secret Manager Support Tasks Tracking #2176

Closed

8 tasks

do alternate method for converted registration

9781956

spencergibb reviewed Feb 5, 2020

View reviewed changes

...gframework/cloud/gcp/autoconfigure/secretmanager/GcpSecretManagerBootstrapConfiguration.java Outdated Show resolved Hide resolved

dzou added 3 commits February 5, 2020 14:48

move converter registration to constructor

cfee77c

change prefix string to empty as default

0afdf65

Make secret prefix empty by default.

114eb1a

meltsufin suggested changes Feb 5, 2020

View reviewed changes

pr comments

45d9f2b

dzou requested a review from meltsufin February 6, 2020 16:25

meltsufin approved these changes Feb 6, 2020

View reviewed changes

elefeint approved these changes Feb 6, 2020

View reviewed changes

dzou merged commit 9c6cf54 into master Feb 7, 2020

dzou deleted the secret-manager branch February 7, 2020 15:52

dzou mentioned this pull request Feb 25, 2020

Add basic template for Secret Manager #2195

Merged


		super(propertySourceName, client);

		Map<String, Object> propertiesMap = initializePropertiesMap(client, projectIdProvider.getProjectId());

Add initial secret manager property source implementation #2168

Add initial secret manager property source implementation #2168

Conversation

dzou commented Feb 2, 2020

codecov bot commented Feb 2, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzou Feb 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meltsufin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elefeint left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzou Feb 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Feb 5, 2020

dzou commented Feb 6, 2020

codecov bot commented Feb 2, 2020 •

edited

Loading

dzou Feb 3, 2020 •

edited

Loading

dzou Feb 5, 2020 •

edited

Loading