From 77f3f11bfcce41bd41418be4b70a47045ab5d4db Mon Sep 17 00:00:00 2001 From: Ian Wahbe Date: Tue, 14 Nov 2023 16:15:42 -0800 Subject: [PATCH] Augment `makeDetailedDiff` when replace is specified (#1520) This will fix (upon adoption) https://github.com/pulumi/pulumi-aws/issues/2971. The design discussion for this PR is https://pulumi.slack.com/archives/C02FXTZEZ6W/p1699989102568939. ### Changes If there is a replace on the underlying TF object (as indicated by `diff.RequiresNew() || diff.Destroy()`) but no associated replace in the wire return (`pulumirpc.DiffResponse.Replaces`), then we set `pulumirpc.DiffResponse.Replaces = pulumirpc.DiffResponse_DIFF_SOME` and insert `__meta` as needing a replace. --- internal/testprovider/schema.go | 13 ++++++ pkg/tfbridge/provider.go | 22 +++++++++- pkg/tfbridge/provider_test.go | 78 +++++++++++++++++++++++++++++---- 3 files changed, 103 insertions(+), 10 deletions(-) diff --git a/internal/testprovider/schema.go b/internal/testprovider/schema.go index 00b3e01d1..f781e6755 100644 --- a/internal/testprovider/schema.go +++ b/internal/testprovider/schema.go @@ -895,6 +895,19 @@ func CustomizedDiffProvider(f func(data *schemav2.ResourceData)) *schemav2.Provi return err }, }, + "test_replace": { + Schema: map[string]*schemav2.Schema{ + "labels": {Type: schemav2.TypeString, Optional: true, Computed: true}, + }, + SchemaVersion: 1, + CustomizeDiff: func(ctx context.Context, diff *schemav2.ResourceDiff, i interface{}) error { + err := diff.SetNew("labels", "1") + if err != nil { + return err + } + return diff.ForceNew("labels") + }, + }, }, } } diff --git a/pkg/tfbridge/provider.go b/pkg/tfbridge/provider.go index f568f6310..484efda5b 100644 --- a/pkg/tfbridge/provider.go +++ b/pkg/tfbridge/provider.go @@ -718,6 +718,21 @@ func (p *Provider) Diff(ctx context.Context, req *pulumirpc.DiffRequest) (*pulum deleteBeforeReplace := len(replaces) > 0 && (res.Schema.DeleteBeforeReplace || nameRequiresDeleteBeforeReplace(news, olds, res.TF.Schema(), res.Schema)) + // If the upstream diff object indicates a replace is necessary and we have not + // recorded any replaces, that means that `makeDetailedDiff` failed to translate a + // property. This is known to happen for computed input properties: + // + // https://github.com/pulumi/pulumi-aws/issues/2971 + if (diff.RequiresNew() || diff.Destroy()) && + // In theory, we should be safe to set __meta as replaces whenever + // `diff.RequiresNew() || diff.Destroy()` but by checking replaces we + // limit the blast radius of this change to diffs that we know will panic + // later on. + len(replaces) == 0 { + replaces = append(replaces, "__meta") + changes = pulumirpc.DiffResponse_DIFF_SOME + } + return &pulumirpc.DiffResponse{ Changes: changes, Replaces: replaces, @@ -977,8 +992,11 @@ func (p *Provider) Update(ctx context.Context, req *pulumirpc.UpdateRequest) (*p // ignoring changes to the keys that would result in replacement/deletion. doIgnoreChanges(ctx, res.TF.Schema(), res.Schema.Fields, olds, news, req.GetIgnoreChanges(), diff) - contract.Assertf(!diff.Destroy() && !diff.RequiresNew(), - "Expected diff to not require deletion or replacement during Update of %s", urn) + if diff.Destroy() || diff.RequiresNew() { + return nil, fmt.Errorf("internal: expected diff to not require deletion or replacement"+ + " during Update of %s. Found delete=%t, replace=%t. This indicates a bug in provider.", + urn, diff.Destroy(), diff.RequiresNew()) + } if req.Timeout != 0 { diff.SetTimeout(req.Timeout, shim.TimeoutUpdate) diff --git a/pkg/tfbridge/provider_test.go b/pkg/tfbridge/provider_test.go index f9d4fdbcb..c111b5edd 100644 --- a/pkg/tfbridge/provider_test.go +++ b/pkg/tfbridge/provider_test.go @@ -1841,32 +1841,35 @@ func TestTransformOutputs(t *testing.T) { } func TestSkipDetailedDiff(t *testing.T) { - provider := func(t *testing.T) *Provider { + provider := func(t *testing.T, skipDetailedDiffForChanges bool) *Provider { p := testprovider.CustomizedDiffProvider(func(data *schema.ResourceData) {}) return &Provider{ tf: shimv2.NewProvider(p), config: shimv2.NewSchemaMap(p.Schema), resources: map[tokens.Type]Resource{ - "TestResource": { + "Resource": { TF: shimv2.NewResource(p.ResourcesMap["test_resource"]), TFName: "test_resource", - Schema: &ResourceInfo{ - Tok: "TestResource", - }, + Schema: &ResourceInfo{Tok: "Resource"}, + }, + "Replace": { + TF: shimv2.NewResource(p.ResourcesMap["test_replace"]), + TFName: "test_replace", + Schema: &ResourceInfo{Tok: "Replace"}, }, }, info: ProviderInfo{ - XSkipDetailedDiffForChanges: true, + XSkipDetailedDiffForChanges: skipDetailedDiffForChanges, }, } } t.Run("Diff", func(t *testing.T) { - testutils.Replay(t, provider(t), ` + testutils.Replay(t, provider(t, true), ` { "method": "/pulumirpc.ResourceProvider/Diff", "request": { "id": "0", - "urn": "urn:pulumi:dev::teststack::TestResource::exres", + "urn": "urn:pulumi:dev::teststack::Resource::exres", "olds": {}, "news": {} }, @@ -1876,6 +1879,65 @@ func TestSkipDetailedDiff(t *testing.T) { } }`) }) + + // This test checks that we will flag some meta field (`__meta`) as a replace if + // the upstream diff indicates a replace but there is no field associated with the + // replace. + // + // We do this since Pulumi's gRPC protocol doesn't have direct support for + // declaring a replace on a resource without an associated property. + t.Run("EmptyDiffWithReplace", func(t *testing.T) { + test := func(skipDetailedDiffForChanges bool) func(t *testing.T) { + return func(t *testing.T) { + testutils.Replay(t, provider(t, skipDetailedDiffForChanges), ` + { + "method": "/pulumirpc.ResourceProvider/Diff", + "request": { + "id": "0", + "urn": "urn:pulumi:dev::teststack::Replace::exres", + "olds": {}, + "news": {} + }, + "response": { + "changes": "DIFF_SOME", + "replaces": ["__meta"], + "hasDetailedDiff": true + } + }`) + } + } + t.Run("withDetailedDiff", test(false)) + t.Run("skipDetailedDiff", test(true)) + }) + + // This test checks that we don't insert extraneous replaces when there is an + // existing field that holds a replace. + t.Run("FullDiffWithReplace", func(t *testing.T) { + test := func(skipDetailedDiffForChanges bool) func(t *testing.T) { + return func(t *testing.T) { + testutils.Replay(t, provider(t, skipDetailedDiffForChanges), ` + { + "method": "/pulumirpc.ResourceProvider/Diff", + "request": { + "id": "0", + "urn": "urn:pulumi:dev::teststack::Replace::exres", + "olds": {"labels": "0"}, + "news": {"labels": "1"} + }, + "response": { + "changes": "DIFF_SOME", + "replaces": ["labels"], + "hasDetailedDiff": true, + "detailedDiff": { "labels": { "kind": "UPDATE_REPLACE" } }, + "diffs": ["labels"] + } + }`) + } + } + t.Run("withDetailedDiff", test(false)) + t.Run("skipDetailedDiff", test(true)) + }) + } func TestTransformFromState(t *testing.T) {