makeDetailedDiff distorts the plan incorrectly deciding whether there are changes or not

[t0yv0]

What happened?

It appears that there are significant corner cases where makeDetailedDiff incorrectly decides pulumirpc.DiffResponse_DiffChanges, where incorrectly means that it does not agree with the underlying upstream provider, on whether there are changes (DiffResponse_DIFF_NONE) or there are no changes (DiffResponse_DIFF_SOME).

Example

• Incorrect tags applied when tag is empty pulumi-aws#2895 highlighted a case where adding an empty tag causes "tags" property makes this function decide there are no changes, and consequently Pulumi to not do an update, see Fix adding empty values to maps #1498 for example

• similarly with GCP 7.0 tfbridge/diff.go does not consider terraform diff #1491 has issues when provider-level labels are applied but are ignored; Terraform-level InstanceDiff object sees them but Pulumi ignores them.

Output of pulumi about

N/A

Additional context

We have been trying to reconstruct why this is happening and the current best guess is this block of code:

	// 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.

	forceDiff := new(bool)
	diff := 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, useRawNames(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, useRawNames(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, useRawNames(etf))
	}

The intention here is to walk Pulumi olds and news values, and translate Pulumi paths to TF diff paths in the process, checking if there is a diff or not. This has worked historically for diffs that originate from the user changing the inputs in a program. HOWEVER, bridged providers allow a degree of flexibility to provider authors to inject diff customizer functions that edit the results of a diff and can suppress or introduce diffs at will. When these changes are happening over values that are not present in Pulumi, the algorithm fails to take them into account.

  sequenceDiagram
      participant Program
      participant Bridge
      participant TFProvider
      Program->>Bridge: Diff(olds={tags={}},news={tags={}})
      Bridge->>TFProvider: SimpleDiff(tr(olds), tr(news))
      TFProvider->>Bridge: terraform.ResourceAttrDiff with "tags.n" added
      Bridge->>Bridge:     makeDetailedDiff traverses olds.tags, news.tags, did not see "tags.n"
      Bridge->>Program:    NO CHANGES (incorrect)

Loading

Contributing

Vote on this issue by adding a 👍 reaction.
To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

[t0yv0]

What if we introduced a change, under a flag, to always respect TF's diff result in what we emit to Pulumi, moving makeDetailedDiff function to be purely concerned with matters of presentation of the diff's details (which are part of UX but no e2e correctness). And as follow up we'd refactor makeDetailedDiff to be accurate in all circumstances (under heavy test).

[t0yv0]

I believe #1491 conclusively fixed it but remains under flag. Once we roll out to AWS and GCP and wait a cycle we can probably remove the flag and adopt the new behavior and close this bug. Open to other rollout plans here.

[t0yv0]

I'm reopening per recent PR discussion - we had some situations around this and got called out by customers therefore I think more is needed here (possibly #1895 ). The initial "fix" which is really a workaround introduced situations where Pulumi is making a change but the detailed diff does not explain it, which was not acceptable. This led to #1696 to provide an answer in pulumi/pulumi-aws#3439 - this works in the happy case but is not a comprehensive fix.

[t0yv0]

Found a reference on how OpenTOFU decides what kind of plan to undertake based on the PlanResourceChange response from a provider:

https://github.com/opentofu/opentofu/blob/main/internal/tofu/node_resource_abstract_instance.go#L1019

This can likely inform our canonical implementation, with some caveats for pulumi-level features such as precise secrets.

[VenelinMartinov]

Hit this in pulumi-azure rolling out PRC: pulumi/pulumi-azure#2322 (comment)

[VenelinMartinov]

fixed in #2379 and #2405 and is now behind a feature flag in the bridge.