helper/schema: Normalize bools to "true"/"false" in diffs

[phinze]

For a long time now, the diff logic has relied on the behavior of
mapstructure.WeakDecode to determine how various primitives are
converted into strings. The schema.DiffString function is used for
all primitive field types: TypeBool, TypeInt, TypeFloat, and TypeString.

The mapstructure library's string representation of booleans is "0"
and "1", which differs from strconv.FormatBool's "false" and "true"
(which is used in writing out boolean fields to the state).

Because of this difference, diffs have long had the potential for
cosmetically odd but semantically neutral output like:

"true" => "1"
"false" => "0"

So long as mapstructure.Decode or strconv.ParseBool are used to
interpret these strings, there's no functional problem.

We had our first clear functional problem with #6005 and friends, where
users noticed diffs like the above showing up unexpectedly and causing
troubles when ignore_changes was in play.

This particular bug occurs down in Terraform core's EvalIgnoreChanges.
There, the diff is modified to account for ignored attributes, and
special logic attempts to handle properly the situation where the
ignored attribute was going to trigger a resource replacement. That
logic relies on the string representations of the Old and New fields in
the diff to be the same so that it filters properly.

So therefore, we now get a bug when a diff includes Old: "0", New: "false" since the strings do not match, and ignore_changes is not
properly handled.

Here, we introduce TypeBool-specific normalizing into finalizeDiff.
I spiked out a full diffBool function, but figuring out which pieces
of diffString to duplicate there got hairy. This seemed like a simpler
and more direct solution.

Fixes #6005 (and potentially others!)

[stack72]

This LGTM!

[catsby]

LGTM! This doesn't affect user configs and such, right?

[phinze]

@catsby yep! configs and state representations are unaffected - this only affects diffs

[jen20]

LGTM @phinze!

[Fodoj]

And why not to write 1\0 into state file initially instead of doing this kind of conversion?

[phinze]

Hi @Fodoj - good question! I decided to go with this implementation for a couple of reasons: (1) it was the smaller, less risky change to make, (2) I think "true" / "false" are a more conventional string representations of booleans anyways.

It's likely that mapstructure will be updated to produce "true" and "false" for its bool-to-string conversion in the future, but I wanted to get a direct solution to the bug in first.

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