Fix makeDetailedDiff inaccuracies

[t0yv0]

What happened?

Consider rewriting makeDetailedDiff for SDKv2 providers.

The objective of makeDetailedDiff is to extract useful information and expose it in the response to
the Diff method so that Pulumi CLI can surface better results to the user:

https://github.com/pulumi/pulumi/blob/master/proto/pulumi/provider.proto#L262

It can surface which paths have changed, as well as the nature of the change:

https://github.com/pulumi/pulumi/blob/master/proto/pulumi/provider.proto#L211

Since TF providers modify changes internally, and have the best information on which changes require
replacements, skipping makeDetailedDiff degrades the usability. The engine in Pulumi CLI will still
attempt to compute diffs but will fail to expose the accurate information only known to the
TF-bridged provider.

There are a few known issues with the current implementation.

1. It claims that terraform.InstanceDiff cannot distinguish betwen add, delete and replace plans,
   and applies inaccurate heuristics in
   https://github.com/pulumi/pulumi-terraform-bridge/blob/master/pkg/tfbridge/diff.go#L289

   This is not entirely accurate since the information is present, but difficult to correlate:
   https://github.com/hashicorp/terraform-plugin-sdk/blob/28e631776d97f0a5a5942b3524814addbef90875/terraform/diff.go#L38C1-L38C1

   InstanceDiff.Attribute uses "flatmap-style" keys that are difficult to interpret. Besides
   InstanceDiff, the provier has out of band information available on the prior state of the
   resource's Pulumi inputs and outputs, as well as the proposed new set of Pulumi inputs and
   outputs. If path correlation could be made to work correctly, the caller could be able to
   interpret any given change as Add/Update/Delete correctly.

2. It is based on traversing Pulumi-level values which misses changes introduced by TF entirely.
   Changes may be coming from diff customizers, one example is applying default GCP labels or
   default tags in AWS. These changes introduce new paths that are currently skipped by
   makeDetailedDiff.

Example

Some back-history:

• makeDetailedDiff distorts the plan incorrectly deciding whether there are changes or not #1501
• Refer to Terraform diff to generate a more precise diff #1499 attempt to fix

Output of pulumi about

N/A

Additional context

v3.63.2 affected.

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]

On the implementation side, one challenge is dealing with 'flatmap-style' keys. It is worth doing a search for supported functionality we can reuse, I was able to locate these:

• RequiresReplace
• FlatmapKeyFromPath

Unfortunately https://github.com/hashicorp/terraform-plugin-sdk/blob/main/internal/configs/hcl2shim/paths.go#L253 comment does not inspire confidence that these methods handle sets at all (or correctly) so this may cause difficulties.

[t0yv0]

Note that translating cty.Path style paths to Pulumi paths is error prone as well unless one is not aware of the gotchas with path shortening with MaxItems=1.

We have some relatively well tested code such as FromCtyPath but unfortunately it works with the simpler structure called SchemaPath; this structure is adequate for locating schema entries but elides value path information. This is not sufficient to drill down PropertyValue. Critically, when drilling down a set element, the TF path will include an IndexStep with an actual value. EG from cty path.go:

// IndexStep is a Step implementation representing applying the index operation
// to a value, which must be of either a list, map, or set type.
//
// When describing a path through a *type* rather than a concrete value,
// the Key may be an unknown value, indicating that the step applies to
// *any* key of the given type.
//
// When indexing into a set, the Key is actually the element being accessed
// itself, since in sets elements are their own identity.
type IndexStep struct {
	pathStepImpl
	Key Value
}

Elsewhere in TF these are represented as hashes. It is therefore an additional somewhat tricky problem to correlate changes on the TF set values to their corresponding paths in Pulumi list projections.

[t0yv0]

My latest impression is that we can just avoid doing this alltogether by rewriting makeDetailedDiff from scratch with #1895 - it's a bigger change but the end result may be easier to verify and justify.

[pulumi-bot]

This issue has been addressed in PR #2405 and shipped in release v3.92.0.