From 521b432728aacc63b5711dc866372c49b23dd52d Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 30 Jun 2014 17:40:56 -0700 Subject: [PATCH 1/5] terraform: add Dependencies to ResourceState --- terraform/state.go | 57 +++++++++++++++++-- terraform/terraform.go | 4 ++ terraform/terraform_test.go | 16 +++++- terraform/test-fixtures/apply-destroy/main.tf | 3 + 4 files changed, 73 insertions(+), 7 deletions(-) diff --git a/terraform/state.go b/terraform/state.go index 933ab264d..4ac4d09b2 100644 --- a/terraform/state.go +++ b/terraform/state.go @@ -16,8 +16,7 @@ import ( // can use to keep track of what real world resources it is actually // managing. type State struct { - Dependencies map[string][][]string - Resources map[string]*ResourceState + Resources map[string]*ResourceState once sync.Once } @@ -75,6 +74,13 @@ func (s *State) String() string { for ak, av := range rs.Attributes { buf.WriteString(fmt.Sprintf(" %s = %s\n", ak, av)) } + + if len(rs.Dependencies) > 0 { + buf.WriteString(fmt.Sprintf("\n Dependencies:\n")) + for _, dep := range rs.Dependencies { + buf.WriteString(fmt.Sprintf(" %s\n", dep.ID)) + } + } } return buf.String() @@ -135,10 +141,43 @@ func WriteState(d *State, dst io.Writer) error { // Extra is just extra data that a provider can return that we store // for later, but is not exposed in any way to the user. type ResourceState struct { - ID string - Type string + // This is filled in and managed by Terraform, and is the resource + // type itself such as "mycloud_instance". If a resource provider sets + // this value, it won't be persisted. + Type string + + // The attributes below are all meant to be filled in by the + // resource providers themselves. Documentation for each are above + // each element. + + // A unique ID for this resource. This is opaque to Terraform + // and is only meant as a lookup mechanism for the providers. + ID string + + // Attributes are basic information about the resource. Any keys here + // are accessible in variable format within Terraform configurations: + // ${resourcetype.name.attribute}. Attributes map[string]string - Extra map[string]interface{} + + // Extra information that the provider can store about a resource. + // This data is opaque, never shown to the user, and is sent back to + // the provider as-is for whatever purpose appropriate. + Extra map[string]interface{} + + // Dependencies are a list of things that this resource relies on + // existing to remain intact. For example: an AWS instance might + // depend on a subnet (which itself might depend on a VPC, and so + // on). + // + // Terraform uses this information to build valid destruction + // orders and to warn the user if they're destroying a resource that + // another resource depends on. + // + // Things can be put into this list that may not be managed by + // Terraform. If Terraform doesn't find a matching ID in the + // overall state, then it assumes it isn't managed and doesn't + // worry about it. + Dependencies []ResourceDependency } // MergeDiff takes a ResourceDiff and merges the attributes into @@ -174,3 +213,11 @@ func (s *ResourceState) MergeDiff(d *ResourceDiff) *ResourceState { return &result } + +// ResourceDependency maps a resource to another resource that it +// depends on to remain intact and uncorrupted. +type ResourceDependency struct { + // ID of the resource that we depend on. This ID should map + // directly to another ResourceState's ID. + ID string +} diff --git a/terraform/terraform.go b/terraform/terraform.go index f16155c61..384b9e7d1 100644 --- a/terraform/terraform.go +++ b/terraform/terraform.go @@ -104,6 +104,10 @@ func (t *Terraform) apply( p *Plan) (*State, error) { s := new(State) err := g.Walk(t.applyWalkFn(s, p)) + + // Now that we've built the state and have the graph, re-calculate + // the dependencies for our state based on what we did. + return s, err } diff --git a/terraform/terraform_test.go b/terraform/terraform_test.go index ad1c0ab15..df62962d0 100644 --- a/terraform/terraform_test.go +++ b/terraform/terraform_test.go @@ -492,15 +492,27 @@ func testProviderFunc(n string, rs []string) ResourceProviderFactory { return nil, nil } + id := "foo" + if idAttr, ok := d.Attributes["id"]; ok { + id = idAttr.New + } + result := &ResourceState{ - ID: "foo", - Attributes: make(map[string]string), + ID: id, } if d != nil { result = result.MergeDiff(d) } + if depAttr, ok := d.Attributes["dep"]; ok { + result.Dependencies = []ResourceDependency{ + ResourceDependency{ + ID: depAttr.New, + }, + } + } + return result, nil } diff --git a/terraform/test-fixtures/apply-destroy/main.tf b/terraform/test-fixtures/apply-destroy/main.tf index e0eeec450..63c561692 100644 --- a/terraform/test-fixtures/apply-destroy/main.tf +++ b/terraform/test-fixtures/apply-destroy/main.tf @@ -1,7 +1,10 @@ resource "aws_instance" "foo" { + id = "foo" num = "2" } resource "aws_instance" "bar" { + id = "bar" foo = "{aws_instance.foo.num}" + dep = "foo" } From 6f274eb7a924e15017da843c80895b651b84e324 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 30 Jun 2014 18:18:56 -0700 Subject: [PATCH 2/5] terraform: GraphAddDiff works on a basic level --- terraform/graph.go | 29 ++++++++++++ terraform/graph_test.go | 52 ++++++++++++++++++++++ terraform/terraform.go | 4 -- terraform/test-fixtures/graph-diff/main.tf | 2 + 4 files changed, 83 insertions(+), 4 deletions(-) create mode 100644 terraform/test-fixtures/graph-diff/main.tf diff --git a/terraform/graph.go b/terraform/graph.go index af99bcf2d..50988ff33 100644 --- a/terraform/graph.go +++ b/terraform/graph.go @@ -102,6 +102,35 @@ func GraphFull(g *depgraph.Graph, ps map[string]ResourceProviderFactory) error { return nil } +// GraphAddDiff takes an already-built graph of resources and adds the +// diffs to the resource nodes themselves. +// +// This may also introduces new graph elements. If there are diffs that +// require a destroy, new elements may be introduced since destroy order +// is different than create order. For example, destroying a VPC requires +// destroying the VPC's subnets first, whereas creating a VPC requires +// doing it before the subnets are created. This function handles inserting +// these nodes for you. +// +// Note that all nodes modifying the same resource will have the same name. +func GraphAddDiff(g *depgraph.Graph, d *Diff) error { + for _, n := range g.Nouns { + rn, ok := n.Meta.(*GraphNodeResource) + if !ok { + continue + } + + rd, ok := d.Resources[rn.Resource.Id] + if !ok { + continue + } + + rn.Resource.Diff = rd + } + + return nil +} + // configGraph turns a configuration structure into a dependency graph. func graphAddConfigResources( g *depgraph.Graph, c *config.Config, s *State) { diff --git a/terraform/graph_test.go b/terraform/graph_test.go index 76a795e2a..7b2634754 100644 --- a/terraform/graph_test.go +++ b/terraform/graph_test.go @@ -1,6 +1,7 @@ package terraform import ( + "reflect" "strings" "testing" ) @@ -109,6 +110,50 @@ func TestGraphFull(t *testing.T) { } } +func TestGraphAddDiff(t *testing.T) { + config := testConfig(t, "graph-diff") + + g := Graph(config, nil) + if err := g.Validate(); err != nil { + t.Fatalf("err: %s", err) + } + + diff := &Diff{ + Resources: map[string]*ResourceDiff{ + "aws_instance.foo": &ResourceDiff{ + Attributes: map[string]*ResourceAttrDiff{ + "foo": &ResourceAttrDiff{ + New: "bar", + }, + }, + }, + }, + } + + if err := GraphAddDiff(g, diff); err != nil { + t.Fatalf("err: %s", err) + } + if err := g.Validate(); err != nil { + t.Fatalf("err: %s", err) + } + + actual := strings.TrimSpace(g.String()) + expected := strings.TrimSpace(testTerraformGraphDiffStr) + if actual != expected { + t.Fatalf("bad:\n\n%s", actual) + } + + // Verify that the state has been added + n := g.Noun("aws_instance.foo") + rn := n.Meta.(*GraphNodeResource) + + expected2 := diff.Resources["aws_instance.foo"] + actual2 := rn.Resource.Diff + if !reflect.DeepEqual(actual2, expected2) { + t.Fatalf("bad: %#v", actual2) + } +} + const testTerraformGraphStr = ` root: root aws_instance.web @@ -130,6 +175,13 @@ root root -> provider.aws ` +const testTerraformGraphDiffStr = ` +root: root +aws_instance.foo +root + root -> aws_instance.foo +` + const testTerraformGraphStateStr = ` root: root aws_instance.old diff --git a/terraform/terraform.go b/terraform/terraform.go index 384b9e7d1..f16155c61 100644 --- a/terraform/terraform.go +++ b/terraform/terraform.go @@ -104,10 +104,6 @@ func (t *Terraform) apply( p *Plan) (*State, error) { s := new(State) err := g.Walk(t.applyWalkFn(s, p)) - - // Now that we've built the state and have the graph, re-calculate - // the dependencies for our state based on what we did. - return s, err } diff --git a/terraform/test-fixtures/graph-diff/main.tf b/terraform/test-fixtures/graph-diff/main.tf new file mode 100644 index 000000000..ca956330c --- /dev/null +++ b/terraform/test-fixtures/graph-diff/main.tf @@ -0,0 +1,2 @@ +resource "aws_instance" "foo" { +} From 2d72164c6a6718ab33245b9bbdf4b9e1fc04ed5c Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 30 Jun 2014 19:10:44 -0700 Subject: [PATCH 3/5] terraform: graph can add "destroy" nodes --- terraform/graph.go | 72 +++++++++++++++++- terraform/graph_test.go | 75 +++++++++++++++++++ .../test-fixtures/graph-diff-destroy/main.tf | 2 + 3 files changed, 147 insertions(+), 2 deletions(-) create mode 100644 terraform/test-fixtures/graph-diff-destroy/main.tf diff --git a/terraform/graph.go b/terraform/graph.go index 50988ff33..97eed7f0f 100644 --- a/terraform/graph.go +++ b/terraform/graph.go @@ -111,9 +111,8 @@ func GraphFull(g *depgraph.Graph, ps map[string]ResourceProviderFactory) error { // destroying the VPC's subnets first, whereas creating a VPC requires // doing it before the subnets are created. This function handles inserting // these nodes for you. -// -// Note that all nodes modifying the same resource will have the same name. func GraphAddDiff(g *depgraph.Graph, d *Diff) error { + var nlist []*depgraph.Noun for _, n := range g.Nouns { rn, ok := n.Meta.(*GraphNodeResource) if !ok { @@ -124,10 +123,79 @@ func GraphAddDiff(g *depgraph.Graph, d *Diff) error { if !ok { continue } + if rd.Empty() { + continue + } + + if rd.Destroy || rd.RequiresNew() { + // If we're destroying, we create a new destroy node with + // the proper dependencies. Perform a dirty copy operation. + newNode := new(GraphNodeResource) + *newNode = *rn + newNode.Resource = new(Resource) + *newNode.Resource = *rn.Resource + + // Make the diff _just_ the destroy. + newNode.Resource.Diff = &ResourceDiff{Destroy: true} + + // Append it to the list so we handle it later + deps := make([]*depgraph.Dependency, len(n.Deps)) + copy(deps, n.Deps) + newN := &depgraph.Noun{ + Name: fmt.Sprintf("%s (destroy)", newNode.Resource.Id), + Meta: newNode, + Deps: deps, + } + nlist = append(nlist, newN) + + // Mark the old diff to not destroy since we handle that in + // the dedicated node. + rd.Destroy = false + + // Add to the new noun to our dependencies so that the destroy + // happens before the apply. + n.Deps = append(n.Deps, &depgraph.Dependency{ + Name: newN.Name, + Source: n, + Target: newN, + }) + } rn.Resource.Diff = rd } + // Go through each noun and make sure we calculate all the dependencies + // properly. + for _, n := range nlist { + rn := n.Meta.(*GraphNodeResource) + + // If we have no dependencies, then just continue + deps := rn.Resource.State.Dependencies + if len(deps) == 0 { + continue + } + + // We have dependencies. We must be destroyed BEFORE those + // dependencies. Look to see if they're managed. + for _, dep := range deps { + for _, n2 := range nlist { + rn2 := n2.Meta.(*GraphNodeResource) + if rn2.Resource.State.ID == dep.ID { + n2.Deps = append(n2.Deps, &depgraph.Dependency{ + Name: n.Name, + Source: n2, + Target: n, + }) + + break + } + } + } + } + + // Add the nouns to the graph + g.Nouns = append(g.Nouns, nlist...) + return nil } diff --git a/terraform/graph_test.go b/terraform/graph_test.go index 7b2634754..ed379a1c6 100644 --- a/terraform/graph_test.go +++ b/terraform/graph_test.go @@ -154,6 +154,67 @@ func TestGraphAddDiff(t *testing.T) { } } +func TestGraphAddDiff_destroy(t *testing.T) { + config := testConfig(t, "graph-diff-destroy") + state := &State{ + Resources: map[string]*ResourceState{ + "aws_instance.foo": &ResourceState{ + ID: "foo", + Type: "aws_instance", + }, + + "aws_instance.bar": &ResourceState{ + ID: "bar", + Type: "aws_instance", + Dependencies: []ResourceDependency{ + ResourceDependency{ + ID: "foo", + }, + }, + }, + }, + } + + g := Graph(config, state) + if err := g.Validate(); err != nil { + t.Fatalf("err: %s", err) + } + + diff := &Diff{ + Resources: map[string]*ResourceDiff{ + "aws_instance.foo": &ResourceDiff{ + Destroy: true, + }, + "aws_instance.bar": &ResourceDiff{ + Destroy: true, + }, + }, + } + + if err := GraphAddDiff(g, diff); err != nil { + t.Fatalf("err: %s", err) + } + if err := g.Validate(); err != nil { + t.Fatalf("err: %s", err) + } + + actual := strings.TrimSpace(g.String()) + expected := strings.TrimSpace(testTerraformGraphDiffDestroyStr) + if actual != expected { + t.Fatalf("bad:\n\n%s", actual) + } + + // Verify that the state has been added + n := g.Noun("aws_instance.foo") + rn := n.Meta.(*GraphNodeResource) + + expected2 := diff.Resources["aws_instance.foo"] + actual2 := rn.Resource.Diff + if !reflect.DeepEqual(actual2, expected2) { + t.Fatalf("bad: %#v", actual2) + } +} + const testTerraformGraphStr = ` root: root aws_instance.web @@ -182,6 +243,20 @@ root root -> aws_instance.foo ` +const testTerraformGraphDiffDestroyStr = ` +root: root +aws_instance.bar + aws_instance.bar -> aws_instance.bar (destroy) +aws_instance.bar (destroy) +aws_instance.foo + aws_instance.foo -> aws_instance.foo (destroy) +aws_instance.foo (destroy) + aws_instance.foo (destroy) -> aws_instance.bar (destroy) +root + root -> aws_instance.bar + root -> aws_instance.foo +` + const testTerraformGraphStateStr = ` root: root aws_instance.old diff --git a/terraform/test-fixtures/graph-diff-destroy/main.tf b/terraform/test-fixtures/graph-diff-destroy/main.tf new file mode 100644 index 000000000..ca956330c --- /dev/null +++ b/terraform/test-fixtures/graph-diff-destroy/main.tf @@ -0,0 +1,2 @@ +resource "aws_instance" "foo" { +} From d026d4207e4f406a8f189dcdd7173beab4b68dc3 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 30 Jun 2014 19:29:07 -0700 Subject: [PATCH 4/5] terraform: apply diff before apply --- terraform/resource.go | 18 ++++++++++ terraform/terraform.go | 33 +++++++++---------- terraform/terraform_test.go | 6 ++-- terraform/test-fixtures/apply-compute/main.tf | 4 +-- 4 files changed, 38 insertions(+), 23 deletions(-) diff --git a/terraform/resource.go b/terraform/resource.go index 3efa11c69..a9db7e6a4 100644 --- a/terraform/resource.go +++ b/terraform/resource.go @@ -1,5 +1,9 @@ package terraform +import ( + "fmt" +) + // Resource encapsulates a resource, its configuration, its provider, // its current state, and potentially a desired diff from the state it // wants to reach. @@ -10,3 +14,17 @@ type Resource struct { Provider ResourceProvider State *ResourceState } + +// TODO: test +func (r *Resource) Vars() map[string]string { + if r.State == nil { + return nil + } + + vars := make(map[string]string) + for ak, av := range r.State.Attributes { + vars[fmt.Sprintf("%s.%s", r.Id, ak)] = av + } + + return vars +} diff --git a/terraform/terraform.go b/terraform/terraform.go index f16155c61..149bd9f6e 100644 --- a/terraform/terraform.go +++ b/terraform/terraform.go @@ -102,6 +102,13 @@ func (t *Terraform) Refresh(c *config.Config, s *State) (*State, error) { func (t *Terraform) apply( g *depgraph.Graph, p *Plan) (*State, error) { + if err := GraphAddDiff(g, p.Diff); err != nil { + return nil, err + } + if err := g.Validate(); err != nil { + return nil, err + } + s := new(State) err := g.Walk(t.applyWalkFn(s, p)) return s, err @@ -170,11 +177,9 @@ func (t *Terraform) applyWalkFn( result.init() cb := func(r *Resource) (map[string]string, error) { - diff, ok := p.Diff.Resources[r.Id] - if !ok { - // Skip if there is no diff for a resource - log.Printf("[DEBUG] No diff for %s, skipping.", r.Id) - return nil, nil + diff := r.Diff + if diff.Empty() { + return r.Vars(), nil } if !diff.Destroy { @@ -229,23 +234,21 @@ func (t *Terraform) applyWalkFn( result.Resources[r.Id] = rs l.Unlock() + // Update the state for the resource itself + r.State = rs + for _, h := range t.hooks { // TODO: return value h.PostApply(r.Id, r.State) } // Determine the new state and update variables - vars := make(map[string]string) - for ak, av := range rs.Attributes { - vars[fmt.Sprintf("%s.%s", r.Id, ak)] = av - } - err = nil if len(errs) > 0 { err = &MultiError{Errors: errs} } - return vars, err + return r.Vars(), err } return t.genericWalkFn(p.Vars, cb) @@ -298,17 +301,11 @@ func (t *Terraform) planWalkFn(result *Plan, opts *PlanOpts) depgraph.WalkFunc { } // Determine the new state and update variables - vars := make(map[string]string) if !diff.Empty() { r.State = r.State.MergeDiff(diff) } - if r.State != nil { - for ak, av := range r.State.Attributes { - vars[fmt.Sprintf("%s.%s", r.Id, ak)] = av - } - } - return vars, nil + return r.Vars(), nil } return t.genericWalkFn(opts.Vars, cb) diff --git a/terraform/terraform_test.go b/terraform/terraform_test.go index df62962d0..dea495dd2 100644 --- a/terraform/terraform_test.go +++ b/terraform/terraform_test.go @@ -493,7 +493,7 @@ func testProviderFunc(n string, rs []string) ResourceProviderFactory { } id := "foo" - if idAttr, ok := d.Attributes["id"]; ok { + if idAttr, ok := d.Attributes["id"]; ok && !idAttr.NewComputed { id = idAttr.New } @@ -687,12 +687,12 @@ const testTerraformApplyComputeStr = ` aws_instance.bar: ID = foo type = aws_instance - foo = computed_id + foo = computed_dynamical aws_instance.foo: ID = foo type = aws_instance num = 2 - id = computed_id + dynamical = computed_dynamical ` const testTerraformApplyDestroyStr = ` diff --git a/terraform/test-fixtures/apply-compute/main.tf b/terraform/test-fixtures/apply-compute/main.tf index cff609825..3999b5912 100644 --- a/terraform/test-fixtures/apply-compute/main.tf +++ b/terraform/test-fixtures/apply-compute/main.tf @@ -1,9 +1,9 @@ resource "aws_instance" "foo" { num = "2" - compute = "id" + compute = "dynamical" compute_value = "${var.value}" } resource "aws_instance" "bar" { - foo = "${aws_instance.foo.id}" + foo = "${aws_instance.foo.dynamical}" } From 2729050d1915e0f0754651d8ed831ccb01ecbf0e Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 30 Jun 2014 20:49:49 -0700 Subject: [PATCH 5/5] command: fix failing tests --- command/apply_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/command/apply_test.go b/command/apply_test.go index 786e3c47b..9ea9c2dcc 100644 --- a/command/apply_test.go +++ b/command/apply_test.go @@ -98,6 +98,14 @@ func TestApply_state(t *testing.T) { statePath := testStateFile(t, originalState) p := testProvider() + p.DiffReturn = &terraform.ResourceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "ami": &terraform.ResourceAttrDiff{ + New: "bar", + }, + }, + } + ui := new(cli.MockUi) c := &ApplyCommand{ TFConfig: testTFConfig(p),