Create protocol for specifying secrets' project and versions #2302

dzou · 2020-04-03T23:59:58Z

This creates a protocol for specifying secrets' projects and versions.

The protocol format uses the same conventions as https://github.com/GoogleCloudPlatform/github-actions/tree/master/get-secretmanager-secrets#inputs

# 1. Long form
spring.property=${secret-prefix:projects/<project-id>/secrets/<secret-id>/versions/<version-id>}

# 2.  Long form - "latest" version
spring.property=${secret-prefix:projects/<project-id>/secrets/<secret-id>}

# 3. Short form
spring.property=${secret-prefix:<project-id>/<secret-id>/<version-id>}

# 4. Short form - default project; specify secret + version
spring.property=${secret-prefix:<secret-id>/<version>} 

# 5. Shortest form - "latest" version and default project.
spring.property=${secret-prefix:<secret-id>}

Some further questions:

How to introduce this feature? And how to deprecate old way of specifying secrets? Can they co-exist for some time?
Which of the two approaches for specifying the default project do you like here: Allow to select the source projectId when using secret #2283 (comment) The OP makes a good point about how the #4 form above might be better replaced by <secret-id>/<version> with it defaulting to a project instead. However this approach comes with the drawback that it diverges from the github actions convention.
Perhaps people need <secret-id>/<version-id> more based on discussion offline.

codecov · 2020-04-04T00:09:40Z

Codecov Report

Merging #2302 into master will decrease coverage by 7.69%.
The diff coverage is 69.73%.

@@             Coverage Diff              @@
##             master    #2302      +/-   ##
============================================
- Coverage     80.95%   73.25%   -7.70%     
+ Complexity     2309     2072     -237     
============================================
  Files           258      258              
  Lines          7456     7471      +15     
  Branches        762      775      +13     
============================================
- Hits           6036     5473     -563     
- Misses         1100     1641     +541     
- Partials        320      357      +37

Flag	Coverage Δ	Complexity Δ
#integration	`?`	`?`
#unittests	`73.25% <69.73%> (+0.57%)`	`2072.00 <15.00> (+13.00)`

Impacted Files	Coverage Δ	Complexity Δ
...anager/GcpSecretManagerBootstrapConfiguration.java	`0.00% <0.00%> (-90.91%)`	`0.00 <0.00> (-3.00)`
...gure/secretmanager/GcpSecretManagerProperties.java	`0.00% <ø> (-73.34%)`	`0.00 <0.00> (-6.00)`
...gcp/secretmanager/SecretManagerPropertySource.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...retmanager/SecretManagerPropertySourceLocator.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
.../gcp/secretmanager/SecretManagerPropertyUtils.java	`77.50% <77.50%> (ø)`	`7.00 <7.00> (?)`
...cloud/gcp/secretmanager/SecretManagerTemplate.java	`98.14% <95.65%> (+0.27%)`	`14.00 <8.00> (ø)`
...a/spanner/repository/query/SpannerQueryMethod.java	`0.00% <0.00%> (-100.00%)`	`0.00% <0.00%> (-6.00%)`
...figure/config/GcpConfigBootstrapConfiguration.java	`0.00% <0.00%> (-100.00%)`	`0.00% <0.00%> (-2.00%)`
...restore/repository/query/FirestoreQueryMethod.java	`0.00% <0.00%> (-100.00%)`	`0.00% <0.00%> (-2.00%)`
...epository/config/SpannerRepositoriesRegistrar.java	`0.00% <0.00%> (-100.00%)`	`0.00% <0.00%> (-3.00%)`
... and 61 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 820a5dc...a81143b. Read the comment docs.

elefeint

Looks great, just a couple of typos and a question (which you also had in PR description) about what to do with the old one.

...g/springframework/cloud/gcp/autoconfigure/secretmanager/SecretManagerPropertyIdentifier.java

elefeint · 2020-04-06T15:04:20Z

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

