From ff21cc3c8d4d90defdb7b475eb9bfcf28e9ce707 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 9 Oct 2020 17:24:10 -0400 Subject: [PATCH] remove the need for destroyRootOutputTransformer Since root outputs can now use the planned changes, we can directly insert the correct applyable or destroyable node into the graph during plan and apply, and it will remove the outputs if they are being destroyed. --- terraform/graph_builder_apply.go | 11 --- terraform/graph_builder_plan_test.go | 4 +- terraform/node_output.go | 43 +++++------ terraform/node_resource_apply.go | 3 +- terraform/transform_output.go | 102 +++++++++++++-------------- terraform/transform_targets_test.go | 4 +- 6 files changed, 73 insertions(+), 94 deletions(-) diff --git a/terraform/graph_builder_apply.go b/terraform/graph_builder_apply.go index 29f4625483..5fc2635167 100644 --- a/terraform/graph_builder_apply.go +++ b/terraform/graph_builder_apply.go @@ -150,17 +150,6 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { Schemas: b.Schemas, }, - // Create a destroy node for root outputs to remove them from the - // state. This does nothing unless invoked via the destroy command - // directly. A destroy is identical to a normal apply, except for the - // fact that we also have configuration to evaluate. While the rest of - // the unused nodes can be programmatically pruned (via - // pruneUnusedNodesTransformer), root module outputs always have an - // implied dependency on remote state. This means that if they exist in - // the configuration, the only signal to remove them is via the destroy - // command itself. - &destroyRootOutputTransformer{Destroy: b.Destroy}, - // We need to remove configuration nodes that are not used at all, as // they may not be able to evaluate, especially during destroy. // These include variables, locals, and instance expanders. diff --git a/terraform/graph_builder_plan_test.go b/terraform/graph_builder_plan_test.go index 956b147ea9..84fec87403 100644 --- a/terraform/graph_builder_plan_test.go +++ b/terraform/graph_builder_plan_test.go @@ -288,10 +288,10 @@ local.instance_id (expand) aws_instance.web (expand) meta.count-boundary (EachMode fixup) aws_load_balancer.weblb (expand) - output.instance_id (expand) + output.instance_id openstack_floating_ip.random (expand) provider["registry.terraform.io/hashicorp/openstack"] -output.instance_id (expand) +output.instance_id local.instance_id (expand) provider["registry.terraform.io/hashicorp/aws"] openstack_floating_ip.random (expand) diff --git a/terraform/node_output.go b/terraform/node_output.go index 07d8ff3354..a9a667bd7f 100644 --- a/terraform/node_output.go +++ b/terraform/node_output.go @@ -15,8 +15,8 @@ import ( "github.com/zclconf/go-cty/cty" ) -// nodeExpandOutput is the placeholder for an output that has not yet had -// its module path expanded. +// nodeExpandOutput is the placeholder for a non-root module output that has +// not yet had its module path expanded. type nodeExpandOutput struct { Addr addrs.OutputValue Module addrs.Module @@ -37,17 +37,21 @@ var ( func (n *nodeExpandOutput) expandsInstances() {} func (n *nodeExpandOutput) temporaryValue() bool { - // this must always be evaluated if it is a root module output + // non root outputs are temporary return !n.Module.IsRoot() } func (n *nodeExpandOutput) DynamicExpand(ctx EvalContext) (*Graph, error) { if n.Destroy { - return n.planDestroyOutputs(ctx) + // if we're planning a destroy, we only need to handle the root outputs. + // The destroy plan doesn't evaluate any other config, so we can skip + // the rest of the outputs. + return n.planDestroyRootOutput(ctx) } - var g Graph expander := ctx.InstanceExpander() + + var g Graph for _, module := range expander.ExpandModule(n.Module) { absAddr := n.Addr.Absolute(module) @@ -71,32 +75,23 @@ func (n *nodeExpandOutput) DynamicExpand(ctx EvalContext) (*Graph, error) { return &g, nil } -// if we're planing a destroy operation, add destroy nodes for all root outputs -// in the state. -func (n *nodeExpandOutput) planDestroyOutputs(ctx EvalContext) (*Graph, error) { - // we only need to plan destroying root outputs - // Other module outputs may be used during destruction by providers that - // need to interpolate values. +// if we're planing a destroy operation, add a destroy node for any root output +func (n *nodeExpandOutput) planDestroyRootOutput(ctx EvalContext) (*Graph, error) { if !n.Module.IsRoot() { return nil, nil } - state := ctx.State() if state == nil { return nil, nil } - ms := state.Module(addrs.RootModuleInstance) - var g Graph - for _, output := range ms.OutputValues { - o := &NodeDestroyableOutput{ - Addr: output.Addr, - Config: n.Config, - } - log.Printf("[TRACE] Expanding output: adding %s as %T", o.Addr.String(), o) - g.Add(o) + o := &NodeDestroyableOutput{ + Addr: n.Addr.Absolute(addrs.RootModuleInstance), + Config: n.Config, } + log.Printf("[TRACE] Expanding output: adding %s as %T", o.Addr.String(), o) + g.Add(o) return &g, nil } @@ -149,6 +144,8 @@ func (n *nodeExpandOutput) ReferenceOutside() (selfPath, referencePath addrs.Mod // GraphNodeReferencer func (n *nodeExpandOutput) References() []*addrs.Reference { + // root outputs might be destroyable, and may not reference anything in + // that case return referencesForOutput(n.Config) } @@ -364,8 +361,6 @@ func (n *NodeDestroyableOutput) Execute(ctx EvalContext, op walkOperation) error change := &plans.OutputChange{ Addr: n.Addr, Change: plans.Change{ - // This is just a weird placeholder delete action since - // we don't have an actual prior value to indicate. // FIXME: Generate real planned changes for output values // that include the old values. Action: plans.Delete, @@ -379,7 +374,7 @@ func (n *NodeDestroyableOutput) Execute(ctx EvalContext, op walkOperation) error // Should never happen, since we just constructed this right above panic(fmt.Sprintf("planned change for %s could not be encoded: %s", n.Addr, err)) } - log.Printf("[TRACE] planDestroyOutput: Saving %s change for %s in changeset", change.Action, n.Addr) + log.Printf("[TRACE] NodeDestroyableOutput: Saving %s change for %s in changeset", change.Action, n.Addr) changes.RemoveOutputChange(n.Addr) // remove any existing planned change, if present changes.AppendOutputChange(cs) // add the new planned change } diff --git a/terraform/node_resource_apply.go b/terraform/node_resource_apply.go index 1994563fa8..ff09ba6145 100644 --- a/terraform/node_resource_apply.go +++ b/terraform/node_resource_apply.go @@ -26,7 +26,8 @@ var ( _ GraphNodeTargetable = (*nodeExpandApplyableResource)(nil) ) -func (n *nodeExpandApplyableResource) expandsInstances() {} +func (n *nodeExpandApplyableResource) expandsInstances() { +} func (n *nodeExpandApplyableResource) References() []*addrs.Reference { return (&NodeApplyableResource{NodeAbstractResource: n.NodeAbstractResource}).References() diff --git a/terraform/transform_output.go b/terraform/transform_output.go index f0f33aa2c8..7685666ce3 100644 --- a/terraform/transform_output.go +++ b/terraform/transform_output.go @@ -43,73 +43,67 @@ func (t *OutputTransformer) transform(g *Graph, c *configs.Config) error { } } - // Add plannable outputs to the graph, which will be dynamically expanded + // Add outputs to the graph, which will be dynamically expanded // into NodeApplyableOutputs to reflect possible expansion // through the presence of "count" or "for_each" on the modules. + // if this is a root output, we add the apply or destroy node directly, as + // the root modules does not expand var changes []*plans.OutputChangeSrc if t.Changes != nil { changes = t.Changes.Outputs } for _, o := range c.Module.Outputs { - node := &nodeExpandOutput{ - Addr: addrs.OutputValue{Name: o.Name}, - Module: c.Path, - Config: o, - Changes: changes, - Destroy: t.Destroy, + addr := addrs.OutputValue{Name: o.Name} + + var rootChange *plans.OutputChangeSrc + for _, c := range changes { + if c.Addr.Module.IsRoot() && c.Addr.OutputValue.Name == o.Name { + rootChange = c + } } + + var node dag.Vertex + switch { + case c.Path.IsRoot() && t.Destroy: + node = &NodeDestroyableOutput{ + Addr: addr.Absolute(addrs.RootModuleInstance), + Config: o, + } + case c.Path.IsRoot(): + destroy := t.Destroy + if rootChange != nil { + destroy = rootChange.Action == plans.Delete + } + + switch { + case destroy: + node = &NodeDestroyableOutput{ + Addr: addr.Absolute(addrs.RootModuleInstance), + Config: o, + } + default: + node = &NodeApplyableOutput{ + Addr: addr.Absolute(addrs.RootModuleInstance), + Config: o, + Change: rootChange, + } + } + + default: + node = &nodeExpandOutput{ + Addr: addr, + Module: c.Path, + Config: o, + Changes: changes, + Destroy: t.Destroy, + } + } + log.Printf("[TRACE] OutputTransformer: adding %s as %T", o.Name, node) g.Add(node) } return nil } - -// destroyRootOutputTransformer is a GraphTransformer that adds nodes to delete -// outputs during destroy. We need to do this to ensure that no stale outputs -// are ever left in the state. -type destroyRootOutputTransformer struct { - Destroy bool -} - -func (t *destroyRootOutputTransformer) Transform(g *Graph) error { - // Only clean root outputs on a full destroy - if !t.Destroy { - return nil - } - - for _, v := range g.Vertices() { - output, ok := v.(*nodeExpandOutput) - if !ok { - continue - } - - // We only destroy root outputs - if !output.Module.Equal(addrs.RootModule) { - continue - } - - // create the destroy node for this output - node := &NodeDestroyableOutput{ - Addr: output.Addr.Absolute(addrs.RootModuleInstance), - Config: output.Config, - } - - log.Printf("[TRACE] creating %s", node.Name()) - g.Add(node) - - deps := g.UpEdges(v) - - for _, d := range deps { - log.Printf("[TRACE] %s depends on %s", node.Name(), dag.VertexName(d)) - g.Connect(dag.BasicEdge(node, d)) - } - - // We no longer need the expand node, since we intend to remove this - // output from the state. - g.Remove(v) - } - return nil -} diff --git a/terraform/transform_targets_test.go b/terraform/transform_targets_test.go index b38be835c7..9339b913ec 100644 --- a/terraform/transform_targets_test.go +++ b/terraform/transform_targets_test.go @@ -122,7 +122,7 @@ module.child.module.grandchild.output.id (expand) module.child.module.grandchild.aws_instance.foo module.child.output.grandchild_id (expand) module.child.module.grandchild.output.id (expand) -output.grandchild_id (expand) +output.grandchild_id module.child.output.grandchild_id (expand) `) if actual != expected { @@ -193,7 +193,7 @@ module.child.module.grandchild.output.id (expand) module.child.module.grandchild.aws_instance.foo module.child.output.grandchild_id (expand) module.child.module.grandchild.output.id (expand) -output.grandchild_id (expand) +output.grandchild_id module.child.output.grandchild_id (expand) `) if actual != expected {