From 48a92cfb1f6f0aea100c7f55608653cb25093bbb Mon Sep 17 00:00:00 2001
From: Jakub Janczak <jakub.janczak@schibsted.pl>
Date: Thu, 5 Feb 2015 09:33:56 +0100
Subject: [PATCH 01/24] remote/s3: s3 remote state storage support

---
 command/remote.go |  27 +++++--
 remote/README.md  |  11 +++
 remote/client.go  |   2 +
 remote/s3.go      | 192 ++++++++++++++++++++++++++++++++++++++++++++++
 remote/s3_test.go | 134 ++++++++++++++++++++++++++++++++
 5 files changed, 359 insertions(+), 7 deletions(-)
 create mode 100644 remote/README.md
 create mode 100644 remote/s3.go
 create mode 100644 remote/s3_test.go

diff --git a/command/remote.go b/command/remote.go
index d9a773704a48..3b9e8c57b1a7 100644
--- a/command/remote.go
+++ b/command/remote.go
@@ -32,7 +32,7 @@ type RemoteCommand struct {
 
 func (c *RemoteCommand) Run(args []string) int {
 	args = c.Meta.process(args, false)
-	var address, accessToken, name, path string
+	var address, accessToken, name, path, region, securityToken, bucket string
 	cmdFlags := flag.NewFlagSet("remote", flag.ContinueOnError)
 	cmdFlags.BoolVar(&c.conf.disableRemote, "disable", false, "")
 	cmdFlags.BoolVar(&c.conf.pullOnDisable, "pull", true, "")
@@ -41,6 +41,9 @@ func (c *RemoteCommand) Run(args []string) int {
 	cmdFlags.StringVar(&c.remoteConf.Type, "backend", "atlas", "")
 	cmdFlags.StringVar(&address, "address", "", "")
 	cmdFlags.StringVar(&accessToken, "access-token", "", "")
+	cmdFlags.StringVar(&securityToken, "security-token", "", "")
+	cmdFlags.StringVar(&bucket, "bucket", "", "")
+	cmdFlags.StringVar(&region, "region", "", "")
 	cmdFlags.StringVar(&name, "name", "", "")
 	cmdFlags.StringVar(&path, "path", "", "")
 	cmdFlags.Usage = func() { c.Ui.Error(c.Help()) }
@@ -57,10 +60,13 @@ func (c *RemoteCommand) Run(args []string) int {
 
 	// Populate the various configurations
 	c.remoteConf.Config = map[string]string{
-		"address":      address,
-		"access_token": accessToken,
-		"name":         name,
-		"path":         path,
+		"address":        address,
+		"access_token":   accessToken,
+		"security_token": securityToken,
+		"name":           name,
+		"path":           path,
+		"bucket":         bucket,
+		"region":         region,
 	}
 
 	// Check if have an existing local state file
@@ -329,13 +335,17 @@ Options:
   -access-token=token    Authentication token for state storage server.
                          Required for Atlas backend, optional for Consul.
 
+  -security-token=token  Security token. Specific to S3 (required).
+
   -backend=Atlas         Specifies the type of remote backend. Must be one
-                         of Atlas, Consul, or HTTP. Defaults to Atlas.
+                         of Atlas, Consul,HTTP or S3. Defaults to Atlas.
 
   -backup=path           Path to backup the existing state file before
                          modifying. Defaults to the "-state" path with
                          ".backup" extension. Set to "-" to disable backup.
 
+  -bucket=bucket         S3 bucket name. Specific to S3 (required).
+
   -disable               Disables remote state management and migrates the state
                          to the -state path.
 
@@ -343,12 +353,15 @@ Options:
                          Required for Atlas backend.
 
   -path=path             Path of the remote state in Consul. Required for the
-                         Consul backend.
+                         Consul and S3 backend.
 
   -pull=true             Controls if the remote state is pulled before disabling.
                          This defaults to true to ensure the latest state is cached
 						 before disabling.
 
+  -region=region         AWS region to use. Specific for S3 (not required if AWS_DEFAULT_REGION 
+                         env variable is set).
+
   -state=path            Path to read state. Defaults to "terraform.tfstate"
                          unless remote state is enabled.
 
diff --git a/remote/README.md b/remote/README.md
new file mode 100644
index 000000000000..60d53a1e2630
--- /dev/null
+++ b/remote/README.md
@@ -0,0 +1,11 @@
+## How to test
+
+### S3 remote state storage
+To run S3 integration tests you need following env variables to be set:
+  * AWS_ACCESS_KEY
+  * AWS_SECRET_KEY
+  * AWS_DEFAULT_REGION
+  * TERRAFORM_STATE_BUCKET
+
+Additionally specified bucket should exist in the defined region and should be accessible 
+using specified credentials.
diff --git a/remote/client.go b/remote/client.go
index c57e40c780cb..56849553e931 100644
--- a/remote/client.go
+++ b/remote/client.go
@@ -59,6 +59,8 @@ func NewClientByType(ctype string, conf map[string]string) (RemoteClient, error)
 		return NewConsulRemoteClient(conf)
 	case "http":
 		return NewHTTPRemoteClient(conf)
+	case "s3":
+		return NewS3RemoteClient(conf)
 	default:
 		return nil, fmt.Errorf("Unknown remote client type '%s'", ctype)
 	}
diff --git a/remote/s3.go b/remote/s3.go
new file mode 100644
index 000000000000..04b202094a4f
--- /dev/null
+++ b/remote/s3.go
@@ -0,0 +1,192 @@
+package remote
+
+import (
+	"bytes"
+	"crypto/md5"
+	"encoding/base64"
+	"fmt"
+	"io"
+	"net/http"
+	"os"
+	"time"
+
+	"github.com/goamz/goamz/aws"
+	"github.com/goamz/goamz/s3"
+)
+
+type S3RemoteClient struct {
+	Bucket *s3.Bucket
+	Path   string
+}
+
+func GetRegion(conf map[string]string) (aws.Region, error) {
+	regionName, ok := conf["region"]
+	if !ok || regionName == "" {
+		regionName = os.Getenv("AWS_DEFAULT_REGION")
+		if regionName == "" {
+			return aws.Region{}, fmt.Errorf("AWS region not set")
+		}
+	}
+
+	region, ok := aws.Regions[regionName]
+	if !ok {
+		return aws.Region{}, fmt.Errorf("AWS region set in configuration '%v' doesn't exist", regionName)
+	}
+	return region, nil
+}
+
+func NewS3RemoteClient(conf map[string]string) (*S3RemoteClient, error) {
+	client := &S3RemoteClient{}
+
+	auth, err := aws.GetAuth(conf["access_token"], conf["secret_token"], "", time.Now())
+	if err != nil {
+		return nil, err
+	}
+
+	region, err := GetRegion(conf)
+	if err != nil {
+		return nil, err
+	}
+
+	bucketName, ok := conf["bucket"]
+	if !ok {
+		return nil, fmt.Errorf("Missing 'bucket_name' configuration")
+	}
+
+	client.Bucket = s3.New(auth, region).Bucket(bucketName)
+
+	path, ok := conf["path"]
+	if !ok {
+		return nil, fmt.Errorf("Missing 'path' configuration")
+	}
+	client.Path = path
+
+	return client, nil
+}
+
+func (c *S3RemoteClient) GetState() (*RemoteStatePayload, error) {
+	resp, err := c.Bucket.GetResponse(c.Path)
+	defer func() {
+		if resp != nil && resp.Body != nil {
+			resp.Body.Close()
+		}
+	}()
+
+	if err != nil {
+		switch err.(type) {
+		case *s3.Error:
+			s3Err := err.(*s3.Error)
+
+			// FIXME copied from Atlas
+			// Handle the common status codes
+			switch s3Err.StatusCode {
+			case http.StatusOK:
+				// Handled after
+			case http.StatusNoContent:
+				return nil, nil
+			case http.StatusNotFound:
+				return nil, nil
+			case http.StatusUnauthorized:
+				return nil, ErrRequireAuth
+			case http.StatusForbidden:
+				return nil, ErrInvalidAuth
+			case http.StatusInternalServerError:
+				return nil, ErrRemoteInternal
+			default:
+				return nil, fmt.Errorf("Unexpected HTTP response code %d", s3Err.StatusCode)
+			}
+		default:
+			return nil, err
+		}
+	}
+
+	// Read in the body
+	buf := bytes.NewBuffer(nil)
+	if _, err := io.Copy(buf, resp.Body); err != nil {
+		return nil, fmt.Errorf("Failed to read remote state: %v", err)
+	}
+
+	// Create the payload
+	payload := &RemoteStatePayload{
+		State: buf.Bytes(),
+	}
+
+	// Check for the MD5
+	if raw := resp.Header.Get("Content-MD5"); raw != "" {
+		md5, err := base64.StdEncoding.DecodeString(raw)
+		if err != nil {
+			return nil, fmt.Errorf("Failed to decode Content-MD5 '%s': %v", raw, err)
+		}
+		payload.MD5 = md5
+
+	} else {
+		// Generate the MD5
+		hash := md5.Sum(payload.State)
+		payload.MD5 = hash[:md5.Size]
+	}
+
+	return payload, nil
+}
+
+func (c *S3RemoteClient) PutState(state []byte, force bool) error {
+	// Generate the MD5
+	hash := md5.Sum(state)
+	b64 := base64.StdEncoding.EncodeToString(hash[:md5.Size])
+
+	options := s3.Options{
+		ContentMD5: b64,
+	}
+
+	err := c.Bucket.Put(c.Path, state, "application/json", s3.Private, options)
+	switch err.(type) {
+	case *s3.Error:
+		s3Err := err.(*s3.Error)
+
+		// Handle the error codes
+		switch s3Err.StatusCode {
+		case http.StatusOK:
+			return nil
+		case http.StatusConflict:
+			return ErrConflict
+		case http.StatusPreconditionFailed:
+			return ErrServerNewer
+		case http.StatusUnauthorized:
+			return ErrRequireAuth
+		case http.StatusForbidden:
+			return ErrInvalidAuth
+		case http.StatusInternalServerError:
+			return ErrRemoteInternal
+		default:
+			return fmt.Errorf("Unexpected HTTP response code %d", s3Err.StatusCode)
+		}
+	default:
+		return err
+	}
+}
+
+func (c *S3RemoteClient) DeleteState() error {
+	err := c.Bucket.Del(c.Path)
+	switch err.(type) {
+	case *s3.Error:
+		s3Err := err.(*s3.Error)
+		// Handle the error codes
+		switch s3Err.StatusCode {
+		case http.StatusOK:
+			return nil
+		case http.StatusNoContent:
+			return nil
+		case http.StatusNotFound:
+			return nil
+		case http.StatusUnauthorized:
+			return ErrRequireAuth
+		case http.StatusForbidden:
+			return ErrInvalidAuth
+		case http.StatusInternalServerError:
+			return ErrRemoteInternal
+		default:
+			return fmt.Errorf("Unexpected HTTP response code %d", s3Err.StatusCode)
+		}
+	default:
+		return err
+	}
+}
diff --git a/remote/s3_test.go b/remote/s3_test.go
new file mode 100644
index 000000000000..41309ae3f1f1
--- /dev/null
+++ b/remote/s3_test.go
@@ -0,0 +1,134 @@
+package remote
+
+import (
+	"bytes"
+	"crypto/md5"
+	"os"
+	"testing"
+
+	"github.com/hashicorp/terraform/terraform"
+)
+
+func TestS3Remote_NewClient(t *testing.T) {
+	conf := map[string]string{}
+	if _, err := NewS3RemoteClient(conf); err == nil {
+		t.Fatalf("expect error")
+	}
+
+	conf["access_token"] = "test"
+	conf["secret_token"] = "test"
+	conf["path"] = "hashicorp/test-state"
+	conf["bucket"] = "plan3-test"
+	conf["region"] = "eu-west-1"
+	if _, err := NewS3RemoteClient(conf); err != nil {
+		t.Fatalf("err: %v", err)
+	}
+}
+
+func TestS3Remote_Validate_envVar(t *testing.T) {
+	conf := map[string]string{}
+	if _, err := NewS3RemoteClient(conf); err == nil {
+		t.Fatalf("expect error")
+	}
+
+	defer os.Setenv("AWS_ACCESS_KEY", os.Getenv("AWS_ACCESS_KEY"))
+	os.Setenv("AWS_ACCESS_KEY", "foo")
+
+	defer os.Setenv("AWS_SECRET_KEY", os.Getenv("AWS_SECRET_KEY"))
+	os.Setenv("AWS_SECRET_KEY", "foo")
+
+	defer os.Setenv("AWS_DEFAULT_REGION", os.Getenv("AWS_DEFAULT_REGION"))
+	os.Setenv("AWS_DEFAULT_REGION", "eu-west-1")
+
+	conf["path"] = "hashicorp/test-state"
+	conf["bucket"] = "plan3-test"
+	if _, err := NewS3RemoteClient(conf); err != nil {
+		t.Fatalf("err: %v", err)
+	}
+}
+
+func checkS3(t *testing.T) {
+	if os.Getenv("AWS_ACCESS_KEY") == "" || os.Getenv("AWS_SECRET_KEY") == "" || os.Getenv("AWS_DEFAULT_REGION") == "" || os.Getenv("TERRAFORM_STATE_BUCKET") == "" {
+		t.SkipNow()
+	}
+}
+
+func TestS3Remote(t *testing.T) {
+	checkS3(t)
+	remote := &terraform.RemoteState{
+		Type: "atlas",
+		Config: map[string]string{
+			"access_token": "some-access-token",
+			"name":         "hashicorp/test-remote-state",
+		},
+	}
+	r, err := NewClientByType("s3", map[string]string{
+		"bucket": os.Getenv("TERRAFORM_STATE_BUCKET"),
+		"path":   "test-remote-state",
+	})
+	if err != nil {
+		t.Fatalf("Err: %v", err)
+	}
+
+	// Get a valid input
+	inp, err := blankState(remote)
+	if err != nil {
+		t.Fatalf("Err: %v", err)
+	}
+	inpMD5 := md5.Sum(inp)
+	hash := inpMD5[:16]
+
+	// Delete the state, should be none
+	err = r.DeleteState()
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	// Ensure no state
+	payload, err := r.GetState()
+	if err != nil {
+		t.Fatalf("Err: %v", err)
+	}
+	if payload != nil {
+		t.Fatalf("unexpected payload")
+	}
+
+	// Put the state
+	err = r.PutState(inp, false)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	// Get it back
+	payload, err = r.GetState()
+	if err != nil {
+		t.Fatalf("Err: %v", err)
+	}
+	if payload == nil {
+		t.Fatalf("unexpected payload")
+	}
+
+	// Check the payload
+	if !bytes.Equal(payload.MD5, hash) {
+		t.Fatalf("bad hash: %x %x", payload.MD5, hash)
+	}
+	if !bytes.Equal(payload.State, inp) {
+		t.Errorf("inp: %s", inp)
+		t.Fatalf("bad response: %s", payload.State)
+	}
+
+	// Delete the state
+	err = r.DeleteState()
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	// Should be gone
+	payload, err = r.GetState()
+	if err != nil {
+		t.Fatalf("Err: %v", err)
+	}
+	if payload != nil {
+		t.Fatalf("unexpected payload")
+	}
+}

From bcfb8df1164a4dab4e1fa31388798cd7e5005ca1 Mon Sep 17 00:00:00 2001
From: Jakub Janczak <jakub.janczak@schibsted.pl>
Date: Fri, 6 Feb 2015 00:02:04 +0100
Subject: [PATCH 02/24] remote/s3: removing code that is serving versioning
 puproses which is not the case for s3 for the moment

---
 remote/s3.go | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/remote/s3.go b/remote/s3.go
index 04b202094a4f..a4c34e53a719 100644
--- a/remote/s3.go
+++ b/remote/s3.go
@@ -146,10 +146,6 @@ func (c *S3RemoteClient) PutState(state []byte, force bool) error {
 		switch s3Err.StatusCode {
 		case http.StatusOK:
 			return nil
-		case http.StatusConflict:
-			return ErrConflict
-		case http.StatusPreconditionFailed:
-			return ErrServerNewer
 		case http.StatusUnauthorized:
 			return ErrRequireAuth
 		case http.StatusForbidden:

From 98d7bcefef6391ee421fd173ce9a60c0fb38f915 Mon Sep 17 00:00:00 2001
From: Jakub Janczak <kubek2k@gmail.com>
Date: Fri, 6 Feb 2015 10:41:50 +0100
Subject: [PATCH 03/24] remote/s3: there is no ok answer on error

---
 command/remote.go | 10 +++-------
 remote/s3.go      |  6 ------
 2 files changed, 3 insertions(+), 13 deletions(-)

diff --git a/command/remote.go b/command/remote.go
index 3b9e8c57b1a7..74ddddbdacdd 100644
--- a/command/remote.go
+++ b/command/remote.go
@@ -32,7 +32,7 @@ type RemoteCommand struct {
 
 func (c *RemoteCommand) Run(args []string) int {
 	args = c.Meta.process(args, false)
-	var address, accessToken, name, path, region, securityToken, bucket string
+	var address, accessToken, name, path, region, securityToken string
 	cmdFlags := flag.NewFlagSet("remote", flag.ContinueOnError)
 	cmdFlags.BoolVar(&c.conf.disableRemote, "disable", false, "")
 	cmdFlags.BoolVar(&c.conf.pullOnDisable, "pull", true, "")
@@ -42,7 +42,6 @@ func (c *RemoteCommand) Run(args []string) int {
 	cmdFlags.StringVar(&address, "address", "", "")
 	cmdFlags.StringVar(&accessToken, "access-token", "", "")
 	cmdFlags.StringVar(&securityToken, "security-token", "", "")
-	cmdFlags.StringVar(&bucket, "bucket", "", "")
 	cmdFlags.StringVar(&region, "region", "", "")
 	cmdFlags.StringVar(&name, "name", "", "")
 	cmdFlags.StringVar(&path, "path", "", "")
@@ -65,7 +64,6 @@ func (c *RemoteCommand) Run(args []string) int {
 		"security_token": securityToken,
 		"name":           name,
 		"path":           path,
-		"bucket":         bucket,
 		"region":         region,
 	}
 
@@ -330,7 +328,7 @@ Usage: terraform remote [options]
 Options:
 
   -address=url           URL of the remote storage server.
-                         Required for HTTP backend, optional for Atlas and Consul.
+                         Required for HTTP and S3 backend, optional for Atlas and Consul.
 
   -access-token=token    Authentication token for state storage server.
                          Required for Atlas backend, optional for Consul.
@@ -344,8 +342,6 @@ Options:
                          modifying. Defaults to the "-state" path with
                          ".backup" extension. Set to "-" to disable backup.
 
-  -bucket=bucket         S3 bucket name. Specific to S3 (required).
-
   -disable               Disables remote state management and migrates the state
                          to the -state path.
 
@@ -353,7 +349,7 @@ Options:
                          Required for Atlas backend.
 
   -path=path             Path of the remote state in Consul. Required for the
-                         Consul and S3 backend.
+                         Consul.
 
   -pull=true             Controls if the remote state is pulled before disabling.
                          This defaults to true to ensure the latest state is cached
diff --git a/remote/s3.go b/remote/s3.go
index a4c34e53a719..6c4d1a41f7d8 100644
--- a/remote/s3.go
+++ b/remote/s3.go
@@ -80,8 +80,6 @@ func (c *S3RemoteClient) GetState() (*RemoteStatePayload, error) {
 			// FIXME copied from Atlas
 			// Handle the common status codes
 			switch s3Err.StatusCode {
-			case http.StatusOK:
-				// Handled after
 			case http.StatusNoContent:
 				return nil, nil
 			case http.StatusNotFound:
@@ -144,8 +142,6 @@ func (c *S3RemoteClient) PutState(state []byte, force bool) error {
 
 		// Handle the error codes
 		switch s3Err.StatusCode {
-		case http.StatusOK:
-			return nil
 		case http.StatusUnauthorized:
 			return ErrRequireAuth
 		case http.StatusForbidden:
@@ -167,8 +163,6 @@ func (c *S3RemoteClient) DeleteState() error {
 		s3Err := err.(*s3.Error)
 		// Handle the error codes
 		switch s3Err.StatusCode {
-		case http.StatusOK:
-			return nil
 		case http.StatusNoContent:
 			return nil
 		case http.StatusNotFound:

From 84e6364bff670f9230a5a90ccf0af0bf53aeb7fe Mon Sep 17 00:00:00 2001
From: Jakub Janczak <jakub.janczak@schibsted.pl>
Date: Fri, 6 Feb 2015 12:26:11 +0100
Subject: [PATCH 04/24] remote/s3: using address for bucket and path

---
 remote/s3.go      | 28 ++++++++++++++++++----------
 remote/s3_test.go |  6 ++----
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/remote/s3.go b/remote/s3.go
index 6c4d1a41f7d8..bc43e9d17f58 100644
--- a/remote/s3.go
+++ b/remote/s3.go
@@ -8,6 +8,7 @@ import (
 	"io"
 	"net/http"
 	"os"
+	"regexp"
 	"time"
 
 	"github.com/goamz/goamz/aws"
@@ -19,7 +20,7 @@ type S3RemoteClient struct {
 	Path   string
 }
 
-func GetRegion(conf map[string]string) (aws.Region, error) {
+func getRegion(conf map[string]string) (aws.Region, error) {
 	regionName, ok := conf["region"]
 	if !ok || regionName == "" {
 		regionName = os.Getenv("AWS_DEFAULT_REGION")
@@ -35,6 +36,15 @@ func GetRegion(conf map[string]string) (aws.Region, error) {
 	return region, nil
 }
 
+func getBucketAndPath(address string) (string, string, error) {
+	re := regexp.MustCompile("^s3://([^/]+)/(.+)$")
+	matches := re.FindStringSubmatch(address)
+	if len(matches) < 3 {
+		return "", "", fmt.Errorf("Address for s3 should be of form: s3://<bucket_name>/<path>")
+	}
+	return matches[1], matches[2], nil
+}
+
 func NewS3RemoteClient(conf map[string]string) (*S3RemoteClient, error) {
 	client := &S3RemoteClient{}
 
@@ -43,22 +53,20 @@ func NewS3RemoteClient(conf map[string]string) (*S3RemoteClient, error) {
 		return nil, err
 	}
 
-	region, err := GetRegion(conf)
+	region, err := getRegion(conf)
 	if err != nil {
 		return nil, err
 	}
 
-	bucketName, ok := conf["bucket"]
+	address, ok := conf["address"]
 	if !ok {
-		return nil, fmt.Errorf("Missing 'bucket_name' configuration")
+		return nil, fmt.Errorf("'address' configuration not set for S3 remote state storage backend")
 	}
-
-	client.Bucket = s3.New(auth, region).Bucket(bucketName)
-
-	path, ok := conf["path"]
-	if !ok {
-		return nil, fmt.Errorf("Missing 'path' configuration")
+	bucketName, path, err := getBucketAndPath(address)
+	if err != nil {
+		return nil, err
 	}
+	client.Bucket = s3.New(auth, region).Bucket(bucketName)
 	client.Path = path
 
 	return client, nil
diff --git a/remote/s3_test.go b/remote/s3_test.go
index 41309ae3f1f1..07e1c2fd593b 100644
--- a/remote/s3_test.go
+++ b/remote/s3_test.go
@@ -17,8 +17,7 @@ func TestS3Remote_NewClient(t *testing.T) {
 
 	conf["access_token"] = "test"
 	conf["secret_token"] = "test"
-	conf["path"] = "hashicorp/test-state"
-	conf["bucket"] = "plan3-test"
+	conf["address"] = "s3://plan3-test/hashicorp/test-state"
 	conf["region"] = "eu-west-1"
 	if _, err := NewS3RemoteClient(conf); err != nil {
 		t.Fatalf("err: %v", err)
@@ -40,8 +39,7 @@ func TestS3Remote_Validate_envVar(t *testing.T) {
 	defer os.Setenv("AWS_DEFAULT_REGION", os.Getenv("AWS_DEFAULT_REGION"))
 	os.Setenv("AWS_DEFAULT_REGION", "eu-west-1")
 
-	conf["path"] = "hashicorp/test-state"
-	conf["bucket"] = "plan3-test"
+	conf["address"] = "s3://terraform-state/hashicorp/test-state"
 	if _, err := NewS3RemoteClient(conf); err != nil {
 		t.Fatalf("err: %v", err)
 	}

From d4b936251882926984ecd59865a1692d93696bcd Mon Sep 17 00:00:00 2001
From: Paul Hinze <phinze@phinze.com>
Date: Thu, 23 Apr 2015 10:52:31 -0500
Subject: [PATCH 05/24] core: validate on verbose graph to detect some cycles
 earlier

Most CBD-related cycles include destroy nodes, and destroy nodes were
all being pruned from the graph before staring the Validate walk.

In practice this meant that we had scenarios that would error out with
graph cycles on Apply that _seemed_ fine during Plan.

This introduces a Verbose option to the GraphBuilder that tells it to
generate a "worst-case" graph. Validate sets this to true so that cycle
errors will always trigger at this step if they're going to happen.

(This Verbose option will be exposed as a CLI flag to `terraform graph`
in a second incoming PR.)

refs #1651
---
 command/graph.go                              |  5 +-
 terraform/context.go                          | 66 +++++++++++++----
 terraform/context_test.go                     | 19 +++++
 terraform/graph_builder.go                    | 55 +++++++++++---
 terraform/graph_builder_test.go               | 71 ++++++++++++++++++-
 .../test-fixtures/validate-cycle/main.tf      | 19 +++++
 6 files changed, 209 insertions(+), 26 deletions(-)
 create mode 100644 terraform/test-fixtures/validate-cycle/main.tf

diff --git a/command/graph.go b/command/graph.go
index 834d6c865bef..8bd5413facf9 100644
--- a/command/graph.go
+++ b/command/graph.go
@@ -52,7 +52,10 @@ func (c *GraphCommand) Run(args []string) int {
 		return 1
 	}
 
-	g, err := ctx.Graph()
+	g, err := ctx.Graph(&terraform.ContextGraphOpts{
+		Validate: true,
+		Verbose:  false,
+	})
 	if err != nil {
 		c.Ui.Error(fmt.Sprintf("Error creating graph: %s", err))
 		return 1
diff --git a/terraform/context.go b/terraform/context.go
index b13f92f638ee..669d50b26546 100644
--- a/terraform/context.go
+++ b/terraform/context.go
@@ -116,14 +116,19 @@ func NewContext(opts *ContextOpts) *Context {
 	}
 }
 
+type ContextGraphOpts struct {
+	Validate bool
+	Verbose  bool
+}
+
 // Graph returns the graph for this config.
-func (c *Context) Graph() (*Graph, error) {
-	return c.GraphBuilder().Build(RootModulePath)
+func (c *Context) Graph(g *ContextGraphOpts) (*Graph, error) {
+	return c.graphBuilder(g).Build(RootModulePath)
 }
 
 // GraphBuilder returns the GraphBuilder that will be used to create
 // the graphs for this context.
-func (c *Context) GraphBuilder() GraphBuilder {
+func (c *Context) graphBuilder(g *ContextGraphOpts) GraphBuilder {
 	// TODO test
 	providers := make([]string, 0, len(c.providers))
 	for k, _ := range c.providers {
@@ -143,6 +148,8 @@ func (c *Context) GraphBuilder() GraphBuilder {
 		State:        c.state,
 		Targets:      c.targets,
 		Destroy:      c.destroy,
+		Validate:     g.Validate,
+		Verbose:      g.Verbose,
 	}
 }
 
@@ -226,8 +233,14 @@ func (c *Context) Input(mode InputMode) error {
 	}
 
 	if mode&InputModeProvider != 0 {
+		// Build the graph
+		graph, err := c.Graph(&ContextGraphOpts{Validate: true})
+		if err != nil {
+			return err
+		}
+
 		// Do the walk
-		if _, err := c.walk(walkInput); err != nil {
+		if _, err := c.walk(graph, walkInput); err != nil {
 			return err
 		}
 	}
@@ -247,8 +260,14 @@ func (c *Context) Apply() (*State, error) {
 	// Copy our own state
 	c.state = c.state.DeepCopy()
 
+	// Build the graph
+	graph, err := c.Graph(&ContextGraphOpts{Validate: true})
+	if err != nil {
+		return nil, err
+	}
+
 	// Do the walk
-	_, err := c.walk(walkApply)
+	_, err = c.walk(graph, walkApply)
 
 	// Clean out any unused things
 	c.state.prune()
@@ -300,8 +319,14 @@ func (c *Context) Plan() (*Plan, error) {
 	c.diff.init()
 	c.diffLock.Unlock()
 
+	// Build the graph
+	graph, err := c.Graph(&ContextGraphOpts{Validate: true})
+	if err != nil {
+		return nil, err
+	}
+
 	// Do the walk
-	if _, err := c.walk(operation); err != nil {
+	if _, err := c.walk(graph, operation); err != nil {
 		return nil, err
 	}
 	p.Diff = c.diff
@@ -322,8 +347,14 @@ func (c *Context) Refresh() (*State, error) {
 	// Copy our own state
 	c.state = c.state.DeepCopy()
 
+	// Build the graph
+	graph, err := c.Graph(&ContextGraphOpts{Validate: true})
+	if err != nil {
+		return nil, err
+	}
+
 	// Do the walk
-	if _, err := c.walk(walkRefresh); err != nil {
+	if _, err := c.walk(graph, walkRefresh); err != nil {
 		return nil, err
 	}
 
@@ -375,8 +406,18 @@ func (c *Context) Validate() ([]string, []error) {
 		}
 	}
 
+	// Build a Verbose version of the graph so we can catch any potential cycles
+	// in the validate stage
+	graph, err := c.Graph(&ContextGraphOpts{
+		Validate: true,
+		Verbose:  true,
+	})
+	if err != nil {
+		return nil, []error{err}
+	}
+
 	// Walk
-	walker, err := c.walk(walkValidate)
+	walker, err := c.walk(graph, walkValidate)
 	if err != nil {
 		return nil, multierror.Append(errs, err).Errors
 	}
@@ -429,13 +470,8 @@ func (c *Context) releaseRun(ch chan<- struct{}) {
 	c.sh.Reset()
 }
 
-func (c *Context) walk(operation walkOperation) (*ContextGraphWalker, error) {
-	// Build the graph
-	graph, err := c.GraphBuilder().Build(RootModulePath)
-	if err != nil {
-		return nil, err
-	}
-
+func (c *Context) walk(
+	graph *Graph, operation walkOperation) (*ContextGraphWalker, error) {
 	// Walk the graph
 	log.Printf("[INFO] Starting graph walk: %s", operation.String())
 	walker := &ContextGraphWalker{Context: c, Operation: operation}
diff --git a/terraform/context_test.go b/terraform/context_test.go
index d4597f11f93f..a68c17e263e9 100644
--- a/terraform/context_test.go
+++ b/terraform/context_test.go
@@ -2365,6 +2365,25 @@ func TestContext2Validate_countVariableNoDefault(t *testing.T) {
 	}
 }
 
+func TestContext2Validate_cycle(t *testing.T) {
+	p := testProvider("aws")
+	m := testModule(t, "validate-cycle")
+	c := testContext2(t, &ContextOpts{
+		Module: m,
+		Providers: map[string]ResourceProviderFactory{
+			"aws": testProviderFuncFixed(p),
+		},
+	})
+
+	w, e := c.Validate()
+	if len(w) > 0 {
+		t.Fatalf("expected no warns, got: %#v", w)
+	}
+	if len(e) != 1 {
+		t.Fatalf("expected 1 err, got: %s", e)
+	}
+}
+
 func TestContext2Validate_moduleBadOutput(t *testing.T) {
 	p := testProvider("aws")
 	m := testModule(t, "validate-bad-module-output")
diff --git a/terraform/graph_builder.go b/terraform/graph_builder.go
index f08e20f16d6b..0b4b32e20384 100644
--- a/terraform/graph_builder.go
+++ b/terraform/graph_builder.go
@@ -16,9 +16,11 @@ type GraphBuilder interface {
 }
 
 // BasicGraphBuilder is a GraphBuilder that builds a graph out of a
-// series of transforms and validates the graph is a valid structure.
+// series of transforms and (optionally) validates the graph is a valid
+// structure.
 type BasicGraphBuilder struct {
-	Steps []GraphTransformer
+	Steps    []GraphTransformer
+	Validate bool
 }
 
 func (b *BasicGraphBuilder) Build(path []string) (*Graph, error) {
@@ -34,9 +36,11 @@ func (b *BasicGraphBuilder) Build(path []string) (*Graph, error) {
 	}
 
 	// Validate the graph structure
-	if err := g.Validate(); err != nil {
-		log.Printf("[ERROR] Graph validation failed. Graph:\n\n%s", g.String())
-		return nil, err
+	if b.Validate {
+		if err := g.Validate(); err != nil {
+			log.Printf("[ERROR] Graph validation failed. Graph:\n\n%s", g.String())
+			return nil, err
+		}
 	}
 
 	return g, nil
@@ -72,12 +76,23 @@ type BuiltinGraphBuilder struct {
 	// Destroy is set to true when we're in a `terraform destroy` or a
 	// `terraform plan -destroy`
 	Destroy bool
+
+	// Determines whether the GraphBuilder should perform graph validation before
+	// returning the Graph. Generally you want this to be done, except when you'd
+	// like to inspect a problematic graph.
+	Validate bool
+
+	// Verbose is set to true when the graph should be built "worst case",
+	// skipping any prune steps. This is used for early cycle detection during
+	// Validate and for manual inspection via `terraform graph -verbose`.
+	Verbose bool
 }
 
 // Build builds the graph according to the steps returned by Steps.
 func (b *BuiltinGraphBuilder) Build(path []string) (*Graph, error) {
 	basic := &BasicGraphBuilder{
-		Steps: b.Steps(),
+		Steps:    b.Steps(),
+		Validate: b.Validate,
 	}
 
 	return basic.Build(path)
@@ -86,7 +101,7 @@ func (b *BuiltinGraphBuilder) Build(path []string) (*Graph, error) {
 // Steps returns the ordered list of GraphTransformers that must be executed
 // to build a complete graph.
 func (b *BuiltinGraphBuilder) Steps() []GraphTransformer {
-	return []GraphTransformer{
+	steps := []GraphTransformer{
 		// Create all our resources from the configuration and state
 		&ConfigTransformer{Module: b.Root},
 		&OrphanTransformer{
@@ -123,7 +138,10 @@ func (b *BuiltinGraphBuilder) Steps() []GraphTransformer {
 		// Create the destruction nodes
 		&DestroyTransformer{},
 		&CreateBeforeDestroyTransformer{},
-		&PruneDestroyTransformer{Diff: b.Diff, State: b.State},
+		b.conditional(&conditionalOpts{
+			If:   func() bool { return !b.Verbose },
+			Then: &PruneDestroyTransformer{Diff: b.Diff, State: b.State},
+		}),
 
 		// Make sure we create one root
 		&RootTransformer{},
@@ -132,4 +150,25 @@ func (b *BuiltinGraphBuilder) Steps() []GraphTransformer {
 		// more sane if possible (it usually is possible).
 		&TransitiveReductionTransformer{},
 	}
+
+	// Remove nils
+	for i, s := range steps {
+		if s == nil {
+			steps = append(steps[:i], steps[i+1:]...)
+		}
+	}
+
+	return steps
+}
+
+type conditionalOpts struct {
+	If   func() bool
+	Then GraphTransformer
+}
+
+func (b *BuiltinGraphBuilder) conditional(o *conditionalOpts) GraphTransformer {
+	if o.If != nil && o.Then != nil && o.If() {
+		return o.Then
+	}
+	return nil
 }
diff --git a/terraform/graph_builder_test.go b/terraform/graph_builder_test.go
index 23d1eb8babdd..0f66947fcf59 100644
--- a/terraform/graph_builder_test.go
+++ b/terraform/graph_builder_test.go
@@ -41,6 +41,7 @@ func TestBasicGraphBuilder_validate(t *testing.T) {
 			&testBasicGraphBuilderTransform{1},
 			&testBasicGraphBuilderTransform{2},
 		},
+		Validate: true,
 	}
 
 	_, err := b.Build(RootModulePath)
@@ -49,6 +50,21 @@ func TestBasicGraphBuilder_validate(t *testing.T) {
 	}
 }
 
+func TestBasicGraphBuilder_validateOff(t *testing.T) {
+	b := &BasicGraphBuilder{
+		Steps: []GraphTransformer{
+			&testBasicGraphBuilderTransform{1},
+			&testBasicGraphBuilderTransform{2},
+		},
+		Validate: false,
+	}
+
+	_, err := b.Build(RootModulePath)
+	if err != nil {
+		t.Fatalf("expected no error, got: %s", err)
+	}
+}
+
 func TestBuiltinGraphBuilder_impl(t *testing.T) {
 	var _ GraphBuilder = new(BuiltinGraphBuilder)
 }
@@ -58,7 +74,8 @@ func TestBuiltinGraphBuilder_impl(t *testing.T) {
 // specific ordering of steps should be added in other tests.
 func TestBuiltinGraphBuilder(t *testing.T) {
 	b := &BuiltinGraphBuilder{
-		Root: testModule(t, "graph-builder-basic"),
+		Root:     testModule(t, "graph-builder-basic"),
+		Validate: true,
 	}
 
 	g, err := b.Build(RootModulePath)
@@ -73,11 +90,31 @@ func TestBuiltinGraphBuilder(t *testing.T) {
 	}
 }
 
+func TestBuiltinGraphBuilder_Verbose(t *testing.T) {
+	b := &BuiltinGraphBuilder{
+		Root:     testModule(t, "graph-builder-basic"),
+		Validate: true,
+		Verbose:  true,
+	}
+
+	g, err := b.Build(RootModulePath)
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	actual := strings.TrimSpace(g.String())
+	expected := strings.TrimSpace(testBuiltinGraphBuilderVerboseStr)
+	if actual != expected {
+		t.Fatalf("bad: %s", actual)
+	}
+}
+
 // This tests a cycle we got when a CBD resource depends on a non-CBD
 // resource. This cycle shouldn't happen in the general case anymore.
 func TestBuiltinGraphBuilder_cbdDepNonCbd(t *testing.T) {
 	b := &BuiltinGraphBuilder{
-		Root: testModule(t, "graph-builder-cbd-non-cbd"),
+		Root:     testModule(t, "graph-builder-cbd-non-cbd"),
+		Validate: true,
 	}
 
 	_, err := b.Build(RootModulePath)
@@ -86,6 +123,19 @@ func TestBuiltinGraphBuilder_cbdDepNonCbd(t *testing.T) {
 	}
 }
 
+func TestBuiltinGraphBuilder_cbdDepNonCbd_errorsWhenVerbose(t *testing.T) {
+	b := &BuiltinGraphBuilder{
+		Root:     testModule(t, "graph-builder-cbd-non-cbd"),
+		Validate: true,
+		Verbose:  true,
+	}
+
+	_, err := b.Build(RootModulePath)
+	if err == nil {
+		t.Fatalf("expected err, got none")
+	}
+}
+
 /*
 TODO: This exposes a really bad bug we need to fix after we merge
 the f-ast-branch. This bug still exists in master.
@@ -130,6 +180,23 @@ aws_instance.web
 provider.aws
 `
 
+const testBuiltinGraphBuilderVerboseStr = `
+aws_instance.db
+  aws_instance.db (destroy tainted)
+  aws_instance.db (destroy)
+aws_instance.db (destroy tainted)
+  aws_instance.web (destroy tainted)
+aws_instance.db (destroy)
+  aws_instance.web (destroy)
+aws_instance.web
+  aws_instance.db
+aws_instance.web (destroy tainted)
+  provider.aws
+aws_instance.web (destroy)
+  provider.aws
+provider.aws
+`
+
 const testBuiltinGraphBuilderModuleStr = `
 aws_instance.web
   aws_instance.web (destroy)
diff --git a/terraform/test-fixtures/validate-cycle/main.tf b/terraform/test-fixtures/validate-cycle/main.tf
new file mode 100644
index 000000000000..3dc503aa7fc1
--- /dev/null
+++ b/terraform/test-fixtures/validate-cycle/main.tf
@@ -0,0 +1,19 @@
+provider "aws" { }
+
+/*
+ * When a CBD resource depends on a non-CBD resource,
+ * a cycle is formed that only shows up when Destroy
+ * nodes are included in the graph.
+ */
+resource "aws_security_group" "firewall" {
+}
+
+resource "aws_instance" "web" {
+    security_groups = [
+        "foo",
+        "${aws_security_group.firewall.foo}"
+    ]
+    lifecycle {
+      create_before_destroy = true
+    }
+}

From 5e67657325669772497bd78e6c66d6c78353c084 Mon Sep 17 00:00:00 2001
From: Paul Hinze <phinze@phinze.com>
Date: Wed, 15 Apr 2015 13:53:32 -0500
Subject: [PATCH 06/24] core: fix targeting in destroy w/ provisioners

The `TargetTransform` was dropping provisioner nodes, which caused graph
validation to fail with messages about uninitialized provisioners when a
`terraform destroy` was attempted.

This was because `destroy` flops the dependency calculation to try and
address any nodes in the graph that "depend on" the target node. But we
still need to keep the provisioner node in the graph.

Here we switch the strategy for filtering nodes to only drop
addressable, non-targeted nodes. This should prevent us from having to
whitelist nodes to keep in the future.

closes #1541
---
 terraform/context_test.go                     | 42 ++++++++++++++++++
 .../test-fixtures/validate-targeted/main.tf   | 13 ++++++
 terraform/transform_targets.go                | 43 +++++++++----------
 3 files changed, 76 insertions(+), 22 deletions(-)
 create mode 100644 terraform/test-fixtures/validate-targeted/main.tf

diff --git a/terraform/context_test.go b/terraform/context_test.go
index 6ff63d813a7b..e9781918b83c 100644
--- a/terraform/context_test.go
+++ b/terraform/context_test.go
@@ -2688,6 +2688,48 @@ func TestContext2Validate_tainted(t *testing.T) {
 	}
 }
 
+func TestContext2Validate_targetedDestroy(t *testing.T) {
+	m := testModule(t, "validate-targeted")
+	p := testProvider("aws")
+	pr := testProvisioner()
+	p.ApplyFn = testApplyFn
+	p.DiffFn = testDiffFn
+	ctx := testContext2(t, &ContextOpts{
+		Module: m,
+		Providers: map[string]ResourceProviderFactory{
+			"aws": testProviderFuncFixed(p),
+		},
+		Provisioners: map[string]ResourceProvisionerFactory{
+			"shell": testProvisionerFuncFixed(pr),
+		},
+		State: &State{
+			Modules: []*ModuleState{
+				&ModuleState{
+					Path: rootModulePath,
+					Resources: map[string]*ResourceState{
+						"aws_instance.foo": resourceState("aws_instance", "i-bcd345"),
+						"aws_instance.bar": resourceState("aws_instance", "i-abc123"),
+					},
+				},
+			},
+		},
+		Targets: []string{"aws_instance.foo"},
+		Destroy: true,
+	})
+
+	w, e := ctx.Validate()
+	if len(w) > 0 {
+		warnStr := ""
+		for _, v := range w {
+			warnStr = warnStr + " " + v
+		}
+		t.Fatalf("bad: %s", warnStr)
+	}
+	if len(e) > 0 {
+		t.Fatalf("bad: %s", e)
+	}
+}
+
 func TestContext2Validate_varRef(t *testing.T) {
 	m := testModule(t, "validate-variable-ref")
 	p := testProvider("aws")
diff --git a/terraform/test-fixtures/validate-targeted/main.tf b/terraform/test-fixtures/validate-targeted/main.tf
new file mode 100644
index 000000000000..b40b126faa36
--- /dev/null
+++ b/terraform/test-fixtures/validate-targeted/main.tf
@@ -0,0 +1,13 @@
+resource "aws_instance" "foo" {
+    num = "2"
+    provisioner "shell" {
+       command = "echo hi"
+    }
+}
+
+resource "aws_instance" "bar" {
+    foo = "bar"
+    provisioner "shell" {
+       command = "echo hi"
+    }
+}
diff --git a/terraform/transform_targets.go b/terraform/transform_targets.go
index 29a6d53c6fbd..1ddb7f9c46bd 100644
--- a/terraform/transform_targets.go
+++ b/terraform/transform_targets.go
@@ -28,9 +28,10 @@ func (t *TargetsTransformer) Transform(g *Graph) error {
 		}
 
 		for _, v := range g.Vertices() {
-			if targetedNodes.Include(v) {
-			} else {
-				g.Remove(v)
+			if _, ok := v.(GraphNodeAddressable); ok {
+				if !targetedNodes.Include(v) {
+					g.Remove(v)
+				}
 			}
 		}
 	}
@@ -49,35 +50,29 @@ func (t *TargetsTransformer) parseTargetAddresses() ([]ResourceAddress, error) {
 	return addrs, nil
 }
 
+// Returns the list of targeted nodes. A targeted node is either addressed
+// directly, or is an Ancestor of a targeted node. Destroy mode keeps
+// Descendents instead of Ancestors.
 func (t *TargetsTransformer) selectTargetedNodes(
 	g *Graph, addrs []ResourceAddress) (*dag.Set, error) {
 	targetedNodes := new(dag.Set)
 	for _, v := range g.Vertices() {
-		// Keep all providers; they'll be pruned later if necessary
-		if r, ok := v.(GraphNodeProvider); ok {
-			targetedNodes.Add(r)
-			continue
-		}
+		if t.nodeIsTarget(v, addrs) {
+			targetedNodes.Add(v)
 
-		// For the remaining filter, we only care about addressable nodes
-		r, ok := v.(GraphNodeAddressable)
-		if !ok {
-			continue
-		}
-
-		if t.nodeIsTarget(r, addrs) {
-			targetedNodes.Add(r)
-			// If the node would like to know about targets, tell it.
-			if n, ok := r.(GraphNodeTargetable); ok {
-				n.SetTargets(addrs)
+			// We inform nodes that ask about the list of targets - helps for nodes
+			// that need to dynamically expand. Note that this only occurs for nodes
+			// that are already directly targeted.
+			if tn, ok := v.(GraphNodeTargetable); ok {
+				tn.SetTargets(addrs)
 			}
 
 			var deps *dag.Set
 			var err error
 			if t.Destroy {
-				deps, err = g.Descendents(r)
+				deps, err = g.Descendents(v)
 			} else {
-				deps, err = g.Ancestors(r)
+				deps, err = g.Ancestors(v)
 			}
 			if err != nil {
 				return nil, err
@@ -92,7 +87,11 @@ func (t *TargetsTransformer) selectTargetedNodes(
 }
 
 func (t *TargetsTransformer) nodeIsTarget(
-	r GraphNodeAddressable, addrs []ResourceAddress) bool {
+	v dag.Vertex, addrs []ResourceAddress) bool {
+	r, ok := v.(GraphNodeAddressable)
+	if !ok {
+		return false
+	}
 	addr := r.ResourceAddress()
 	for _, targetAddr := range addrs {
 		if targetAddr.Equals(addr) {

From ce49dd60806e3f8b739dc713e73eff31699f8d0d Mon Sep 17 00:00:00 2001
From: Paul Hinze <phinze@phinze.com>
Date: Thu, 23 Apr 2015 13:24:26 -0500
Subject: [PATCH 07/24] core: graph command gets -verbose and -draw-cycles

When you specify `-verbose` you'll get the whole graph of operations,
which gives a better idea of the operations terraform performs and in
what order.

The DOT graph is now generated with a small internal library instead of
simple string building. This allows us to ensure the graph generation is
as consistent as possible, among other benefits.

We set `newrank = true` in the graph, which I've found does just as good
a job organizing things visually as manually attempting to rank the nodes
based on depth.

This also fixes `-module-depth`, which was broken post-AST refector.
Modules are now expanded into subgraphs with labels and borders. We
have yet to regain the plan graphing functionality, so I removed that
from the docs for now.

Finally, if `-draw-cycles` is added, extra colored edges will be drawn
to indicate the path of any cycles detected in the graph.

A notable implementation change included here is that
{Reverse,}DepthFirstWalk has been made deterministic. (Before it was
dependent on `map` ordering.) This turned out to be unnecessary to gain
determinism in the final DOT-level implementation, but it seemed
a desirable enough of a property that I left it in.
---
 command/graph.go                              |  35 ++-
 dag/dag.go                                    | 118 ++++---
 dot/graph.go                                  | 224 ++++++++++++++
 dot/graph_writer.go                           |  47 +++
 terraform/graph_config_node.go                |  50 ++-
 terraform/graph_dot.go                        | 184 ++++++++---
 terraform/graph_dot_test.go                   | 287 ++++++++++++++++++
 terraform/transform_provider.go               |  32 +-
 terraform/transform_root.go                   |  10 +-
 .../source/docs/commands/graph.html.markdown  |  18 +-
 10 files changed, 875 insertions(+), 130 deletions(-)
 create mode 100644 dot/graph.go
 create mode 100644 dot/graph_writer.go
 create mode 100644 terraform/graph_dot_test.go

diff --git a/command/graph.go b/command/graph.go
index 8bd5413facf9..97473f5ce3aa 100644
--- a/command/graph.go
+++ b/command/graph.go
@@ -17,11 +17,15 @@ type GraphCommand struct {
 
 func (c *GraphCommand) Run(args []string) int {
 	var moduleDepth int
+	var verbose bool
+	var drawCycles bool
 
 	args = c.Meta.process(args, false)
 
 	cmdFlags := flag.NewFlagSet("graph", flag.ContinueOnError)
 	cmdFlags.IntVar(&moduleDepth, "module-depth", 0, "module-depth")
+	cmdFlags.BoolVar(&verbose, "verbose", false, "verbose")
+	cmdFlags.BoolVar(&drawCycles, "draw-cycles", false, "draw-cycles")
 	cmdFlags.Usage = func() { c.Ui.Error(c.Help()) }
 	if err := cmdFlags.Parse(args); err != nil {
 		return 1
@@ -52,28 +56,38 @@ func (c *GraphCommand) Run(args []string) int {
 		return 1
 	}
 
+	// Skip validation during graph generation - we want to see the graph even if
+	// it is invalid for some reason.
 	g, err := ctx.Graph(&terraform.ContextGraphOpts{
-		Validate: true,
-		Verbose:  false,
+		Verbose:  verbose,
+		Validate: false,
 	})
 	if err != nil {
 		c.Ui.Error(fmt.Sprintf("Error creating graph: %s", err))
 		return 1
 	}
 
-	c.Ui.Output(terraform.GraphDot(g, nil))
+	graphStr, err := terraform.GraphDot(g, &terraform.GraphDotOpts{
+		DrawCycles: drawCycles,
+		MaxDepth:   moduleDepth,
+		Verbose:    verbose,
+	})
+	if err != nil {
+		c.Ui.Error(fmt.Sprintf("Error converting graph: %s", err))
+		return 1
+	}
+
+	c.Ui.Output(graphStr)
 
 	return 0
 }
 
 func (c *GraphCommand) Help() string {
 	helpText := `
-Usage: terraform graph [options] PATH
+Usage: terraform graph [options] [DIR]
 
-  Outputs the visual graph of Terraform resources. If the path given is
-  the path to a configuration, the dependency graph of the resources are
-  shown. If the path is a plan file, then the dependency graph of the
-  plan itself is shown.
+  Outputs the visual dependency graph of Terraform resources according to
+  configuration files in DIR (or the current directory if omitted).
 
   The graph is outputted in DOT format. The typical program that can
   read this format is GraphViz, but many web services are also available
@@ -81,9 +95,14 @@ Usage: terraform graph [options] PATH
 
 Options:
 
+  -draw-cycles         Highlight any cycles in the graph with colored edges.
+                       This helps when diagnosing cycle errors.
+
   -module-depth=n      The maximum depth to expand modules. By default this is
                        zero, which will not expand modules at all.
 
+  -verbose             Generate a verbose, "worst-case" graph, with all nodes
+                       for potential operations in place.
 `
 	return strings.TrimSpace(helpText)
 }
diff --git a/dag/dag.go b/dag/dag.go
index 0f53fb1f00b3..0148ee07ca13 100644
--- a/dag/dag.go
+++ b/dag/dag.go
@@ -2,6 +2,7 @@ package dag
 
 import (
 	"fmt"
+	"sort"
 	"strings"
 	"sync"
 
@@ -17,17 +18,21 @@ type AcyclicGraph struct {
 // WalkFunc is the callback used for walking the graph.
 type WalkFunc func(Vertex) error
 
+// DepthWalkFunc is a walk function that also receives the current depth of the
+// walk as an argument
+type DepthWalkFunc func(Vertex, int) error
+
 // Returns a Set that includes every Vertex yielded by walking down from the
 // provided starting Vertex v.
 func (g *AcyclicGraph) Ancestors(v Vertex) (*Set, error) {
 	s := new(Set)
-	start := asVertexList(g.DownEdges(v))
-	memoFunc := func(v Vertex) error {
+	start := AsVertexList(g.DownEdges(v))
+	memoFunc := func(v Vertex, d int) error {
 		s.Add(v)
 		return nil
 	}
 
-	if err := g.depthFirstWalk(start, memoFunc); err != nil {
+	if err := g.DepthFirstWalk(start, memoFunc); err != nil {
 		return nil, err
 	}
 
@@ -38,13 +43,13 @@ func (g *AcyclicGraph) Ancestors(v Vertex) (*Set, error) {
 // provided starting Vertex v.
 func (g *AcyclicGraph) Descendents(v Vertex) (*Set, error) {
 	s := new(Set)
-	start := asVertexList(g.UpEdges(v))
-	memoFunc := func(v Vertex) error {
+	start := AsVertexList(g.UpEdges(v))
+	memoFunc := func(v Vertex, d int) error {
 		s.Add(v)
 		return nil
 	}
 
-	if err := g.reverseDepthFirstWalk(start, memoFunc); err != nil {
+	if err := g.ReverseDepthFirstWalk(start, memoFunc); err != nil {
 		return nil, err
 	}
 
@@ -92,14 +97,13 @@ func (g *AcyclicGraph) TransitiveReduction() {
 	// v such that the edge (u,v) exists (v is a direct descendant of u).
 	//
 	// For each v-prime reachable from v, remove the edge (u, v-prime).
-
 	for _, u := range g.Vertices() {
 		uTargets := g.DownEdges(u)
-		vs := asVertexList(g.DownEdges(u))
+		vs := AsVertexList(g.DownEdges(u))
 
-		g.depthFirstWalk(vs, func(v Vertex) error {
+		g.DepthFirstWalk(vs, func(v Vertex, d int) error {
 			shared := uTargets.Intersection(g.DownEdges(v))
-			for _, vPrime := range asVertexList(shared) {
+			for _, vPrime := range AsVertexList(shared) {
 				g.RemoveEdge(BasicEdge(u, vPrime))
 			}
 
@@ -117,12 +121,7 @@ func (g *AcyclicGraph) Validate() error {
 
 	// Look for cycles of more than 1 component
 	var err error
-	var cycles [][]Vertex
-	for _, cycle := range StronglyConnected(&g.Graph) {
-		if len(cycle) > 1 {
-			cycles = append(cycles, cycle)
-		}
-	}
+	cycles := g.Cycles()
 	if len(cycles) > 0 {
 		for _, cycle := range cycles {
 			cycleStr := make([]string, len(cycle))
@@ -146,6 +145,16 @@ func (g *AcyclicGraph) Validate() error {
 	return err
 }
 
+func (g *AcyclicGraph) Cycles() [][]Vertex {
+	var cycles [][]Vertex
+	for _, cycle := range StronglyConnected(&g.Graph) {
+		if len(cycle) > 1 {
+			cycles = append(cycles, cycle)
+		}
+	}
+	return cycles
+}
+
 // Walk walks the graph, calling your callback as each node is visited.
 // This will walk nodes in parallel if it can. Because the walk is done
 // in parallel, the error returned will be a multierror.
@@ -175,7 +184,7 @@ func (g *AcyclicGraph) Walk(cb WalkFunc) error {
 	for _, v := range vertices {
 		// Build our list of dependencies and the list of channels to
 		// wait on until we start executing for this vertex.
-		deps := asVertexList(g.DownEdges(v))
+		deps := AsVertexList(g.DownEdges(v))
 		depChs := make([]<-chan struct{}, len(deps))
 		for i, dep := range deps {
 			depChs[i] = vertMap[dep]
@@ -229,7 +238,7 @@ func (g *AcyclicGraph) Walk(cb WalkFunc) error {
 }
 
 // simple convenience helper for converting a dag.Set to a []Vertex
-func asVertexList(s *Set) []Vertex {
+func AsVertexList(s *Set) []Vertex {
 	rawList := s.List()
 	vertexList := make([]Vertex, len(rawList))
 	for i, raw := range rawList {
@@ -238,13 +247,23 @@ func asVertexList(s *Set) []Vertex {
 	return vertexList
 }
 
+type vertexAtDepth struct {
+	Vertex Vertex
+	Depth  int
+}
+
 // depthFirstWalk does a depth-first walk of the graph starting from
 // the vertices in start. This is not exported now but it would make sense
 // to export this publicly at some point.
-func (g *AcyclicGraph) depthFirstWalk(start []Vertex, cb WalkFunc) error {
+func (g *AcyclicGraph) DepthFirstWalk(start []Vertex, f DepthWalkFunc) error {
 	seen := make(map[Vertex]struct{})
-	frontier := make([]Vertex, len(start))
-	copy(frontier, start)
+	frontier := make([]*vertexAtDepth, len(start))
+	for i, v := range start {
+		frontier[i] = &vertexAtDepth{
+			Vertex: v,
+			Depth:  0,
+		}
+	}
 	for len(frontier) > 0 {
 		// Pop the current vertex
 		n := len(frontier)
@@ -252,20 +271,24 @@ func (g *AcyclicGraph) depthFirstWalk(start []Vertex, cb WalkFunc) error {
 		frontier = frontier[:n-1]
 
 		// Check if we've seen this already and return...
-		if _, ok := seen[current]; ok {
+		if _, ok := seen[current.Vertex]; ok {
 			continue
 		}
-		seen[current] = struct{}{}
+		seen[current.Vertex] = struct{}{}
 
 		// Visit the current node
-		if err := cb(current); err != nil {
+		if err := f(current.Vertex, current.Depth); err != nil {
 			return err
 		}
 
-		// Visit targets of this in reverse order.
-		targets := g.DownEdges(current).List()
-		for i := len(targets) - 1; i >= 0; i-- {
-			frontier = append(frontier, targets[i].(Vertex))
+		// Visit targets of this in a consistent order.
+		targets := AsVertexList(g.DownEdges(current.Vertex))
+		sort.Sort(byVertexName(targets))
+		for _, t := range targets {
+			frontier = append(frontier, &vertexAtDepth{
+				Vertex: t,
+				Depth:  current.Depth + 1,
+			})
 		}
 	}
 
@@ -274,10 +297,15 @@ func (g *AcyclicGraph) depthFirstWalk(start []Vertex, cb WalkFunc) error {
 
 // reverseDepthFirstWalk does a depth-first walk _up_ the graph starting from
 // the vertices in start.
-func (g *AcyclicGraph) reverseDepthFirstWalk(start []Vertex, cb WalkFunc) error {
+func (g *AcyclicGraph) ReverseDepthFirstWalk(start []Vertex, f DepthWalkFunc) error {
 	seen := make(map[Vertex]struct{})
-	frontier := make([]Vertex, len(start))
-	copy(frontier, start)
+	frontier := make([]*vertexAtDepth, len(start))
+	for i, v := range start {
+		frontier[i] = &vertexAtDepth{
+			Vertex: v,
+			Depth:  0,
+		}
+	}
 	for len(frontier) > 0 {
 		// Pop the current vertex
 		n := len(frontier)
@@ -285,22 +313,36 @@ func (g *AcyclicGraph) reverseDepthFirstWalk(start []Vertex, cb WalkFunc) error
 		frontier = frontier[:n-1]
 
 		// Check if we've seen this already and return...
-		if _, ok := seen[current]; ok {
+		if _, ok := seen[current.Vertex]; ok {
 			continue
 		}
-		seen[current] = struct{}{}
+		seen[current.Vertex] = struct{}{}
 
 		// Visit the current node
-		if err := cb(current); err != nil {
+		if err := f(current.Vertex, current.Depth); err != nil {
 			return err
 		}
 
-		// Visit targets of this in reverse order.
-		targets := g.UpEdges(current).List()
-		for i := len(targets) - 1; i >= 0; i-- {
-			frontier = append(frontier, targets[i].(Vertex))
+		// Visit targets of this in a consistent order.
+		targets := AsVertexList(g.UpEdges(current.Vertex))
+		sort.Sort(byVertexName(targets))
+		for _, t := range targets {
+			frontier = append(frontier, &vertexAtDepth{
+				Vertex: t,
+				Depth:  current.Depth + 1,
+			})
 		}
 	}
 
 	return nil
 }
+
+// byVertexName implements sort.Interface so a list of Vertices can be sorted
+// consistently by their VertexName
+type byVertexName []Vertex
+
+func (b byVertexName) Len() int      { return len(b) }
+func (b byVertexName) Swap(i, j int) { b[i], b[j] = b[j], b[i] }
+func (b byVertexName) Less(i, j int) bool {
+	return VertexName(b[i]) < VertexName(b[j])
+}
diff --git a/dot/graph.go b/dot/graph.go
new file mode 100644
index 000000000000..91fe9fc601e8
--- /dev/null
+++ b/dot/graph.go
@@ -0,0 +1,224 @@
+// The dot package contains utilities for working with DOT graphs.
+package dot
+
+import (
+	"bytes"
+	"fmt"
+	"sort"
+	"strings"
+)
+
+// Graph is a representation of a drawable DOT graph.
+type Graph struct {
+	// Whether this is a "digraph" or just a "graph"
+	Directed bool
+
+	// Used for K/V settings in the DOT
+	Attrs map[string]string
+
+	Nodes     []*Node
+	Edges     []*Edge
+	Subgraphs []*Subgraph
+
+	nodesByName map[string]*Node
+}
+
+// Subgraph is a Graph that lives inside a Parent graph, and contains some
+// additional parameters to control how it is drawn.
+type Subgraph struct {
+	Graph
+	Name    string
+	Parent  *Graph
+	Cluster bool
+}
+
+// An Edge in a DOT graph, as expressed by recording the Name of the Node at
+// each end.
+type Edge struct {
+	// Name of source node.
+	Source string
+
+	// Name of dest node.
+	Dest string
+
+	// List of K/V attributes for this edge.
+	Attrs map[string]string
+}
+
+// A Node in a DOT graph.
+type Node struct {
+	Name  string
+	Attrs map[string]string
+}
+
+// Creates a properly initialized DOT Graph.
+func NewGraph(attrs map[string]string) *Graph {
+	return &Graph{
+		Attrs:       attrs,
+		nodesByName: make(map[string]*Node),
+	}
+}
+
+func NewEdge(src, dst string, attrs map[string]string) *Edge {
+	return &Edge{
+		Source: src,
+		Dest:   dst,
+		Attrs:  attrs,
+	}
+}
+
+func NewNode(n string, attrs map[string]string) *Node {
+	return &Node{
+		Name:  n,
+		Attrs: attrs,
+	}
+}
+
+// Initializes a Subgraph with the provided name, attaches is to this Graph,
+// and returns it.
+func (g *Graph) AddSubgraph(name string) *Subgraph {
+	subgraph := &Subgraph{
+		Graph:  *NewGraph(map[string]string{}),
+		Parent: g,
+		Name:   name,
+	}
+	g.Subgraphs = append(g.Subgraphs, subgraph)
+	return subgraph
+}
+
+func (g *Graph) AddAttr(k, v string) {
+	g.Attrs[k] = v
+}
+
+func (g *Graph) AddNode(n *Node) {
+	g.Nodes = append(g.Nodes, n)
+	g.nodesByName[n.Name] = n
+}
+
+func (g *Graph) AddEdge(e *Edge) {
+	g.Edges = append(g.Edges, e)
+}
+
+// Adds an edge between two Nodes.
+//
+// Note this does not do any verification of the existence of these nodes,
+// which means that any strings you provide that are not existing nodes will
+// result in extra auto-defined nodes in your resulting DOT.
+func (g *Graph) AddEdgeBetween(src, dst string, attrs map[string]string) error {
+	g.AddEdge(NewEdge(src, dst, attrs))
+
+	return nil
+}
+
+// Look up a node by name
+func (g *Graph) GetNode(name string) (*Node, error) {
+	node, ok := g.nodesByName[name]
+	if !ok {
+		return nil, fmt.Errorf("Could not find node: %s", name)
+	}
+	return node, nil
+}
+
+// Returns the DOT representation of this Graph.
+func (g *Graph) String() string {
+	w := newGraphWriter()
+
+	g.drawHeader(w)
+	w.Indent()
+	g.drawBody(w)
+	w.Unindent()
+	g.drawFooter(w)
+
+	return w.String()
+}
+
+func (g *Graph) drawHeader(w *graphWriter) {
+	if g.Directed {
+		w.Printf("digraph {\n")
+	} else {
+		w.Printf("graph {\n")
+	}
+}
+
+func (g *Graph) drawBody(w *graphWriter) {
+	for _, as := range attrStrings(g.Attrs) {
+		w.Printf("%s\n", as)
+	}
+
+	nodeStrings := make([]string, 0, len(g.Nodes))
+	for _, n := range g.Nodes {
+		nodeStrings = append(nodeStrings, n.String())
+	}
+	sort.Strings(nodeStrings)
+	for _, ns := range nodeStrings {
+		w.Printf(ns)
+	}
+
+	edgeStrings := make([]string, 0, len(g.Edges))
+	for _, e := range g.Edges {
+		edgeStrings = append(edgeStrings, e.String())
+	}
+	sort.Strings(edgeStrings)
+	for _, es := range edgeStrings {
+		w.Printf(es)
+	}
+
+	for _, s := range g.Subgraphs {
+		s.drawHeader(w)
+		w.Indent()
+		s.drawBody(w)
+		w.Unindent()
+		s.drawFooter(w)
+	}
+}
+
+func (g *Graph) drawFooter(w *graphWriter) {
+	w.Printf("}\n")
+}
+
+// Returns the DOT representation of this Edge.
+func (e *Edge) String() string {
+	var buf bytes.Buffer
+	buf.WriteString(
+		fmt.Sprintf(
+			"%q -> %q", e.Source, e.Dest))
+	writeAttrs(&buf, e.Attrs)
+	buf.WriteString("\n")
+
+	return buf.String()
+}
+
+func (s *Subgraph) drawHeader(w *graphWriter) {
+	name := s.Name
+	if s.Cluster {
+		name = fmt.Sprintf("cluster_%s", name)
+	}
+	w.Printf("subgraph %q {\n", name)
+}
+
+// Returns the DOT representation of this Node.
+func (n *Node) String() string {
+	var buf bytes.Buffer
+	buf.WriteString(fmt.Sprintf("%q", n.Name))
+	writeAttrs(&buf, n.Attrs)
+	buf.WriteString("\n")
+
+	return buf.String()
+}
+
+func writeAttrs(buf *bytes.Buffer, attrs map[string]string) {
+	if len(attrs) > 0 {
+		buf.WriteString(" [")
+		buf.WriteString(strings.Join(attrStrings(attrs), ", "))
+		buf.WriteString("]")
+	}
+}
+
+func attrStrings(attrs map[string]string) []string {
+	strings := make([]string, 0, len(attrs))
+	for k, v := range attrs {
+		strings = append(strings, fmt.Sprintf("%s = %q", k, v))
+	}
+	sort.Strings(strings)
+	return strings
+}
diff --git a/dot/graph_writer.go b/dot/graph_writer.go
new file mode 100644
index 000000000000..7fa5d9cac66b
--- /dev/null
+++ b/dot/graph_writer.go
@@ -0,0 +1,47 @@
+package dot
+
+import (
+	"bytes"
+	"fmt"
+)
+
+// graphWriter wraps a bytes.Buffer and tracks indent level levels.
+type graphWriter struct {
+	bytes.Buffer
+	indent    int
+	indentStr string
+}
+
+// Returns an initialized graphWriter at indent level 0.
+func newGraphWriter() *graphWriter {
+	w := &graphWriter{
+		indent: 0,
+	}
+	w.init()
+	return w
+}
+
+// Prints to the buffer at the current indent level.
+func (w *graphWriter) Printf(s string, args ...interface{}) {
+	w.WriteString(w.indentStr + fmt.Sprintf(s, args...))
+}
+
+// Increase the indent level.
+func (w *graphWriter) Indent() {
+	w.indent++
+	w.init()
+}
+
+// Decrease the indent level.
+func (w *graphWriter) Unindent() {
+	w.indent--
+	w.init()
+}
+
+func (w *graphWriter) init() {
+	indentBuf := new(bytes.Buffer)
+	for i := 0; i < w.indent; i++ {
+		indentBuf.WriteString("\t")
+	}
+	w.indentStr = indentBuf.String()
+}
diff --git a/terraform/graph_config_node.go b/terraform/graph_config_node.go
index abe1e58c1089..ac5af2e5947a 100644
--- a/terraform/graph_config_node.go
+++ b/terraform/graph_config_node.go
@@ -7,6 +7,7 @@ import (
 	"github.com/hashicorp/terraform/config"
 	"github.com/hashicorp/terraform/config/module"
 	"github.com/hashicorp/terraform/dag"
+	"github.com/hashicorp/terraform/dot"
 )
 
 // graphNodeConfig is an interface that all graph nodes for the
@@ -219,14 +220,16 @@ func (n *GraphNodeConfigProvider) ProviderConfig() *config.RawConfig {
 }
 
 // GraphNodeDotter impl.
-func (n *GraphNodeConfigProvider) Dot(name string) string {
-	return fmt.Sprintf(
-		"\"%s\" [\n"+
-			"\tlabel=\"%s\"\n"+
-			"\tshape=diamond\n"+
-			"];",
-		name,
-		n.Name())
+func (n *GraphNodeConfigProvider) DotNode(name string, opts *GraphDotOpts) *dot.Node {
+	return dot.NewNode(name, map[string]string{
+		"label": n.Name(),
+		"shape": "diamond",
+	})
+}
+
+// GraphNodeDotterOrigin impl.
+func (n *GraphNodeConfigProvider) DotOrigin() bool {
+	return true
 }
 
 // GraphNodeConfigResource represents a resource within the config graph.
@@ -318,18 +321,14 @@ func (n *GraphNodeConfigResource) Name() string {
 }
 
 // GraphNodeDotter impl.
-func (n *GraphNodeConfigResource) Dot(name string) string {
-	if n.DestroyMode != DestroyNone {
-		return ""
+func (n *GraphNodeConfigResource) DotNode(name string, opts *GraphDotOpts) *dot.Node {
+	if n.DestroyMode != DestroyNone && !opts.Verbose {
+		return nil
 	}
-
-	return fmt.Sprintf(
-		"\"%s\" [\n"+
-			"\tlabel=\"%s\"\n"+
-			"\tshape=box\n"+
-			"];",
-		name,
-		n.Name())
+	return dot.NewNode(name, map[string]string{
+		"label": n.Name(),
+		"shape": "box",
+	})
 }
 
 // GraphNodeDynamicExpandable impl.
@@ -635,14 +634,11 @@ func (n *graphNodeModuleExpanded) ConfigType() GraphNodeConfigType {
 }
 
 // GraphNodeDotter impl.
-func (n *graphNodeModuleExpanded) Dot(name string) string {
-	return fmt.Sprintf(
-		"\"%s\" [\n"+
-			"\tlabel=\"%s\"\n"+
-			"\tshape=component\n"+
-			"];",
-		name,
-		dag.VertexName(n.Original))
+func (n *graphNodeModuleExpanded) DotNode(name string, opts *GraphDotOpts) *dot.Node {
+	return dot.NewNode(name, map[string]string{
+		"label": dag.VertexName(n.Original),
+		"shape": "component",
+	})
 }
 
 // GraphNodeEvalable impl.
diff --git a/terraform/graph_dot.go b/terraform/graph_dot.go
index 2e420c637c5e..b2c064e63933 100644
--- a/terraform/graph_dot.go
+++ b/terraform/graph_dot.go
@@ -1,12 +1,10 @@
 package terraform
 
 import (
-	"bufio"
-	"bytes"
 	"fmt"
-	"strings"
 
 	"github.com/hashicorp/terraform/dag"
+	"github.com/hashicorp/terraform/dot"
 )
 
 // GraphNodeDotter can be implemented by a node to cause it to be included
@@ -14,58 +12,174 @@ import (
 // return a representation of this node.
 type GraphNodeDotter interface {
 	// Dot is called to return the dot formatting for the node.
-	// The parameter must be the title of the node.
-	Dot(string) string
+	// The first parameter is the title of the node.
+	// The second parameter includes user-specified options that affect the dot
+	// graph. See GraphDotOpts below for details.
+	DotNode(string, *GraphDotOpts) *dot.Node
+}
+
+type GraphNodeDotOrigin interface {
+	DotOrigin() bool
 }
 
 // GraphDotOpts are the options for generating a dot formatted Graph.
-type GraphDotOpts struct{}
+type GraphDotOpts struct {
+	// Allows some nodes to decide to only show themselves when the user has
+	// requested the "verbose" graph.
+	Verbose bool
+
+	// Highlight Cycles
+	DrawCycles bool
+
+	// How many levels to expand modules as we draw
+	MaxDepth int
+}
 
 // GraphDot returns the dot formatting of a visual representation of
 // the given Terraform graph.
-func GraphDot(g *Graph, opts *GraphDotOpts) string {
-	buf := new(bytes.Buffer)
+func GraphDot(g *Graph, opts *GraphDotOpts) (string, error) {
+	dg := dot.NewGraph(map[string]string{
+		"compound": "true",
+		"newrank":  "true",
+	})
+	dg.Directed = true
+
+	err := graphDotSubgraph(dg, "root", g, opts, 0)
+	if err != nil {
+		return "", err
+	}
+
+	return dg.String(), nil
+}
 
-	// Start the graph
-	buf.WriteString("digraph {\n")
-	buf.WriteString("\tcompound = true;\n")
+func graphDotSubgraph(
+	dg *dot.Graph, modName string, g *Graph, opts *GraphDotOpts, modDepth int) error {
+	// Respect user-specified module depth
+	if opts.MaxDepth >= 0 && modDepth > opts.MaxDepth {
+		return nil
+	}
+
+	// Begin module subgraph
+	var sg *dot.Subgraph
+	if modDepth == 0 {
+		sg = dg.AddSubgraph(modName)
+	} else {
+		sg = dg.AddSubgraph(modName)
+		sg.Cluster = true
+		sg.AddAttr("label", modName)
+	}
 
-	// Go through all the vertices and draw it
-	vertices := g.Vertices()
-	dotVertices := make(map[dag.Vertex]struct{}, len(vertices))
-	for _, v := range vertices {
+	origins, err := graphDotFindOrigins(g)
+	if err != nil {
+		return err
+	}
+
+	drawableVertices := make(map[dag.Vertex]struct{})
+	toDraw := make([]dag.Vertex, 0, len(g.Vertices()))
+	subgraphVertices := make(map[dag.Vertex]*Graph)
+
+	walk := func(v dag.Vertex, depth int) error {
+		// We only care about nodes that yield non-empty Dot strings.
 		if dn, ok := v.(GraphNodeDotter); !ok {
-			continue
-		} else if dn.Dot("fake") == "" {
-			continue
+			return nil
+		} else if dn.DotNode("fake", opts) == nil {
+			return nil
 		}
 
-		dotVertices[v] = struct{}{}
+		drawableVertices[v] = struct{}{}
+		toDraw = append(toDraw, v)
+
+		if sn, ok := v.(GraphNodeSubgraph); ok {
+			subgraphVertices[v] = sn.Subgraph()
+		}
+		return nil
+	}
+
+	if err := g.ReverseDepthFirstWalk(origins, walk); err != nil {
+		return err
 	}
 
-	for v, _ := range dotVertices {
+	for _, v := range toDraw {
 		dn := v.(GraphNodeDotter)
-		scanner := bufio.NewScanner(strings.NewReader(
-			dn.Dot(dag.VertexName(v))))
-		for scanner.Scan() {
-			buf.WriteString("\t" + scanner.Text() + "\n")
-		}
+		nodeName := graphDotNodeName(modName, v)
+		sg.AddNode(dn.DotNode(nodeName, opts))
 
-		// Draw all the edges
-		for _, t := range g.DownEdges(v).List() {
+		// Draw all the edges from this vertex to other nodes
+		targets := dag.AsVertexList(g.DownEdges(v))
+		for _, t := range targets {
 			target := t.(dag.Vertex)
-			if _, ok := dotVertices[target]; !ok {
+			// Only want edges where both sides are drawable.
+			if _, ok := drawableVertices[target]; !ok {
 				continue
 			}
 
-			buf.WriteString(fmt.Sprintf(
-				"\t\"%s\" -> \"%s\";\n",
-				dag.VertexName(v),
-				dag.VertexName(target)))
+			if err := sg.AddEdgeBetween(
+				graphDotNodeName(modName, v),
+				graphDotNodeName(modName, target),
+				map[string]string{}); err != nil {
+				return err
+			}
+		}
+	}
+
+	// Recurse into any subgraphs
+	for _, v := range toDraw {
+		subgraph, ok := subgraphVertices[v]
+		if !ok {
+			continue
+		}
+
+		err := graphDotSubgraph(dg, dag.VertexName(v), subgraph, opts, modDepth+1)
+		if err != nil {
+			return err
+		}
+	}
+
+	if opts.DrawCycles {
+		colors := []string{"red", "green", "blue"}
+		for ci, cycle := range g.Cycles() {
+			for i, c := range cycle {
+				// Catch the last wrapping edge of the cycle
+				if i+1 >= len(cycle) {
+					i = -1
+				}
+				edgeAttrs := map[string]string{
+					"color":    colors[ci%len(colors)],
+					"penwidth": "2.0",
+				}
+
+				if err := sg.AddEdgeBetween(
+					graphDotNodeName(modName, c),
+					graphDotNodeName(modName, cycle[i+1]),
+					edgeAttrs); err != nil {
+					return err
+				}
+
+			}
 		}
 	}
 
-	// End the graph
-	buf.WriteString("}\n")
-	return buf.String()
+	return nil
+}
+
+func graphDotNodeName(modName, v dag.Vertex) string {
+	return fmt.Sprintf("[%s] %s", modName, dag.VertexName(v))
+}
+
+func graphDotFindOrigins(g *Graph) ([]dag.Vertex, error) {
+	var origin []dag.Vertex
+
+	for _, v := range g.Vertices() {
+		if dr, ok := v.(GraphNodeDotOrigin); ok {
+			if dr.DotOrigin() {
+				origin = append(origin, v)
+			}
+		}
+	}
+
+	if len(origin) == 0 {
+		return nil, fmt.Errorf("No DOT origin nodes found.\nGraph: %s", g)
+	}
+
+	return origin, nil
 }
diff --git a/terraform/graph_dot_test.go b/terraform/graph_dot_test.go
new file mode 100644
index 000000000000..da0e1f55ed77
--- /dev/null
+++ b/terraform/graph_dot_test.go
@@ -0,0 +1,287 @@
+package terraform
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/hashicorp/terraform/dot"
+)
+
+func TestGraphDot(t *testing.T) {
+	cases := map[string]struct {
+		Graph  testGraphFunc
+		Opts   GraphDotOpts
+		Expect string
+		Error  string
+	}{
+		"empty": {
+			Graph: func() *Graph { return &Graph{} },
+			Error: "No DOT origin nodes found",
+		},
+		"three-level": {
+			Graph: func() *Graph {
+				var g Graph
+				root := &testDrawableOrigin{"root"}
+				g.Add(root)
+
+				levelOne := []string{"foo", "bar"}
+				for _, s := range levelOne {
+					g.Add(&testDrawable{
+						VertexName:      s,
+						DependentOnMock: []string{"root"},
+					})
+				}
+
+				levelTwo := []string{"baz", "qux"}
+				for i, s := range levelTwo {
+					g.Add(&testDrawable{
+						VertexName:      s,
+						DependentOnMock: levelOne[i : i+1],
+					})
+				}
+
+				g.ConnectDependents()
+				return &g
+			},
+			Expect: `
+digraph {
+	compound = "true"
+	newrank = "true"
+	subgraph "root" {
+		"[root] bar"
+		"[root] baz"
+		"[root] foo"
+		"[root] qux"
+		"[root] root"
+		"[root] bar" -> "[root] root"
+		"[root] baz" -> "[root] foo"
+		"[root] foo" -> "[root] root"
+		"[root] qux" -> "[root] bar"
+	}
+}
+			`,
+		},
+		"cycle": {
+			Opts: GraphDotOpts{
+				DrawCycles: true,
+			},
+			Graph: func() *Graph {
+				var g Graph
+				root := &testDrawableOrigin{"root"}
+				g.Add(root)
+
+				g.Add(&testDrawable{
+					VertexName:      "A",
+					DependentOnMock: []string{"root", "C"},
+				})
+
+				g.Add(&testDrawable{
+					VertexName:      "B",
+					DependentOnMock: []string{"A"},
+				})
+
+				g.Add(&testDrawable{
+					VertexName:      "C",
+					DependentOnMock: []string{"B"},
+				})
+
+				g.ConnectDependents()
+				return &g
+			},
+			Expect: `
+digraph {
+	compound = "true"
+	newrank = "true"
+	subgraph "root" {
+		"[root] A"
+		"[root] B"
+		"[root] C"
+		"[root] root"
+		"[root] A" -> "[root] B" [color = "red", penwidth = "2.0"]
+		"[root] A" -> "[root] C"
+		"[root] A" -> "[root] root"
+		"[root] B" -> "[root] A"
+		"[root] B" -> "[root] C" [color = "red", penwidth = "2.0"]
+		"[root] C" -> "[root] A" [color = "red", penwidth = "2.0"]
+		"[root] C" -> "[root] B"
+	}
+}
+			`,
+		},
+		"subgraphs, no depth restriction": {
+			Opts: GraphDotOpts{
+				MaxDepth: -1,
+			},
+			Graph: func() *Graph {
+				var g Graph
+				root := &testDrawableOrigin{"root"}
+				g.Add(root)
+
+				var sub Graph
+				sub.Add(&testDrawableOrigin{"sub_root"})
+
+				var subsub Graph
+				subsub.Add(&testDrawableOrigin{"subsub_root"})
+				sub.Add(&testDrawableSubgraph{
+					VertexName:      "subsub",
+					SubgraphMock:    &subsub,
+					DependentOnMock: []string{"sub_root"},
+				})
+				g.Add(&testDrawableSubgraph{
+					VertexName:      "sub",
+					SubgraphMock:    &sub,
+					DependentOnMock: []string{"root"},
+				})
+
+				g.ConnectDependents()
+				sub.ConnectDependents()
+				return &g
+			},
+			Expect: `
+digraph {
+	compound = "true"
+	newrank = "true"
+	subgraph "root" {
+		"[root] root"
+		"[root] sub"
+		"[root] sub" -> "[root] root"
+	}
+	subgraph "cluster_sub" {
+		label = "sub"
+		"[sub] sub_root"
+		"[sub] subsub"
+		"[sub] subsub" -> "[sub] sub_root"
+	}
+	subgraph "cluster_subsub" {
+		label = "subsub"
+		"[subsub] subsub_root"
+	}
+}
+			`,
+		},
+		"subgraphs, with depth restriction": {
+			Opts: GraphDotOpts{
+				MaxDepth: 1,
+			},
+			Graph: func() *Graph {
+				var g Graph
+				root := &testDrawableOrigin{"root"}
+				g.Add(root)
+
+				var sub Graph
+				sub.Add(&testDrawableOrigin{"sub_root"})
+
+				var subsub Graph
+				subsub.Add(&testDrawableOrigin{"subsub_root"})
+				sub.Add(&testDrawableSubgraph{
+					VertexName:      "subsub",
+					SubgraphMock:    &subsub,
+					DependentOnMock: []string{"sub_root"},
+				})
+				g.Add(&testDrawableSubgraph{
+					VertexName:      "sub",
+					SubgraphMock:    &sub,
+					DependentOnMock: []string{"root"},
+				})
+
+				g.ConnectDependents()
+				sub.ConnectDependents()
+				return &g
+			},
+			Expect: `
+digraph {
+	compound = "true"
+	newrank = "true"
+	subgraph "root" {
+		"[root] root"
+		"[root] sub"
+		"[root] sub" -> "[root] root"
+	}
+	subgraph "cluster_sub" {
+		label = "sub"
+		"[sub] sub_root"
+		"[sub] subsub"
+		"[sub] subsub" -> "[sub] sub_root"
+	}
+}
+			`,
+		},
+	}
+
+	for tn, tc := range cases {
+		actual, err := GraphDot(tc.Graph(), &tc.Opts)
+		if (err == nil) && tc.Error != "" {
+			t.Fatalf("%s: expected err: %s, got none", tn, tc.Error)
+		}
+		if (err != nil) && (tc.Error == "") {
+			t.Fatalf("%s: unexpected err: %s", tn, err)
+		}
+		if (err != nil) && (tc.Error != "") {
+			if !strings.Contains(err.Error(), tc.Error) {
+				t.Fatalf("%s: expected err: %s\nto contain: %s", tn, err, tc.Error)
+			}
+			continue
+		}
+
+		expected := strings.TrimSpace(tc.Expect) + "\n"
+		if actual != expected {
+			t.Fatalf("%s:\n\nexpected:\n%s\n\ngot:\n%s", tn, expected, actual)
+		}
+	}
+}
+
+type testGraphFunc func() *Graph
+
+type testDrawable struct {
+	VertexName      string
+	DependentOnMock []string
+}
+
+func (node *testDrawable) Name() string {
+	return node.VertexName
+}
+func (node *testDrawable) DotNode(n string, opts *GraphDotOpts) *dot.Node {
+	return dot.NewNode(n, map[string]string{})
+}
+func (node *testDrawable) DependableName() []string {
+	return []string{node.VertexName}
+}
+func (node *testDrawable) DependentOn() []string {
+	return node.DependentOnMock
+}
+
+type testDrawableOrigin struct {
+	VertexName string
+}
+
+func (node *testDrawableOrigin) Name() string {
+	return node.VertexName
+}
+func (node *testDrawableOrigin) DotNode(n string, opts *GraphDotOpts) *dot.Node {
+	return dot.NewNode(n, map[string]string{})
+}
+func (node *testDrawableOrigin) DotOrigin() bool {
+	return true
+}
+func (node *testDrawableOrigin) DependableName() []string {
+	return []string{node.VertexName}
+}
+
+type testDrawableSubgraph struct {
+	VertexName      string
+	SubgraphMock    *Graph
+	DependentOnMock []string
+}
+
+func (node *testDrawableSubgraph) Name() string {
+	return node.VertexName
+}
+func (node *testDrawableSubgraph) Subgraph() *Graph {
+	return node.SubgraphMock
+}
+func (node *testDrawableSubgraph) DotNode(n string, opts *GraphDotOpts) *dot.Node {
+	return dot.NewNode(n, map[string]string{})
+}
+func (node *testDrawableSubgraph) DependentOn() []string {
+	return node.DependentOnMock
+}
diff --git a/terraform/transform_provider.go b/terraform/transform_provider.go
index 351e8eb12abc..664c1b032786 100644
--- a/terraform/transform_provider.go
+++ b/terraform/transform_provider.go
@@ -6,6 +6,7 @@ import (
 	"github.com/hashicorp/go-multierror"
 	"github.com/hashicorp/terraform/config"
 	"github.com/hashicorp/terraform/dag"
+	"github.com/hashicorp/terraform/dot"
 )
 
 // GraphNodeProvider is an interface that nodes that can be a provider
@@ -176,6 +177,19 @@ func (n *graphNodeDisabledProvider) Name() string {
 	return fmt.Sprintf("%s (disabled)", dag.VertexName(n.GraphNodeProvider))
 }
 
+// GraphNodeDotter impl.
+func (n *graphNodeDisabledProvider) DotNode(name string, opts *GraphDotOpts) *dot.Node {
+	return dot.NewNode(name, map[string]string{
+		"label": n.Name(),
+		"shape": "diamond",
+	})
+}
+
+// GraphNodeDotterOrigin impl.
+func (n *graphNodeDisabledProvider) DotOrigin() bool {
+	return true
+}
+
 type graphNodeMissingProvider struct {
 	ProviderNameValue string
 }
@@ -198,14 +212,16 @@ func (n *graphNodeMissingProvider) ProviderConfig() *config.RawConfig {
 }
 
 // GraphNodeDotter impl.
-func (n *graphNodeMissingProvider) Dot(name string) string {
-	return fmt.Sprintf(
-		"\"%s\" [\n"+
-			"\tlabel=\"%s\"\n"+
-			"\tshape=diamond\n"+
-			"];",
-		name,
-		n.Name())
+func (n *graphNodeMissingProvider) DotNode(name string, opts *GraphDotOpts) *dot.Node {
+	return dot.NewNode(name, map[string]string{
+		"label": n.Name(),
+		"shape": "diamond",
+	})
+}
+
+// GraphNodeDotterOrigin impl.
+func (n *graphNodeMissingProvider) DotOrigin() bool {
+	return true
 }
 
 func providerVertexMap(g *Graph) map[string]dag.Vertex {
diff --git a/terraform/transform_root.go b/terraform/transform_root.go
index bf5640ac6fb5..4d04df9beb71 100644
--- a/terraform/transform_root.go
+++ b/terraform/transform_root.go
@@ -1,10 +1,6 @@
 package terraform
 
-import (
-	"fmt"
-
-	"github.com/hashicorp/terraform/dag"
-)
+import "github.com/hashicorp/terraform/dag"
 
 // RootTransformer is a GraphTransformer that adds a root to the graph.
 type RootTransformer struct{}
@@ -38,7 +34,3 @@ type graphNodeRoot struct{}
 func (n graphNodeRoot) Name() string {
 	return "root"
 }
-
-func (n graphNodeRoot) Dot(name string) string {
-	return fmt.Sprintf("\"%s\" [shape=circle];", name)
-}
diff --git a/website/source/docs/commands/graph.html.markdown b/website/source/docs/commands/graph.html.markdown
index fe7adb1b3658..c7c426142a7f 100644
--- a/website/source/docs/commands/graph.html.markdown
+++ b/website/source/docs/commands/graph.html.markdown
@@ -16,18 +16,26 @@ The output is in the DOT format, which can be used by
 
 ## Usage
 
-Usage: `terraform graph [options] PATH`
+Usage: `terraform graph [options] [DIR]`
 
-Outputs the visual graph of Terraform resources. If the path given is
-the path to a configuration, the dependency graph of the resources are
-shown. If the path is a plan file, then the dependency graph of the
-plan itself is shown.
+Outputs the visual dependency graph of Terraform resources according to
+configuration files in DIR (or the current directory if omitted).
+
+The graph is outputted in DOT format. The typical program that can
+read this format is GraphViz, but many web services are also available
+to read this format.
 
 Options:
 
+* `-draw-cycles`    - Highlight any cycles in the graph with colored edges.
+                      This helps when diagnosing cycle errors.
+
 * `-module-depth=n` - The maximum depth to expand modules. By default this is
                       zero, which will not expand modules at all.
 
+* `-verbose`        - Generate a verbose, "worst-case" graph, with all nodes
+                      for potential operations in place.
+
 ## Generating Images
 
 The output of `terraform graph` is in the DOT format, which can

From e4e6ac5d9126dabf5f92fc98feadb0447d8e8b4b Mon Sep 17 00:00:00 2001
From: Paul Hinze <phinze@phinze.com>
Date: Tue, 28 Apr 2015 09:40:19 -0500
Subject: [PATCH 08/24] providers/aws: add source_security_group to elb

---
 builtin/providers/aws/resource_aws_elb.go             | 9 +++++++++
 website/source/docs/providers/aws/r/elb.html.markdown | 3 +++
 2 files changed, 12 insertions(+)

diff --git a/builtin/providers/aws/resource_aws_elb.go b/builtin/providers/aws/resource_aws_elb.go
index 8f4c613cd29b..ef96743243f1 100644
--- a/builtin/providers/aws/resource_aws_elb.go
+++ b/builtin/providers/aws/resource_aws_elb.go
@@ -70,6 +70,12 @@ func resourceAwsElb() *schema.Resource {
 				},
 			},
 
+			"source_security_group": &schema.Schema{
+				Type:     schema.TypeString,
+				Optional: true,
+				Computed: true,
+			},
+
 			"subnets": &schema.Schema{
 				Type:     schema.TypeSet,
 				Elem:     &schema.Schema{Type: schema.TypeString},
@@ -282,6 +288,9 @@ func resourceAwsElbRead(d *schema.ResourceData, meta interface{}) error {
 	d.Set("instances", flattenInstances(lb.Instances))
 	d.Set("listener", flattenListeners(lb.ListenerDescriptions))
 	d.Set("security_groups", lb.SecurityGroups)
+	if lb.SourceSecurityGroup != nil {
+		d.Set("source_security_group", lb.SourceSecurityGroup.GroupName)
+	}
 	d.Set("subnets", lb.Subnets)
 	d.Set("idle_timeout", lbAttrs.ConnectionSettings.IdleTimeout)
 	d.Set("connection_draining", lbAttrs.ConnectionDraining.Enabled)
diff --git a/website/source/docs/providers/aws/r/elb.html.markdown b/website/source/docs/providers/aws/r/elb.html.markdown
index 491cad074cee..20b007f6eaab 100644
--- a/website/source/docs/providers/aws/r/elb.html.markdown
+++ b/website/source/docs/providers/aws/r/elb.html.markdown
@@ -93,3 +93,6 @@ The following attributes are exported:
 * `name` - The name of the ELB
 * `dns_name` - The DNS name of the ELB
 * `instances` - The list of instances in the ELB
+* `source_security_group` - The name of the security group that you can use as
+  part of your inbound rules for your load balancer's back-end application
+  instances.

From 2ca181d42d12141b52a674901e78ba1f3e70943b Mon Sep 17 00:00:00 2001
From: Mitchell Hashimoto <mitchell.hashimoto@gmail.com>
Date: Wed, 29 Apr 2015 11:18:58 -0700
Subject: [PATCH 09/24] terraform: add output orphan transformer

---
 terraform/graph_config_node.go                |  4 ++
 terraform/graph_config_node_test.go           |  7 +++
 .../transform-orphan-output-basic/main.tf     |  1 +
 terraform/transform_output.go                 | 59 +++++++++++++++++++
 terraform/transform_output_test.go            | 45 ++++++++++++++
 5 files changed, 116 insertions(+)
 create mode 100644 terraform/test-fixtures/transform-orphan-output-basic/main.tf
 create mode 100644 terraform/transform_output.go
 create mode 100644 terraform/transform_output_test.go

diff --git a/terraform/graph_config_node.go b/terraform/graph_config_node.go
index abe1e58c1089..723526c36f34 100644
--- a/terraform/graph_config_node.go
+++ b/terraform/graph_config_node.go
@@ -137,6 +137,10 @@ func (n *GraphNodeConfigOutput) ConfigType() GraphNodeConfigType {
 	return GraphNodeConfigTypeOutput
 }
 
+func (n *GraphNodeConfigOutput) OutputName() string {
+	return n.Output.Name
+}
+
 func (n *GraphNodeConfigOutput) DependableName() []string {
 	return []string{n.Name()}
 }
diff --git a/terraform/graph_config_node_test.go b/terraform/graph_config_node_test.go
index 33390fc24b59..13848663d5e4 100644
--- a/terraform/graph_config_node_test.go
+++ b/terraform/graph_config_node_test.go
@@ -41,6 +41,13 @@ func TestGraphNodeConfigModuleExpand(t *testing.T) {
 	}
 }
 
+func TestGraphNodeConfigOutput_impl(t *testing.T) {
+	var _ dag.Vertex = new(GraphNodeConfigOutput)
+	var _ dag.NamedVertex = new(GraphNodeConfigOutput)
+	var _ graphNodeConfig = new(GraphNodeConfigOutput)
+	var _ GraphNodeOutput = new(GraphNodeConfigOutput)
+}
+
 func TestGraphNodeConfigProvider_impl(t *testing.T) {
 	var _ dag.Vertex = new(GraphNodeConfigProvider)
 	var _ dag.NamedVertex = new(GraphNodeConfigProvider)
diff --git a/terraform/test-fixtures/transform-orphan-output-basic/main.tf b/terraform/test-fixtures/transform-orphan-output-basic/main.tf
new file mode 100644
index 000000000000..70619c4e3667
--- /dev/null
+++ b/terraform/test-fixtures/transform-orphan-output-basic/main.tf
@@ -0,0 +1 @@
+output "foo" { value = "bar" }
diff --git a/terraform/transform_output.go b/terraform/transform_output.go
new file mode 100644
index 000000000000..62e127d380fb
--- /dev/null
+++ b/terraform/transform_output.go
@@ -0,0 +1,59 @@
+package terraform
+
+import (
+	"fmt"
+)
+
+// GraphNodeOutput is an interface that nodes that are outputs must
+// implement. The OutputName returned is the name of the output key
+// that they manage.
+type GraphNodeOutput interface {
+	OutputName() string
+}
+
+// AddOutputOrphanTransformer is a transformer that adds output orphans
+// to the graph. Output orphans are outputs that are no longer in the
+// configuration and therefore need to be removed from the state.
+type AddOutputOrphanTransformer struct {
+	State *State
+}
+
+func (t *AddOutputOrphanTransformer) Transform(g *Graph) error {
+	// Get the state for this module. If we have no state, we have no orphans
+	state := t.State.ModuleByPath(g.Path)
+	if state == nil {
+		return nil
+	}
+
+	// Create the set of outputs we do have in the graph
+	found := make(map[string]struct{})
+	for _, v := range g.Vertices() {
+		on, ok := v.(GraphNodeOutput)
+		if !ok {
+			continue
+		}
+
+		found[on.OutputName()] = struct{}{}
+	}
+
+	// Go over all the outputs. If we don't have a graph node for it,
+	// create it. It doesn't need to depend on anything, since its just
+	// setting it empty.
+	for k, _ := range state.Outputs {
+		if _, ok := found[k]; ok {
+			continue
+		}
+
+		g.Add(&graphNodeOrphanOutput{OutputName: k})
+	}
+
+	return nil
+}
+
+type graphNodeOrphanOutput struct {
+	OutputName string
+}
+
+func (n *graphNodeOrphanOutput) Name() string {
+	return fmt.Sprintf("output.%s (orphan)", n.OutputName)
+}
diff --git a/terraform/transform_output_test.go b/terraform/transform_output_test.go
new file mode 100644
index 000000000000..dc9ea0a76497
--- /dev/null
+++ b/terraform/transform_output_test.go
@@ -0,0 +1,45 @@
+package terraform
+
+import (
+	"strings"
+	"testing"
+)
+
+func TestAddOutputOrphanTransformer(t *testing.T) {
+	mod := testModule(t, "transform-orphan-output-basic")
+	state := &State{
+		Modules: []*ModuleState{
+			&ModuleState{
+				Path: RootModulePath,
+				Outputs: map[string]string{
+					"foo": "bar",
+					"bar": "baz",
+				},
+			},
+		},
+	}
+
+	g := Graph{Path: RootModulePath}
+	{
+		tf := &ConfigTransformer{Module: mod}
+		if err := tf.Transform(&g); err != nil {
+			t.Fatalf("err: %s", err)
+		}
+	}
+
+	transform := &AddOutputOrphanTransformer{State: state}
+	if err := transform.Transform(&g); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	actual := strings.TrimSpace(g.String())
+	expected := strings.TrimSpace(testTransformOrphanOutputBasicStr)
+	if actual != expected {
+		t.Fatalf("bad:\n\n%s", actual)
+	}
+}
+
+const testTransformOrphanOutputBasicStr = `
+output.bar (orphan)
+output.foo
+`

From 873f5a91bb37fb838575d269799a093826125b19 Mon Sep 17 00:00:00 2001
From: Mitchell Hashimoto <mitchell.hashimoto@gmail.com>
Date: Wed, 29 Apr 2015 11:27:12 -0700
Subject: [PATCH 10/24] terraform: EvalDeleteOutput and context test

---
 terraform/context_test.go                     | 42 +++++++++++++++++++
 terraform/eval_output.go                      | 28 +++++++++++++
 terraform/graph_builder.go                    |  3 ++
 terraform/terraform_test.go                   |  7 ++++
 .../test-fixtures/apply-output-orphan/main.tf |  1 +
 terraform/transform_output.go                 |  9 ++++
 6 files changed, 90 insertions(+)
 create mode 100644 terraform/test-fixtures/apply-output-orphan/main.tf

diff --git a/terraform/context_test.go b/terraform/context_test.go
index d4597f11f93f..a85e77db6eae 100644
--- a/terraform/context_test.go
+++ b/terraform/context_test.go
@@ -4128,6 +4128,48 @@ func TestContext2Apply_nilDiff(t *testing.T) {
 	}
 }
 
+func TestContext2Apply_outputOrphan(t *testing.T) {
+	m := testModule(t, "apply-output-orphan")
+	p := testProvider("aws")
+	p.ApplyFn = testApplyFn
+	p.DiffFn = testDiffFn
+
+	state := &State{
+		Modules: []*ModuleState{
+			&ModuleState{
+				Path: rootModulePath,
+				Outputs: map[string]string{
+					"foo": "bar",
+					"bar": "baz",
+				},
+			},
+		},
+	}
+
+	ctx := testContext2(t, &ContextOpts{
+		Module: m,
+		Providers: map[string]ResourceProviderFactory{
+			"aws": testProviderFuncFixed(p),
+		},
+		State: state,
+	})
+
+	if _, err := ctx.Plan(); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	state, err := ctx.Apply()
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	actual := strings.TrimSpace(state.String())
+	expected := strings.TrimSpace(testTerraformApplyOutputOrphanStr)
+	if actual != expected {
+		t.Fatalf("bad: \n%s", actual)
+	}
+}
+
 func TestContext2Apply_Provisioner_compute(t *testing.T) {
 	m := testModule(t, "apply-provisioner-compute")
 	p := testProvider("aws")
diff --git a/terraform/eval_output.go b/terraform/eval_output.go
index 0d9056d70a65..acdc268c37df 100644
--- a/terraform/eval_output.go
+++ b/terraform/eval_output.go
@@ -6,6 +6,34 @@ import (
 	"github.com/hashicorp/terraform/config"
 )
 
+// EvalDeleteOutput is an EvalNode implementation that deletes an output
+// from the state.
+type EvalDeleteOutput struct {
+	Name string
+}
+
+// TODO: test
+func (n *EvalDeleteOutput) Eval(ctx EvalContext) (interface{}, error) {
+	state, lock := ctx.State()
+	if state == nil {
+		return nil, nil
+	}
+
+	// Get a write lock so we can access this instance
+	lock.Lock()
+	defer lock.Unlock()
+
+	// Look for the module state. If we don't have one, create it.
+	mod := state.ModuleByPath(ctx.Path())
+	if mod == nil {
+		return nil, nil
+	}
+
+	delete(mod.Outputs, n.Name)
+
+	return nil, nil
+}
+
 // EvalWriteOutput is an EvalNode implementation that writes the output
 // for the given name to the current state.
 type EvalWriteOutput struct {
diff --git a/terraform/graph_builder.go b/terraform/graph_builder.go
index f08e20f16d6b..425d2ef1310e 100644
--- a/terraform/graph_builder.go
+++ b/terraform/graph_builder.go
@@ -95,6 +95,9 @@ func (b *BuiltinGraphBuilder) Steps() []GraphTransformer {
 			Targeting: (len(b.Targets) > 0),
 		},
 
+		// Output-related transformations
+		&AddOutputOrphanTransformer{State: b.State},
+
 		// Provider-related transformations
 		&MissingProviderTransformer{Providers: b.Providers},
 		&ProviderTransformer{},
diff --git a/terraform/terraform_test.go b/terraform/terraform_test.go
index 38be7b9fd2fa..09c3307922b6 100644
--- a/terraform/terraform_test.go
+++ b/terraform/terraform_test.go
@@ -373,6 +373,13 @@ do_instance.foo:
   type = do_instance
 `
 
+const testTerraformApplyOutputOrphanStr = `
+<no state>
+Outputs:
+
+foo = bar
+`
+
 const testTerraformApplyProvisionerStr = `
 aws_instance.bar:
   ID = foo
diff --git a/terraform/test-fixtures/apply-output-orphan/main.tf b/terraform/test-fixtures/apply-output-orphan/main.tf
new file mode 100644
index 000000000000..70619c4e3667
--- /dev/null
+++ b/terraform/test-fixtures/apply-output-orphan/main.tf
@@ -0,0 +1 @@
+output "foo" { value = "bar" }
diff --git a/terraform/transform_output.go b/terraform/transform_output.go
index 62e127d380fb..7741f9d3df3a 100644
--- a/terraform/transform_output.go
+++ b/terraform/transform_output.go
@@ -57,3 +57,12 @@ type graphNodeOrphanOutput struct {
 func (n *graphNodeOrphanOutput) Name() string {
 	return fmt.Sprintf("output.%s (orphan)", n.OutputName)
 }
+
+func (n *graphNodeOrphanOutput) EvalTree() EvalNode {
+	return &EvalOpFilter{
+		Ops: []walkOperation{walkApply, walkRefresh},
+		Node: &EvalDeleteOutput{
+			Name: n.OutputName,
+		},
+	}
+}

From 1099e3f59fe299d046bd1e168bfb1979655d7253 Mon Sep 17 00:00:00 2001
From: Mitchell Hashimoto <mitchell.hashimoto@gmail.com>
Date: Wed, 29 Apr 2015 17:12:28 -0700
Subject: [PATCH 11/24] config: add module raw configs to InterpolatedConfigs
 [GH-1448]

---
 config/config.go                                     |  5 +++++
 .../validate-bad-output-to-module/child/main.tf      |  1 +
 .../validate-bad-output-to-module/main.tf            |  8 ++++++++
 config/module/tree_test.go                           | 12 ++++++++++++
 4 files changed, 26 insertions(+)
 create mode 100644 config/module/test-fixtures/validate-bad-output-to-module/child/main.tf
 create mode 100644 config/module/test-fixtures/validate-bad-output-to-module/main.tf

diff --git a/config/config.go b/config/config.go
index 2bdab8947f8d..c085e1f2616d 100644
--- a/config/config.go
+++ b/config/config.go
@@ -570,6 +570,11 @@ func (c *Config) InterpolatedVariables() map[string][]InterpolatedVariable {
 // a human-friendly source.
 func (c *Config) rawConfigs() map[string]*RawConfig {
 	result := make(map[string]*RawConfig)
+	for _, m := range c.Modules {
+		source := fmt.Sprintf("module '%s'", m.Name)
+		result[source] = m.RawConfig
+	}
+
 	for _, pc := range c.ProviderConfigs {
 		source := fmt.Sprintf("provider config '%s'", pc.Name)
 		result[source] = pc.RawConfig
diff --git a/config/module/test-fixtures/validate-bad-output-to-module/child/main.tf b/config/module/test-fixtures/validate-bad-output-to-module/child/main.tf
new file mode 100644
index 000000000000..4d68c80b3e72
--- /dev/null
+++ b/config/module/test-fixtures/validate-bad-output-to-module/child/main.tf
@@ -0,0 +1 @@
+variable "memory" { default = "foo" }
diff --git a/config/module/test-fixtures/validate-bad-output-to-module/main.tf b/config/module/test-fixtures/validate-bad-output-to-module/main.tf
new file mode 100644
index 000000000000..4b627bbe57a3
--- /dev/null
+++ b/config/module/test-fixtures/validate-bad-output-to-module/main.tf
@@ -0,0 +1,8 @@
+module "child" {
+    source = "./child"
+}
+
+module "child2" {
+    source = "./child"
+    memory = "${module.child.memory_max}"
+}
diff --git a/config/module/tree_test.go b/config/module/tree_test.go
index 9667c0157cbb..74757a498a4b 100644
--- a/config/module/tree_test.go
+++ b/config/module/tree_test.go
@@ -224,6 +224,18 @@ func TestTreeValidate_badChildOutput(t *testing.T) {
 	}
 }
 
+func TestTreeValidate_badChildOutputToModule(t *testing.T) {
+	tree := NewTree("", testConfig(t, "validate-bad-output-to-module"))
+
+	if err := tree.Load(testStorage(t), GetModeGet); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	if err := tree.Validate(); err == nil {
+		t.Fatal("should error")
+	}
+}
+
 func TestTreeValidate_badChildVar(t *testing.T) {
 	tree := NewTree("", testConfig(t, "validate-bad-var"))
 

From 914740f065f510897c53234d008725dd5c56abb9 Mon Sep 17 00:00:00 2001
From: Mitchell Hashimoto <mitchell.hashimoto@gmail.com>
Date: Wed, 29 Apr 2015 17:30:58 -0700
Subject: [PATCH 12/24] provider/openstack: enable_dhcp should be bool
 [GH-1741]

---
 ...resource_openstack_networking_subnet_v2.go | 25 ++++++-------------
 1 file changed, 7 insertions(+), 18 deletions(-)

diff --git a/builtin/providers/openstack/resource_openstack_networking_subnet_v2.go b/builtin/providers/openstack/resource_openstack_networking_subnet_v2.go
index 573e4d7eed50..d43fa6088226 100644
--- a/builtin/providers/openstack/resource_openstack_networking_subnet_v2.go
+++ b/builtin/providers/openstack/resource_openstack_networking_subnet_v2.go
@@ -3,7 +3,6 @@ package openstack
 import (
 	"fmt"
 	"log"
-	"strconv"
 
 	"github.com/hashicorp/terraform/helper/hashcode"
 	"github.com/hashicorp/terraform/helper/schema"
@@ -72,7 +71,7 @@ func resourceNetworkingSubnetV2() *schema.Resource {
 				ForceNew: true,
 			},
 			"enable_dhcp": &schema.Schema{
-				Type:     schema.TypeString,
+				Type:     schema.TypeBool,
 				Optional: true,
 				ForceNew: false,
 			},
@@ -125,13 +124,9 @@ func resourceNetworkingSubnetV2Create(d *schema.ResourceData, meta interface{})
 		HostRoutes:      resourceSubnetHostRoutesV2(d),
 	}
 
-	edRaw := d.Get("enable_dhcp").(string)
-	if edRaw != "" {
-		ed, err := strconv.ParseBool(edRaw)
-		if err != nil {
-			return fmt.Errorf("enable_dhcp, if provided, must be either 'true' or 'false'")
-		}
-		createOpts.EnableDHCP = &ed
+	if raw, ok := d.GetOk("enable_dhcp"); ok {
+		value := raw.(bool)
+		createOpts.EnableDHCP = &value
 	}
 
 	log.Printf("[DEBUG] Create Options: %#v", createOpts)
@@ -167,7 +162,7 @@ func resourceNetworkingSubnetV2Read(d *schema.ResourceData, meta interface{}) er
 	d.Set("tenant_id", s.TenantID)
 	d.Set("allocation_pools", s.AllocationPools)
 	d.Set("gateway_ip", s.GatewayIP)
-	d.Set("enable_dhcp", strconv.FormatBool(s.EnableDHCP))
+	d.Set("enable_dhcp", s.EnableDHCP)
 	d.Set("dns_nameservers", s.DNSNameservers)
 	d.Set("host_routes", s.HostRoutes)
 
@@ -200,14 +195,8 @@ func resourceNetworkingSubnetV2Update(d *schema.ResourceData, meta interface{})
 	}
 
 	if d.HasChange("enable_dhcp") {
-		edRaw := d.Get("enable_dhcp").(string)
-		if edRaw != "" {
-			ed, err := strconv.ParseBool(edRaw)
-			if err != nil {
-				return fmt.Errorf("enable_dhcp, if provided, must be either 'true' or 'false'")
-			}
-			updateOpts.EnableDHCP = &ed
-		}
+		v := d.Get("enable_dhcp").(bool)
+		updateOpts.EnableDHCP = &v
 	}
 
 	log.Printf("[DEBUG] Updating Subnet %s with options: %+v", d.Id(), updateOpts)

From 043387fc8f8325a2a0de45c0933ff0af16ebb55b Mon Sep 17 00:00:00 2001
From: Paul Hinze <phinze@phinze.com>
Date: Thu, 30 Apr 2015 14:50:21 -0500
Subject: [PATCH 13/24] docs: Fix styling in provider code block

Font was different from rest of the page and very weird lookin'
---
 website/source/docs/plugins/provider.html.md | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/website/source/docs/plugins/provider.html.md b/website/source/docs/plugins/provider.html.md
index 232dd8c8c1a8..5c3c720be3e2 100644
--- a/website/source/docs/plugins/provider.html.md
+++ b/website/source/docs/plugins/provider.html.md
@@ -202,7 +202,7 @@ subsequent `terraform apply` fixes this resource.
 Most of the time, partial state is not required. When it is, it must be
 specifically enabled. An example is shown below:
 
-<pre class="prettyprint">
+```
 func resourceUpdate(d *schema.ResourceData, meta interface{}) error {
 	// Enable partial state mode
 	d.Partial(true)
@@ -230,7 +230,7 @@ func resourceUpdate(d *schema.ResourceData, meta interface{}) error {
 
 	return nil
 }
-</pre>
+```
 
 In the example above, it is possible that setting the `tags` succeeds,
 but setting the `name` fails. In this scenario, we want to make sure

From 152298bcf0f376674dfdaa3ae76c1b39ffae57b0 Mon Sep 17 00:00:00 2001
From: Mitchell Hashimoto <mitchell.hashimoto@gmail.com>
Date: Thu, 30 Apr 2015 13:26:42 -0700
Subject: [PATCH 14/24] update CHANGELOG

---
 CHANGELOG.md | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index b532350ed174..f35a0b716656 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -8,6 +8,8 @@ FEATURES:
   * **Environmental variables to set variables**: Environment variables can be
      used to set variables. The environment variables must be in the format
      `TF_VAR_name` and this will be checked last for a value.
+  * **New remote state backend: `s3`**: You can now store remote state in
+     an S3 bucket. [GH-1723]
 
 IMPROVEMENTS:
 
@@ -19,11 +21,11 @@ IMPROVEMENTS:
   * **New resource: `aws_lb_cookie_stickiness_policy`**
   * **New resource: `google_dns_managed_zone`**
   * **New resource: `google_dns_record_set`**
-  * **Migrate to upstream AWS SDK:** Migrate the AWS provider to 
-      [awslabs/aws-sdk-go](https://github.com/awslabs/aws-sdk-go), 
-      the offical `awslabs` library. Previously we had forked the library for 
+  * **Migrate to upstream AWS SDK:** Migrate the AWS provider to
+      [awslabs/aws-sdk-go](https://github.com/awslabs/aws-sdk-go),
+      the offical `awslabs` library. Previously we had forked the library for
       stability while `awslabs` refactored. Now that work has completed, and we've
-      migrated back to the upstream version. 
+      migrated back to the upstream version.
   * core: Improve error message on diff mismatch [GH-1501]
   * provisioner/file: expand `~` in source path [GH-1569]
   * provider/aws: Improved credential detection [GH-1470]

From 1061bbfbfe81da8495694b92d8ef2d5045d02fc5 Mon Sep 17 00:00:00 2001
From: Mitchell Hashimoto <mitchell.hashimoto@gmail.com>
Date: Thu, 30 Apr 2015 13:27:28 -0700
Subject: [PATCH 15/24] update CHANGELOG

---
 CHANGELOG.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index f35a0b716656..c1fb359ed28e 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -75,6 +75,7 @@ BUG FIXES:
       if the value was computed [GH-1507]
   * core: Fix issue where values in sets on resources couldn't contain
       hyphens. [GH-1641]
+  * core: Outputs removed from the config are removed from the state [GH-1714]
   * command: remote states with uppercase types work [GH-1356]
   * provider/aws: launch configuration ID set after create success [GH-1518]
   * provider/aws: Fixed an issue with creating ELBs without any tags [GH-1580]

From 9295e1eca6a54d57e3fd98a1f1a2dc08439fd305 Mon Sep 17 00:00:00 2001
From: Paul Hinze <phinze@phinze.com>
Date: Thu, 30 Apr 2015 15:49:37 -0500
Subject: [PATCH 16/24] core: fix targeting with non-word chars

Flips the regex strategy to capture everything that's _not_ the next
separator instead of whitelisting `\w`

fixes #1699
---
 terraform/resource_address.go      | 4 ++--
 terraform/resource_address_test.go | 9 +++++++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/terraform/resource_address.go b/terraform/resource_address.go
index b54a923d8847..01d7e0c02789 100644
--- a/terraform/resource_address.go
+++ b/terraform/resource_address.go
@@ -77,9 +77,9 @@ func tokenizeResourceAddress(s string) (map[string]string, error) {
 	// string "aws_instance.web.tainted[1]"
 	re := regexp.MustCompile(`\A` +
 		// "aws_instance"
-		`(?P<type>\w+)\.` +
+		`(?P<type>[^.]+)\.` +
 		// "web"
-		`(?P<name>\w+)` +
+		`(?P<name>[^.[]+)` +
 		// "tainted" (optional, omission implies: "primary")
 		`(?:\.(?P<instance_type>\w+))?` +
 		// "1" (optional, omission implies: "0")
diff --git a/terraform/resource_address_test.go b/terraform/resource_address_test.go
index 2a8caa1f8f96..f6439b2e8b79 100644
--- a/terraform/resource_address_test.go
+++ b/terraform/resource_address_test.go
@@ -55,6 +55,15 @@ func TestParseResourceAddress(t *testing.T) {
 				Index:        -1,
 			},
 		},
+		"with a hyphen": {
+			Input: "aws_instance.foo-bar",
+			Expected: &ResourceAddress{
+				Type:         "aws_instance",
+				Name:         "foo-bar",
+				InstanceType: TypePrimary,
+				Index:        -1,
+			},
+		},
 	}
 
 	for tn, tc := range cases {

From 01450b420f959c4b4c25e4d8e33fc2924d9ed195 Mon Sep 17 00:00:00 2001
From: Paul Hinze <phinze@phinze.com>
Date: Thu, 30 Apr 2015 16:05:31 -0500
Subject: [PATCH 17/24] Update CHANGELOG.md

---
 CHANGELOG.md | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index c1fb359ed28e..b82b9a29deb9 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -117,6 +117,8 @@ BUG FIXES:
       systems so copying your ".terraform" folder works. [GH-1418]
   * core: don't validate providers too early when nested in a module [GH-1380]
   * core: fix race condition in `count.index` interpolation [GH-1454]
+  * core: properly initialize provisioners, fixing resource targeting
+      during destroy [GH-1544]
   * command/push: don't ask for input if terraform.tfvars is present
   * command/remote-config: remove spurrious error "nil" when initializing
       remote state on a new configuration. [GH-1392]

From 754eb8c77d6c145593f6d78d96616b8f1a769c64 Mon Sep 17 00:00:00 2001
From: Paul Hinze <phinze@phinze.com>
Date: Thu, 30 Apr 2015 16:09:50 -0500
Subject: [PATCH 18/24] Update CHANGELOG.md

---
 CHANGELOG.md | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index b82b9a29deb9..5f5058ab903e 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -76,6 +76,8 @@ BUG FIXES:
   * core: Fix issue where values in sets on resources couldn't contain
       hyphens. [GH-1641]
   * core: Outputs removed from the config are removed from the state [GH-1714]
+  * core: Validate against the worst-case graph during plan phase to catch cycles
+      that would previously only show up during apply [GH-1655]
   * command: remote states with uppercase types work [GH-1356]
   * provider/aws: launch configuration ID set after create success [GH-1518]
   * provider/aws: Fixed an issue with creating ELBs without any tags [GH-1580]

From f843370f13bfbb3df45a8dff9501bbb38bbea7f2 Mon Sep 17 00:00:00 2001
From: Paul Hinze <phinze@phinze.com>
Date: Thu, 30 Apr 2015 16:10:50 -0500
Subject: [PATCH 19/24] Update CHANGELOG.md

---
 CHANGELOG.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5f5058ab903e..d0f3dcbd1164 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -49,6 +49,7 @@ IMPROVEMENTS:
       static default value [GH-1632]
   * provider/aws: automatically set the private IP as the SSH address
       if not specified and no public IP is available [GH-1623]
+  * provider/aws: `aws_elb` exports `source_security_group` field [GH-1708]
   * provider/docker: `docker_container` can specify links [GH-1564]
   * provider/google: `resource_compute_disk` supports snapshots [GH-1426]
   * provider/google: `resource_compute_instance` supports specifying the

From bd7d73791e1988a19fb73e76740f6330dc03b212 Mon Sep 17 00:00:00 2001
From: Mitchell Hashimoto <mitchell.hashimoto@gmail.com>
Date: Thu, 30 Apr 2015 14:27:51 -0700
Subject: [PATCH 20/24] update CHANGELOG

---
 CHANGELOG.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index d0f3dcbd1164..7ed2ca38d965 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -90,6 +90,7 @@ BUG FIXES:
   * provider/aws: ASG health check grace period can be updated in-place [GH-1682]
   * provider/aws: ELB security groups can be updated in-place [GH-1662]
   * provider/openstack: region config is not required [GH-1441]
+  * provider/openstack: `enable_dhcp` for networking subnet should be bool [GH-1741]
   * provisioner/remote-exec: add random number to uploaded script path so
       that parallel provisions work [GH-1588]
 

From a8165fea115d5dbb0a0b03fc99c3b1cb450a815d Mon Sep 17 00:00:00 2001
From: Mitchell Hashimoto <mitchell.hashimoto@gmail.com>
Date: Thu, 30 Apr 2015 14:36:57 -0700
Subject: [PATCH 21/24] update CHANGELOG

---
 CHANGELOG.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 7ed2ca38d965..acf20dcf8e44 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -79,6 +79,7 @@ BUG FIXES:
   * core: Outputs removed from the config are removed from the state [GH-1714]
   * core: Validate against the worst-case graph during plan phase to catch cycles
       that would previously only show up during apply [GH-1655]
+  * core: Referencing invalid module output in module validates [GH-1448]
   * command: remote states with uppercase types work [GH-1356]
   * provider/aws: launch configuration ID set after create success [GH-1518]
   * provider/aws: Fixed an issue with creating ELBs without any tags [GH-1580]

From 338ae601bcb63e641d1c8a060d6bbd821a5cf241 Mon Sep 17 00:00:00 2001
From: Camilo Aguilar <camilo.aguilar@gmail.com>
Date: Tue, 28 Apr 2015 15:57:05 -0400
Subject: [PATCH 22/24] providers/aws: Implements DHCP Options Set support.

---
 builtin/providers/aws/provider.go             |   2 +
 builtin/providers/aws/resource_aws_vpc.go     |   6 +
 .../aws/resource_aws_vpc_dhcp_options.go      | 280 ++++++++++++++++++
 ...source_aws_vpc_dhcp_options_association.go |  99 +++++++
 ...e_aws_vpc_dhcp_options_association_test.go |  99 +++++++
 .../aws/resource_aws_vpc_dhcp_options_test.go | 115 +++++++
 .../aws/r/vpc_dhcp_options.html.markdown      |  66 +++++
 ...vpc_dhcp_options_association.html.markdown |  37 +++
 website/source/layouts/aws.erb                |  16 +-
 9 files changed, 716 insertions(+), 4 deletions(-)
 create mode 100644 builtin/providers/aws/resource_aws_vpc_dhcp_options.go
 create mode 100644 builtin/providers/aws/resource_aws_vpc_dhcp_options_association.go
 create mode 100644 builtin/providers/aws/resource_aws_vpc_dhcp_options_association_test.go
 create mode 100644 builtin/providers/aws/resource_aws_vpc_dhcp_options_test.go
 create mode 100644 website/source/docs/providers/aws/r/vpc_dhcp_options.html.markdown
 create mode 100644 website/source/docs/providers/aws/r/vpc_dhcp_options_association.html.markdown

diff --git a/builtin/providers/aws/provider.go b/builtin/providers/aws/provider.go
index 9efc95a04ff4..4e2c2b5708b9 100644
--- a/builtin/providers/aws/provider.go
+++ b/builtin/providers/aws/provider.go
@@ -105,6 +105,8 @@ func Provider() terraform.ResourceProvider {
 			"aws_subnet":                       resourceAwsSubnet(),
 			"aws_vpc":                          resourceAwsVpc(),
 			"aws_vpc_peering_connection":       resourceAwsVpcPeeringConnection(),
+			"aws_vpc_dhcp_options":             resourceAwsVpcDhcpOptions(),
+			"aws_vpc_dhcp_options_association": resourceAwsVpcDhcpOptionsAssociation(),
 			"aws_vpn_gateway":                  resourceAwsVpnGateway(),
 		},
 
diff --git a/builtin/providers/aws/resource_aws_vpc.go b/builtin/providers/aws/resource_aws_vpc.go
index 14e5521825e8..d27bde533abb 100644
--- a/builtin/providers/aws/resource_aws_vpc.go
+++ b/builtin/providers/aws/resource_aws_vpc.go
@@ -53,6 +53,11 @@ func resourceAwsVpc() *schema.Resource {
 				Computed: true,
 			},
 
+			"dhcp_options_id": &schema.Schema{
+				Type:     schema.TypeString,
+				Computed: true,
+			},
+
 			"default_security_group_id": &schema.Schema{
 				Type:     schema.TypeString,
 				Computed: true,
@@ -126,6 +131,7 @@ func resourceAwsVpcRead(d *schema.ResourceData, meta interface{}) error {
 	vpc := vpcRaw.(*ec2.VPC)
 	vpcid := d.Id()
 	d.Set("cidr_block", vpc.CIDRBlock)
+	d.Set("dhcp_options_id", vpc.DHCPOptionsID)
 
 	// Tags
 	d.Set("tags", tagsToMapSDK(vpc.Tags))
diff --git a/builtin/providers/aws/resource_aws_vpc_dhcp_options.go b/builtin/providers/aws/resource_aws_vpc_dhcp_options.go
new file mode 100644
index 000000000000..c496b1eb8163
--- /dev/null
+++ b/builtin/providers/aws/resource_aws_vpc_dhcp_options.go
@@ -0,0 +1,280 @@
+package aws
+
+import (
+	"fmt"
+	"log"
+	"strings"
+	"time"
+
+	"github.com/awslabs/aws-sdk-go/aws"
+	"github.com/awslabs/aws-sdk-go/service/ec2"
+	"github.com/hashicorp/terraform/helper/resource"
+	"github.com/hashicorp/terraform/helper/schema"
+)
+
+func resourceAwsVpcDhcpOptions() *schema.Resource {
+	return &schema.Resource{
+		Create: resourceAwsVpcDhcpOptionsCreate,
+		Read:   resourceAwsVpcDhcpOptionsRead,
+		Update: resourceAwsVpcDhcpOptionsUpdate,
+		Delete: resourceAwsVpcDhcpOptionsDelete,
+
+		Schema: map[string]*schema.Schema{
+			"domain_name": &schema.Schema{
+				Type:     schema.TypeString,
+				Optional: true,
+				ForceNew: true,
+			},
+
+			"domain_name_servers": &schema.Schema{
+				Type:     schema.TypeList,
+				Optional: true,
+				ForceNew: true,
+				Elem:     &schema.Schema{Type: schema.TypeString},
+			},
+
+			"ntp_servers": &schema.Schema{
+				Type:     schema.TypeList,
+				Optional: true,
+				ForceNew: true,
+				Elem:     &schema.Schema{Type: schema.TypeString},
+			},
+
+			"netbios_node_type": &schema.Schema{
+				Type:     schema.TypeString,
+				Optional: true,
+				ForceNew: true,
+			},
+
+			"netbios_name_servers": &schema.Schema{
+				Type:     schema.TypeList,
+				Optional: true,
+				ForceNew: true,
+				Elem:     &schema.Schema{Type: schema.TypeString},
+			},
+
+			"tags": &schema.Schema{
+				Type:     schema.TypeMap,
+				Optional: true,
+			},
+		},
+	}
+}
+
+func resourceAwsVpcDhcpOptionsCreate(d *schema.ResourceData, meta interface{}) error {
+	conn := meta.(*AWSClient).ec2conn
+
+	setDHCPOption := func(key string) *ec2.NewDHCPConfiguration {
+		log.Printf("[DEBUG] Setting DHCP option %s...", key)
+		tfKey := strings.Replace(key, "-", "_", -1)
+
+		value, ok := d.GetOk(tfKey)
+		if !ok {
+			return nil
+		}
+
+		if v, ok := value.(string); ok {
+			return &ec2.NewDHCPConfiguration{
+				Key: aws.String(key),
+				Values: []*string{
+					aws.String(v),
+				},
+			}
+		}
+
+		if v, ok := value.([]interface{}); ok {
+			var s []*string
+			for _, attr := range v {
+				s = append(s, aws.String(attr.(string)))
+			}
+
+			return &ec2.NewDHCPConfiguration{
+				Key:    aws.String(key),
+				Values: s,
+			}
+		}
+
+		return nil
+	}
+
+	createOpts := &ec2.CreateDHCPOptionsInput{
+		DHCPConfigurations: []*ec2.NewDHCPConfiguration{
+			setDHCPOption("domain-name"),
+			setDHCPOption("domain-name-servers"),
+			setDHCPOption("ntp-servers"),
+			setDHCPOption("netbios-node-type"),
+			setDHCPOption("netbios-name-servers"),
+		},
+	}
+
+	resp, err := conn.CreateDHCPOptions(createOpts)
+	if err != nil {
+		return fmt.Errorf("Error creating DHCP Options Set: %s", err)
+	}
+
+	dos := resp.DHCPOptions
+	d.SetId(*dos.DHCPOptionsID)
+	log.Printf("[INFO] DHCP Options Set ID: %s", d.Id())
+
+	// Wait for the DHCP Options to become available
+	log.Printf("[DEBUG] Waiting for DHCP Options (%s) to become available", d.Id())
+	stateConf := &resource.StateChangeConf{
+		Pending: []string{"pending"},
+		Target:  "",
+		Refresh: DHCPOptionsStateRefreshFunc(conn, d.Id()),
+		Timeout: 1 * time.Minute,
+	}
+	if _, err := stateConf.WaitForState(); err != nil {
+		return fmt.Errorf(
+			"Error waiting for DHCP Options (%s) to become available: %s",
+			d.Id(), err)
+	}
+
+	return resourceAwsVpcDhcpOptionsUpdate(d, meta)
+}
+
+func resourceAwsVpcDhcpOptionsRead(d *schema.ResourceData, meta interface{}) error {
+	conn := meta.(*AWSClient).ec2conn
+	req := &ec2.DescribeDHCPOptionsInput{
+		DHCPOptionsIDs: []*string{
+			aws.String(d.Id()),
+		},
+	}
+
+	resp, err := conn.DescribeDHCPOptions(req)
+	if err != nil {
+		return fmt.Errorf("Error retrieving DHCP Options: %s", err)
+	}
+
+	if len(resp.DHCPOptions) == 0 {
+		return nil
+	}
+
+	opts := resp.DHCPOptions[0]
+	d.Set("tags", tagsToMapSDK(opts.Tags))
+
+	for _, cfg := range opts.DHCPConfigurations {
+		tfKey := strings.Replace(*cfg.Key, "-", "_", -1)
+
+		if _, ok := d.Get(tfKey).(string); ok {
+			d.Set(tfKey, cfg.Values[0].Value)
+		} else {
+			values := make([]string, 0, len(cfg.Values))
+			for _, v := range cfg.Values {
+				values = append(values, *v.Value)
+			}
+
+			d.Set(tfKey, values)
+		}
+	}
+
+	return nil
+}
+
+func resourceAwsVpcDhcpOptionsUpdate(d *schema.ResourceData, meta interface{}) error {
+	conn := meta.(*AWSClient).ec2conn
+	return setTagsSDK(conn, d)
+}
+
+func resourceAwsVpcDhcpOptionsDelete(d *schema.ResourceData, meta interface{}) error {
+	conn := meta.(*AWSClient).ec2conn
+
+	return resource.Retry(3*time.Minute, func() error {
+		log.Printf("[INFO] Deleting DHCP Options ID %s...", d.Id())
+		_, err := conn.DeleteDHCPOptions(&ec2.DeleteDHCPOptionsInput{
+			DHCPOptionsID: aws.String(d.Id()),
+		})
+
+		if err == nil {
+			return nil
+		}
+
+		log.Printf("[WARN] %s", err)
+
+		ec2err, ok := err.(aws.APIError)
+		if !ok {
+			return err
+		}
+
+		switch ec2err.Code {
+		case "InvalidDhcpOptionsID.NotFound":
+			return nil
+		case "DependencyViolation":
+			// If it is a dependency violation, we want to disassociate
+			// all VPCs using the given DHCP Options ID, and retry deleting.
+			vpcs, err2 := findVPCsByDHCPOptionsID(conn, d.Id())
+			if err2 != nil {
+				log.Printf("[ERROR] %s", err2)
+				return err2
+			}
+
+			for _, vpc := range vpcs {
+				log.Printf("[INFO] Disassociating DHCP Options Set %s from VPC %s...", d.Id(), *vpc.VPCID)
+				if _, err := conn.AssociateDHCPOptions(&ec2.AssociateDHCPOptionsInput{
+					DHCPOptionsID: aws.String("default"),
+					VPCID:         vpc.VPCID,
+				}); err != nil {
+					return err
+				}
+			}
+			return err //retry
+		default:
+			// Any other error, we want to quit the retry loop immediately
+			return resource.RetryError{Err: err}
+		}
+
+		return nil
+	})
+}
+
+func findVPCsByDHCPOptionsID(conn *ec2.EC2, id string) ([]*ec2.VPC, error) {
+	req := &ec2.DescribeVPCsInput{
+		Filters: []*ec2.Filter{
+			&ec2.Filter{
+				Name: aws.String("dhcp-options-id"),
+				Values: []*string{
+					aws.String(id),
+				},
+			},
+		},
+	}
+
+	resp, err := conn.DescribeVPCs(req)
+	if err != nil {
+		if ec2err, ok := err.(aws.APIError); ok && ec2err.Code == "InvalidVpcID.NotFound" {
+			return nil, nil
+		}
+		return nil, err
+	}
+
+	return resp.VPCs, nil
+}
+
+func DHCPOptionsStateRefreshFunc(conn *ec2.EC2, id string) resource.StateRefreshFunc {
+	return func() (interface{}, string, error) {
+		DescribeDhcpOpts := &ec2.DescribeDHCPOptionsInput{
+			DHCPOptionsIDs: []*string{
+				aws.String(id),
+			},
+		}
+
+		resp, err := conn.DescribeDHCPOptions(DescribeDhcpOpts)
+		if err != nil {
+			if ec2err, ok := err.(aws.APIError); ok && ec2err.Code == "InvalidDhcpOptionsID.NotFound" {
+				resp = nil
+			} else {
+				log.Printf("Error on DHCPOptionsStateRefresh: %s", err)
+				return nil, "", err
+			}
+		}
+
+		if resp == nil {
+			// Sometimes AWS just has consistency issues and doesn't see
+			// our instance yet. Return an empty state.
+			return nil, "", nil
+		}
+
+		dos := resp.DHCPOptions[0]
+		return dos, "", nil
+	}
+}
diff --git a/builtin/providers/aws/resource_aws_vpc_dhcp_options_association.go b/builtin/providers/aws/resource_aws_vpc_dhcp_options_association.go
new file mode 100644
index 000000000000..46c447213f09
--- /dev/null
+++ b/builtin/providers/aws/resource_aws_vpc_dhcp_options_association.go
@@ -0,0 +1,99 @@
+package aws
+
+import (
+	"log"
+
+	"github.com/awslabs/aws-sdk-go/aws"
+	"github.com/awslabs/aws-sdk-go/service/ec2"
+	"github.com/hashicorp/terraform/helper/schema"
+)
+
+func resourceAwsVpcDhcpOptionsAssociation() *schema.Resource {
+	return &schema.Resource{
+		Create: resourceAwsVpcDhcpOptionsAssociationCreate,
+		Read:   resourceAwsVpcDhcpOptionsAssociationRead,
+		Update: resourceAwsVpcDhcpOptionsAssociationUpdate,
+		Delete: resourceAwsVpcDhcpOptionsAssociationDelete,
+
+		Schema: map[string]*schema.Schema{
+			"vpc_id": &schema.Schema{
+				Type:     schema.TypeString,
+				Required: true,
+			},
+
+			"dhcp_options_id": &schema.Schema{
+				Type:     schema.TypeString,
+				Required: true,
+			},
+		},
+	}
+}
+
+func resourceAwsVpcDhcpOptionsAssociationCreate(d *schema.ResourceData, meta interface{}) error {
+	conn := meta.(*AWSClient).ec2conn
+
+	log.Printf(
+		"[INFO] Creating DHCP Options association: %s => %s",
+		d.Get("vpc_id").(string),
+		d.Get("dhcp_options_id").(string))
+
+	optsID := aws.String(d.Get("dhcp_options_id").(string))
+	vpcID := aws.String(d.Get("vpc_id").(string))
+
+	if _, err := conn.AssociateDHCPOptions(&ec2.AssociateDHCPOptionsInput{
+		DHCPOptionsID: optsID,
+		VPCID:         vpcID,
+	}); err != nil {
+		return err
+	}
+
+	// Set the ID and return
+	d.SetId(*optsID + "-" + *vpcID)
+	log.Printf("[INFO] Association ID: %s", d.Id())
+
+	return nil
+}
+
+func resourceAwsVpcDhcpOptionsAssociationRead(d *schema.ResourceData, meta interface{}) error {
+	conn := meta.(*AWSClient).ec2conn
+	// Get the VPC that this association belongs to
+	vpcRaw, _, err := VPCStateRefreshFunc(conn, d.Get("vpc_id").(string))()
+
+	if err != nil {
+		return err
+	}
+
+	if vpcRaw == nil {
+		return nil
+	}
+
+	vpc := vpcRaw.(*ec2.VPC)
+	if *vpc.VPCID != d.Get("vpc_id") || *vpc.DHCPOptionsID != d.Get("dhcp_options_id") {
+		log.Printf("[INFO] It seems the DHCP Options association is gone. Deleting reference from Graph...")
+		d.SetId("")
+	}
+
+	return nil
+}
+
+// DHCP Options Asociations cannot be updated.
+func resourceAwsVpcDhcpOptionsAssociationUpdate(d *schema.ResourceData, meta interface{}) error {
+	return resourceAwsVpcDhcpOptionsAssociationCreate(d, meta)
+}
+
+// AWS does not provide an API to disassociate a DHCP Options set from a VPC.
+// So, we do this by setting the VPC to the default DHCP Options Set.
+func resourceAwsVpcDhcpOptionsAssociationDelete(d *schema.ResourceData, meta interface{}) error {
+	conn := meta.(*AWSClient).ec2conn
+
+	log.Printf("[INFO] Disassociating DHCP Options Set %s from VPC %s...", d.Get("dhcp_options_id"), d.Get("vpc_id"))
+	if _, err := conn.AssociateDHCPOptions(&ec2.AssociateDHCPOptionsInput{
+		DHCPOptionsID: aws.String("default"),
+		VPCID:         aws.String(d.Get("vpc_id").(string)),
+	}); err != nil {
+		return err
+	}
+
+	d.SetId("")
+	return nil
+}
diff --git a/builtin/providers/aws/resource_aws_vpc_dhcp_options_association_test.go b/builtin/providers/aws/resource_aws_vpc_dhcp_options_association_test.go
new file mode 100644
index 000000000000..1b00ce3dcf1d
--- /dev/null
+++ b/builtin/providers/aws/resource_aws_vpc_dhcp_options_association_test.go
@@ -0,0 +1,99 @@
+package aws
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/awslabs/aws-sdk-go/service/ec2"
+	"github.com/hashicorp/terraform/helper/resource"
+	"github.com/hashicorp/terraform/terraform"
+)
+
+func TestAccAWSDHCPOptionsAssociation(t *testing.T) {
+	var v ec2.VPC
+	var d ec2.DHCPOptions
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckDHCPOptionsAssociationDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: testAccDHCPOptionsAssociationConfig,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckDHCPOptionsExists("aws_vpc_dhcp_options.foo", &d),
+					testAccCheckVpcExists("aws_vpc.foo", &v),
+					testAccCheckDHCPOptionsAssociationExist("aws_vpc_dhcp_options_association.foo", &v),
+				),
+			},
+		},
+	})
+}
+
+func testAccCheckDHCPOptionsAssociationDestroy(s *terraform.State) error {
+	conn := testAccProvider.Meta().(*AWSClient).ec2conn
+
+	for _, rs := range s.RootModule().Resources {
+		if rs.Type != "aws_vpc_dhcp_options_association" {
+			continue
+		}
+
+		// Try to find the VPC associated to the DHCP Options set
+		vpcs, err := findVPCsByDHCPOptionsID(conn, rs.Primary.Attributes["dhcp_options_id"])
+		if err != nil {
+			return err
+		}
+
+		if len(vpcs) > 0 {
+			return fmt.Errorf("DHCP Options association is still associated to %d VPCs.", len(vpcs))
+		}
+	}
+
+	return nil
+}
+
+func testAccCheckDHCPOptionsAssociationExist(n string, vpc *ec2.VPC) resource.TestCheckFunc {
+	return func(s *terraform.State) error {
+		rs, ok := s.RootModule().Resources[n]
+		if !ok {
+			return fmt.Errorf("Not found: %s", n)
+		}
+
+		if rs.Primary.ID == "" {
+			return fmt.Errorf("No DHCP Options Set association ID is set")
+		}
+
+		if *vpc.DHCPOptionsID != rs.Primary.Attributes["dhcp_options_id"] {
+			return fmt.Errorf("VPC %s does not have DHCP Options Set %s associated", *vpc.VPCID, rs.Primary.Attributes["dhcp_options_id"])
+		}
+
+		if *vpc.VPCID != rs.Primary.Attributes["vpc_id"] {
+			return fmt.Errorf("DHCP Options Set %s is not associated with VPC %s", rs.Primary.Attributes["dhcp_options_id"], *vpc.VPCID)
+		}
+
+		return nil
+	}
+}
+
+const testAccDHCPOptionsAssociationConfig = `
+resource "aws_vpc" "foo" {
+	cidr_block = "10.1.0.0/16"
+}
+
+resource "aws_vpc_dhcp_options" "foo" {
+	domain_name = "service.consul"
+	domain_name_servers = ["127.0.0.1", "10.0.0.2"]
+	ntp_servers = ["127.0.0.1"]
+	netbios_name_servers = ["127.0.0.1"]
+	netbios_node_type = 2
+
+	tags {
+		Name = "foo"
+	}
+}
+
+resource "aws_vpc_dhcp_options_association" "foo" {
+	vpc_id = "${aws_vpc.foo.id}"
+	dhcp_options_id = "${aws_vpc_dhcp_options.foo.id}"
+}
+`
diff --git a/builtin/providers/aws/resource_aws_vpc_dhcp_options_test.go b/builtin/providers/aws/resource_aws_vpc_dhcp_options_test.go
new file mode 100644
index 000000000000..74ab9395344c
--- /dev/null
+++ b/builtin/providers/aws/resource_aws_vpc_dhcp_options_test.go
@@ -0,0 +1,115 @@
+package aws
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/awslabs/aws-sdk-go/aws"
+	"github.com/awslabs/aws-sdk-go/service/ec2"
+	"github.com/hashicorp/terraform/helper/resource"
+	"github.com/hashicorp/terraform/terraform"
+)
+
+func TestAccDHCPOptions(t *testing.T) {
+	var d ec2.DHCPOptions
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckDHCPOptionsDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: testAccDHCPOptionsConfig,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckDHCPOptionsExists("aws_vpc_dhcp_options.foo", &d),
+					resource.TestCheckResourceAttr("aws_vpc_dhcp_options.foo", "domain_name", "service.consul"),
+					resource.TestCheckResourceAttr("aws_vpc_dhcp_options.foo", "domain_name_servers.0", "127.0.0.1"),
+					resource.TestCheckResourceAttr("aws_vpc_dhcp_options.foo", "domain_name_servers.1", "10.0.0.2"),
+					resource.TestCheckResourceAttr("aws_vpc_dhcp_options.foo", "ntp_servers.0", "127.0.0.1"),
+					resource.TestCheckResourceAttr("aws_vpc_dhcp_options.foo", "netbios_name_servers.0", "127.0.0.1"),
+					resource.TestCheckResourceAttr("aws_vpc_dhcp_options.foo", "netbios_node_type", "2"),
+					resource.TestCheckResourceAttr("aws_vpc_dhcp_options.foo", "tags.Name", "foo-name"),
+				),
+			},
+		},
+	})
+}
+
+func testAccCheckDHCPOptionsDestroy(s *terraform.State) error {
+	conn := testAccProvider.Meta().(*AWSClient).ec2conn
+
+	for _, rs := range s.RootModule().Resources {
+		if rs.Type != "aws_vpc_dhcp_options" {
+			continue
+		}
+
+		// Try to find the resource
+		resp, err := conn.DescribeDHCPOptions(&ec2.DescribeDHCPOptionsInput{
+			DHCPOptionsIDs: []*string{
+				aws.String(rs.Primary.ID),
+			},
+		})
+		if err == nil {
+			if len(resp.DHCPOptions) > 0 {
+				return fmt.Errorf("still exist.")
+			}
+
+			return nil
+		}
+
+		// Verify the error is what we want
+		ec2err, ok := err.(aws.APIError)
+		if !ok {
+			return err
+		}
+		if ec2err.Code != "InvalidDhcpOptionsID.NotFound" {
+			return err
+		}
+	}
+
+	return nil
+}
+
+func testAccCheckDHCPOptionsExists(n string, d *ec2.DHCPOptions) resource.TestCheckFunc {
+	return func(s *terraform.State) error {
+		rs, ok := s.RootModule().Resources[n]
+		if !ok {
+			return fmt.Errorf("Not found: %s", n)
+		}
+
+		if rs.Primary.ID == "" {
+			return fmt.Errorf("No ID is set")
+		}
+
+		conn := testAccProvider.Meta().(*AWSClient).ec2conn
+		resp, err := conn.DescribeDHCPOptions(&ec2.DescribeDHCPOptionsInput{
+			DHCPOptionsIDs: []*string{
+				aws.String(rs.Primary.ID),
+			},
+		})
+		if err != nil {
+			return err
+		}
+		if len(resp.DHCPOptions) == 0 {
+			return fmt.Errorf("DHCP Options not found")
+		}
+
+		*d = *resp.DHCPOptions[0]
+
+		return nil
+	}
+}
+
+const testAccDHCPOptionsConfig = `
+resource "aws_vpc_dhcp_options" "foo" {
+	domain_name = "service.consul"
+	domain_name_servers = ["127.0.0.1", "10.0.0.2"]
+	ntp_servers = ["127.0.0.1"]
+	netbios_name_servers = ["127.0.0.1"]
+	netbios_node_type = 2
+
+	tags {
+		Name = "foo-name"
+	}
+}
+`
diff --git a/website/source/docs/providers/aws/r/vpc_dhcp_options.html.markdown b/website/source/docs/providers/aws/r/vpc_dhcp_options.html.markdown
new file mode 100644
index 000000000000..3e595adcc7e0
--- /dev/null
+++ b/website/source/docs/providers/aws/r/vpc_dhcp_options.html.markdown
@@ -0,0 +1,66 @@
+---
+layout: "aws"
+page_title: "AWS: aws_vpc_dhcp_options"
+sidebar_current: "docs-aws-resource-vpc-dhcp-options"
+description: |-
+  Provides a VPC DHCP Options resource.
+---
+
+# aws\_vpc\_dhcp\_options
+
+Provides a VPC DHCP Options resource.
+
+## Example Usage
+
+Basic usage:
+
+```
+resource "aws_vpc_dhcp_options" "dns_resolver" {
+	domain_name_servers = ["8.8.8.8", "8.8.4.4"]
+}
+```
+
+Full usage:
+
+```
+resource "aws_vpc_dhcp_options" "foo" {
+	domain_name = "service.consul"
+	domain_name_servers = ["127.0.0.1", "10.0.0.2"]
+	ntp_servers = ["127.0.0.1"]
+	netbios_name_servers = ["127.0.0.1"]
+	netbios_node_type = 2
+
+	tags {
+		Name = "foo-name"
+	}
+}
+```
+
+## Argument Reference
+
+The following arguments are supported:
+
+* `domain_name` - (Optional) the suffix domain name to use by default when resolving non Fully Qualified Domain Names. In other words, this is what ends up being the `search` value in the `/etc/resolv.conf` file.
+* `domain_name_servers` - (Optional) List of name servers to configure in `/etc/resolv.conf`.
+* `ntp_servers` - (Optional) List of NTP servers to configure.
+* `netbios_name_servers` - (Optional) List of NETBIOS name servers.
+* `netbios_node_type` - (Optional) The NetBIOS node type (1, 2, 4, or 8). AWS recommends to specify 2 since broadcast and multicast are not supported in their network. For more information about these node types, see [RFC 2132](http://www.ietf.org/rfc/rfc2132.txt).
+* `tags` - (Optional) A mapping of tags to assign to the resource.
+
+## Remarks
+* Notice that all arguments are optional but you have to specify at least one argument.
+* `domain_name_servers`, `netbios_name_servers`, `ntp_servers` are limited by AWS to maximum four servers only.
+* To actually use the DHCP Options Set you need to associate it to a VPC using [`aws_vpc_dhcp_options_association`](/docs/providers/aws/r/vpc_dhcp_options_association.html).
+* If you delete a DHCP Options Set, all VPCs using it will be associated to AWS's `default` DHCP Option Set.
+
+## Attributes Reference
+
+The following attributes are exported:
+
+* `id` - The ID of the DHCP Options Set.
+
+## Known Issues
+* https://github.com/awslabs/aws-sdk-go/issues/210
+
+You can find more technical documentation about DHCP Options Set in the
+official [AWS User Guide](http://docs.aws.amazon.com/AmazonVPC/latest/UserGuide/VPC_DHCP_Options.html).
diff --git a/website/source/docs/providers/aws/r/vpc_dhcp_options_association.html.markdown b/website/source/docs/providers/aws/r/vpc_dhcp_options_association.html.markdown
new file mode 100644
index 000000000000..ee0cfe3e2cdf
--- /dev/null
+++ b/website/source/docs/providers/aws/r/vpc_dhcp_options_association.html.markdown
@@ -0,0 +1,37 @@
+---
+layout: "aws"
+page_title: "AWS: aws_vpc_dhcp_options_association"
+sidebar_current: "docs-aws-resource-vpc-dhcp-options-association"
+description: |-
+  Provides a VPC DHCP Options Association resource.
+---
+
+# aws\_vpc\_dhcp\_options\_<wbr>association
+
+Provides a VPC DHCP Options Association resource.
+
+## Example Usage
+
+```
+resource "aws_vpc_dhcp_options_association" "dns_resolver" {
+	vpc_id = "${aws_vpc.foo.id}"
+	dhcp_options_id = "${aws_vpc_dhcp_options.foo.id}"
+}
+```
+
+## Argument Reference
+
+The following arguments are supported:
+
+* `vpc_id` - (Required) The ID of the VPC to which we would like to associate a DHCP Options Set.
+* `dhcp_options_id` - (Required) The ID of the DHCP Options Set to associate to the VPC.
+
+## Remarks
+* You can only associate one DHCP Options Set to a given VPC ID.
+* Removing the DHCP Options Association automatically sets AWS's `default` DHCP Options Set to the VPC.
+
+## Attributes Reference
+
+The following attributes are exported:
+
+* `id` - The ID of the DHCP Options Set Association.
diff --git a/website/source/layouts/aws.erb b/website/source/layouts/aws.erb
index eeaaaff8515d..b10ac513a34f 100644
--- a/website/source/layouts/aws.erb
+++ b/website/source/layouts/aws.erb
@@ -110,13 +110,21 @@
 
 						<li<%= sidebar_current("docs-aws-resource-vpc-peering") %>>
 							<a href="/docs/providers/aws/r/vpc_peering.html">aws_vpc_peering</a>
+						</li>
 
-							<li<%= sidebar_current("docs-aws-resource-vpn-gateway") %>>
-								<a href="/docs/providers/aws/r/vpn_gateway.html">aws_vpn_gateway</a>
+						<li<%= sidebar_current("docs-aws-resource-vpc-dhcp-options") %>>
+							<a href="/docs/providers/aws/r/vpc_dhcp_options.html">aws_vpc_dhcp_options</a>
+						</li>
 
-							</li>
-					</ul>
+						<li<%= sidebar_current("docs-aws-resource-vpc-dhcp-options-association") %>>
+							<a href="/docs/providers/aws/r/vpc_dhcp_options_association.html">aws_vpc_dhcp_options_association</a>
 						</li>
+
+						<li<%= sidebar_current("docs-aws-resource-vpn-gateway") %>>
+							<a href="/docs/providers/aws/r/vpn_gateway.html">aws_vpn_gateway</a>
+						</li>
+					</ul>
+				</li>
 			</ul>
 		</div>
 	<% end %>

From e3616d391ac01503094621b316921bdd4833e369 Mon Sep 17 00:00:00 2001
From: Paul Hinze <phinze@phinze.com>
Date: Thu, 30 Apr 2015 17:43:41 -0500
Subject: [PATCH 23/24] Update CHANGELOG.md

---
 CHANGELOG.md | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index acf20dcf8e44..6844628b91e3 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -19,6 +19,8 @@ IMPROVEMENTS:
   * **New resource: `aws_customer_gateway`**
   * **New resource: `aws_ebs_volume`**
   * **New resource: `aws_lb_cookie_stickiness_policy`**
+  * **New resource: `aws_vpc_dhcp_options`**
+  * **New resource: `aws_vpc_dhcp_options_association`**
   * **New resource: `google_dns_managed_zone`**
   * **New resource: `google_dns_record_set`**
   * **Migrate to upstream AWS SDK:** Migrate the AWS provider to

From dbf6d1bd00cc231e66e647fee49c8ed9d24744a1 Mon Sep 17 00:00:00 2001
From: Paul Hinze <phinze@phinze.com>
Date: Fri, 1 May 2015 11:11:16 -0500
Subject: [PATCH 24/24] helper/resource: fix accidentaly swallowing of acctest
 step errors

With #1757 I unwittingly reused an err variable, causing all test check
errors to be swallowed. -_-
---
 helper/resource/testing.go | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/helper/resource/testing.go b/helper/resource/testing.go
index d153f93dc6f7..e1156422eba3 100644
--- a/helper/resource/testing.go
+++ b/helper/resource/testing.go
@@ -235,8 +235,8 @@ func testStep(
 
 	// Check! Excitement!
 	if step.Check != nil {
-		if err = step.Check(state); err != nil {
-			err = fmt.Errorf("Check failed: %s", err)
+		if err := step.Check(state); err != nil {
+			return state, fmt.Errorf("Check failed: %s", err)
 		}
 	}