Plan Modifiers Not Executed Before Plan Computed-Only Unknown Marking #183
Comments
…knowns for Computed-only attributes in plans Reference: #181 Previously, any `Computed` attributes would always trigger plan differences due to the unknown marking. ``` Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols: ~ update in-place Terraform will perform the following actions: # hashicups_order.edu will be updated in-place ~ resource "hashicups_order" "edu" { ~ id = "1" -> (known after apply) ~ items = [ ~ { ~ coffee = { + description = (known after apply) id = 3 ~ image = "/nomad.png" -> (known after apply) ~ name = "Nomadicano" -> (known after apply) ~ price = 150 -> (known after apply) ~ teaser = "Drink one today and you will want to schedule another" -> (known after apply) } # (1 unchanged attribute hidden) ~ coffee = { + description = (known after apply) id = 1 ~ image = "/packer.png" -> (known after apply) ~ name = "Packer Spiced Latte" -> (known after apply) ~ price = 350 -> (known after apply) ~ teaser = "Packed with goodness to spice up your images" -> (known after apply) } # (2 unchanged attributes hidden) }, ] ~ last_updated = "Tuesday, 28-Sep-21 11:53:27 EDT" -> (known after apply) } Plan: 0 to add, 1 to change, 0 to destroy. ``` This change limits the unknown marking to only be performed when there is actually a change between the `ProposedNewState` and `State`. Without a configuration change: ``` No changes. Your infrastructure matches the configuration. ``` With a configuration change: ``` Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols: ~ update in-place Terraform will perform the following actions: # hashicups_order.edu will be updated in-place ~ resource "hashicups_order" "edu" { ~ id = "1" -> (known after apply) ~ items = [ ~ { ~ coffee = { + description = (known after apply) id = 3 ~ image = "/nomad.png" -> (known after apply) ~ name = "Nomadicano" -> (known after apply) ~ price = 150 -> (known after apply) ~ teaser = "Drink one today and you will want to schedule another" -> (known after apply) } # (1 unchanged attribute hidden) ~ coffee = { + description = (known after apply) ~ id = 1 -> 2 ~ image = "/packer.png" -> (known after apply) ~ name = "Packer Spiced Latte" -> (known after apply) ~ price = 350 -> (known after apply) ~ teaser = "Packed with goodness to spice up your images" -> (known after apply) } # (2 unchanged attributes hidden) }, ] ~ last_updated = "Tuesday, 28-Sep-21 11:53:27 EDT" -> (known after apply) } Plan: 0 to add, 1 to change, 0 to destroy. ``` As a followup, it may be necessary to invoke two passes of attribute and resource plan modifiers to ensure any changes captured by those modifiers triggers the unknown marking. As it stands now, a plan with no configuration changes, but changes due to plan modifiers will not correctly trigger the unknown marking. See also: #183
…knowns for Computed-only attributes in plans (#184) * tfsdk: Check ProposedNewState for State differences before marking unknowns for Computed-only attributes in plans Reference: #181 Previously, any `Computed` attributes would always trigger plan differences due to the unknown marking. ``` Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols: ~ update in-place Terraform will perform the following actions: # hashicups_order.edu will be updated in-place ~ resource "hashicups_order" "edu" { ~ id = "1" -> (known after apply) ~ items = [ ~ { ~ coffee = { + description = (known after apply) id = 3 ~ image = "/nomad.png" -> (known after apply) ~ name = "Nomadicano" -> (known after apply) ~ price = 150 -> (known after apply) ~ teaser = "Drink one today and you will want to schedule another" -> (known after apply) } # (1 unchanged attribute hidden) ~ coffee = { + description = (known after apply) id = 1 ~ image = "/packer.png" -> (known after apply) ~ name = "Packer Spiced Latte" -> (known after apply) ~ price = 350 -> (known after apply) ~ teaser = "Packed with goodness to spice up your images" -> (known after apply) } # (2 unchanged attributes hidden) }, ] ~ last_updated = "Tuesday, 28-Sep-21 11:53:27 EDT" -> (known after apply) } Plan: 0 to add, 1 to change, 0 to destroy. ``` This change limits the unknown marking to only be performed when there is actually a change between the `ProposedNewState` and `State`. Without a configuration change: ``` No changes. Your infrastructure matches the configuration. ``` With a configuration change: ``` Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols: ~ update in-place Terraform will perform the following actions: # hashicups_order.edu will be updated in-place ~ resource "hashicups_order" "edu" { ~ id = "1" -> (known after apply) ~ items = [ ~ { ~ coffee = { + description = (known after apply) id = 3 ~ image = "/nomad.png" -> (known after apply) ~ name = "Nomadicano" -> (known after apply) ~ price = 150 -> (known after apply) ~ teaser = "Drink one today and you will want to schedule another" -> (known after apply) } # (1 unchanged attribute hidden) ~ coffee = { + description = (known after apply) ~ id = 1 -> 2 ~ image = "/packer.png" -> (known after apply) ~ name = "Packer Spiced Latte" -> (known after apply) ~ price = 350 -> (known after apply) ~ teaser = "Packed with goodness to spice up your images" -> (known after apply) } # (2 unchanged attributes hidden) }, ] ~ last_updated = "Tuesday, 28-Sep-21 11:53:27 EDT" -> (known after apply) } Plan: 0 to add, 1 to change, 0 to destroy. ``` As a followup, it may be necessary to invoke two passes of attribute and resource plan modifiers to ensure any changes captured by those modifiers triggers the unknown marking. As it stands now, a plan with no configuration changes, but changes due to plan modifiers will not correctly trigger the unknown marking. See also: #183 Co-authored-by: Katy Moe <katy@katy.moe>
|
It'd probably be best to tackle this after #224 is reviewed and potentially merged. |
|
This actually extends to more than just computed-only attributes. And the consequences of the computed-only unknown marking system seem larger in scope than just this ordering bug. I don't know how feasible of a change this is, but if my understanding is correct I would like to maybe propose a better solution? I'm not too familiar with Terraform's internals, so apologies if I'm completely off on this analysis: Unknown values are treated as "get the new value from the provider create/update". But we don't want computed attributes to always be in the unknown state, since that would force The marking-if-we-definitely-know-there's-a-change has three major problems, as far as I can tell:
These issues combine to make a default value modifier like #285 somewhat problematic and error prone: while (2) would fix the issue of "if the default value changes, things need to be marked as unknown correctly", it doesn't fix (1) or (3). The proposed fix in this issue would maybe fix (2), but it sounds very confusing to run plan modifiers multiple times with different inputs. It sounds to me like what we actually need is for attributes to be set as "may change if an update is going to happen, but I won't cause an update myself". This would be the default plan for all computed attributes, always. If when the plan is built all attributes have either "unknown if update" or match state, no update has to happen on the resource. If a value is "true unknown" or different from the state, then an update will happen and all "unknown if update" properties get flipped to unknown as well. Plan modifiers can mess with this to their heart's content: changing an attribute plan to a true unknown or a value will trigger an update, without need to re-run all modifiers after. All modifiers get consistent and predictable sets of inputs. I don't know how feasible this would be to implement though, for example if there's any problems with protocol versioning or whatever. |
|
Thank you for the comment there, @PJB3005 👍 I want to provide some additional context about Terraform and framework design, as a drive-by comment while my focus is currently elsewhere since there's a lot of details about this particular portion of Terraform, the protocol, and the framework. Some of the Terraform details about planning behaviors and each of the data objects during that phase can be found in the following upstream documentation:
While the original design choices in this framework have prioritized the following two details that are pertinent here:
The automatic unknown marking was a pragmatic compromise because it was extremely likely that provider developers and practitioners would begin receiving "provider produced inconsistent result after apply" errors from Terraform as it has many constraints on how configuration, plan, and state data must operate across the resource lifecycle. (Aside: The previous terraform-plugin-sdk sets a special protocol flag to demote these errors to warning logs for various backwards compatibility reasons.) It would have otherwise introduced an unreasonable amount of provider developer burden to always generate a perfect plan according to those constraints. So far it appears this choice has been working out okay, due to lack of dissenting feedback in this issue tracker and HashiCorp Discuss. This edge with differing inputs to attribute plan modifiers seems to be the roughest, albeit on a much smaller scale than prior to the automatic behavior introduction. There's some additional nuance to planning and the current framework implementation that also affects the choices here:
If I'm understanding your whole comment above (you may find some of the Terraform repository documentation links help clarify some of your points there), I believe that this in particular:
Along with getting some Terraform design clarification on whether "no update" plans should actually be able to update non-configurable values will help guide what the framework should do or allow in this situation. The framework could possibly just ignore running schema-based or resource-level plan modification on those types of plans, leaving the refresh phase of the Another option instead of running the plan modifiers twice may be running the automatic unknown marking again after always running the other plan modifiers, keeping the same plan versus prior state check to still try to prevent spurious plans. I think this may be what you are alluding to, but maybe phrased differently for the actual implementation. This would help with preventing errors in situations like #456, but still means that plan modifiers need to account for "no update" plans in their logic. I hope this helps expand the discussion here. Please reach out with any questions. |
|
I was able to get some Terraform design clarification around "no configuration update" plans where Computed-only (read-only) attributes can plan updates in this manner. It was brought up that provider developers could use this mechanism to signal potential resource behavior changes to practitioners via an apply cycle, rather than via more practitioner-transparent mechanisms such as resource state upgrades or updates during refresh logic which would only show up in drift output, if enabled. While this type of provider logic may be less common, it might be ideal if the framework did not particularly make it more difficult or convoluted to implement, similar to other design choices we have made in the framework. In effect, I think this means we should shy away from automatically ignoring schema-based or resource-level plan modification on those types of plans. What this does mean though is that we should likely consider prioritizing the documentation of various possible plan modifier implementation details that provider developers may want to check, such as detecting whether a resource is being created, a resource is being destroyed, or whether configuration updates are actually occurring. The differing attribute plan value problem is still an interesting situation though. Plan modification logic can always reliably check the attribute configuration value for null, but an attribute plan value may either be null (if the marking didn't happen because the plan didn't have any configuration changes) or already marked as unknown (if the plan did have configuration changes). If this is a problem in practice, it would be helpful to see a particular use case that currently cannot be achieved, since the configuration value, current plan value (after prior plan modifiers on the attribute), and prior state values are available. |
|
After some further investigation this morning, the framework cannot just automatically run the Computed attribute configuration null to unknown logic after plan modifiers, since it would undo some of the prior plan modification as its only looking at the configuration. The framework cannot also just automatically run something similar, but only for the plan, since there would be no way for provider developers to set/leave a Computed attribute as null in the plan which should be valid. I believe this leaves us back to the originally proposed option of running plan modifiers twice, if we are to do anything, ensuring that the automatic null to unknown handling is called between those and never at the end. For provider developers wishing to not run plan modifier logic when there are no changes, they can return early with the following for now: // Prevent further action when the plan has no changes
if req.Plan.Raw.Equal(req.State.Raw) {
return
} |
bendbennett
added
the
breaking-change
|
This should be covered by #668, which introduces new resource default value handling before the unknown marking, which will release with terraform-plugin-framework version 1.2.0 🔜 . If there are further bug reports, feature requests, or documentation suggestions with resource default value handling, please raise a new issue. 👍 |
|
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. |
Module version
Followup to #181 fix.
Relevant framework source code
Expected Behavior
Having attribute or resource plan modifiers that cause a plan difference would cause the framework behavior of marking all Computed-only attributes as unknown in the plan. Attribute and resource plan modifiers would then re-run to allow providers to overwrite the unknowns or further perform plan actions.
e.g. Test passes as
computed_string_no_modifiersshould be unknown.Actual Behavior
Attribute and resource plan modifiers are only run after the marking of all Computed-only attributes, meaning that any changes caused by the plan modifiers may not correctly surface the marking behavior.
e.g. Test fails
Steps to Reproduce
go test -count=1 ./...with the above test addedReferences
(Config).GetAttribute()can error instead of returning nil in certain scenarios)The text was updated successfully, but these errors were encountered: