Merge pull request #23304 from hashicorp/jbardin/tainted-fail-destroy

Make sure a failed destroy is marked as tainted

terraform/context_apply_test.go

@@ -10963,3 +10963,139 @@ func TestContext2Apply_destroyDataCycle(t *testing.T) {
 		t.Fatalf("diags: %s", diags.Err())
 	}
 }
+
+func TestContext2Apply_taintedDestroyFailure(t *testing.T) {
+	m := testModule(t, "apply-destroy-tainted")
+	p := testProvider("test")
+	p.DiffFn = testDiffFn
+	p.ApplyFn = func(info *InstanceInfo, s *InstanceState, d *InstanceDiff) (*InstanceState, error) {
+		// All destroys fail.
+		// c will also fail to create, meaning the existing tainted instance
+		// becomes deposed, ans is then promoted back to current.
+		// only C has a foo attribute
+		attr := d.Attributes["foo"]
+		if d.Destroy || (attr != nil && attr.New == "c") {
+			return nil, errors.New("failure")
+		}
+
+		return testApplyFn(info, s, d)
+	}
+
+	state := states.NewState()
+	root := state.EnsureModule(addrs.RootModuleInstance)
+	root.SetResourceInstanceCurrent(
+		addrs.Resource{
+			Mode: addrs.ManagedResourceMode,
+			Type: "test_instance",
+			Name: "a",
+		}.Instance(addrs.NoKey),
+		&states.ResourceInstanceObjectSrc{
+			Status:    states.ObjectTainted,
+			AttrsJSON: []byte(`{"id":"a","foo":"a"}`),
+		},
+		addrs.ProviderConfig{
+			Type: "test",
+		}.Absolute(addrs.RootModuleInstance),
+	)
+	root.SetResourceInstanceCurrent(
+		addrs.Resource{
+			Mode: addrs.ManagedResourceMode,
+			Type: "test_instance",
+			Name: "b",
+		}.Instance(addrs.NoKey),
+		&states.ResourceInstanceObjectSrc{
+			Status:    states.ObjectTainted,
+			AttrsJSON: []byte(`{"id":"b","foo":"b"}`),
+		},
+		addrs.ProviderConfig{
+			Type: "test",
+		}.Absolute(addrs.RootModuleInstance),
+	)
+	root.SetResourceInstanceCurrent(
+		addrs.Resource{
+			Mode: addrs.ManagedResourceMode,
+			Type: "test_instance",
+			Name: "c",
+		}.Instance(addrs.NoKey),
+		&states.ResourceInstanceObjectSrc{
+			Status:    states.ObjectTainted,
+			AttrsJSON: []byte(`{"id":"c","foo":"old"}`),
+		},
+		addrs.ProviderConfig{
+			Type: "test",
+		}.Absolute(addrs.RootModuleInstance),
+	)
+
+	providerResolver := providers.ResolverFixed(
+		map[string]providers.Factory{
+			"test": testProviderFuncFixed(p),
+		},
+	)
+
+	ctx := testContext2(t, &ContextOpts{
+		Config:           m,
+		ProviderResolver: providerResolver,
+		State:            state,
+		Hooks:            []Hook{&testHook{}},
+	})
+
+	_, diags := ctx.Plan()
+	diags.HasErrors()
+	if diags.HasErrors() {
+		t.Fatalf("diags: %s", diags.Err())
+	}
+
+	state, diags = ctx.Apply()
+	if !diags.HasErrors() {
+		t.Fatal("expected error")
+	}
+
+	root = state.Module(addrs.RootModuleInstance)
+
+	// the instance that failed to destroy should remain tainted
+	a := root.ResourceInstance(addrs.Resource{
+		Mode: addrs.ManagedResourceMode,
+		Type: "test_instance",
+		Name: "a",
+	}.Instance(addrs.NoKey))
+
+	if a.Current.Status != states.ObjectTainted {
+		t.Fatal("test_instance.a should be tainted")
+	}
+
+	// b is create_before_destroy, and the destroy failed, so there should be 1
+	// deposed instance.
+	b := root.ResourceInstance(addrs.Resource{
+		Mode: addrs.ManagedResourceMode,
+		Type: "test_instance",
+		Name: "b",
+	}.Instance(addrs.NoKey))
+
+	if b.Current.Status != states.ObjectReady {
+		t.Fatal("test_instance.b should be Ready")
+	}
+
+	if len(b.Deposed) != 1 {
+		t.Fatal("test_instance.b failed to keep deposed instance")
+	}
+
+	// the desposed c instance should be promoted back to Current, and remain
+	// tainted
+	c := root.ResourceInstance(addrs.Resource{
+		Mode: addrs.ManagedResourceMode,
+		Type: "test_instance",
+		Name: "c",
+	}.Instance(addrs.NoKey))
+
+	if c.Current.Status != states.ObjectTainted {
+		t.Fatal("test_instance.c should be tainted")
+	}
+
+	if len(c.Deposed) != 0 {
+		t.Fatal("test_instance.c should have no deposed instances")
+	}
+
+	if string(c.Current.AttrsJSON) != `{"id":"c","foo":"old"}` {
+		t.Fatalf("unexpected attrs for c: %q\n", c.Current.AttrsJSON)
+	}
+}

