Enable DiffStrategy PlanState by default

pkg/tfbridge/diff_test.go

@@ -150,49 +150,45 @@ 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,
-					CustomizeDiff: func(_ context.Context, diff *v2Schema.ResourceDiff, _ interface{}) error {
-						rawStateType := diff.GetRawState().Type()
-						if !rawStateType.HasAttribute("outp") {
-							return fmt.Errorf("Expected rawState type to have attribute: outp")
-						}
-						rawConfigType := diff.GetRawConfig().Type()
-						if !rawConfigType.HasAttribute("outp") {
-							return fmt.Errorf("Expected rawConfig type to have attribute: outp")
-						}
-						return nil
-					},
-				}
 
-				v2Provider := &v2Schema.Provider{
-					ResourcesMap: map[string]*v2Schema.Resource{
-						"resource": customDiffRes,
-					},
+		ctx := context.Background()
+		customDiffRes := &v2Schema.Resource{
+			Schema: tfs,
+			CustomizeDiff: func(_ context.Context, diff *v2Schema.ResourceDiff, _ interface{}) error {
+				rawStateType := diff.GetRawState().Type()
+				if !rawStateType.HasAttribute("outp") {
+					return fmt.Errorf("Expected rawState type to have attribute: outp")
 				}
-
-				provider := shimv2.NewProvider(v2Provider, shimv2.WithDiffStrategy(diffStrat))
-
-				// Convert the inputs and state to TF config and resource attributes.
-				r := Resource{
-					TF:     shimv2.NewResource(customDiffRes),
-					Schema: &ResourceInfo{Fields: info},
+				rawConfigType := diff.GetRawConfig().Type()
+				if !rawConfigType.HasAttribute("outp") {
+					return fmt.Errorf("Expected rawConfig type to have attribute: outp")
 				}
-				tfState, err := MakeTerraformState(ctx, r, "id", stateMap)
-				assert.NoError(t, err)
+				return nil
+			},
+		}
+
+		v2Provider := &v2Schema.Provider{
+			ResourcesMap: map[string]*v2Schema.Resource{
+				"resource": customDiffRes,
+			},
+		}
 
-				config, _, err := MakeTerraformConfig(ctx, &Provider{tf: provider}, inputsMap, sch, info)
-				assert.NoError(t, err)
+		provider := shimv2.NewProvider(v2Provider)
 
-				// 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)
-			})
+		// Convert the inputs and state to TF config and resource attributes.
+		r := Resource{
+			TF:     shimv2.NewResource(customDiffRes),
+			Schema: &ResourceInfo{Fields: info},
 		}
+		tfState, err := MakeTerraformState(ctx, r, "id", stateMap)
+		assert.NoError(t, err)
+
+		config, _, err := MakeTerraformConfig(ctx, &Provider{tf: provider}, inputsMap, sch, info)
+		assert.NoError(t, err)
+
+		// 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,8 @@ 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.
 type DiffStrategy int
 
 const (
@@ -49,6 +51,7 @@ func (s DiffStrategy) String() string {
 	}
 }
 
+// Deprecated.
 func ParseDiffStrategy(text string) (DiffStrategy, error) {
 	switch text {
 	case "ClassicDiff":
@@ -64,6 +67,7 @@ func ParseDiffStrategy(text string) (DiffStrategy, error) {
 
 const diffStrategyEnvVar = "PULUMI_DIFF_STRATEGY"
 
+// Deprecated.
 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,14 @@
 package sdkv2
 
 type providerOptions struct {
-	diffStrategy             DiffStrategy
 	planResourceChangeFilter func(string) bool
 }
 
 type providerOption func(providerOptions) (providerOptions, error)
 
+// Deprecated.
 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
 	}
 }