deprecation warning for destroy provisioner refs #23559

jbardin · 2019-12-04T15:40:25Z

Add deprecation warning for references from destroy provisioners or
their connections to external resources or values. In order to ensure
resource destruction can be completed correctly, destroy nodes must be
able to evaluate with only their instance state.

We have sufficient information to validate destroy-time provisioners
early on during the config loading process. Later on these can be
converted to hard errors, and only allow self, count.index, and each.key
in destroy provisioners. Limiting the provisioner and block evaluation
scope later on is tricky, but if the references can never be loaded,
then they will never be encountered during evaluation.

The new warning output will look like

Warning: External references from destroy provisioners are deprecated

  on main.tf line 12, in resource "null_resource" "new":
  12:     command = local.command

Destroy-time provisioners and their connection configurations may only
reference attributes of the related resource, via 'self', 'count.index', or
'each.key'.

References to other resources during the destroy phase can cause dependency
cycles and interact poorly with create_before_destroy.

Background for the upcoming change

Destroy provisioners were added way back in the 0.9.0 release. The initial concept was simple, but the implications of the added dependencies of evaluating provisioners during destroy was not fully thought out. It turns out that it is very easy to inadvertently introduce dependency cycles, yet hard to detect when they might happen, since it requires a certain combination of updates to trigger. For example, the image below shows a simplified graph of 2 resources being replaced, with a_destroy containing a provisioner that indirectly references resource b. These resource may even be spread across modules, and updated due to unrelated events.

In the releases since 0.9.0, these cycles have both become easier to create and much harder to detect and reason about due to the increased use of modules. Modules themselves also lose some of their intended composability, because the inputs and outputs may have unexpected restrictions on which resources can be used with them to ensure there could be no cycles.

Since adding more heuristics and workarounds to detect and break these cycles won't ever be able to solve them all within Terraform's current operating model, we're proposing to deprecate the possibility of evaluating anything but a resource's own state during destroy. This results in a destroy subgraph that will only ever involve dependent resources, and create_before_destroy will only need to take into account the edges between the create and destroy nodes (notice in the graph above, create_before_destroy would remove the cycle, but in other cases it is just as likely to introduce one). In short, this means that any updates involving resource destruction are more reliable and easier to reason about.

configs/provisioner.go

mildwonkey · 2019-12-04T16:09:23Z

configs/provisioner.go

@@ -50,6 +50,11 @@ func decodeProvisionerBlock(block *hcl.Block) (*Provisioner, hcl.Diagnostics) {
 		}
 	}

+	// destroy provisioners can only refer to self


Tiny nitpick/question: since we're only giving warnings right now should this comment say destroy provisioners _should_ only refer to self?
I am completely ok with disagreement :)

Even though that was only there because I backported this from it being an error, I think it might be better as "can". In this case, they "can only refer to self" in order to avoid the deprecation warnings?

Nice clarification, makes sense to me (and I wasn't going to fight you on it anyway)!

Add deprecation warning for references from destroy provisioners or their connections to external resources or values. In order to ensure resource destruction can be completed correctly, destroy nodes must be able to evaluate with only their instance state. We have sufficient information to validate destroy-time provisioners early on during the config loading process. Later on these can be converted to hard errors, and only allow self, count.index, and each.key in destroy provisioners. Limited the provisioner and block evaluation scope later on is tricky, but if the references can never be loaded, then they will never be encountered during evaluation.

pselle · 2019-12-04T23:31:43Z

configs/provisioner.go

+						valid = true
+					}
+				}
+			}


Shouldn't there be a panic on default to catch other RootNames that wouldn't be expected?

Any names not marked as valid here return the diagnostic below. We don't want to panic in the config loader, because we can instead present a diagnostic to the user to indicate the exact source location that caused the error.

As for a default case, any possible identifier is valid in other contexts, but we want to limit the destroy provisioners to only these 3 cases. I didn't use the default here, because 2 of the cases have extra conditions that need to be checked.