-				this.versions);
+		CompositePropertySource compositePropertySource = new CompositePropertySource(SECRET_MANAGER_NAME);
+
+		compositePropertySource.addPropertySource(


Do we want to have both resolvable? The new version is more comprehensive. Or are we doing it for backwards compatibility?

Perhaps we could automatically configure only the newest property source, and provide a "legacy" property for loading the old one?

Yeah I wasn't sure how to handle this. I think would prefer if we only have the new version. Maybe if it's ok, I'd like to just remove the existing functionality and replace it with the new method. @meltsufin

I guess it doesn't make sense for both to co-exist since the legacy one already does the work of downloading all the secrets, and then if the new system exists they might redownload secrets they already loaded into their properties.

dzou · 2020-04-06T18:10:43Z

...ud-gcp-samples/spring-cloud-gcp-secretmanager-sample/src/main/resources/bootstrap.properties

-my-application-secret=${secrets.application-secret}
+# This demonstrates multiple ways of loading the same application secret using template syntax.
+my-application-secret-1=${secrets.application-secret}
+my-application-secret-2=${gcp-secret/projects/my-kubernetes-codelab-217414/secrets/application-secret}


Consider different prefix usage

dzou · 2020-04-06T18:11:31Z

...ud-gcp-samples/spring-cloud-gcp-secretmanager-sample/src/main/resources/bootstrap.properties

+# This demonstrates multiple ways of loading the same application secret using template syntax.
+my-application-secret-1=${secrets.application-secret}
+my-application-secret-2=${gcp-secret/projects/my-kubernetes-codelab-217414/secrets/application-secret}
+my-application-secret-3=${gcp-secret/projects/my-kubernetes-codelab-217414/secrets/application-secret/version/latest}


Can also be a project number for projects/ value

...g/springframework/cloud/gcp/autoconfigure/secretmanager/SecretManagerPropertyIdentifier.java

dzou · 2020-04-06T22:53:43Z

Redid the PR to just remove the legacy way of doing things and replace with new. If you feel good about this, I can move forward with doc updates.

meltsufin · 2020-04-07T13:33:46Z

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

-	private static final String LATEST_VERSION_STRING = "latest";
-
-	private final Map<String, Object> properties;
+	private static final String GCP_SECRET_PREFIX = "gcp-secret/";


Did the Secrets Manager team comment on this prefix? Have you looked into using sm://?

Colon can't be used in a property name because it means something special in SPEL. I wouldn't want to try to escape the colon either; I think it would look worse with the back slashes.

Am open to other secret prefixes though.

I wonder if we can tap into that "special meaning" and configure it for our purpose.

Colon is like the null-safety operator. If the left side of the colon is evaluated to null, then it defaults to the value on the right side.

So if i do gs://blah/blah - it firsts evaluates gs which it finds to be null, then returns the string //blah/blah.

This is only the case if SPEL is used, such as with @Value, right?
Notice that with @Value on a Resource it's interpreted as a protocol instead.
For example, see the "gs://" implementation we have for Storage.

Ran the code through the debugger, the colon is a special character; i don't think this is configurable. I think we should avoid it to avoid unexpected behavior in the future with null values.

I would be open to other choices of prefixes, maybe sm| or gcp-sm| these seem to all work. I recall Seth suggested using | as an alternative.

Should we ever return null though? Is there something wrong with throwing an exception for a property that doesn't exist?

Yeah I can see the argument where if a user specifies a secret using our syntax, there is the expectation that a secret is there; if it's missing then that's most likely an error and they probably don't want null there.

I would be comfortable with either approach. We can still throw exception using | too by the way.

To me it seems the tradeoff here is purely cosmetic. I.e. if the cosmetic benefits of using : outweighs the risk that we need to change behavior to return null someday.

What's the prefix that the Secret Manager team suggested we use? There's value in being compatible with other libraries for this product. Let's confirm with the product team.

Based on discussion offline, looks like sm:// has precedent of being used, so I just changed it to that for now.

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

...va/org/springframework/cloud/gcp/autoconfigure/secretmanager/GcpSecretManagerProperties.java

meltsufin · 2020-04-07T13:38:18Z

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

+			return response.getPayload().getData();
+		}
+		catch (NotFoundException e) {
+			return null;


Wouldn't this be considered an error if we got here? Not sure we should swallow the exception.

Sure, let's throw exception, this might be good for supporting the gcp-sm:// prefix per our discussion above.

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

...ava/org/springframework/cloud/gcp/autoconfigure/secretmanager/SecretPropertySourceTests.java

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

meltsufin · 2020-04-07T13:49:23Z

...ava/org/springframework/cloud/gcp/autoconfigure/secretmanager/SecretPropertySourceTests.java

+		assertThat(secretIdentifier.getProject()).isEqualTo("my-project");
+		assertThat(secretIdentifier.getSecret()).isEqualTo("the-secret");
+		assertThat(secretIdentifier.getSecretVersion()).isEqualTo("3");
+	}


