Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

StateFunc applied twice for resources under PRC #2408

Closed
flostadler opened this issue Sep 9, 2024 · 5 comments · Fixed by #2407
Closed

StateFunc applied twice for resources under PRC #2408

flostadler opened this issue Sep 9, 2024 · 5 comments · Fixed by #2407
Assignees
Labels
kind/bug Some behavior is incorrect or out of spec resolution/fixed This issue was fixed

Comments

@flostadler
Copy link
Contributor

What happened?

Resources under PRC have the StateFunc applied twice to their resources.
This leads to mangled data in state and perma diffs.

ApplyResourceChange applies the StateFunc, but the preceding Diff also applies it.
Often this is fine because StateFunc can be applied multiple times (e.g. normalizing json). But for some operations like calculating a hash it's not.

This causes: pulumi/pulumi-aws#4446

Example

Output of pulumi about

n/a

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

@flostadler flostadler added kind/bug Some behavior is incorrect or out of spec needs-triage Needs attention from the triage team labels Sep 9, 2024
@t0yv0
Copy link
Member

t0yv0 commented Sep 9, 2024

That's unfortunate. Does TF not apply it twice e.g not once in PlanResourceChange and once in ApplyResourceChange? Or is it that Pulumi specifically recomputes the plan and that is the problem?

@flostadler
Copy link
Contributor Author

flostadler commented Sep 10, 2024

This seems to be caused by not setting NewExtra in the ResourceAttrDiff under PRC in the diff function. Without PRC this is set to the original value. The field readers return that value instead if it's set.

So right now under PRC the resource CRUD methods retrieve the value that's already been processed by StateFunc instead of the raw value and then process it again.

@flostadler
Copy link
Contributor Author

We're loosing NewExtra here:

diff := p.unpackDiff(ty, d)
cfg, st, pl := diff.config, state.stateValue, diff.plannedState
priv := diff.v2InstanceDiff.tf.Meta
resp, err := p.server.ApplyResourceChange(ctx, t, ty, cfg, st, pl, priv, meta)

It's set in the diff.v2InstanceDiff.tf.Attributes, but we're not passing that down to ARC. Instead we're looking for NewExtra Fields in PlannedPrivate of the request. PlannedPrivate gets constructed from diff.v2InstanceDiff.tf.Meta, but that's nil in the cases listed in the issue.

Will investigate if we're loosing diff.v2InstanceDiff.tf.Meta somewhere or PlannedPrivate needs to be set to some other value.

@flostadler
Copy link
Contributor Author

flostadler commented Sep 10, 2024

We're loosing PlannedMeta here:

plan, err := p.server.PlanResourceChange(ctx, t, ty, cfg, st, prop, priv, meta, ic)
if err != nil {
return nil, err
}
return &v2InstanceDiff2{
v2InstanceDiff: v2InstanceDiff{
tf: plan.PlannedDiff,
},
config: cfg,
plannedState: plan.PlannedState,
}, nil

PRC returns it, but we're dropping it because the type v2InstanceDiff2 doesn't include it. We'll need to fix that

@flostadler flostadler removed the needs-triage Needs attention from the triage team label Sep 10, 2024
@flostadler flostadler self-assigned this Sep 10, 2024
@pulumi-bot pulumi-bot added the resolution/fixed This issue was fixed label Sep 11, 2024
@pulumi-bot
Copy link
Contributor

This issue has been addressed in PR #2407 and shipped in release v3.91.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Some behavior is incorrect or out of spec resolution/fixed This issue was fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants