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.
This commit is contained in:
parent
07930aa7fb
commit
a81bc23611
|
@ -9986,6 +9986,56 @@ func TestContext2Apply_scaleInMultivarRef(t *testing.T) {
|
||||||
assertNoErrors(t, diags)
|
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
|
// 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
|
// 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
|
// value for it. Previously we were incorrectly removing it from the state
|
||||||
|
|
|
@ -11,6 +11,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/plans"
|
||||||
|
"github.com/hashicorp/terraform/plans/objchange"
|
||||||
"github.com/hashicorp/terraform/providers"
|
"github.com/hashicorp/terraform/providers"
|
||||||
"github.com/hashicorp/terraform/provisioners"
|
"github.com/hashicorp/terraform/provisioners"
|
||||||
"github.com/hashicorp/terraform/states"
|
"github.com/hashicorp/terraform/states"
|
||||||
|
@ -171,6 +172,23 @@ func (n *EvalApply) Eval(ctx EvalContext) (interface{}, error) {
|
||||||
newVal = cty.UnknownAsNull(newVal)
|
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
|
// 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
|
// 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,
|
// where it seems like Terraform is failing to take any action at all,
|
||||||
|
|
|
@ -0,0 +1,2 @@
|
||||||
|
resource "test" "foo" {
|
||||||
|
}
|
Loading…
Reference in New Issue