Skip to content

Commit

Permalink
Fix unexplained detailed diffs (#1696)
Browse files Browse the repository at this point in the history 
Fixes pulumi/pulumi-aws#3439

makeDetailedDiff does not report collection changes. It instead hopes
that elements of the collection will produce diff entries. When changes
involve collections all the way down and there are no element entries,
makeDetailedDiff produces no records.

However we have some workarounds now that produce a DIFF_SOME even when
there are no entries per
#1501

```
	if p.info.XSkipDetailedDiffForChanges && len(diff.Attributes()) > 0 {
		changes = pulumirpc.DiffResponse_DIFF_SOME
```

The change in this PR embellishes this case with collection-level diffs
so there is at least something to show.
  • Loading branch information
@t0yv0
t0yv0 authored Feb 23, 2024
1 parent 3425c55 commit afedecf
Show file tree
Hide file tree
Showing 3 changed files with 186 additions and 16 deletions.
74 changes: 60 additions & 14 deletions pkg/tfbridge/diff.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,23 +156,42 @@ func visitPropertyValue(
}
}

func makePropertyDiff(ctx context.Context, name, path string, v resource.PropertyValue, tfDiff shim.InstanceDiff,
diff map[string]*pulumirpc.PropertyDiff, forceDiff *bool,
tfs shim.Schema, ps *SchemaInfo, finalize, rawNames bool) {
func makePropertyDiff(
ctx context.Context,
name, path string,
v resource.PropertyValue,
tfDiff shim.InstanceDiff,
diff map[string]*pulumirpc.PropertyDiff, // makePropertyDiff populates this output map
collectionDiffs map[string]*pulumirpc.PropertyDiff, // optional collection-level diffs
forceDiff *bool,
tfs shim.Schema,
ps *SchemaInfo,
finalize, rawNames bool,
) {

visitor := func(name, path string, v resource.PropertyValue) bool {
switch {
case v.IsArray():
// If this value has a diff and is considered computed by Terraform, the diff will be woefully incomplete. In
// this case, do not recurse into the array; instead, just use the count diff for the details.
if d := tfDiff.Attribute(name + ".#"); d == nil || !d.NewComputed {
if d != nil && d.Old != d.New {
collectionDiffs[path] = &pulumirpc.PropertyDiff{
Kind: pulumirpc.PropertyDiff_UPDATE,
}
}
return true
}
name += ".#"
case v.IsObject():
// If this value has a diff and is considered computed by Terraform, the diff will be woefully incomplete. In
// this case, do not recurse into the array; instead, just use the count diff for the details.
if d := tfDiff.Attribute(name + ".%"); d == nil || !d.NewComputed {
if d != nil && d.Old != d.New {
collectionDiffs[path] = &pulumirpc.PropertyDiff{
Kind: pulumirpc.PropertyDiff_UPDATE,
}
}
return true
}
name += ".%"
Expand Down Expand Up @@ -286,7 +305,8 @@ func computeIgnoreChanges(
return ignoredKeySet
}

// makeDetailedDiff converts the given state (olds), config (news), and InstanceDiff to a Pulumi property diff.
// makeDetailedDiff converts the given state (olds), config (news), and InstanceDiff to a Pulumi
// property diff.
//
// See makePropertyDiff for more details.
func makeDetailedDiff(
Expand All @@ -296,37 +316,63 @@ func makeDetailedDiff(
olds, news resource.PropertyMap,
tfDiff shim.InstanceDiff,
) (map[string]*pulumirpc.PropertyDiff, pulumirpc.DiffResponse_DiffChanges) {
dd := makeDetailedDiffExtra(ctx, tfs, ps, olds, news, tfDiff)
return dd.diffs, dd.changes
}

type detailedDiffExtra struct {
changes pulumirpc.DiffResponse_DiffChanges
diffs map[string]*pulumirpc.PropertyDiff
collectionDiffs map[string]*pulumirpc.PropertyDiff
}

func makeDetailedDiffExtra(
ctx context.Context,
tfs shim.SchemaMap,
ps map[string]*SchemaInfo,
olds, news resource.PropertyMap,
tfDiff shim.InstanceDiff,
) detailedDiffExtra {
if tfDiff == nil {
return map[string]*pulumirpc.PropertyDiff{}, pulumirpc.DiffResponse_DIFF_NONE
return detailedDiffExtra{changes: pulumirpc.DiffResponse_DIFF_NONE}
}

// Check both the old state and the new config for diffs and report them as necessary.
//
// There is a minor complication here: Terraform has no concept of an "add" diff. Instead, adds are recorded as
// updates with an old property value of the empty string. In order to detect adds--and to ensure that all diffs
// in the InstanceDiff are reflected in the resulting Pulumi property diff--we first call this function with
// each property in a resource's state, then with each property in its config. Any diffs that only appear in the
// config are treated as adds; diffs that appear in both the state and config are treated as updates.
// There is a minor complication here: Terraform has no concept of an "add" diff. Instead,
// adds are recorded as updates with an old property value of the empty string. In order to
// detect adds--and to ensure that all diffs in the InstanceDiff are reflected in the
// resulting Pulumi property diff--we first call this function with each property in a
// resource's state, then with each property in its config. Any diffs that only appear in
// the config are treated as adds; diffs that appear in both the state and config are
// treated as updates.

forceDiff := new(bool)
diff := map[string]*pulumirpc.PropertyDiff{}
collectionDiffs := map[string]*pulumirpc.PropertyDiff{}
for k, v := range olds {
en, etf, eps := getInfoFromPulumiName(k, tfs, ps, false)
makePropertyDiff(ctx, en, string(k), v, tfDiff, diff, forceDiff, etf, eps, false, shimutil.IsOfTypeMap(etf))
makePropertyDiff(ctx, en, string(k), v, tfDiff, diff, collectionDiffs, forceDiff,
etf, eps, false, shimutil.IsOfTypeMap(etf))
}
for k, v := range news {
en, etf, eps := getInfoFromPulumiName(k, tfs, ps, false)
makePropertyDiff(ctx, en, string(k), v, tfDiff, diff, forceDiff, etf, eps, false, shimutil.IsOfTypeMap(etf))
makePropertyDiff(ctx, en, string(k), v, tfDiff, diff, collectionDiffs, forceDiff,
etf, eps, false, shimutil.IsOfTypeMap(etf))
}
for k, v := range olds {
en, etf, eps := getInfoFromPulumiName(k, tfs, ps, false)
makePropertyDiff(ctx, en, string(k), v, tfDiff, diff, forceDiff, etf, eps, true, shimutil.IsOfTypeMap(etf))
makePropertyDiff(ctx, en, string(k), v, tfDiff, diff, collectionDiffs, forceDiff,
etf, eps, true, shimutil.IsOfTypeMap(etf))
}

changes := pulumirpc.DiffResponse_DIFF_NONE
if len(diff) > 0 || *forceDiff {
changes = pulumirpc.DiffResponse_DIFF_SOME
}
return diff, changes
return detailedDiffExtra{
changes: changes,
diffs: diff,
collectionDiffs: collectionDiffs,
}
}
121 changes: 120 additions & 1 deletion pkg/tfbridge/diff_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,14 @@ import (
"fmt"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
v2Schema "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/pulumi/pulumi/sdk/v3/go/common/resource"
"github.com/pulumi/pulumi/sdk/v3/go/common/resource/plugin"
pulumirpc "github.com/pulumi/pulumi/sdk/v3/proto/go"
"github.com/stretchr/testify/assert"

shim "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim"
shimv1 "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim/sdk-v1"
Expand Down Expand Up @@ -2066,3 +2069,119 @@ func TestListNestedAddMaxItemsOne(t *testing.T) {
},
pulumirpc.DiffResponse_DIFF_SOME)
}

type diffTestCase struct {
resourceSchema map[string]*schema.Schema
resourceFields map[string]*SchemaInfo
state resource.PropertyMap
inputs resource.PropertyMap
expected map[string]*pulumirpc.PropertyDiff
expectedDiffChanges pulumirpc.DiffResponse_DiffChanges
ignoreChanges []string
XSkipDetailedDiffForChanges bool
}

func diffTest2(t *testing.T, tc diffTestCase) {
ctx := context.Background()
res := &schema.Resource{
Schema: tc.resourceSchema,
}
provider := shimv1.NewProvider(&schema.Provider{
ResourcesMap: map[string]*schema.Resource{
"p_resource": res,
},
})
state, err := plugin.MarshalProperties(tc.state, plugin.MarshalOptions{})
require.NoError(t, err)

inputs, err := plugin.MarshalProperties(tc.inputs, plugin.MarshalOptions{})
require.NoError(t, err)

p := Provider{
tf: provider,
info: ProviderInfo{
XSkipDetailedDiffForChanges: tc.XSkipDetailedDiffForChanges,
Resources: map[string]*ResourceInfo{
"p_resource": {
Tok: "pkg:index:PResource",
Fields: tc.resourceFields,
},
},
},
}

p.initResourceMaps()

resp, err := p.Diff(ctx, &pulumirpc.DiffRequest{
Id: "myResource",
Urn: "urn:pulumi:test::test::pkg:index:PResource::n1",
Olds: state,
News: inputs,
IgnoreChanges: tc.ignoreChanges,
})
require.NoError(t, err)

assert.Equal(t, tc.expectedDiffChanges, resp.Changes)
require.Equal(t, tc.expected, resp.DetailedDiff)
}

func TestChangingMaxItems1FilterProperty(t *testing.T) {
schema := map[string]*schema.Schema{
"rule": {
Type: schema.TypeList,
Required: true,
MaxItems: 1000,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"filter": {
Type: schema.TypeList,
Optional: true,
MaxItems: 1,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"prefix": {
Type: schema.TypeString,
Optional: true,
},
},
},
},
},
},
},
}
diffTest2(t, diffTestCase{
XSkipDetailedDiffForChanges: true,
resourceSchema: schema,
state: resource.PropertyMap{
"rules": resource.NewArrayProperty(
[]resource.PropertyValue{
resource.NewObjectProperty(
resource.PropertyMap{
"filter": resource.NewNullProperty(),
},
),
},
),
},
inputs: resource.PropertyMap{
"rules": resource.NewArrayProperty([]resource.PropertyValue{
resource.NewObjectProperty(resource.PropertyMap{
"filter": resource.NewObjectProperty(
resource.PropertyMap{
"__defaults": resource.NewArrayProperty(
[]resource.PropertyValue{},
),
},
),
}),
}),
},
expectedDiffChanges: pulumirpc.DiffResponse_DIFF_SOME,
expected: map[string]*pulumirpc.PropertyDiff{
"rules[0].filter": {
Kind: pulumirpc.PropertyDiff_UPDATE,
},
},
})
}
7 changes: 6 additions & 1 deletion pkg/tfbridge/provider.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -895,7 +895,8 @@ func (p *Provider) Diff(ctx context.Context, req *pulumirpc.DiffRequest) (*pulum
return nil, errors.Wrapf(err, "diffing %s", urn)
}

detailedDiff, changes := makeDetailedDiff(ctx, schema, fields, olds, news, diff)
dd := makeDetailedDiffExtra(ctx, schema, fields, olds, news, diff)
detailedDiff, changes := dd.diffs, dd.changes

// There are some providers/situations which `makeDetailedDiff` distorts the expected changes, leading
// to changes being dropped by Pulumi.
Expand All @@ -906,6 +907,10 @@ func (p *Provider) Diff(ctx context.Context, req *pulumirpc.DiffRequest) (*pulum
// See also https://github.com/pulumi/pulumi-terraform-bridge/issues/1501.
if p.info.XSkipDetailedDiffForChanges && len(diff.Attributes()) > 0 {
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 {
detailedDiff[path] = diff
}
}

// If there were changes in this diff, check to see if we have a replacement.
Expand Down

0 comments on commit afedecf

Please sign in to comment.