From ce59e15ebc3f2680f51c7e758d1930b51353aa21 Mon Sep 17 00:00:00 2001 From: Anton Tayanovskyy Date: Wed, 15 May 2024 11:04:26 -0400 Subject: [PATCH 1/6] Enable DiffStrategy PlanState by default 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 | 70 ++++++++++----------- pkg/tfshim/sdk-v2/cty.go | 5 -- pkg/tfshim/sdk-v2/cty_test.go | 2 +- pkg/tfshim/sdk-v2/diff_strategy.go | 4 ++ pkg/tfshim/sdk-v2/provider.go | 13 +--- pkg/tfshim/sdk-v2/provider_diff.go | 83 +------------------------ pkg/tfshim/sdk-v2/provider_diff_test.go | 2 +- pkg/tfshim/sdk-v2/provider_options.go | 14 +---- 8 files changed, 45 insertions(+), 148 deletions(-) diff --git a/pkg/tfbridge/diff_test.go b/pkg/tfbridge/diff_test.go index f3e17e72e..a10254f5f 100644 --- a/pkg/tfbridge/diff_test.go +++ b/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) }) } diff --git a/pkg/tfshim/sdk-v2/cty.go b/pkg/tfshim/sdk-v2/cty.go index 1338d516a..5c88658c0 100644 --- a/pkg/tfshim/sdk-v2/cty.go +++ b/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 diff --git a/pkg/tfshim/sdk-v2/cty_test.go b/pkg/tfshim/sdk-v2/cty_test.go index fd835d5b0..ac6c99f3c 100644 --- a/pkg/tfshim/sdk-v2/cty_test.go +++ b/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() diff --git a/pkg/tfshim/sdk-v2/diff_strategy.go b/pkg/tfshim/sdk-v2/diff_strategy.go index de0985b52..7eaf7b586 100644 --- a/pkg/tfshim/sdk-v2/diff_strategy.go +++ b/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 == "" { diff --git a/pkg/tfshim/sdk-v2/provider.go b/pkg/tfshim/sdk-v2/provider.go index 3a40536cd..2eef34b72 100644 --- a/pkg/tfshim/sdk-v2/provider.go +++ b/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 { diff --git a/pkg/tfshim/sdk-v2/provider_diff.go b/pkg/tfshim/sdk-v2/provider_diff.go index 02e2f183d..00c54b4ff 100644 --- a/pkg/tfshim/sdk-v2/provider_diff.go +++ b/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 diff --git a/pkg/tfshim/sdk-v2/provider_diff_test.go b/pkg/tfshim/sdk-v2/provider_diff_test.go index 72490d10c..adc214b53 100644 --- a/pkg/tfshim/sdk-v2/provider_diff_test.go +++ b/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")}), diff --git a/pkg/tfshim/sdk-v2/provider_options.go b/pkg/tfshim/sdk-v2/provider_options.go index 5ba6229ad..76b7cbd7c 100644 --- a/pkg/tfshim/sdk-v2/provider_options.go +++ b/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 } } From cda2cfae92adc160e181930b639c0ee1bd1e497c Mon Sep 17 00:00:00 2001 From: Anton Tayanovskyy Date: Wed, 15 May 2024 11:46:21 -0400 Subject: [PATCH 2/6] Fix failing test --- pkg/tfbridge/provider_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/tfbridge/provider_test.go b/pkg/tfbridge/provider_test.go index 9e35376e6..186de8454 100644 --- a/pkg/tfbridge/provider_test.go +++ b/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) { From c12f66f2760c9dffb0681c0f6d7d8e9e0779a013 Mon Sep 17 00:00:00 2001 From: Anton Tayanovskyy Date: Wed, 15 May 2024 11:54:53 -0400 Subject: [PATCH 3/6] Pass the linter --- pkg/tfshim/sdk-v2/provider_diff.go | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/pkg/tfshim/sdk-v2/provider_diff.go b/pkg/tfshim/sdk-v2/provider_diff.go index 00c54b4ff..ca67a9b27 100644 --- a/pkg/tfshim/sdk-v2/provider_diff.go +++ b/pkg/tfshim/sdk-v2/provider_diff.go @@ -133,23 +133,3 @@ func (p v2Provider) simpleDiff( 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" - } -} From 6b8a481ab98c7256224cf37d3e41617c56059240 Mon Sep 17 00:00:00 2001 From: Anton Tayanovskyy Date: Wed, 5 Jun 2024 11:16:43 -0400 Subject: [PATCH 4/6] Accept the new test result --- pkg/tfbridge/provider_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/tfbridge/provider_test.go b/pkg/tfbridge/provider_test.go index 186de8454..e543646c0 100644 --- a/pkg/tfbridge/provider_test.go +++ b/pkg/tfbridge/provider_test.go @@ -4742,7 +4742,8 @@ func TestUnknowns(t *testing.T) { }, "response": { "properties":{ - "id":"" + "id":"", + "setBlockProps": [{"prop": ""}] } } }`) From cfc2429b333f3f3ab67e2d086cc16785f95ab154 Mon Sep 17 00:00:00 2001 From: Anton Tayanovskyy Date: Wed, 5 Jun 2024 11:31:33 -0400 Subject: [PATCH 5/6] Minimize diff --- pkg/tfbridge/diff_test.go | 69 ++++++++++++++++++++------------------- 1 file changed, 36 insertions(+), 33 deletions(-) diff --git a/pkg/tfbridge/diff_test.go b/pkg/tfbridge/diff_test.go index a10254f5f..272241a08 100644 --- a/pkg/tfbridge/diff_test.go +++ b/pkg/tfbridge/diff_test.go @@ -150,45 +150,48 @@ func TestCustomizeDiff(t *testing.T) { }) t.Run("CustomDiffDoesNotPanicOnGetRawStateOrRawConfig", 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") + { + { + 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 + }, } - return nil - }, - } - v2Provider := &v2Schema.Provider{ - ResourcesMap: map[string]*v2Schema.Resource{ - "resource": customDiffRes, - }, - } + v2Provider := &v2Schema.Provider{ + ResourcesMap: map[string]*v2Schema.Resource{ + "resource": customDiffRes, + }, + } - provider := shimv2.NewProvider(v2Provider) + provider := shimv2.NewProvider(v2Provider) - // 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) + // 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) + 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) + // 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) + } + } }) } From 840f281410a06a39218e6892470955d7e43791cc Mon Sep 17 00:00:00 2001 From: Anton Tayanovskyy Date: Wed, 5 Jun 2024 11:34:35 -0400 Subject: [PATCH 6/6] PR feedback --- pkg/tfshim/sdk-v2/diff_strategy.go | 6 ++++++ pkg/tfshim/sdk-v2/provider_options.go | 1 + 2 files changed, 7 insertions(+) diff --git a/pkg/tfshim/sdk-v2/diff_strategy.go b/pkg/tfshim/sdk-v2/diff_strategy.go index 7eaf7b586..9ed5c71d7 100644 --- a/pkg/tfshim/sdk-v2/diff_strategy.go +++ b/pkg/tfshim/sdk-v2/diff_strategy.go @@ -23,6 +23,8 @@ import ( // 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 ( @@ -52,6 +54,8 @@ 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": @@ -68,6 +72,8 @@ 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 == "" { diff --git a/pkg/tfshim/sdk-v2/provider_options.go b/pkg/tfshim/sdk-v2/provider_options.go index 76b7cbd7c..0191973aa 100644 --- a/pkg/tfshim/sdk-v2/provider_options.go +++ b/pkg/tfshim/sdk-v2/provider_options.go @@ -21,6 +21,7 @@ type providerOptions struct { 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) { return opts, nil