From 682083cdd185abdd7efb72c3d018ef59b570eff4 Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Sun, 17 Nov 2019 09:56:44 -0500
Subject: [PATCH] Creators cannot depend directly on CBD dest nodes

Since a create node cannot both depend on its destroy node AND be
CreateBeforeDestroy, the same goes for its dependencies. While we do
connect resources with dependency destroy nodes so that updates are
ordered correctly, this ordering does not make sense in the
CreateBeforeDestroy case.

If resource node is CreateBeforeDestroy, we need to remove any direct
dependencies from it to destroy nodes to prevent cycles. Since we don't
know for certain if a crate node is going to be CreateBeforeDestroy at
the time the edge is added in the graph, we add it unconditionally and
prune it out later on. The pruning happens during the CBD transformer
when the CBD destroy node reverses it's own destroy edge.  The reason
this works for detecting the original edge, is that dependencies of CBD
resources are forced to be CBD themselves. This does have a false
positive where the case of the original node is NOT CBD, but this can be
taken care of later when we gather enough information in the graph to
prevent the connection in the first place.
---
 terraform/transform_destroy_cbd.go  | 40 +++++++++++++++++------------
 terraform/transform_destroy_edge.go |  6 +++++
 2 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/terraform/transform_destroy_cbd.go b/terraform/transform_destroy_cbd.go
index 21b75c141..5945eeadd 100644
--- a/terraform/transform_destroy_cbd.go
+++ b/terraform/transform_destroy_cbd.go
@@ -160,23 +160,27 @@ func (t *CBDEdgeTransformer) Transform(g *Graph) error {
 			continue
 		}
 
-		// Find the destroy edge. There should only be one.
+		// Find the resource edges
 		for _, e := range g.EdgesTo(v) {
-			// Not a destroy edge, ignore it
-			de, ok := e.(*DestroyEdge)
-			if !ok {
-				continue
+			switch de := e.(type) {
+			case *DestroyEdge:
+				// we need to invert the destroy edge from the create node
+				log.Printf("[TRACE] CBDEdgeTransformer: inverting edge: %s => %s",
+					dag.VertexName(de.Source()), dag.VertexName(de.Target()))
+
+				// Found it! Invert.
+				g.RemoveEdge(de)
+				applyNode := de.Source()
+				destroyNode := de.Target()
+				g.Connect(&DestroyEdge{S: destroyNode, T: applyNode})
+			default:
+				// We cannot have any direct dependencies from creators when
+				// the node is CBD without inducing a cycle.
+				if _, ok := e.Source().(GraphNodeCreator); ok {
+					log.Printf("[TRACE] CBDEdgeTransformer: removing non DestroyEdge to CBD destroy node: %s => %s", dag.VertexName(e.Source()), dag.VertexName(e.Target()))
+					g.RemoveEdge(e)
+				}
 			}
-
-			log.Printf("[TRACE] CBDEdgeTransformer: inverting edge: %s => %s",
-				dag.VertexName(de.Source()), dag.VertexName(de.Target()))
-
-			// Found it! Invert.
-			g.RemoveEdge(de)
-			applyNode := de.Source()
-			destroyNode := de.Target()
-			g.Connect(&DestroyEdge{S: destroyNode, T: applyNode})
-			break
 		}
 
 		// If the address has an index, we strip that. Our depMap creation
@@ -289,7 +293,11 @@ func (t *CBDEdgeTransformer) depMap(g *Graph, destroyMap map[string][]dag.Vertex
 			// Keep track of the destroy nodes that this address
 			// needs to depend on.
 			key := rn.ResourceAddr().String()
-			depMap[key] = append(depMap[key], dns...)
+
+			// we only need to record the same dns once
+			if _, ok := depMap[key]; !ok {
+				depMap[key] = append(depMap[key], dns...)
+			}
 		}
 	}
 
diff --git a/terraform/transform_destroy_edge.go b/terraform/transform_destroy_edge.go
index c25bdce7e..df3872a1d 100644
--- a/terraform/transform_destroy_edge.go
+++ b/terraform/transform_destroy_edge.go
@@ -122,6 +122,12 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error {
 
 		for _, resAddr := range ri.StateDependencies() {
 			for _, desDep := range destroyersByResource[resAddr.String()] {
+				// TODO: don't connect this if c is CreateBeforeDestroy.
+				//       This will require getting the actual change action at
+				//       this point, since the lifecycle may have been forced
+				//       by a dependent. This should prevent needing to prune
+				//       the edge back out in CBDEdgeTransformer, and allow
+				//       non-CBD nodes to depend on CBD destroys directly.
 				log.Printf("[TRACE] DestroyEdgeTransformer: %s has stored dependency of %s\n", dag.VertexName(c), dag.VertexName(desDep))
 				g.Connect(dag.BasicEdge(c, desDep))