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.
This commit is contained in:
Martin Atkins 2019-12-18 16:55:46 -08:00
parent 413e423bba
commit 7f8e087ce3
3 changed files with 47 additions and 3 deletions

View File

@ -5010,7 +5010,7 @@ func TestContext2Apply_error_createBeforeDestroy(t *testing.T) {
State: state, State: state,
}) })
p.ApplyFn = func(info *InstanceInfo, is *InstanceState, id *InstanceDiff) (*InstanceState, error) { 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 p.DiffFn = testDiffFn
@ -5022,6 +5022,11 @@ func TestContext2Apply_error_createBeforeDestroy(t *testing.T) {
if diags == nil { if diags == nil {
t.Fatal("should have error") 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()) actual := strings.TrimSpace(state.String())
expected := strings.TrimSpace(testTerraformApplyErrorCreateBeforeDestroyStr) expected := strings.TrimSpace(testTerraformApplyErrorCreateBeforeDestroyStr)
@ -11118,6 +11123,10 @@ func TestContext2Apply_taintedDestroyFailure(t *testing.T) {
Name: "c", Name: "c",
}.Instance(addrs.NoKey)) }.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 { if c.Current.Status != states.ObjectTainted {
t.Fatal("test_instance.c should be tainted") t.Fatal("test_instance.c should be tainted")
} }

View File

@ -7,6 +7,7 @@ import (
"github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/addrs"
"github.com/hashicorp/terraform/configs" "github.com/hashicorp/terraform/configs"
"github.com/hashicorp/terraform/plans"
"github.com/hashicorp/terraform/providers" "github.com/hashicorp/terraform/providers"
"github.com/hashicorp/terraform/states" "github.com/hashicorp/terraform/states"
"github.com/hashicorp/terraform/tfdiags" "github.com/hashicorp/terraform/tfdiags"
@ -388,6 +389,12 @@ func (n *EvalDeposeState) Eval(ctx EvalContext) (interface{}, error) {
type EvalMaybeRestoreDeposedObject struct { type EvalMaybeRestoreDeposedObject struct {
Addr addrs.ResourceInstance 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 // Key is a pointer to the deposed object key that should be forgotten
// from the state, which must be non-nil. // from the state, which must be non-nil.
Key *states.DeposedKey Key *states.DeposedKey
@ -399,6 +406,33 @@ func (n *EvalMaybeRestoreDeposedObject) Eval(ctx EvalContext) (interface{}, erro
dk := *n.Key dk := *n.Key
state := ctx.State() 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) restored := state.MaybeRestoreResourceInstanceDeposed(absAddr, dk)
if restored { if restored {
log.Printf("[TRACE] EvalMaybeRestoreDeposedObject: %s deposed object %s was restored as the current object", absAddr, dk) log.Printf("[TRACE] EvalMaybeRestoreDeposedObject: %s deposed object %s was restored as the current object", absAddr, dk)

View File

@ -395,6 +395,7 @@ func (n *NodeApplyableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe
}, },
Then: &EvalMaybeRestoreDeposedObject{ Then: &EvalMaybeRestoreDeposedObject{
Addr: addr.Resource, Addr: addr.Resource,
PlannedChange: &diffApply,
Key: &deposedKey, Key: &deposedKey,
}, },
}, },