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

Add a fallback for detailed diff replace decisions to ensure detailed diff is presentation-only #2757

Merged
merged 7 commits into from
Dec 17, 2024
5 changes: 4 additions & 1 deletion pkg/pf/tests/diff_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,10 @@ func TestDetailedDiffStringAttribute(t *testing.T) {
res := pb.NewResource(pb.NewResourceArgs{
ResourceSchema: schema.schema,
})
diff := crosstests.Diff(t, res, map[string]cty.Value{"key": initialValue}, map[string]cty.Value{"key": changeValue})
diff := crosstests.Diff(
t, res, map[string]cty.Value{"key": initialValue}, map[string]cty.Value{"key": changeValue},
crosstests.DisableAccurateBridgePreview(),
)

autogold.ExpectFile(t, testOutput{
initialValue: scenario.initialValue,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,9 @@ Plan: 0 to add, 1 to change, 0 to destroy.
~ testprovider:index/test:Test: (update)
[id=test-id]
[urn=urn:pulumi:test::project::testprovider:index/test:Test::p]
~ key: "default" => "value"
+ key: "value"
Resources:
~ 1 to update
1 unchanged
`,
detailedDiff: map[string]interface{}{"key": map[string]interface{}{"kind": "UPDATE"}},
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,5 +28,4 @@ Resources:
~ 1 to update
1 unchanged
`,
detailedDiff: map[string]interface{}{"key": map[string]interface{}{"kind": "UPDATE"}},
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,4 @@ Resources:
~ 1 to update
1 unchanged
`,
detailedDiff: map[string]interface{}{"key": map[string]interface{}{"kind": "UPDATE"}},
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@ Plan: 1 to add, 0 to change, 1 to destroy.
+-testprovider:index/test:Test: (replace)
[id=test-id]
[urn=urn:pulumi:test::project::testprovider:index/test:Test::p]
~ id : "test-id" => output<string>
~ key: "default" => "value"
Resources:
+-1 to replace
1 unchanged
`,
detailedDiff: map[string]interface{}{"key": map[string]interface{}{"kind": "UPDATE"}},
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,10 @@ Plan: 1 to add, 0 to change, 1 to destroy.
+-testprovider:index/test:Test: (replace)
[id=test-id]
[urn=urn:pulumi:test::project::testprovider:index/test:Test::p]
~ id : "test-id" => output<string>
~ key: "value" => "value1"
Resources:
+-1 to replace
1 unchanged
`,
detailedDiff: map[string]interface{}{"key": map[string]interface{}{"kind": "UPDATE"}},
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@ Plan: 1 to add, 0 to change, 1 to destroy.
+-testprovider:index/test:Test: (replace)
[id=test-id]
[urn=urn:pulumi:test::project::testprovider:index/test:Test::p]
- key: "value"
~ id : "test-id" => output<string>
~ key: "value" => "default"
Resources:
+-1 to replace
1 unchanged
`,
detailedDiff: map[string]interface{}{"key": map[string]interface{}{"kind": "UPDATE"}},
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,4 @@ Resources:
~ 1 to update
1 unchanged
`,
detailedDiff: map[string]interface{}{"key": map[string]interface{}{}},
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,5 +28,4 @@ Resources:
~ 1 to update
1 unchanged
`,
detailedDiff: map[string]interface{}{"key": map[string]interface{}{"kind": "UPDATE"}},
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,4 @@ Resources:
~ 1 to update
1 unchanged
`,
detailedDiff: map[string]interface{}{"key": map[string]interface{}{"kind": "DELETE"}},
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,9 @@ Plan: 0 to add, 1 to change, 0 to destroy.
~ testprovider:index/test:Test: (update)
[id=test-id]
[urn=urn:pulumi:test::project::testprovider:index/test:Test::p]
~ key: "default" => "value"
+ key: "value"
Resources:
~ 1 to update
1 unchanged
`,
detailedDiff: map[string]interface{}{"key": map[string]interface{}{"kind": "UPDATE"}},
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,5 +28,4 @@ Resources:
~ 1 to update
1 unchanged
`,
detailedDiff: map[string]interface{}{"key": map[string]interface{}{"kind": "UPDATE"}},
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@ Plan: 1 to add, 0 to change, 1 to destroy.
+-testprovider:index/test:Test: (replace)
[id=test-id]
[urn=urn:pulumi:test::project::testprovider:index/test:Test::p]
~ id : "test-id" => output<string>
~ key: "default" => "value"
Resources:
+-1 to replace
1 unchanged
`,
detailedDiff: map[string]interface{}{"key": map[string]interface{}{"kind": "UPDATE"}},
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,10 @@ Plan: 1 to add, 0 to change, 1 to destroy.
+-testprovider:index/test:Test: (replace)
[id=test-id]
[urn=urn:pulumi:test::project::testprovider:index/test:Test::p]
~ id : "test-id" => output<string>
~ key: "value" => "value1"
Resources:
+-1 to replace
1 unchanged
`,
detailedDiff: map[string]interface{}{"key": map[string]interface{}{"kind": "UPDATE"}},
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@ Plan: 1 to add, 0 to change, 1 to destroy.
+-testprovider:index/test:Test: (replace)
[id=test-id]
[urn=urn:pulumi:test::project::testprovider:index/test:Test::p]
~ id : "test-id" => output<string>
+ key: "value"
Resources:
+-1 to replace
1 unchanged
`,
detailedDiff: map[string]interface{}{"key": map[string]interface{}{}},
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,10 @@ Plan: 1 to add, 0 to change, 1 to destroy.
+-testprovider:index/test:Test: (replace)
[id=test-id]
[urn=urn:pulumi:test::project::testprovider:index/test:Test::p]
~ id : "test-id" => output<string>
~ key: "value" => "value1"
Resources:
+-1 to replace
1 unchanged
`,
detailedDiff: map[string]interface{}{"key": map[string]interface{}{"kind": "UPDATE"}},
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@ Plan: 1 to add, 0 to change, 1 to destroy.
+-testprovider:index/test:Test: (replace)
[id=test-id]
[urn=urn:pulumi:test::project::testprovider:index/test:Test::p]
~ id : "test-id" => output<string>
- key: "value"
Resources:
+-1 to replace
1 unchanged
`,
detailedDiff: map[string]interface{}{"key": map[string]interface{}{"kind": "DELETE"}},
}
7 changes: 5 additions & 2 deletions pkg/pf/tfbridge/provider_diff.go
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,9 @@ func (p *provider) DiffWithContext(
}

if providerOpts.enableAccurateBridgePreview {
pluginDetailedDiff, err := calculateDetailedDiff(ctx, &rh, priorState, plannedStateValue, checkedInputs)
replaceOverride := len(replaceKeys) > 0
pluginDetailedDiff, err := calculateDetailedDiff(
ctx, &rh, priorState, plannedStateValue, checkedInputs, &replaceOverride)
if err != nil {
return plugin.DiffResult{}, err
}
Expand All @@ -148,7 +150,7 @@ func (p *provider) DiffWithContext(

func calculateDetailedDiff(
ctx context.Context, rh *resourceHandle, priorState *upgradedResourceState,
plannedStateValue tftypes.Value, checkedInputs resource.PropertyMap,
plannedStateValue tftypes.Value, checkedInputs resource.PropertyMap, replaceOverride *bool,
) (map[string]plugin.PropertyDiff, error) {
priorProps, err := convert.DecodePropertyMap(ctx, rh.decoder, priorState.state.Value)
if err != nil {
Expand All @@ -167,6 +169,7 @@ func calculateDetailedDiff(
priorProps,
props,
checkedInputs,
replaceOverride,
)

pluginDetailedDiff := make(map[string]plugin.PropertyDiff, len(detailedDiff))
Expand Down
8 changes: 4 additions & 4 deletions pkg/tests/detailed_diff_unknown_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -900,7 +900,7 @@ func TestUnknownCollectionForceNewDetailedDiff(t *testing.T) {
runTest(t, program2, autogold.Expect(`
+ prov:index/aux:Aux: (create)
[urn=urn:pulumi:test::test::prov:index/aux:Aux::auxRes]
+-prov:index/test:Test: (replace)
~ prov:index/test:Test: (update)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that TF is optimistic about replacing ForceNew properties with unknown and we were being pessimistic. This change ensures we match TF and ForceNew proeprties replaced with unknowns are now predicted NOT to cause a replacement.

We can add a workaround if we disagree here.

[id=newid]
[urn=urn:pulumi:test::test::prov:index/test:Test::mainRes]
~ tests: [
Expand All @@ -917,7 +917,7 @@ func TestUnknownCollectionForceNewDetailedDiff(t *testing.T) {
runTest(t, program2, autogold.Expect(`
+ prov:index/aux:Aux: (create)
[urn=urn:pulumi:test::test::prov:index/aux:Aux::auxRes]
+-prov:index/test:Test: (replace)
~ prov:index/test:Test: (update)
[id=newid]
[urn=urn:pulumi:test::test::prov:index/test:Test::mainRes]
- tests: [
Expand Down Expand Up @@ -1026,7 +1026,7 @@ func TestUnknownCollectionForceNewDetailedDiff(t *testing.T) {
runTest(t, program2, autogold.Expect(`
+ prov:index/aux:Aux: (create)
[urn=urn:pulumi:test::test::prov:index/aux:Aux::auxRes]
+-prov:index/test:Test: (replace)
~ prov:index/test:Test: (update)
[id=newid]
[urn=urn:pulumi:test::test::prov:index/test:Test::mainRes]
~ tests: [
Expand All @@ -1043,7 +1043,7 @@ func TestUnknownCollectionForceNewDetailedDiff(t *testing.T) {
runTest(t, program2, autogold.Expect(`
+ prov:index/aux:Aux: (create)
[urn=urn:pulumi:test::test::prov:index/aux:Aux::auxRes]
+-prov:index/test:Test: (replace)
~ prov:index/test:Test: (update)
[id=newid]
[urn=urn:pulumi:test::test::prov:index/test:Test::mainRes]
- tests: [
Expand Down
58 changes: 56 additions & 2 deletions pkg/tfbridge/detailed_diff.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,18 @@ func isTypeShapeMismatched(val resource.PropertyValue, propType shim.ValueType)
}
}

func containsReplace(m map[string]*pulumirpc.PropertyDiff) bool {
for _, v := range m {
switch v.GetKind() {
case pulumirpc.PropertyDiff_UPDATE_REPLACE,
pulumirpc.PropertyDiff_ADD_REPLACE,
pulumirpc.PropertyDiff_DELETE_REPLACE:
return true
}
}
return false
}

func promoteToReplace(diff *pulumirpc.PropertyDiff) *pulumirpc.PropertyDiff {
if diff == nil {
return nil
Expand All @@ -75,6 +87,23 @@ func promoteToReplace(diff *pulumirpc.PropertyDiff) *pulumirpc.PropertyDiff {
}
}

func demoteToNoReplace(diff *pulumirpc.PropertyDiff) *pulumirpc.PropertyDiff {
if diff == nil {
return nil
}
kind := diff.GetKind()
switch kind {
case pulumirpc.PropertyDiff_ADD_REPLACE:
return &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_ADD}
case pulumirpc.PropertyDiff_DELETE_REPLACE:
return &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_DELETE}
case pulumirpc.PropertyDiff_UPDATE_REPLACE:
return &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_UPDATE}
default:
return diff
}
}

type baseDiff string

const (
Expand Down Expand Up @@ -477,11 +506,17 @@ func (differ detailedDiffer) makeDetailedDiffPropertyMap(

// MakeDetailedDiffV2 is the main entry point for calculating the detailed diff.
// This is an internal function that should not be used outside of the pulumi-terraform-bridge.
//
// The `replaceOverride` parameter is used to override the replace behavior of the detailed diff.
// If true, the diff will be overridden to return a replace.
// If false, the diff will be overridden to not return a replace.
// If nil, the detailed diff will be returned as is.
func MakeDetailedDiffV2(
ctx context.Context,
tfs shim.SchemaMap,
ps map[string]*SchemaInfo,
priorProps, props, newInputs resource.PropertyMap,
replaceOverride *bool,
) map[string]*pulumirpc.PropertyDiff {
// Strip secrets and outputs from the properties before calculating the diff.
// This allows the rest of the algorithm to focus on the actual changes and not
Expand All @@ -497,7 +532,25 @@ func MakeDetailedDiffV2(
props = stripSecretsAndOutputs(props)
newInputs = stripSecretsAndOutputs(newInputs)
differ := detailedDiffer{ctx: ctx, tfs: tfs, ps: ps, newInputs: newInputs}
return differ.makeDetailedDiffPropertyMap(priorProps, props)
res := differ.makeDetailedDiffPropertyMap(priorProps, props)

if replaceOverride != nil {
if *replaceOverride {
// We need to make sure there is a replace.
if !containsReplace(res) {
// We use the internal __meta property to trigger a replace when we have failed to
// determine the correct detailed diff for it.
res[metaKey] = &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_UPDATE_REPLACE}
}
} else {
// There is an override for no replaces, so ensure we don't have any.
for k, v := range res {
res[k] = demoteToNoReplace(v)
}
}
}

return res
}

func makeDetailedDiffV2(
Expand All @@ -511,6 +564,7 @@ func makeDetailedDiffV2(
assets AssetTable,
supportsSecrets bool,
newInputs resource.PropertyMap,
replaceOverride *bool,
) (map[string]*pulumirpc.PropertyDiff, error) {
// We need to compare the new and olds after all transformations have been applied.
// ex. state upgrades, implementation-specific normalizations etc.
Expand All @@ -532,5 +586,5 @@ func makeDetailedDiffV2(
return nil, err
}

return MakeDetailedDiffV2(ctx, tfs, ps, priorProps, props, newInputs), nil
return MakeDetailedDiffV2(ctx, tfs, ps, priorProps, props, newInputs, replaceOverride), nil
}
Loading
Loading