Fix accidental mutation of shared cty.Paths in ValueMarks funcs#32543
Fix accidental mutation of shared cty.Paths in ValueMarks funcs#32543nfagerlund merged 1 commit intomainfrom
cty.Paths in ValueMarks funcs#32543Conversation
Go's `append()` reserves the right to mutate its primary argument in-place, and expects the caller to assign its return value to the same variable that was passed as the primary argument. Due to what was almost definitely a typo (followed by copy-paste mishap), the configschema `Block.ValueMarks` and `Object.ValueMarks` functions were treating it like an immutable function that returns a new slice. In rare and hard-to-reproduce cases, this was causing bizarre malfunctions when marking sensitive schema attributes in deeply-nested block structures -- omitting the marks for some sensitive values (🚨), and marking other entire blocks as sensitive (which is supposed to be impossible). The chaotic and unreliable nature of the bugs is likely related to `append()`'s automatic slice reallocation behavior (if the append operation overflows the original array allocation, the resulting behavior can _look_ immutable), but there might be other contributing factors too. This commit fixes existing instances of the problem, and wraps the desired copy-and-append behavior in a helper function to simplify handling shared parent paths in an immutable way.
|
I think I now have a slightly more nuanced (but still incomplete) theory of how the bad behavior occurs. If you have a path (step slice) with a bit of extra headroom left in its underlying array, then any of its direct children who append to it will successively clobber the next slot out past the parent's length. So you'll end up with multiple PathValueMarks, one for each sibling child, but every one of their paths will be set to whatever the LAST one to be processed was. And then when they get marshalled into their final structure, they get de-duplicated, leaving only one. That de-duplication probably favors the first one, even though the last one was the one that contributed the path. So:
|
|
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
4 participants
Go's
append()reserves the right to mutate (the backing storage of) its primary argument in-place, and expects the caller to assign its return value to the same variable that was passed as the primary argument. Due to what was almost definitely a typo (followed by copy-paste mishap), the configschemaBlock.ValueMarksandObject.ValueMarksfunctions were treating it like an immutable function that returns a new slice.In rare and hard-to-reproduce cases, this was causing bizarre malfunctions when marking sensitive schema attributes in deeply-nested block structures -- omitting the marks for some sensitive values (🚨), and marking other entire blocks as sensitive (which is supposed to be impossible). The chaotic and unreliable nature of the bugs is likely related to
append()'s automatic slice reallocation behavior (if the append operation overflows the original array allocation, the resulting behavior can look immutable), but there might be other contributing factors too.This commit fixes existing instances of the problem, and wraps the desired copy-and-append behavior in a helper function to simplify handling shared parent paths in an immutable way.
Discussion for Reviewers
We originally started investigating this as a structured log bug (@brandonc had a previous PR for that, on which @alisdair pointed out that the "bug" should have been impossible), and ultimately chased it back to legit inaccurate info in the
after_sensitiveobject in the JSON plan.Reproducing this problem is incredibly dicey! Minimal repro attempts using the
tfcoremockandalisdair/nestedproviders all failed, and I only eventually succeeded by copy-pasting the actual schema fromrancher/rancher2into a dummy provider and playing with the nesting levels.To test the problem (on main or any released 1.x Terraform) and the fix (on this branch), use:
terraform plan -out plan.tfplan/terraform show -json plan.tfplan > plan.jsonOnce you've got the plan json, jump to the
after_sensitiveproperty for the one resource_change:etcdblock should have bothcertandkeyattrs marked as sensitive.s3_backup_configblock should have bothaccess_keyandsecret_keyattrs marked as sensitive.In trying to pin down the problem, I duplicated the problematic nested schema a few times in my dummy resource, with one instance at the original nesting level, one instance nested another level down, and instances out-dented by one and two levels. As you can see in this side-by-side of excerpted values, the bug behaves differently for all of them.
To me, that smells like something profoundly order-dependent and haunted.
And that, dear reader, is why I don't have tests on this PR -- nailing down this chaotic behavior enough to get a minimal, reliable repro using only an in-memory schema was going to take at least another two days, and since the problem originates with some blatant and inarguable typos, I couldn't quite justify the extra expense to my team.
(...But if reviewers insist on giving me an excuse to, obviously I'm chomping at the bit to blow another two days nailing a stake through this thing's heart, y'all know what I'm about. Just, trying to be pragmatic here.)
Target Release
1.4.x
Draft CHANGELOG entry
BUG FIXES
before_sensitive/after_sensitiveannotations in JSON plan output for deeply nested structures. This was only observed in the wild on the rancher/rancher2 provider, and resulted in glitched display in Terraform Cloud's structured plan log view.