From 07af1c6225647ed3f4f85f574db802b7855f4e78 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 1 Oct 2020 17:11:46 -0400 Subject: [PATCH] destroy time eval of resources pending deletion Allow the evaluation of resource pending deleting only during a full destroy. With this change we can ensure deposed instances are not evaluated under normal circumstances, but can be references when needed. This also allows us to remove the fixup transformer that added connections so temporary values would evaluate in the correct order when referencing destroy nodes. In the majority of cases, we do not want to evaluate resources that are pending deletion since configuration references only can refer to resources that is intended to be managed by the configuration. An exception to that rule is when Terraform is performing a full `destroy` operation, and providers need to evaluate existing resources for their configuration. --- terraform/evaluate.go | 21 +++--- terraform/graph_builder_apply.go | 4 -- terraform/transform_reference.go | 120 ------------------------------- 3 files changed, 9 insertions(+), 136 deletions(-) diff --git a/terraform/evaluate.go b/terraform/evaluate.go index 5f5c944e6..2b9211288 100644 --- a/terraform/evaluate.go +++ b/terraform/evaluate.go @@ -621,6 +621,11 @@ func (d *evaluationStateData) GetPathAttr(addr addrs.PathAttr, rng tfdiags.Sourc } } +// Try to guess if we have a pending destroy for all resources. +func (s *evaluationStateData) pendingDestroy() bool { + return true +} + func (d *evaluationStateData) GetResource(addr addrs.Resource, rng tfdiags.SourceRange) (cty.Value, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics // First we'll consult the configuration to see if an resource of this @@ -695,6 +700,7 @@ func (d *evaluationStateData) GetResource(addr addrs.Resource, rng tfdiags.Sourc // Decode all instances in the current state instances := map[addrs.InstanceKey]cty.Value{} + pendingDestroy := d.Evaluator.Changes.FullDestroy() for key, is := range rs.Instances { if is == nil || is.Current == nil { // Assume we're dealing with an instance that hasn't been created yet. @@ -711,18 +717,9 @@ func (d *evaluationStateData) GetResource(addr addrs.Resource, rng tfdiags.Sourc // instances will be in the state, as they are not destroyed until // after their dependants are updated. if change.Action == plans.Delete { - // FIXME: we should not be evaluating resources that are going - // to be destroyed, but this needs to happen always since - // destroy-time provisioners need to reference their self - // value, and providers need to evaluate their configuration - // during a full destroy, even of they depend on resources - // being destroyed. - // - // Since this requires a special transformer to try and fixup - // the order of evaluation when possible, reference it here to - // ensure that we remove the transformer when this is fixed. - _ = GraphTransformer((*applyDestroyNodeReferenceFixupTransformer)(nil)) - // continue + if !pendingDestroy { + continue + } } } diff --git a/terraform/graph_builder_apply.go b/terraform/graph_builder_apply.go index 2221c526c..b4e573142 100644 --- a/terraform/graph_builder_apply.go +++ b/terraform/graph_builder_apply.go @@ -185,10 +185,6 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { &CloseProviderTransformer{}, &CloseProvisionerTransformer{}, - // Add destroy node reference edges where needed, until we can fix - // full-destroy evaluation. - &applyDestroyNodeReferenceFixupTransformer{}, - // close the root module &CloseRootModuleTransformer{}, diff --git a/terraform/transform_reference.go b/terraform/transform_reference.go index 44aae0fc8..110612e14 100644 --- a/terraform/transform_reference.go +++ b/terraform/transform_reference.go @@ -540,123 +540,3 @@ func modulePrefixList(result []string, prefix string) []string { return result } - -// destroyNodeReferenceFixupTransformer is a GraphTransformer that connects all -// temporary values to any destroy instances of their references. This ensures -// that they are evaluated after the destroy operations of all instances, since -// the evaluator will currently return data from instances that are scheduled -// for deletion. -// -// This breaks the rules that destroy nodes are not referencable, and can cause -// cycles in the current graph structure. The cycles however are usually caused -// by passing through a provider node, and that is the specific case we do not -// want to wait for destroy evaluation since the evaluation result may need to -// be used in the provider for a full destroy operation. -// -// Once the evaluator can again ignore any instances scheduled for deletion, -// this transformer should be removed. -type applyDestroyNodeReferenceFixupTransformer struct{} - -func (t *applyDestroyNodeReferenceFixupTransformer) Transform(g *Graph) error { - // Create mapping of destroy nodes by address. - // Because the values which are providing the references won't yet be - // expanded, we need to index these by configuration address, rather than - // absolute. - destroyers := map[string][]dag.Vertex{} - for _, v := range g.Vertices() { - if v, ok := v.(GraphNodeDestroyer); ok { - addr := v.DestroyAddr().ContainingResource().Config().String() - destroyers[addr] = append(destroyers[addr], v) - } - } - _ = destroyers - - // nothing being destroyed - if len(destroyers) == 0 { - return nil - } - - // Now find any temporary values (variables, locals, outputs) that might - // reference the resources with instances being destroyed. - for _, v := range g.Vertices() { - rn, ok := v.(GraphNodeReferencer) - if !ok { - continue - } - - // we only want temporary value referencers - if _, ok := v.(graphNodeTemporaryValue); !ok { - continue - } - - modulePath := rn.ModulePath() - - // If this value is possibly consumed by a provider configuration, we - // must attempt to evaluate early during a full destroy, and cannot - // wait on the resource destruction. This would also likely cause a - // cycle in most configurations. - des, _ := g.Descendents(rn) - providerDescendant := false - for _, v := range des { - if _, ok := v.(GraphNodeProvider); ok { - providerDescendant = true - break - } - } - - if providerDescendant { - log.Printf("[WARN] Value %q has provider descendant, not waiting on referenced destroy instance", dag.VertexName(rn)) - continue - } - - refs := rn.References() - for _, ref := range refs { - - var addr addrs.ConfigResource - // get the configuration level address for this reference, since - // that is how we indexed the destroyers - switch tr := ref.Subject.(type) { - case addrs.Resource: - addr = addrs.ConfigResource{ - Module: modulePath, - Resource: tr, - } - case addrs.ResourceInstance: - addr = addrs.ConfigResource{ - Module: modulePath, - Resource: tr.ContainingResource(), - } - default: - // this is not a resource reference - continue - } - - // see if there are any destroyers registered for this address - for _, dest := range destroyers[addr.String()] { - // check that we are not introducing a cycle, by looking for - // our own node in the ancestors of the destroy node. - // This should theoretically only happen if we had a provider - // descendant which was checked already, but since this edge is - // being added outside the normal rules of the graph, check - // again to be certain. - anc, _ := g.Ancestors(dest) - cycle := false - for _, a := range anc { - if a == rn { - log.Printf("[WARN] Not adding fixup edge %q->%q which introduces a cycle", dag.VertexName(rn), dag.VertexName(dest)) - cycle = true - break - } - } - if cycle { - continue - } - - log.Printf("[DEBUG] adding fixup edge %q->%q to prevent destroy node evaluation", dag.VertexName(rn), dag.VertexName(dest)) - g.Connect(dag.BasicEdge(rn, dest)) - } - } - } - - return nil -}