From 9319c4bef31e083e846c2e2a7ab684b14f071dcc Mon Sep 17 00:00:00 2001 From: VenelinMartinov Date: Tue, 26 Nov 2024 12:48:56 +0000 Subject: [PATCH] Detailed diff handle secrets and outputs (#2643) This change adds secret and output handling to the detailed diff v2 code. Secrets and outputs are not necessary for the algorithm so we just strip both from the inputs. fixes https://github.com/pulumi/pulumi-terraform-bridge/issues/2526 fixes https://github.com/pulumi/pulumi-terraform-bridge/issues/2651 --- pkg/pf/tests/diff_secret_test.go | 18 ++- pkg/pf/tests/provider_diff_test.go | 3 - pkg/pf/tests/util.go | 9 +- pkg/tfbridge/detailed_diff.go | 14 +++ pkg/tfbridge/detailed_diff_test.go | 174 +++++++++++++++++++++++++++-- 5 files changed, 189 insertions(+), 29 deletions(-) diff --git a/pkg/pf/tests/diff_secret_test.go b/pkg/pf/tests/diff_secret_test.go index 75fa89d83..d6de0e962 100644 --- a/pkg/pf/tests/diff_secret_test.go +++ b/pkg/pf/tests/diff_secret_test.go @@ -18,7 +18,6 @@ import ( ) func TestSecretBasic(t *testing.T) { - t.Skip("skipping until #2643") t.Parallel() provBuilder := providerbuilder.NewProvider( providerbuilder.NewProviderArgs{ @@ -58,7 +57,7 @@ resources: ~ testprovider:index/test:Test: (update) [id=test-id] [urn=urn:pulumi:test::test::testprovider:index/test:Test::mainRes] - s: [secret] + ~ s: [secret] => [secret] Resources: ~ 1 to update 1 unchanged @@ -66,7 +65,6 @@ Resources: } func TestSecretSet(t *testing.T) { - t.Skip("skipping until #2643") t.Parallel() provBuilder := pb.NewProvider(pb.NewProviderArgs{ @@ -160,7 +158,6 @@ Resources: } func TestSecretObjectBlock(t *testing.T) { - t.Skip("skipping until #2643") t.Parallel() provBuilder := pb.NewProvider(pb.NewProviderArgs{ @@ -205,7 +202,9 @@ resources: ~ testprovider:index/test:Test: (update) [id=test-id] [urn=urn:pulumi:test::test::testprovider:index/test:Test::mainRes] - key: [secret] + ~ key: { + ~ prop1: [secret] => [secret] + } Resources: ~ 1 to update 1 unchanged @@ -226,9 +225,8 @@ Resources: ~ testprovider:index/test:Test: (update) [id=test-id] [urn=urn:pulumi:test::test::testprovider:index/test:Test::mainRes] - key: { - prop1: [secret] - prop2: "value2" + ~ key: { + ~ prop1: [secret] => [secret] } Resources: ~ 1 to update @@ -251,7 +249,6 @@ Resources: [id=test-id] [urn=urn:pulumi:test::test::testprovider:index/test:Test::mainRes] ~ key: { - prop1: [secret] ~ prop2: "value2" => "value3" } Resources: @@ -262,7 +259,6 @@ Resources: } func TestSecretPulumiSchema(t *testing.T) { - t.Skip("skipping until #2643") t.Parallel() provBuilder := pb.NewProvider(pb.NewProviderArgs{ @@ -306,7 +302,7 @@ resources: ~ testprovider:index/test:Test: (update) [id=test-id] [urn=urn:pulumi:test::test::testprovider:index/test:Test::mainRes] - s: [secret] + ~ s: [secret] => [secret] Resources: ~ 1 to update 1 unchanged diff --git a/pkg/pf/tests/provider_diff_test.go b/pkg/pf/tests/provider_diff_test.go index c3158eefc..c551823a0 100644 --- a/pkg/pf/tests/provider_diff_test.go +++ b/pkg/pf/tests/provider_diff_test.go @@ -129,8 +129,6 @@ func TestEmptyTestresDiffWithOptionalComputed(t *testing.T) { func TestDiffWithSecrets(t *testing.T) { t.Parallel() - t.Skip("TODO: secrets") - server, err := newProviderServer(t, testprovider.RandomProvider()) require.NoError(t, err) @@ -175,7 +173,6 @@ func TestDiffWithSecrets(t *testing.T) { // See https://github.com/pulumi/pulumi-random/issues/258 func TestDiffVersionUpgrade(t *testing.T) { t.Parallel() - t.Skip("TODO: secrets") server, err := newProviderServer(t, testprovider.RandomProvider()) require.NoError(t, err) testCase := ` diff --git a/pkg/pf/tests/util.go b/pkg/pf/tests/util.go index 044c662b4..7eac4e7b2 100644 --- a/pkg/pf/tests/util.go +++ b/pkg/pf/tests/util.go @@ -51,10 +51,11 @@ func bridgedProvider(prov *providerbuilder.Provider) info.Provider { shimProvider := tfbridge.ShimProvider(prov) provider := tfbridge0.ProviderInfo{ - P: shimProvider, - Name: prov.TypeName, - Version: "0.0.1", - MetadataInfo: &tfbridge0.MetadataInfo{}, + P: shimProvider, + Name: prov.TypeName, + Version: "0.0.1", + MetadataInfo: &tfbridge0.MetadataInfo{}, + EnableAccurateBridgePreview: true, } provider.MustComputeTokens(tokens.SingleModule(prov.TypeName, "index", tokens.MakeStandard(prov.TypeName))) diff --git a/pkg/tfbridge/detailed_diff.go b/pkg/tfbridge/detailed_diff.go index a50e8563b..e241e90ee 100644 --- a/pkg/tfbridge/detailed_diff.go +++ b/pkg/tfbridge/detailed_diff.go @@ -12,6 +12,7 @@ import ( shim "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim" "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim/walk" + "github.com/pulumi/pulumi-terraform-bridge/v3/unstable/propertyvalue" ) func isPresent(val resource.PropertyValue) bool { @@ -482,6 +483,19 @@ func MakeDetailedDiffV2( ps map[string]*SchemaInfo, priorProps, props, newInputs resource.PropertyMap, ) map[string]*pulumirpc.PropertyDiff { + // Strip secrets and outputs from the properties before calculating the diff. + // This allows the rest of the algorithm to focus on the actual changes and not + // have to deal with the extra noise. + // This is safe to do here because the detailed diff we return to the engine + // is only represented by paths to the values and not the values themselves. + // The engine will then takes care of masking secrets. + stripSecretsAndOutputs := func(props resource.PropertyMap) resource.PropertyMap { + propsVal := propertyvalue.RemoveSecretsAndOutputs(resource.NewProperty(props)) + return propsVal.ObjectValue() + } + priorProps = stripSecretsAndOutputs(priorProps) + props = stripSecretsAndOutputs(props) + newInputs = stripSecretsAndOutputs(newInputs) differ := detailedDiffer{ctx: ctx, tfs: tfs, ps: ps, newInputs: newInputs} return differ.makeDetailedDiffPropertyMap(priorProps, props) } diff --git a/pkg/tfbridge/detailed_diff_test.go b/pkg/tfbridge/detailed_diff_test.go index fbfe92df6..2130d0ce8 100644 --- a/pkg/tfbridge/detailed_diff_test.go +++ b/pkg/tfbridge/detailed_diff_test.go @@ -299,8 +299,7 @@ func runDetailedDiffTest( expected map[string]*pulumirpc.PropertyDiff, ) { t.Helper() - differ := detailedDiffer{tfs: tfs, ps: ps, newInputs: new} - actual := differ.makeDetailedDiffPropertyMap(old, new) + actual := MakeDetailedDiffV2(context.Background(), tfs, ps, old, new, new) require.Equal(t, expected, actual) } @@ -484,21 +483,54 @@ func TestBasicDetailedDiff(t *testing.T) { "foo": ComputedVal, }, ) + propertyMapSecretValue1 := resource.NewPropertyMapFromMap( + map[string]interface{}{ + "foo": tt.value1, + }, + ) + propertyMapSecretValue1["foo"] = resource.NewSecretProperty( + &resource.Secret{Element: propertyMapSecretValue1["foo"]}) + + propertyMapSecretValue2 := resource.NewPropertyMapFromMap( + map[string]interface{}{ + "foo": tt.value2, + }, + ) + propertyMapSecretValue2["foo"] = resource.NewSecretProperty( + &resource.Secret{Element: propertyMapSecretValue2["foo"]}) + + propertyMapOutputValue1 := resource.NewPropertyMapFromMap( + map[string]interface{}{ + "foo": tt.value1, + }, + ) + propertyMapOutputValue1["foo"] = resource.NewOutputProperty( + resource.Output{Element: propertyMapOutputValue1["foo"], Known: true}) + + propertyMapOutputValue2 := resource.NewPropertyMapFromMap( + map[string]interface{}{ + "foo": tt.value2, + }, + ) + propertyMapOutputValue2["foo"] = resource.NewOutputProperty( + resource.Output{Element: propertyMapOutputValue2["foo"], Known: true}) + + defaultChangePath := "foo" + if tt.listLike && tt.objectLike { + defaultChangePath = "foo[0].foo" + } else if tt.listLike { + defaultChangePath = "foo[0]" + } else if tt.objectLike { + defaultChangePath = "foo.foo" + } t.Run("unchanged", func(t *testing.T) { runDetailedDiffTest(t, propertyMapValue1, propertyMapValue1, tfs, ps, map[string]*pulumirpc.PropertyDiff{}) }) t.Run("changed non-empty", func(t *testing.T) { - expected := make(map[string]*pulumirpc.PropertyDiff) - if tt.listLike && tt.objectLike { - expected["foo[0].foo"] = &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_UPDATE} - } else if tt.listLike { - expected["foo[0]"] = &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_UPDATE} - } else if tt.objectLike { - expected["foo.foo"] = &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_UPDATE} - } else { - expected["foo"] = &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_UPDATE} + expected := map[string]*pulumirpc.PropertyDiff{ + defaultChangePath: {Kind: pulumirpc.PropertyDiff_UPDATE}, } runDetailedDiffTest(t, propertyMapValue1, propertyMapValue2, tfs, ps, expected) }) @@ -566,6 +598,56 @@ func TestBasicDetailedDiff(t *testing.T) { runDetailedDiffTest(t, propertyMapNil, propertyMapEmpty, tfs, ps, added()) }) } + + t.Run("secret unchanged", func(t *testing.T) { + runDetailedDiffTest( + t, propertyMapSecretValue1, propertyMapSecretValue1, tfs, ps, map[string]*pulumirpc.PropertyDiff{}) + }) + + t.Run("value unchanged secretness changed", func(t *testing.T) { + runDetailedDiffTest( + t, propertyMapValue1, propertyMapSecretValue1, tfs, ps, map[string]*pulumirpc.PropertyDiff{}) + }) + + t.Run("secret added", func(t *testing.T) { + runDetailedDiffTest(t, propertyMapNil, propertyMapSecretValue1, tfs, ps, added()) + }) + + t.Run("secret deleted", func(t *testing.T) { + runDetailedDiffTest(t, propertyMapSecretValue1, propertyMapNil, tfs, ps, deleted()) + }) + + t.Run("secret changed", func(t *testing.T) { + expected := map[string]*pulumirpc.PropertyDiff{ + defaultChangePath: {Kind: pulumirpc.PropertyDiff_UPDATE}, + } + runDetailedDiffTest(t, propertyMapSecretValue1, propertyMapSecretValue2, tfs, ps, expected) + }) + + t.Run("output unchanged", func(t *testing.T) { + runDetailedDiffTest( + t, propertyMapOutputValue1, propertyMapOutputValue1, tfs, ps, map[string]*pulumirpc.PropertyDiff{}) + }) + + t.Run("value unchanged outputness changed", func(t *testing.T) { + runDetailedDiffTest( + t, propertyMapValue1, propertyMapOutputValue1, tfs, ps, map[string]*pulumirpc.PropertyDiff{}) + }) + + t.Run("output added", func(t *testing.T) { + runDetailedDiffTest(t, propertyMapNil, propertyMapOutputValue1, tfs, ps, added()) + }) + + t.Run("output deleted", func(t *testing.T) { + runDetailedDiffTest(t, propertyMapOutputValue1, propertyMapNil, tfs, ps, deleted()) + }) + + t.Run("output changed", func(t *testing.T) { + expected := map[string]*pulumirpc.PropertyDiff{ + defaultChangePath: {Kind: pulumirpc.PropertyDiff_UPDATE}, + } + runDetailedDiffTest(t, propertyMapOutputValue1, propertyMapOutputValue2, tfs, ps, expected) + }) }) } } @@ -612,6 +694,16 @@ func TestDetailedDiffObject(t *testing.T) { }, ) + propertyMapWithSecrets := resource.PropertyMap{ + resource.PropertyKey("foo"): resource.NewPropertyValue( + resource.PropertyMap{ + resource.PropertyKey("prop1"): resource.NewSecretProperty( + &resource.Secret{Element: resource.NewStringProperty("val1")}), + resource.PropertyKey("prop2"): resource.NewStringProperty("qux"), + }, + ), + } + t.Run("unchanged", func(t *testing.T) { runDetailedDiffTest(t, propertyMapProp1Val1, propertyMapProp1Val1, tfs, ps, map[string]*pulumirpc.PropertyDiff{}) }) @@ -653,6 +745,18 @@ func TestDetailedDiffObject(t *testing.T) { "foo.prop2": {Kind: pulumirpc.PropertyDiff_ADD}, }) }) + + t.Run("secret added", func(t *testing.T) { + runDetailedDiffTest(t, propertyMapProp2, propertyMapWithSecrets, tfs, ps, map[string]*pulumirpc.PropertyDiff{ + "foo.prop1": {Kind: pulumirpc.PropertyDiff_ADD}, + }) + }) + + t.Run("secret deleted", func(t *testing.T) { + runDetailedDiffTest(t, propertyMapWithSecrets, propertyMapProp2, tfs, ps, map[string]*pulumirpc.PropertyDiff{ + "foo.prop1": {Kind: pulumirpc.PropertyDiff_DELETE}, + }) + }) } func TestDetailedDiffList(t *testing.T) { @@ -822,6 +926,22 @@ func TestDetailedDiffSet(t *testing.T) { }, ) + propertyMapWithSecrets := resource.NewPropertyMapFromMap( + map[string]interface{}{ + "foo": []interface{}{resource.NewSecretProperty( + &resource.Secret{Element: resource.NewStringProperty("val1")}), "val2"}, + }, + ) + + propertyMapWithSecretsAndOutputs := resource.NewPropertyMapFromMap( + map[string]interface{}{ + "foo": []interface{}{ + resource.NewSecretProperty(&resource.Secret{Element: resource.NewStringProperty("val1")}), + resource.NewOutputProperty(resource.Output{Element: resource.NewStringProperty("val2")}), + }, + }, + ) + t.Run("unchanged", func(t *testing.T) { runDetailedDiffTest(t, propertyMapVal1, propertyMapVal1, tfs, ps, map[string]*pulumirpc.PropertyDiff{}) }) @@ -870,6 +990,38 @@ func TestDetailedDiffSet(t *testing.T) { "foo[1]": {Kind: pulumirpc.PropertyDiff_ADD}, }) }) + + t.Run("secret added", func(t *testing.T) { + runDetailedDiffTest(t, propertyMapVal2, propertyMapWithSecrets, tfs, ps, map[string]*pulumirpc.PropertyDiff{ + "foo[0]": {Kind: pulumirpc.PropertyDiff_ADD}, + }) + }) + + t.Run("secret and output added", func(t *testing.T) { + runDetailedDiffTest( + t, propertyMapEmpty, propertyMapWithSecretsAndOutputs, tfs, ps, map[string]*pulumirpc.PropertyDiff{ + "foo[0]": {Kind: pulumirpc.PropertyDiff_ADD}, + "foo[1]": {Kind: pulumirpc.PropertyDiff_ADD}, + }) + }) + + t.Run("secret removed", func(t *testing.T) { + runDetailedDiffTest(t, propertyMapWithSecrets, propertyMapVal2, tfs, ps, map[string]*pulumirpc.PropertyDiff{ + "foo[0]": {Kind: pulumirpc.PropertyDiff_DELETE}, + }) + }) + + t.Run("output removed", func(t *testing.T) { + runDetailedDiffTest(t, propertyMapWithSecretsAndOutputs, propertyMapVal1, tfs, ps, map[string]*pulumirpc.PropertyDiff{ + "foo[1]": {Kind: pulumirpc.PropertyDiff_DELETE}, + }) + }) + + t.Run("secretness and outputness changed", func(t *testing.T) { + runDetailedDiffTest(t, propertyMapWithSecretsAndOutputs, propertyMapBoth, tfs, ps, map[string]*pulumirpc.PropertyDiff{ + "foo[1]": {Kind: pulumirpc.PropertyDiff_UPDATE}, + }) + }) } func TestDetailedDiffTFForceNewPlain(t *testing.T) {