Merge pull request #6 from hashicorp/f-resourcedeps

Destroy order, ResourceState dependencies

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),

terraform/graph.go

@@ -102,6 +102,103 @@ 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.
+func GraphAddDiff(g *depgraph.Graph, d *Diff) error {
+	var nlist []*depgraph.Noun
+	for _, n := range g.Nouns {
+		rn, ok := n.Meta.(*GraphNodeResource)
+		if !ok {
+			continue
+		}
+
+		rd, ok := d.Resources[rn.Resource.Id]
+		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
+}
+
 // configGraph turns a configuration structure into a dependency graph.
 func graphAddConfigResources(
 	g *depgraph.Graph, c *config.Config, s *State) {

terraform/graph_test.go

@@ -1,6 +1,7 @@
 package terraform
 
 import (
+	"reflect"
 	"strings"
 	"testing"
 )
@@ -109,6 +110,111 @@ 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)
+	}
+}
+
+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
@@ -130,6 +236,27 @@ root
   root -> provider.aws
 `
 
+const testTerraformGraphDiffStr = `
+root: root
+aws_instance.foo
+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

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
+}

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
+}

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)

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 && !idAttr.NewComputed {
+				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
 		}
 
@@ -675,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 = `

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}"
 }

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"
 }

terraform/test-fixtures/graph-diff-destroy/main.tf

@@ -0,0 +1,2 @@
+resource "aws_instance" "foo" {
+}

terraform/test-fixtures/graph-diff/main.tf

@@ -0,0 +1,2 @@
+resource "aws_instance" "foo" {
+}