Refresh reorders set elements causing spurious diffs

t0yv0 commented 3 months ago

What happened?

When servicing pulumi refresh the provider only gets to influence the result in a Read call, but the diff will be computed outside of the provider by the Pulumi CLI and the engine within; the engine does not understand sets and therefore deals with a list projection. The inputs are ordered arbitrarily according to program order, but the state read from the cloud might be (should be?) ordered by TF-canonical order (using Set function that gives an identity to each element). This makes Pulumi display spurious reordering diffs the are not visible in TF.

What we could do here:

protocol change that lets bridged provider participate in computing diffs for refresh; make that set-aware
without a protocol change we can do a pass over Read results that attempts to reorder set element projection to match the order from inputs as much as possible, that is intentionally minimize the spurious diffs within the current model; this is doable quickly I suggest we stat here

Example

A good example for Plugin Framework based resources is provided in https://github.com/pulumi/pulumi-aws/issues/3303

I'm searching for more examples for SDKv2 based resources.

Output of `pulumi about`

See above.

Additional context

N/A

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

iwahbe commented 3 months ago

Thanks for raising the issue. We will prioritize next iteration.

t0yv0 commented 3 months ago

Slightly more involved proposal that could also fix for this: https://github.com/pulumi/pulumi/issues/16072 - in this proposal the state out of Read would be threaded through the program again, so the usual Diff machinery would kick in that handles sets already.

t0yv0 commented 2 months ago

This might change significantly with https://github.com/pulumi/pulumi/pull/16146 that plumbs refresh results through Diff, though I've not verifed.

t0yv0 commented 2 months ago

@mikhailshilkov reminds of an issue https://github.com/pulumi/pulumi-gcp/issues/1935 affecting import (not just refresh). Possibly same fix if Read could canonicalize the ordering of sets.

guineveresaenger commented 1 month ago

https://github.com/pulumi/pulumi-gcp/issues/1935 was fixed via https://github.com/pulumi/pulumi/pull/16024.

t0yv0 commented 1 month ago

I'm not sure I agree with https://github.com/pulumi/pulumi/pull/16024 being a fix for this. It's a workaround for Providers returning inconsistent data. Consider an example where the set re-order is coupled with another independent actual change that makes the diff result DIFF_SOME. Then 16024 will not apply and 1935 will be right back.

t0yv0 commented 1 week ago

We need to retry it in light of changed CLI behavior in https://github.com/pulumi/pulumi/pull/16146

CLI should engage Diff now which may suppress reordering issues.

t0yv0 commented 6 days ago

Doing some more research around this. Current theory is this:

the provider behaves correctly for most scenarios
we need to fix detailed diff display for sets (https://github.com/pulumi/pulumi-terraform-bridge/issues/2235 and #186)
during pulumi refresh ExtractInputsFromOutputs is particularly liable to reorder to-be-read inputs, which breaks index-based ignoreChanges implemented in the engine, but if we implement https://github.com/pulumi/pulumi-terraform-bridge/issues/2240 we will warn the user about this

Not sure if it is feasible to adjust ExtractInputsFromOutputs pass to try to align the ordering of elements to that present in the inputs, if it would help ignoreChanges fire up correctly. Possibly not worth doing even if feasible.

pulumi / pulumi-terraform-bridge