From 82bf4f485ba7fbce2be786a7a23ac3d507cc9924 Mon Sep 17 00:00:00 2001
From: Mitchell Hashimoto <mitchell.hashimoto@gmail.com>
Date: Thu, 16 Oct 2014 23:19:07 -0700
Subject: [PATCH] terraform: taint resources who error on create with
 provisioners [GH-434]

---
 CHANGELOG.md                                  |  2 +
 terraform/context.go                          | 27 ++++++++-----
 terraform/context_test.go                     | 40 +++++++++++++++++++
 terraform/terraform_test.go                   |  6 +++
 .../apply-provisioner-fail-create/main.tf     |  3 ++
 5 files changed, 68 insertions(+), 10 deletions(-)
 create mode 100644 terraform/test-fixtures/apply-provisioner-fail-create/main.tf

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 2b21b26e06..bcf9e9f553 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7,6 +7,8 @@ BUG FIXES:
   * core: Fix a hang that can occur with enough resources. [GH-410]
   * core: Config validation will not error if the field is being
       computed so the value is still unknown.
+  * core: If a resource fails to create and has provisioners, it is
+      marked as tainted. [GH-434]
   * providers/aws: Refresh of launch configs and autoscale groups load
       the correct data and don't incorrectly recreate themselves. [GH-425]
   * providers/aws: Fix case where ELB would incorrectly plan to modify
diff --git a/terraform/context.go b/terraform/context.go
index ea4f4f39aa..3565fbe665 100644
--- a/terraform/context.go
+++ b/terraform/context.go
@@ -743,19 +743,26 @@ func (c *walkContext) applyWalkFn() depgraph.WalkFunc {
 		// Additionally, we need to be careful to not run this if there
 		// was an error during the provider apply.
 		tainted := false
-		if applyerr == nil && createNew && len(r.Provisioners) > 0 {
-			for _, h := range c.Context.hooks {
-				handleHook(h.PreProvisionResource(r.Info, is))
-			}
+		if createNew && len(r.Provisioners) > 0 {
+			if applyerr == nil {
+				// If the apply succeeded, we have to run the provisioners
+				for _, h := range c.Context.hooks {
+					handleHook(h.PreProvisionResource(r.Info, is))
+				}
 
-			if err := c.applyProvisioners(r, is); err != nil {
-				errs = append(errs, err)
+				if err := c.applyProvisioners(r, is); err != nil {
+					errs = append(errs, err)
+					tainted = true
+				}
+
+				for _, h := range c.Context.hooks {
+					handleHook(h.PostProvisionResource(r.Info, is))
+				}
+			} else {
+				// If we failed to create properly and we have provisioners,
+				// then we have to mark ourselves as tainted to try again.
 				tainted = true
 			}
-
-			for _, h := range c.Context.hooks {
-				handleHook(h.PostProvisionResource(r.Info, is))
-			}
 		}
 
 		// If we're tainted then we need to update some flags
diff --git a/terraform/context_test.go b/terraform/context_test.go
index 4e526455c0..1ef4a1a2c5 100644
--- a/terraform/context_test.go
+++ b/terraform/context_test.go
@@ -1304,6 +1304,46 @@ func TestContextApply_Provisioner_compute(t *testing.T) {
 	}
 }
 
+func TestContextApply_provisionerCreateFail(t *testing.T) {
+	m := testModule(t, "apply-provisioner-fail-create")
+	p := testProvider("aws")
+	pr := testProvisioner()
+	p.DiffFn = testDiffFn
+
+	p.ApplyFn = func(
+		info *InstanceInfo,
+		is *InstanceState,
+		id *InstanceDiff) (*InstanceState, error) {
+		is.ID = "foo"
+		return is, fmt.Errorf("error")
+	}
+
+	ctx := testContext(t, &ContextOpts{
+		Module: m,
+		Providers: map[string]ResourceProviderFactory{
+			"aws": testProviderFuncFixed(p),
+		},
+		Provisioners: map[string]ResourceProvisionerFactory{
+			"shell": testProvisionerFuncFixed(pr),
+		},
+	})
+
+	if _, err := ctx.Plan(nil); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	state, err := ctx.Apply()
+	if err == nil {
+		t.Fatal("should error")
+	}
+
+	actual := strings.TrimSpace(state.String())
+	expected := strings.TrimSpace(testTerraformApplyProvisionerFailCreateStr)
+	if actual != expected {
+		t.Fatalf("bad: \n%s", actual)
+	}
+}
+
 func TestContextApply_provisionerFail(t *testing.T) {
 	m := testModule(t, "apply-provisioner-fail")
 	p := testProvider("aws")
diff --git a/terraform/terraform_test.go b/terraform/terraform_test.go
index 51e1324cf5..f056a8c5fb 100644
--- a/terraform/terraform_test.go
+++ b/terraform/terraform_test.go
@@ -275,6 +275,12 @@ aws_instance.foo:
   type = aws_instance
 `
 
+const testTerraformApplyProvisionerFailCreateStr = `
+aws_instance.bar: (1 tainted)
+  ID = <not created>
+  Tainted ID 1 = foo
+`
+
 const testTerraformApplyProvisionerFailCreateBeforeDestroyStr = `
 aws_instance.bar: (1 tainted)
   ID = bar
diff --git a/terraform/test-fixtures/apply-provisioner-fail-create/main.tf b/terraform/test-fixtures/apply-provisioner-fail-create/main.tf
new file mode 100644
index 0000000000..c1dcd222c0
--- /dev/null
+++ b/terraform/test-fixtures/apply-provisioner-fail-create/main.tf
@@ -0,0 +1,3 @@
+resource "aws_instance" "bar" {
+    provisioner "shell" {}
+}