This repository has been archived by the owner on Jan 19, 2022. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Create protocol for specifying secrets' project and versions #2302
Create protocol for specifying secrets' project and versions #2302
Changes from 4 commits
222575d
69ea656
d9c7592
3d87209
f8d5fd2
231b208
4ae2c45
1f20609
f026809
5b01c33
f00e1c3
32a8165
d54c7d6
8b145f1
2ce82cb
41c83f0
1116fb8
c48a3a5
968e44a
a81143b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Did the Secrets Manager team comment on this prefix? Have you looked into using
sm://
?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.
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.
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 wonder if we can tap into that "special meaning" and configure it for our purpose.
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.
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 evaluatesgs
which it finds to be null, then returns the string//blah/blah
.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.
This is only the case if SPEL is used, such as with
@Value
, right?Notice that with
@Value
on aResource
it's interpreted as a protocol instead.For example, see the "gs://" implementation we have for Storage.
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.
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|
orgcp-sm|
these seem to all work. I recall Seth suggested using|
as an alternative.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.
Should we ever return
null
though? Is there something wrong with throwing an exception for a property that doesn't exist?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.
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 returnnull
someday.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.
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.
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.
Based on discussion offline, looks like
sm://
has precedent of being used, so I just changed it to that for now.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.
Wouldn't this be considered an error if we got here? Not sure we should swallow the exception.
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.
Sure, let's throw exception, this might be good for supporting the
gcp-sm://
prefix per our discussion above.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.
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.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.
Done; I added these methods for getSecret, but for
createSecret
andsecretExists
it doesn't quite fit since those methods can accept project and secretId as valid inputs but are not fixed to a version.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.
Missing a test for
getSecretPayload
.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.
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.