Consider rewriting makeDetailedDiff

[t0yv0]

Hello!

• Vote on this issue by adding a 👍 reaction
• If you want to implement this feature, comment to let us know (we'll work with you on design, scheduling, etc.)

Issue details

Consider rewriting makeDetailedDiff logic that populates detailedDiff.

Desired properties

• when provider returns DIFF_SOME, it must also return a detailedDiff explaining why the changes are happening
• when provider returns DIFF_NONE, it must return no detailedDiff contradicting that
• when the user specifies an ignoreChanges option, changes are suppressed and detailedDiff reflects that correctly

Why the current implementation cannot deliver the desired properties

This is elaborated in #1501 - essentially the current
implementation is trying to inspect a difficult to work with InstanceDiff object by doing 3 passes over news and olds
projection to TF. This works for the happy case but it does not faithfully hit the keys that may be changed by the TF
planning phase if they are not present in the union of Pulumi olds and news, and it creates confusion for changes
involving paths to set elements that are projected to Pulumi.

How can a new implementation look like

The suggested replacement is to compute the diff directly on Pulumi level (comparing PropertyValue representations of
resource state). Ideally this would take the exact same logic that Pulumi CLI uses to diff two PropertyValue instances
when the CLI is in charge of the diff completely.

What makes bridged providers special is that there is the underground phase of doing PlanResourceChange together with TF
that cannot be cleanly exposed to the engine in the current provider protocol. As a result bridged providers partially
take over the responsibility for the diffs and in particular detailed diffs and ignore changes.

This can still be made to work in the following way:

1. as is already done, compute planned state (via PlanResourceChange); this phase should continue taking care of
   ignoreChanges at a semantic level

2. translate this planned state back to the PropertyValue domain

3. elaborate the difference between old state PropertyValue and the planned state PropertyValue at Pulumi level without
   any further reference to the underlying TF provider schema

This implementation will avoid working with InstanceDiff object, work entirely at Pulumi level and be WISYWIG-defensible at Pulumi level, and support ignoreChanges.

Downsides

Changs involving sets will still involve confusing reordering of list elements in the Pulumi projection. I believe there is no universally good way currently to surface set diffs without making changes to Pulumi CLI diff display to cooperate a little bit better when displaying these changes, although partial solutions may be possible - heuristics that improve a lot of common cases.

Issues

#1501
#186
#1245
#752
#1867
#1756

Affected area/feature

[VenelinMartinov]

Note that a prerequisite here is rolling out PlanResourceChange - the proposed implementation will only take effect for resources under that

That could be an upside too - any changes here would already be flagged behind PlanResourceChange.

related: #1785

---

Linking in the plugin-sdk patch which we added in order to support PlanResourceChange: pulumi/terraform-plugin-sdk#35 - that was partly motivated by the way we do DetaileDiff in the bridge

---

We might also want to put in a bit of work in #1867 to be more confident in any changes we ship to detailedDiff - otherwise we'd likely be stuck with an unending resource-by-resource rollout.

[VenelinMartinov]

@t0yv0 is this a duplicate of #1504? Either way, some useful context there too.

[t0yv0]

Actually indeed, looks like this one I wrote a bit later so it has my latest understanding but the ask is the same as in #1504. We should be able to write a much better detailedDiff now.

[VenelinMartinov]

Note that the proposal here is not only for detailed diff - the same algorithm influences if we return DIFF_SOME or DIFF_NONE to the engine, so the name is a bit misleading.

pulumi-terraform-bridge/pkg/tfbridge/diff.go

Lines 379 to 382 in ddb54e8

	changes := pulumirpc.DiffResponse_DIFF_NONE
	if len(diff) > 0 || *forceDiff {
	changes = pulumirpc.DiffResponse_DIFF_SOME
	}

[t0yv0]

It shouldn't influence result, indeed, once we're done here.