From 5fda76e31d0e75570564f5407035859a24684359 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Sun, 5 Apr 2020 10:46:36 -0400 Subject: [PATCH 1/3] simplify module expansion eval Make the expansion logic easier to follow, keeping the evaluation and registration local to switch cases. We don't validate anything between count or for_each (config loading should handle that), and we don't need to keep relying on the count == -1 sentinel value. --- terraform/node_module_expand.go | 36 ++++++++++++--------------------- 1 file changed, 13 insertions(+), 23 deletions(-) diff --git a/terraform/node_module_expand.go b/terraform/node_module_expand.go index 8e0729fb84..dd06d1bbdb 100644 --- a/terraform/node_module_expand.go +++ b/terraform/node_module_expand.go @@ -6,7 +6,6 @@ import ( "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/configs" "github.com/hashicorp/terraform/lang" - "github.com/hashicorp/terraform/states" ) // graphNodeModuleCloser is an interface implemented by nodes that finalize the @@ -184,9 +183,7 @@ type evalPrepareModuleExpansion struct { } func (n *evalPrepareModuleExpansion) Eval(ctx EvalContext) (interface{}, error) { - eachMode := states.NoEach expander := ctx.InstanceExpander() - _, call := n.Addr.Call() // nodeExpandModule itself does not have visibility into how its ancestors @@ -194,29 +191,22 @@ func (n *evalPrepareModuleExpansion) Eval(ctx EvalContext) (interface{}, error) // to our module, and register module instances with each of them. for _, module := range expander.ExpandModule(n.Addr.Parent()) { ctx = ctx.WithPath(module) - count, countDiags := evaluateResourceCountExpression(n.ModuleCall.Count, ctx) - if countDiags.HasErrors() { - return nil, countDiags.Err() - } - - if count >= 0 { // -1 signals "count not set" - eachMode = states.EachList - } - - forEach, forEachDiags := evaluateResourceForEachExpression(n.ModuleCall.ForEach, ctx) - if forEachDiags.HasErrors() { - return nil, forEachDiags.Err() - } - if forEach != nil { - eachMode = states.EachMap - } - - switch eachMode { - case states.EachList: + switch { + case n.ModuleCall.Count != nil: + count, diags := evaluateResourceCountExpression(n.ModuleCall.Count, ctx) + if diags.HasErrors() { + return nil, diags.Err() + } expander.SetModuleCount(module, call, count) - case states.EachMap: + + case n.ModuleCall.ForEach != nil: + forEach, diags := evaluateResourceForEachExpression(n.ModuleCall.ForEach, ctx) + if diags.HasErrors() { + return nil, diags.Err() + } expander.SetModuleForEach(module, call, forEach) + default: expander.SetModuleSingle(module, call) } From 73492fd2d53dc255c4d3aac49470f7c072177d8d Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 3 Apr 2020 22:30:12 -0400 Subject: [PATCH 2/3] add module expansion to validation We cannot evaluate expansion during validation, since the values may not be known at that time. Inject a nodeValidateModule, using the "Concrete" pattern used for other node types during graph building. This node will always evaluate to a single module instance, so that we have a valid context within which to evaluate all sub resources. --- terraform/graph_builder_plan.go | 6 +++- terraform/graph_builder_validate.go | 6 ++++ terraform/node_module_expand.go | 40 +++++++++++++++++++++++++ terraform/transform_module_expansion.go | 10 +++++-- 4 files changed, 59 insertions(+), 3 deletions(-) diff --git a/terraform/graph_builder_plan.go b/terraform/graph_builder_plan.go index adce65b4fb..82974e949b 100644 --- a/terraform/graph_builder_plan.go +++ b/terraform/graph_builder_plan.go @@ -52,6 +52,7 @@ type PlanGraphBuilder struct { ConcreteProvider ConcreteProviderNodeFunc ConcreteResource ConcreteResourceNodeFunc ConcreteResourceOrphan ConcreteResourceInstanceNodeFunc + ConcreteModule ConcreteModuleNodeFunc once sync.Once } @@ -140,7 +141,10 @@ func (b *PlanGraphBuilder) Steps() []GraphTransformer { // Create expansion nodes for all of the module calls. This must // come after all other transformers that create nodes representing // objects that can belong to modules. - &ModuleExpansionTransformer{Config: b.Config}, + &ModuleExpansionTransformer{ + Concrete: b.ConcreteModule, + Config: b.Config, + }, // Connect so that the references are ready for targeting. We'll // have to connect again later for providers and so on. diff --git a/terraform/graph_builder_validate.go b/terraform/graph_builder_validate.go index 1881f95f2b..57d4c7b77b 100644 --- a/terraform/graph_builder_validate.go +++ b/terraform/graph_builder_validate.go @@ -27,6 +27,12 @@ func ValidateGraphBuilder(p *PlanGraphBuilder) GraphBuilder { } } + p.ConcreteModule = func(n *nodeExpandModule) dag.Vertex { + return &nodeValidateModule{ + nodeExpandModule: *n, + } + } + // We purposely don't set any other concrete types since they don't // require validation. diff --git a/terraform/node_module_expand.go b/terraform/node_module_expand.go index dd06d1bbdb..f779898905 100644 --- a/terraform/node_module_expand.go +++ b/terraform/node_module_expand.go @@ -5,6 +5,7 @@ import ( "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/configs" + "github.com/hashicorp/terraform/dag" "github.com/hashicorp/terraform/lang" ) @@ -14,6 +15,8 @@ type graphNodeModuleCloser interface { CloseModule() addrs.Module } +type ConcreteModuleNodeFunc func(n *nodeExpandModule) dag.Vertex + // nodeExpandModule represents a module call in the configuration that // might expand into multiple module instances depending on how it is // configured. @@ -214,3 +217,40 @@ func (n *evalPrepareModuleExpansion) Eval(ctx EvalContext) (interface{}, error) return nil, nil } + +// nodeValidateModule wraps a nodeExpand module for validation, ensuring that +// no expansion is attempted during evaluation, when count and for_each +// expressions may not be known. +type nodeValidateModule struct { + nodeExpandModule +} + +// GraphNodeEvalable +func (n *nodeValidateModule) EvalTree() EvalNode { + return &evalValidateModule{ + Addr: n.Addr, + Config: n.Config, + ModuleCall: n.ModuleCall, + } +} + +type evalValidateModule struct { + Addr addrs.Module + Config *configs.Module + ModuleCall *configs.ModuleCall +} + +func (n *evalValidateModule) Eval(ctx EvalContext) (interface{}, error) { + _, call := n.Addr.Call() + expander := ctx.InstanceExpander() + + // Modules all evaluate to single instances during validation, only to + // create a proper context within which to evaluate. All parent modules + // will be a single instance, but still get our address in the expected + // manner anyway to ensure they've been registered correctly. + for _, module := range expander.ExpandModule(n.Addr.Parent()) { + // now set our own mode to single + ctx.InstanceExpander().SetModuleSingle(module, call) + } + return nil, nil +} diff --git a/terraform/transform_module_expansion.go b/terraform/transform_module_expansion.go index 7799875e2a..090f1bfccf 100644 --- a/terraform/transform_module_expansion.go +++ b/terraform/transform_module_expansion.go @@ -16,7 +16,8 @@ import ( // This transform must be applied only after all nodes representing objects // that can be contained within modules have already been added. type ModuleExpansionTransformer struct { - Config *configs.Config + Config *configs.Config + Concrete ConcreteModuleNodeFunc } func (t *ModuleExpansionTransformer) Transform(g *Graph) error { @@ -36,11 +37,16 @@ func (t *ModuleExpansionTransformer) transform(g *Graph, c *configs.Config, pare _, call := c.Path.Call() modCall := c.Parent.Module.ModuleCalls[call.Name] - v := &nodeExpandModule{ + n := &nodeExpandModule{ Addr: c.Path, Config: c.Module, ModuleCall: modCall, } + var v dag.Vertex = n + if t.Concrete != nil { + v = t.Concrete(n) + } + g.Add(v) log.Printf("[TRACE] ModuleExpansionTransformer: Added %s as %T", c.Path, v) From e23aa02560ab6d1106e02afa8cf89147170b10e8 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Sun, 5 Apr 2020 11:17:28 -0400 Subject: [PATCH 3/3] modules expansion validate test --- terraform/context_validate_test.go | 55 +++++++++++++++++++++++++ terraform/transform_module_expansion.go | 5 ++- 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/terraform/context_validate_test.go b/terraform/context_validate_test.go index 6d27184622..a528145162 100644 --- a/terraform/context_validate_test.go +++ b/terraform/context_validate_test.go @@ -1518,3 +1518,58 @@ variable "test" { t.Fatalf("unexpected error\ngot: %s", diags.Err().Error()) } } + +func TestContext2Validate_expandModules(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ` +module "mod1" { + for_each = toset(["a", "b"]) + source = "./mod" +} + +module "mod2" { + for_each = module.mod1 + source = "./mod" +} + +module "mod3" { + count = len(module.mod2) + source = "./mod" +} +`, + "mod/main.tf": ` +resource "aws_instance" "foo" { +} + +module "nested" { + count = 2 + source = "./nested" + input = 2 +} +`, + "mod/nested/main.tf": ` +variable "input" { +} + +resource "aws_instance" "foo" { + count = var.input +} +`, + }) + + p := testProvider("aws") + p.DiffFn = testDiffFn + ctx := testContext2(t, &ContextOpts{ + Config: m, + ProviderResolver: providers.ResolverFixed( + map[addrs.Provider]providers.Factory{ + addrs.NewLegacyProvider("aws"): testProviderFuncFixed(p), + }, + ), + }) + + diags := ctx.Validate() + if diags.HasErrors() { + t.Fatal(diags.ErrWithWarnings()) + } +} diff --git a/terraform/transform_module_expansion.go b/terraform/transform_module_expansion.go index 090f1bfccf..4e688718fd 100644 --- a/terraform/transform_module_expansion.go +++ b/terraform/transform_module_expansion.go @@ -16,7 +16,10 @@ import ( // This transform must be applied only after all nodes representing objects // that can be contained within modules have already been added. type ModuleExpansionTransformer struct { - Config *configs.Config + Config *configs.Config + + // Concrete allows injection of a wrapped module node by the graph builder + // to alter the evaluation behavior. Concrete ConcreteModuleNodeFunc }