From 7f8e087ce34a3329531d0c008808546999e21368 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Wed, 18 Dec 2019 16:55:46 -0800 Subject: [PATCH] core: Don't panic if EvalMaybeResourceDeposedObject has no DeposedKey This is a "should never happen" case, but we have reports of it actually happening. In order to try to collect a bit more data about what's going on here, we're changing what was previously a hard panic into a normal error message that can include the address of the instance we were working on and the action we were trying to do to it at the time. The hope is to narrow down what situations can trigger this in order to find a reliable reproduction case in order to debug further. This also means that for those who _do_ encounter this problem in the meantime Terraform will have a chance to shut down cleanly and therefore be more likely to be able to recover on a subsequent plan/apply cycle. Further investigation of this will follow once we see a report or two of this updated error message. --- terraform/context_apply_test.go | 11 +++++++- terraform/eval_state.go | 34 +++++++++++++++++++++++ terraform/node_resource_apply_instance.go | 5 ++-- 3 files changed, 47 insertions(+), 3 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 1de9e380b..88ca29cc4 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -5010,7 +5010,7 @@ func TestContext2Apply_error_createBeforeDestroy(t *testing.T) { State: state, }) p.ApplyFn = func(info *InstanceInfo, is *InstanceState, id *InstanceDiff) (*InstanceState, error) { - return nil, fmt.Errorf("error") + return nil, fmt.Errorf("placeholder error from ApplyFn") } p.DiffFn = testDiffFn @@ -5022,6 +5022,11 @@ func TestContext2Apply_error_createBeforeDestroy(t *testing.T) { if diags == nil { t.Fatal("should have error") } + if got, want := diags.Err().Error(), "placeholder error from ApplyFn"; got != want { + // We're looking for our artificial error from ApplyFn above, whose + // message is literally "placeholder error from ApplyFn". + t.Fatalf("wrong error\ngot: %s\nwant: %s", got, want) + } actual := strings.TrimSpace(state.String()) expected := strings.TrimSpace(testTerraformApplyErrorCreateBeforeDestroyStr) @@ -11118,6 +11123,10 @@ func TestContext2Apply_taintedDestroyFailure(t *testing.T) { Name: "c", }.Instance(addrs.NoKey)) + if c.Current == nil { + t.Fatal("test_instance.c has no current instance, but it should") + } + if c.Current.Status != states.ObjectTainted { t.Fatal("test_instance.c should be tainted") } diff --git a/terraform/eval_state.go b/terraform/eval_state.go index fddc62bf2..fe5abb24f 100644 --- a/terraform/eval_state.go +++ b/terraform/eval_state.go @@ -7,6 +7,7 @@ import ( "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/configs" + "github.com/hashicorp/terraform/plans" "github.com/hashicorp/terraform/providers" "github.com/hashicorp/terraform/states" "github.com/hashicorp/terraform/tfdiags" @@ -388,6 +389,12 @@ func (n *EvalDeposeState) Eval(ctx EvalContext) (interface{}, error) { type EvalMaybeRestoreDeposedObject struct { Addr addrs.ResourceInstance + // PlannedChange might be the action we're performing that includes + // the possiblity of restoring a deposed object. However, it might also + // be nil. It's here only for use in error messages and must not be + // used for business logic. + PlannedChange **plans.ResourceInstanceChange + // Key is a pointer to the deposed object key that should be forgotten // from the state, which must be non-nil. Key *states.DeposedKey @@ -399,6 +406,33 @@ func (n *EvalMaybeRestoreDeposedObject) Eval(ctx EvalContext) (interface{}, erro dk := *n.Key state := ctx.State() + if dk == states.NotDeposed { + // This should never happen, and so it always indicates a bug. + // We should evaluate this node only if we've previously deposed + // an object as part of the same operation. + var diags tfdiags.Diagnostics + if n.PlannedChange != nil && *n.PlannedChange != nil { + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Attempt to restore non-existent deposed object", + fmt.Sprintf( + "Terraform has encountered a bug where it would need to restore a deposed object for %s without knowing a deposed object key for that object. This occurred during a %s action. This is a bug in Terraform; please report it!", + absAddr, (*n.PlannedChange).Action, + ), + )) + } else { + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Attempt to restore non-existent deposed object", + fmt.Sprintf( + "Terraform has encountered a bug where it would need to restore a deposed object for %s without knowing a deposed object key for that object. This is a bug in Terraform; please report it!", + absAddr, + ), + )) + } + return nil, diags.Err() + } + restored := state.MaybeRestoreResourceInstanceDeposed(absAddr, dk) if restored { log.Printf("[TRACE] EvalMaybeRestoreDeposedObject: %s deposed object %s was restored as the current object", absAddr, dk) diff --git a/terraform/node_resource_apply_instance.go b/terraform/node_resource_apply_instance.go index 8fa9b1283..217a06b7d 100644 --- a/terraform/node_resource_apply_instance.go +++ b/terraform/node_resource_apply_instance.go @@ -394,8 +394,9 @@ func (n *NodeApplyableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe return createBeforeDestroyEnabled && err != nil, nil }, Then: &EvalMaybeRestoreDeposedObject{ - Addr: addr.Resource, - Key: &deposedKey, + Addr: addr.Resource, + PlannedChange: &diffApply, + Key: &deposedKey, }, },