Strip secret values from Configure #2437
Conversation
iwahbe
force-pushed
the
iwahbe/2405/strip-configure-secrets
branch
from
276ac8e to
90e406e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2437 +/- ##
==========================================
+ Coverage 57.40% 57.44% +0.03%
==========================================
Files 369 369
Lines 50250 50299 +49
==========================================
+ Hits 28848 28894 +46
- Misses 19822 19825 +3
Partials 1580 1580 ☔ View full report in Codecov by Sentry. |
iwahbe
force-pushed
the
iwahbe/2405/strip-configure-secrets
branch
from
b18ba93 to
2b3f3c7
Compare
iwahbe
force-pushed
the
iwahbe/2405/strip-configure-secrets
branch
2 times, most recently
from
1b2a5ce to
8989436
Compare
There was a problem hiding this comment.
Since pulumi/pulumi-gcp#2405 doesn't really have any context on why this happened, can we add some background in the PR description?
iwahbe
force-pushed
the
iwahbe/2405/strip-configure-secrets
branch
from
8989436 to
da54c20
Compare
iwahbe
force-pushed
the
iwahbe/2405/strip-configure-secrets
branch
from
da54c20 to
4acb843
Compare
|
I'm not comfortable here. Is it the case that Configure started receiving secrets in |
| @@ -68,19 +68,33 @@ func (*ConfigEncoding) tryUnwrapSecret(encoded any) (any, bool) { | |||
| } | |||
|
|
|||
| func (enc *ConfigEncoding) convertStringToPropertyValue(s string, typ shim.ValueType) (resource.PropertyValue, error) { | |||
| // If the schema expects a string, we can just return this as-is. | |||
There was a problem hiding this comment.
This seems rather risky. The former behavior of treating strings plainly was there for a reason, and in my last archaeology round I couldn't fully recover it so I left it as-is. Stringly typed values were not subject to ConfigEncoding, I think. This code was not designed to handle secrets, but the change does more than introduce secret handling.
Now for something of type string that "looks like" JSON without any secrets, the new code will try to parse it and in case of success introduce changes that might be unwanted?
There was a problem hiding this comment.
Stringly typed values were not subject to ConfigEncoding, I think.
They weren't, but this means that string values cannot be secret if they are encoded like other values. This seems pretty suspect to me.
This code was not designed to handle secrets, but the change does more than introduce secret handling.
This code does need to handle secrets, with or without #2410.
Now for something of type string that "looks like" JSON without any secrets, the new code will try to parse it and in case of success introduce changes that might be unwanted?
Only if the thing that "looks like" JSON looks like a JSON encoded secret or computed string. If we trust that pulumi signifier keys don't appear in the wild1, then this is safe.
Footnotes
-
We already make this assumption on the wire. ↩
There was a problem hiding this comment.
It is suspect in indeed but this is the production behavior. Is your change needed to fix the listed issue? How so? Is this a change that's a refactor for reasons separate from pulumi/pulumi-gcp#2405 ?
| return p.GrpcProvider.Configure(ctx, req) | ||
| } | ||
|
|
||
| func removeSecrets(pMap resource.PropertyMap) resource.PropertyMap { |
There was a problem hiding this comment.
| @@ -80,5 +81,41 @@ func (p providerThunk) Configure( | |||
| if ctx.Value(setupConfigureKey) != nil { | |||
| return plugin.ConfigureResponse{}, nil | |||
| } | |||
| req.Inputs = removeSecrets(req.Inputs) | |||
There was a problem hiding this comment.
I'm confused here, is the panic we're trying to fix scoped to PF, or SDKv2, or both?
There was a problem hiding this comment.
Just PF (and muxed providers that use PF).
|
This PR relies on unreverting #2410. |
Fixes pulumi/pulumi-gcp#2405