From 8f359847911953b839f23c57226ca6ecd7c6cffb Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 25 Sep 2019 21:31:17 -0400 Subject: [PATCH] Use Descendants rather than UpEdges in CBD When looking for dependencies to fix when handling create_before_destroy, we need to look past more than one edge, as dependencies may appear transitively through outputs and variables. Use Descendants rather than UpEdges. We have the full graph to use for the CBD transformation, so there's no longer any need to create a temporary graph, which may differ from the original. --- terraform/transform_config_flat.go | 71 ------------------------- terraform/transform_config_flat_test.go | 42 --------------- terraform/transform_destroy_cbd.go | 38 ++++--------- 3 files changed, 10 insertions(+), 141 deletions(-) delete mode 100644 terraform/transform_config_flat.go delete mode 100644 terraform/transform_config_flat_test.go diff --git a/terraform/transform_config_flat.go b/terraform/transform_config_flat.go deleted file mode 100644 index 866c91759..000000000 --- a/terraform/transform_config_flat.go +++ /dev/null @@ -1,71 +0,0 @@ -package terraform - -import ( - "github.com/hashicorp/terraform/configs" - "github.com/hashicorp/terraform/dag" -) - -// FlatConfigTransformer is a GraphTransformer that adds the configuration -// to the graph. The module used to configure this transformer must be -// the root module. -// -// This transform adds the nodes but doesn't connect any of the references. -// The ReferenceTransformer should be used for that. -// -// NOTE: In relation to ConfigTransformer: this is a newer generation config -// transformer. It puts the _entire_ config into the graph (there is no -// "flattening" step as before). -type FlatConfigTransformer struct { - Concrete ConcreteResourceNodeFunc // What to turn resources into - - Config *configs.Config -} - -func (t *FlatConfigTransformer) Transform(g *Graph) error { - // We have nothing to do if there is no configuration. - if t.Config == nil { - return nil - } - - return t.transform(g, t.Config) -} - -func (t *FlatConfigTransformer) transform(g *Graph, config *configs.Config) error { - // If we have no configuration then there's nothing to do. - if config == nil { - return nil - } - - // Transform all the children. - for _, c := range config.Children { - if err := t.transform(g, c); err != nil { - return err - } - } - - module := config.Module - // For now we assume that each module call produces only one module - // instance with no key, since we don't yet support "count" and "for_each" - // on modules. - // FIXME: As part of supporting "count" and "for_each" on modules, rework - // this so that we'll "expand" the module call first and then create graph - // nodes for each module instance separately. - instPath := config.Path.UnkeyedInstanceShim() - - for _, r := range module.ManagedResources { - addr := r.Addr().Absolute(instPath) - abstract := &NodeAbstractResource{ - Addr: addr, - Config: r, - } - // Grab the address for this resource - var node dag.Vertex = abstract - if f := t.Concrete; f != nil { - node = f(abstract) - } - - g.Add(node) - } - - return nil -} diff --git a/terraform/transform_config_flat_test.go b/terraform/transform_config_flat_test.go deleted file mode 100644 index 79fc09367..000000000 --- a/terraform/transform_config_flat_test.go +++ /dev/null @@ -1,42 +0,0 @@ -package terraform - -import ( - "strings" - "testing" - - "github.com/hashicorp/terraform/addrs" -) - -func TestFlatConfigTransformer_nilModule(t *testing.T) { - g := Graph{Path: addrs.RootModuleInstance} - tf := &FlatConfigTransformer{} - if err := tf.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - - if len(g.Vertices()) > 0 { - t.Fatal("graph should be empty") - } -} - -func TestFlatConfigTransformer(t *testing.T) { - g := Graph{Path: addrs.RootModuleInstance} - tf := &FlatConfigTransformer{ - Config: testModule(t, "transform-flat-config-basic"), - } - if err := tf.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - - actual := strings.TrimSpace(g.String()) - expected := strings.TrimSpace(testTransformFlatConfigBasicStr) - if actual != expected { - t.Fatalf("bad:\n\n%s", actual) - } -} - -const testTransformFlatConfigBasicStr = ` -aws_instance.bar -aws_instance.foo -module.child.aws_instance.baz -` diff --git a/terraform/transform_destroy_cbd.go b/terraform/transform_destroy_cbd.go index 2f4d5edeb..4b954facc 100644 --- a/terraform/transform_destroy_cbd.go +++ b/terraform/transform_destroy_cbd.go @@ -169,6 +169,7 @@ func (t *CBDEdgeTransformer) Transform(g *Graph) error { 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 @@ -201,12 +202,7 @@ func (t *CBDEdgeTransformer) Transform(g *Graph) error { // 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. - // - // We can't trust that the resource being destroyed or anything that - // depends on it is actually in our current graph so we make a new - // graph in order to determine those dependencies and add them in. - log.Printf("[TRACE] CBDEdgeTransformer: building graph to find dependencies...") - depMap, err := t.depMap(destroyMap) + depMap, err := t.depMap(g, destroyMap) if err != nil { return err } @@ -248,26 +244,10 @@ func (t *CBDEdgeTransformer) Transform(g *Graph) error { return nil } -func (t *CBDEdgeTransformer) depMap(destroyMap map[string][]dag.Vertex) (map[string][]dag.Vertex, error) { - // Build the graph of our config, this ensures that all resources - // are present in the graph. - g, diags := (&BasicGraphBuilder{ - Steps: []GraphTransformer{ - &FlatConfigTransformer{Config: t.Config}, - &AttachResourceConfigTransformer{Config: t.Config}, - &AttachStateTransformer{State: t.State}, - &AttachSchemaTransformer{Schemas: t.Schemas}, - &ReferenceTransformer{}, - }, - Name: "CBDEdgeTransformer", - }).Build(nil) - if diags.HasErrors() { - return nil, diags.Err() - } - - // Using this graph, 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. +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. depMap := make(map[string][]dag.Vertex) for _, v := range g.Vertices() { // We're looking for resources. @@ -289,8 +269,10 @@ func (t *CBDEdgeTransformer) depMap(destroyMap map[string][]dag.Vertex) (map[str } // Get the nodes that depend on this on. In the example above: - // finding B in A => B. - for _, v := range g.UpEdges(v).List() { + // 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 {