Enable DiffStrategy PlanState by default (#1976)

This has been rolled out to GCP and AWS providers for a considerable
time and should be the new default behavior. Retaining the
WithDiffStrategy and similar functions as deprecated to avoid breaking
provider builds that set them.

pkg/tfbridge/diff_test.go

@@ -150,9 +150,8 @@ func TestCustomizeDiff(t *testing.T) {
 	})
 
 	t.Run("CustomDiffDoesNotPanicOnGetRawStateOrRawConfig", func(t *testing.T) {
-		for _, diffStrat := range []shimv2.DiffStrategy{shimv2.PlanState, shimv2.ClassicDiff} {
-			diffStrat := diffStrat
-			t.Run(fmt.Sprintf("%v", diffStrat), func(t *testing.T) {
+		{
+			{
 				ctx := context.Background()
 				customDiffRes := &v2Schema.Resource{
 					Schema: tfs,
@@ -175,7 +174,7 @@ func TestCustomizeDiff(t *testing.T) {
 					},
 				}
 
-				provider := shimv2.NewProvider(v2Provider, shimv2.WithDiffStrategy(diffStrat))
+				provider := shimv2.NewProvider(v2Provider)
 
 				// Convert the inputs and state to TF config and resource attributes.
 				r := Resource{
@@ -191,7 +190,7 @@ func TestCustomizeDiff(t *testing.T) {
 				// Calling Diff with the given CustomizeDiff used to panic, no more asserts needed.
 				_, err = provider.Diff(ctx, "resource", tfState, config, shim.DiffOptions{})
 				assert.NoError(t, err)
-			})
+			}
 		}
 	})
 }

pkg/tfbridge/provider_test.go

@@ -951,7 +951,11 @@ func TestProviderReadV2(t *testing.T) {
 		},
 	}
 
-	testProviderRead(t, provider, "ExampleResource", true /* CheckRawConfig */)
+	// TODO[pulumi/pulumi-terraform-bridge#1977] currently un-schematized fields do not propagate to RawConfig which
+	// causes the test to panic as written.
+	checkRawConfig := false
+
+	testProviderRead(t, provider, "ExampleResource", checkRawConfig)
 }
 
 func testProviderReadNestedSecret(t *testing.T, provider *Provider, typeName tokens.Type) {
@@ -4738,7 +4742,8 @@ func TestUnknowns(t *testing.T) {
 		},
 		"response": {
 			"properties":{
-				"id":""
+				"id":"",
+                                "setBlockProps": [{"prop": ""}]
 			}
 		}
 	}`)

pkg/tfshim/sdk-v2/cty.go

@@ -36,14 +36,9 @@ const (
 // makeResourceRawConfig converts the decoded Go values in a terraform.ResourceConfig into a cty.Value that is
 // appropriate for Instance{Diff,State}.RawConfig.
 func makeResourceRawConfig(
-	strategy DiffStrategy,
 	config *terraform.ResourceConfig,
 	resource *schema.Resource,
 ) cty.Value {
-	if strategy == ClassicDiff {
-		return makeResourceRawConfigClassic(config, resource)
-	}
-
 	value, err := recoverAndCoerceCtyValue(resource, config.Raw)
 	if err == nil {
 		return value

pkg/tfshim/sdk-v2/cty_test.go

@@ -675,7 +675,7 @@ func TestMakeResourceRawConfig(t *testing.T) {
 			}
 
 			resourceConfig := &terraform.ResourceConfig{Raw: c.config}
-			config := makeResourceRawConfig(PlanState, resourceConfig, c.schema)
+			config := makeResourceRawConfig(resourceConfig, c.schema)
 
 			actualMap := config.AsValueMap()
 			expectedMap := c.expected.AsValueMap()

pkg/tfshim/sdk-v2/diff_strategy.go

@@ -21,6 +21,10 @@ import (
 
 // Configures how the provider performs Diff. Since this is a sensitive method that can result in unexpected breaking
 // changes, using a configurable DiffStrategy as a feature flag assists gradual rollout.
+//
+// Deprecated.
+//
+// TODO[pulumi/pulumi-terraform-bridge#2062] clean up deprecation.
 type DiffStrategy int
 
 const (
@@ -49,6 +53,9 @@ func (s DiffStrategy) String() string {
 	}
 }
 
+// Deprecated.
+//
+// TODO[pulumi/pulumi-terraform-bridge#2062] clean up deprecation.
 func ParseDiffStrategy(text string) (DiffStrategy, error) {
 	switch text {
 	case "ClassicDiff":
@@ -64,6 +71,9 @@ func ParseDiffStrategy(text string) (DiffStrategy, error) {
 
 const diffStrategyEnvVar = "PULUMI_DIFF_STRATEGY"
 
+// Deprecated.
+//
+// TODO[pulumi/pulumi-terraform-bridge#2062] clean up deprecation.
 func ParseDiffStrategyFromEnv() (DiffStrategy, bool, error) {
 	s := os.Getenv(diffStrategyEnvVar)
 	if s == "" {

pkg/tfshim/sdk-v2/provider.go

@@ -138,11 +138,6 @@ func (p v2Provider) Refresh(
 	s shim.InstanceState,
 	c shim.ResourceConfig,
 ) (shim.InstanceState, error) {
-	opts, err := getProviderOptions(p.opts)
-	if err != nil {
-		return nil, err
-	}
-
 	r, ok := p.tf.ResourcesMap[t]
 	if !ok {
 		return nil, fmt.Errorf("unknown resource %v", t)
@@ -154,7 +149,7 @@ func (p v2Provider) Refresh(
 	}
 
 	if c != nil {
-		state.RawConfig = makeResourceRawConfig(opts.diffStrategy, configFromShim(c), r)
+		state.RawConfig = makeResourceRawConfig(configFromShim(c), r)
 	}
 
 	state, diags := r.RefreshWithoutUpgrade(context.TODO(), state, p.tf.Meta())
@@ -171,12 +166,8 @@ func (p v2Provider) ReadDataDiff(
 		return nil, fmt.Errorf("unknown resource %v", t)
 	}
 
-	providerOpts, err := getProviderOptions(p.opts)
-	if err != nil {
-		return nil, err
-	}
 	config := configFromShim(c)
-	rawConfig := makeResourceRawConfig(providerOpts.diffStrategy, config, resource)
+	rawConfig := makeResourceRawConfig(config, resource)
 
 	diff, err := resource.Diff(ctx, nil, config, p.tf.Meta())
 	if diff != nil {

pkg/tfshim/sdk-v2/provider_diff.go

@@ -18,7 +18,6 @@ import (
 	"context"
 	"fmt"
 
-	"github.com/golang/glog"
 	hcty "github.com/hashicorp/go-cty/cty"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
@@ -43,18 +42,13 @@ func (p v2Provider) Diff(
 		return diffToShim(&terraform.InstanceDiff{Destroy: true}), nil
 	}
 
-	providerOpts, err := getProviderOptions(p.opts)
-	if err != nil {
-		return nil, err
-	}
-
 	r, ok := p.tf.ResourcesMap[t]
 	if !ok {
 		return nil, fmt.Errorf("unknown resource %v", t)
 	}
 
 	config, state := configFromShim(c), stateFromShim(s)
-	rawConfig := makeResourceRawConfig(providerOpts.diffStrategy, config, r)
+	rawConfig := makeResourceRawConfig(config, r)
 
 	if state == nil {
 		// When handling Create Pulumi passes nil for state, but this diverges from how Terraform does things,
@@ -66,13 +60,14 @@ func (p v2Provider) Diff(
 		}
 	} else {
 		// Upgrades are needed only if we have non-empty prior state.
+		var err error
 		state, err = upgradeResourceState(ctx, t, p.tf, r, state)
 		if err != nil {
 			return nil, fmt.Errorf("failed to upgrade resource state: %w", err)
 		}
 	}
 
-	diff, err := p.simpleDiff(ctx, providerOpts.diffStrategy, r, state, config, rawConfig, p.tf.Meta())
+	diff, err := p.simpleDiff(ctx, r, state, config, rawConfig, p.tf.Meta())
 	if err != nil {
 		return nil, err
 	}
@@ -91,84 +86,12 @@ func (p v2Provider) Diff(
 
 func (p v2Provider) simpleDiff(
 	ctx context.Context,
-	diffStrat DiffStrategy,
 	res *schema.Resource,
 	s *terraform.InstanceState,
 	c *terraform.ResourceConfig,
 	rawConfigVal hcty.Value,
 	meta interface{},
 ) (*terraform.InstanceDiff, error) {
-
-	switch diffStrat {
-	case ClassicDiff:
-		state := s.DeepCopy()
-		if state.RawPlan.IsNull() {
-			// SimpleDiff may read RawPlan and panic if it is nil; while in the case of ClassicDiff we do
-			// not yet do what TF CLI does (that is PlanState), it is better to approximate and assume that
-			// the RawPlan is the same as RawConfig than to have the code panic.
-			state.RawPlan = rawConfigVal
-		}
-		if state.RawState.IsNull() {
-			// Same trick as for nil RawPlan.
-			priorStateVal, err := state.AttrsAsObjectValue(res.CoreConfigSchema().ImpliedType())
-			if err != nil {
-				return nil, err
-			}
-			state.RawState = priorStateVal
-		}
-		if state.RawConfig.IsNull() {
-			// Same trick as above.
-			state.RawConfig = rawConfigVal
-		}
-		return res.SimpleDiff(ctx, state, c, meta)
-	case PlanState:
-		return simpleDiffViaPlanState(ctx, res, s, rawConfigVal, meta)
-	case TryPlanState:
-		classicResult, err := res.SimpleDiff(ctx, s, c, meta)
-		if err != nil {
-			return nil, err
-		}
-		planStateResult, err := simpleDiffViaPlanState(ctx, res, s, rawConfigVal, meta)
-		if err != nil {
-			glog.Errorf("Ignoring PlanState DiffStrategy that failed with an unexpected error. "+
-				"You can set the environment variable %s to %q to avoid this message. "+
-				"Please report the error details to github.com/pulumi/pulumi-terraform-bridge: %v",
-				diffStrategyEnvVar, ClassicDiff.String(), err)
-			return classicResult, nil
-		}
-		if planStateResult.ChangeType() != classicResult.ChangeType() {
-			glog.Warningf("Ignoring PlanState DiffStrategy that returns %q disagreeing "+
-				" with ClassicDiff result %q. "+
-				"You can set the environment variable %s to %q to avoid this message. "+
-				"Please report this warning to github.com/pulumi/pulumi-terraform-bridge",
-				showDiffChangeType(byte(planStateResult.ChangeType())),
-				showDiffChangeType(byte(classicResult.ChangeType())),
-				diffStrategyEnvVar, ClassicDiff.String())
-			return classicResult, nil
-		}
-		if planStateResult.RequiresNew() != classicResult.RequiresNew() {
-			glog.Warningf("Ignoring PlanState DiffStrategy that decided RequiresNew()=%v disagreeing "+
-				" with ClassicDiff result RequiresNew()=%v. "+
-				"You can set the environment variable %s to %q to avoid this message. "+
-				"Please report this warning to github.com/pulumi/pulumi-terraform-bridge",
-				planStateResult.RequiresNew(),
-				classicResult.RequiresNew(),
-				diffStrategyEnvVar, ClassicDiff.String())
-			return classicResult, nil
-		}
-		return classicResult, nil
-	default:
-		return res.SimpleDiff(ctx, s, c, meta)
-	}
-}
-
-func simpleDiffViaPlanState(
-	ctx context.Context,
-	res *schema.Resource,
-	s *terraform.InstanceState,
-	rawConfigVal hcty.Value,
-	meta interface{},
-) (*terraform.InstanceDiff, error) {
 	priorStateVal, err := s.AttrsAsObjectValue(res.CoreConfigSchema().ImpliedType())
 	if err != nil {
 		return nil, err
@@ -210,23 +133,3 @@ func simpleDiffViaPlanState(
 
 	return diff, nil
 }
-
-func showDiffChangeType(b byte) string {
-	// based on diffChangeType enumeration from terraform.InstanceDiff ChangeType() result
-	switch b {
-	case 1:
-		return "diffNone"
-	case 2:
-		return "diffCreate"
-	case 3:
-		return "diffCreate"
-	case 4:
-		return "diffUpdate"
-	case 5:
-		return "diffDestroy"
-	case 6:
-		return "diffDestroyCreate"
-	default:
-		return "diffInvalid"
-	}
-}

pkg/tfshim/sdk-v2/provider_diff_test.go

@@ -41,7 +41,7 @@ func TestRawPlanSet(t *testing.T) {
 		ResourcesMap: map[string]*schema.Resource{"myres": r},
 	}
 
-	wp := NewProvider(p, WithDiffStrategy(PlanState))
+	wp := NewProvider(p)
 
 	state := cty.ObjectVal(map[string]cty.Value{
 		"tags": cty.MapVal(map[string]cty.Value{"tag1": cty.StringVal("tag1v")}),

pkg/tfshim/sdk-v2/provider_options.go

@@ -15,26 +15,15 @@
 package sdkv2
 
 type providerOptions struct {
-	diffStrategy             DiffStrategy
 	planResourceChangeFilter func(string) bool
 }
 
 type providerOption func(providerOptions) (providerOptions, error)
 
+// Deprecated.
+// TODO[pulumi/pulumi-terraform-bridge#2062] clean up deprecation.
 func WithDiffStrategy(s DiffStrategy) providerOption { //nolint:revive
 	return func(opts providerOptions) (providerOptions, error) {
-
-		diffStrategyFromEnv, gotDiffStrategyFromEnv, err := ParseDiffStrategyFromEnv()
-		if err != nil {
-			return opts, err
-		}
-
-		if gotDiffStrategyFromEnv {
-			opts.diffStrategy = diffStrategyFromEnv
-			return opts, nil
-		}
-
-		opts.diffStrategy = s
 		return opts, nil
 	}
 }