Strip secret values from Configure #2437
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| // Copyright 2016-2024, Pulumi Corporation. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package plugin | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/pulumi/pulumi/sdk/v3/go/common/resource" | ||
| prapid "github.com/pulumi/pulumi/sdk/v3/go/property/testing" | ||
| "github.com/stretchr/testify/assert" | ||
| "pgregory.net/rapid" | ||
| ) | ||
|
|
||
| // TestRemoveSecrets checks that removeSecrets removes [resource.Secret] values and unsets | ||
| // [resource.Output.Secret] fields without making any other changes. | ||
| func TestRemoveSecrets(t *testing.T) { | ||
|
|
||
| // These functions validate that a diff does not contain any non-secret changes. | ||
| var ( | ||
| validateObjectDiff func(assert.TestingT, resource.ObjectDiff) | ||
| validateArrayDiff func(assert.TestingT, resource.ArrayDiff) | ||
| validateValueDiff func(assert.TestingT, resource.ValueDiff) | ||
| ) | ||
|
|
||
| validateValueDiff = func(t assert.TestingT, v resource.ValueDiff) { | ||
| switch { | ||
| case v.Old.IsOutput(): | ||
| oOld := v.Old.OutputValue() | ||
| oOld.Secret = false | ||
| if d := resource.NewProperty(oOld).DiffIncludeUnknowns(v.New); d != nil { | ||
| validateValueDiff(t, *d) | ||
| } | ||
| case v.Old.IsSecret(): | ||
| if d := v.Old.SecretValue().Element.DiffIncludeUnknowns(v.New); d != nil { | ||
| validateValueDiff(t, *d) | ||
| } | ||
| case v.Old.IsObject(): | ||
| validateObjectDiff(t, *v.Object) | ||
| case v.Old.IsArray(): | ||
| validateArrayDiff(t, *v.Array) | ||
| default: | ||
| assert.Failf(t, "", "unexpected Update.Old type %q", v.Old.TypeString()) | ||
| } | ||
| } | ||
|
|
||
| validateArrayDiff = func(t assert.TestingT, diff resource.ArrayDiff) { | ||
| assert.Empty(t, diff.Adds) | ||
| assert.Empty(t, diff.Deletes) | ||
|
|
||
| for _, v := range diff.Updates { | ||
| validateValueDiff(t, v) | ||
| } | ||
| } | ||
|
|
||
| validateObjectDiff = func(t assert.TestingT, diff resource.ObjectDiff) { | ||
| assert.Empty(t, diff.Adds) | ||
|
|
||
| // Diff does not distinguish from a missing key and a null property, so | ||
| // when we go from a map{k: secret(null)} to a map{k: null}, the diff | ||
| // machinery shows a delete. | ||
| // | ||
| // We have an explicit test for this behavior. | ||
| for _, v := range diff.Deletes { | ||
| assert.Equal(t, resource.MakeSecret(resource.NewNullProperty()), v) | ||
| } | ||
|
|
||
| for _, v := range diff.Updates { | ||
| validateValueDiff(t, v) | ||
| } | ||
| } | ||
|
|
||
| t.Run("rapid", rapid.MakeCheck(func(t *rapid.T) { | ||
| m := resource.ToResourcePropertyValue(prapid.Map(5).Draw(t, "top-level")).ObjectValue() | ||
| if m.ContainsSecrets() { | ||
| unsecreted := removeSecrets(m) | ||
| assert.False(t, unsecreted.ContainsSecrets()) | ||
|
|
||
| // We need to assert that the only change between m and unsecreted | ||
| // is that secret values went to their element values. | ||
| if d := m.DiffIncludeUnknowns(unsecreted); d != nil { | ||
| validateObjectDiff(t, *d) | ||
| } | ||
| } else { | ||
| assert.Equal(t, m, removeSecrets(m)) | ||
| } | ||
| })) | ||
|
|
||
| t.Run("map-null-secrets", func(t *testing.T) { | ||
| assert.Equal(t, | ||
| resource.PropertyMap{ | ||
| "null": resource.NewNullProperty(), | ||
| }, | ||
| removeSecrets(resource.PropertyMap{ | ||
| "null": resource.MakeSecret(resource.NewNullProperty()), | ||
| }), | ||
| ) | ||
| }) | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| // Copyright 2016-2024, Pulumi Corporation. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
|
|
@@ -68,19 +68,33 @@ func (*ConfigEncoding) tryUnwrapSecret(encoded any) (any, bool) { | |
| } | ||
|
|
||
| func (enc *ConfigEncoding) convertStringToPropertyValue(s string, typ shim.ValueType) (resource.PropertyValue, error) { | ||
|
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.
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 does need to handle secrets, with or without #2410.
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
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 ? |
||
| // We attempt to deserialize the input string as JSON and convert the result into a Pulumi | ||
| // property. If the input string is empty, we will return an appropriate zero value. | ||
| // | ||
| // We need to attempt JSON de-serialization for all incoming strings, even if the target type is also | ||
| // a string. This allows us to correctly handle secret or computed string values. For example, the | ||
| // provider might be sent an input like this: | ||
| // | ||
| // resource.PropertyMap{ | ||
| // "k": resource.NewProperty(`{"4dabf18193072939515e22adb298388d":"1b47061264138c4ac30d75fd1eb44270","value":"secret-value"}`), | ||
| // } | ||
| // | ||
| // Even if `k` is a property of type string and it's value is a string, it should be decoded as: | ||
| // | ||
| // resource.PropertyMap{ | ||
| // "k": resource.MakeSecret(resource.NewProperty("secret-value")), | ||
| // } | ||
| // | ||
| //nolint:lll | ||
| if s == "" { | ||
| return enc.zeroValue(typ), nil | ||
| } | ||
|
|
||
| var jsonValue interface{} | ||
| if err := json.Unmarshal([]byte(s), &jsonValue); err != nil { | ||
| if typ == shim.TypeString { | ||
| return resource.NewProperty(s), nil | ||
| } | ||
| return resource.PropertyValue{}, err | ||
| } | ||
|
|
||
|
|
@@ -102,10 +116,12 @@ func (enc *ConfigEncoding) convertStringToPropertyValue(s string, typ shim.Value | |
|
|
||
| func (*ConfigEncoding) zeroValue(typ shim.ValueType) resource.PropertyValue { | ||
| switch typ { | ||
| case shim.TypeString: | ||
| return resource.NewProperty("") | ||
| case shim.TypeBool: | ||
| return resource.NewProperty(false) | ||
| case shim.TypeInt, shim.TypeFloat: | ||
| return resource.NewProperty[float64](0) | ||
| case shim.TypeList, shim.TypeSet: | ||
| return resource.NewPropertyValue([]interface{}{}) | ||
| default: | ||
|
|
||
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.
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'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just PF (and muxed providers that use PF).