From 009a136fa2b299b755e4bca374100e2653a99c6c Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 18 May 2020 20:59:17 -0400 Subject: [PATCH] requiresInstanceExpansion and instanceExpander create interfaces that nodes can implement to declare whether they expand into instances of some sort, using the instances.Expander, and/or whether use the instances.Expander to find instances. included is a rough transformer implementation to remove these nodes from the apply graph. --- terraform/graph_builder_apply.go | 4 +- terraform/instance_expanders.go | 19 +++ terraform/node_local.go | 8 ++ terraform/node_module_expand.go | 16 ++- terraform/node_module_variable.go | 8 ++ terraform/node_output.go | 9 +- terraform/node_resource_apply.go | 12 ++ terraform/node_resource_apply_instance.go | 4 + terraform/transform_destroy_edge.go | 166 ++++++++++++++++------ terraform/transform_module_expansion.go | 6 +- terraform/transform_reference.go | 3 +- 11 files changed, 205 insertions(+), 50 deletions(-) create mode 100644 terraform/instance_expanders.go diff --git a/terraform/graph_builder_apply.go b/terraform/graph_builder_apply.go index 86ea86e05..3736fd3d7 100644 --- a/terraform/graph_builder_apply.go +++ b/terraform/graph_builder_apply.go @@ -165,9 +165,7 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { // Prune unreferenced values, which may have interpolations that can't // be resolved. - &PruneUnusedValuesTransformer{ - Destroy: b.Destroy, - }, + &pruneUnusedExpanderTransformer{}, // Add the node to fix the state count boundaries &CountBoundaryTransformer{ diff --git a/terraform/instance_expanders.go b/terraform/instance_expanders.go new file mode 100644 index 000000000..7dd5496a0 --- /dev/null +++ b/terraform/instance_expanders.go @@ -0,0 +1,19 @@ +package terraform + +import "github.com/hashicorp/terraform/addrs" + +// instanceExpander is implemented by nodes that causes instances to be +// registered in the instances.Expander. +// This is used to determine during apply whether a node is required to be in +// the graph, by checking if it has any requiresInstanceExpansion dependents. +// This prevents unnecessary nodes from being evaluated, and if the module is +// being removed, we may not be able to evaluate the expansion at all. +type instanceExpander interface { + expandsInstances() addrs.Module +} + +// requiresInstanceExpansion is implemented by nodes that require their address +// be previously registered in the instances.Expander in order to evaluate. +type requiresInstanceExpansion interface { + requiresExpansion() +} diff --git a/terraform/node_local.go b/terraform/node_local.go index a3a635eba..d0a15c7ff 100644 --- a/terraform/node_local.go +++ b/terraform/node_local.go @@ -23,8 +23,16 @@ var ( _ GraphNodeReferencer = (*nodeExpandLocal)(nil) _ GraphNodeDynamicExpandable = (*nodeExpandLocal)(nil) _ graphNodeTemporaryValue = (*nodeExpandLocal)(nil) + _ requiresInstanceExpansion = (*nodeExpandLocal)(nil) ) +func (n *nodeExpandLocal) expandsInstances() addrs.Module { + return n.Module +} + +// requiresInstanceExpansion implementation +func (n *nodeExpandLocal) requiresExpansion() {} + // graphNodeTemporaryValue func (n *nodeExpandLocal) temporaryValue() bool { return true diff --git a/terraform/node_module_expand.go b/terraform/node_module_expand.go index 08961e9e7..dac3b9085 100644 --- a/terraform/node_module_expand.go +++ b/terraform/node_module_expand.go @@ -31,8 +31,21 @@ var ( _ RemovableIfNotTargeted = (*nodeExpandModule)(nil) _ GraphNodeEvalable = (*nodeExpandModule)(nil) _ GraphNodeReferencer = (*nodeExpandModule)(nil) + + // modules both record their expansion, and require expansion from their + // parent modules. + _ requiresInstanceExpansion = (*nodeExpandModule)(nil) + _ instanceExpander = (*nodeExpandModule)(nil) ) +// requiresInstanceExpansion implementation +func (n *nodeExpandModule) requiresExpansion() {} + +// instanceExander implementation +func (n *nodeExpandModule) expandsInstances() addrs.Module { + return n.Addr +} + func (n *nodeExpandModule) Name() string { return n.Addr.String() + " (expand)" } @@ -125,8 +138,7 @@ var ( ) func (n *nodeCloseModule) ModulePath() addrs.Module { - mod, _ := n.Addr.Call() - return mod + return n.Addr } func (n *nodeCloseModule) ReferenceableAddrs() []addrs.Referenceable { diff --git a/terraform/node_module_variable.go b/terraform/node_module_variable.go index 706b22e49..6d16ef90a 100644 --- a/terraform/node_module_variable.go +++ b/terraform/node_module_variable.go @@ -27,8 +27,16 @@ var ( _ GraphNodeReferencer = (*nodeExpandModuleVariable)(nil) _ graphNodeTemporaryValue = (*nodeExpandModuleVariable)(nil) _ RemovableIfNotTargeted = (*nodeExpandModuleVariable)(nil) + _ requiresInstanceExpansion = (*nodeExpandModuleVariable)(nil) ) +// requiresInstanceExpansion implementation +func (n *nodeExpandModuleVariable) requiresExpansion() {} + +func (n *nodeExpandModuleVariable) expandsInstances() addrs.Module { + return n.Module +} + func (n *nodeExpandModuleVariable) temporaryValue() bool { return true } diff --git a/terraform/node_output.go b/terraform/node_output.go index 4df5b7246..75e438986 100644 --- a/terraform/node_output.go +++ b/terraform/node_output.go @@ -23,9 +23,16 @@ var ( _ GraphNodeReferenceable = (*nodeExpandOutput)(nil) _ GraphNodeReferencer = (*nodeExpandOutput)(nil) _ GraphNodeDynamicExpandable = (*nodeExpandOutput)(nil) - _ graphNodeTemporaryValue = (*NodeApplyableOutput)(nil) + _ graphNodeTemporaryValue = (*nodeExpandOutput)(nil) + _ requiresInstanceExpansion = (*nodeExpandOutput)(nil) ) +func (n *nodeExpandOutput) expandsInstances() addrs.Module { + return n.Module +} + +func (m *nodeExpandOutput) requiresExpansion() {} + func (n *nodeExpandOutput) temporaryValue() bool { // this must always be evaluated if it is a root module output return !n.Module.IsRoot() diff --git a/terraform/node_resource_apply.go b/terraform/node_resource_apply.go index d4330a166..b4de20c01 100644 --- a/terraform/node_resource_apply.go +++ b/terraform/node_resource_apply.go @@ -22,8 +22,20 @@ var ( _ GraphNodeReferencer = (*nodeExpandApplyableResource)(nil) _ GraphNodeConfigResource = (*nodeExpandApplyableResource)(nil) _ GraphNodeAttachResourceConfig = (*nodeExpandApplyableResource)(nil) + + // Resource both expand instances and require module path expansion. + _ requiresInstanceExpansion = (*nodeExpandApplyableResource)(nil) + _ instanceExpander = (*nodeExpandApplyableResource)(nil) ) +// requiresInstanceExpansion implementation +func (n *nodeExpandApplyableResource) requiresExpansion() {} + +// instanceExpander implementation +func (n *nodeExpandApplyableResource) expandsInstances() addrs.Module { + return n.ModulePath() +} + func (n *nodeExpandApplyableResource) References() []*addrs.Reference { return (&NodeApplyableResource{NodeAbstractResource: n.NodeAbstractResource}).References() } diff --git a/terraform/node_resource_apply_instance.go b/terraform/node_resource_apply_instance.go index 1282f8a11..45fa33ccf 100644 --- a/terraform/node_resource_apply_instance.go +++ b/terraform/node_resource_apply_instance.go @@ -37,8 +37,12 @@ var ( _ GraphNodeDeposer = (*NodeApplyableResourceInstance)(nil) _ GraphNodeEvalable = (*NodeApplyableResourceInstance)(nil) _ GraphNodeAttachDependencies = (*NodeApplyableResourceInstance)(nil) + _ requiresInstanceExpansion = (*NodeApplyableResourceInstance)(nil) ) +// requiresInstanceExpansion implementation +func (n *NodeApplyableResourceInstance) requiresExpansion() {} + // GraphNodeAttachDestroyer func (n *NodeApplyableResourceInstance) AttachDestroyNode(d GraphNodeDestroyerCBD) { n.destroyNode = d diff --git a/terraform/transform_destroy_edge.go b/terraform/transform_destroy_edge.go index 7c926a070..7f54f0c3e 100644 --- a/terraform/transform_destroy_edge.go +++ b/terraform/transform_destroy_edge.go @@ -2,6 +2,7 @@ package terraform import ( "log" + "sort" "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/states" @@ -163,46 +164,129 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error { } } - return t.pruneResources(g) -} - -// If there are only destroy instances for a particular resource, there's no -// reason for the resource node to prepare the state. Remove Resource nodes so -// that they don't fail by trying to evaluate a resource that is only being -// destroyed along with its dependencies. -func (t *DestroyEdgeTransformer) pruneResources(g *Graph) error { - for _, v := range g.Vertices() { - n, ok := v.(*nodeExpandApplyableResource) - if !ok { - continue - } - - // if there are only destroy dependencies, we don't need this node - descendents, err := g.Descendents(n) - if err != nil { - return err - } - - nonDestroyInstanceFound := false - for _, v := range descendents { - if _, ok := v.(*NodeApplyableResourceInstance); ok { - nonDestroyInstanceFound = true - break - } - } - - if nonDestroyInstanceFound { - continue - } - - // connect all the through-edges, then delete the node - for _, d := range g.DownEdges(n) { - for _, u := range g.UpEdges(n) { - g.Connect(dag.BasicEdge(u, d)) - } - } - log.Printf("DestroyEdgeTransformer: pruning unused resource node %s", dag.VertexName(n)) - g.Remove(n) - } return nil } + +// Nodes that register instances in the instances.Expander are not needed +// during apply if there are no instances that will lookup the expansion. This +// is the case when a module tree is removed or during a full destroy, and we +// may not be able to evaluate the expansion expression. +type pruneUnusedExpanderTransformer struct { +} + +func (t *pruneUnusedExpanderTransformer) Transform(g *Graph) error { + // We need a reverse depth first walk of modules, but it needs to be + // recursive so that we can process the lead modules first. + + // collect all nodes into their containing module + type mod struct { + addr addrs.Module + nodes []dag.Vertex + } + + // first collect the nodes into their respective modules + moduleMap := make(map[string]*mod) + for _, v := range g.Vertices() { + var path addrs.Module + switch v := v.(type) { + case instanceExpander: + path = v.expandsInstances() + + case graphNodeModuleCloser: + // module closers are connected like module calls, and report + // their parent module address + path = v.CloseModule() + + case GraphNodeModulePath: + path = v.ModulePath() + } + m, ok := moduleMap[path.String()] + if !ok { + m = &mod{} + moduleMap[path.String()] = m + } + + m.addr = path + m.nodes = append(m.nodes, v) + } + + // now we need to restructure the modules so we can sort them + var modules []*mod + + for _, mod := range moduleMap { + modules = append(modules, mod) + } + + // Sort them by path length, longest first, so that we process the deepest + // modules first. The order of modules at the same tree level doesn't + // matter, we just need to ensure that child modules are processed before + // parent modules. + sort.Slice(modules, func(i, j int) bool { + return len(modules[i].addr) > len(modules[j].addr) + }) + + for _, module := range modules { + t.removeUnused(module.nodes, g) + } + + return nil +} + +func (t *pruneUnusedExpanderTransformer) removeUnused(nodes []dag.Vertex, g *Graph) { + // since we have no defined structure within the module, just cycle through + // the nodes until there are no more removals + removed := true + for { + if !removed { + return + } + removed = false + + last := len(nodes) - 1 + + NEXT: + for i := 0; i < len(nodes); i++ { + n := nodes[i] + switch n.(type) { + case graphNodeTemporaryValue: + if n, ok := n.(GraphNodeModulePath); ok { + // root outputs always have a dependency on remote state + if n.ModulePath().IsRoot() { + continue NEXT + } + } + for _, vv := range g.UpEdges(n) { + if _, ok := vv.(GraphNodeReferencer); ok { + continue NEXT + } + } + + case instanceExpander: + for _, vv := range g.UpEdges(n) { + if _, ok := vv.(requiresInstanceExpansion); ok { + continue NEXT + } + } + + default: + continue NEXT + } + + removed = true + + //// connect through edges + //for _, d := range g.DownEdges(n) { + // for _, u := range g.UpEdges(n) { + // g.Connect(dag.BasicEdge(u, d)) + // } + //} + + g.Remove(n) + + // remove the node from our iteration as well + nodes[i], nodes[last] = nodes[last], nodes[i] + nodes = nodes[:last] + last-- + } + } +} diff --git a/terraform/transform_module_expansion.go b/terraform/transform_module_expansion.go index f26bc8385..41484cb6d 100644 --- a/terraform/transform_module_expansion.go +++ b/terraform/transform_module_expansion.go @@ -42,6 +42,10 @@ func (t *ModuleExpansionTransformer) Transform(g *Graph) error { // handled by the RemovedModuleTransformer, and those module closers are in // the graph already, and need to be connected to their parent closers. for _, v := range g.Vertices() { + // skip closers so they don't attach to themselves + if _, ok := v.(graphNodeModuleCloser); ok { + continue + } // any node that executes within the scope of a module should be a // GraphNodeModulePath pather, ok := v.(GraphNodeModulePath) @@ -49,7 +53,7 @@ func (t *ModuleExpansionTransformer) Transform(g *Graph) error { continue } if closer, ok := t.closers[pather.ModulePath().String()]; ok { - // The module root depends on each child resource instance, since + // The module closer depends on each child resource instance, since // during apply the module expansion will complete before the // individual instances are applied. g.Connect(dag.BasicEdge(closer, v)) diff --git a/terraform/transform_reference.go b/terraform/transform_reference.go index 13f02b818..7fef2e8ee 100644 --- a/terraform/transform_reference.go +++ b/terraform/transform_reference.go @@ -259,7 +259,6 @@ func (t AttachDependenciesTransformer) Transform(g *Graph) error { // values reference a resource that is no longer in the state the interpolation // could fail. type PruneUnusedValuesTransformer struct { - Destroy bool } func (t *PruneUnusedValuesTransformer) Transform(g *Graph) error { @@ -282,7 +281,7 @@ func (t *PruneUnusedValuesTransformer) Transform(g *Graph) error { // any referencers in the dependents means we need to keep this // value for evaluation removable := true - for _, d := range dependants.List() { + for _, d := range dependants { if _, ok := d.(GraphNodeReferencer); ok { removable = false break