simplify CBD transformation

Start by removing the DestroyEdge type altogether. This is only used to
detect the natural edge between a resource's create and destroy nodes,
but that's not necessary for any transformations. The custom edge type
also interferes with normal graph manipulations, because you can't
delete an arbitrary edge without knowing the type, so deletion of the
edge based only on the endpoints is often done incorrectly. The dag
package itself does this incorrectly in TransitiveReduction, which
always assumes the BasicEdge type.

Now that inter-resource destroy dependencies are already connected in the
DestroyEdgeTransformer (from the stored deps in state), there's no need
to search out all dependant resources in the CBD transformation, as they
should all be connected. This makes the CBD transformation rule quite
simple: reverse any edges from create nodes.

terraform/transform_destroy_cbd.go

@@ -145,14 +145,12 @@ func (t *CBDEdgeTransformer) Transform(g *Graph) error {
 	}
 
 	// Go through and reverse any destroy edges
-	destroyMap := make(map[string][]dag.Vertex)
 	for _, v := range g.Vertices() {
 		dn, ok := v.(GraphNodeDestroyerCBD)
 		if !ok {
 			continue
 		}
-		dern, ok := v.(GraphNodeDestroyer)
-		if !ok {
+		if _, ok = v.(GraphNodeDestroyer); !ok {
 			continue
 		}
 
@@ -162,158 +160,17 @@ func (t *CBDEdgeTransformer) Transform(g *Graph) error {
 
 		// Find the resource edges
 		for _, e := range g.EdgesTo(v) {
-			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()))
+			src := e.Source()
 
-				// 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)
-				}
+			// If source is a create node, invert the edge.
+			// This covers both the node's own creator, as well as reversing
+			// any dependants' edges.
+			if _, ok := src.(GraphNodeCreator); ok {
+				log.Printf("[TRACE] CBDEdgeTransformer: reversing edge %s -> %s", dag.VertexName(src), dag.VertexName(v))
+				g.RemoveEdge(e)
+				g.Connect(dag.BasicEdge(v, src))
 			}
 		}
-
-		// If the address has an index, we strip that. Our depMap creation
-		// graph doesn't expand counts so we don't currently get _exact_
-		// dependencies. One day when we limit dependencies more exactly
-		// this will have to change. We have a test case covering this
-		// (depNonCBDCountBoth) so it'll be caught.
-		addr := dern.DestroyAddr()
-		key := addr.ContainingResource().String()
-
-		// Add this to the list of nodes that we need to fix up
-		// the edges for (step 2 above in the docs).
-		destroyMap[key] = append(destroyMap[key], v)
 	}
-
-	// If we have no CBD nodes, then our work here is done
-	if len(destroyMap) == 0 {
-		return nil
-	}
-
-	// We have CBD nodes. We now have to move on to the much more difficult
-	// task of connecting dependencies of the creation side of the destroy
-	// to the destruction node. The easiest way to explain this is an example:
-	//
-	// Given a pre-destroy dependence of: A => B
-	//   And A has CBD set.
-	//
-	// The resulting graph should be: A => B => A_d
-	//
-	// They key here is that B happens before A is destroyed. This is to
-	// facilitate the primary purpose for CBD: making sure that downstreams
-	// are properly updated to avoid downtime before the resource is destroyed.
-	depMap, err := t.depMap(g, destroyMap)
-	if err != nil {
-		return err
-	}
-
-	// We now have the mapping of resource addresses to the destroy
-	// nodes they need to depend on. We now go through our own vertices to
-	// find any matching these addresses and make the connection.
-	for _, v := range g.Vertices() {
-		// We're looking for creators
-		rn, ok := v.(GraphNodeCreator)
-		if !ok {
-			continue
-		}
-
-		// Get the address
-		addr := rn.CreateAddr()
-
-		// If the address has an index, we strip that. Our depMap creation
-		// graph doesn't expand counts so we don't currently get _exact_
-		// dependencies. One day when we limit dependencies more exactly
-		// this will have to change. We have a test case covering this
-		// (depNonCBDCount) so it'll be caught.
-		key := addr.ContainingResource().String()
-
-		// If there is nothing this resource should depend on, ignore it
-		dns, ok := depMap[key]
-		if !ok {
-			continue
-		}
-
-		// We have nodes! Make the connection
-		for _, dn := range dns {
-			log.Printf("[TRACE] CBDEdgeTransformer: destroy depends on dependence: %s => %s",
-				dag.VertexName(dn), dag.VertexName(v))
-			g.Connect(dag.BasicEdge(dn, v))
-		}
-	}
-
 	return nil
 }
-
-func (t *CBDEdgeTransformer) depMap(g *Graph, destroyMap map[string][]dag.Vertex) (map[string][]dag.Vertex, error) {
-	// Build the list of destroy nodes that each resource address should depend
-	// on. For example, when we find B, we map the address of B to A_d in the
-	// "depMap" variable below.
-
-	// Use a nested map to remove duplicate edges.
-	depMap := make(map[string]map[dag.Vertex]struct{})
-	for _, v := range g.Vertices() {
-		// We're looking for resources.
-		rn, ok := v.(GraphNodeResource)
-		if !ok {
-			continue
-		}
-
-		// Get the address
-		addr := rn.ResourceAddr()
-		key := addr.String()
-
-		// Get the destroy nodes that are destroying this resource.
-		// If there aren't any, then we don't need to worry about
-		// any connections.
-		dns, ok := destroyMap[key]
-		if !ok {
-			continue
-		}
-
-		// Get the nodes that depend on this on. In the example above:
-		// finding B in A => B. Since dependencies can span modules, walk all
-		// descendents of the resource.
-		des, _ := g.Descendents(v)
-		for _, v := range des.List() {
-			// We're looking for resources.
-			rn, ok := v.(GraphNodeResource)
-			if !ok {
-				continue
-			}
-
-			// Keep track of the destroy nodes that this address
-			// needs to depend on.
-			key := rn.ResourceAddr().String()
-
-			deps, ok := depMap[key]
-			if !ok {
-				deps = make(map[dag.Vertex]struct{})
-			}
-
-			for _, d := range dns {
-				deps[d] = struct{}{}
-			}
-			depMap[key] = deps
-		}
-	}
-
-	result := map[string][]dag.Vertex{}
-	for k, m := range depMap {
-		for v := range m {
-			result[k] = append(result[k], v)
-		}
-	}
-
-	return result, nil
-}

terraform/transform_destroy_edge.go

@@ -151,7 +151,7 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error {
 				"[TRACE] DestroyEdgeTransformer: connecting creator %q with destroyer %q",
 				dag.VertexName(a), dag.VertexName(a_d))
 
-			g.Connect(&DestroyEdge{S: a, T: a_d})
+			g.Connect(dag.BasicEdge(a, a_d))
 
 			// Attach the destroy node to the creator
 			// There really shouldn't be more than one destroyer, but even if

terraform/transform_reference.go

@@ -205,7 +205,7 @@ func (t *DestroyValueReferenceTransformer) Transform(g *Graph) error {
 			log.Printf("[TRACE] output dep: %s", dag.VertexName(target))
 
 			g.RemoveEdge(e)
-			g.Connect(&DestroyEdge{S: target, T: v})
+			g.Connect(dag.BasicEdge(target, v))
 		}
 	}