diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 8d50a3e9e..4bc016e9f 100644 --- a/terraform/context_apply_test.go +++ b/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) + } +} diff --git a/terraform/eval_apply.go b/terraform/eval_apply.go index cbabaefb1..215b9b657 100644 --- a/terraform/eval_apply.go +++ b/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 diff --git a/terraform/node_resource_apply_instance.go b/terraform/node_resource_apply_instance.go index 753ef9677..8fa9b1283 100644 --- a/terraform/node_resource_apply_instance.go +++ b/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, diff --git a/terraform/testdata/apply-destroy-tainted/main.tf b/terraform/testdata/apply-destroy-tainted/main.tf new file mode 100644 index 000000000..48f4f1378 --- /dev/null +++ b/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 + } +}