Don't crash when populating NewExtra. #686
Conversation
When populating NewExtra, we iterate over attributes and inspect their NewExtra property. In the case of null attributes, this results in a crash, as we're dereferencing a nil pointer. I'm still not clear on why this is happening, where it's coming from, or what it means. The current understanding is it has something to do with usage of CustomizeDiff. I'm not sure why yet. But this commit at least surfaces which attribute was null, by logging it, and prevents the crash, even if it doesn't resolve the problem.
There was a problem hiding this comment.
This seems wise - my only concern is that by preventing the crash without understanding why it occurred, we might be allowing incorrect behaviour. Would it be better to log and still crash?
Approving anyway because based on hashicorp/terraform-provider-google#7934, the risk of incorrect behaviour seems small when compared to the effect of the crash on users right now.
Add logging that will catch the bug red-handed, hopefully, or at least give us more details if our understanding of the bug is off. Also, restore the crash behavior so we can see where it fits in with the logs.
Previously, we'd assign the result of finalizeDiff to the resource diff without checking its return. This caused problems because a "finalized" diff for any given attribute could, in fact, be no diff at all. Which we represent as `nil`. But some consumers of the resource diff expect every attribute in the map to be non-`nil`, and so crash on these attributes that have diff entries but no diffs. See for example hashicorp/terraform-provider-google#7934, which would crash when a config had an explicit empty string as the value for a field that a CustomizeDiff function set to ForceNew. Technically, there was a diff, but finalizeDiff decided it wasn't a "real" diff, because the SDK still interprets empty strings as "unset" for computed fields to align with legacy behavior. But that meant a nil in the resource's map of attribute diffs, which then was dereferenced when populating the response to PlanResourceChange. This caused a crash. This commit fixes that issue by updating all our usages of finalizeDiff to check for a nil diff _before_ writing it to the resource's map of attribute diffs. This was easier than tracking down all the usages of a ResourceAttributeDiff and trying to ensure they were ignoring nil values.
|
@kmoe I successfully got to the bottom of the problem, and a customer that could reproduce it got me logs (see 99e4a70) that caught the bug red-handed and confirmed that I could've updated our usage of Instead, I opted to (in 570e128) update every usage of This took so long to come up in such an old and well-used resource because it only triggers when there's technically a diff and
When those conditions are met, I added a log line to explicitly call out when these conditions are met and we're squashing a diff for legacy reasons. Mind taking another look? |
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked as resolved and limited conversation to collaborators
When populating NewExtra, we iterate over attributes and inspect their
NewExtra property. In the case of null attributes, this results in a
crash, as we're dereferencing a nil pointer.
I'm still not clear on why this is happening, where it's coming from, or
what it means. The current understanding is it has something to do with
usage of CustomizeDiff. I'm not sure why yet. But this commit at least
surfaces which attribute was null, by logging it, and prevents the
crash, even if it doesn't resolve the problem.
See #548, hashicorp/terraform-provider-google#7934