From 012ac655189047bcd62bc4b7f25104d8f072da0d Mon Sep 17 00:00:00 2001 From: Anton Tayanovskyy Date: Mon, 6 May 2024 16:05:47 -0400 Subject: [PATCH] Fix overeager diffs (#1927) With AWS 3880 there is some evidence (derivation in https://github.com/pulumi/pulumi-terraform-bridge/pull/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 (https://github.com/pulumi/pulumi-terraform-bridge/issues/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 --- pkg/tfbridge/provider.go | 2 +- pkg/tfbridge/tests/provider_test.go | 131 ++++++++++++++++++++++++++ pkg/tfshim/sdk-v1/instance_diff.go | 4 + pkg/tfshim/sdk-v2/instance_diff.go | 4 + pkg/tfshim/sdk-v2/provider2.go | 11 +++ pkg/tfshim/shim.go | 2 +- pkg/tfshim/tfplugin5/instance_diff.go | 4 + pkg/tfshim/tfplugin5/provider_test.go | 2 +- 8 files changed, 157 insertions(+), 3 deletions(-) diff --git a/pkg/tfbridge/provider.go b/pkg/tfbridge/provider.go index 5144271e3..3922b1b6c 100644 --- a/pkg/tfbridge/provider.go +++ b/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 { diff --git a/pkg/tfbridge/tests/provider_test.go b/pkg/tfbridge/tests/provider_test.go index c7296ed85..02c1cf0a1 100644 --- a/pkg/tfbridge/tests/provider_test.go +++ b/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, diff --git a/pkg/tfshim/sdk-v1/instance_diff.go b/pkg/tfshim/sdk-v1/instance_diff.go index d347a1dc7..2badd8c27 100644 --- a/pkg/tfshim/sdk-v1/instance_diff.go +++ b/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 { diff --git a/pkg/tfshim/sdk-v2/instance_diff.go b/pkg/tfshim/sdk-v2/instance_diff.go index aaac5c00f..c12ad306a 100644 --- a/pkg/tfshim/sdk-v2/instance_diff.go +++ b/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 { diff --git a/pkg/tfshim/sdk-v2/provider2.go b/pkg/tfshim/sdk-v2/provider2.go index e4428670e..627eeb8bc 100644 --- a/pkg/tfshim/sdk-v2/provider2.go +++ b/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 { diff --git a/pkg/tfshim/shim.go b/pkg/tfshim/shim.go index 38b7175b5..23b3f4161 100644 --- a/pkg/tfshim/shim.go +++ b/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 diff --git a/pkg/tfshim/tfplugin5/instance_diff.go b/pkg/tfshim/tfplugin5/instance_diff.go index 75f6d206c..c8848f6cb 100644 --- a/pkg/tfshim/tfplugin5/instance_diff.go +++ b/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 { diff --git a/pkg/tfshim/tfplugin5/provider_test.go b/pkg/tfshim/tfplugin5/provider_test.go index 06adfc438..0865bf506 100644 --- a/pkg/tfshim/tfplugin5/provider_test.go +++ b/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 }