Detailed diff does not mark fully computed properties for recomputation when the resource is replaced

[VenelinMartinov]

Fully computed properties (like ID) do not get marked as unknown in previews which suggest a replace. This leads to an incorrect preview as the value will most likely change.

@Frassle also noted that the diff output is used for update plans, so this is not purely presentational either. It is also likely that the #2672 affects previews where there is another resource which depends on this fully computed property.

Example:

If we transition from: properties: {} to properties: {key: "value"} for the PF schema

rschema.Schema{
	Attributes: map[string]rschema.Attribute{
		"key": rschema.StringAttribute{
			Optional:      true,
			PlanModifiers: []planmodifier.String{stringplanmodifier.RequiresReplace()},
		},
		"id": rschema.StringAttribute{
			Computed: true,
			PlanModifiers: []planmodifier.String{
				stringplanmodifier.UseStateForUnknown(),
			},
		},
	},
}

We get:

	pulumiOut: `Previewing update (test):
  pulumi:pulumi:Stack: (same)
    [urn=urn:pulumi:test::project::pulumi:pulumi:Stack::project-test]
    +-testprovider:index/test:Test: (replace)
        [id=test-id]
        [urn=urn:pulumi:test::project::testprovider:index/test:Test::p]
      + key: "value"
Resources:
    +-1 to replace
    1 unchanged

versus

  # testprovider_test.res must be replaced
+/- resource "testprovider_test" "res" {
      ~ id  = "test-id" -> (known after apply)
      + key = "value" # forces replacement
    }

Notice that we do not flag the ID property as changing but TF does. We should as well, since if the resource is replaced, all computed properties will get re-computed.

Note that a similar issue arises in SDKv2 where this is a pre-existing bug.

See TestDetailedDiffStringAttribute/replace/added

[t0yv0]

Can we have a fully worked example here to help understand impact? Thanks!

[VenelinMartinov]

I've added an example to the description, thanks for flagging that up.

[VenelinMartinov]

Looked up what Opentofu does here: https://github.com/opentofu/opentofu/blob/864aa9d1d629090cfc4ddf9fdd344d34dee9793e/internal/tofu/node_resource_abstract_instance.go#L1054

Looks like they re-plan the change with a nil prior if they detect a replace in order to re-computed computed properties. I'll attempt something similar for Diff.

Wondering if this also affects Update - should Update re-plan the output once we detect a replace. Opened #2672 to follow up on what happens in Update

[t0yv0]

Can the example include the before and after program indicating which state transition this is in?

[t0yv0]

From the information so far it appears that this is a cosmetic opportunity for improvement but not necessarily a deal breaker (e.g. Pulumi correctly indicates the replace) and the only issue is that Pulumi is a bit overconfident about these computed attributes which might in fact change. Does that sound about right?

[VenelinMartinov]

Yeah, it is also a regression in PF as it previously did detect these changes and displayed them to the user.

Is it possible that this also affects correctness? What if another resource depends on this property and we do not mark it as possibly Unknown? Would the dependent resource always get updated too, even if we indicate that the property it depends on does not change? I opened #2672 to potentially follow-up on that

Do you think we should backlog this?

[VenelinMartinov]

Discussed offline. We decided this is indeed a blocker as it affects the preview of all Computed properties and they have regressed compared to not using detailed diff v2.

It is also a pre-existing bug in SDKv2, so affects quite a few resources.

[VenelinMartinov]

Attempted to solve this the same way that opentofu does, by re-planning the value with a nil prior after detecting a replace. Unfortunately, we seem to be running into engine - provider protocol limitations:

For the example above and the following detailed diff:

detailedDiff: map[string]interface{}{
	"id":  map[string]interface{}{"kind": "UPDATE"},
	"key": map[string]interface{}{"kind": "ADD"},
}

we get the following preview:

	pulumiOut: `Previewing update (test):
  pulumi:pulumi:Stack: (same)
    [urn=urn:pulumi:test::project::pulumi:pulumi:Stack::project-test]
    +-testprovider:index/test:Test: (replace)
        [id=test-id]
        [urn=urn:pulumi:test::project::testprovider:index/test:Test::p]
      - id : "test-id"
      + key: "value"
Resources:
    +-1 to replace
    1 unchanged

which is not correct. Notice that id is marked for deletion instead of update to <unknown>.

I believe this is because the engine is using the values from old state and new inputs for showing the detailed diff. This is wrong because the new inputs have no values for fully computed properties, so we get a delete instead of an Update to UNKNOWN.

---

An interesting question here is how does the engine display this correctly when no detailed diff is returned from Diff. Perhaps it is using the output of Update(preview=true), so the question becomes why does it not use that when a detailed diff is returned from Diff.

EDIT: Found something in the step_generator which might be responsible for marking computed values as unknown: https://github.com/pulumi/pulumi/blob/8314293d6e9901c747b0d397eff9a4a0c0e00466/pkg/resource/deploy/step_generator.go#L2120

That iterates, however, on the inputs, not outputs, so might not be the right thing which does this.

[VenelinMartinov]

Found some indication in the engine that this might have been considered before:

https://github.com/pulumi/pulumi/blob/8314293d6e9901c747b0d397eff9a4a0c0e00466/pkg/resource/deploy/step_generator.go#L1218-L1221

The engine re-runs Check if Diff indicates a resource is going to be replaced.

[VenelinMartinov]

Here is a branch where we do this re-planning: #2673 It shows the effect on the previews and the wrong display of deletions of computed resources.

[VenelinMartinov]

This is blocked on #2281 or something equivalent which gives the engine access to the planned outputs. Alternatively, there might be a solution where the engine works around the issue and marks Output-only properties as unknown on replace, like it does when no detailed diff is returned from Diff