Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deprecation warning for destroy provisioner refs #23559

Merged
merged 1 commit into from
Dec 5, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 53 additions & 2 deletions configs/provisioner.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ func decodeProvisionerBlock(block *hcl.Block) (*Provisioner, hcl.Diagnostics) {
}
}

// destroy provisioners can only refer to self
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if pv.When == ProvisionerWhenDestroy {
diags = append(diags, onlySelfRefs(config)...)
}

if attr, exists := content.Attributes["on_failure"]; exists {
expr, shimDiags := shimTraversalInString(attr.Expr, true)
diags = append(diags, shimDiags...)
Expand Down Expand Up @@ -85,8 +90,11 @@ func decodeProvisionerBlock(block *hcl.Block) (*Provisioner, hcl.Diagnostics) {
}
seenConnection = block

//conn, connDiags := decodeConnectionBlock(block)
//diags = append(diags, connDiags...)
// destroy provisioners can only refer to self
if pv.When == ProvisionerWhenDestroy {
diags = append(diags, onlySelfRefs(block.Body)...)
}

pv.Connection = &Connection{
Config: block.Body,
DeclRange: block.DefRange,
Expand All @@ -107,6 +115,49 @@ func decodeProvisionerBlock(block *hcl.Block) (*Provisioner, hcl.Diagnostics) {
return pv, diags
}

func onlySelfRefs(body hcl.Body) hcl.Diagnostics {
var diags hcl.Diagnostics

// Provisioners currently do not use any blocks in their configuration.
// Blocks are likely to remain solely for meta parameters, but in the case
// that blocks are supported for provisioners, we will want to extend this
// to find variables in nested blocks.
attrs, _ := body.JustAttributes()
for _, attr := range attrs {
for _, v := range attr.Expr.Variables() {
valid := false
switch v.RootName() {
case "self":
valid = true
case "count":
// count must use "index"
if len(v) == 2 {
if t, ok := v[1].(hcl.TraverseAttr); ok && t.Name == "index" {
valid = true
}
}

case "each":
if len(v) == 2 {
if t, ok := v[1].(hcl.TraverseAttr); ok && t.Name == "key" {
valid = true
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@jbardin jbardin Dec 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops! Thank you for the explanation.


if !valid {
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'.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Subject: attr.Expr.Range().Ptr(),
})
}
}
}
return diags
}

// Connection represents a "connection" block when used within either a
// "resource" or "provisioner" block in a module or file.
type Connection struct {
Expand Down
11 changes: 11 additions & 0 deletions configs/resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,17 @@ func decodeResourceBlock(block *hcl.Block) (*Resource, hcl.Diagnostics) {
}
}

// Now we can validate the connection block references if there are any destroy provisioners.
// TODO: should we eliminate standalone connection blocks?
if r.Managed.Connection != nil {
for _, p := range r.Managed.Provisioners {
if p.When == ProvisionerWhenDestroy {
diags = append(diags, onlySelfRefs(r.Managed.Connection.Config)...)
break
}
}
}

return r, diags
}

Expand Down
40 changes: 40 additions & 0 deletions configs/testdata/warning-files/destroy-provisioners.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
locals {
user = "name"
}

resource "null_resource" "a" {
connection {
host = self.hostname
user = local.user # WARNING: External references from destroy provisioners are deprecated
}

provisioner "remote-exec" {
when = destroy
index = count.index
key = each.key

}
}

resource "null_resource" "b" {
connection {
host = self.hostname
# this is OK since there is no destroy provisioner
user = local.user
}

provisioner "remote-exec" {
}
}

resource "null_resource" "b" {
provisioner "remote-exec" {
when = destroy
connection {
host = self.hostname
user = local.user # WARNING: External references from destroy provisioners are deprecated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay comments!

Copy link
Member Author

@jbardin jbardin Dec 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

command = "echo ${local.name}" # WARNING: External references from destroy provisioners are deprecated
}
}