From b861f5a4d721c7f70af90c182c8e63d05fba8406 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 9 Jun 2020 21:58:58 -0400 Subject: [PATCH 1/6] unkeyed target ModulesInstance can be Modules If the last step in a module target is an unkeyed instance, and it's being compared against keyed instances, we have to assume it was intended to be used as a Module rather than a ModuleInstance. --- addrs/module_instance.go | 19 ++++++++++++++++++- addrs/target_test.go | 12 +++++------- 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/addrs/module_instance.go b/addrs/module_instance.go index 49cbf7863..75c69254a 100644 --- a/addrs/module_instance.go +++ b/addrs/module_instance.go @@ -414,9 +414,26 @@ func (m ModuleInstance) TargetContains(other Targetable) bool { } for i, ourStep := range m { otherStep := to[i] - if ourStep != otherStep { + + if ourStep.Name != otherStep.Name { return false } + + // if this is our last step, because all targets are parsed as + // instances, this may be a ModuleInstance intended to be used as a + // Module. + if i == len(m)-1 { + if ourStep.InstanceKey == NoKey { + // If the other step is a keyed instance, then we contain that + // step, and if it isn't it's a match, which is true either way + return true + } + } + + if ourStep.InstanceKey != otherStep.InstanceKey { + return false + } + } return true diff --git a/addrs/target_test.go b/addrs/target_test.go index be54f9427..69792c974 100644 --- a/addrs/target_test.go +++ b/addrs/target_test.go @@ -20,13 +20,6 @@ func TestTargetContains(t *testing.T) { mustParseTarget("module.foo"), true, }, - { - // module.foo is an unkeyed module instance here, so it cannot - // contain another instance - mustParseTarget("module.foo"), - mustParseTarget("module.foo[0]"), - false, - }, { RootModuleInstance, mustParseTarget("module.foo"), @@ -99,6 +92,11 @@ func TestTargetContains(t *testing.T) { mustParseTarget("module.bar.test_resource.foo[0]"), true, }, + { + mustParseTarget("module.bax"), + mustParseTarget("module.bax[0].test_resource.foo[0]"), + true, + }, // Config paths, while never returned from parsing a target, must still // be targetable From 7022345b8fefd1b7fb31ef4315dbcfcac2624b1e Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 10 Jun 2020 15:36:44 -0400 Subject: [PATCH 2/6] Targets was being dropped in data source nodes --- terraform/node_data_refresh.go | 1 + 1 file changed, 1 insertion(+) diff --git a/terraform/node_data_refresh.go b/terraform/node_data_refresh.go index 135bcb2a2..4f2fd902c 100644 --- a/terraform/node_data_refresh.go +++ b/terraform/node_data_refresh.go @@ -123,6 +123,7 @@ func (n *NodeRefreshableDataResource) DynamicExpand(ctx EvalContext) (*Graph, er a.ProviderMetas = n.ProviderMetas a.dependsOn = n.dependsOn a.forceDependsOn = n.forceDependsOn + a.Targets = n.Targets return &NodeRefreshableDataResourceInstance{ NodeAbstractResourceInstance: a, From aa7e6f8d86fc159283a0283ffbe37b367d59a8a7 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 10 Jun 2020 15:37:15 -0400 Subject: [PATCH 3/6] nodeCloseModule needs to be kept for downstream --- terraform/node_module_expand.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/terraform/node_module_expand.go b/terraform/node_module_expand.go index 892b6f4bd..e4f778682 100644 --- a/terraform/node_module_expand.go +++ b/terraform/node_module_expand.go @@ -145,6 +145,10 @@ func (n *nodeCloseModule) ReferenceableAddrs() []addrs.Referenceable { } } +func (n *nodeCloseModule) TargetDownstream(targeted, untargeted dag.Set) bool { + return true +} + func (n *nodeCloseModule) Name() string { if len(n.Addr) == 0 { return "root" From a2d8376eeb50c1491300159ff146a024af20de39 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 10 Jun 2020 15:38:35 -0400 Subject: [PATCH 4/6] TransformTargets cannot depends on knowing Destroy There is no reliable way to know if `destroy` was called from the cli. --- terraform/transform_targets_test.go | 52 ----------------------------- 1 file changed, 52 deletions(-) diff --git a/terraform/transform_targets_test.go b/terraform/transform_targets_test.go index 83bda3ed1..b38be835c 100644 --- a/terraform/transform_targets_test.go +++ b/terraform/transform_targets_test.go @@ -200,55 +200,3 @@ output.grandchild_id (expand) t.Fatalf("bad:\n\nexpected:\n%s\n\ngot:\n%s\n", expected, actual) } } - -func TestTargetsTransformer_destroy(t *testing.T) { - mod := testModule(t, "transform-targets-destroy") - - g := Graph{Path: addrs.RootModuleInstance} - { - tf := &ConfigTransformer{Config: mod} - if err := tf.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - } - - { - transform := &AttachResourceConfigTransformer{Config: mod} - if err := transform.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - } - - { - transform := &ReferenceTransformer{} - if err := transform.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - } - - { - transform := &TargetsTransformer{ - Targets: []addrs.Targetable{ - addrs.RootModuleInstance.Resource( - addrs.ManagedResourceMode, "aws_instance", "me", - ), - }, - Destroy: true, - } - if err := transform.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - } - - actual := strings.TrimSpace(g.String()) - expected := strings.TrimSpace(` -aws_elb.me - aws_instance.me -aws_instance.me -aws_instance.metoo - aws_instance.me - `) - if actual != expected { - t.Fatalf("bad:\n\nexpected:\n%s\n\ngot:\n%s\n", expected, actual) - } -} From 98b323d815efb87ede27e84034c482e86171edca Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 10 Jun 2020 15:39:29 -0400 Subject: [PATCH 5/6] ignore module indices in pre-expansion targeting The TargetsTransformer ignored resource indices before expansion could happen, but was not handling module indices. Ensure that we collapse all pre-expansion addresses to "configuration" addresses, with no module or resource keys. --- terraform/node_resource_apply.go | 1 + terraform/node_resource_plan.go | 1 + terraform/transform_targets.go | 21 +++++++-------------- 3 files changed, 9 insertions(+), 14 deletions(-) diff --git a/terraform/node_resource_apply.go b/terraform/node_resource_apply.go index d02dc09c8..15e57c09f 100644 --- a/terraform/node_resource_apply.go +++ b/terraform/node_resource_apply.go @@ -23,6 +23,7 @@ var ( _ GraphNodeConfigResource = (*nodeExpandApplyableResource)(nil) _ GraphNodeAttachResourceConfig = (*nodeExpandApplyableResource)(nil) _ graphNodeExpandsInstances = (*nodeExpandApplyableResource)(nil) + _ GraphNodeTargetable = (*nodeExpandApplyableResource)(nil) ) func (n *nodeExpandApplyableResource) expandsInstances() {} diff --git a/terraform/node_resource_plan.go b/terraform/node_resource_plan.go index af4de2470..db149328e 100644 --- a/terraform/node_resource_plan.go +++ b/terraform/node_resource_plan.go @@ -29,6 +29,7 @@ var ( _ GraphNodeReferencer = (*nodeExpandPlannableResource)(nil) _ GraphNodeConfigResource = (*nodeExpandPlannableResource)(nil) _ GraphNodeAttachResourceConfig = (*nodeExpandPlannableResource)(nil) + _ GraphNodeTargetable = (*nodeExpandPlannableResource)(nil) ) func (n *nodeExpandPlannableResource) Name() string { diff --git a/terraform/transform_targets.go b/terraform/transform_targets.go index a5d4ba934..437d02f15 100644 --- a/terraform/transform_targets.go +++ b/terraform/transform_targets.go @@ -43,10 +43,6 @@ type TargetsTransformer struct { // counted resources have not yet been expanded, since otherwise // the unexpanded nodes (which never have indices) would not match. IgnoreIndices bool - - // Set to true when we're in a `terraform destroy` or a - // `terraform plan -destroy` - Destroy bool } func (t *TargetsTransformer) Transform(g *Graph) error { @@ -78,7 +74,7 @@ func (t *TargetsTransformer) Transform(g *Graph) error { // Returns a set of targeted nodes. A targeted node is either addressed // directly, address indirectly via its container, or it's a dependency of a -// targeted node. Destroy mode keeps dependents instead of dependencies. +// targeted node. func (t *TargetsTransformer) selectTargetedNodes(g *Graph, addrs []addrs.Targetable) (dag.Set, error) { targetedNodes := make(dag.Set) @@ -95,13 +91,7 @@ func (t *TargetsTransformer) selectTargetedNodes(g *Graph, addrs []addrs.Targeta tn.SetTargets(addrs) } - var deps dag.Set - var err error - if t.Destroy { - deps, err = g.Descendents(v) - } else { - deps, err = g.Ancestors(v) - } + deps, err := g.Ancestors(v) if err != nil { return nil, err } @@ -239,8 +229,11 @@ func (t *TargetsTransformer) nodeIsTarget(v dag.Vertex, targets []addrs.Targetab // vertexAddr because instance addresses are contained within // their associated resources, and so .TargetContains will take // care of this for us. - if instance, isInstance := targetAddr.(addrs.AbsResourceInstance); isInstance { - targetAddr = instance.ContainingResource() + switch instance := targetAddr.(type) { + case addrs.AbsResourceInstance: + targetAddr = instance.ContainingResource().Config() + case addrs.ModuleInstance: + targetAddr = instance.Module() } } if targetAddr.TargetContains(vertexAddr) { From 13c6b83e29e267770e2c1ea8f5b5f1b93027ba72 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 10 Jun 2020 16:11:05 -0400 Subject: [PATCH 6/6] expanded module targeting test --- terraform/context_plan_test.go | 65 ++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index b7eb83619..2c2282d93 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -5882,3 +5882,68 @@ output"out" { t.Fatal(diags.ErrWithWarnings()) } } + +func TestContext2Plan_targetExpandedAddress(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ` +module "mod" { + count = 3 + source = "./mod" +} +`, + "mod/main.tf": ` +resource "aws_instance" "foo" { + count = 2 +} +`, + }) + + p := testProvider("aws") + p.DiffFn = testDiffFn + + targets := []addrs.Targetable{} + target, diags := addrs.ParseTargetStr("module.mod[1].aws_instance.foo[0]") + if diags.HasErrors() { + t.Fatal(diags.ErrWithWarnings()) + } + targets = append(targets, target.Subject) + + target, diags = addrs.ParseTargetStr("module.mod[2]") + if diags.HasErrors() { + t.Fatal(diags.ErrWithWarnings()) + } + targets = append(targets, target.Subject) + + ctx := testContext2(t, &ContextOpts{ + Config: m, + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("aws"): testProviderFuncFixed(p), + }, + Targets: targets, + }) + + plan, diags := ctx.Plan() + if diags.HasErrors() { + t.Fatal(diags.ErrWithWarnings()) + } + + expected := map[string]plans.Action{ + // the single targeted mod[1] instances + `module.mod[1].aws_instance.foo[0]`: plans.Create, + // the whole mode[2] + `module.mod[2].aws_instance.foo[0]`: plans.Create, + `module.mod[2].aws_instance.foo[1]`: plans.Create, + } + + for _, res := range plan.Changes.Resources { + want := expected[res.Addr.String()] + if res.Action != want { + t.Fatalf("expected %s action, got: %q %s", want, res.Addr, res.Action) + } + delete(expected, res.Addr.String()) + } + + for res, action := range expected { + t.Errorf("missing %s change for %s", action, res) + } +}