From 312d798a89daa929a2c10a9ee84bb98dd4fe8e7f Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 7 Feb 2019 18:33:05 -0800 Subject: [PATCH] core: Restore our EvalReadData behavior In an earlier commit we changed objchange.ProposedNewObject so that the task of populating unknown values for attributes not known during apply is the responsibility of the provider's PlanResourceChange method, rather than being handled automatically. However, we were also using objchange.ProposedNewObject to construct the placeholder new object for a deferred data resource read, and so we inadvertently broke that deferral behavior. Here we restore the old behavior by introducing a new function objchange.PlannedDataResourceObject which is a specialized version of objchange.ProposedNewObject that includes the forced behavior of populating unknown values, because the provider gets no opportunity to customize a deferred read. TestContext2Plan_createBeforeDestroy_depends_datasource required some updates here because its implementation of PlanResourceChange was not handling the insertion of the unknown value for attribute "computed". The other changes here are just in an attempt to make the flow of this test more obvious, by clarifying that it is simulating a -refresh=false run, which effectively forces a deferred read since we skip the eager read that would normally happen in the refresh step. --- plans/objchange/objchange.go | 26 ++++++++++++++++++++ terraform/context_plan_test.go | 43 +++++++++++++++++++++++++++++++++- terraform/eval_read_data.go | 8 +++++-- 3 files changed, 74 insertions(+), 3 deletions(-) diff --git a/plans/objchange/objchange.go b/plans/objchange/objchange.go index 1f59e6f41..15565a400 100644 --- a/plans/objchange/objchange.go +++ b/plans/objchange/objchange.go @@ -32,6 +32,32 @@ func ProposedNewObject(schema *configschema.Block, prior, config cty.Value) cty. // below by giving us one non-null level of object to pull values from. prior = AllAttributesNull(schema) } + return proposedNewObject(schema, prior, config) +} + +// PlannedDataResourceObject is similar to ProposedNewObject but tailored for +// planning data resources in particular. Specifically, it replaces the values +// of any Computed attributes not set in the configuration with an unknown +// value, which serves as a placeholder for a value to be filled in by the +// provider when the data resource is finally read. +// +// Data resources are different because the planning of them is handled +// entirely within Terraform Core and not subject to customization by the +// provider. This function is, in effect, producing an equivalent result to +// passing the ProposedNewObject result into a provider's PlanResourceChange +// function, assuming a fixed implementation of PlanResourceChange that just +// fills in unknown values as needed. +func PlannedDataResourceObject(schema *configschema.Block, config cty.Value) cty.Value { + // Our trick here is to run the ProposedNewObject logic with an + // entirely-unknown prior value. Because of cty's unknown short-circuit + // behavior, any operation on prior returns another unknown, and so + // unknown values propagate into all of the parts of the resulting value + // that would normally be filled in by preserving the prior state. + prior := cty.UnknownVal(schema.ImpliedType()) + return proposedNewObject(schema, prior, config) +} + +func proposedNewObject(schema *configschema.Block, prior, config cty.Value) cty.Value { if config.IsNull() || !config.IsKnown() { // This is a weird situation, but we'll allow it anyway to free // callers from needing to specifically check for these cases. diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index f95f59068..a58035233 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -4990,8 +4990,20 @@ func TestContext2Plan_createBeforeDestroy_depends_datasource(t *testing.T) { }, } p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse { + computedVal := req.ProposedNewState.GetAttr("computed") + if computedVal.IsNull() { + computedVal = cty.UnknownVal(cty.String) + } return providers.PlanResourceChangeResponse{ - PlannedState: req.ProposedNewState, + PlannedState: cty.ObjectVal(map[string]cty.Value{ + "num": req.ProposedNewState.GetAttr("num"), + "computed": computedVal, + }), + } + } + p.ReadDataSourceFn = func(req providers.ReadDataSourceRequest) providers.ReadDataSourceResponse { + return providers.ReadDataSourceResponse{ + Diagnostics: tfdiags.Diagnostics(nil).Append(fmt.Errorf("ReadDataSource called, but should not have been")), } } @@ -5004,11 +5016,20 @@ func TestContext2Plan_createBeforeDestroy_depends_datasource(t *testing.T) { ), }) + // We're skipping ctx.Refresh here, which simulates what happens when + // running "terraform plan -refresh=false". As a result, we don't get our + // usual opportunity to read the data source during the refresh step and + // thus the plan call below is forced to produce a deferred read action. + plan, diags := ctx.Plan() + if p.ReadDataSourceCalled { + t.Errorf("ReadDataSource was called on the provider, but should not have been because we didn't refresh") + } if diags.HasErrors() { t.Fatalf("unexpected errors: %s", diags.Err()) } + seenAddrs := make(map[string]struct{}) for _, res := range plan.Changes.Resources { var schema *configschema.Block switch res.Addr.Resource.Resource.Mode { @@ -5023,6 +5044,8 @@ func TestContext2Plan_createBeforeDestroy_depends_datasource(t *testing.T) { t.Fatal(err) } + seenAddrs[ric.Addr.String()] = struct{}{} + t.Run(ric.Addr.String(), func(t *testing.T) { switch i := ric.Addr.String(); i { case "aws_instance.foo[0]": @@ -5046,6 +5069,10 @@ func TestContext2Plan_createBeforeDestroy_depends_datasource(t *testing.T) { t.Fatalf("resource %s should be read, got %s", ric.Addr, ric.Action) } checkVals(t, objectVal(t, schema, map[string]cty.Value{ + // In a normal flow we would've read an exact value in + // ReadDataSource, but because this test doesn't run + // cty.Refresh we have no opportunity to do that lookup + // and a deferred read is forced. "id": cty.UnknownVal(cty.String), "foo": cty.StringVal("0"), }), ric.After) @@ -5054,6 +5081,10 @@ func TestContext2Plan_createBeforeDestroy_depends_datasource(t *testing.T) { t.Fatalf("resource %s should be read, got %s", ric.Addr, ric.Action) } checkVals(t, objectVal(t, schema, map[string]cty.Value{ + // In a normal flow we would've read an exact value in + // ReadDataSource, but because this test doesn't run + // cty.Refresh we have no opportunity to do that lookup + // and a deferred read is forced. "id": cty.UnknownVal(cty.String), "foo": cty.StringVal("1"), }), ric.After) @@ -5062,6 +5093,16 @@ func TestContext2Plan_createBeforeDestroy_depends_datasource(t *testing.T) { } }) } + + wantAddrs := map[string]struct{}{ + "aws_instance.foo[0]": struct{}{}, + "aws_instance.foo[1]": struct{}{}, + "data.aws_vpc.bar[0]": struct{}{}, + "data.aws_vpc.bar[1]": struct{}{}, + } + if !cmp.Equal(seenAddrs, wantAddrs) { + t.Errorf("incorrect addresses in changeset:\n%s", cmp.Diff(wantAddrs, seenAddrs)) + } } // interpolated lists need to be stored in the original order. diff --git a/terraform/eval_read_data.go b/terraform/eval_read_data.go index 54d59eea4..d89d02ba9 100644 --- a/terraform/eval_read_data.go +++ b/terraform/eval_read_data.go @@ -104,7 +104,7 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { return nil, diags.Err() } - proposedNewVal := objchange.ProposedNewObject(schema, priorVal, configVal) + proposedNewVal := objchange.PlannedDataResourceObject(schema, configVal) // If our configuration contains any unknown values then we must defer the // read to the apply phase by producing a "Read" change for this resource, @@ -119,7 +119,11 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { absAddr, ) } - log.Printf("[TRACE] EvalReadData: %s configuration not fully known yet, so deferring to apply phase", absAddr) + if n.ForcePlanRead { + log.Printf("[TRACE] EvalReadData: %s configuration is fully known, but we're forcing a read plan to be created", absAddr) + } else { + log.Printf("[TRACE] EvalReadData: %s configuration not fully known yet, so deferring to apply phase", absAddr) + } err := ctx.Hook(func(h Hook) (HookAction, error) { return h.PreDiff(absAddr, states.CurrentGen, priorVal, proposedNewVal)