Refactor IgnoreChanges

pf/internal/schemashim/provider.go

@@ -79,7 +79,7 @@ func (p *SchemaOnlyProvider) Configure(ctx context.Context, c shim.ResourceConfi
 }
 
 func (p *SchemaOnlyProvider) Diff(
-	ctx context.Context, t string, s shim.InstanceState, c shim.ResourceConfig,
+	ctx context.Context, t string, s shim.InstanceState, c shim.ResourceConfig, opts shim.DiffOptions,
 ) (shim.InstanceDiff, error) {
 	panic("schemaOnlyProvider does not implement runtime operation Diff")
 }

pkg/tfbridge/diff.go

@@ -241,22 +241,37 @@ func makePropertyDiff(ctx context.Context, name, path string, v resource.Propert
 	visitPropertyValue(ctx, name, path, v, tfs, ps, rawNames, visitor)
 }
 
-func doIgnoreChanges(ctx context.Context, tfs shim.SchemaMap, ps map[string]*SchemaInfo,
-	olds, news resource.PropertyMap, ignoredPaths []string, tfDiff shim.InstanceDiff) {
-
-	if tfDiff == nil {
-		return
+func newIgnoreChanges(
+	ctx context.Context,
+	tfs shim.SchemaMap,
+	ps map[string]*SchemaInfo,
+	olds, news resource.PropertyMap,
+	ignoredPaths []string,
+) shim.IgnoreChanges {
+	if len(ignoredPaths) == 0 {
+		return nil
+	}
+	return func() map[string]struct{} {
+		return computeIgnoreChanges(ctx, tfs, ps, olds, news, ignoredPaths)
 	}
+}
 
+// Computes the ignored key set.
+func computeIgnoreChanges(
+	ctx context.Context,
+	tfs shim.SchemaMap,
+	ps map[string]*SchemaInfo,
+	olds, news resource.PropertyMap,
+	ignoredPaths []string,
+) map[string]struct{} {
 	ignoredPathSet := map[string]bool{}
 	for _, p := range ignoredPaths {
 		ignoredPathSet[p] = true
 	}
-
-	ignoredKeySet := map[string]bool{}
+	ignoredKeySet := map[string]struct{}{}
 	visitor := func(attributeKey, propertyPath string, _ resource.PropertyValue) bool {
 		if ignoredPathSet[propertyPath] {
-			ignoredKeySet[attributeKey] = true
+			ignoredKeySet[attributeKey] = struct{}{}
 		}
 		return true
 	}
@@ -268,8 +283,7 @@ func doIgnoreChanges(ctx context.Context, tfs shim.SchemaMap, ps map[string]*Sch
 		en, etf, eps := getInfoFromPulumiName(k, tfs, ps, false)
 		visitPropertyValue(ctx, en, string(k), v, etf, eps, shimutil.IsOfTypeMap(etf), visitor)
 	}
-
-	tfDiff.IgnoreChanges(ignoredKeySet)
+	return ignoredKeySet
 }
 
 // makeDetailedDiff converts the given state (olds), config (news), and InstanceDiff to a Pulumi property diff.

pkg/tfbridge/diff_test.go

@@ -11,6 +11,7 @@ import (
 	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"
 	shimv2 "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim/sdk-v2"
 )
@@ -89,12 +90,11 @@ func TestCustomizeDiff(t *testing.T) {
 		config, _, err := MakeTerraformConfig(ctx, &Provider{tf: provider}, inputsMap, sch, info)
 		assert.NoError(t, err)
 
-		tfDiff, err := provider.Diff(ctx, "resource", tfState, config)
+		tfDiff, err := provider.Diff(ctx, "resource", tfState, config, shim.DiffOptions{
+			IgnoreChanges: newIgnoreChanges(ctx, sch, info, stateMap, inputsMap, ignores),
+		})
 		assert.NoError(t, err)
 
-		// ProcessIgnoreChanges
-		doIgnoreChanges(ctx, sch, info, stateMap, inputsMap, ignores, tfDiff)
-
 		// Convert the diff to a detailed diff and check the result.
 		diff, changes := makeDetailedDiff(ctx, sch, info, stateMap, inputsMap, tfDiff)
 		expectedDiff := map[string]*pulumirpc.PropertyDiff{}
@@ -131,12 +131,11 @@ func TestCustomizeDiff(t *testing.T) {
 		config, _, err := MakeTerraformConfig(ctx, &Provider{tf: provider}, inputsMap, sch, info)
 		assert.NoError(t, err)
 
-		tfDiff, err := provider.Diff(ctx, "resource", tfState, config)
+		tfDiff, err := provider.Diff(ctx, "resource", tfState, config, shim.DiffOptions{
+			IgnoreChanges: newIgnoreChanges(ctx, sch, info, stateMap, inputsMap, ignores),
+		})
 		assert.NoError(t, err)
 
-		// ProcessIgnoreChanges
-		doIgnoreChanges(ctx, sch, info, stateMap, inputsMap, ignores, tfDiff)
-
 		// Convert the diff to a detailed diff and check the result.
 		diff, changes := makeDetailedDiff(ctx, sch, info, stateMap, inputsMap, tfDiff)
 		expectedDiff := map[string]*pulumirpc.PropertyDiff{}
@@ -187,7 +186,7 @@ func TestCustomizeDiff(t *testing.T) {
 				assert.NoError(t, err)
 
 				// Calling Diff with the given CustomizeDiff used to panic, no more asserts needed.
-				_, err = provider.Diff(ctx, "resource", tfState, config)
+				_, err = provider.Diff(ctx, "resource", tfState, config, shim.DiffOptions{})
 				assert.NoError(t, err)
 			})
 		}
@@ -229,30 +228,37 @@ func diffTest(t *testing.T, tfs map[string]*schema.Schema, info map[string]*Sche
 	config, _, err := MakeTerraformConfig(ctx, &Provider{tf: provider}, inputsMap, sch, info)
 	assert.NoError(t, err)
 
-	tfDiff, err := provider.Diff(ctx, "resource", tfState, config)
-	assert.NoError(t, err)
-
-	// ProcessIgnoreChanges
-	doIgnoreChanges(ctx, sch, info, stateMap, inputsMap, ignoreChanges, tfDiff)
+	t.Run("standard", func(t *testing.T) {
+		tfDiff, err := provider.Diff(ctx, "resource", tfState, config, shim.DiffOptions{
+			IgnoreChanges: newIgnoreChanges(ctx, sch, info, stateMap, inputsMap, ignoreChanges),
+		})
+		assert.NoError(t, err)
 
-	// Convert the diff to a detailed diff and check the result.
-	diff, changes := makeDetailedDiff(ctx, sch, info, stateMap, inputsMap, tfDiff)
-	expectedDiff := map[string]*pulumirpc.PropertyDiff{}
-	for k, v := range expected {
-		expectedDiff[k] = &pulumirpc.PropertyDiff{Kind: v}
-	}
-	assert.Equal(t, expectedDiffChanges, changes)
-	assert.Equal(t, expectedDiff, diff)
+		// Convert the diff to a detailed diff and check the result.
+		diff, changes := makeDetailedDiff(ctx, sch, info, stateMap, inputsMap, tfDiff)
+		expectedDiff := map[string]*pulumirpc.PropertyDiff{}
+		for k, v := range expected {
+			expectedDiff[k] = &pulumirpc.PropertyDiff{Kind: v}
+		}
+		assert.Equal(t, expectedDiffChanges, changes)
+		assert.Equal(t, expectedDiff, diff)
+	})
 
-	// Add an ignoreChanges entry for each path in the expected diff, then re-convert the diff and check the result.
-	for k := range expected {
-		ignoreChanges = append(ignoreChanges, k)
-	}
-	doIgnoreChanges(ctx, sch, info, stateMap, inputsMap, ignoreChanges, tfDiff)
+	// Add an ignoreChanges entry for each path in the expected diff, then re-convert the diff
+	// and check the result.
+	t.Run("withIgnoreAllExpected", func(t *testing.T) {
+		for k := range expected {
+			ignoreChanges = append(ignoreChanges, k)
+		}
+		tfDiff, err := provider.Diff(ctx, "resource", tfState, config, shim.DiffOptions{
+			IgnoreChanges: newIgnoreChanges(ctx, sch, info, stateMap, inputsMap, ignoreChanges),
+		})
+		assert.NoError(t, err)
 
-	diff, changes = makeDetailedDiff(ctx, sch, info, stateMap, inputsMap, tfDiff)
-	assert.Equal(t, pulumirpc.DiffResponse_DIFF_NONE, changes)
-	assert.Equal(t, map[string]*pulumirpc.PropertyDiff{}, diff)
+		diff, changes := makeDetailedDiff(ctx, sch, info, stateMap, inputsMap, tfDiff)
+		assert.Equal(t, pulumirpc.DiffResponse_DIFF_NONE, changes)
+		assert.Equal(t, map[string]*pulumirpc.PropertyDiff{}, diff)
+	})
 }
 
 func TestCustomDiffProducesReplace(t *testing.T) {

pkg/tfbridge/provider.go

@@ -869,18 +869,24 @@ func (p *Provider) Diff(ctx context.Context, req *pulumirpc.DiffRequest) (*pulum
 	if err != nil {
 		return nil, err
 	}
-	config, _, err := MakeTerraformConfig(ctx, p, news, res.TF.Schema(), res.Schema.Fields)
+
+	schema, fields := res.TF.Schema(), res.Schema.Fields
+
+	config, _, err := MakeTerraformConfig(ctx, p, news, schema, fields)
 	if err != nil {
 		return nil, errors.Wrapf(err, "preparing %s's new property state", urn)
 	}
 
-	diff, err := p.tf.Diff(ctx, res.TFName, state, config)
+	ic := newIgnoreChanges(ctx, schema, fields, olds, news, req.GetIgnoreChanges())
+
+	diff, err := p.tf.Diff(ctx, res.TFName, state, config, shim.DiffOptions{
+		IgnoreChanges: ic,
+	})
 	if err != nil {
 		return nil, errors.Wrapf(err, "diffing %s", urn)
 	}
 
-	doIgnoreChanges(ctx, res.TF.Schema(), res.Schema.Fields, olds, news, req.GetIgnoreChanges(), diff)
-	detailedDiff, changes := makeDetailedDiff(ctx, res.TF.Schema(), res.Schema.Fields, olds, news, diff)
+	detailedDiff, changes := makeDetailedDiff(ctx, schema, fields, olds, news, diff)
 
 	// There are some providers/situations which `makeDetailedDiff` distorts the expected changes, leading
 	// to changes being dropped by Pulumi.
@@ -923,8 +929,8 @@ func (p *Provider) Diff(ctx context.Context, req *pulumirpc.DiffRequest) (*pulum
 	// For all properties that are ForceNew, but didn't change, assume they are stable.  Also recognize
 	// overlays that have requested that we treat specific properties as stable.
 	var stables []string
-	res.TF.Schema().Range(func(k string, sch shim.Schema) bool {
-		name, _, cust := getInfoFromTerraformName(k, res.TF.Schema(), res.Schema.Fields, false)
+	schema.Range(func(k string, sch shim.Schema) bool {
+		name, _, cust := getInfoFromTerraformName(k, schema, fields, false)
 		if !replaced[string(name)] &&
 			(sch.ForceNew() || (cust != nil && cust.Stable != nil && *cust.Stable)) {
 			stables = append(stables, string(name))
@@ -933,7 +939,7 @@ 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))
+		(res.Schema.DeleteBeforeReplace || nameRequiresDeleteBeforeReplace(news, olds, 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
@@ -951,7 +957,7 @@ func (p *Provider) Diff(ctx context.Context, req *pulumirpc.DiffRequest) (*pulum
 	}
 
 	if changes == pulumirpc.DiffResponse_DIFF_NONE &&
-		markWronglyTypedMaxItemsOneStateDiff(res.TF.Schema(), res.Schema.Fields, olds) {
+		markWronglyTypedMaxItemsOneStateDiff(schema, fields, olds) {
 
 		changes = pulumirpc.DiffResponse_DIFF_SOME
 	}
@@ -996,7 +1002,7 @@ func (p *Provider) Create(ctx context.Context, req *pulumirpc.CreateRequest) (*p
 		return nil, errors.Wrapf(err, "preparing %s's new property state", urn)
 	}
 
-	diff, err := p.tf.Diff(ctx, res.TFName, nil, config)
+	diff, err := p.tf.Diff(ctx, res.TFName, nil, config, shim.DiffOptions{})
 	if err != nil {
 		return nil, errors.Wrapf(err, "diffing %s", urn)
 	}
@@ -1213,12 +1219,18 @@ func (p *Provider) Update(ctx context.Context, req *pulumirpc.UpdateRequest) (*p
 	if err != nil {
 		return nil, err
 	}
-	config, assets, err := MakeTerraformConfig(ctx, p, news, res.TF.Schema(), res.Schema.Fields)
+
+	schema, fields := res.TF.Schema(), res.Schema.Fields
+
+	config, assets, err := MakeTerraformConfig(ctx, p, news, schema, fields)
 	if err != nil {
 		return nil, errors.Wrapf(err, "preparing %s's new property state", urn)
 	}
 
-	diff, err := p.tf.Diff(ctx, res.TFName, state, config)
+	ic := newIgnoreChanges(ctx, schema, fields, olds, news, req.GetIgnoreChanges())
+	diff, err := p.tf.Diff(ctx, res.TFName, state, config, shim.DiffOptions{
+		IgnoreChanges: ic,
+	})
 	if err != nil {
 		return nil, errors.Wrapf(err, "diffing %s", urn)
 	}
@@ -1230,10 +1242,6 @@ func (p *Provider) Update(ctx context.Context, req *pulumirpc.UpdateRequest) (*p
 		return &pulumirpc.UpdateResponse{Properties: req.GetOlds()}, nil
 	}
 
-	// Apply any ignoreChanges before we check that the diff doesn't require replacement or deletion since we may be
-	// ignoring changes to the keys that would result in replacement/deletion.
-	doIgnoreChanges(ctx, res.TF.Schema(), res.Schema.Fields, olds, news, req.GetIgnoreChanges(), diff)
-
 	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.",

pkg/tfbridge/schema_test.go

@@ -689,7 +689,7 @@ func TestMetaProperties(t *testing.T) {
 			ok = clearID(state)
 			assert.True(t, ok)
 			cfg := prov.NewResourceConfig(ctx, map[string]interface{}{})
-			diff, err := prov.Diff(ctx, resName, state, cfg)
+			diff, err := prov.Diff(ctx, resName, state, cfg, shim.DiffOptions{})
 			assert.NoError(t, err)
 
 			// To populate default timeouts, we take the timeouts from the resource schema and insert them into the diff
@@ -765,7 +765,7 @@ func TestInjectingCustomTimeouts(t *testing.T) {
 			ok = clearID(state)
 			assert.True(t, ok)
 			cfg := prov.NewResourceConfig(ctx, map[string]interface{}{})
-			diff, err := prov.Diff(ctx, resName, state, cfg)
+			diff, err := prov.Diff(ctx, resName, state, cfg, shim.DiffOptions{})
 			assert.NoError(t, err)
 
 			// To populate default timeouts, we take the timeouts from the resource schema and insert them into the diff

pkg/tfshim/schema/provider.go

@@ -64,7 +64,7 @@ func (ProviderShim) Configure(
 }
 
 func (ProviderShim) Diff(
-	ctx context.Context, t string, s shim.InstanceState, c shim.ResourceConfig,
+	ctx context.Context, t string, s shim.InstanceState, c shim.ResourceConfig, opts shim.DiffOptions,
 ) (shim.InstanceDiff, error) {
 	panic("this provider is schema-only and does not support runtime operations")
 }

pkg/tfshim/sdk-v1/instance_diff.go

@@ -77,12 +77,13 @@ func (d v1InstanceDiff) RequiresNew() bool {
 	return d.tf.RequiresNew()
 }
 
-func (d v1InstanceDiff) IgnoreChanges(ignored map[string]bool) {
+func (d v1InstanceDiff) processIgnoreChanges(ignored shim.IgnoreChanges) {
+	i := ignored()
 	for k := range d.tf.Attributes {
-		if ignored[k] {
+		if _, ok := i[k]; ok {
 			delete(d.tf.Attributes, k)
 		} else {
-			for attr := range ignored {
+			for attr := range i {
 				if strings.HasPrefix(k, attr+".") {
 					delete(d.tf.Attributes, k)
 					break

pkg/tfshim/sdk-v1/provider.go

@@ -87,14 +87,21 @@ func (p v1Provider) Configure(_ context.Context, c shim.ResourceConfig) error {
 }
 
 func (p v1Provider) Diff(
-	_ context.Context, t string, s shim.InstanceState, c shim.ResourceConfig,
+	_ context.Context, t string, s shim.InstanceState, c shim.ResourceConfig, opts shim.DiffOptions,
 ) (shim.InstanceDiff, error) {
 	if c == nil {
 		return diffToShim(&terraform.InstanceDiff{Destroy: true}), nil
 	}
 
 	diff, err := p.tf.SimpleDiff(instanceInfo(t), stateFromShim(s), configFromShim(c))
-	return diffToShim(diff), err
+
+	d := diffToShim(diff)
+
+	if dd, ok := d.(v1InstanceDiff); ok && opts.IgnoreChanges != nil {
+		dd.processIgnoreChanges(opts.IgnoreChanges)
+	}
+
+	return d, err
 }
 
 func (p v1Provider) Apply(

pkg/tfshim/sdk-v2/instance_diff.go

@@ -70,12 +70,13 @@ func (d v2InstanceDiff) RequiresNew() bool {
 	return d.tf.RequiresNew()
 }
 
-func (d v2InstanceDiff) IgnoreChanges(ignored map[string]bool) {
+func (d v2InstanceDiff) processIgnoreChanges(ignored shim.IgnoreChanges) {
+	i := ignored()
 	for k := range d.tf.Attributes {
-		if ignored[k] {
+		if _, ok := i[k]; ok {
 			delete(d.tf.Attributes, k)
 		} else {
-			for attr := range ignored {
+			for attr := range i {
 				if strings.HasPrefix(k, attr+".") {
 					delete(d.tf.Attributes, k)
 					break