Polish Secret Manager Code #2270
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2270 +/- ##
============================================
- Coverage 79.73% 72.63% -7.11%
+ Complexity 2276 2055 -221
============================================
Files 258 258
Lines 7460 7450 -10
Branches 759 762 +3
============================================
- Hits 5948 5411 -537
- Misses 1193 1689 +496
- Partials 319 350 +31
Continue to review full report at Codecov.
|
|
@eddumelendez FYI |
|
thanks for let me know @meltsufin |
...a/org/springframework/cloud/gcp/autoconfigure/secretmanager/SecretManagerPropertySource.java
Outdated
Show resolved
Hide resolved
| } | ||
| catch (Exception e) { | ||
| if (e instanceof NotFoundException) { | ||
| LOGGER.debug("Skipped loading secret " + secretId + " because it does not have version " + defaultVersion); |
There was a problem hiding this comment.
Notice that this will also happen when a specific secret.version is specified.
I fear that this make it hard to debug mistakes in the client code.
Consider adding another property to chose whether to skip silently or fail.
Something like failOnMissingSecretVersion.
There was a problem hiding this comment.
For this one, this is what I think makes the most sense:
- If a user-specified version doesn't exist, throw an error.
- If a default version doesn't exist for a secret, ignore the secret.
I think this makes the most sense for the default version handling because most users will have multiple secrets in their project, and they might all be at different versions (like some you'll be at version 5 and maybe some at version 1 or 2).
In practice, for any value of default-version other than "latest", there is a low chance that every secret in the project is up to that version. For example, if you choose to set default-version to 2, this bootstrap loading would fail if any secret in the project was only at version 1. So I think the default-version option is only useful if you skip the secrets that aren't up to the default version.
There was a problem hiding this comment.
Your reasoning makes sense, but I still feel that it's just too complicated and will leave room for some surprises for people who do not carefully read the documentation. You can argue that the default version setting should never be used, unless the project keeps all of the secrets at the same version. Maybe they have a convention where each version of the application has a corresponding version of the secrets. In that case, as a developer I would like a hard fail if I make a mistake and forget to include a secret with the right version that follows that convention.
Of course there could be exceptions. Therefore, I'm suggesting to add a property to allow skipping secrets with non-existing versions.
I guess we're struggling here partly because we're not sure what is the common use-case for default-version.
Maybe @eddumelendez can help us out here.
There was a problem hiding this comment.
I see. I just was not a fan of the additional setting because I imagine most projects will have secrets of different versions.
I don't see it being likely that each version of an application will have its own version of secrets - like a password to your MySQL database might stay the same across all of your application versions but some other secret might be updated more frequently. And what if you have to add a new secret to an application? If you're at version 5 for your current secrets, the new secret must be updated 5 times to hit version 5. (The only way to increment a secret version is to create a new version of it in the APIs; you can't create a new secret and set it to 5 directly)
Hmm, the more I think about it, the less useful overriding the default-version is. It feels like defaulting to latest makes the most sense and overriding secret versions should be done on an individual level per secret. What do you guys think? @meltsufin @eddumelendez
There was a problem hiding this comment.
I didn't realize that the version cannot be set directly.
I happy to remove the default version feature.
@saturnism @eddumelendez WDYT?
There was a problem hiding this comment.
It feels like defaulting to latest makes the most sense and overriding secret versions should be done on an individual level per secret.
I'm in favor of this.
There is just two options for every secret which is use the latest or a specific version. Given that, default-version doesn't make much sense.
Also, regarding to:
If a user-specified version doesn't exist, throw an error.
If a default version doesn't exist for a secret, ignore the secret
So, if the secret is ignored then @Value will not resolve it, right? I think the docs are pretty clear about the context but not sure if will be hard to figure it out is we are missing to read the docs.
There was a problem hiding this comment.
Sounds like we're all leaning towards just removal of default-version and hard fail in case a specified version doesn't exist.
There was a problem hiding this comment.
Thank you both for the input, this is much appreiciated. Yes in that case, let's go in that direction.
.../springframework/cloud/gcp/autoconfigure/secretmanager/it/SecretManagerIntegrationTests.java
Outdated
Show resolved
Hide resolved
# Conflicts: # docs/src/main/asciidoc/secretmanager.adoc
|
@meltsufin - done, just removed the |
|
SonarCloud Quality Gate failed.
|
Removes the version property for secret manager
Removes the version property for secret manager
Followup to secret manager version feature changes.
Contents of PR: