jsonformat: Tolerate incorrect paths in plan relevant_attributes#2988
jsonformat: Tolerate incorrect paths in plan relevant_attributes#2988apparentlymart merged 1 commit intomainfrom
Conversation
The code for matching relevant_attributes against resource_drift entries (a part of the heuristic for deciding whether to show "changes outside of OpenTofu" in the human-oriented plan UI) was previously assuming that paths in resource_drift would always be valid for the associated resource instance object values because in most cases the language runtime will detect invalid references and so fail to generate a plan at all. However, when the reference is to something within a dynamically-typed argument (such as the manifest in kubernetes_manifest) and when it appears only as an argument to either the "try" or "can" functions (so the dynamic error is intentionally suppressed) the language runtime can't catch it and so the incorrect reference will leak out into relevant_attributes, thereby violating assumptions made by the path matcher. Instead then, we'll continue the existing precedent that this "relevant attributes" mechanism is a best-effort heuristic that prefers to succeed with an incomplete result rather than to fail, extending that to the traversals in the plan renderer which will now treat incorrectly-typed steps as not matching rather than causing OpenTofu to crash completely. Since a reference to something that doesn't exist cannot succeed it also cannot possibly _actually_ contribute directly to the final result of the expression it appeared in, so in practice it should be okay to disregard these invalid references for the purposes of deciding which changes outside of OpenTofu seem likely to have caused the actions that OpenTofu is proposing to make during the apply phase. Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
|
I'm in favor of a backport. It's a unlikely situation granted, but it's still a clearly visible bug to users. |
|
Cool, okay! In that case, I'll leave this without a changelog entry and will then add the changelog entry in the subsequent backlog PR, so that it ends up on the changelog of the correct branch. |
yottta
left a comment
There was a problem hiding this comment.
The test in plan_test.go makes this clearer. Thanks a lot for it.
LGTM
|
It looks like HC is working on a change to HCL to work around this issue: hashicorp/hcl#763 |
|
That HCL change does address a proximate cause of the problem reported against the Making use of that schema information means that (once upgraded to a newer version of HCL with that fix) OpenTofu would catch the invalid use of However, it doesn't address the root cause: the code that populates The HCL fix is not wrong though: it complements the root-cause fix I made here by adding some additional cases where the language runtime will be able to reject an expression as invalid before trying to analyze the references in it. I do think we should also adopt that HCL patch once it's been included in a release, but we won't need to backport it to the v1.10 branch because this PR will prevent that situation from causing a crash. (Backporting seems a little risky because it subtly changes HCL's treatment of an invalid expression in a way that might affect existing uses of |
Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
Signed-off-by: Martin Atkins <mart@degeneration.co.uk> Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
Signed-off-by: Martin Atkins <mart@degeneration.co.uk> Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
Signed-off-by: Martin Atkins <mart@degeneration.co.uk> Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
The code for matching
relevant_attributesagainstresource_driftentries (a part of the heuristic for deciding whether to show "changes outside of OpenTofu" in the human-oriented plan UI) was previously assuming that paths inresource_driftwould always be valid for the associated resource instance object values because in most cases the language runtime will detect invalid references and so fail to generate a plan at all.However, when the reference is to something within a dynamically-typed argument (such as the manifest in
kubernetes_manifest) and when it appears only as an argument to either thetryorcanfunctions (so the dynamic error is intentionally suppressed) the language runtime can't catch it and so the incorrect reference will leak out intorelevant_attributes, thereby violating assumptions made by the path matcher.Instead then, we'll continue the existing precedent that this "relevant attributes" mechanism is a best-effort heuristic that prefers to succeed with an incomplete result rather than to fail, extending that to the traversals in the plan renderer which will now treat incorrectly-typed steps as not matching rather than causing OpenTofu to crash completely.
Since a reference to something that doesn't exist cannot succeed it also cannot possibly actually contribute directly to the final result of the expression it appeared in, so in practice it should be okay to disregard these invalid references for the purposes of deciding which changes outside of OpenTofu seem likely to have caused the actions that OpenTofu is proposing to make during the apply phase.
This problem was originally reported as a provider bug in hashicorp/terraform-provider-helm#1657. Although it was a change to the provider that caused existing valid references to become incorrect after upgrading, it's OpenTofu CLI's responsibility to describe that situation correctly without crashing, and so that's what this change aims to achieve.
Since this is arguably a pretty rare situation that arises only when a few different components interact together in a specific way, I'm ambivalent about whether this justifies a v1.10 backport. I've left this without a changelog entry for now so we can discuss that risk/reward tradeoff, but if we ultimately decide not to backport it then I'll add another commit here to introduce the changelog entry on the main branch rather than on the v1.10 branch.