diff --git a/terraform/eval_count_boundary.go b/terraform/eval_count_boundary.go deleted file mode 100644 index 855f14897..000000000 --- a/terraform/eval_count_boundary.go +++ /dev/null @@ -1,76 +0,0 @@ -package terraform - -import ( - "fmt" - "log" - - "github.com/hashicorp/terraform/addrs" - "github.com/hashicorp/terraform/configs" -) - -// EvalCountFixZeroOneBoundaryGlobal is an EvalNode that fixes up the state -// when there is a resource count with zero/one boundary, i.e. fixing -// a resource named "aws_instance.foo" to "aws_instance.foo.0" and vice-versa. -// -// This works on the global state. -type EvalCountFixZeroOneBoundaryGlobal struct { - Config *configs.Config -} - -// TODO: test -func (n *EvalCountFixZeroOneBoundaryGlobal) Eval(ctx EvalContext) (interface{}, error) { - // We'll temporarily lock the state to grab the modules, then work on each - // one separately while taking a lock again for each separate resource. - // This means that if another caller concurrently adds a module here while - // we're working then we won't update it, but that's no worse than the - // concurrent writer blocking for our entire fixup process and _then_ - // adding a new module, and in practice the graph node associated with - // this eval depends on everything else in the graph anyway, so there - // should not be concurrent writers. - state := ctx.State().Lock() - moduleAddrs := make([]addrs.ModuleInstance, 0, len(state.Modules)) - for _, m := range state.Modules { - moduleAddrs = append(moduleAddrs, m.Addr) - } - ctx.State().Unlock() - - for _, addr := range moduleAddrs { - cfg := n.Config.DescendentForInstance(addr) - if cfg == nil { - log.Printf("[WARN] Not fixing up EachModes for %s because it has no config", addr) - continue - } - if err := n.fixModule(ctx, addr); err != nil { - return nil, err - } - } - - return nil, nil -} - -func (n *EvalCountFixZeroOneBoundaryGlobal) fixModule(ctx EvalContext, moduleAddr addrs.ModuleInstance) error { - ms := ctx.State().Module(moduleAddr) - cfg := n.Config.DescendentForInstance(moduleAddr) - if ms == nil { - // Theoretically possible for a concurrent writer to delete a module - // while we're running, but in practice the graph node that called us - // depends on everything else in the graph and so there can never - // be a concurrent writer. - return fmt.Errorf("[WARN] no state found for %s while trying to fix up EachModes", moduleAddr) - } - if cfg == nil { - return fmt.Errorf("[WARN] no config found for %s while trying to fix up EachModes", moduleAddr) - } - - for _, r := range ms.Resources { - rCfg := cfg.Module.ResourceByAddr(r.Addr.Resource) - if rCfg == nil { - log.Printf("[WARN] Not fixing up EachModes for %s because it has no config", r.Addr) - continue - } - hasCount := rCfg.Count != nil - fixResourceCountSetTransition(ctx, r.Addr.Config(), hasCount) - } - - return nil -} diff --git a/terraform/eval_local.go b/terraform/eval_local.go deleted file mode 100644 index f30286e2f..000000000 --- a/terraform/eval_local.go +++ /dev/null @@ -1,74 +0,0 @@ -package terraform - -import ( - "fmt" - - "github.com/hashicorp/hcl/v2" - "github.com/zclconf/go-cty/cty" - - "github.com/hashicorp/terraform/addrs" - "github.com/hashicorp/terraform/lang" - "github.com/hashicorp/terraform/tfdiags" -) - -// EvalLocal is an EvalNode implementation that evaluates the -// expression for a local value and writes it into a transient part of -// the state. -type EvalLocal struct { - Addr addrs.LocalValue - Expr hcl.Expression -} - -func (n *EvalLocal) Eval(ctx EvalContext) (interface{}, error) { - var diags tfdiags.Diagnostics - - // We ignore diags here because any problems we might find will be found - // again in EvaluateExpr below. - refs, _ := lang.ReferencesInExpr(n.Expr) - for _, ref := range refs { - if ref.Subject == n.Addr { - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Self-referencing local value", - Detail: fmt.Sprintf("Local value %s cannot use its own result as part of its expression.", n.Addr), - Subject: ref.SourceRange.ToHCL().Ptr(), - Context: n.Expr.Range().Ptr(), - }) - } - } - if diags.HasErrors() { - return nil, diags.Err() - } - - val, moreDiags := ctx.EvaluateExpr(n.Expr, cty.DynamicPseudoType, nil) - diags = diags.Append(moreDiags) - if moreDiags.HasErrors() { - return nil, diags.Err() - } - - state := ctx.State() - if state == nil { - return nil, fmt.Errorf("cannot write local value to nil state") - } - - state.SetLocalValue(n.Addr.Absolute(ctx.Path()), val) - - return nil, nil -} - -// EvalDeleteLocal is an EvalNode implementation that deletes a Local value -// from the state. Locals aren't persisted, but we don't need to evaluate them -// during destroy. -type EvalDeleteLocal struct { - Addr addrs.LocalValue -} - -func (n *EvalDeleteLocal) Eval(ctx EvalContext) (interface{}, error) { - state := ctx.State() - if state == nil { - return nil, nil - } - - state.RemoveLocalValue(n.Addr.Absolute(ctx.Path())) - return nil, nil -} diff --git a/terraform/node_count_boundary.go b/terraform/node_count_boundary.go index e4952039c..bfdbd1efa 100644 --- a/terraform/node_count_boundary.go +++ b/terraform/node_count_boundary.go @@ -1,6 +1,10 @@ package terraform import ( + "fmt" + "log" + + "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/configs" ) @@ -14,9 +18,59 @@ func (n *NodeCountBoundary) Name() string { return "meta.count-boundary (EachMode fixup)" } -// GraphNodeEvalable -func (n *NodeCountBoundary) EvalTree() EvalNode { - return &EvalCountFixZeroOneBoundaryGlobal{ - Config: n.Config, +// GraphNodeExecutable +func (n *NodeCountBoundary) Execute(ctx EvalContext, op walkOperation) error { + // We'll temporarily lock the state to grab the modules, then work on each + // one separately while taking a lock again for each separate resource. + // This means that if another caller concurrently adds a module here while + // we're working then we won't update it, but that's no worse than the + // concurrent writer blocking for our entire fixup process and _then_ + // adding a new module, and in practice the graph node associated with + // this eval depends on everything else in the graph anyway, so there + // should not be concurrent writers. + state := ctx.State().Lock() + moduleAddrs := make([]addrs.ModuleInstance, 0, len(state.Modules)) + for _, m := range state.Modules { + moduleAddrs = append(moduleAddrs, m.Addr) } + ctx.State().Unlock() + + for _, addr := range moduleAddrs { + cfg := n.Config.DescendentForInstance(addr) + if cfg == nil { + log.Printf("[WARN] Not fixing up EachModes for %s because it has no config", addr) + continue + } + if err := n.fixModule(ctx, addr); err != nil { + return err + } + } + return nil +} + +func (n *NodeCountBoundary) fixModule(ctx EvalContext, moduleAddr addrs.ModuleInstance) error { + ms := ctx.State().Module(moduleAddr) + cfg := n.Config.DescendentForInstance(moduleAddr) + if ms == nil { + // Theoretically possible for a concurrent writer to delete a module + // while we're running, but in practice the graph node that called us + // depends on everything else in the graph and so there can never + // be a concurrent writer. + return fmt.Errorf("[WARN] no state found for %s while trying to fix up EachModes", moduleAddr) + } + if cfg == nil { + return fmt.Errorf("[WARN] no config found for %s while trying to fix up EachModes", moduleAddr) + } + + for _, r := range ms.Resources { + rCfg := cfg.Module.ResourceByAddr(r.Addr.Resource) + if rCfg == nil { + log.Printf("[WARN] Not fixing up EachModes for %s because it has no config", r.Addr) + continue + } + hasCount := rCfg.Count != nil + fixResourceCountSetTransition(ctx, r.Addr.Config(), hasCount) + } + + return nil } diff --git a/terraform/node_count_boundary_test.go b/terraform/node_count_boundary_test.go new file mode 100644 index 000000000..13c5fac29 --- /dev/null +++ b/terraform/node_count_boundary_test.go @@ -0,0 +1,72 @@ +package terraform + +import ( + "testing" + + "github.com/hashicorp/hcl/v2/hcltest" + "github.com/hashicorp/terraform/addrs" + "github.com/hashicorp/terraform/configs" + "github.com/hashicorp/terraform/states" + "github.com/zclconf/go-cty/cty" +) + +func TestNodeCountBoundaryExecute(t *testing.T) { + + // Create a state with a single instance (addrs.NoKey) of test_instance.foo + state := states.NewState() + state.Module(addrs.RootModuleInstance).SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_instance", + Name: "foo", + }.Instance(addrs.NoKey), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"type":"string","value":"hello"}`), + }, + addrs.AbsProviderConfig{ + Provider: addrs.NewDefaultProvider("test"), + Module: addrs.RootModule, + }, + ) + + // Create a config that uses count to create 2 instances of test_instance.foo + rc := &configs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_instance", + Name: "foo", + Count: hcltest.MockExprLiteral(cty.NumberIntVal(2)), + Config: configs.SynthBody("", map[string]cty.Value{ + "test_string": cty.StringVal("hello"), + }), + } + config := &configs.Config{ + Module: &configs.Module{ + ManagedResources: map[string]*configs.Resource{ + "test_instance.foo": rc, + }, + }, + } + + ctx := &MockEvalContext{ + StateState: state.SyncWrapper(), + } + node := NodeCountBoundary{Config: config} + + err := node.Execute(ctx, walkApply) + if err != nil { + t.Fatalf("unexpected error: %s", err.Error()) + } + if !state.HasResources() { + t.Fatal("resources missing from state") + } + + // verify that the resource changed from test_instance.foo to + // test_instance.foo.0 in the state + actual := state.String() + expected := "test_instance.foo.0:\n ID = \n provider = provider[\"registry.terraform.io/hashicorp/test\"]\n type = string\n value = hello" + + if actual != expected { + t.Fatalf("wrong result: %s", actual) + } +} diff --git a/terraform/node_local.go b/terraform/node_local.go index 8924d5336..055492fe8 100644 --- a/terraform/node_local.go +++ b/terraform/node_local.go @@ -1,12 +1,16 @@ package terraform import ( + "fmt" "log" + "github.com/hashicorp/hcl/v2" "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/configs" "github.com/hashicorp/terraform/dag" "github.com/hashicorp/terraform/lang" + "github.com/hashicorp/terraform/tfdiags" + "github.com/zclconf/go-cty/cty" ) // nodeExpandLocal represents a named local value in a configuration module, @@ -85,7 +89,7 @@ var ( _ GraphNodeModuleInstance = (*NodeLocal)(nil) _ GraphNodeReferenceable = (*NodeLocal)(nil) _ GraphNodeReferencer = (*NodeLocal)(nil) - _ GraphNodeEvalable = (*NodeLocal)(nil) + _ GraphNodeExecutable = (*NodeLocal)(nil) _ graphNodeTemporaryValue = (*NodeLocal)(nil) _ dag.GraphNodeDotter = (*NodeLocal)(nil) ) @@ -120,12 +124,49 @@ func (n *NodeLocal) References() []*addrs.Reference { return refs } -// GraphNodeEvalable -func (n *NodeLocal) EvalTree() EvalNode { - return &EvalLocal{ - Addr: n.Addr.LocalValue, - Expr: n.Config.Expr, +// GraphNodeExecutable +// NodeLocal.Execute is an Execute implementation that evaluates the +// expression for a local value and writes it into a transient part of +// the state. +func (n *NodeLocal) Execute(ctx EvalContext, op walkOperation) error { + + var diags tfdiags.Diagnostics + + expr := n.Config.Expr + addr := n.Addr.LocalValue + + // We ignore diags here because any problems we might find will be found + // again in EvaluateExpr below. + refs, _ := lang.ReferencesInExpr(expr) + for _, ref := range refs { + if ref.Subject == addr { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Self-referencing local value", + Detail: fmt.Sprintf("Local value %s cannot use its own result as part of its expression.", addr), + Subject: ref.SourceRange.ToHCL().Ptr(), + Context: expr.Range().Ptr(), + }) + } } + if diags.HasErrors() { + return diags.Err() + } + + val, moreDiags := ctx.EvaluateExpr(expr, cty.DynamicPseudoType, nil) + diags = diags.Append(moreDiags) + if moreDiags.HasErrors() { + return diags.Err() + } + + state := ctx.State() + if state == nil { + return fmt.Errorf("cannot write local value to nil state") + } + + state.SetLocalValue(addr.Absolute(ctx.Path()), val) + + return nil } // dag.GraphNodeDotter impl. diff --git a/terraform/eval_local_test.go b/terraform/node_local_test.go similarity index 85% rename from terraform/eval_local_test.go rename to terraform/node_local_test.go index a76a763ba..d53f49e3e 100644 --- a/terraform/eval_local_test.go +++ b/terraform/node_local_test.go @@ -10,15 +10,12 @@ import ( "github.com/zclconf/go-cty/cty" "github.com/hashicorp/terraform/addrs" + "github.com/hashicorp/terraform/configs" "github.com/hashicorp/terraform/configs/hcl2shim" "github.com/hashicorp/terraform/states" ) -func TestEvalLocal_impl(t *testing.T) { - var _ EvalNode = new(EvalLocal) -} - -func TestEvalLocal(t *testing.T) { +func TestNodeLocalExecute(t *testing.T) { tests := []struct { Value string Want interface{} @@ -48,9 +45,11 @@ func TestEvalLocal(t *testing.T) { t.Fatal(diags.Error()) } - n := &EvalLocal{ - Addr: addrs.LocalValue{Name: "foo"}, - Expr: expr, + n := &NodeLocal{ + Addr: addrs.LocalValue{Name: "foo"}.Absolute(addrs.RootModuleInstance), + Config: &configs.Local{ + Expr: expr, + }, } ctx := &MockEvalContext{ StateState: states.NewState().SyncWrapper(), @@ -58,7 +57,7 @@ func TestEvalLocal(t *testing.T) { EvaluateExprResult: hcl2shim.HCL2ValueFromConfigValue(test.Want), } - _, err := n.Eval(ctx) + err := n.Execute(ctx, walkApply) if (err != nil) != test.Err { if err != nil { t.Errorf("unexpected error: %s", err) diff --git a/terraform/node_module_expand.go b/terraform/node_module_expand.go index be65768d5..1e27fd274 100644 --- a/terraform/node_module_expand.go +++ b/terraform/node_module_expand.go @@ -22,7 +22,7 @@ type nodeExpandModule struct { } var ( - _ GraphNodeEvalable = (*nodeExpandModule)(nil) + _ GraphNodeExecutable = (*nodeExpandModule)(nil) _ GraphNodeReferencer = (*nodeExpandModule)(nil) _ GraphNodeReferenceOutside = (*nodeExpandModule)(nil) _ graphNodeExpandsInstances = (*nodeExpandModule)(nil) @@ -98,13 +98,38 @@ func (n *nodeExpandModule) ReferenceOutside() (selfPath, referencePath addrs.Mod return n.Addr, n.Addr.Parent() } -// GraphNodeEvalable -func (n *nodeExpandModule) EvalTree() EvalNode { - return &evalPrepareModuleExpansion{ - Addr: n.Addr, - Config: n.Config, - ModuleCall: n.ModuleCall, +// GraphNodeExecutable +func (n *nodeExpandModule) Execute(ctx EvalContext, op walkOperation) error { + expander := ctx.InstanceExpander() + _, call := n.Addr.Call() + + // nodeExpandModule itself does not have visibility into how its ancestors + // were expanded, so we use the expander here to provide all possible paths + // to our module, and register module instances with each of them. + for _, module := range expander.ExpandModule(n.Addr.Parent()) { + ctx = ctx.WithPath(module) + switch { + case n.ModuleCall.Count != nil: + count, diags := evaluateCountExpression(n.ModuleCall.Count, ctx) + if diags.HasErrors() { + return diags.Err() + } + expander.SetModuleCount(module, call, count) + + case n.ModuleCall.ForEach != nil: + forEach, diags := evaluateForEachExpression(n.ModuleCall.ForEach, ctx) + if diags.HasErrors() { + return diags.Err() + } + expander.SetModuleForEach(module, call, forEach) + + default: + expander.SetModuleSingle(module, call) + } } + + return nil + } // nodeCloseModule represents an expanded module during apply, and is visited @@ -145,88 +170,33 @@ func (n *nodeCloseModule) Name() string { return n.Addr.String() + " (close)" } -func (n *nodeCloseModule) EvalTree() EvalNode { - return &EvalSequence{ - Nodes: []EvalNode{ - &EvalOpFilter{ - Ops: []walkOperation{walkApply, walkDestroy}, - Node: &evalCloseModule{ - Addr: n.Addr, - }, - }, - }, - } -} +func (n *nodeCloseModule) Execute(ctx EvalContext, op walkOperation) error { + switch op { + case walkApply, walkDestroy: + state := ctx.State().Lock() + defer ctx.State().Unlock() -type evalCloseModule struct { - Addr addrs.Module -} + for modKey, mod := range state.Modules { + if !n.Addr.Equal(mod.Addr.Module()) { + continue + } -func (n *evalCloseModule) Eval(ctx EvalContext) (interface{}, error) { - // We need the full, locked state, because SyncState does not provide a way to - // transact over multiple module instances at the moment. - state := ctx.State().Lock() - defer ctx.State().Unlock() + // clean out any empty resources + for resKey, res := range mod.Resources { + if len(res.Instances) == 0 { + delete(mod.Resources, resKey) + } + } - for modKey, mod := range state.Modules { - if !n.Addr.Equal(mod.Addr.Module()) { - continue - } - - // clean out any empty resources - for resKey, res := range mod.Resources { - if len(res.Instances) == 0 { - delete(mod.Resources, resKey) + // empty child modules are always removed + if len(mod.Resources) == 0 && !mod.Addr.IsRoot() { + delete(state.Modules, modKey) } } - - // empty child modules are always removed - if len(mod.Resources) == 0 && !mod.Addr.IsRoot() { - delete(state.Modules, modKey) - } + return nil + default: + return nil } - return nil, nil -} - -// evalPrepareModuleExpansion is an EvalNode implementation -// that sets the count or for_each on the instance expander -type evalPrepareModuleExpansion struct { - Addr addrs.Module - Config *configs.Module - ModuleCall *configs.ModuleCall -} - -func (n *evalPrepareModuleExpansion) Eval(ctx EvalContext) (interface{}, error) { - expander := ctx.InstanceExpander() - _, call := n.Addr.Call() - - // nodeExpandModule itself does not have visibility into how its ancestors - // were expanded, so we use the expander here to provide all possible paths - // to our module, and register module instances with each of them. - for _, module := range expander.ExpandModule(n.Addr.Parent()) { - ctx = ctx.WithPath(module) - - switch { - case n.ModuleCall.Count != nil: - count, diags := evaluateCountExpression(n.ModuleCall.Count, ctx) - if diags.HasErrors() { - return nil, diags.Err() - } - expander.SetModuleCount(module, call, count) - - case n.ModuleCall.ForEach != nil: - forEach, diags := evaluateForEachExpression(n.ModuleCall.ForEach, ctx) - if diags.HasErrors() { - return nil, diags.Err() - } - expander.SetModuleForEach(module, call, forEach) - - default: - expander.SetModuleSingle(module, call) - } - } - - return nil, nil } // nodeValidateModule wraps a nodeExpand module for validation, ensuring that @@ -237,21 +207,7 @@ type nodeValidateModule struct { } // 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) { +func (n *nodeValidateModule) Execute(ctx EvalContext, op walkOperation) error { _, call := n.Addr.Call() var diags tfdiags.Diagnostics expander := ctx.InstanceExpander() @@ -283,8 +239,8 @@ func (n *evalValidateModule) Eval(ctx EvalContext) (interface{}, error) { } if diags.HasErrors() { - return nil, diags.ErrWithWarnings() + return diags.ErrWithWarnings() } - return nil, nil + return nil } diff --git a/terraform/node_module_expand_test.go b/terraform/node_module_expand_test.go new file mode 100644 index 000000000..578b4276f --- /dev/null +++ b/terraform/node_module_expand_test.go @@ -0,0 +1,116 @@ +package terraform + +import ( + "testing" + + "github.com/hashicorp/hcl/v2/hcltest" + "github.com/hashicorp/terraform/addrs" + "github.com/hashicorp/terraform/configs" + "github.com/hashicorp/terraform/instances" + "github.com/hashicorp/terraform/states" + "github.com/zclconf/go-cty/cty" +) + +func TestNodeExpandModuleExecute(t *testing.T) { + ctx := &MockEvalContext{ + InstanceExpanderExpander: instances.NewExpander(), + } + ctx.installSimpleEval() + + node := nodeExpandModule{ + Addr: addrs.Module{"child"}, + ModuleCall: &configs.ModuleCall{ + Count: hcltest.MockExprLiteral(cty.NumberIntVal(2)), + }, + } + + err := node.Execute(ctx, walkApply) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + + if !ctx.InstanceExpanderCalled { + t.Fatal("did not expand") + } +} + +func TestNodeCloseModuleExecute(t *testing.T) { + t.Run("walkApply", func(t *testing.T) { + state := states.NewState() + state.EnsureModule(addrs.RootModuleInstance.Child("child", addrs.NoKey)) + ctx := &MockEvalContext{ + StateState: state.SyncWrapper(), + } + node := nodeCloseModule{addrs.Module{"child"}} + err := node.Execute(ctx, walkApply) + if err != nil { + t.Fatalf("unexpected error: %s", err.Error()) + } + + // Since module.child has no resources, it should be removed + if _, ok := state.Modules["module.child"]; ok { + t.Fatal("module.child was not removed from state") + } + }) + + // walkImport is a no-op + t.Run("walkImport", func(t *testing.T) { + state := states.NewState() + state.EnsureModule(addrs.RootModuleInstance.Child("child", addrs.NoKey)) + ctx := &MockEvalContext{ + StateState: state.SyncWrapper(), + } + node := nodeCloseModule{addrs.Module{"child"}} + + err := node.Execute(ctx, walkImport) + if err != nil { + t.Fatalf("unexpected error: %s", err.Error()) + } + if _, ok := state.Modules["module.child"]; !ok { + t.Fatal("module.child was removed from state, expected no-op") + } + }) +} + +func TestNodeValidateModuleExecute(t *testing.T) { + t.Run("success", func(t *testing.T) { + ctx := &MockEvalContext{ + InstanceExpanderExpander: instances.NewExpander(), + } + ctx.installSimpleEval() + node := nodeValidateModule{ + nodeExpandModule{ + Addr: addrs.Module{"child"}, + ModuleCall: &configs.ModuleCall{ + Count: hcltest.MockExprLiteral(cty.NumberIntVal(2)), + }, + }, + } + + err := node.Execute(ctx, walkApply) + if err != nil { + t.Fatalf("unexpected error: %s", err.Error()) + } + }) + + t.Run("invalid count", func(t *testing.T) { + ctx := &MockEvalContext{ + InstanceExpanderExpander: instances.NewExpander(), + } + ctx.installSimpleEval() + node := nodeValidateModule{ + nodeExpandModule{ + Addr: addrs.Module{"child"}, + ModuleCall: &configs.ModuleCall{ + Count: hcltest.MockExprLiteral(cty.StringVal("invalid")), + }, + }, + } + + err := node.Execute(ctx, walkApply) + if err == nil { + t.Fatal("expected error, got success") + } + }) + +}