From 016463ea9c01fbfff0bbd4bb29b29b03d935be45 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 28 Sep 2021 17:57:10 -0400 Subject: [PATCH 1/2] don't check all ancestors for data depends_on Only depends_on ancestors for transitive dependencies when we're not pointed directly at a resource. We can't be much more precise here, since in order to maintain our guarantee that data sources will wait for explicit dependencies, if those dependencies happen to be a module, output, or variable, we have to find some upstream managed resource in order to check for a planned change. --- internal/terraform/transform_reference.go | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/internal/terraform/transform_reference.go b/internal/terraform/transform_reference.go index fe64e3707..bd07161ca 100644 --- a/internal/terraform/transform_reference.go +++ b/internal/terraform/transform_reference.go @@ -353,11 +353,19 @@ func (m ReferenceMap) dependsOn(g *Graph, depender graphNodeDependsOn) ([]dag.Ve } res = append(res, rv) - // and check any ancestors for transitive dependencies - ans, _ := g.Ancestors(rv) - for _, v := range ans { - if isDependableResource(v) { - res = append(res, v) + // Check any ancestors for transitive dependencies when we're + // not pointed directly at a resource. We can't be much more + // precise here, since in order to maintain our guarantee that data + // sources will wait for explicit dependencies, if those dependencies + // happen to be a module, output, or variable, we have to find some + // upstream managed resource in order to check for a planned + // change. + if _, ok := rv.(GraphNodeConfigResource); !ok { + ans, _ := g.Ancestors(rv) + for _, v := range ans { + if isDependableResource(v) { + res = append(res, v) + } } } } From 618e9cf8ecd1b02323af7fa3e86a4dc9c4848379 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 30 Sep 2021 16:00:37 -0400 Subject: [PATCH 2/2] test for unexpected data reads --- internal/terraform/context_plan2_test.go | 108 +++++++++++++++++++++++ 1 file changed, 108 insertions(+) diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index 6d54e9eef..b1034d7fb 100644 --- a/internal/terraform/context_plan2_test.go +++ b/internal/terraform/context_plan2_test.go @@ -1971,3 +1971,111 @@ func TestContext2Plan_forceReplaceIncompleteAddr(t *testing.T) { } }) } + +// Verify that adding a module instance does force existing module data sources +// to be deferred +func TestContext2Plan_noChangeDataSourceAddingModuleInstance(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ` +locals { + data = { + a = "a" + b = "b" + } +} + +module "one" { + source = "./mod" + for_each = local.data + input = each.value +} + +module "two" { + source = "./mod" + for_each = module.one + input = each.value.output +} +`, + "mod/main.tf": ` +variable "input" { +} + +resource "test_resource" "x" { + value = var.input +} + +data "test_data_source" "d" { + foo = test_resource.x.id +} + +output "output" { + value = test_resource.x.id +} +`, + }) + + p := testProvider("test") + p.ReadDataSourceResponse = &providers.ReadDataSourceResponse{ + State: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("data"), + "foo": cty.StringVal("foo"), + }), + } + state := states.NewState() + modOne := addrs.RootModuleInstance.Child("one", addrs.StringKey("a")) + modTwo := addrs.RootModuleInstance.Child("two", addrs.StringKey("a")) + one := state.EnsureModule(modOne) + two := state.EnsureModule(modTwo) + one.SetResourceInstanceCurrent( + mustResourceInstanceAddr(`test_resource.x`).Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"foo","value":"a"}`), + }, + mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`), + ) + one.SetResourceInstanceCurrent( + mustResourceInstanceAddr(`data.test_data_source.d`).Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"data"}`), + }, + mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`), + ) + two.SetResourceInstanceCurrent( + mustResourceInstanceAddr(`test_resource.x`).Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"foo","value":"foo"}`), + }, + mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`), + ) + two.SetResourceInstanceCurrent( + mustResourceInstanceAddr(`data.test_data_source.d`).Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"data"}`), + }, + mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`), + ) + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + plan, diags := ctx.Plan(m, state, DefaultPlanOpts) + assertNoErrors(t, diags) + + for _, res := range plan.Changes.Resources { + // both existing data sources should be read during plan + if res.Addr.Module[0].InstanceKey == addrs.StringKey("b") { + continue + } + + if res.Addr.Resource.Resource.Mode == addrs.DataResourceMode && res.Action != plans.NoOp { + t.Errorf("unexpected %s plan for %s", res.Action, res.Addr) + } + } +}