Oops! Thank you for the explanation.

pselle · 2019-12-04T23:32:35Z

configs/testdata/warning-files/destroy-provisioners.tf

+    when = destroy
+    connection {
+      host = self.hostname
+      user = local.user # WARNING: External references from destroy provisioners are deprecated


yay comments!

Yes, I missed this feature last time around, but it's a really nice test fixture from @apparentlymart!
The test matches the comment locations and text to the diagnostic output.

danieldreier · 2019-12-05T01:20:09Z

We should probably put a pretty prominent note in the release notes, so that people notice and can give feedback on this change.

pselle

💭 I feel like doing diag-collapse on this the way we did with interpolation is a good idea if that doesn't automagically happen

pselle · 2019-12-05T17:32:21Z

configs/provisioner.go

+				diags = append(diags, &hcl.Diagnostic{
+					Severity: hcl.DiagWarning,
+					Summary:  "External references from destroy provisioners are deprecated",
+					Detail:   "Destroy time provisioners and their connections may only reference values stored in the instance state, which include 'self', 'count.index', or 'each.key'.",


Would it be handy to also include what they had used/referenced?

That will be automatically generated by the Diagnostic.Subject field when printed out.

This is a subjective thing and I won't mind at all if others disagree, but I think I'd lean towards making this error message a little less implementation-details-y by not mentioning the "instance state" at all.

For example:

Destroy-time provisioners and their connection configurations may only reference attributes of the related resource, via 'self', 'count.index', or 'each.key'.

For our other deprecation warnings in this cycle we've included an extra paragraph giving a bit of a "why". This one is too hairy to go into all the details, but perhaps as a compromise we could add an additional sentence something like this:

References to other resources during the destroy phase can cause dependency cycles and interact poorly with create_before_destroy.

As a bonus, a sentence like that might help a user connect the dots between this warning and a subsequent error reporting a graph cycle, giving them a better clue as to how to resolve that error.

Another idea, though this one is definitely in the "optional polish" category:

Could we perhaps recognize the situation where the problem is in a connection block that is shared with other provisioners and add an additional hint that it could help to override the shared block with a connection block directly inside the when = destroy provisioner block? From my memory of how this is modeled in configuration I have a feeling it would be easier said than done, so no worries if it seems super hard, but I wanted to raise it just in case there were a relatively straightforward way to hint that here.

I do like that wording better, I'll push an update to that tomorrow.

As for the separate connection blocks, you're right it's a bit hairy because they're loaded separately. Following the path of making config more explicit and easier to understand, I don't think this imposes large burden to move a resource connection into the compatible provisioner when it's encountered, and then the logic of overriding doesn't have to be taken into account be the loader or the user.

apparentlymart

Some late-arriving "polish-ish" feedback here. I know this is already merged, and none of this is critical stuff so I'm leaving it here only in case it seems interesting for a follow-up commit.

apparentlymart · 2019-12-05T23:10:53Z

configs/provisioner.go

+				diags = append(diags, &hcl.Diagnostic{
+					Severity: hcl.DiagWarning,
+					Summary:  "External references from destroy provisioners are deprecated",
+					Detail:   "Destroy time provisioners and their connections may only reference values stored in the instance state, which include 'self', 'count.index', or 'each.key'.",


This is a subjective thing and I won't mind at all if others disagree, but I think I'd lean towards making this error message a little less implementation-details-y by not mentioning the "instance state" at all.

For example:

Destroy-time provisioners and their connection configurations may only reference attributes of the related resource, via 'self', 'count.index', or 'each.key'.

For our other deprecation warnings in this cycle we've included an extra paragraph giving a bit of a "why". This one is too hairy to go into all the details, but perhaps as a compromise we could add an additional sentence something like this:

References to other resources during the destroy phase can cause dependency cycles and interact poorly with create_before_destroy.

As a bonus, a sentence like that might help a user connect the dots between this warning and a subsequent error reporting a graph cycle, giving them a better clue as to how to resolve that error.

apparentlymart · 2019-12-05T23:13:54Z

configs/provisioner.go

+				diags = append(diags, &hcl.Diagnostic{
+					Severity: hcl.DiagWarning,
+					Summary:  "External references from destroy provisioners are deprecated",
+					Detail:   "Destroy time provisioners and their connections may only reference values stored in the instance state, which include 'self', 'count.index', or 'each.key'.",


Another idea, though this one is definitely in the "optional polish" category:

Could we perhaps recognize the situation where the problem is in a connection block that is shared with other provisioners and add an additional hint that it could help to override the shared block with a connection block directly inside the when = destroy provisioner block? From my memory of how this is modeled in configuration I have a feeling it would be easier said than done, so no worries if it seems super hard, but I wanted to raise it just in case there were a relatively straightforward way to hint that here.

ghost · 2020-03-28T02:06:20Z

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.

jbardin requested a review from a team December 4, 2019 15:40

ghost added the sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK label Dec 4, 2019

mildwonkey reviewed Dec 4, 2019

View reviewed changes

configs/provisioner.go Outdated Show resolved Hide resolved

mildwonkey reviewed Dec 4, 2019

View reviewed changes

mildwonkey approved these changes Dec 4, 2019

View reviewed changes

jbardin force-pushed the jbardin/deprecate-destroy-references branch from 8fc94c6 to 8547603 Compare December 4, 2019 16:14

pselle reviewed Dec 4, 2019

View reviewed changes

pselle reviewed Dec 5, 2019

View reviewed changes

jbardin merged commit 6817c84 into master Dec 5, 2019

jbardin deleted the jbardin/deprecate-destroy-references branch December 5, 2019 23:07

apparentlymart reviewed Dec 5, 2019

View reviewed changes

jbardin mentioned this pull request Dec 6, 2019

Update destroy provisioner warnings #23595

Merged

dmrzzz mentioned this pull request Dec 13, 2019

permit destroy-time local-exec provisioner to reference path.module #23675

Closed

jbardin mentioned this pull request Jan 6, 2020

Destroy Provisioner throwing Cycle error on resources getting destroyed when running Terraform Apply #23782

Closed

jbardin mentioned this pull request Feb 12, 2020

Instance state only destroys #24083

Merged

danieldreier mentioned this pull request Mar 11, 2020

Allow destroy-time provisioners to access variables #23679

Open

ghost locked and limited conversation to collaborators Mar 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deprecation warning for destroy provisioner refs #23559

deprecation warning for destroy provisioner refs #23559

jbardin commented Dec 4, 2019 •

edited

Loading

mildwonkey Dec 4, 2019

jbardin Dec 4, 2019

mildwonkey Dec 4, 2019

pselle Dec 4, 2019

jbardin Dec 5, 2019 •

edited

Loading

pselle Dec 5, 2019

pselle Dec 4, 2019

jbardin Dec 5, 2019 •

edited

Loading

danieldreier commented Dec 5, 2019

pselle left a comment

pselle Dec 5, 2019

jbardin Dec 5, 2019

apparentlymart Dec 5, 2019

apparentlymart Dec 5, 2019

jbardin Dec 5, 2019

apparentlymart left a comment

apparentlymart Dec 5, 2019

apparentlymart Dec 5, 2019

ghost commented Mar 28, 2020

deprecation warning for destroy provisioner refs #23559

deprecation warning for destroy provisioner refs #23559

Conversation

jbardin commented Dec 4, 2019 • edited Loading

Background for the upcoming change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbardin Dec 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbardin Dec 5, 2019 • edited Loading

Choose a reason for hiding this comment

danieldreier commented Dec 5, 2019

pselle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apparentlymart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Mar 28, 2020

jbardin commented Dec 4, 2019 •

edited

Loading

jbardin Dec 5, 2019 •

edited

Loading

jbardin Dec 5, 2019 •

edited

Loading