Missing a test for getSecretPayload.

We'll cover this directly in your configuration tests.

I don't think it's worth making it non-private to write tests for it unlike the parseProperty method which had a lot more code.

meltsufin · 2020-04-07T19:11:30Z

@dzou Please make sure the sample app integration tests work as well.
Also, I think at this point we should make the bootstrap auto-config enabled by default.

dzou · 2020-04-07T23:09:56Z

@dzou Please make sure the sample app integration tests work as well.
Also, I think at this point we should make the bootstrap auto-config enabled by default.

Done.
Done - I actually just deleted the normal autoconfiguration class and we'll use the bootstrap by default?

meltsufin · 2020-04-08T13:47:03Z

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

@@ -48,7 +49,7 @@
 @Configuration
 @EnableConfigurationProperties(GcpSecretManagerProperties.class)
 @ConditionalOnClass(SecretManagerServiceClient.class)
-@ConditionalOnProperty("spring.cloud.gcp.secretmanager.bootstrap.enabled")


Docs need to be updated following this change.

Done, updated docs.

meltsufin · 2020-04-08T13:49:42Z

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

-	private static final String LATEST_VERSION_STRING = "latest";
-
-	private final Map<String, Object> properties;
+	private static final String GCP_SECRET_PREFIX = "gcp-secret/";


I'm not following fully. Let's discuss.

meltsufin

The main feedback I have is that the SecretManagerTemlate needs to be updated as well to accept fully qualified secret names.

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

meltsufin · 2020-04-08T18:11:57Z

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

-				secretsMap.put(secretsPrefix + secretId, secretPayload);
-			}
+	// Visible for Testing
+	static SecretVersionName parseFromProperty(String property, GcpProjectIdProvider projectIdProvider) {


Should this move to SecretManagerTemplate? We probably want to reconsider all those additional methods for specifying project and version and use the fully qualified secret identifiers instead.

Done; I added these methods for getSecret, but for createSecret and secretExists it doesn't quite fit since those methods can accept project and secretId as valid inputs but are not fixed to a version.

docs/src/main/asciidoc/secretmanager.adoc

meltsufin

I think we can simplify further by removing the *Uri methods and re-using the template in the property source.

meltsufin · 2020-04-09T19:57:14Z

...nager/src/main/java/org/springframework/cloud/gcp/secretmanager/SecretManagerOperations.java

 	 * @return The secret payload as String
 	 */
-	String getSecretString(String secretId, String versionName);
+	String getSecretStringByUri(String secretUri);


Do we really need a separate method for getting by URI?
We can easily detect that something is a URI and do it all in the existing getSecretString(String).

meltsufin · 2020-04-09T19:57:24Z

...nager/src/main/java/org/springframework/cloud/gcp/secretmanager/SecretManagerOperations.java

-	 * @return The secret payload as {@link ByteString}
-	 */
-	ByteString getSecretByteString(String secretId, String versionName, String projectId);
+	byte[] getSecretBytesByUri(String secretUri);


...r/src/main/java/org/springframework/cloud/gcp/secretmanager/SecretManagerPropertySource.java

sonarcloud · 2020-04-10T18:59:01Z

SonarCloud Quality Gate failed.

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

70.6% Coverage
0.0% Duplication

meltsufin

Looks good at this point.
One minor comment which you can ignore.

meltsufin · 2020-04-10T19:01:05Z

...er/src/main/java/org/springframework/cloud/gcp/secretmanager/SecretManagerPropertyUtils.java

+					"Unrecognized format for specifying a GCP Secret Manager secret: " + input);
+		}
+
+		Assert.isTrue(


Assert.hasText instead maybe?

Gotcha, i'll change it.

Let me submit this and i'll send a followup cleanup PR; some other stuff I want to cleanup too.

gsdatta · 2021-01-19T19:28:25Z

Apologies in advance for commenting on such an old PR! I'm currently in the process of setting up a spring boot project on GCP app engine, which requires us us to use secrets manager as a replacement for env vars.

One of my worries here is that the forced protocol prefix adds to vendor lock-in that we'd like to avoid. I was hoping I could add a custom prefix (e.g. MY_ENV_VAR_) to make it look like an environment variable (I initially misunderstood the secret-name-prefix property as a way to replace the sm://).

I'm curious to hear why the protocol prefix is necessary. Is it just to denote to the starter that it should be looked up via secrets manager? Can this protocol prefix be configurable?

dzou · 2021-01-19T19:55:43Z

Apologies in advance for commenting on such an old PR! I'm currently in the process of setting up a spring boot project on GCP app engine, which requires us us to use secrets manager as a replacement for env vars.

One of my worries here is that the forced protocol prefix adds to vendor lock-in that we'd like to avoid. I was hoping I could add a custom prefix (e.g. MY_ENV_VAR_) to make it look like an environment variable (I initially misunderstood the secret-name-prefix property as a way to replace the sm://).

I'm curious to hear why the protocol prefix is necessary. Is it just to denote to the starter that it should be looked up via secrets manager? Can this protocol prefix be configurable?

@gsdatta -- The sm:// prefix is necessary to indicate which property values should be looked up in Secret Manager. Basically all the property values with this prefix -- we assume that the string after the prefix corresponds to the secret's ID. We don't want to look up every value in an application.properties file in secret manager.

Hmm we could make the prefix configurable, but I have an alternative approach which may be simpler than a custom prefix like MY_ENV_VAR_.

What about a default value feature? Something like:

spring.properties.password=${sm://my-secret:PASSWORD_ENV} or spring.properties.password=${PASSWORD_ENV:sm://my-secret}

So we could check if the secret is present/null; if so, default to the provided value PASSWORD_ENV. Or the other way around in the second example.

This way you can keep the behavior with relying on env vars to be vendor-generic. Does that work for you?

gsdatta · 2021-01-19T20:23:30Z

Makes sense. I think the default value structure would work for us quite well! Gives us the flexibility of overriding in certain environments as required when secrets manager may not be necessary.

dzou · 2021-01-19T20:41:46Z

@gsdatta -- Thanks for the input. We're starting version 2.0.0 of our repo under the GoogleCloudPlatform organization; see: https://github.com/GoogleCloudPlatform/spring-cloud-gcp

We're going to start making all of our updates there.

I filed a new feature request for this in our new repo: GoogleCloudPlatform/spring-cloud-gcp#213

dzou · 2021-01-26T21:05:18Z

@gsdatta -- Hey I did some research, I think we have a decent way of doing this already in the current implementation.

You can try something like below; in the example FOOBAR is the environmental variable:

spring.password=${FOOBAR:${sm://application-secret/latest}}

In this case, if foobar is not null, it will use the value of FOOBAR. Otherwise, it will retrieve the secret manager secret. Let me know if this solution solves your problem.

This uses the : syntax in SPEL which will evaluate the expression on the left first, and then if it's null it will attempt to evaluate the expression on the right.

Create protocol for specifying secrets

222575d

dzou requested review from dmitry-s, meltsufin and elefeint April 3, 2020 23:59

elefeint suggested changes Apr 6, 2020

View reviewed changes

dzou added 2 commits April 6, 2020 12:33

Fix assert notNull statements

69ea656

reduce visibility of SecretManagerPropertyIdentifier

d9c7592

dzou added the secret-manager label Apr 6, 2020

dzou commented Apr 6, 2020

View reviewed changes

meltsufin suggested changes Apr 6, 2020

View reviewed changes

...g/springframework/cloud/gcp/autoconfigure/secretmanager/SecretManagerPropertyIdentifier.java Outdated Show resolved Hide resolved

PR Comments

3d87209

dzou requested review from meltsufin and elefeint April 6, 2020 22:53

elefeint previously approved these changes Apr 7, 2020

View reviewed changes

meltsufin suggested changes Apr 7, 2020

View reviewed changes

dzou added 5 commits April 7, 2020 16:53

PR Comments

f8d5fd2

update sample

231b208

Fix checkstyle

4ae2c45

Fix sample

1f20609

make private visibility for sample fields

f026809

dzou dismissed elefeint’s stale review via f026809 April 7, 2020 21:23

dzou added 2 commits April 7, 2020 18:39

fix tests

5b01c33

add visible for testing comment

f00e1c3

dzou requested review from meltsufin and elefeint April 7, 2020 23:10

meltsufin reviewed Apr 8, 2020

View reviewed changes

dzou added 3 commits April 8, 2020 12:43

documentation updates

32a8165

Update secret manager enabled property name.

d54c7d6

remove enabled property from tests

8b145f1

meltsufin suggested changes Apr 8, 2020

View reviewed changes

dzou added 5 commits April 8, 2020 15:00

PR Comments

2ce82cb

template changes + refactor

41c83f0

secret manager prefix change to sm://

1116fb8

add comments

c48a3a5

fix checkstyle

968e44a

dzou requested a review from meltsufin April 8, 2020 21:21

dzou mentioned this pull request Apr 8, 2020

Secret manager autoconfigure projectId selection #2289

Closed

meltsufin suggested changes Apr 9, 2020

View reviewed changes

PR Comments

a81143b

dzou requested a review from meltsufin April 10, 2020 18:51

meltsufin approved these changes Apr 10, 2020

View reviewed changes

dzou merged commit e5c1b08 into master Apr 10, 2020

This was referenced Apr 10, 2020

Consider using underscores to be the namespace-separating character in Secret Manager #2285

Closed

Allow to select the source projectId when using secret #2283

Closed

dzou mentioned this pull request Apr 21, 2020

Add support for Google Cloud Secret Manager #2115

Closed

sethvargo mentioned this pull request May 7, 2020

[AIRFLOW-7104] Add Secret backend for GCP Secrets Manager apache/airflow#7795

Merged

5 tasks

eddumelendez mentioned this pull request May 25, 2020

Add secret manager enabled property to properties metadata #2363

Merged

dzou mentioned this pull request Jan 27, 2021

Add a note in the sample for how to use default values GoogleCloudPlatform/spring-cloud-gcp#229

Merged

ttomsu deleted the secret-manager-protocol branch February 3, 2021 13:19

Create protocol for specifying secrets' project and versions #2302

Create protocol for specifying secrets' project and versions #2302

Conversation

dzou commented Apr 3, 2020 • edited Loading

codecov bot commented Apr 4, 2020 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

dzou commented Apr 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzou Apr 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzou Apr 8, 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

meltsufin commented Apr 7, 2020

dzou commented Apr 7, 2020

Choose a reason for hiding this comment

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

meltsufin left a comment • 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

sonarcloud bot commented Apr 10, 2020

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

gsdatta commented Jan 19, 2021

dzou commented Jan 19, 2021

gsdatta commented Jan 19, 2021

dzou commented Jan 19, 2021

dzou commented Jan 26, 2021 • edited Loading

dzou commented Apr 3, 2020 •

edited

Loading

codecov bot commented Apr 4, 2020 •

edited

Loading

dzou Apr 7, 2020 •

edited

Loading

dzou Apr 8, 2020 •

edited

Loading

meltsufin left a comment •

edited

Loading

dzou commented Jan 26, 2021 •

edited

Loading