From 0eb7cfd0d94050492e3f7dbaad430d470fd9ef18 Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Mon, 26 Aug 2019 13:27:33 -0400 Subject: [PATCH 1/6] Check for wholly known for forEach evaluation, add some tests --- terraform/context_apply_test.go | 35 +++++++++++++++++++ terraform/context_plan_test.go | 35 +++++++++++++++++++ terraform/eval_for_each.go | 2 +- terraform/node_data_refresh.go | 1 + terraform/terraform_test.go | 17 ++++++++- .../testdata/plan-for-each-expression/main.tf | 11 ++++++ 6 files changed, 99 insertions(+), 2 deletions(-) create mode 100644 terraform/testdata/plan-for-each-expression/main.tf diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 20c6212c61..4d0c216e0b 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -2562,6 +2562,41 @@ func TestContext2Apply_provisionerInterpCount(t *testing.T) { } } +func TestContext2Apply_foreachVariable(t *testing.T) { + m := testModule(t, "plan-for-each-expression") + p := testProvider("aws") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn + ctx := testContext2(t, &ContextOpts{ + Config: m, + ProviderResolver: providers.ResolverFixed( + map[string]providers.Factory{ + "aws": testProviderFuncFixed(p), + }, + ), + Variables: InputValues{ + "foo": &InputValue{ + Value: cty.StringVal("hello"), + }, + }, + }) + + if _, diags := ctx.Plan(); diags.HasErrors() { + t.Fatalf("plan errors: %s", diags.Err()) + } + + state, diags := ctx.Apply() + if diags.HasErrors() { + t.Fatalf("diags: %s", diags.Err()) + } + + actual := strings.TrimSpace(state.String()) + expected := strings.TrimSpace(testTerraformApplyForEachVariableStr) + if actual != expected { + t.Fatalf("wrong result\n\ngot:\n%s\n\nwant:\n%s", actual, expected) + } +} + func TestContext2Apply_moduleBasic(t *testing.T) { m := testModule(t, "apply-module") p := testProvider("aws") diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index d1e3c9ed26..4b16ceb731 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -3385,7 +3385,42 @@ func TestContext2Plan_forEach(t *testing.T) { t.Fatal(err) } } +} +func TestContext2Plan_forEachExpression(t *testing.T) { + m := testModule(t, "plan-for-each-expression") + p := testProvider("aws") + p.DiffFn = testDiffFn + ctx := testContext2(t, &ContextOpts{ + Config: m, + ProviderResolver: providers.ResolverFixed( + map[string]providers.Factory{ + "aws": testProviderFuncFixed(p), + }, + ), + }) + + plan, diags := ctx.Plan() + if diags.HasErrors() { + t.Fatalf("unexpected errors: %s", diags.Err()) + } + + schema := p.GetSchemaReturn.ResourceTypes["aws_instance"] + ty := schema.ImpliedType() + + if len(plan.Changes.Resources) != 3 { + t.Fatal("expected 3 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) + } + _, err := res.Decode(ty) + if err != nil { + t.Fatal(err) + } + } } func TestContext2Plan_destroy(t *testing.T) { diff --git a/terraform/eval_for_each.go b/terraform/eval_for_each.go index 333f65e268..86160e7656 100644 --- a/terraform/eval_for_each.go +++ b/terraform/eval_for_each.go @@ -50,7 +50,7 @@ func evaluateResourceForEachExpressionKnown(expr hcl.Expression, ctx EvalContext Subject: expr.Range().Ptr(), }) return nil, true, diags - case !forEachVal.IsKnown(): + case !forEachVal.IsWhollyKnown(): return map[string]cty.Value{}, false, diags } diff --git a/terraform/node_data_refresh.go b/terraform/node_data_refresh.go index dd9286648b..12f8bbce5e 100644 --- a/terraform/node_data_refresh.go +++ b/terraform/node_data_refresh.go @@ -39,6 +39,7 @@ func (n *NodeRefreshableDataResource) DynamicExpand(ctx EvalContext) (*Graph, er } forEachMap, forEachKnown, forEachDiags := evaluateResourceForEachExpressionKnown(n.Config.ForEach, ctx) + diags = diags.Append(forEachDiags) if forEachDiags.HasErrors() { return nil, diags.Err() } diff --git a/terraform/terraform_test.go b/terraform/terraform_test.go index 9f0a63529d..3a9d88c0b0 100644 --- a/terraform/terraform_test.go +++ b/terraform/terraform_test.go @@ -538,7 +538,22 @@ aws_instance.foo.1: ID = foo provider = provider.aws ` - +const testTerraformApplyForEachVariableStr = ` +aws_instance.foo["b15c6d616d6143248c575900dff57325eb1de498"]: + ID = foo + provider = provider.aws + foo = foo + type = aws_instance +aws_instance.foo["c3de47d34b0a9f13918dd705c141d579dd6555fd"]: + ID = foo + provider = provider.aws + foo = foo + type = aws_instance +aws_instance.foo["e30a7edcc42a846684f2a4eea5f3cd261d33c46d"]: + ID = foo + provider = provider.aws + foo = foo + type = aws_instance` const testTerraformApplyMinimalStr = ` aws_instance.bar: ID = foo diff --git a/terraform/testdata/plan-for-each-expression/main.tf b/terraform/testdata/plan-for-each-expression/main.tf new file mode 100644 index 0000000000..2b26dbd1ff --- /dev/null +++ b/terraform/testdata/plan-for-each-expression/main.tf @@ -0,0 +1,11 @@ +# expressions with variable reference +variable "foo" { + type = string +} + +resource "aws_instance" "foo" { + for_each = toset( + [for i in range(0,3) : sha1("${i}${var.foo}")] + ) + foo = "foo" +} From a4d2bf45fc1d1a6f4ad6d63876ad2aa9e2a36190 Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Mon, 26 Aug 2019 15:25:03 -0400 Subject: [PATCH 2/6] Update tests, plan test is able to reproduce panic without fix --- terraform/context_apply_test.go | 4 +-- terraform/context_plan_test.go | 29 +++++-------------- terraform/eval_for_each.go | 2 +- .../main.tf | 0 4 files changed, 10 insertions(+), 25 deletions(-) rename terraform/testdata/{plan-for-each-expression => plan-for-each-unknown-value}/main.tf (100%) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 4d0c216e0b..e9bd58dc83 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -2562,8 +2562,8 @@ func TestContext2Apply_provisionerInterpCount(t *testing.T) { } } -func TestContext2Apply_foreachVariable(t *testing.T) { - m := testModule(t, "plan-for-each-expression") +func TestContext2Apply_foreachUnknownValue(t *testing.T) { + m := testModule(t, "plan-for-each-unknown-value") p := testProvider("aws") p.ApplyFn = testApplyFn p.DiffFn = testDiffFn diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index 4b16ceb731..0265faf8ad 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -3387,8 +3387,10 @@ func TestContext2Plan_forEach(t *testing.T) { } } -func TestContext2Plan_forEachExpression(t *testing.T) { - m := testModule(t, "plan-for-each-expression") +func TestContext2Plan_forEachUnknownValue(t *testing.T) { + // This module has a variable defined, but it is not provided + // in the context and we expect the plan to error, but not panic + m := testModule(t, "plan-for-each-unknown-value") p := testProvider("aws") p.DiffFn = testDiffFn ctx := testContext2(t, &ContextOpts{ @@ -3400,26 +3402,9 @@ func TestContext2Plan_forEachExpression(t *testing.T) { ), }) - plan, diags := ctx.Plan() - if diags.HasErrors() { - t.Fatalf("unexpected errors: %s", diags.Err()) - } - - schema := p.GetSchemaReturn.ResourceTypes["aws_instance"] - ty := schema.ImpliedType() - - if len(plan.Changes.Resources) != 3 { - t.Fatal("expected 3 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) - } - _, err := res.Decode(ty) - if err != nil { - t.Fatal(err) - } + _, diags := ctx.Plan() + if !diags.HasErrors() { + t.Fatalf("Expected invalid error") } } diff --git a/terraform/eval_for_each.go b/terraform/eval_for_each.go index 86160e7656..23ba388b5c 100644 --- a/terraform/eval_for_each.go +++ b/terraform/eval_for_each.go @@ -19,7 +19,7 @@ func evaluateResourceForEachExpression(expr hcl.Expression, ctx EvalContext) (fo // Attach a diag as we do with count, with the same downsides diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, - Summary: "Invalid forEach argument", + Summary: "Invalid for_each argument", Detail: `The "for_each" value depends on resource attributes that cannot be determined until apply, so Terraform cannot predict how many instances will be created. To work around this, use the -target argument to first apply only the resources that the for_each depends on.`, }) } diff --git a/terraform/testdata/plan-for-each-expression/main.tf b/terraform/testdata/plan-for-each-unknown-value/main.tf similarity index 100% rename from terraform/testdata/plan-for-each-expression/main.tf rename to terraform/testdata/plan-for-each-unknown-value/main.tf From cce35e4a368be084c43ce1ac41e59aab6e95bf26 Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Mon, 26 Aug 2019 15:27:07 -0400 Subject: [PATCH 3/6] update name --- terraform/context_apply_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index e9bd58dc83..73dab280ec 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -2562,7 +2562,7 @@ func TestContext2Apply_provisionerInterpCount(t *testing.T) { } } -func TestContext2Apply_foreachUnknownValue(t *testing.T) { +func TestContext2Apply_foreachVariable(t *testing.T) { m := testModule(t, "plan-for-each-unknown-value") p := testProvider("aws") p.ApplyFn = testApplyFn From 0f3d8b4884785730465b3382e3c8f1547f342c7a Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Mon, 26 Aug 2019 15:30:42 -0400 Subject: [PATCH 4/6] More explicit err testing --- terraform/context_plan_test.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index 0265faf8ad..8d15078f0d 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -3389,7 +3389,7 @@ func TestContext2Plan_forEach(t *testing.T) { func TestContext2Plan_forEachUnknownValue(t *testing.T) { // This module has a variable defined, but it is not provided - // in the context and we expect the plan to error, but not panic + // in the context below and we expect the plan to error, but not panic m := testModule(t, "plan-for-each-unknown-value") p := testProvider("aws") p.DiffFn = testDiffFn @@ -3404,7 +3404,15 @@ func TestContext2Plan_forEachUnknownValue(t *testing.T) { _, diags := ctx.Plan() if !diags.HasErrors() { - t.Fatalf("Expected invalid error") + // Should get this error: + // Invalid for_each argument: The "for_each" value depends on resource attributes that cannot be determined until apply... + t.Fatal("succeeded; want errors") + } + + gotErrStr := diags.Err().Error() + wantErrStr := "Invalid for_each argument" + if !strings.Contains(gotErrStr, wantErrStr) { + t.Fatalf("missing expected error\ngot: %s\n\nwant: error containing %q", gotErrStr, wantErrStr) } } From 35016a5ea32f5f05a30e09905c0bfcde07cc54bd Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Wed, 28 Aug 2019 14:02:11 -0400 Subject: [PATCH 5/6] Move things around, add test for resource references --- terraform/eval_for_each.go | 7 ++++++- terraform/terraform_test.go | 20 ++++++++++++++++++- .../plan-for-each-unknown-value/main.tf | 9 +++++++++ 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/terraform/eval_for_each.go b/terraform/eval_for_each.go index 23ba388b5c..1db1c3fad6 100644 --- a/terraform/eval_for_each.go +++ b/terraform/eval_for_each.go @@ -50,7 +50,7 @@ func evaluateResourceForEachExpressionKnown(expr hcl.Expression, ctx EvalContext Subject: expr.Range().Ptr(), }) return nil, true, diags - case !forEachVal.IsWhollyKnown(): + case !forEachVal.IsKnown(): return map[string]cty.Value{}, false, diags } @@ -74,6 +74,11 @@ func evaluateResourceForEachExpressionKnown(expr hcl.Expression, ctx EvalContext }) return nil, true, diags } + + // For sets, we need to recursively check ...and add reasoning why this works here + if !forEachVal.IsWhollyKnown() { + return map[string]cty.Value{}, false, diags + } } // If the map is empty ({}), return an empty map, because cty will return nil when representing {} AsValueMap diff --git a/terraform/terraform_test.go b/terraform/terraform_test.go index 3a9d88c0b0..0d8cc0900a 100644 --- a/terraform/terraform_test.go +++ b/terraform/terraform_test.go @@ -553,7 +553,25 @@ aws_instance.foo["e30a7edcc42a846684f2a4eea5f3cd261d33c46d"]: ID = foo provider = provider.aws foo = foo - type = aws_instance` + type = aws_instance +aws_instance.one["a"]: + ID = foo + provider = provider.aws +aws_instance.one["b"]: + ID = foo + provider = provider.aws +aws_instance.two["a"]: + ID = foo + provider = provider.aws + + Dependencies: + aws_instance.one +aws_instance.two["b"]: + ID = foo + provider = provider.aws + + Dependencies: + aws_instance.one` const testTerraformApplyMinimalStr = ` aws_instance.bar: ID = foo diff --git a/terraform/testdata/plan-for-each-unknown-value/main.tf b/terraform/testdata/plan-for-each-unknown-value/main.tf index 2b26dbd1ff..933ed5f4c3 100644 --- a/terraform/testdata/plan-for-each-unknown-value/main.tf +++ b/terraform/testdata/plan-for-each-unknown-value/main.tf @@ -9,3 +9,12 @@ resource "aws_instance" "foo" { ) foo = "foo" } + +# referencing another resource, which means it has some unknown values in it +resource "aws_instance" "one" { + for_each = toset(["a", "b"]) +} + +resource "aws_instance" "two" { + for_each = aws_instance.one +} From 37e8147c4ff0efbd73c4d07398cdc209b9c93e1a Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Wed, 28 Aug 2019 15:13:36 -0400 Subject: [PATCH 6/6] Spiffy comment --- terraform/eval_for_each.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/terraform/eval_for_each.go b/terraform/eval_for_each.go index 1db1c3fad6..f03e02c273 100644 --- a/terraform/eval_for_each.go +++ b/terraform/eval_for_each.go @@ -75,7 +75,10 @@ func evaluateResourceForEachExpressionKnown(expr hcl.Expression, ctx EvalContext return nil, true, diags } - // For sets, we need to recursively check ...and add reasoning why this works here + // A set may contain unknown values that must be + // discovered by checking with IsWhollyKnown (which iterates through the + // structure), while for maps in cty, keys can never be unknown or null, + // thus the earlier IsKnown check suffices for maps if !forEachVal.IsWhollyKnown() { return map[string]cty.Value{}, false, diags }