Fix overeager diffs (#1927)

With AWS 3880 there is some evidence (derivation in
#1917) that
sometimes TF has entries in the InstanceDiff.Attributes while still
planning to take the resource to the end-state that is identical to the
original state. IN these cases, TF does not display a diff but Pulumi
does.

The root cause here remains unfixed
(#1895) - Pulumi
bridge is editing terraform-pulgin-sdk to expose the InstanceDiff
structure to connect it to the makeDetailedDiff machinery. Pulumi
should, like TF, stick to the gRPC protocol and rely only on the
PlannedState value.

We can incrementally approach the desired behavior with this change
though which detects PlannedState=PriorState case and suppresses any
diffs in this case.

Fixes:

- pulumi/pulumi-aws#3880
- pulumi/pulumi-aws#3306
- pulumi/pulumi-aws#3190
- pulumi/pulumi-aws#3454

---------

Co-authored-by: Venelin <[email protected]>

pkg/tfbridge/provider.go

@@ -960,7 +960,7 @@ func (p *Provider) Diff(ctx context.Context, req *pulumirpc.DiffRequest) (*pulum
 	// We will still use `detailedDiff` for diff display purposes.
 
 	// See also https://github.com/pulumi/pulumi-terraform-bridge/issues/1501.
-	if p.info.XSkipDetailedDiffForChanges && len(diff.Attributes()) > 0 {
+	if p.info.XSkipDetailedDiffForChanges && !diff.HasNoChanges() {
 		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 {

pkg/tfbridge/tests/provider_test.go

@@ -3,6 +3,7 @@ package tfbridgetests
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 	"io"
 	"testing"
 
@@ -68,6 +69,136 @@ func TestWithNewTestProvider(t *testing.T) {
 	`)
 }
 
+func TestReproMinimalDiffCycle(t *testing.T) {
+	customResponseSchema := func() *schema.Schema {
+		return &schema.Schema{
+			Type:     schema.TypeList,
+			Optional: true,
+			MaxItems: 1,
+			Elem: &schema.Resource{
+				Schema: map[string]*schema.Schema{
+					"custom_response_body_key": {
+						Type:     schema.TypeString,
+						Optional: true,
+					},
+				},
+			},
+		}
+	}
+	blockConfigSchema := func() *schema.Schema {
+		return &schema.Schema{
+			Type:     schema.TypeList,
+			Optional: true,
+			MaxItems: 1,
+			Elem: &schema.Resource{
+				Schema: map[string]*schema.Schema{
+					"custom_response": customResponseSchema(),
+				},
+			},
+		}
+	}
+	ruleElement := &schema.Resource{
+		Schema: map[string]*schema.Schema{
+			"action": {
+				Type:     schema.TypeList,
+				Optional: true,
+				MaxItems: 1,
+				Elem: &schema.Resource{
+					Schema: map[string]*schema.Schema{
+						"block": blockConfigSchema(),
+					},
+				},
+			},
+		},
+	}
+
+	resource := &schema.Resource{
+		Schema: map[string]*schema.Schema{
+			"rule": {
+				Type:     schema.TypeSet,
+				Optional: true,
+				Elem:     ruleElement,
+			},
+		},
+	}
+
+	// Here i may receive maps or slices over base types and *schema.Set which is not friendly to diffing.
+	resource.Schema["rule"].Set = func(i interface{}) int {
+		actual := schema.HashResource(resource.Schema["rule"].Elem.(*schema.Resource))(i)
+		fmt.Printf("hashing %#v as %d\n", i, actual)
+		return actual
+	}
+	ctx := context.Background()
+	p := newTestProvider(ctx, tfbridge.ProviderInfo{
+		P: shimv2.NewProvider(&schema.Provider{
+			Schema: map[string]*schema.Schema{},
+			ResourcesMap: map[string]*schema.Resource{
+				"example_resource": resource,
+			},
+		}, shimv2.WithPlanResourceChange(func(tfResourceType string) bool {
+			return true
+		})),
+		Name:           "testprov",
+		ResourcePrefix: "example",
+		Resources: map[string]*tfbridge.ResourceInfo{
+			"example_resource": {Tok: "testprov:index:ExampleResource"},
+		},
+	}, newTestProviderOptions{})
+
+	replay.Replay(t, p, `
+	{
+	  "method": "/pulumirpc.ResourceProvider/Diff",
+	  "request": {
+	    "id": "newid",
+	    "urn": "urn:pulumi:test::project::testprov:index:ExampleResource::example",
+	    "olds": {
+	      "id": "newid",
+	      "rules": [
+		{
+		  "action": {
+		    "block": {
+		      "customResponse": null
+		    }
+		  }
+		}
+	      ]
+	    },
+	    "news": {
+	      "__defaults": [],
+	      "rules": [
+		{
+		  "__defaults": [],
+		  "action": {
+		    "__defaults": [],
+		    "block": {
+		      "__defaults": []
+		    }
+		  }
+		}
+	      ]
+	    },
+	    "oldInputs": {
+	      "__defaults": [],
+	      "rules": [
+		{
+		  "__defaults": [],
+		  "action": {
+		    "__defaults": [],
+		    "block": {
+		      "__defaults": []
+		    }
+		  }
+		}
+	      ]
+	    }
+	  },
+	  "response": {
+	    "changes": "DIFF_NONE",
+	    "hasDetailedDiff": true
+	  }
+	}`)
+}
+
 func nilSink() diag.Sink {
 	nilSink := diag.DefaultSink(io.Discard, io.Discard, diag.FormatOptions{
 		Color: colors.Never,

pkg/tfshim/sdk-v1/instance_diff.go

@@ -57,6 +57,10 @@ func (d v1InstanceDiff) Attribute(key string) *shim.ResourceAttrDiff {
 	return resourceAttrDiffToShim(d.tf.Attributes[key])
 }
 
+func (d v1InstanceDiff) HasNoChanges() bool {
+	return len(d.Attributes()) == 0
+}
+
 func (d v1InstanceDiff) Attributes() map[string]shim.ResourceAttrDiff {
 	m := map[string]shim.ResourceAttrDiff{}
 	for k, v := range d.tf.Attributes {

pkg/tfshim/sdk-v2/instance_diff.go

@@ -46,6 +46,10 @@ func (d v2InstanceDiff) Attribute(key string) *shim.ResourceAttrDiff {
 	return resourceAttrDiffToShim(d.tf.Attributes[key])
 }
 
+func (d v2InstanceDiff) HasNoChanges() bool {
+	return len(d.Attributes()) == 0
+}
+
 func (d v2InstanceDiff) Attributes() map[string]shim.ResourceAttrDiff {
 	m := map[string]shim.ResourceAttrDiff{}
 	for k, v := range d.tf.Attributes {

pkg/tfshim/sdk-v2/provider2.go

@@ -444,6 +444,17 @@ func (s *grpcServer) PlanResourceChange(
 	if err != nil {
 		return nil, err
 	}
+
+	// There are cases where planned state is equal to the original state, but InstanceDiff still displays changes.
+	// Pulumi considers this to be a no-change diff, and as a workaround here any InstanceDiff changes are deleted
+	// and ignored (simlar to processIgnoreChanges).
+	//
+	// See pulumi/pulumi-aws#3880
+	same := plannedState.Equals(priorState)
+	if same.IsKnown() && same.True() {
+		resp.InstanceDiff.Attributes = map[string]*terraform.ResourceAttrDiff{}
+	}
+
 	var meta map[string]interface{}
 	if resp.PlannedPrivate != nil {
 		if err := json.Unmarshal(resp.PlannedPrivate, &meta); err != nil {

pkg/tfshim/shim.go

@@ -43,7 +43,7 @@ type ResourceAttrDiff struct {
 
 type InstanceDiff interface {
 	Attribute(key string) *ResourceAttrDiff
-	Attributes() map[string]ResourceAttrDiff
+	HasNoChanges() bool
 	ProposedState(res Resource, priorState InstanceState) (InstanceState, error)
 	Destroy() bool
 	RequiresNew() bool

pkg/tfshim/tfplugin5/instance_diff.go

@@ -63,6 +63,10 @@ func (d *instanceDiff) Attributes() map[string]shim.ResourceAttrDiff {
 	return d.attributes
 }
 
+func (d *instanceDiff) HasNoChanges() bool {
+	return len(d.attributes) == 0
+}
+
 func (d *instanceDiff) ProposedState(res shim.Resource, priorState shim.InstanceState) (shim.InstanceState, error) {
 	plannedObject, err := ctyToGo(d.planned)
 	if err != nil {

pkg/tfshim/tfplugin5/provider_test.go

@@ -1180,7 +1180,7 @@ func TestApply(t *testing.T) {
 			diff, err := p.Diff(ctx, "example_resource", state, config, shim.DiffOptions{})
 			require.NoError(t, err)
 
-			if len(diff.Attributes()) == 0 {
+			if diff.HasNoChanges() {
 				return
 			}