PF wrong diff for set of objects

[VenelinMartinov]

What happened?

During pulumi/pulumi-meraki#97 it was discovered that the pf bridge does not produce a diff when an element is added to set of objects.

Seems like this happens only if all of the properties of the object are Computed + Optional

Example

#2168

Output of pulumi about

.

Additional context

No response

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

[VenelinMartinov]

Removing the Computed from one of the attributes of the object prevents the issue:

"vlan_names": schema.SetNestedAttribute{
		MarkdownDescription: `An array of named VLANs`,
		Optional:            true,
		NestedObject: schema.NestedAttributeObject{
			Attributes: map[string]schema.Attribute{
				"name": schema.StringAttribute{
					MarkdownDescription: `Name of the VLAN, string length must be from 1 to 32 characters`,
					// Computed:            true,
					Optional:            true,
				},
				"vlan_id": schema.StringAttribute{
					MarkdownDescription: `VLAN ID`,
					Computed:            true,
					Optional:            true,
				},
			},
		},
	},

[VenelinMartinov]

We seem to get the wrong value from the TF plan in

pulumi-terraform-bridge/pf/tfbridge/provider_diff.go

Line 81 in 18db7c5

planResp, err := p.plan(ctx, rh.terraformResourceName, rh.schema, priorState, checkedInputsValue)

However this does seem to repro in TF, so they are likely doing something different with the plan parameters.

[VenelinMartinov]

Possibly related: #703 - this specifically calls out nested Optional + Computed as a likely issue

[VenelinMartinov]

We recently updated our ProposedNew copy in SDKV2: #2060

Could be that the PF re-implementation is based on the old TF version

[VenelinMartinov]

Seems like we didn't re-use that as it works off the cty types instead of the pf tftypes and instead opted to re-implement it on our side.

[VenelinMartinov]

Some relevant-looking changes upstream, maybe related: opentofu/opentofu@d49e991

a few others too https://github.com/opentofu/opentofu/commits/main/internal/plans/objchange

We have not picked up any of these fixes - I wonder if re-using the upstream code and adding a translation layer will be better here - it'd make picking up usptream fixes much easier like we do in SDKV2

[VenelinMartinov]

Verified the issue indeed happens during the joining of the prior state and new config here:

pulumi-terraform-bridge/pf/internal/pfutils/proposed_new.go

Line 105 in 18db7c5

joined, err := Join(joinOpts, tftypes.NewAttributePath(), &priorState, &config)

Join incorrectly returns the old state of the set with a single element for the test above.

[VenelinMartinov]

Think I might have found the culprit:

pulumi-terraform-bridge/pf/internal/pfutils/proposed_new.go

Lines 64 to 67 in 18db7c5

	joinOpts := JoinOptions{
	SetElementEqual: NonComputedEq(schema),
	}

We have an override for Set equality which replaces all Computed attributes with nulls - this prob means that if a set contains only entirely computed elements, they'd get wrongly equated.

Indeed commenting out that line seems to resolve the issue - now on to find why it is there in the first place.

[VenelinMartinov]

I've opened #2172 as a follow up for potentially updating our adaptation of objchange.

Also opened #2171 for investigating support for PlanModifiers as it doesn't look as if these get used anywhere. This might have been the original cause of the above change for set equality.

[t0yv0]

There's a chance it might be addressed by #2188 but will need to confirm explicitly

[VenelinMartinov]

I don't think #2188 addresses this - the reason this is happening is #2170 (comment)

We consider elements with all properties computed (+optional) equal, no matter their values.

[t0yv0]

Can you elaborate? 2188 literally deletes the code you reference in that comment.

[VenelinMartinov]

Okay, fair, didn't realise that - good chance it fixes it then!