diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 3fef47809..b98b55a7e 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -9941,3 +9941,91 @@ func TestContext2Apply_scaleInMultivarRef(t *testing.T) { _, diags = ctx.Apply() assertNoErrors(t, diags) } + +// Issue 19908 was about retaining an existing object in the state when an +// update to it fails and the provider does not return a partially-updated +// value for it. Previously we were incorrectly removing it from the state +// in that case, but instead it should be retained so the update can be +// retried. +func TestContext2Apply_issue19908(t *testing.T) { + m := testModule(t, "apply-issue19908") + p := testProvider("test") + p.GetSchemaReturn = &ProviderSchema{ + ResourceTypes: map[string]*configschema.Block{ + "test": { + Attributes: map[string]*configschema.Attribute{ + "baz": {Type: cty.String, Required: true}, + }, + }, + }, + } + p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse { + return providers.PlanResourceChangeResponse{ + PlannedState: req.ProposedNewState, + } + } + p.ApplyResourceChangeFn = func(req providers.ApplyResourceChangeRequest) providers.ApplyResourceChangeResponse { + var diags tfdiags.Diagnostics + diags = diags.Append(fmt.Errorf("update failed")) + return providers.ApplyResourceChangeResponse{ + Diagnostics: diags, + } + } + ctx := testContext2(t, &ContextOpts{ + Config: m, + State: states.BuildState(func(s *states.SyncState) { + s.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test", + Name: "foo", + }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance), + &states.ResourceInstanceObjectSrc{ + AttrsJSON: []byte(`{"baz":"old"}`), + Status: states.ObjectReady, + }, + addrs.ProviderConfig{ + Type: "test", + }.Absolute(addrs.RootModuleInstance), + ) + }), + ProviderResolver: providers.ResolverFixed( + map[string]providers.Factory{ + "test": testProviderFuncFixed(p), + }, + ), + }) + + if _, diags := ctx.Plan(); diags.HasErrors() { + t.Fatalf("plan errors: %s", diags.Err()) + } + + state, diags := ctx.Apply() + if !diags.HasErrors() { + t.Fatalf("apply succeeded; want error") + } + if got, want := diags.Err().Error(), "update failed"; !strings.Contains(got, want) { + t.Fatalf("wrong error\ngot: %s\nshould contain: %s", got, want) + } + + mod := state.RootModule() + rs := mod.Resources["test.foo"] + if rs == nil { + t.Fatalf("test.foo not in state after apply, but should be") + } + is := rs.Instances[addrs.NoKey] + if is == nil { + t.Fatalf("test.foo not in state after apply, but should be") + } + obj := is.Current + if obj == nil { + t.Fatalf("test.foo has no current object in state after apply, but should do") + } + + if got, want := obj.Status, states.ObjectReady; got != want { + t.Errorf("test.foo has wrong status %s after apply; want %s", got, want) + } + if got, want := obj.AttrsJSON, []byte(`"old"`); !bytes.Contains(got, want) { + t.Errorf("test.foo attributes JSON doesn't contain %s after apply\ngot: %s", want, got) + } +} diff --git a/terraform/eval_apply.go b/terraform/eval_apply.go index 73b8a47a2..7924bd743 100644 --- a/terraform/eval_apply.go +++ b/terraform/eval_apply.go @@ -88,11 +88,28 @@ func (n *EvalApply) Eval(ctx EvalContext) (interface{}, error) { // incomplete. newVal := resp.NewState - // newVal should never be cty.NilVal in a real case, but it can happen - // sometimes in sloppy mocks in tests where error diagnostics are returned - // and the mock implementation doesn't populate the value at all. if newVal == cty.NilVal { - newVal = cty.NullVal(schema.ImpliedType()) + // Providers are supposed to return a partial new value even when errors + // occur, but sometimes they don't and so in that case we'll patch that up + // by just using the prior state, so we'll at least keep track of the + // object for the user to retry. + newVal = change.Before + + // As a special case, we'll set the new value to null if it looks like + // we were trying to execute a delete, because the provider in this case + // probably left the newVal unset intending it to be interpreted as "null". + if change.After.IsNull() { + newVal = cty.NullVal(schema.ImpliedType()) + } + + // Ideally we'd produce an error or warning here if newVal is nil and + // there are no errors in diags, because that indicates a buggy + // provider not properly reporting its result, but unfortunately many + // of our historical test mocks behave in this way and so producing + // a diagnostic here fails hundreds of tests. Instead, we must just + // silently retain the old value for now. Returning a nil value with + // no errors is still always considered a bug in the provider though, + // and should be fixed for any "real" providers that do it. } var conformDiags tfdiags.Diagnostics @@ -101,7 +118,7 @@ func (n *EvalApply) Eval(ctx EvalContext) (interface{}, error) { tfdiags.Error, "Provider produced invalid object", fmt.Sprintf( - "Provider %q planned an invalid value after apply for %s. The result cannot not be saved in the Terraform state.\n\nThis is a bug in the provider, which should be reported in the provider's own issue tracker.", + "Provider %q produced an invalid value after apply for %s. The result cannot not be saved in the Terraform state.\n\nThis is a bug in the provider, which should be reported in the provider's own issue tracker.", n.ProviderAddr.ProviderConfig.Type, tfdiags.FormatErrorPrefixed(err, absAddr.String()), ), )) @@ -158,7 +175,7 @@ func (n *EvalApply) Eval(ctx EvalContext) (interface{}, error) { // we still want to save that but it often causes some confusing behaviors // where it seems like Terraform is failing to take any action at all, // so we'll generate some errors to draw attention to it. - if !applyDiags.HasErrors() { + if !diags.HasErrors() { if change.Action == plans.Delete && !newVal.IsNull() { diags = diags.Append(tfdiags.Sourceless( tfdiags.Error, diff --git a/terraform/test-fixtures/apply-issue19908/issue19908.tf b/terraform/test-fixtures/apply-issue19908/issue19908.tf new file mode 100644 index 000000000..0c802fb65 --- /dev/null +++ b/terraform/test-fixtures/apply-issue19908/issue19908.tf @@ -0,0 +1,3 @@ +resource "test" "foo" { + baz = "updated" +}