From 8db6f722d2c55312e217438e66b3dcff2e9ab2b4 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sun, 15 Feb 2015 19:17:23 -0800 Subject: [PATCH] terraform: CBD edge transpositions must happen atomically --- .../main.tf | 9 +++ .../transform-destroy-deps/main.tf | 5 ++ terraform/transform_destroy.go | 22 ++++- terraform/transform_destroy_test.go | 80 +++++++++++++++++++ 4 files changed, 112 insertions(+), 4 deletions(-) create mode 100644 terraform/test-fixtures/transform-create-before-destroy-twice/main.tf create mode 100644 terraform/test-fixtures/transform-destroy-deps/main.tf diff --git a/terraform/test-fixtures/transform-create-before-destroy-twice/main.tf b/terraform/test-fixtures/transform-create-before-destroy-twice/main.tf new file mode 100644 index 000000000..c84a7a678 --- /dev/null +++ b/terraform/test-fixtures/transform-create-before-destroy-twice/main.tf @@ -0,0 +1,9 @@ +resource "aws_lc" "foo" { + lifecycle { create_before_destroy = true } +} + +resource "aws_autoscale" "bar" { + lc = "${aws_lc.foo.id}" + + lifecycle { create_before_destroy = true } +} diff --git a/terraform/test-fixtures/transform-destroy-deps/main.tf b/terraform/test-fixtures/transform-destroy-deps/main.tf new file mode 100644 index 000000000..1419d893c --- /dev/null +++ b/terraform/test-fixtures/transform-destroy-deps/main.tf @@ -0,0 +1,5 @@ +resource "aws_lc" "foo" {} + +resource "aws_asg" "bar" { + lc = "${aws_lc.foo.id}" +} diff --git a/terraform/transform_destroy.go b/terraform/transform_destroy.go index 87c98e901..c5025e55d 100644 --- a/terraform/transform_destroy.go +++ b/terraform/transform_destroy.go @@ -110,6 +110,13 @@ func (t *DestroyTransformer) Transform(g *Graph) error { type CreateBeforeDestroyTransformer struct{} func (t *CreateBeforeDestroyTransformer) Transform(g *Graph) error { + // We "stage" the edge connections/destroys in these slices so that + // while we're doing the edge transformations (transpositions) in + // the graph, we're not affecting future edge transpositions. These + // slices let us stage ALL the changes that WILL happen so that all + // of the transformations happen atomically. + var connect, destroy []dag.Edge + for _, v := range g.Vertices() { // We only care to use the destroy nodes dn, ok := v.(GraphNodeDestroy) @@ -125,7 +132,7 @@ func (t *CreateBeforeDestroyTransformer) Transform(g *Graph) error { // Get the creation side of this node cn := dn.CreateNode() - // Take all the things which depend on the web creation and + // Take all the things which depend on the creation node and // make them dependencies on the destruction. Clarifying this // with an example: if you have a web server and a load balancer // and the load balancer depends on the web server, then when we @@ -138,13 +145,20 @@ func (t *CreateBeforeDestroyTransformer) Transform(g *Graph) error { // This ensures that. for _, sourceRaw := range g.UpEdges(cn).List() { source := sourceRaw.(dag.Vertex) - g.Connect(dag.BasicEdge(dn, source)) + connect = append(connect, dag.BasicEdge(dn, source)) } // Swap the edge so that the destroy depends on the creation // happening... - g.Connect(dag.BasicEdge(dn, cn)) - g.RemoveEdge(dag.BasicEdge(cn, dn)) + connect = append(connect, dag.BasicEdge(dn, cn)) + destroy = append(destroy, dag.BasicEdge(cn, dn)) + } + + for _, edge := range connect { + g.Connect(edge) + } + for _, edge := range destroy { + g.RemoveEdge(edge) } return nil diff --git a/terraform/transform_destroy_test.go b/terraform/transform_destroy_test.go index 97341e8e7..af8a64ba9 100644 --- a/terraform/transform_destroy_test.go +++ b/terraform/transform_destroy_test.go @@ -30,6 +30,31 @@ func TestDestroyTransformer(t *testing.T) { } } +func TestDestroyTransformer_deps(t *testing.T) { + mod := testModule(t, "transform-destroy-deps") + + g := Graph{Path: RootModulePath} + { + tf := &ConfigTransformer{Module: mod} + if err := tf.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + + { + tf := &DestroyTransformer{} + if err := tf.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + + actual := strings.TrimSpace(g.String()) + expected := strings.TrimSpace(testTransformDestroyDepsStr) + if actual != expected { + t.Fatalf("bad:\n\n%s", actual) + } +} + func TestCreateBeforeDestroyTransformer(t *testing.T) { mod := testModule(t, "transform-create-before-destroy-basic") @@ -62,6 +87,38 @@ func TestCreateBeforeDestroyTransformer(t *testing.T) { } } +func TestCreateBeforeDestroyTransformer_twice(t *testing.T) { + mod := testModule(t, "transform-create-before-destroy-twice") + + g := Graph{Path: RootModulePath} + { + tf := &ConfigTransformer{Module: mod} + if err := tf.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + + { + tf := &DestroyTransformer{} + if err := tf.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + + { + tf := &CreateBeforeDestroyTransformer{} + if err := tf.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + + actual := strings.TrimSpace(g.String()) + expected := strings.TrimSpace(testTransformCreateBeforeDestroyTwiceStr) + if actual != expected { + t.Fatalf("bad:\n\n%s", actual) + } +} + const testTransformDestroyBasicStr = ` aws_instance.bar aws_instance.bar (destroy) @@ -73,6 +130,17 @@ aws_instance.foo (destroy) aws_instance.bar (destroy) ` +const testTransformDestroyDepsStr = ` +aws_asg.bar + aws_asg.bar (destroy) + aws_lc.foo +aws_asg.bar (destroy) +aws_lc.foo + aws_lc.foo (destroy) +aws_lc.foo (destroy) + aws_asg.bar (destroy) +` + const testTransformCreateBeforeDestroyBasicStr = ` aws_instance.web aws_instance.web (destroy) @@ -84,3 +152,15 @@ aws_load_balancer.lb aws_load_balancer.lb (destroy) aws_load_balancer.lb (destroy) ` + +const testTransformCreateBeforeDestroyTwiceStr = ` +aws_autoscale.bar + aws_lc.foo +aws_autoscale.bar (destroy) + aws_autoscale.bar +aws_lc.foo +aws_lc.foo (destroy) + aws_autoscale.bar + aws_autoscale.bar (destroy) + aws_lc.foo +`