Consider rewriting makeDetailedDiff

t0yv0 commented 5 months ago

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 https://github.com/pulumi/pulumi-terraform-bridge/issues/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:

as is already done, compute planned state (via PlanResourceChange); this phase should continue taking care of ignoreChanges at a semantic level
translate this planned state back to the PropertyValue domain
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

https://github.com/pulumi/pulumi-terraform-bridge/issues/1501 https://github.com/pulumi/pulumi-terraform-bridge/issues/186 https://github.com/pulumi/pulumi-terraform-bridge/issues/1245 https://github.com/pulumi/pulumi-terraform-bridge/issues/752 https://github.com/pulumi/pulumi-terraform-bridge/issues/1867 https://github.com/pulumi/pulumi-terraform-bridge/issues/1756

Affected area/feature

VenelinMartinov commented 5 months ago

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.

Linking in the plugin-sdk patch which we added in order to support PlanResourceChange: https://github.com/pulumi/terraform-plugin-sdk/pull/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 https://github.com/pulumi/pulumi-terraform-bridge/issues/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 commented 5 months ago

@t0yv0 is this a duplicate of https://github.com/pulumi/pulumi-terraform-bridge/issues/1504? Either way, some useful context there too.

t0yv0 commented 5 months ago

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 commented 5 months ago

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.

https://github.com/pulumi/pulumi-terraform-bridge/blob/ddb54e8861be1257fdb34ff22986d7fe15af44d0/pkg/tfbridge/diff.go#L379-L382

t0yv0 commented 5 months ago

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

pulumi / pulumi-terraform-bridge