Fix unexplained detailed diffs

pkg/tfbridge/diff.go

@@ -156,23 +156,42 @@ func visitPropertyValue(
 	}
 }
 
-func makePropertyDiff(ctx context.Context, name, path string, v resource.PropertyValue, tfDiff shim.InstanceDiff,
-	diff map[string]*pulumirpc.PropertyDiff, forceDiff *bool,
-	tfs shim.Schema, ps *SchemaInfo, finalize, rawNames bool) {
+func makePropertyDiff(
+	ctx context.Context,
+	name, path string,
+	v resource.PropertyValue,
+	tfDiff shim.InstanceDiff,
+	diff map[string]*pulumirpc.PropertyDiff, // makePropertyDiff populates this output map
+	collectionDiffs map[string]*pulumirpc.PropertyDiff, // optional collection-level diffs
+	forceDiff *bool,
+	tfs shim.Schema,
+	ps *SchemaInfo,
+	finalize, rawNames bool,
+) {
 
 	visitor := func(name, path string, v resource.PropertyValue) bool {
 		switch {
 		case v.IsArray():
 			// If this value has a diff and is considered computed by Terraform, the diff will be woefully incomplete. In
 			// this case, do not recurse into the array; instead, just use the count diff for the details.
 			if d := tfDiff.Attribute(name + ".#"); d == nil || !d.NewComputed {
+				if d != nil && d.Old != d.New {
+					collectionDiffs[path] = &pulumirpc.PropertyDiff{
+						Kind: pulumirpc.PropertyDiff_UPDATE,
+					}
+				}
 				return true
 			}
 			name += ".#"
 		case v.IsObject():
 			// If this value has a diff and is considered computed by Terraform, the diff will be woefully incomplete. In
 			// this case, do not recurse into the array; instead, just use the count diff for the details.
 			if d := tfDiff.Attribute(name + ".%"); d == nil || !d.NewComputed {
+				if d != nil && d.Old != d.New {
+					collectionDiffs[path] = &pulumirpc.PropertyDiff{
+						Kind: pulumirpc.PropertyDiff_UPDATE,
+					}
+				}
 				return true
 			}
 			name += ".%"
@@ -286,7 +305,8 @@ func computeIgnoreChanges(
 	return ignoredKeySet
 }
 
-// makeDetailedDiff converts the given state (olds), config (news), and InstanceDiff to a Pulumi property diff.
+// makeDetailedDiff converts the given state (olds), config (news), and InstanceDiff to a Pulumi
+// property diff.
 //
 // See makePropertyDiff for more details.
 func makeDetailedDiff(
@@ -296,37 +316,63 @@ func makeDetailedDiff(
 	olds, news resource.PropertyMap,
 	tfDiff shim.InstanceDiff,
 ) (map[string]*pulumirpc.PropertyDiff, pulumirpc.DiffResponse_DiffChanges) {
+	dd := makeDetailedDiffExtra(ctx, tfs, ps, olds, news, tfDiff)
+	return dd.diffs, dd.changes
+}
 
+type detailedDiffExtra struct {
+	changes         pulumirpc.DiffResponse_DiffChanges
+	diffs           map[string]*pulumirpc.PropertyDiff
+	collectionDiffs map[string]*pulumirpc.PropertyDiff
+}
+
+func makeDetailedDiffExtra(
+	ctx context.Context,
+	tfs shim.SchemaMap,
+	ps map[string]*SchemaInfo,
+	olds, news resource.PropertyMap,
+	tfDiff shim.InstanceDiff,
+) detailedDiffExtra {
 	if tfDiff == nil {
-		return map[string]*pulumirpc.PropertyDiff{}, pulumirpc.DiffResponse_DIFF_NONE
+		return detailedDiffExtra{changes: pulumirpc.DiffResponse_DIFF_NONE}
 	}
 
 	// Check both the old state and the new config for diffs and report them as necessary.
 	//
-	// There is a minor complication here: Terraform has no concept of an "add" diff. Instead, adds are recorded as
-	// updates with an old property value of the empty string. In order to detect adds--and to ensure that all diffs
-	// in the InstanceDiff are reflected in the resulting Pulumi property diff--we first call this function with
-	// each property in a resource's state, then with each property in its config. Any diffs that only appear in the
-	// config are treated as adds; diffs that appear in both the state and config are treated as updates.
+	// There is a minor complication here: Terraform has no concept of an "add" diff. Instead,
+	// adds are recorded as updates with an old property value of the empty string. In order to
+	// detect adds--and to ensure that all diffs in the InstanceDiff are reflected in the
+	// resulting Pulumi property diff--we first call this function with each property in a
+	// resource's state, then with each property in its config. Any diffs that only appear in
+	// the config are treated as adds; diffs that appear in both the state and config are
+	// treated as updates.
 
 	forceDiff := new(bool)
 	diff := map[string]*pulumirpc.PropertyDiff{}
+	collectionDiffs := map[string]*pulumirpc.PropertyDiff{}
 	for k, v := range olds {
 		en, etf, eps := getInfoFromPulumiName(k, tfs, ps, false)
-		makePropertyDiff(ctx, en, string(k), v, tfDiff, diff, forceDiff, etf, eps, false, shimutil.IsOfTypeMap(etf))
+		makePropertyDiff(ctx, en, string(k), v, tfDiff, diff, collectionDiffs, forceDiff,
+			etf, eps, false, shimutil.IsOfTypeMap(etf))
 	}
 	for k, v := range news {
 		en, etf, eps := getInfoFromPulumiName(k, tfs, ps, false)
-		makePropertyDiff(ctx, en, string(k), v, tfDiff, diff, forceDiff, etf, eps, false, shimutil.IsOfTypeMap(etf))
+		makePropertyDiff(ctx, en, string(k), v, tfDiff, diff, collectionDiffs, forceDiff,
+			etf, eps, false, shimutil.IsOfTypeMap(etf))
 	}
 	for k, v := range olds {
 		en, etf, eps := getInfoFromPulumiName(k, tfs, ps, false)
-		makePropertyDiff(ctx, en, string(k), v, tfDiff, diff, forceDiff, etf, eps, true, shimutil.IsOfTypeMap(etf))
+		makePropertyDiff(ctx, en, string(k), v, tfDiff, diff, collectionDiffs, forceDiff,
+			etf, eps, true, shimutil.IsOfTypeMap(etf))
 	}
 
 	changes := pulumirpc.DiffResponse_DIFF_NONE
 	if len(diff) > 0 || *forceDiff {
 		changes = pulumirpc.DiffResponse_DIFF_SOME
 	}
-	return diff, changes
+	return detailedDiffExtra{
+		changes:         changes,
+		diffs:           diff,
+		collectionDiffs: collectionDiffs,
+	}
 }

pkg/tfbridge/diff_test.go

@@ -5,11 +5,14 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
 	"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
 	v2Schema "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	"github.com/pulumi/pulumi/sdk/v3/go/common/resource"
+	"github.com/pulumi/pulumi/sdk/v3/go/common/resource/plugin"
 	pulumirpc "github.com/pulumi/pulumi/sdk/v3/proto/go"
-	"github.com/stretchr/testify/assert"
 
 	shim "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim"
 	shimv1 "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim/sdk-v1"
@@ -2066,3 +2069,119 @@ func TestListNestedAddMaxItemsOne(t *testing.T) {
 		},
 		pulumirpc.DiffResponse_DIFF_SOME)
 }
+
+type diffTestCase struct {
+	resourceSchema              map[string]*schema.Schema
+	resourceFields              map[string]*SchemaInfo
+	state                       resource.PropertyMap
+	inputs                      resource.PropertyMap
+	expected                    map[string]*pulumirpc.PropertyDiff
+	expectedDiffChanges         pulumirpc.DiffResponse_DiffChanges
+	ignoreChanges               []string
+	XSkipDetailedDiffForChanges bool
+}
+
+func diffTest2(t *testing.T, tc diffTestCase) {
+	ctx := context.Background()
+	res := &schema.Resource{
+		Schema: tc.resourceSchema,
+	}
+	provider := shimv1.NewProvider(&schema.Provider{
+		ResourcesMap: map[string]*schema.Resource{
+			"p_resource": res,
+		},
+	})
+	state, err := plugin.MarshalProperties(tc.state, plugin.MarshalOptions{})
+	require.NoError(t, err)
+
+	inputs, err := plugin.MarshalProperties(tc.inputs, plugin.MarshalOptions{})
+	require.NoError(t, err)
+
+	p := Provider{
+		tf: provider,
+		info: ProviderInfo{
+			XSkipDetailedDiffForChanges: tc.XSkipDetailedDiffForChanges,
+			Resources: map[string]*ResourceInfo{
+				"p_resource": {
+					Tok:    "pkg:index:PResource",
+					Fields: tc.resourceFields,
+				},
+			},
+		},
+	}
+
+	p.initResourceMaps()
+
+	resp, err := p.Diff(ctx, &pulumirpc.DiffRequest{
+		Id:            "myResource",
+		Urn:           "urn:pulumi:test::test::pkg:index:PResource::n1",
+		Olds:          state,
+		News:          inputs,
+		IgnoreChanges: tc.ignoreChanges,
+	})
+	require.NoError(t, err)
+
+	assert.Equal(t, tc.expectedDiffChanges, resp.Changes)
+	require.Equal(t, tc.expected, resp.DetailedDiff)
+}
+
+func TestChangingMaxItems1FilterProperty(t *testing.T) {
+	schema := map[string]*schema.Schema{
+		"rule": {
+			Type:     schema.TypeList,
+			Required: true,
+			MaxItems: 1000,
+			Elem: &schema.Resource{
+				Schema: map[string]*schema.Schema{
+					"filter": {
+						Type:     schema.TypeList,
+						Optional: true,
+						MaxItems: 1,
+						Elem: &schema.Resource{
+							Schema: map[string]*schema.Schema{
+								"prefix": {
+									Type:     schema.TypeString,
+									Optional: true,
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+	diffTest2(t, diffTestCase{
+		XSkipDetailedDiffForChanges: true,
+		resourceSchema:              schema,
+		state: resource.PropertyMap{
+			"rules": resource.NewArrayProperty(
+				[]resource.PropertyValue{
+					resource.NewObjectProperty(
+						resource.PropertyMap{
+							"filter": resource.NewNullProperty(),
+						},
+					),
+				},
+			),
+		},
+		inputs: resource.PropertyMap{
+			"rules": resource.NewArrayProperty([]resource.PropertyValue{
+				resource.NewObjectProperty(resource.PropertyMap{
+					"filter": resource.NewObjectProperty(
+						resource.PropertyMap{
+							"__defaults": resource.NewArrayProperty(
+								[]resource.PropertyValue{},
+							),
+						},
+					),
+				}),
+			}),
+		},
+		expectedDiffChanges: pulumirpc.DiffResponse_DIFF_SOME,
+		expected: map[string]*pulumirpc.PropertyDiff{
+			"rules[0].filter": {
+				Kind: pulumirpc.PropertyDiff_UPDATE,
+			},
+		},
+	})
+}

pkg/tfbridge/provider.go

@@ -895,7 +895,8 @@ func (p *Provider) Diff(ctx context.Context, req *pulumirpc.DiffRequest) (*pulum
 		return nil, errors.Wrapf(err, "diffing %s", urn)
 	}
 
-	detailedDiff, changes := makeDetailedDiff(ctx, schema, fields, olds, news, diff)
+	dd := makeDetailedDiffExtra(ctx, schema, fields, olds, news, diff)
+	detailedDiff, changes := dd.diffs, dd.changes
 
 	// There are some providers/situations which `makeDetailedDiff` distorts the expected changes, leading
 	// to changes being dropped by Pulumi.
@@ -906,6 +907,10 @@ func (p *Provider) Diff(ctx context.Context, req *pulumirpc.DiffRequest) (*pulum
 	// See also https://github.com/pulumi/pulumi-terraform-bridge/issues/1501.
 	if p.info.XSkipDetailedDiffForChanges && len(diff.Attributes()) > 0 {
 		changes = pulumirpc.DiffResponse_DIFF_SOME
+		// Perhaps collectionDiffs can shed some light and locate the changes to the end-user.
+		for path, diff := range dd.collectionDiffs {
+			detailedDiff[path] = diff
+		}
 	}
 
 	// If there were changes in this diff, check to see if we have a replacement.