From c9e7346bfd9352397c21e5f0884a200ff8f37724 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 31 Oct 2018 13:44:21 -0400 Subject: [PATCH 1/6] create a new proposed value when replacing When replacing an instance, calculate a new proposed value from the null state and the config. This ensures that all unknown values are properly set. --- terraform/eval_diff.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/terraform/eval_diff.go b/terraform/eval_diff.go index 0b6c4aff3..50bb5ea3b 100644 --- a/terraform/eval_diff.go +++ b/terraform/eval_diff.go @@ -310,11 +310,15 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { // from known prior values to unknown values, unless the provider is // able to predict new values for any of these computed attributes. nullPriorVal := cty.NullVal(schema.ImpliedType()) + + // create a new proposed value from the null state and the config + proposedNewVal = objchange.ProposedNewObject(schema, nullPriorVal, configVal) + resp = provider.PlanResourceChange(providers.PlanResourceChangeRequest{ TypeName: n.Addr.Resource.Type, Config: configVal, PriorState: nullPriorVal, - ProposedNewState: configVal, + ProposedNewState: proposedNewVal, PriorPrivate: plannedPrivate, }) // We need to tread carefully here, since if there are any warnings From 21064771ea3d9c2fcebd364997f1d25eec3ec191 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 31 Oct 2018 16:41:36 -0400 Subject: [PATCH 2/6] add failing test for required output value The required value from an output is nil when it should be unknown --- terraform/context_plan_test.go | 65 +++++++++++++++++++ .../plan-required-output/main.tf | 7 ++ .../plan-required-output/mod/main.tf | 7 ++ 3 files changed, 79 insertions(+) create mode 100644 terraform/test-fixtures/plan-required-output/main.tf create mode 100644 terraform/test-fixtures/plan-required-output/mod/main.tf diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index 581a0f429..3a9b826be 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -5513,3 +5513,68 @@ func objectVal(t *testing.T, schema *configschema.Block, m map[string]cty.Value) } return v } + +func TestContext2Plan_requiredModuleOutput(t *testing.T) { + m := testModule(t, "plan-required-output") + 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 { + 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/test-fixtures/plan-required-output/main.tf b/terraform/test-fixtures/plan-required-output/main.tf new file mode 100644 index 000000000..227b5c153 --- /dev/null +++ b/terraform/test-fixtures/plan-required-output/main.tf @@ -0,0 +1,7 @@ +resource "test_resource" "root" { + required = module.mod.object.id +} + +module "mod" { + source = "./mod" +} diff --git a/terraform/test-fixtures/plan-required-output/mod/main.tf b/terraform/test-fixtures/plan-required-output/mod/main.tf new file mode 100644 index 000000000..772f1645f --- /dev/null +++ b/terraform/test-fixtures/plan-required-output/mod/main.tf @@ -0,0 +1,7 @@ +resource "test_resource" "for_output" { + required = "val" +} + +output "object" { + value = test_resource.for_output +} From d73b2d778fe15cbd0cee7e5ea1bf9077c5106197 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 1 Nov 2018 17:32:30 -0700 Subject: [PATCH 3/6] core: TestContext2Plan_requiredModuleOutput to use t.Run This allows us to see the results of the tests for all resources even if one of them fails. --- terraform/context_plan_test.go | 48 ++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 23 deletions(-) diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index 3a9b826be..03dee235e 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -5551,30 +5551,32 @@ func TestContext2Plan_requiredModuleOutput(t *testing.T) { } for _, res := range plan.Changes.Resources { - 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) - } + 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) - } + 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) + checkVals(t, expected, ric.After) + }) } } From bbf8dacac808ee3dc2a9ff27adb2073a155b0a63 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 1 Nov 2018 17:33:10 -0700 Subject: [PATCH 4/6] plans: OutputChange.Encode must preserve Addr field --- plans/changes.go | 1 + 1 file changed, 1 insertion(+) diff --git a/plans/changes.go b/plans/changes.go index cc6e58263..d7e0dcdb8 100644 --- a/plans/changes.go +++ b/plans/changes.go @@ -254,6 +254,7 @@ func (oc *OutputChange) Encode() (*OutputChangeSrc, error) { return nil, err } return &OutputChangeSrc{ + Addr: oc.Addr, ChangeSrc: *cs, Sensitive: oc.Sensitive, }, err From 21577a5f158adffc4e87d6734a708b452b9c70d5 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 1 Nov 2018 17:41:35 -0700 Subject: [PATCH 5/6] 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 +} From ab62b330c16ce38ea453d925813b4521c65ce8a6 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Mon, 5 Nov 2018 16:02:45 -0800 Subject: [PATCH 6/6] core: Allow planned output changes to be updated during apply If plan and apply are both run against the same context then we still have the planned output values in memory while we're doing the apply walk, so we need to make sure to update them along with the state as we learn the final known values of each output. There were actually two different bugs here: - We weren't removing any existing planned change for an output when setting a new one. In retrospect a map would've been a better data structure for the output changes, rather than a slice to mimic what we do for resource instance objects, but for now we'll leave the structures alone and clean up as needed. (The set of outputs should be small for any reasonable configuration, so the main impact of this is some ugly code in RemoveOutputChange.) - RemoveOutputChange itself had a bug where it was iterating over the resource changes rather than the output changes. This didn't matter before because we weren't actually using that function, but now we are. This fix is confirmed by restoring various existing context apply tests back to passing again. --- plans/changes_sync.go | 4 ++-- terraform/eval_output.go | 20 ++++++++++++++++---- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/plans/changes_sync.go b/plans/changes_sync.go index e9305eaf9..6b4ff98ff 100644 --- a/plans/changes_sync.go +++ b/plans/changes_sync.go @@ -133,8 +133,8 @@ func (cs *ChangesSync) RemoveOutputChange(addr addrs.AbsOutputValue) { defer cs.lock.Unlock() addrStr := addr.String() - for i, r := range cs.changes.Resources { - if r.Addr.String() != addrStr { + for i, o := range cs.changes.Outputs { + if o.Addr.String() != addrStr { continue } copy(cs.changes.Outputs[i:], cs.changes.Outputs[i+1:]) diff --git a/terraform/eval_output.go b/terraform/eval_output.go index 6829934f0..10573971f 100644 --- a/terraform/eval_output.go +++ b/terraform/eval_output.go @@ -9,6 +9,7 @@ import ( "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/plans" + "github.com/hashicorp/terraform/states" ) // EvalDeleteOutput is an EvalNode implementation that deletes an output @@ -61,22 +62,33 @@ func (n *EvalWriteOutput) Eval(ctx EvalContext) (interface{}, error) { // if we're continuing, make sure the output is included, and // marked as unknown. If the evaluator was able to find a type // for the value in spite of the error then we'll use it. - state.SetOutputValue(addr, cty.UnknownVal(val.Type()), n.Sensitive) + n.setValue(addr, state, changes, cty.UnknownVal(val.Type())) return nil, EvalEarlyExitError{} } return nil, diags.Err() } + n.setValue(addr, state, changes, val) + + return nil, nil +} + +func (n *EvalWriteOutput) setValue(addr addrs.AbsOutputValue, state *states.SyncState, changes *plans.ChangesSync, val cty.Value) { if val.IsKnown() && !val.IsNull() { // The state itself doesn't represent unknown values, so we null them // out here and then we'll save the real unknown value in the planned // changeset below, if we have one on this graph walk. + log.Printf("[TRACE] EvalWriteOutput: Saving value for %s in state", addr) stateVal := cty.UnknownAsNull(val) state.SetOutputValue(addr, stateVal, n.Sensitive) } else { + log.Printf("[TRACE] EvalWriteOutput: Removing %s from state (it is now null)", addr) state.RemoveOutputValue(addr) } + // If we also have an active changeset then we'll replicate the value in + // there. This is used in preference to the state where present, since it + // *is* able to represent unknowns, while the state cannot. if changes != nil { // For the moment we are not properly tracking changes to output // values, and just marking them always as "Create" or "Destroy" @@ -116,8 +128,8 @@ func (n *EvalWriteOutput) Eval(ctx EvalContext) (interface{}, error) { // Should never happen, since we just constructed this right above panic(fmt.Sprintf("planned change for %s could not be encoded: %s", addr, err)) } - changes.AppendOutputChange(cs) + log.Printf("[TRACE] EvalWriteOutput: Saving %s change for %s in changeset", change.Action, addr) + changes.RemoveOutputChange(addr) // remove any existing planned change, if present + changes.AppendOutputChange(cs) // add the new planned change } - - return nil, nil }