From a81bc2361108ca1997d4720b401223ff2b41b7ef Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Tue, 5 Feb 2019 11:03:28 -0800 Subject: [PATCH] core: Verify that objects don't change unexpectedly during apply Previously we would allow providers to change anything about the planned object value during apply, possibly returning an entirely-unrelated object of the same type. In practice this led to some subtle bugs where a single planned attribute value would change during apply and cause a downstream failure due to a dependent resource now seeing input other than what _it_ expected during plan. Now we'll produce an explicit error message for this case which places the blame with the correct party: the upstream resource that changed. Without this, unexpected changes would often lead to the downstream resource implementation being blamed in error message even though it was just reacting to the change from upstream. As with most errors during apply, we'll still save the updated value in the state but we'll halt the walk to ensure that the unexpected value cannot propagate further and cause the result to potentially diverge greatly from the changeset shown in the plan. Compared to Terraform 0.11, we expect to see this error in many of the same cases we saw the "diffs didn't match during apply" error in earlier versions, since it is likely that many errors of that sort were the result of unexpected upstream changes being incorrectly blamed on the downstream resource that then used the result. --- terraform/context_apply_test.go | 50 +++++++++++++++++++ terraform/eval_apply.go | 18 +++++++ .../apply-inconsistent-with-plan/main.tf | 2 + 3 files changed, 70 insertions(+) create mode 100644 terraform/test-fixtures/apply-inconsistent-with-plan/main.tf diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 78549a2ec..27c08369b 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -9986,6 +9986,56 @@ func TestContext2Apply_scaleInMultivarRef(t *testing.T) { assertNoErrors(t, diags) } +func TestContext2Apply_inconsistentWithPlan(t *testing.T) { + m := testModule(t, "apply-inconsistent-with-plan") + p := testProvider("test") + p.GetSchemaReturn = &ProviderSchema{ + ResourceTypes: map[string]*configschema.Block{ + "test": { + Attributes: map[string]*configschema.Attribute{ + "id": {Type: cty.String, Computed: true}, + }, + }, + }, + } + p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse { + return providers.PlanResourceChangeResponse{ + PlannedState: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("before"), + }), + } + } + p.ApplyResourceChangeFn = func(req providers.ApplyResourceChangeRequest) providers.ApplyResourceChangeResponse { + return providers.ApplyResourceChangeResponse{ + NewState: cty.ObjectVal(map[string]cty.Value{ + // This is intentionally incorrect: because id was fixed at "before" + // during plan, it must not change during apply. + "id": cty.StringVal("after"), + }), + } + } + ctx := testContext2(t, &ContextOpts{ + Config: m, + ProviderResolver: providers.ResolverFixed( + map[string]providers.Factory{ + "test": testProviderFuncFixed(p), + }, + ), + }) + + if _, diags := ctx.Plan(); diags.HasErrors() { + t.Fatalf("plan errors: %s", diags.Err()) + } + + _, diags := ctx.Apply() + if !diags.HasErrors() { + t.Fatalf("apply succeeded; want error") + } + if got, want := diags.Err().Error(), "Provider produced inconsistent result after apply"; !strings.Contains(got, want) { + t.Fatalf("wrong error\ngot: %s\nshould contain: %s", got, want) + } +} + // 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 diff --git a/terraform/eval_apply.go b/terraform/eval_apply.go index 7924bd743..82be36ec9 100644 --- a/terraform/eval_apply.go +++ b/terraform/eval_apply.go @@ -11,6 +11,7 @@ import ( "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/configs" "github.com/hashicorp/terraform/plans" + "github.com/hashicorp/terraform/plans/objchange" "github.com/hashicorp/terraform/providers" "github.com/hashicorp/terraform/provisioners" "github.com/hashicorp/terraform/states" @@ -171,6 +172,23 @@ func (n *EvalApply) Eval(ctx EvalContext) (interface{}, error) { newVal = cty.UnknownAsNull(newVal) } + // 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 errs := objchange.AssertObjectCompatible(schema, change.After, newVal); len(errs) > 0 { + for _, err := range errs { + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Provider produced inconsistent result after apply", + fmt.Sprintf( + "When applying changes to %s, provider %q produced an unexpected new value for %s.\n\nThis is a bug in the provider, which should be reported in the provider's own issue tracker.", + absAddr, n.ProviderAddr.ProviderConfig.Type, tfdiags.FormatError(err), + ), + )) + } + } + // If a provider returns a null or non-null object at the wrong time then // 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, diff --git a/terraform/test-fixtures/apply-inconsistent-with-plan/main.tf b/terraform/test-fixtures/apply-inconsistent-with-plan/main.tf new file mode 100644 index 000000000..9284072dc --- /dev/null +++ b/terraform/test-fixtures/apply-inconsistent-with-plan/main.tf @@ -0,0 +1,2 @@ +resource "test" "foo" { +}