From 05575a863c82bcbac038a593f7fca0077acfb34d Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 8 May 2020 18:46:32 -0400 Subject: [PATCH] check for data source changed during plan Rather than re-read the data source during every plan cycle, apply the config to the prior state, and skip reading if there is no change. Remove the TODOs, as we're going to accept that data-only changes will still not be plan-able for the time being. Fix the null data source test resource, as it had no computed fields at all, even the id. --- terraform/context_apply_test.go | 10 ++++++ terraform/context_plan_test.go | 5 +-- terraform/context_test.go | 3 +- terraform/eval_read_data.go | 34 +++++++++---------- terraform/eval_read_data_plan.go | 34 ++++++++++--------- .../testdata/apply-data-depends-on/main.tf | 1 - terraform/transform_reference.go | 4 +-- 7 files changed, 52 insertions(+), 39 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index f87bb720c..b26d2c481 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -8782,6 +8782,16 @@ func TestContext2Apply_dataDependsOn(t *testing.T) { if actual != expected { t.Fatalf("bad:\n%s", strings.TrimSpace(state.String())) } + + // run another plan to make sure the data source doesn't show as a change + plan, diags := ctx.Plan() + assertNoErrors(t, diags) + + for _, c := range plan.Changes.Resources { + if c.Action != plans.NoOp { + t.Fatalf("unexpected change for %s", c.Addr) + } + } } func TestContext2Apply_terraformWorkspace(t *testing.T) { diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index 282f07f1b..d83ab465b 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -1892,8 +1892,9 @@ func TestContext2Plan_computedInFunction(t *testing.T) { _, diags = ctx.Plan() // should do nothing with data resource in this step, since it was already read assertNoErrors(t, diags) - if !p.ReadDataSourceCalled { - t.Fatalf("ReadDataSource should have been called") + if p.ReadDataSourceCalled { + // there was no config change to read during plan + t.Fatalf("ReadDataSource should not have been called") } } diff --git a/terraform/context_test.go b/terraform/context_test.go index 58b896284..f071a2afa 100644 --- a/terraform/context_test.go +++ b/terraform/context_test.go @@ -693,11 +693,12 @@ func testProviderSchema(name string) *ProviderSchema { Attributes: map[string]*configschema.Attribute{ "id": { Type: cty.String, - Optional: true, + Computed: true, }, "foo": { Type: cty.String, Optional: true, + Computed: true, }, }, }, diff --git a/terraform/eval_read_data.go b/terraform/eval_read_data.go index 001afd325..c85e60f73 100644 --- a/terraform/eval_read_data.go +++ b/terraform/eval_read_data.go @@ -236,19 +236,24 @@ func (n *EvalReadDataRefresh) Eval(ctx EvalContext) (interface{}, error) { } configKnown := configVal.IsWhollyKnown() - // If our configuration contains any unknown values, or we depend on any - // unknown values then we must defer the read to the apply phase by - // producing a "Read" change for this resource, and a placeholder value for - // it in the state. - if len(n.Config.DependsOn) > 0 || !configKnown { + // If our configuration contains any unknown values, then we must defer the + // read until plan or apply. If we've never read this data source and we + // have any depends_on, we will have to defer reading until plan to resolve + // the dependency changes. + // Assuming we can read the data source with depends_on if we have + // existing state is a compromise to prevent data sources from continually + // showing a diff. We have to make the assumption that if we have a prior + // state, since there are no prior dependency changes happening during + // refresh, that we can read this resource. If there are dependency updates + // in the config, they we be discovered in plan and the data source will be + // read again. + if !configKnown || (priorVal.IsNull() && len(n.Config.DependsOn) > 0) { if configKnown { log.Printf("[TRACE] EvalReadDataRefresh: %s configuration is fully known, but we're forcing a read plan to be created", absAddr) } else { log.Printf("[TRACE] EvalReadDataRefresh: %s configuration not fully known yet, so deferring to apply phase", absAddr) } - proposedNewVal := objchange.PlannedDataResourceObject(schema, configVal) - // We need to store a change so tat other references to this data // source can resolve correctly, since the state is not going to be up // to date. @@ -258,21 +263,17 @@ func (n *EvalReadDataRefresh) Eval(ctx EvalContext) (interface{}, error) { Change: plans.Change{ Action: plans.Read, Before: priorVal, - After: proposedNewVal, + After: objchange.PlannedDataResourceObject(schema, configVal), }, } if n.OutputChange != nil { *n.OutputChange = change } + if n.State != nil { *n.State = &states.ResourceInstanceObject{ - // We need to keep the prior value in the state so that plan - // has something to diff against. - Value: priorVal, - // TODO: this needs to be ObjectPlanned to trigger a plan, but - // the prior value is lost preventing plan from resulting in a - // NoOp + Value: cty.NullVal(objTy), Status: states.ObjectPlanned, } } @@ -293,9 +294,8 @@ func (n *EvalReadDataRefresh) Eval(ctx EvalContext) (interface{}, error) { return nil, diags.ErrWithWarnings() } - // TODO: Need to signal to plan that this may have changed. We may be able - // to use ObjectPlanned for that, but that currently causes the state to be - // dropped altogether + // This may still have been refreshed with references to resources that + // will be updated, but that will be caught as a change during plan. outputState := &states.ResourceInstanceObject{ Value: newVal, Status: states.ObjectReady, diff --git a/terraform/eval_read_data_plan.go b/terraform/eval_read_data_plan.go index 63051cbd1..fe0761056 100644 --- a/terraform/eval_read_data_plan.go +++ b/terraform/eval_read_data_plan.go @@ -61,7 +61,6 @@ func (n *EvalReadDataPlan) Eval(ctx EvalContext) (interface{}, error) { } configKnown := configVal.IsWhollyKnown() - proposedNewVal := objchange.PlannedDataResourceObject(schema, configVal) // If our configuration contains any unknown values, or we depend on any // unknown values then we must defer the read to the apply phase by // producing a "Read" change for this resource, and a placeholder value for @@ -73,6 +72,8 @@ func (n *EvalReadDataPlan) Eval(ctx EvalContext) (interface{}, error) { log.Printf("[TRACE] EvalReadDataPlan: %s configuration not fully known yet, so deferring to apply phase", absAddr) } + proposedNewVal := objchange.PlannedDataResourceObject(schema, configVal) + err := ctx.Hook(func(h Hook) (HookAction, error) { return h.PreDiff(absAddr, states.CurrentGen, priorVal, proposedNewVal) }) @@ -101,42 +102,43 @@ func (n *EvalReadDataPlan) Eval(ctx EvalContext) (interface{}, error) { return nil, diags.ErrWithWarnings() } + // If we have a stored state we may not need to re-read the data source. + // Check the config against the state to see if there are any difference. + if !priorVal.IsNull() { + // Applying the configuration to the prior state lets us see if there + // are any differences. + proposed := objchange.ProposedNewObject(schema, priorVal, configVal) + if proposed.Equals(priorVal).True() { + log.Printf("[TRACE] EvalReadDataPlan: %s no change detected, using existing state", absAddr) + // state looks up to date, and must have been read during refresh + return nil, diags.ErrWithWarnings() + } + } + newVal, readDiags := n.readDataSource(ctx, configVal) diags = diags.Append(readDiags) if diags.HasErrors() { return nil, diags.ErrWithWarnings() } - action := plans.NoOp - if !newVal.IsNull() && newVal.IsKnown() && newVal.Equals(priorVal).False() { - // since a data source is read-only, update here only means that we - // need to update the state. - action = plans.Update - } - // Produce a change regardless of the outcome. change := &plans.ResourceInstanceChange{ Addr: absAddr, ProviderAddr: n.ProviderAddr, Change: plans.Change{ - Action: action, + Action: plans.Update, Before: priorVal, After: newVal, }, } - status := states.ObjectReady - if action == plans.Update { - status = states.ObjectPlanned - } - outputState := &states.ResourceInstanceObject{ Value: newVal, - Status: status, + Status: states.ObjectPlanned, } if err := ctx.Hook(func(h Hook) (HookAction, error) { - return h.PostDiff(absAddr, states.CurrentGen, action, priorVal, newVal) + return h.PostDiff(absAddr, states.CurrentGen, plans.Update, priorVal, newVal) }); err != nil { return nil, err } diff --git a/terraform/testdata/apply-data-depends-on/main.tf b/terraform/testdata/apply-data-depends-on/main.tf index 0371413cd..f48b48e40 100644 --- a/terraform/testdata/apply-data-depends-on/main.tf +++ b/terraform/testdata/apply-data-depends-on/main.tf @@ -3,6 +3,5 @@ resource "null_instance" "write" { } data "null_data_source" "read" { - foo = "" depends_on = ["null_instance.write"] } diff --git a/terraform/transform_reference.go b/terraform/transform_reference.go index d246a56ce..5280c67d2 100644 --- a/terraform/transform_reference.go +++ b/terraform/transform_reference.go @@ -51,7 +51,7 @@ type GraphNodeAttachDependencies interface { // not yet expended in the graph. While this will cause some extra data // resources to show in the plan when their depends_on references may be in // unrelated module instances, the fact that it only happens when there are any -// resource updates pending means we ca still avoid the problem of the +// resource updates pending means we can still avoid the problem of the // "perpetual diff" type GraphNodeAttachDependsOn interface { GraphNodeConfigResource @@ -83,7 +83,7 @@ type GraphNodeReferenceOutside interface { ReferenceOutside() (selfPath, referencePath addrs.Module) } -// Referenceeransformer is a GraphTransformer that connects all the +// ReferenceTransformer is a GraphTransformer that connects all the // nodes that reference each other in order to form the proper ordering. type ReferenceTransformer struct{}