Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix overeager diffs #1927

Merged
merged 5 commits into from
May 6, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/tfbridge/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 len(diff.Attributes()) > 0 {
if !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 {
Expand Down
131 changes: 131 additions & 0 deletions pkg/tfbridge/tests/provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package tfbridgetests
import (
"context"
"encoding/json"
"fmt"
"io"
"testing"

Expand Down Expand Up @@ -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,
Expand Down
4 changes: 4 additions & 0 deletions pkg/tfshim/sdk-v1/instance_diff.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
4 changes: 4 additions & 0 deletions pkg/tfshim/sdk-v2/instance_diff.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
11 changes: 11 additions & 0 deletions pkg/tfshim/sdk-v2/provider2.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion pkg/tfshim/shim.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions pkg/tfshim/tfplugin5/instance_diff.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion pkg/tfshim/tfplugin5/provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down