From d469e86331a6dc4eee9a6d45e00c12dae80bcb32 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 14 Dec 2021 17:50:53 -0500 Subject: [PATCH 1/3] revert 6b8b0617 Revert the evaluation change from #29862. While returning a dynamic value for all expanded resources during validation is not optimal, trying to work around this using unknown maps and lists is causing other undesirable behaviors during evaluation. --- internal/terraform/context_plan_test.go | 1 - internal/terraform/evaluate.go | 19 +++++-------------- .../testdata/plan-self-ref-multi-all/main.tf | 2 +- 3 files changed, 6 insertions(+), 16 deletions(-) diff --git a/internal/terraform/context_plan_test.go b/internal/terraform/context_plan_test.go index 1159f0b42..cfd51da8c 100644 --- a/internal/terraform/context_plan_test.go +++ b/internal/terraform/context_plan_test.go @@ -5308,7 +5308,6 @@ func TestContext2Plan_selfRefMultiAll(t *testing.T) { ResourceTypes: map[string]*configschema.Block{ "aws_instance": { Attributes: map[string]*configschema.Attribute{ - "id": {Type: cty.String, Computed: true}, "foo": {Type: cty.List(cty.String), Optional: true}, }, }, diff --git a/internal/terraform/evaluate.go b/internal/terraform/evaluate.go index 8fd05d548..a017cf175 100644 --- a/internal/terraform/evaluate.go +++ b/internal/terraform/evaluate.go @@ -696,21 +696,12 @@ func (d *evaluationStateData) GetResource(addr addrs.Resource, rng tfdiags.Sourc log.Printf("[ERROR] unknown instance %q referenced during %s", addr.Absolute(d.ModulePath), d.Operation) return cty.DynamicVal, diags } - default: - if d.Operation != walkValidate { - log.Printf("[ERROR] missing state for %q while in %s\n", addr.Absolute(d.ModulePath), d.Operation) - } - // Validation is done with only the configuration, so generate - // unknown values of the correct shape for evaluation. - switch { - case config.Count != nil: - return cty.UnknownVal(cty.List(ty)), diags - case config.ForEach != nil: - return cty.UnknownVal(cty.Map(ty)), diags - default: - return cty.UnknownVal(ty), diags - } + default: + // We should only end up here during the validate walk, + // since later walks should have at least partial states populated + // for all resources in the configuration. + return cty.DynamicVal, diags } } diff --git a/internal/terraform/testdata/plan-self-ref-multi-all/main.tf b/internal/terraform/testdata/plan-self-ref-multi-all/main.tf index a4d28f8a0..d3a9857f7 100644 --- a/internal/terraform/testdata/plan-self-ref-multi-all/main.tf +++ b/internal/terraform/testdata/plan-self-ref-multi-all/main.tf @@ -1,4 +1,4 @@ resource "aws_instance" "web" { - foo = aws_instance.web[*].id + foo = "${aws_instance.web.*.foo}" count = 4 } From 71b9682e8ce40d389b48377ff23c98c887bbcc11 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 14 Dec 2021 18:02:57 -0500 Subject: [PATCH 2/3] ensure there is always a valid return value --- internal/terraform/evaluate.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/terraform/evaluate.go b/internal/terraform/evaluate.go index a017cf175..322ef6fda 100644 --- a/internal/terraform/evaluate.go +++ b/internal/terraform/evaluate.go @@ -794,7 +794,7 @@ func (d *evaluationStateData) GetResource(addr addrs.Resource, rng tfdiags.Sourc instances[key] = val } - var ret cty.Value + ret := cty.DynamicVal switch { case config.Count != nil: From 645fcc5f1298b2b05b6e5151263cb08edd72ce4a Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 14 Dec 2021 18:29:38 -0500 Subject: [PATCH 3/3] test for lookup regression during validation --- internal/terraform/context_validate_test.go | 57 +++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/internal/terraform/context_validate_test.go b/internal/terraform/context_validate_test.go index 4f628945e..1ed5ad425 100644 --- a/internal/terraform/context_validate_test.go +++ b/internal/terraform/context_validate_test.go @@ -2031,3 +2031,60 @@ resource "test_object" "t" { t.Fatal(diags.ErrWithWarnings()) } } + +func TestContext2Plan_lookupMismatchedObjectTypes(t *testing.T) { + p := new(MockProvider) + p.GetProviderSchemaResponse = getProviderSchemaResponseFromProviderSchema(&ProviderSchema{ + ResourceTypes: map[string]*configschema.Block{ + "test_instance": { + Attributes: map[string]*configschema.Attribute{ + "id": { + Type: cty.String, + Computed: true, + }, + "things": { + Type: cty.List(cty.String), + Optional: true, + }, + }, + }, + }, + }) + + m := testModuleInline(t, map[string]string{ + "main.tf": ` +variable "items" { + type = list(string) + default = [] +} + +resource "test_instance" "a" { + for_each = length(var.items) > 0 ? { default = {} } : {} +} + +output "out" { + // Strictly speaking, this expression is incorrect because the map element + // type is a different type from the default value, and the lookup + // implementation expects to be able to convert the default to match the + // element type. + // There are two reasons this works which we need to maintain for + // compatibility. First during validation the 'test_instance.a' expression + // only returns a dynamic value, preventing any type comparison. Later during + // plan and apply 'test_instance.a' is an object and not a map, and the + // lookup implementation skips the type comparison when the keys are known + // statically. + value = lookup(test_instance.a, "default", { id = null })["id"] +} +`}) + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + diags := ctx.Validate(m) + if diags.HasErrors() { + t.Fatal(diags.ErrWithWarnings()) + } +}