Fix races in GetVariableValue and login

[jbardin]

Fix a few races that had crept into the codebase. Now that we've moved to a new CI system, we can probably start testing for these automatically, but there will still be some test fixtures that need updating.

• GetVariableValue was missing the variable mutex calls.
• The login command had a race around a closed-over err value.
• The testHook mock wasn't synchronized in the tests.
• Missing call to wg.Wait() caused a race in the deferred cleanup function.

Fixes #24589

[codecov]

Codecov Report

Merging #24599 into master will increase coverage by 0.00%.
The diff coverage is 75.00%.

Impacted Files	Coverage Δ
command/login.go	48.27% <50.00%> (-0.13%)	⬇️
terraform/eval_context_builtin.go	77.35% <100.00%> (+0.28%)	⬆️
dag/marshal.go	53.40% <0.00%> (+1.13%)	⬆️

[pselle]

LGTM, but I wouldn't object to another viewer

[corentone]

Thank you @jbardin !

For my education, --if you have time-- could you describe roughly how you were able to reproduce/identify the issue? I'm not too familiar with the TF code so it was tricky for me ( I swear I tried!)
I was looking at the DAG so I was pretty off from where you found the missing lock. The stacktrace, being in GetVariableValue, is not found again in the stacktrace but I only found in two other goroutines in the stacktrace that are in the Eval areas (but not explicitely GetVariableValue), so that would likely be them?

[jbardin]

No problem @corentone,

The most relevant portion of a stack trace is always the first stack. Since this is a fatal error and not a normal panic, it's coming out of the runtime and we don't have the corresponding read/write locations like we would from the race-detector, but it will show a map access somewhere. If you step back through the frames of that first stack, the first instance of terraform code you encounter is the GetVariableValue call. From here it was a quick diagnosis, since the mutex for the variables map was missing from that method. (If that method was not supposed to be safe for concurrent access in the first place, that would be a larger issue to diagnose)

I actually don't have a test fixture to trigger this at the moment, as any test configuration I have doesn't end up triggering the race. I have an idea to create a larger config that will, but since this bug is simple to visually inspect we can leave that for later, and try to fish out more of these in the long term with some fuzzing.

Thanks for reporting it!

[Ninir]

@jbardin Do you think it would be posslbe to backport that to a patch version for the TF 0.12 branch somehow?

I've heard reports of this impacting some people, and this is biting us also.

Edit: It's done as per the commit below 🙇‍♂️

[ghost]

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.