From 5e545ff427a31dfe5b60d91dab367c4a7e7e5a2b Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 29 Feb 2024 16:14:31 -0800 Subject: [PATCH 1/3] configs: Provisioners in "removed" blocks When the removed_provisioners experiment is active, removed blocks referring to managed resources are allowed to include "connection" and "provisioner" blocks, as long as all of the "provisioner" blocks specify when = destroy to indicate that they should execute as part of the resource's "destroy" action. This commit only deals with parsing the configuration. The logic to react to this during the apply phase will follow in later commits. --- internal/configs/removed.go | 81 +++++++++++- internal/configs/removed_test.go | 208 +++++++++++++++++++++++++++++++ 2 files changed, 286 insertions(+), 3 deletions(-) diff --git a/internal/configs/removed.go b/internal/configs/removed.go index 7ec39f01912d..dadaee17bad7 100644 --- a/internal/configs/removed.go +++ b/internal/configs/removed.go @@ -4,6 +4,8 @@ package configs import ( + "fmt" + "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/hcl/v2" @@ -19,6 +21,12 @@ type Removed struct { // from state. Defaults to true. Destroy bool + // Managed captures a number of metadata fields that are applicable only + // for managed resources, and not for other resource modes. + // + // "removed" blocks support only a subset of the fields in [ManagedResource]. + Managed *ManagedResource + DeclRange hcl.Range } @@ -31,6 +39,8 @@ func decodeRemovedBlock(block *hcl.Block) (*Removed, hcl.Diagnostics) { content, moreDiags := block.Body.Content(removedBlockSchema) diags = append(diags, moreDiags...) + var targetKind addrs.RemoveTargetKind + var resourceMode addrs.ResourceMode // only valid if targetKind is addrs.RemoveTargetResource if attr, exists := content.Attributes["from"]; exists { from, traversalDiags := hcl.AbsTraversalForExpr(attr.Expr) diags = append(diags, traversalDiags...) @@ -38,11 +48,21 @@ func decodeRemovedBlock(block *hcl.Block) (*Removed, hcl.Diagnostics) { from, fromDiags := addrs.ParseRemoveTarget(from) diags = append(diags, fromDiags.ToHCL()...) removed.From = from + if removed.From != nil { + targetKind = removed.From.ObjectKind() + if targetKind == addrs.RemoveTargetResource { + resourceMode = removed.From.RelSubject.(addrs.ConfigResource).Resource.Mode + } + } } } removed.Destroy = true + if resourceMode == addrs.ManagedResourceMode { + removed.Managed = &ManagedResource{} + } + var seenConnection *hcl.Block for _, block := range content.Blocks { switch block.Type { case "lifecycle": @@ -53,6 +73,61 @@ func decodeRemovedBlock(block *hcl.Block) (*Removed, hcl.Diagnostics) { valDiags := gohcl.DecodeExpression(attr.Expr, nil, &removed.Destroy) diags = append(diags, valDiags...) } + + case "connection": + if removed.Managed == nil { + // target is not a managed resource, then + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid connection block", + Detail: "Provisioner connection configuration is valid only when a removed block targets a managed resource.", + Subject: &block.DefRange, + }) + continue + } + + if seenConnection != nil { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Duplicate connection block", + Detail: fmt.Sprintf("This \"removed\" block already has a connection block at %s.", seenConnection.DefRange), + Subject: &block.DefRange, + }) + continue + } + seenConnection = block + + removed.Managed.Connection = &Connection{ + Config: block.Body, + DeclRange: block.DefRange, + } + + case "provisioner": + if removed.Managed == nil { + // target is not a managed resource, then + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid provisioner block", + Detail: "Provisioners are valid only when a removed block targets a managed resource.", + Subject: &block.DefRange, + }) + continue + } + + pv, pvDiags := decodeProvisionerBlock(block) + diags = append(diags, pvDiags...) + if pv != nil { + removed.Managed.Provisioners = append(removed.Managed.Provisioners, pv) + + if pv.When != ProvisionerWhenDestroy { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid provisioner block", + Detail: "Only destroy-time provisioners are valid in \"removed\" blocks. To declare a destroy-time provisioner, use:\n when = destroy", + Subject: &block.DefRange, + }) + } + } } } @@ -67,9 +142,9 @@ var removedBlockSchema = &hcl.BodySchema{ }, }, Blocks: []hcl.BlockHeaderSchema{ - { - Type: "lifecycle", - }, + {Type: "lifecycle"}, + {Type: "connection"}, + {Type: "provisioner", LabelNames: []string{"type"}}, }, } diff --git a/internal/configs/removed_test.go b/internal/configs/removed_test.go index 5ac1480f05f2..dc43225b8fc0 100644 --- a/internal/configs/removed_test.go +++ b/internal/configs/removed_test.go @@ -60,6 +60,7 @@ func TestRemovedBlock_decode(t *testing.T) { &Removed{ From: mustRemoveEndpointFromExpr(foo_expr), Destroy: true, + Managed: &ManagedResource{}, DeclRange: blockRange, }, ``, @@ -93,10 +94,155 @@ func TestRemovedBlock_decode(t *testing.T) { &Removed{ From: mustRemoveEndpointFromExpr(foo_expr), Destroy: false, + Managed: &ManagedResource{}, DeclRange: blockRange, }, ``, }, + "provisioner when = destroy": { + &hcl.Block{ + Type: "removed", + Body: hcltest.MockBody(&hcl.BodyContent{ + Attributes: hcl.Attributes{ + "from": { + Name: "from", + Expr: foo_expr, + }, + }, + Blocks: hcl.Blocks{ + &hcl.Block{ + Type: "provisioner", + Labels: []string{"remote-exec"}, + LabelRanges: []hcl.Range{{}}, + Body: hcltest.MockBody(&hcl.BodyContent{ + Attributes: hcl.Attributes{ + "when": { + Name: "when", + Expr: hcltest.MockExprTraversalSrc("destroy"), + }, + }, + }), + }, + }, + }), + DefRange: blockRange, + }, + &Removed{ + From: mustRemoveEndpointFromExpr(foo_expr), + Destroy: true, + Managed: &ManagedResource{ + Provisioners: []*Provisioner{ + { + Type: "remote-exec", + Config: hcltest.MockBody(&hcl.BodyContent{ + Attributes: hcl.Attributes{}, + Blocks: hcl.Blocks{}, + }), + When: ProvisionerWhenDestroy, + OnFailure: ProvisionerOnFailureFail, + }, + }, + }, + DeclRange: blockRange, + }, + ``, + }, + "provisioner when = create": { + &hcl.Block{ + Type: "removed", + Body: hcltest.MockBody(&hcl.BodyContent{ + Attributes: hcl.Attributes{ + "from": { + Name: "from", + Expr: foo_expr, + }, + }, + Blocks: hcl.Blocks{ + &hcl.Block{ + Type: "provisioner", + Labels: []string{"local-exec"}, + LabelRanges: []hcl.Range{{}}, + Body: hcltest.MockBody(&hcl.BodyContent{ + Attributes: hcl.Attributes{ + "when": { + Name: "when", + Expr: hcltest.MockExprTraversalSrc("create"), + }, + }, + }), + }, + }, + }), + DefRange: blockRange, + }, + &Removed{ + From: mustRemoveEndpointFromExpr(foo_expr), + Destroy: true, + Managed: &ManagedResource{ + Provisioners: []*Provisioner{ + { + Type: "local-exec", + Config: hcltest.MockBody(&hcl.BodyContent{ + Attributes: hcl.Attributes{}, + Blocks: hcl.Blocks{}, + }), + When: ProvisionerWhenCreate, + OnFailure: ProvisionerOnFailureFail, + }, + }, + }, + DeclRange: blockRange, + }, + `Invalid provisioner block`, + }, + "provisioner no when": { + &hcl.Block{ + Type: "removed", + Body: hcltest.MockBody(&hcl.BodyContent{ + Attributes: hcl.Attributes{ + "from": { + Name: "from", + Expr: foo_expr, + }, + }, + Blocks: hcl.Blocks{ + &hcl.Block{ + Type: "connection", + Body: hcltest.MockBody(&hcl.BodyContent{}), + }, + &hcl.Block{ + Type: "provisioner", + Labels: []string{"local-exec"}, + LabelRanges: []hcl.Range{{}}, + Body: hcltest.MockBody(&hcl.BodyContent{}), + }, + }, + }), + DefRange: blockRange, + }, + &Removed{ + From: mustRemoveEndpointFromExpr(foo_expr), + Destroy: true, + Managed: &ManagedResource{ + Connection: &Connection{ + Config: hcltest.MockBody(&hcl.BodyContent{}), + }, + Provisioners: []*Provisioner{ + { + Type: "local-exec", + Config: hcltest.MockBody(&hcl.BodyContent{ + Attributes: hcl.Attributes{}, + Blocks: hcl.Blocks{}, + }), + When: ProvisionerWhenCreate, + OnFailure: ProvisionerOnFailureFail, + }, + }, + }, + DeclRange: blockRange, + }, + `Invalid provisioner block`, + }, "modules": { &hcl.Block{ Type: "removed", @@ -130,6 +276,67 @@ func TestRemovedBlock_decode(t *testing.T) { }, ``, }, + "provisioner for module": { + &hcl.Block{ + Type: "removed", + Body: hcltest.MockBody(&hcl.BodyContent{ + Attributes: hcl.Attributes{ + "from": { + Name: "from", + Expr: mod_foo_expr, + }, + }, + Blocks: hcl.Blocks{ + &hcl.Block{ + Type: "provisioner", + Labels: []string{"local-exec"}, + LabelRanges: []hcl.Range{{}}, + Body: hcltest.MockBody(&hcl.BodyContent{ + Attributes: hcl.Attributes{ + "when": { + Name: "when", + Expr: hcltest.MockExprTraversalSrc("destroy"), + }, + }, + }), + }, + }, + }), + DefRange: blockRange, + }, + &Removed{ + From: mustRemoveEndpointFromExpr(mod_foo_expr), + Destroy: true, + DeclRange: blockRange, + }, + `Invalid provisioner block`, + }, + "connection for module": { + &hcl.Block{ + Type: "removed", + Body: hcltest.MockBody(&hcl.BodyContent{ + Attributes: hcl.Attributes{ + "from": { + Name: "from", + Expr: mod_foo_expr, + }, + }, + Blocks: hcl.Blocks{ + &hcl.Block{ + Type: "connection", + Body: hcltest.MockBody(&hcl.BodyContent{}), + }, + }, + }), + DefRange: blockRange, + }, + &Removed{ + From: mustRemoveEndpointFromExpr(mod_foo_expr), + Destroy: true, + DeclRange: blockRange, + }, + `Invalid connection block`, + }, // KEM Unspecified behaviour "no lifecycle block": { &hcl.Block{ @@ -147,6 +354,7 @@ func TestRemovedBlock_decode(t *testing.T) { &Removed{ From: mustRemoveEndpointFromExpr(foo_expr), Destroy: true, + Managed: &ManagedResource{}, DeclRange: blockRange, }, ``, From 1b9c0cbe5d9cd03074b86f00a0026f341e571f77 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 29 Feb 2024 17:46:23 -0800 Subject: [PATCH 2/3] terraform: Basic support for provisioners in "removed" blocks During the apply phase, we'll check if there are provisioners either in the matching "resource" block or the matching "removed" block -- whichever of the two is present -- and execute the destroy-time subset of them either way. This also establishes a standard way to attach a "removed" block to a NodeResourceAbstract when one is defined, which is likely to be useful for supporting other resource-related meta arguments in "removed" blocks in future. One known limitation and design question from this initial implementation is: how should each.key, each.value, and count.index behave when used as part of a provisioner configuration in a "removed" block? This is a tricky question because whereas a "resource" block allows us to determine from the configuration whether we're using count, for_each, or neither, removed blocks must accept whatever happens to be in the state and so in unusual cases there might even be a mixture of numeric instance keys and string instance keys for the same resource, making it impossible to write a provisioner configuration that would work with both. --- internal/terraform/context_apply_test.go | 51 +++++++++++++++++++ internal/terraform/node_resource_abstract.go | 9 +++- .../node_resource_abstract_instance.go | 28 ++++++---- .../apply-provisioner-destroy-removed/main.tf | 8 +++ .../transform_attach_config_resource.go | 31 +++++++++-- 5 files changed, 111 insertions(+), 16 deletions(-) create mode 100644 internal/terraform/testdata/apply-provisioner-destroy-removed/main.tf diff --git a/internal/terraform/context_apply_test.go b/internal/terraform/context_apply_test.go index f9a753a48675..a8b2cc15e399 100644 --- a/internal/terraform/context_apply_test.go +++ b/internal/terraform/context_apply_test.go @@ -4908,6 +4908,57 @@ func TestContext2Apply_provisionerDestroy(t *testing.T) { } } +func TestContext2Apply_provisionerDestroyRemoved(t *testing.T) { + m := testModule(t, "apply-provisioner-destroy-removed") + p := testProvider("aws") + pr := testProvisioner() + p.PlanResourceChangeFn = testDiffFn + pr.ProvisionResourceFn = func(req provisioners.ProvisionResourceRequest) (resp provisioners.ProvisionResourceResponse) { + val := req.Config.GetAttr("command").AsString() + // The following is "destroy ${each.key} ${self.foo}" + if val != "destroy a bar" { + t.Fatalf("wrong value for command: %q", val) + } + + return + } + + state := states.NewState() + root := state.RootModule() + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr(`aws_instance.foo["a"]`).Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"bar","foo":"bar"}`), + }, + mustProviderConfig(`provider["registry.terraform.io/hashicorp/aws"]`), + ) + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("aws"): testProviderFuncFixed(p), + }, + Provisioners: map[string]provisioners.Factory{ + "shell": testProvisionerFuncFixed(pr), + }, + }) + + plan, diags := ctx.Plan(m, state, SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables))) + assertNoErrors(t, diags) + + state, diags = ctx.Apply(plan, m, nil) + if diags.HasErrors() { + t.Fatalf("diags: %s", diags.Err()) + } + + checkStateString(t, state, ``) + + // Verify apply was invoked + if !pr.ProvisionResourceCalled { + t.Fatalf("provisioner not invoked") + } +} + // Verify that on destroy provisioner failure, nothing happens to the instance func TestContext2Apply_provisionerDestroyFail(t *testing.T) { m := testModule(t, "apply-provisioner-destroy") diff --git a/internal/terraform/node_resource_abstract.go b/internal/terraform/node_resource_abstract.go index c29e74c2aa46..b5f940d87780 100644 --- a/internal/terraform/node_resource_abstract.go +++ b/internal/terraform/node_resource_abstract.go @@ -55,7 +55,11 @@ type NodeAbstractResource struct { Schema *configschema.Block // Schema for processing the configuration body SchemaVersion uint64 // Schema version of "Schema", as decided by the provider - Config *configs.Resource // Config is the resource in the config + + // Config and RemovedConfig are mutally-exclusive, because a + // resource can't be both declared and removed at the same time. + Config *configs.Resource // Config is the resource in the config, if any + RemovedConfig *configs.Removed // RemovedConfig is the "removed" block for this resource, if any // ProviderMetas is the provider_meta configs for the module this resource belongs to ProviderMetas map[addrs.Provider]*configs.ProviderMeta @@ -359,8 +363,9 @@ func (n *NodeAbstractResource) AttachDataResourceDependsOn(deps []addrs.ConfigRe } // GraphNodeAttachResourceConfig -func (n *NodeAbstractResource) AttachResourceConfig(c *configs.Resource) { +func (n *NodeAbstractResource) AttachResourceConfig(c *configs.Resource, rc *configs.Removed) { n.Config = c + n.RemovedConfig = rc } // GraphNodeAttachResourceSchema impl diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index 7b442a714de5..d298961ff557 100644 --- a/internal/terraform/node_resource_abstract_instance.go +++ b/internal/terraform/node_resource_abstract_instance.go @@ -2091,7 +2091,15 @@ func (n *NodeAbstractResourceInstance) evalApplyProvisioners(ctx EvalContext, st return nil } - provs := filterProvisioners(n.Config, when) + var allProvs []*configs.Provisioner + switch { + case n.Config != nil && n.Config.Managed != nil: + allProvs = n.Config.Managed.Provisioners + case n.RemovedConfig != nil && n.RemovedConfig.Managed != nil: + allProvs = n.RemovedConfig.Managed.Provisioners + } + + provs := filterProvisioners(allProvs, when) if len(provs) == 0 { // We have no provisioners, so don't do anything return nil @@ -2121,18 +2129,13 @@ func (n *NodeAbstractResourceInstance) evalApplyProvisioners(ctx EvalContext, st // filterProvisioners filters the provisioners on the resource to only // the provisioners specified by the "when" option. -func filterProvisioners(config *configs.Resource, when configs.ProvisionerWhen) []*configs.Provisioner { - // Fast path the zero case - if config == nil || config.Managed == nil { +func filterProvisioners(configured []*configs.Provisioner, when configs.ProvisionerWhen) []*configs.Provisioner { + if len(configured) == 0 { return nil } - if len(config.Managed.Provisioners) == 0 { - return nil - } - - result := make([]*configs.Provisioner, 0, len(config.Managed.Provisioners)) - for _, p := range config.Managed.Provisioners { + result := make([]*configs.Provisioner, 0, len(configured)) + for _, p := range configured { if p.When == when { result = append(result, p) } @@ -2161,8 +2164,11 @@ func (n *NodeAbstractResourceInstance) applyProvisioners(ctx EvalContext, state // then it'll serve as a base connection configuration for all of the // provisioners. var baseConn hcl.Body - if n.Config.Managed != nil && n.Config.Managed.Connection != nil { + switch { + case n.Config != nil && n.Config.Managed != nil && n.Config.Managed.Connection != nil: baseConn = n.Config.Managed.Connection.Config + case n.RemovedConfig != nil && n.RemovedConfig.Managed != nil && n.RemovedConfig.Managed.Connection != nil: + baseConn = n.RemovedConfig.Managed.Connection.Config } for _, prov := range provs { diff --git a/internal/terraform/testdata/apply-provisioner-destroy-removed/main.tf b/internal/terraform/testdata/apply-provisioner-destroy-removed/main.tf new file mode 100644 index 000000000000..a050d3ce6337 --- /dev/null +++ b/internal/terraform/testdata/apply-provisioner-destroy-removed/main.tf @@ -0,0 +1,8 @@ +removed { + from = aws_instance.foo + + provisioner "shell" { + when = "destroy" + command = "destroy ${each.key} ${self.foo}" + } +} diff --git a/internal/terraform/transform_attach_config_resource.go b/internal/terraform/transform_attach_config_resource.go index c582ad9fd0a9..cbd8d57f2e46 100644 --- a/internal/terraform/transform_attach_config_resource.go +++ b/internal/terraform/transform_attach_config_resource.go @@ -6,6 +6,7 @@ package terraform import ( "log" + "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/configs" "github.com/hashicorp/terraform/internal/dag" ) @@ -15,8 +16,11 @@ import ( type GraphNodeAttachResourceConfig interface { GraphNodeConfigResource - // Sets the configuration - AttachResourceConfig(*configs.Resource) + // Sets the configuration, either to a present resource block or to + // a "removed" block commemorating a resource that has since been + // removed. Callers should always leave at least one of these + // arguments set to nil. + AttachResourceConfig(*configs.Resource, *configs.Removed) } // AttachResourceConfigTransformer goes through the graph and attaches @@ -51,7 +55,7 @@ func (t *AttachResourceConfigTransformer) Transform(g *Graph) error { if r := config.Module.ResourceByAddr(addr.Resource); r != nil { log.Printf("[TRACE] AttachResourceConfigTransformer: attaching to %q (%T) config from %#v", dag.VertexName(v), v, r.DeclRange) - arn.AttachResourceConfig(r) + arn.AttachResourceConfig(r, nil) if gnapmc, ok := v.(GraphNodeAttachProviderMetaConfigs); ok { log.Printf("[TRACE] AttachResourceConfigTransformer: attaching provider meta configs to %s", dag.VertexName(v)) if config.Module.ProviderMetas != nil { @@ -61,6 +65,27 @@ func (t *AttachResourceConfigTransformer) Transform(g *Graph) error { } } } + + for _, r := range config.Module.Removed { + crAddr, ok := r.From.RelSubject.(addrs.ConfigResource) + if !ok { + // Not for a resource at all, so can't possibly match + continue + } + rAddr := crAddr.Resource + if rAddr != addr.Resource { + // Not the same resource + continue + } + + log.Printf("[TRACE] AttachResourceConfigTransformer: attaching to %q (%T) removed block from %#v", dag.VertexName(v), v, r.DeclRange) + + // Validation ensures that there can't be both a resource/data block + // and a removed block referring to the same configuration, so + // we can assume that this isn't clobbering a non-removed resource + // configuration we already attached above. + arn.AttachResourceConfig(nil, r) + } } return nil From ddd7ddd1ed2c38d84f6f70eff52dd0b650c953b1 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 22 May 2024 11:20:51 -0400 Subject: [PATCH 3/3] use try() in removed provisioner test This records in a test that both `count.index` and `each.key` will validate properly within a `removed` block provisioner. --- .../testdata/apply-provisioner-destroy-removed/main.tf | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/internal/terraform/testdata/apply-provisioner-destroy-removed/main.tf b/internal/terraform/testdata/apply-provisioner-destroy-removed/main.tf index a050d3ce6337..541fce304030 100644 --- a/internal/terraform/testdata/apply-provisioner-destroy-removed/main.tf +++ b/internal/terraform/testdata/apply-provisioner-destroy-removed/main.tf @@ -3,6 +3,10 @@ removed { provisioner "shell" { when = "destroy" - command = "destroy ${each.key} ${self.foo}" + + // Capture that we can reference either count.index or each.key from a + // removed block, and it's up to the user to ensure the provisioner is + // correct for the now removed resources. + command = "destroy ${try(count.index, each.key)} ${self.foo}" } }