Augment makeDetailedDiff when replace is specified (#1520)

This will fix (upon adoption) pulumi/pulumi-aws#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.
pulumi · Nov 15, 2023 · 77f3f11 · 77f3f11
1 parent 6f40195
commit 77f3f11
Show file tree

Hide file tree

Showing 3 changed files with 103 additions and 10 deletions.
diff --git 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
@@ -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
@@ -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) {