diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index fc9dbb0a3..72b5ead79 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -7097,6 +7097,159 @@ func TestContext2Apply_error(t *testing.T) { } } +func TestContext2Apply_errorCreateInvalidNew(t *testing.T) { + m := testModule(t, "apply-error") + + p := testProvider("aws") + p.GetSchemaReturn = &ProviderSchema{ + ResourceTypes: map[string]*configschema.Block{ + "aws_instance": { + Attributes: map[string]*configschema.Attribute{ + "value": {Type: cty.String, Optional: true}, + "foo": {Type: cty.String, Optional: true}, + }, + }, + }, + } + p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse { + return providers.PlanResourceChangeResponse{ + PlannedState: req.ProposedNewState, + } + } + p.ApplyResourceChangeFn = func(req providers.ApplyResourceChangeRequest) providers.ApplyResourceChangeResponse { + // We're intentionally returning an inconsistent new state here + // because we want to test that Terraform ignores the inconsistency + // when accompanied by another error. + return providers.ApplyResourceChangeResponse{ + NewState: cty.ObjectVal(map[string]cty.Value{ + "value": cty.StringVal("wrong wrong wrong wrong"), + "foo": cty.StringVal("absolutely brimming over with wrongability"), + }), + Diagnostics: tfdiags.Diagnostics(nil).Append(fmt.Errorf("forced error")), + } + } + + ctx := testContext2(t, &ContextOpts{ + Config: m, + ProviderResolver: providers.ResolverFixed( + map[string]providers.Factory{ + "aws": testProviderFuncFixed(p), + }, + ), + }) + + if _, diags := ctx.Plan(); diags.HasErrors() { + t.Fatalf("plan errors: %s", diags.Err()) + } + + state, diags := ctx.Apply() + if diags == nil { + t.Fatal("should have error") + } + if got, want := len(diags), 1; got != want { + // There should be no additional diagnostics generated by Terraform's own eval logic, + // because the provider's own error supersedes them. + t.Errorf("wrong number of diagnostics %d; want %d\n%s", got, want, diags.Err()) + } + if got, want := diags.Err().Error(), "forced error"; !strings.Contains(got, want) { + t.Errorf("returned error does not contain %q, but it should\n%s", want, diags.Err()) + } + if got, want := len(state.RootModule().Resources), 2; got != want { + t.Errorf("%d resources in state before prune; should have %d\n%s", got, want, spew.Sdump(state)) + } + state.PruneResourceHusks() // aws_instance.bar with no instances gets left behind when we bail out, but that's okay + if got, want := len(state.RootModule().Resources), 1; got != want { + t.Errorf("%d resources in state after prune; should have only one (aws_instance.foo, tainted)\n%s", got, spew.Sdump(state)) + } +} + +func TestContext2Apply_errorUpdateNullNew(t *testing.T) { + m := testModule(t, "apply-error") + + p := testProvider("aws") + p.GetSchemaReturn = &ProviderSchema{ + ResourceTypes: map[string]*configschema.Block{ + "aws_instance": { + Attributes: map[string]*configschema.Attribute{ + "value": {Type: cty.String, Optional: true}, + "foo": {Type: cty.String, Optional: true}, + }, + }, + }, + } + p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse { + return providers.PlanResourceChangeResponse{ + PlannedState: req.ProposedNewState, + } + } + p.ApplyResourceChangeFn = func(req providers.ApplyResourceChangeRequest) providers.ApplyResourceChangeResponse { + // We're intentionally returning no NewState here because we want to + // test that Terraform retains the prior state, rather than treating + // the returned null as "no state" (object deleted). + return providers.ApplyResourceChangeResponse{ + Diagnostics: tfdiags.Diagnostics(nil).Append(fmt.Errorf("forced error")), + } + } + + ctx := testContext2(t, &ContextOpts{ + Config: m, + State: states.BuildState(func(ss *states.SyncState) { + ss.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "aws_instance", + Name: "foo", + }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"value":"old"}`), + }, + addrs.ProviderConfig{ + Type: "aws", + }.Absolute(addrs.RootModuleInstance), + ) + }), + ProviderResolver: providers.ResolverFixed( + map[string]providers.Factory{ + "aws": testProviderFuncFixed(p), + }, + ), + }) + + if _, diags := ctx.Plan(); diags.HasErrors() { + t.Fatalf("plan errors: %s", diags.Err()) + } + + state, diags := ctx.Apply() + if diags == nil { + t.Fatal("should have error") + } + if got, want := len(diags), 1; got != want { + // There should be no additional diagnostics generated by Terraform's own eval logic, + // because the provider's own error supersedes them. + t.Errorf("wrong number of diagnostics %d; want %d\n%s", got, want, diags.Err()) + } + if got, want := diags.Err().Error(), "forced error"; !strings.Contains(got, want) { + t.Errorf("returned error does not contain %q, but it should\n%s", want, diags.Err()) + } + state.PruneResourceHusks() + if got, want := len(state.RootModule().Resources), 1; got != want { + t.Fatalf("%d resources in state; should have only one (aws_instance.foo, unmodified)\n%s", got, spew.Sdump(state)) + } + + is := state.ResourceInstance(addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "aws_instance", + Name: "foo", + }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance)) + if is == nil { + t.Fatalf("aws_instance.foo is not in the state after apply") + } + if got, want := is.Current.AttrsJSON, []byte(`"old"`); !bytes.Contains(got, want) { + t.Fatalf("incorrect attributes for aws_instance.foo\ngot: %s\nwant: JSON containing %s\n\n%s", got, want, spew.Sdump(is)) + } +} + func TestContext2Apply_errorPartial(t *testing.T) { errored := false diff --git a/terraform/eval_apply.go b/terraform/eval_apply.go index de8fd346d..1da62d30a 100644 --- a/terraform/eval_apply.go +++ b/terraform/eval_apply.go @@ -173,11 +173,16 @@ func (n *EvalApply) Eval(ctx EvalContext) (interface{}, error) { newVal = cty.UnknownAsNull(newVal) } - if change.Action != plans.Delete { + if change.Action != plans.Delete && !diags.HasErrors() { // Only values that were marked as unknown in the planned value are allowed // to change during the apply operation. (We do this after the unknown-ness // check above so that we also catch anything that became unknown after // being known during plan.) + // + // If we are returning other errors anyway then we'll give this + // a pass since the other errors are usually the explanation for + // this one and so it's more helpful to let the user focus on the + // root cause rather than distract with this extra problem. if errs := objchange.AssertObjectCompatible(schema, change.After, newVal); len(errs) > 0 { if resp.LegacyTypeSystem { // The shimming of the old type system in the legacy SDK is not precise @@ -240,6 +245,20 @@ func (n *EvalApply) Eval(ctx EvalContext) (interface{}, error) { } } + // Sometimes providers return a null value when an operation fails for some + // reason, but for any action other than delete 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 though, or else we may discard partial + // updates the provider was able to complete. + if change.Action != plans.Delete && diags.HasErrors() && newVal.IsNull() { + // Otherwise, we'll continue but using the prior state as the new value, + // making this effectively a no-op. If the item really _has_ been + // deleted then our next refresh will detect that and fix it up. + // If change.Action is Create then change.Before will also be null, + // which is fine. + newVal = change.Before + } + 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{