terraform/eval_apply.go

@@ -253,6 +253,8 @@ func (n *EvalApply) Eval(ctx EvalContext) (interface{}, error) {
 		}
 	}
 
+	newStatus := states.ObjectReady
+
 	// Sometimes providers return a null value when an operation fails for some
 	// reason, but we'd rather keep the prior state so that the error can be
 	// corrected on a subsequent run. We must only do this for null new value
@@ -265,12 +267,18 @@ func (n *EvalApply) Eval(ctx EvalContext) (interface{}, error) {
 		// If change.Action is Create then change.Before will also be null,
 		// which is fine.
 		newVal = change.Before
+
+		// If we're recovering the previous state, we also want to restore the
+		// the tainted status of the object.
+		if state.Status == states.ObjectTainted {
+			newStatus = states.ObjectTainted
+		}
 	}
 
 	var newState *states.ResourceInstanceObject
 	if !newVal.IsNull() { // null value indicates that the object is deleted, so we won't set a new state in that case
 		newState = &states.ResourceInstanceObject{
-			Status:  states.ObjectReady,
+			Status:  newStatus,
 			Value:   newVal,
 			Private: resp.Private,
 		}
@@ -376,40 +384,39 @@ type EvalMaybeTainted struct {
 	Change **plans.ResourceInstanceChange
 	State  **states.ResourceInstanceObject
 	Error  *error
-
-	// If StateOutput is not nil, its referent will be assigned either the same
-	// pointer as State or a new object with its status set as Tainted,
-	// depending on whether an error is given and if this was a create action.
-	StateOutput **states.ResourceInstanceObject
 }
 
-// TODO: test
 func (n *EvalMaybeTainted) Eval(ctx EvalContext) (interface{}, error) {
+	if n.State == nil || n.Change == nil || n.Error == nil {
+		return nil, nil
+	}
+
 	state := *n.State
 	change := *n.Change
 	err := *n.Error
 
+	// nothing to do if everything went as planned
+	if err == nil {
+		return nil, nil
+	}
+
 	if state != nil && state.Status == states.ObjectTainted {
 		log.Printf("[TRACE] EvalMaybeTainted: %s was already tainted, so nothing to do", n.Addr.Absolute(ctx.Path()))
 		return nil, nil
 	}
 
-	if n.StateOutput != nil {
-		if err != nil && change.Action == plans.Create {
-			// If there are errors during a _create_ then the object is
-			// in an undefined state, and so we'll mark it as tainted so
-			// we can try again on the next run.
-			//
-			// We don't do this for other change actions because errors
-			// during updates will often not change the remote object at all.
-			// If there _were_ changes prior to the error, it's the provider's
-			// responsibility to record the effect of those changes in the
-			// object value it returned.
-			log.Printf("[TRACE] EvalMaybeTainted: %s encountered an error during creation, so it is now marked as tainted", n.Addr.Absolute(ctx.Path()))
-			*n.StateOutput = state.AsTainted()
-		} else {
-			*n.StateOutput = state
-		}
+	if change.Action == plans.Create {
+		// If there are errors during a _create_ then the object is
+		// in an undefined state, and so we'll mark it as tainted so
+		// we can try again on the next run.
+		//
+		// We don't do this for other change actions because errors
+		// during updates will often not change the remote object at all.
+		// If there _were_ changes prior to the error, it's the provider's
+		// responsibility to record the effect of those changes in the
+		// object value it returned.
+		log.Printf("[TRACE] EvalMaybeTainted: %s encountered an error during creation, so it is now marked as tainted", n.Addr.Absolute(ctx.Path()))
+		*n.State = state.AsTainted()
 	}
 
 	return nil, nil

terraform/node_resource_apply_instance.go

@@ -356,11 +356,10 @@ func (n *NodeApplyableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe
 				CreateNew:      &createNew,
 			},
 			&EvalMaybeTainted{
-				Addr:        addr.Resource,
-				State:       &state,
-				Change:      &diffApply,
-				Error:       &err,
-				StateOutput: &state,
+				Addr:   addr.Resource,
+				State:  &state,
+				Change: &diffApply,
+				Error:  &err,
 			},
 			&EvalWriteState{
 				Addr:           addr.Resource,
@@ -378,11 +377,10 @@ func (n *NodeApplyableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe
 				When:           configs.ProvisionerWhenCreate,
 			},
 			&EvalMaybeTainted{
-				Addr:        addr.Resource,
-				State:       &state,
-				Change:      &diffApply,
-				Error:       &err,
-				StateOutput: &state,
+				Addr:   addr.Resource,
+				State:  &state,
+				Change: &diffApply,
+				Error:  &err,
 			},
 			&EvalWriteState{
 				Addr:           addr.Resource,

terraform/testdata/apply-destroy-tainted/main.tf

@@ -0,0 +1,17 @@
+resource "test_instance" "a" {
+  foo = "a"
+}
+
+resource "test_instance" "b" {
+  foo = "b"
+  lifecycle {
+    create_before_destroy = true
+  }
+}
+
+resource "test_instance" "c" {
+  foo = "c"
+  lifecycle {
+    create_before_destroy = true
+  }
+}