From 21577a5f158adffc4e87d6734a708b452b9c70d5 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 1 Nov 2018 17:41:35 -0700 Subject: [PATCH] core: Whole-module evaluation must consider planned output values Just as when we resolve single output values we must check to see if there is a planned new value for an output before using the value in state, because the planned new value might contain unknowns that can't be represented directly in the state (and would thus be incorrectly returned as null). --- terraform/context_plan_test.go | 67 +++++++++++++++++++ terraform/evaluate.go | 31 +++++++-- .../plan-required-whole-mod/main.tf | 17 +++++ .../plan-required-whole-mod/mod/main.tf | 7 ++ 4 files changed, 115 insertions(+), 7 deletions(-) create mode 100644 terraform/test-fixtures/plan-required-whole-mod/main.tf create mode 100644 terraform/test-fixtures/plan-required-whole-mod/mod/main.tf diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index 03dee235e..e7f0e7ab2 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -5580,3 +5580,70 @@ func TestContext2Plan_requiredModuleOutput(t *testing.T) { }) } } + +func TestContext2Plan_requiredModuleObject(t *testing.T) { + m := testModule(t, "plan-required-whole-mod") + p := testProvider("test") + p.GetSchemaReturn = &ProviderSchema{ + ResourceTypes: map[string]*configschema.Block{ + "test_resource": { + Attributes: map[string]*configschema.Attribute{ + "id": {Type: cty.String, Computed: true}, + "required": {Type: cty.String, Required: true}, + }, + }, + }, + } + p.DiffFn = testDiffFn + + ctx := testContext2(t, &ContextOpts{ + Config: m, + ProviderResolver: providers.ResolverFixed( + map[string]providers.Factory{ + "test": testProviderFuncFixed(p), + }, + ), + }) + + plan, diags := ctx.Plan() + if diags.HasErrors() { + t.Fatalf("unexpected errors: %s", diags.Err()) + } + + schema := p.GetSchemaReturn.ResourceTypes["test_resource"] + ty := schema.ImpliedType() + + if len(plan.Changes.Resources) != 2 { + t.Fatal("expected 2 changes, got", len(plan.Changes.Resources)) + } + + for _, res := range plan.Changes.Resources { + t.Run(fmt.Sprintf("%s %s", res.Action, res.Addr), func(t *testing.T) { + if res.Action != plans.Create { + t.Fatalf("expected resource creation, got %s", res.Action) + } + ric, err := res.Decode(ty) + if err != nil { + t.Fatal(err) + } + + var expected cty.Value + switch i := ric.Addr.String(); i { + case "test_resource.root": + expected = objectVal(t, schema, map[string]cty.Value{ + "id": cty.UnknownVal(cty.String), + "required": cty.UnknownVal(cty.String), + }) + case "module.mod.test_resource.for_output": + expected = objectVal(t, schema, map[string]cty.Value{ + "id": cty.UnknownVal(cty.String), + "required": cty.StringVal("val"), + }) + default: + t.Fatal("unknown instance:", i) + } + + checkVals(t, expected, ric.After) + }) + } +} diff --git a/terraform/evaluate.go b/terraform/evaluate.go index bed3a6cc8..918df2e87 100644 --- a/terraform/evaluate.go +++ b/terraform/evaluate.go @@ -305,14 +305,31 @@ func (d *evaluationStateData) GetModuleInstance(addr addrs.ModuleCallInstance, r vals := map[string]cty.Value{} for n := range outputConfigs { addr := addrs.OutputValue{Name: n}.Absolute(moduleAddr) - os := d.Evaluator.State.OutputValue(addr) - if os == nil { - // Not evaluated yet? - vals[n] = cty.DynamicVal - continue - } - vals[n] = os.Value + // If a pending change is present in our current changeset then its value + // takes priority over what's in state. (It will usually be the same but + // will differ if the new value is unknown during planning.) + if changeSrc := d.Evaluator.Changes.GetOutputChange(addr); changeSrc != nil { + change, err := changeSrc.Decode() + if err != nil { + // This should happen only if someone has tampered with a plan + // file, so we won't bother with a pretty error for it. + diags = diags.Append(fmt.Errorf("planned change for %s could not be decoded: %s", addr, err)) + vals[n] = cty.DynamicVal + continue + } + // We care only about the "after" value, which is the value this output + // will take on after the plan is applied. + vals[n] = change.After + } else { + os := d.Evaluator.State.OutputValue(addr) + if os == nil { + // Not evaluated yet? + vals[n] = cty.DynamicVal + continue + } + vals[n] = os.Value + } } return cty.ObjectVal(vals), diags } diff --git a/terraform/test-fixtures/plan-required-whole-mod/main.tf b/terraform/test-fixtures/plan-required-whole-mod/main.tf new file mode 100644 index 000000000..9deb3c5a1 --- /dev/null +++ b/terraform/test-fixtures/plan-required-whole-mod/main.tf @@ -0,0 +1,17 @@ +resource "test_resource" "root" { + required = local.object.id +} + +locals { + # This indirection is here to force the evaluator to produce the whole + # module object here rather than just fetching the single "object" output. + # This makes this fixture different than plan-required-output, which just + # accesses module.mod.object.id directly and thus visits a different + # codepath in the evaluator. + mod = module.mod + object = local.mod.object +} + +module "mod" { + source = "./mod" +} diff --git a/terraform/test-fixtures/plan-required-whole-mod/mod/main.tf b/terraform/test-fixtures/plan-required-whole-mod/mod/main.tf new file mode 100644 index 000000000..772f1645f --- /dev/null +++ b/terraform/test-fixtures/plan-required-whole-mod/mod/main.tf @@ -0,0 +1,7 @@ +resource "test_resource" "for_output" { + required = "val" +} + +output "object" { + value = test_resource.for_output +}