fix: Fix panic on deposed object with precondition and nop change#38587
fix: Fix panic on deposed object with precondition and nop change#38587armsnyder wants to merge 1 commit into
Conversation
mildwonkey
left a comment
There was a problem hiding this comment.
Hi @armsnyder , thanks for submitting this, and we're sorry you ran into this bad behavior!
The change in transform_diff is good, but please update the comment for accuracy (I left a vague suggestion). Let's also skip the change readDiff; i'll be removing that shortly :)
| // results of other changes. | ||
| update = t.hasConfigConditions(addr) | ||
| // | ||
| // Condition checks only apply to the current (non-deposed) |
There was a problem hiding this comment.
A more accurate comment would be something along the lines of:
A noop deposed change occurs when the instance no longer exists, so there's no reason to process conditions.
| } | ||
|
|
||
| change := changes.GetResourceInstanceChange(addr, addrs.NotDeposed) | ||
| if change == nil { |
There was a problem hiding this comment.
Let's leave this check out, please! We're actually going to refactor and remove readDiff entirely (and it's all thanks to you reminding us that it's not really necessary now that we don't need schemas to read diffs anymore)
d9b1efc to
4aad5fc
Compare
c68d9d8 to
21800cb
Compare
|
CI is failing due to changelog entry added in recently merged #38352 |
21800cb to
a945611
Compare
|
Rebased to pick up the changelog CI fix from main |
a945611 to
3d1218c
Compare
|
I am not sure why Validate Changelog CI is failing now. I see changes were made to the CI recently in #38598 and #38599. |
Yeah that is unrelated to your PR here, but the recently merged #38607 should resolve that CI failure 👍🏻 (GitHub actions will probably need to resync once you update your branch) |
Fixes #38586
When the plan contained a no-op change for a deposed object on a resource whose configuration declared a
lifecycle.preconditionorlifecycle.postcondition,terraform applywould panic withruntime error: invalid memory address or nil pointer dereferencein(*NodeAbstractResourceInstance).readDiff.Why this happens
Two ingredients combine:
DiffTransformer(internal/terraform/transform_diff.go), for aplans.NoOpchange, callshasConfigConditions(addr):hasConfigConditionsreturns true whenever the resource block declares preconditions or postconditions. Whenupdateis true, a regularNodeApplyableResourceInstanceis created for the non-deposed address. The deposed key is never propagated to that node.The resulting apply node calls
readDiff(internal/terraform/node_resource_abstract_instance.go):which would panic on
change.Actionbecausechangeisnil.Every downstream caller (
managedResourceExecute,dataResourceExecute) already guards againstchange == nil, so the nil-check appears to have been intended.Fix
Two changes, both small and independent:
internal/terraform/transform_diff.go(root cause): in theplans.NoOparm, only callhasConfigConditionswhen the change is for the current (non-deposed) object. Preconditions and postconditions only apply to the current object; a NoOp on a deposed object never needs an update node.internal/terraform/node_resource_abstract_instance.go(defense in depth): restore the nil-check inreadDiff. The function is supposed to returnnilwhen no change is recorded — all callers already handle it — and only the trace log line was panicking.Either change alone is sufficient to prevent this specific panic (verified by reverting them one at a time and re-running the regression test). I am including both because the
DiffTransformerchange is the correct semantic fix and thereadDiffguard restores the documented contract; happy to drop the second commit if reviewers prefer the minimum change.Test
Adds
TestContext2Apply_deposedNoLongerExists_withConditionsininternal/terraform/context_apply2_test.go. It constructs an orphan-deposed-object state for acount = 0resource that has alifecycle.precondition, with the provider'sReadResourcereturning null. With both fixes reverted the test reproduces the original panic with the same stack trace (node_resource_abstract_instance.go:178→node_resource_apply_instance.go:219). With either fix in place it passes.The closely related existing test
TestContext2Plan_deposedNoLongerExists(context_plan2_test.go) covers the plan-side scenario but stops short ofApplyand uses a resource without conditions, so it does not exercise this path.Target Release
1.16.x
Rollback Plan
Changes to Security Controls
No changes to security controls.
CHANGELOG entry
Entry:
.changes/v1.16/BUG FIXES-20260514-081915.yaml.AI Usage
I worked with Claude to localize the panic, identify the two contributing code paths, and draft the regression test. I read and reasoned about every line of the diff myself. The
DiffTransformerguard and thereadDiffnil-check are both small, local changes, and I empirically validated the necessity of each by reverting them in isolation and confirming the test still passes or panics as expected. The PR description and changelog wording were drafted with AI assistance and edited by me to reduce verbosity.