From 0b025d74e50cf15c117f492c85fa1334d94955c7 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 17 Mar 2020 14:02:46 -0400 Subject: [PATCH 01/15] add EvalContext.WithPath As the Graph is walked, the current way to set the context path was to have the walker return a context from EnterPath. This required that every node know it's absolute path, which can no longer be the case during plan when modules have not been expanded. This introduces a new method called WithPath, which returns a copy of the context with the internal path updated to reflect the method argument. Any use of the EvalContext that requires knowing the path will now panic if it wasn't explicitly set to ensure that evaluations always occur in the correct path. Add EvalContext to the GraphWalker interface. EvalContext returns an EvalContext that has not yet set a path. This will allow us to enforce that all context operations requiring a module instance path will require that a path be explicitly set rather than evaluating within the wrong path. --- terraform/eval.go | 16 ++++++++++------ terraform/eval_context.go | 4 ++++ terraform/eval_context_builtin.go | 24 ++++++++++++++++++++++++ terraform/eval_context_mock.go | 6 ++++++ terraform/graph.go | 5 ++--- terraform/graph_walk.go | 2 ++ terraform/graph_walk_context.go | 12 ++++++++---- 7 files changed, 56 insertions(+), 13 deletions(-) diff --git a/terraform/eval.go b/terraform/eval.go index 48ed3533ac15..51e89d8b5d23 100644 --- a/terraform/eval.go +++ b/terraform/eval.go @@ -46,12 +46,16 @@ func Eval(n EvalNode, ctx EvalContext) (interface{}, error) { // signal something normal such as EvalEarlyExitError. func EvalRaw(n EvalNode, ctx EvalContext) (interface{}, error) { path := "unknown" - if ctx != nil { - path = ctx.Path().String() - } - if path == "" { - path = "" - } + + // FIXME: restore the path here somehow or log this in another manner + // We cannot call Path here, since the context may not yet have the path + // set. + //if ctx != nil { + // path = ctx.Path().String() + //} + //if path == "" { + // path = "" + //} log.Printf("[TRACE] %s: eval: %T", path, n) output, err := n.Eval(ctx) diff --git a/terraform/eval_context.go b/terraform/eval_context.go index a682b3d6c41f..241662f7baf2 100644 --- a/terraform/eval_context.go +++ b/terraform/eval_context.go @@ -161,4 +161,8 @@ type EvalContext interface { // The InstanceExpander is a global object that is shared across all of the // EvalContext objects for a given configuration. InstanceExpander() *instances.Expander + + // WithPath returns a copy of the context with the internal path set to the + // path argument. + WithPath(path addrs.ModuleInstance) EvalContext } diff --git a/terraform/eval_context_builtin.go b/terraform/eval_context_builtin.go index 746b00fe142a..b5e400be4779 100644 --- a/terraform/eval_context_builtin.go +++ b/terraform/eval_context_builtin.go @@ -32,6 +32,13 @@ type BuiltinEvalContext struct { // PathValue is the Path that this context is operating within. PathValue addrs.ModuleInstance + // pathSet indicates that this context was explicitly created for a + // specific path, and can be safely used for evaluation. This lets us + // differentiate between Pathvalue being unset, and the zero value which is + // equivalent to RootModuleInstance. Path and Evaluation methods will + // panic if this is not set. + pathSet bool + // Evaluator is used for evaluating expressions within the scope of this // eval context. Evaluator *Evaluator @@ -70,6 +77,13 @@ type BuiltinEvalContext struct { // BuiltinEvalContext implements EvalContext var _ EvalContext = (*BuiltinEvalContext)(nil) +func (ctx *BuiltinEvalContext) WithPath(path addrs.ModuleInstance) EvalContext { + ctx.pathSet = true + newCtx := *ctx + newCtx.PathValue = path + return &newCtx +} + func (ctx *BuiltinEvalContext) Stopped() <-chan struct{} { // This can happen during tests. During tests, we just block forever. if ctx.StopContext == nil { @@ -291,6 +305,9 @@ func (ctx *BuiltinEvalContext) EvaluateExpr(expr hcl.Expression, wantType cty.Ty } func (ctx *BuiltinEvalContext) EvaluationScope(self addrs.Referenceable, keyData InstanceKeyEvalData) *lang.Scope { + if !ctx.pathSet { + panic("context path not set") + } data := &evaluationStateData{ Evaluator: ctx.Evaluator, ModulePath: ctx.PathValue, @@ -301,6 +318,9 @@ func (ctx *BuiltinEvalContext) EvaluationScope(self addrs.Referenceable, keyData } func (ctx *BuiltinEvalContext) Path() addrs.ModuleInstance { + if !ctx.pathSet { + panic("context path not set") + } return ctx.PathValue } @@ -308,6 +328,10 @@ func (ctx *BuiltinEvalContext) SetModuleCallArguments(n addrs.ModuleCallInstance ctx.VariableValuesLock.Lock() defer ctx.VariableValuesLock.Unlock() + if !ctx.pathSet { + panic("context path not set") + } + childPath := n.ModuleInstance(ctx.PathValue) key := childPath.String() diff --git a/terraform/eval_context_mock.go b/terraform/eval_context_mock.go index 210a40d801d1..15bf80569e5f 100644 --- a/terraform/eval_context_mock.go +++ b/terraform/eval_context_mock.go @@ -305,6 +305,12 @@ func (c *MockEvalContext) EvaluationScope(self addrs.Referenceable, keyData Inst return c.EvaluationScopeScope } +func (c *MockEvalContext) WithPath(path addrs.ModuleInstance) EvalContext { + newC := *c + newC.PathPath = path + return &newC +} + func (c *MockEvalContext) Path() addrs.ModuleInstance { c.PathCalled = true return c.PathPath diff --git a/terraform/graph.go b/terraform/graph.go index 373819f7eb7a..4c9f2f0c20a1 100644 --- a/terraform/graph.go +++ b/terraform/graph.go @@ -35,8 +35,7 @@ func (g *Graph) Walk(walker GraphWalker) tfdiags.Diagnostics { func (g *Graph) walk(walker GraphWalker) tfdiags.Diagnostics { // The callbacks for enter/exiting a graph - ctx := walker.EnterPath(g.Path) - defer walker.ExitPath(g.Path) + ctx := walker.EvalContext() // Walk the graph. var walkFn dag.WalkFunc @@ -54,7 +53,7 @@ func (g *Graph) walk(walker GraphWalker) tfdiags.Diagnostics { // is normally the context of our graph but can be overridden // with a GraphNodeModuleInstance impl. vertexCtx := ctx - if pn, ok := v.(GraphNodeModuleInstance); ok && len(pn.Path()) > 0 { + if pn, ok := v.(GraphNodeModuleInstance); ok { vertexCtx = walker.EnterPath(pn.Path()) defer walker.ExitPath(pn.Path()) } diff --git a/terraform/graph_walk.go b/terraform/graph_walk.go index e980e0c6d3c6..706b7e0ab061 100644 --- a/terraform/graph_walk.go +++ b/terraform/graph_walk.go @@ -9,6 +9,7 @@ import ( // GraphWalker is an interface that can be implemented that when used // with Graph.Walk will invoke the given callbacks under certain events. type GraphWalker interface { + EvalContext() EvalContext EnterPath(addrs.ModuleInstance) EvalContext ExitPath(addrs.ModuleInstance) EnterVertex(dag.Vertex) @@ -22,6 +23,7 @@ type GraphWalker interface { // implementing all the required functions. type NullGraphWalker struct{} +func (NullGraphWalker) EvalContext() EvalContext { return new(MockEvalContext) } func (NullGraphWalker) EnterPath(addrs.ModuleInstance) EvalContext { return new(MockEvalContext) } func (NullGraphWalker) ExitPath(addrs.ModuleInstance) {} func (NullGraphWalker) EnterVertex(dag.Vertex) {} diff --git a/terraform/graph_walk_context.go b/terraform/graph_walk_context.go index d53ebe97aa65..5025c98b6649 100644 --- a/terraform/graph_walk_context.go +++ b/terraform/graph_walk_context.go @@ -51,8 +51,6 @@ type ContextGraphWalker struct { } func (w *ContextGraphWalker) EnterPath(path addrs.ModuleInstance) EvalContext { - w.once.Do(w.init) - w.contextLock.Lock() defer w.contextLock.Unlock() @@ -62,6 +60,14 @@ func (w *ContextGraphWalker) EnterPath(path addrs.ModuleInstance) EvalContext { return ctx } + ctx := w.EvalContext().WithPath(path) + w.contexts[key] = ctx.(*BuiltinEvalContext) + return ctx +} + +func (w *ContextGraphWalker) EvalContext() EvalContext { + w.once.Do(w.init) + // Our evaluator shares some locks with the main context and the walker // so that we can safely run multiple evaluations at once across // different modules. @@ -78,7 +84,6 @@ func (w *ContextGraphWalker) EnterPath(path addrs.ModuleInstance) EvalContext { ctx := &BuiltinEvalContext{ StopContext: w.StopContext, - PathValue: path, Hooks: w.Context.hooks, InputValue: w.Context.uiInput, InstanceExpanderValue: w.InstanceExpander, @@ -96,7 +101,6 @@ func (w *ContextGraphWalker) EnterPath(path addrs.ModuleInstance) EvalContext { VariableValuesLock: &w.variableValuesLock, } - w.contexts[key] = ctx return ctx } From 40f09027f02462f5e2fd4be9fe5f7246c9530e3f Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 20 Mar 2020 15:19:01 -0400 Subject: [PATCH 02/15] expand planned resources While the Expander itself now handles the recursive expansion of modules, Resources themselves still need to be expanded twice, because the evaluation of the Resource, which entails evaluating the for_each or count expressions, is separate from the ResourceInstance expansion. Add a nodeExpandPlannableResource to do handle this expansion to allow all NodePlannableResources to call EvalWriteResourceState with an absolute address. --- terraform/eval_context_builtin_test.go | 5 +- terraform/eval_output.go | 4 +- terraform/eval_state.go | 38 ++++++------- terraform/graph_builder_plan.go | 2 +- terraform/node_output.go | 5 +- terraform/node_output_orphan.go | 2 +- terraform/node_resource_apply.go | 2 +- terraform/node_resource_plan.go | 75 +++++++++++++++++++++++++- terraform/transform_output.go | 3 +- 9 files changed, 102 insertions(+), 34 deletions(-) diff --git a/terraform/eval_context_builtin_test.go b/terraform/eval_context_builtin_test.go index bdcd0948465b..c6d27d498047 100644 --- a/terraform/eval_context_builtin_test.go +++ b/terraform/eval_context_builtin_test.go @@ -15,12 +15,12 @@ func TestBuiltinEvalContextProviderInput(t *testing.T) { cache := make(map[string]map[string]cty.Value) ctx1 := testBuiltinEvalContext(t) - ctx1.PathValue = addrs.RootModuleInstance + ctx1 = ctx1.WithPath(addrs.RootModuleInstance).(*BuiltinEvalContext) ctx1.ProviderInputConfig = cache ctx1.ProviderLock = &lock ctx2 := testBuiltinEvalContext(t) - ctx2.PathValue = addrs.RootModuleInstance.Child("child", addrs.NoKey) + ctx2 = ctx2.WithPath(addrs.RootModuleInstance.Child("child", addrs.NoKey)).(*BuiltinEvalContext) ctx2.ProviderInputConfig = cache ctx2.ProviderLock = &lock @@ -56,6 +56,7 @@ func TestBuildingEvalContextInitProvider(t *testing.T) { testP := &MockProvider{} ctx := testBuiltinEvalContext(t) + ctx = ctx.WithPath(addrs.RootModuleInstance).(*BuiltinEvalContext) ctx.ProviderLock = &lock ctx.ProviderCache = make(map[string]providers.Interface) ctx.Components = &basicComponentFactory{ diff --git a/terraform/eval_output.go b/terraform/eval_output.go index 181e55635d8a..f1a195f6d5b6 100644 --- a/terraform/eval_output.go +++ b/terraform/eval_output.go @@ -15,7 +15,7 @@ import ( // EvalDeleteOutput is an EvalNode implementation that deletes an output // from the state. type EvalDeleteOutput struct { - Addr addrs.OutputValue + Addr addrs.AbsOutputValue } // TODO: test @@ -25,7 +25,7 @@ func (n *EvalDeleteOutput) Eval(ctx EvalContext) (interface{}, error) { return nil, nil } - state.RemoveOutputValue(n.Addr.Absolute(ctx.Path())) + state.RemoveOutputValue(n.Addr) return nil, nil } diff --git a/terraform/eval_state.go b/terraform/eval_state.go index 8f134d4655ec..16655080e9b3 100644 --- a/terraform/eval_state.go +++ b/terraform/eval_state.go @@ -452,16 +452,20 @@ func (n *EvalMaybeRestoreDeposedObject) Eval(ctx EvalContext) (interface{}, erro // in that case, allowing expression evaluation to see it as a zero-element // list rather than as not set at all. type EvalWriteResourceState struct { - Addr addrs.ConfigResource + Addr addrs.AbsResource Config *configs.Resource ProviderAddr addrs.AbsProviderConfig } -// TODO: test func (n *EvalWriteResourceState) Eval(ctx EvalContext) (interface{}, error) { var diags tfdiags.Diagnostics state := ctx.State() + // We'll record our expansion decision in the shared "expander" object + // so that later operations (i.e. DynamicExpand and expression evaluation) + // can refer to it. Since this node represents the abstract module, we need + // to expand the module here to create all resources. + expander := ctx.InstanceExpander() count, countDiags := evaluateResourceCountExpression(n.Config.Count, ctx) diags = diags.Append(countDiags) if countDiags.HasErrors() { @@ -482,25 +486,17 @@ func (n *EvalWriteResourceState) Eval(ctx EvalContext) (interface{}, error) { if forEach != nil { eachMode = states.EachMap } - - // We'll record our expansion decision in the shared "expander" object - // so that later operations (i.e. DynamicExpand and expression evaluation) - // can refer to it. Since this node represents the abstract module, we need - // to expand the module here to create all resources. - expander := ctx.InstanceExpander() - for _, module := range expander.ExpandModule(n.Addr.Module) { - // This method takes care of all of the business logic of updating this - // while ensuring that any existing instances are preserved, etc. - state.SetResourceMeta(n.Addr.Absolute(module), eachMode, n.ProviderAddr) - - switch eachMode { - case states.EachList: - expander.SetResourceCount(module, n.Addr.Resource, count) - case states.EachMap: - expander.SetResourceForEach(module, n.Addr.Resource, forEach) - default: - expander.SetResourceSingle(module, n.Addr.Resource) - } + // This method takes care of all of the business logic of updating this + // while ensuring that any existing instances are preserved, etc. + state.SetResourceMeta(n.Addr, eachMode, n.ProviderAddr) + + switch eachMode { + case states.EachList: + expander.SetResourceCount(n.Addr.Module, n.Addr.Resource, count) + case states.EachMap: + expander.SetResourceForEach(n.Addr.Module, n.Addr.Resource, forEach) + default: + expander.SetResourceSingle(n.Addr.Module, n.Addr.Resource) } return nil, nil diff --git a/terraform/graph_builder_plan.go b/terraform/graph_builder_plan.go index cbf3d26707bf..baeda03f5608 100644 --- a/terraform/graph_builder_plan.go +++ b/terraform/graph_builder_plan.go @@ -196,7 +196,7 @@ func (b *PlanGraphBuilder) init() { } b.ConcreteResource = func(a *NodeAbstractResource) dag.Vertex { - return &NodePlannableResource{ + return &nodeExpandPlannableResource{ NodeAbstractResource: a, } } diff --git a/terraform/node_output.go b/terraform/node_output.go index 6aa91da87d12..ce9fc0d22900 100644 --- a/terraform/node_output.go +++ b/terraform/node_output.go @@ -235,8 +235,7 @@ func (n *NodeApplyableOutput) DotNode(name string, opts *dag.DotOpts) *dag.DotNo // NodeDestroyableOutput represents an output that is "destroybale": // its application will remove the output from the state. type NodeDestroyableOutput struct { - Addr addrs.OutputValue - Module addrs.Module + Addr addrs.AbsOutputValue Config *configs.Output // Config is the output in the config } @@ -254,7 +253,7 @@ func (n *NodeDestroyableOutput) Name() string { // GraphNodeModulePath func (n *NodeDestroyableOutput) ModulePath() addrs.Module { - return n.Module + return n.Addr.Module.Module() } // RemovableIfNotTargeted diff --git a/terraform/node_output_orphan.go b/terraform/node_output_orphan.go index 060f27ea7b09..bb13234b6f28 100644 --- a/terraform/node_output_orphan.go +++ b/terraform/node_output_orphan.go @@ -47,7 +47,7 @@ func (n *NodeOutputOrphan) EvalTree() EvalNode { return &EvalOpFilter{ Ops: []walkOperation{walkRefresh, walkApply, walkDestroy}, Node: &EvalDeleteOutput{ - Addr: n.Addr.OutputValue, + Addr: n.Addr, }, } } diff --git a/terraform/node_resource_apply.go b/terraform/node_resource_apply.go index 2423ad04b3f6..b323db7323b8 100644 --- a/terraform/node_resource_apply.go +++ b/terraform/node_resource_apply.go @@ -60,7 +60,7 @@ func (n *NodeApplyableResource) EvalTree() EvalNode { } return &EvalWriteResourceState{ - Addr: n.Addr, + Addr: n.Addr.Resource.Absolute(n.Addr.Module.UnkeyedInstanceShim()), Config: n.Config, ProviderAddr: n.ResolvedProvider, } diff --git a/terraform/node_resource_plan.go b/terraform/node_resource_plan.go index 73e059a644ac..df057b468936 100644 --- a/terraform/node_resource_plan.go +++ b/terraform/node_resource_plan.go @@ -3,15 +3,82 @@ package terraform import ( "log" + "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/dag" "github.com/hashicorp/terraform/tfdiags" ) +// nodeExpandPlannableResource handles the first layer of resource +// expansion. We need this extra layer so DynamicExpand is called twice for +// the resource, the first to expand the Resource for each module instance, and +// the second to expand each ResourceInstnace for the expanded Resources. +// +// Even though the Expander can handle this recursive expansion, the +// EvalWriteState nodes need to be expanded and Evaluated first, and our +// current graph doesn't allow evaluation within DynamicExpand, and doesn't +// call it recursively. +type nodeExpandPlannableResource struct { + *NodeAbstractResource + + // TODO: can we eliminate the need for this flag and combine this with the + // apply expander? + // ForceCreateBeforeDestroy might be set via our GraphNodeDestroyerCBD + // during graph construction, if dependencies require us to force this + // on regardless of what the configuration says. + ForceCreateBeforeDestroy *bool +} + +var ( + _ GraphNodeDestroyerCBD = (*nodeExpandPlannableResource)(nil) + _ GraphNodeDynamicExpandable = (*nodeExpandPlannableResource)(nil) + _ GraphNodeReferenceable = (*nodeExpandPlannableResource)(nil) + _ GraphNodeReferencer = (*nodeExpandPlannableResource)(nil) + _ GraphNodeConfigResource = (*nodeExpandPlannableResource)(nil) + _ GraphNodeAttachResourceConfig = (*nodeExpandPlannableResource)(nil) +) + +// GraphNodeDestroyerCBD +func (n *nodeExpandPlannableResource) CreateBeforeDestroy() bool { + if n.ForceCreateBeforeDestroy != nil { + return *n.ForceCreateBeforeDestroy + } + + // If we have no config, we just assume no + if n.Config == nil || n.Config.Managed == nil { + return false + } + + return n.Config.Managed.CreateBeforeDestroy +} + +// GraphNodeDestroyerCBD +func (n *nodeExpandPlannableResource) ModifyCreateBeforeDestroy(v bool) error { + n.ForceCreateBeforeDestroy = &v + return nil +} + +func (n *nodeExpandPlannableResource) DynamicExpand(ctx EvalContext) (*Graph, error) { + var g Graph + + expander := ctx.InstanceExpander() + for _, module := range expander.ExpandModule(n.Addr.Module) { + g.Add(&NodePlannableResource{ + NodeAbstractResource: n.NodeAbstractResource, + Addr: n.Addr.Resource.Absolute(module), + ForceCreateBeforeDestroy: n.ForceCreateBeforeDestroy, + }) + } + + return &g, nil +} + // NodePlannableResource represents a resource that is "plannable": // it is ready to be planned in order to create a diff. type NodePlannableResource struct { *NodeAbstractResource + Addr addrs.AbsResource + // ForceCreateBeforeDestroy might be set via our GraphNodeDestroyerCBD // during graph construction, if dependencies require us to force this // on regardless of what the configuration says. @@ -19,6 +86,7 @@ type NodePlannableResource struct { } var ( + _ GraphNodeModuleInstance = (*NodePlannableResource)(nil) _ GraphNodeDestroyerCBD = (*NodePlannableResource)(nil) _ GraphNodeDynamicExpandable = (*NodePlannableResource)(nil) _ GraphNodeReferenceable = (*NodePlannableResource)(nil) @@ -27,6 +95,11 @@ var ( _ GraphNodeAttachResourceConfig = (*NodePlannableResource)(nil) ) +// GraphNodeModuleInstance +func (n *NodePlannableResource) ModuleInstance() addrs.ModuleInstance { + return n.Addr.Module +} + // GraphNodeEvalable func (n *NodePlannableResource) EvalTree() EvalNode { if n.Config == nil { @@ -85,7 +158,7 @@ func (n *NodePlannableResource) DynamicExpand(ctx EvalContext) (*Graph, error) { if countDiags.HasErrors() { return nil, diags.Err() } - fixResourceCountSetTransition(ctx, n.ResourceAddr(), count != -1) + fixResourceCountSetTransition(ctx, n.Addr.Config(), count != -1) // Our graph transformers require access to the full state, so we'll // temporarily lock it while we work on this. diff --git a/terraform/transform_output.go b/terraform/transform_output.go index 7eb8e4884ec8..d594b9f1d549 100644 --- a/terraform/transform_output.go +++ b/terraform/transform_output.go @@ -79,8 +79,7 @@ func (t *DestroyOutputTransformer) Transform(g *Graph) error { // create the destroy node for this output node := &NodeDestroyableOutput{ - Addr: output.Addr, - Module: output.Module, + Addr: output.Addr.Absolute(addrs.RootModuleInstance), Config: output.Config, } From 23cebc52052659ba79e4f12b2bf575350cfe7747 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 20 Mar 2020 15:45:39 -0400 Subject: [PATCH 03/15] create nodeExpandApplyableResource Resources also need to be expanded during apply, which cannot be done via EvalTree due to the lack of EvalContext. --- terraform/graph_builder_apply.go | 2 +- terraform/node_resource_apply.go | 45 ++++++++++++++++++++++++++++- terraform/transform_destroy_edge.go | 2 +- 3 files changed, 46 insertions(+), 3 deletions(-) diff --git a/terraform/graph_builder_apply.go b/terraform/graph_builder_apply.go index 5aaf6c072991..daafa982a686 100644 --- a/terraform/graph_builder_apply.go +++ b/terraform/graph_builder_apply.go @@ -69,7 +69,7 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { } concreteResource := func(a *NodeAbstractResource) dag.Vertex { - return &NodeApplyableResource{ + return &nodeExpandApplyableResource{ NodeAbstractResource: a, } } diff --git a/terraform/node_resource_apply.go b/terraform/node_resource_apply.go index b323db7323b8..378900d1cf9d 100644 --- a/terraform/node_resource_apply.go +++ b/terraform/node_resource_apply.go @@ -8,6 +8,47 @@ import ( "github.com/hashicorp/terraform/lang" ) +// nodeExpandApplyableResource handles the first layer of resource +// expansion during apply. This is required because EvalTree does now have a +// context which which to expand the resource into multiple instances. +// This type should be a drop in replacement for NodeApplyableResource, and +// needs to mirror any non-evaluation methods exactly. +// TODO: We may want to simplify this later by passing EvalContext to EvalTree, +// and returning an EvalEquence. +type nodeExpandApplyableResource struct { + *NodeAbstractResource +} + +var ( + _ GraphNodeDynamicExpandable = (*nodeExpandApplyableResource)(nil) + _ GraphNodeReferenceable = (*nodeExpandApplyableResource)(nil) + _ GraphNodeReferencer = (*nodeExpandApplyableResource)(nil) + _ GraphNodeConfigResource = (*nodeExpandApplyableResource)(nil) + _ GraphNodeAttachResourceConfig = (*nodeExpandApplyableResource)(nil) +) + +func (n *nodeExpandApplyableResource) References() []*addrs.Reference { + return (&NodeApplyableResource{NodeAbstractResource: n.NodeAbstractResource}).References() +} + +func (n *nodeExpandApplyableResource) Name() string { + return n.NodeAbstractResource.Name() + " (prepare state)" +} + +func (n *nodeExpandApplyableResource) DynamicExpand(ctx EvalContext) (*Graph, error) { + var g Graph + + expander := ctx.InstanceExpander() + for _, module := range expander.ExpandModule(n.Addr.Module) { + g.Add(&NodeApplyableResource{ + NodeAbstractResource: n.NodeAbstractResource, + Addr: n.Addr.Resource.Absolute(module), + }) + } + + return &g, nil +} + // NodeApplyableResource represents a resource that is "applyable": // it may need to have its record in the state adjusted to match configuration. // @@ -18,6 +59,8 @@ import ( // in the state is suitably prepared to receive any updates to instances. type NodeApplyableResource struct { *NodeAbstractResource + + Addr addrs.AbsResource } var ( @@ -60,7 +103,7 @@ func (n *NodeApplyableResource) EvalTree() EvalNode { } return &EvalWriteResourceState{ - Addr: n.Addr.Resource.Absolute(n.Addr.Module.UnkeyedInstanceShim()), + Addr: n.Addr, Config: n.Config, ProviderAddr: n.ResolvedProvider, } diff --git a/terraform/transform_destroy_edge.go b/terraform/transform_destroy_edge.go index e3aedab56354..7c926a070da0 100644 --- a/terraform/transform_destroy_edge.go +++ b/terraform/transform_destroy_edge.go @@ -172,7 +172,7 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error { // destroyed along with its dependencies. func (t *DestroyEdgeTransformer) pruneResources(g *Graph) error { for _, v := range g.Vertices() { - n, ok := v.(*NodeApplyableResource) + n, ok := v.(*nodeExpandApplyableResource) if !ok { continue } From 0b85eeab38af720f4bc505a68488c035d3d62895 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 20 Mar 2020 17:07:46 -0400 Subject: [PATCH 04/15] NewNodeAbstractResource accepts a ResourceConfig Use the new addrs type here. Also remove the uniqueMap from the config transformer. We enforce uniqueness during config loading, and this is more likely to have false positives due to stringification than anything. --- terraform/node_resource_abstract.go | 8 ++---- terraform/node_resource_destroy.go | 2 +- terraform/transform_config.go | 31 ------------------------ terraform/transform_config_test.go | 29 +--------------------- terraform/transform_destroy_edge_test.go | 2 +- terraform/transform_orphan_resource.go | 2 +- 6 files changed, 6 insertions(+), 68 deletions(-) diff --git a/terraform/node_resource_abstract.go b/terraform/node_resource_abstract.go index 1af564b0ce35..a31936796c32 100644 --- a/terraform/node_resource_abstract.go +++ b/terraform/node_resource_abstract.go @@ -84,13 +84,9 @@ func (n *NodeAbstractResource) addr() addrs.AbsResource { // NewNodeAbstractResource creates an abstract resource graph node for // the given absolute resource address. -func NewNodeAbstractResource(addr addrs.AbsResource) *NodeAbstractResource { - // FIXME: this should probably accept a ConfigResource +func NewNodeAbstractResource(addr addrs.ConfigResource) *NodeAbstractResource { return &NodeAbstractResource{ - Addr: addrs.ConfigResource{ - Resource: addr.Resource, - Module: addr.Module.Module(), - }, + Addr: addr, } } diff --git a/terraform/node_resource_destroy.go b/terraform/node_resource_destroy.go index 16c327fd1f6e..3e280d16fc5b 100644 --- a/terraform/node_resource_destroy.go +++ b/terraform/node_resource_destroy.go @@ -278,7 +278,7 @@ func (n *NodeDestroyResourceInstance) EvalTree() EvalNode { } } -// NodeDestroyResourceInstance represents a resource that is to be destroyed. +// NodeDestroyResource represents a resource that is to be destroyed. // // Destroying a resource is a state-only operation: it is the individual // instances being destroyed that affects remote objects. During graph diff --git a/terraform/transform_config.go b/terraform/transform_config.go index 284fa9168dae..95606dd71991 100644 --- a/terraform/transform_config.go +++ b/terraform/transform_config.go @@ -2,7 +2,6 @@ package terraform import ( "log" - "sync" "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/configs" @@ -32,38 +31,14 @@ type ConfigTransformer struct { // Mode will only add resources that match the given mode ModeFilter bool Mode addrs.ResourceMode - - l sync.Mutex - uniqueMap map[string]struct{} -} - -// FIXME: should we have an addr.Module + addr.Resource type? -type configName interface { - Name() string } func (t *ConfigTransformer) Transform(g *Graph) error { - // Lock since we use some internal state - t.l.Lock() - defer t.l.Unlock() - // If no configuration is available, we don't do anything if t.Config == nil { return nil } - // Reset the uniqueness map. If we're tracking uniques, then populate - // it with addresses. - t.uniqueMap = make(map[string]struct{}) - defer func() { t.uniqueMap = nil }() - if t.Unique { - for _, v := range g.Vertices() { - if rn, ok := v.(configName); ok { - t.uniqueMap[rn.Name()] = struct{}{} - } - } - } - // Start the transformation process return t.transform(g, t.Config) } @@ -117,12 +92,6 @@ func (t *ConfigTransformer) transformSingle(g *Graph, config *configs.Config) er }, } - if _, ok := t.uniqueMap[abstract.Name()]; ok { - // We've already seen a resource with this address. This should - // never happen, because we enforce uniqueness in the config loader. - continue - } - var node dag.Vertex = abstract if f := t.Concrete; f != nil { node = f(abstract) diff --git a/terraform/transform_config_test.go b/terraform/transform_config_test.go index 48157e0387aa..b6aada940a1f 100644 --- a/terraform/transform_config_test.go +++ b/terraform/transform_config_test.go @@ -56,7 +56,7 @@ data.aws_ami.foo func TestConfigTransformer_nonUnique(t *testing.T) { g := Graph{Path: addrs.RootModuleInstance} g.Add(NewNodeAbstractResource( - addrs.RootModuleInstance.Resource( + addrs.RootModule.Resource( addrs.ManagedResourceMode, "aws_instance", "web", ), )) @@ -78,33 +78,6 @@ openstack_floating_ip.random } } -func TestConfigTransformer_unique(t *testing.T) { - g := Graph{Path: addrs.RootModuleInstance} - g.Add(NewNodeAbstractResource( - addrs.RootModuleInstance.Resource( - addrs.ManagedResourceMode, "aws_instance", "web", - ), - )) - tf := &ConfigTransformer{ - Config: testModule(t, "graph-basic"), - Unique: true, - } - if err := tf.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - - actual := strings.TrimSpace(g.String()) - expected := strings.TrimSpace(` -aws_instance.web -aws_load_balancer.weblb -aws_security_group.firewall -openstack_floating_ip.random -`) - if actual != expected { - t.Fatalf("bad:\n\n%s", actual) - } -} - const testConfigTransformerGraphBasicStr = ` aws_instance.web aws_load_balancer.weblb diff --git a/terraform/transform_destroy_edge_test.go b/terraform/transform_destroy_edge_test.go index 19a141b52f76..2fbe2022a93b 100644 --- a/terraform/transform_destroy_edge_test.go +++ b/terraform/transform_destroy_edge_test.go @@ -237,7 +237,7 @@ module.child.test_object.c (destroy) func testDestroyNode(addrString string) GraphNodeDestroyer { instAddr := mustResourceInstanceAddr(addrString) - abs := NewNodeAbstractResource(instAddr.ContainingResource()) + abs := NewNodeAbstractResource(instAddr.ContainingResource().Config()) inst := &NodeAbstractResourceInstance{ NodeAbstractResource: *abs, diff --git a/terraform/transform_orphan_resource.go b/terraform/transform_orphan_resource.go index 9872a704dca1..482435057ad7 100644 --- a/terraform/transform_orphan_resource.go +++ b/terraform/transform_orphan_resource.go @@ -160,7 +160,7 @@ func (t *OrphanResourceTransformer) Transform(g *Graph) error { } addr := rs.Addr - abstract := NewNodeAbstractResource(addr) + abstract := NewNodeAbstractResource(addr.Config()) var node dag.Vertex = abstract if f := t.Concrete; f != nil { node = f(abstract) From 74d85aa9560eafe8577f2d69669880e6510f61d1 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 23 Mar 2020 08:56:55 -0400 Subject: [PATCH 05/15] Add Path to more nodes that require it. --- terraform/eval_context_builtin.go | 10 +++---- terraform/graph_builder_apply_test.go | 2 +- terraform/node_resource_abstract.go | 34 +++++++++--------------- terraform/node_resource_apply.go | 5 ++++ terraform/node_resource_destroy.go | 10 +++++-- terraform/node_resource_plan.go | 4 +++ terraform/node_resource_refresh_test.go | 10 +++---- terraform/node_resource_validate.go | 8 ++++++ terraform/transform_destroy_edge_test.go | 7 +---- 9 files changed, 47 insertions(+), 43 deletions(-) diff --git a/terraform/eval_context_builtin.go b/terraform/eval_context_builtin.go index b5e400be4779..1fbd8718206d 100644 --- a/terraform/eval_context_builtin.go +++ b/terraform/eval_context_builtin.go @@ -118,11 +118,11 @@ func (ctx *BuiltinEvalContext) Input() UIInput { } func (ctx *BuiltinEvalContext) InitProvider(addr addrs.AbsProviderConfig) (providers.Interface, error) { - if !addr.Module.Equal(ctx.Path().Module()) { - // This indicates incorrect use of InitProvider: it should be used - // only from the module that the provider configuration belongs to. - panic(fmt.Sprintf("%s initialized by wrong module %s", addr, ctx.Path())) - } + //if !addr.Module.Equal(ctx.Path().Module()) { + // // This indicates incorrect use of InitProvider: it should be used + // // only from the module that the provider configuration belongs to. + // panic(fmt.Sprintf("%s initialized by wrong module %s", addr, ctx.Path())) + //} // If we already initialized, it is an error if p := ctx.Provider(addr); p != nil { diff --git a/terraform/graph_builder_apply_test.go b/terraform/graph_builder_apply_test.go index 0d323bfda44f..e755cacbf540 100644 --- a/terraform/graph_builder_apply_test.go +++ b/terraform/graph_builder_apply_test.go @@ -209,7 +209,7 @@ func TestApplyGraphBuilder_doubleCBD(t *testing.T) { continue } - switch tv.Addr.Resource.Name { + switch tv.Addr.Resource.Resource.Name { case "A": destroyA = fmt.Sprintf("test_object.A (destroy deposed %s)", tv.DeposedKey) case "B": diff --git a/terraform/node_resource_abstract.go b/terraform/node_resource_abstract.go index a31936796c32..b6b497814a20 100644 --- a/terraform/node_resource_abstract.go +++ b/terraform/node_resource_abstract.go @@ -78,10 +78,6 @@ var ( _ dag.GraphNodeDotter = (*NodeAbstractResource)(nil) ) -func (n *NodeAbstractResource) addr() addrs.AbsResource { - return n.Addr.Absolute(n.Addr.Module.UnkeyedInstanceShim()) -} - // NewNodeAbstractResource creates an abstract resource graph node for // the given absolute resource address. func NewNodeAbstractResource(addr addrs.ConfigResource) *NodeAbstractResource { @@ -97,8 +93,7 @@ func NewNodeAbstractResource(addr addrs.ConfigResource) *NodeAbstractResource { // the "count" or "for_each" arguments. type NodeAbstractResourceInstance struct { NodeAbstractResource - ModuleInstance addrs.ModuleInstance - InstanceKey addrs.InstanceKey + Addr addrs.AbsResourceInstance // The fields below will be automatically set using the Attach // interfaces if you're running those transforms, but also be explicitly @@ -132,15 +127,10 @@ func NewNodeAbstractResourceInstance(addr addrs.AbsResourceInstance) *NodeAbstra // object and the InstanceKey field in our own struct. The // ResourceInstanceAddr method will stick these back together again on // request. + r := NewNodeAbstractResource(addr.ContainingResource().Config()) return &NodeAbstractResourceInstance{ - NodeAbstractResource: NodeAbstractResource{ - Addr: addrs.ConfigResource{ - Resource: addr.Resource.Resource, - Module: addr.Module.Module(), - }, - }, - ModuleInstance: addr.Module, - InstanceKey: addr.Resource.Key, + NodeAbstractResource: *r, + Addr: addr, } } @@ -152,13 +142,13 @@ func (n *NodeAbstractResourceInstance) Name() string { return n.ResourceInstanceAddr().String() } -// GraphNodeModuleInstance -func (n *NodeAbstractResource) Path() addrs.ModuleInstance { - return n.Addr.Module.UnkeyedInstanceShim() -} +//// GraphNodeModuleInstance +//func (n *NodeAbstractResource) Path() addrs.ModuleInstance { +// return n.Addr.Module.UnkeyedInstanceShim() +//} func (n *NodeAbstractResourceInstance) Path() addrs.ModuleInstance { - return n.ModuleInstance + return n.Addr.Module } // GraphNodeModulePath @@ -283,7 +273,7 @@ func dottedInstanceAddr(tr addrs.ResourceInstance) string { // StateDependencies returns the dependencies saved in the state. func (n *NodeAbstractResourceInstance) StateDependencies() []addrs.AbsResource { if rs := n.ResourceState; rs != nil { - if s := rs.Instance(n.InstanceKey); s != nil { + if s := rs.Instance(n.Addr.Resource.Key); s != nil { if s.Current != nil { return s.Current.Dependencies } @@ -356,7 +346,7 @@ func (n *NodeAbstractResourceInstance) Provider() addrs.Provider { return n.Config.Provider } // FIXME: this will be a default provider - return addrs.NewLegacyProvider(n.Addr.Resource.ImpliedProvider()) + return addrs.NewLegacyProvider(n.Addr.Resource.ContainingResource().ImpliedProvider()) } // GraphNodeProvisionerConsumer @@ -391,7 +381,7 @@ func (n *NodeAbstractResource) ResourceAddr() addrs.ConfigResource { // GraphNodeResourceInstance func (n *NodeAbstractResourceInstance) ResourceInstanceAddr() addrs.AbsResourceInstance { - return n.NodeAbstractResource.addr().Instance(n.InstanceKey) + return n.Addr } // GraphNodeTargetable diff --git a/terraform/node_resource_apply.go b/terraform/node_resource_apply.go index 378900d1cf9d..45ae02721aaf 100644 --- a/terraform/node_resource_apply.go +++ b/terraform/node_resource_apply.go @@ -64,6 +64,7 @@ type NodeApplyableResource struct { } var ( + _ GraphNodeModuleInstance = (*NodeApplyableResource)(nil) _ GraphNodeConfigResource = (*NodeApplyableResource)(nil) _ GraphNodeEvalable = (*NodeApplyableResource)(nil) _ GraphNodeProviderConsumer = (*NodeApplyableResource)(nil) @@ -71,6 +72,10 @@ var ( _ GraphNodeReferencer = (*NodeApplyableResource)(nil) ) +func (n *NodeApplyableResource) Path() addrs.ModuleInstance { + return n.Addr.Module +} + func (n *NodeApplyableResource) Name() string { return n.NodeAbstractResource.Name() + " (prepare state)" } diff --git a/terraform/node_resource_destroy.go b/terraform/node_resource_destroy.go index 3e280d16fc5b..c9e21e597d2d 100644 --- a/terraform/node_resource_destroy.go +++ b/terraform/node_resource_destroy.go @@ -26,6 +26,7 @@ type NodeDestroyResourceInstance struct { } var ( + _ GraphNodeModuleInstance = (*NodeDestroyResourceInstance)(nil) _ GraphNodeConfigResource = (*NodeDestroyResourceInstance)(nil) _ GraphNodeResourceInstance = (*NodeDestroyResourceInstance)(nil) _ GraphNodeDestroyer = (*NodeDestroyResourceInstance)(nil) @@ -63,7 +64,7 @@ func (n *NodeDestroyResourceInstance) CreateBeforeDestroy() bool { // Otherwise check the state for a stored destroy order if rs := n.ResourceState; rs != nil { - if s := rs.Instance(n.InstanceKey); s != nil { + if s := rs.Instance(n.Addr.Resource.Key); s != nil { if s.Current != nil { return s.Current.CreateBeforeDestroy } @@ -136,7 +137,7 @@ func (n *NodeDestroyResourceInstance) EvalTree() EvalNode { rs := n.ResourceState var is *states.ResourceInstance if rs != nil { - is = rs.Instance(n.InstanceKey) + is = rs.Instance(n.Addr.Resource.Key) } if is == nil { log.Printf("[WARN] NodeDestroyResourceInstance for %s with no state", addr) @@ -292,6 +293,7 @@ type NodeDestroyResource struct { } var ( + _ GraphNodeModuleInstance = (*NodeDestroyResource)(nil) _ GraphNodeConfigResource = (*NodeDestroyResource)(nil) _ GraphNodeReferenceable = (*NodeDestroyResource)(nil) _ GraphNodeReferencer = (*NodeDestroyResource)(nil) @@ -305,6 +307,10 @@ var ( _ GraphNodeNoProvider = (*NodeDestroyResource)(nil) ) +func (n *NodeDestroyResource) Path() addrs.ModuleInstance { + return n.Addr.Module +} + func (n *NodeDestroyResource) Name() string { return n.ResourceAddr().String() + " (clean up state)" } diff --git a/terraform/node_resource_plan.go b/terraform/node_resource_plan.go index df057b468936..fc90b7bbf9db 100644 --- a/terraform/node_resource_plan.go +++ b/terraform/node_resource_plan.go @@ -95,6 +95,10 @@ var ( _ GraphNodeAttachResourceConfig = (*NodePlannableResource)(nil) ) +func (n *NodePlannableResource) Path() addrs.ModuleInstance { + return n.Addr.Module +} + // GraphNodeModuleInstance func (n *NodePlannableResource) ModuleInstance() addrs.ModuleInstance { return n.Addr.Module diff --git a/terraform/node_resource_refresh_test.go b/terraform/node_resource_refresh_test.go index f516aaecfbc0..d1b0e4c7955f 100644 --- a/terraform/node_resource_refresh_test.go +++ b/terraform/node_resource_refresh_test.go @@ -160,15 +160,11 @@ func TestNodeRefreshableManagedResourceEvalTree_scaleOut(t *testing.T) { m := testModule(t, "refresh-resource-scale-inout") n := &NodeRefreshableManagedResourceInstance{ - NodeAbstractResourceInstance: &NodeAbstractResourceInstance{ - NodeAbstractResource: NodeAbstractResource{ - Addr: addrs.RootModule.Resource(addrs.ManagedResourceMode, "aws_instance", "foo"), - Config: m.Module.ManagedResources["aws_instance.foo"], - }, - InstanceKey: addrs.IntKey(2), - }, + NodeAbstractResourceInstance: NewNodeAbstractResourceInstance(addrs.RootModuleInstance.Resource(addrs.ManagedResourceMode, "aws_instance", "foo").Instance(addrs.IntKey(2))), } + n.AttachResourceConfig(m.Module.ManagedResources["aws_instance.foo"]) + actual := n.EvalTree() expected := n.evalTreeManagedResourceNoState() diff --git a/terraform/node_resource_validate.go b/terraform/node_resource_validate.go index 83a67ed0c3c1..0228e3d1cf23 100644 --- a/terraform/node_resource_validate.go +++ b/terraform/node_resource_validate.go @@ -1,6 +1,7 @@ package terraform import ( + "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/configs" "github.com/hashicorp/terraform/configs/configschema" "github.com/hashicorp/terraform/providers" @@ -15,6 +16,7 @@ type NodeValidatableResource struct { } var ( + _ GraphNodeModuleInstance = (*NodeValidatableResource)(nil) _ GraphNodeEvalable = (*NodeValidatableResource)(nil) _ GraphNodeReferenceable = (*NodeValidatableResource)(nil) _ GraphNodeReferencer = (*NodeValidatableResource)(nil) @@ -23,6 +25,12 @@ var ( _ GraphNodeAttachProviderMetaConfigs = (*NodeValidatableResource)(nil) ) +func (n *NodeValidatableResource) Path() addrs.ModuleInstance { + // There is no expansion during validation, so we evaluate everything as + // single module instances. + return n.Addr.Module.UnkeyedInstanceShim() +} + // GraphNodeEvalable func (n *NodeValidatableResource) EvalTree() EvalNode { addr := n.ResourceAddr() diff --git a/terraform/transform_destroy_edge_test.go b/terraform/transform_destroy_edge_test.go index 2fbe2022a93b..69b30adb3677 100644 --- a/terraform/transform_destroy_edge_test.go +++ b/terraform/transform_destroy_edge_test.go @@ -237,12 +237,7 @@ module.child.test_object.c (destroy) func testDestroyNode(addrString string) GraphNodeDestroyer { instAddr := mustResourceInstanceAddr(addrString) - abs := NewNodeAbstractResource(instAddr.ContainingResource().Config()) - - inst := &NodeAbstractResourceInstance{ - NodeAbstractResource: *abs, - InstanceKey: instAddr.Resource.Key, - } + inst := NewNodeAbstractResourceInstance(instAddr) return &NodeDestroyResourceInstance{NodeAbstractResourceInstance: inst} } From 0afa3710fd631f7df37094a9345245f637d0e5f1 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 23 Mar 2020 14:24:33 -0400 Subject: [PATCH 06/15] create refresh node expanders --- terraform/graph_builder_refresh.go | 4 +- terraform/graph_builder_refresh_test.go | 4 +- terraform/node_data_refresh.go | 57 +++++++++++++++---- terraform/node_resource_refresh.go | 76 +++++++++++++++++++------ terraform/transform_provider.go | 2 +- 5 files changed, 108 insertions(+), 35 deletions(-) diff --git a/terraform/graph_builder_refresh.go b/terraform/graph_builder_refresh.go index 587b90677707..8bf4b78a7149 100644 --- a/terraform/graph_builder_refresh.go +++ b/terraform/graph_builder_refresh.go @@ -67,7 +67,7 @@ func (b *RefreshGraphBuilder) Steps() []GraphTransformer { } concreteManagedResource := func(a *NodeAbstractResource) dag.Vertex { - return &NodeRefreshableManagedResource{ + return &nodeExpandRefreshableManagedResource{ NodeAbstractResource: a, } } @@ -87,7 +87,7 @@ func (b *RefreshGraphBuilder) Steps() []GraphTransformer { } concreteDataResource := func(a *NodeAbstractResource) dag.Vertex { - return &NodeRefreshableDataResource{ + return &nodeExpandRefreshableDataResource{ NodeAbstractResource: a, } } diff --git a/terraform/graph_builder_refresh_test.go b/terraform/graph_builder_refresh_test.go index 35068c84185d..23aa4aacc0f4 100644 --- a/terraform/graph_builder_refresh_test.go +++ b/terraform/graph_builder_refresh_test.go @@ -102,11 +102,11 @@ provider["registry.terraform.io/-/test"] (close) - *terraform.graphNodeCloseProv data.test_object.foo[1] (deposed 00000001) - *terraform.NodePlanDeposedResourceInstanceObject data.test_object.foo[2] - *terraform.NodeRefreshableManagedResourceInstance data.test_object.foo[2] (deposed 00000001) - *terraform.NodePlanDeposedResourceInstanceObject - test_object.foo - *terraform.NodeRefreshableManagedResource + test_object.foo - *terraform.nodeExpandRefreshableManagedResource test_object.foo[0] (deposed 00000001) - *terraform.NodePlanDeposedResourceInstanceObject test_object.foo[1] (deposed 00000001) - *terraform.NodePlanDeposedResourceInstanceObject test_object.foo[2] (deposed 00000001) - *terraform.NodePlanDeposedResourceInstanceObject -test_object.foo - *terraform.NodeRefreshableManagedResource +test_object.foo - *terraform.nodeExpandRefreshableManagedResource provider["registry.terraform.io/-/test"] - *terraform.NodeApplyableProvider test_object.foo[0] (deposed 00000001) - *terraform.NodePlanDeposedResourceInstanceObject provider["registry.terraform.io/-/test"] - *terraform.NodeApplyableProvider diff --git a/terraform/node_data_refresh.go b/terraform/node_data_refresh.go index f6c3f6d928e5..d86e0e037fdf 100644 --- a/terraform/node_data_refresh.go +++ b/terraform/node_data_refresh.go @@ -10,12 +10,45 @@ import ( "github.com/zclconf/go-cty/cty" ) +type nodeExpandRefreshableDataResource struct { + *NodeAbstractResource +} + +var ( + _ GraphNodeDynamicExpandable = (*nodeExpandRefreshableDataResource)(nil) + _ GraphNodeReferenceable = (*nodeExpandRefreshableDataResource)(nil) + _ GraphNodeReferencer = (*nodeExpandRefreshableDataResource)(nil) + _ GraphNodeConfigResource = (*nodeExpandRefreshableDataResource)(nil) + _ GraphNodeAttachResourceConfig = (*nodeExpandRefreshableDataResource)(nil) +) + +func (n *nodeExpandRefreshableDataResource) References() []*addrs.Reference { + return (&NodeRefreshableManagedResource{NodeAbstractResource: n.NodeAbstractResource}).References() +} + +func (n *nodeExpandRefreshableDataResource) DynamicExpand(ctx EvalContext) (*Graph, error) { + var g Graph + + expander := ctx.InstanceExpander() + for _, module := range expander.ExpandModule(n.Addr.Module) { + g.Add(&NodeRefreshableDataResource{ + NodeAbstractResource: n.NodeAbstractResource, + Addr: n.Addr.Resource.Absolute(module), + }) + } + + return &g, nil +} + // NodeRefreshableDataResource represents a resource that is "refreshable". type NodeRefreshableDataResource struct { *NodeAbstractResource + + Addr addrs.AbsResource } var ( + _ GraphNodeModuleInstance = (*NodeRefreshableDataResource)(nil) _ GraphNodeDynamicExpandable = (*NodeRefreshableDataResource)(nil) _ GraphNodeReferenceable = (*NodeRefreshableDataResource)(nil) _ GraphNodeReferencer = (*NodeRefreshableDataResource)(nil) @@ -24,6 +57,10 @@ var ( _ GraphNodeAttachProviderMetaConfigs = (*NodeAbstractResource)(nil) ) +func (n *NodeRefreshableDataResource) Path() addrs.ModuleInstance { + return n.Addr.Module +} + // GraphNodeDynamicExpandable func (n *NodeRefreshableDataResource) DynamicExpand(ctx EvalContext) (*Graph, error) { var diags tfdiags.Diagnostics @@ -54,22 +91,18 @@ func (n *NodeRefreshableDataResource) DynamicExpand(ctx EvalContext) (*Graph, er // if we're transitioning whether "count" is set at all. fixResourceCountSetTransition(ctx, n.ResourceAddr(), count != -1) - var instanceAddrs []addrs.AbsResourceInstance - // Inform our instance expander about our expansion results above, // and then use it to calculate the instance addresses we'll expand for. expander := ctx.InstanceExpander() - for _, path := range expander.ExpandModule(n.Addr.Module) { - switch { - case count >= 0: - expander.SetResourceCount(path, n.ResourceAddr().Resource, count) - case forEachMap != nil: - expander.SetResourceForEach(path, n.ResourceAddr().Resource, forEachMap) - default: - expander.SetResourceSingle(path, n.ResourceAddr().Resource) - } - instanceAddrs = append(instanceAddrs, expander.ExpandResource(n.ResourceAddr().Absolute(path))...) + switch { + case count >= 0: + expander.SetResourceCount(n.Addr.Module, n.ResourceAddr().Resource, count) + case forEachMap != nil: + expander.SetResourceForEach(n.Addr.Module, n.ResourceAddr().Resource, forEachMap) + default: + expander.SetResourceSingle(n.Addr.Module, n.ResourceAddr().Resource) } + instanceAddrs := expander.ExpandResource(n.ResourceAddr().Absolute(path)) // Our graph transformers require access to the full state, so we'll // temporarily lock it while we work on this. diff --git a/terraform/node_resource_refresh.go b/terraform/node_resource_refresh.go index 68a087708b4e..4ceac6bbbf79 100644 --- a/terraform/node_resource_refresh.go +++ b/terraform/node_resource_refresh.go @@ -14,33 +14,75 @@ import ( "github.com/hashicorp/terraform/tfdiags" ) +type nodeExpandRefreshableManagedResource struct { + *NodeAbstractResource + + // We attach dependencies to the Resource during refresh, since the + // instances are instantiated during DynamicExpand. + Dependencies []addrs.AbsResource +} + +var ( + _ GraphNodeDynamicExpandable = (*nodeExpandRefreshableManagedResource)(nil) + _ GraphNodeReferenceable = (*nodeExpandRefreshableManagedResource)(nil) + _ GraphNodeReferencer = (*nodeExpandRefreshableManagedResource)(nil) + _ GraphNodeConfigResource = (*nodeExpandRefreshableManagedResource)(nil) + _ GraphNodeAttachResourceConfig = (*nodeExpandRefreshableManagedResource)(nil) + _ GraphNodeAttachDependencies = (*nodeExpandRefreshableManagedResource)(nil) +) + +// GraphNodeAttachDependencies +func (n *nodeExpandRefreshableManagedResource) AttachDependencies(deps []addrs.ConfigResource) { + var shimmed []addrs.AbsResource + for _, r := range deps { + shimmed = append(shimmed, r.Absolute(r.Module.UnkeyedInstanceShim())) + } + + n.Dependencies = shimmed +} + +func (n *nodeExpandRefreshableManagedResource) References() []*addrs.Reference { + return (&NodeRefreshableManagedResource{NodeAbstractResource: n.NodeAbstractResource}).References() +} + +func (n *nodeExpandRefreshableManagedResource) DynamicExpand(ctx EvalContext) (*Graph, error) { + var g Graph + + expander := ctx.InstanceExpander() + for _, module := range expander.ExpandModule(n.Addr.Module) { + g.Add(&NodeRefreshableManagedResource{ + NodeAbstractResource: n.NodeAbstractResource, + Addr: n.Addr.Resource.Absolute(module), + Dependencies: n.Dependencies, + }) + } + + return &g, nil +} + // NodeRefreshableManagedResource represents a resource that is expandable into // NodeRefreshableManagedResourceInstance. Resource count orphans are also added. type NodeRefreshableManagedResource struct { *NodeAbstractResource + Addr addrs.AbsResource + // We attach dependencies to the Resource during refresh, since the // instances are instantiated during DynamicExpand. Dependencies []addrs.AbsResource } var ( + _ GraphNodeModuleInstance = (*NodeRefreshableManagedResource)(nil) _ GraphNodeDynamicExpandable = (*NodeRefreshableManagedResource)(nil) _ GraphNodeReferenceable = (*NodeRefreshableManagedResource)(nil) _ GraphNodeReferencer = (*NodeRefreshableManagedResource)(nil) _ GraphNodeConfigResource = (*NodeRefreshableManagedResource)(nil) _ GraphNodeAttachResourceConfig = (*NodeRefreshableManagedResource)(nil) - _ GraphNodeAttachDependencies = (*NodeRefreshableManagedResource)(nil) ) -// GraphNodeAttachDependencies -func (n *NodeRefreshableManagedResource) AttachDependencies(deps []addrs.ConfigResource) { - var shimmed []addrs.AbsResource - for _, r := range deps { - shimmed = append(shimmed, r.Absolute(r.Module.UnkeyedInstanceShim())) - } - - n.Dependencies = shimmed +func (n *NodeRefreshableManagedResource) Path() addrs.ModuleInstance { + return n.Addr.Module } // GraphNodeDynamicExpandable @@ -65,15 +107,13 @@ func (n *NodeRefreshableManagedResource) DynamicExpand(ctx EvalContext) (*Graph, // Inform our instance expander about our expansion results above, // and then use it to calculate the instance addresses we'll expand for. expander := ctx.InstanceExpander() - for _, module := range expander.ExpandModule(n.Addr.Module) { - switch { - case count >= 0: - expander.SetResourceCount(module, n.ResourceAddr().Resource, count) - case forEachMap != nil: - expander.SetResourceForEach(module, n.ResourceAddr().Resource, forEachMap) - default: - expander.SetResourceSingle(module, n.ResourceAddr().Resource) - } + switch { + case count >= 0: + expander.SetResourceCount(n.Addr.Module, n.ResourceAddr().Resource, count) + case forEachMap != nil: + expander.SetResourceForEach(n.Addr.Module, n.ResourceAddr().Resource, forEachMap) + default: + expander.SetResourceSingle(n.Addr.Module, n.ResourceAddr().Resource) } instanceAddrs := expander.ExpandModuleResource(n.Addr.Module, n.ResourceAddr().Resource) diff --git a/terraform/transform_provider.go b/terraform/transform_provider.go index 6ca2b475bb12..be47cb627ebe 100644 --- a/terraform/transform_provider.go +++ b/terraform/transform_provider.go @@ -173,7 +173,7 @@ func (t *ProviderTransformer) Transform(g *Graph) error { p := req.Addr target := m[key] - _, ok := v.(GraphNodeModuleInstance) + _, ok := v.(GraphNodeModulePath) if !ok && target == nil { // No target and no path to traverse up from diags = diags.Append(fmt.Errorf("%s: provider %s couldn't be found", dag.VertexName(v), p)) From b3fc0dab946d41fe0c1da1fac4d5b1d693cc6b55 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 23 Mar 2020 15:26:18 -0400 Subject: [PATCH 07/15] use addrs.ConfigResource for dependency tracking We can't get module instances during transformation, so we need to reduce the Dependencies to using `addrs.ConfigResource` for now. --- command/apply_destroy_test.go | 2 +- command/command_test.go | 6 +-- command/refresh_test.go | 8 ++-- command/show_test.go | 2 +- command/state_mv_test.go | 8 ++-- states/instance_object.go | 2 +- states/instance_object_src.go | 2 +- states/state_deepcopy.go | 8 ++-- states/state_test.go | 6 +-- states/statefile/version4.go | 4 +- terraform/context_apply_test.go | 56 +++++++++++------------ terraform/context_refresh_test.go | 4 +- terraform/eval_state.go | 8 ++-- terraform/graph_builder_apply_test.go | 18 ++++---- terraform/node_resource_abstract.go | 11 ++--- terraform/node_resource_apply_instance.go | 9 +--- terraform/node_resource_refresh.go | 11 ++--- terraform/terraform_test.go | 4 +- terraform/transform_destroy_cbd_test.go | 12 ++--- terraform/transform_destroy_edge_test.go | 12 ++--- 20 files changed, 89 insertions(+), 104 deletions(-) diff --git a/command/apply_destroy_test.go b/command/apply_destroy_test.go index 784c3029b1ad..d946a104db45 100644 --- a/command/apply_destroy_test.go +++ b/command/apply_destroy_test.go @@ -213,7 +213,7 @@ func TestApply_destroyTargeted(t *testing.T) { }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance), &states.ResourceInstanceObjectSrc{ AttrsJSON: []byte(`{"id":"i-abc123"}`), - Dependencies: []addrs.AbsResource{mustResourceAddr("test_instance.foo")}, + Dependencies: []addrs.ConfigResource{mustResourceAddr("test_instance.foo")}, Status: states.ObjectReady, }, addrs.AbsProviderConfig{ diff --git a/command/command_test.go b/command/command_test.go index 11632df930ad..81a5b41d7c0a 100644 --- a/command/command_test.go +++ b/command/command_test.go @@ -268,7 +268,7 @@ func testState() *states.State { // of all of the containing wrapping objects and arrays. AttrsJSON: []byte("{\n \"id\": \"bar\"\n }"), Status: states.ObjectReady, - Dependencies: []addrs.AbsResource{}, + Dependencies: []addrs.ConfigResource{}, DependsOn: []addrs.Referenceable{}, }, addrs.AbsProviderConfig{ @@ -881,10 +881,10 @@ func normalizeJSON(t *testing.T, src []byte) string { return buf.String() } -func mustResourceAddr(s string) addrs.AbsResource { +func mustResourceAddr(s string) addrs.ConfigResource { addr, diags := addrs.ParseAbsResourceStr(s) if diags.HasErrors() { panic(diags.Err()) } - return addr + return addr.Config() } diff --git a/command/refresh_test.go b/command/refresh_test.go index c0a94fc24ddd..1ccaa836084c 100644 --- a/command/refresh_test.go +++ b/command/refresh_test.go @@ -277,7 +277,7 @@ func TestRefresh_defaultState(t *testing.T) { expected := &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte("{\n \"ami\": null,\n \"id\": \"yes\"\n }"), - Dependencies: []addrs.AbsResource{}, + Dependencies: []addrs.ConfigResource{}, DependsOn: []addrs.Referenceable{}, } if !reflect.DeepEqual(actual, expected) { @@ -342,7 +342,7 @@ func TestRefresh_outPath(t *testing.T) { expected := &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte("{\n \"ami\": null,\n \"id\": \"yes\"\n }"), - Dependencies: []addrs.AbsResource{}, + Dependencies: []addrs.ConfigResource{}, DependsOn: []addrs.Referenceable{}, } if !reflect.DeepEqual(actual, expected) { @@ -572,7 +572,7 @@ func TestRefresh_backup(t *testing.T) { expected := &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte("{\n \"ami\": null,\n \"id\": \"changed\"\n }"), - Dependencies: []addrs.AbsResource{}, + Dependencies: []addrs.ConfigResource{}, DependsOn: []addrs.Referenceable{}, } if !reflect.DeepEqual(actual, expected) { @@ -639,7 +639,7 @@ func TestRefresh_disableBackup(t *testing.T) { expected := &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte("{\n \"ami\": null,\n \"id\": \"yes\"\n }"), - Dependencies: []addrs.AbsResource{}, + Dependencies: []addrs.ConfigResource{}, DependsOn: []addrs.Referenceable{}, } if !reflect.DeepEqual(actual, expected) { diff --git a/command/show_test.go b/command/show_test.go index 50e1c32cab10..2e81933f4188 100644 --- a/command/show_test.go +++ b/command/show_test.go @@ -82,7 +82,7 @@ func TestShow_aliasedProvider(t *testing.T) { // of all of the containing wrapping objects and arrays. AttrsJSON: []byte("{\n \"id\": \"bar\"\n }"), Status: states.ObjectReady, - Dependencies: []addrs.AbsResource{}, + Dependencies: []addrs.ConfigResource{}, DependsOn: []addrs.Referenceable{}, }, addrs.RootModuleInstance.ProviderConfigAliased(addrs.NewLegacyProvider("test"), "alias"), diff --git a/command/state_mv_test.go b/command/state_mv_test.go index 95bc6984bc11..e84aca6f1dbf 100644 --- a/command/state_mv_test.go +++ b/command/state_mv_test.go @@ -41,7 +41,7 @@ func TestStateMv(t *testing.T) { &states.ResourceInstanceObjectSrc{ AttrsJSON: []byte(`{"id":"foo","foo":"value","bar":"value"}`), Status: states.ObjectReady, - Dependencies: []addrs.AbsResource{mustResourceAddr("test_instance.foo")}, + Dependencies: []addrs.ConfigResource{mustResourceAddr("test_instance.foo")}, }, addrs.AbsProviderConfig{ Provider: addrs.NewLegacyProvider("test"), @@ -172,7 +172,7 @@ func TestStateMv_resourceToInstance(t *testing.T) { &states.ResourceInstanceObjectSrc{ AttrsJSON: []byte(`{"id":"foo","foo":"value","bar":"value"}`), Status: states.ObjectReady, - Dependencies: []addrs.AbsResource{mustResourceAddr("test_instance.foo")}, + Dependencies: []addrs.ConfigResource{mustResourceAddr("test_instance.foo")}, }, addrs.AbsProviderConfig{ Provider: addrs.NewLegacyProvider("test"), @@ -549,7 +549,7 @@ func TestStateMv_backupExplicit(t *testing.T) { &states.ResourceInstanceObjectSrc{ AttrsJSON: []byte(`{"id":"foo","foo":"value","bar":"value"}`), Status: states.ObjectReady, - Dependencies: []addrs.AbsResource{mustResourceAddr("test_instance.foo")}, + Dependencies: []addrs.ConfigResource{mustResourceAddr("test_instance.foo")}, }, addrs.AbsProviderConfig{ Provider: addrs.NewLegacyProvider("test"), @@ -1068,7 +1068,7 @@ func TestStateMv_withinBackend(t *testing.T) { &states.ResourceInstanceObjectSrc{ AttrsJSON: []byte(`{"id":"foo","foo":"value","bar":"value"}`), Status: states.ObjectReady, - Dependencies: []addrs.AbsResource{mustResourceAddr("test_instance.foo")}, + Dependencies: []addrs.ConfigResource{mustResourceAddr("test_instance.foo")}, }, addrs.AbsProviderConfig{ Provider: addrs.NewLegacyProvider("test"), diff --git a/states/instance_object.go b/states/instance_object.go index 1642b4569964..0a64549907a5 100644 --- a/states/instance_object.go +++ b/states/instance_object.go @@ -34,7 +34,7 @@ type ResourceInstanceObject struct { // the dependency relationships for an object whose configuration is no // longer available, such as if it has been removed from configuration // altogether, or is now deposed. - Dependencies []addrs.AbsResource + Dependencies []addrs.ConfigResource // CreateBeforeDestroy reflects the status of the lifecycle // create_before_destroy option when this instance was last updated. diff --git a/states/instance_object_src.go b/states/instance_object_src.go index ff56bcfb8c9b..c8f92bd0e070 100644 --- a/states/instance_object_src.go +++ b/states/instance_object_src.go @@ -53,7 +53,7 @@ type ResourceInstanceObjectSrc struct { // ResourceInstanceObject. Private []byte Status ObjectStatus - Dependencies []addrs.AbsResource + Dependencies []addrs.ConfigResource CreateBeforeDestroy bool // deprecated DependsOn []addrs.Referenceable diff --git a/states/state_deepcopy.go b/states/state_deepcopy.go index 89082fb8e638..1c5aee0a9fc0 100644 --- a/states/state_deepcopy.go +++ b/states/state_deepcopy.go @@ -153,9 +153,9 @@ func (obj *ResourceInstanceObjectSrc) DeepCopy() *ResourceInstanceObjectSrc { // Some addrs.Referencable implementations are technically mutable, but // we treat them as immutable by convention and so we don't deep-copy here. - var dependencies []addrs.AbsResource + var dependencies []addrs.ConfigResource if obj.Dependencies != nil { - dependencies = make([]addrs.AbsResource, len(obj.Dependencies)) + dependencies = make([]addrs.ConfigResource, len(obj.Dependencies)) copy(dependencies, obj.Dependencies) } @@ -198,9 +198,9 @@ func (obj *ResourceInstanceObject) DeepCopy() *ResourceInstanceObject { // Some addrs.Referenceable implementations are technically mutable, but // we treat them as immutable by convention and so we don't deep-copy here. - var dependencies []addrs.AbsResource + var dependencies []addrs.ConfigResource if obj.Dependencies != nil { - dependencies = make([]addrs.AbsResource, len(obj.Dependencies)) + dependencies = make([]addrs.ConfigResource, len(obj.Dependencies)) copy(dependencies, obj.Dependencies) } diff --git a/states/state_test.go b/states/state_test.go index bff8c4c15de1..2a38a9c53071 100644 --- a/states/state_test.go +++ b/states/state_test.go @@ -141,7 +141,7 @@ func TestStateDeepCopy(t *testing.T) { SchemaVersion: 1, AttrsJSON: []byte(`{"woozles":"confuzles"}`), Private: []byte("private data"), - Dependencies: []addrs.AbsResource{}, + Dependencies: []addrs.ConfigResource{}, }, addrs.AbsProviderConfig{ Provider: addrs.NewDefaultProvider("test"), @@ -159,9 +159,9 @@ func TestStateDeepCopy(t *testing.T) { SchemaVersion: 1, AttrsJSON: []byte(`{"woozles":"confuzles"}`), Private: []byte("private data"), - Dependencies: []addrs.AbsResource{ + Dependencies: []addrs.ConfigResource{ { - Module: addrs.RootModuleInstance, + Module: addrs.RootModule, Resource: addrs.Resource{ Mode: addrs.ManagedResourceMode, Type: "test_thing", diff --git a/states/statefile/version4.go b/states/statefile/version4.go index 050be0ac9308..c49599d8208b 100644 --- a/states/statefile/version4.go +++ b/states/statefile/version4.go @@ -218,14 +218,14 @@ func prepareStateV4(sV4 *stateV4) (*File, tfdiags.Diagnostics) { { depsRaw := isV4.Dependencies - deps := make([]addrs.AbsResource, 0, len(depsRaw)) + deps := make([]addrs.ConfigResource, 0, len(depsRaw)) for _, depRaw := range depsRaw { addr, addrDiags := addrs.ParseAbsResourceStr(depRaw) diags = diags.Append(addrDiags) if addrDiags.HasErrors() { continue } - deps = append(deps, addr) + deps = append(deps, addr.Config()) } obj.Dependencies = deps } diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index c33885e68a87..4e87a4984070 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -298,7 +298,7 @@ func TestContext2Apply_resourceDependsOnModuleStateOnly(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"parent"}`), - Dependencies: []addrs.AbsResource{mustResourceAddr("module.child.aws_instance.child")}, + Dependencies: []addrs.ConfigResource{mustResourceAddr("module.child.aws_instance.child")}, }, mustProviderConfig(`provider["registry.terraform.io/-/aws"]`), ) @@ -1273,7 +1273,7 @@ func testContext2Apply_destroyDependsOn(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"foo"}`), - Dependencies: []addrs.AbsResource{mustResourceAddr("aws_instance.bar")}, + Dependencies: []addrs.ConfigResource{mustResourceAddr("aws_instance.bar")}, }, mustProviderConfig(`provider["registry.terraform.io/-/aws"]`), ) @@ -1329,7 +1329,7 @@ func TestContext2Apply_destroyDependsOnStateOnly(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"foo"}`), - Dependencies: []addrs.AbsResource{}, + Dependencies: []addrs.ConfigResource{}, }, addrs.AbsProviderConfig{ Provider: addrs.NewLegacyProvider("aws"), @@ -1345,14 +1345,14 @@ func TestContext2Apply_destroyDependsOnStateOnly(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"bar"}`), - Dependencies: []addrs.AbsResource{ - addrs.AbsResource{ + Dependencies: []addrs.ConfigResource{ + addrs.ConfigResource{ Resource: addrs.Resource{ Mode: addrs.ManagedResourceMode, Type: "aws_instance", Name: "foo", }, - Module: root.Addr, + Module: root.Addr.Module(), }, }, }, @@ -1427,7 +1427,7 @@ func TestContext2Apply_destroyDependsOnStateOnlyModule(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"foo"}`), - Dependencies: []addrs.AbsResource{}, + Dependencies: []addrs.ConfigResource{}, }, addrs.AbsProviderConfig{ Provider: addrs.NewLegacyProvider("aws"), @@ -1443,14 +1443,14 @@ func TestContext2Apply_destroyDependsOnStateOnlyModule(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"bar"}`), - Dependencies: []addrs.AbsResource{ - addrs.AbsResource{ + Dependencies: []addrs.ConfigResource{ + addrs.ConfigResource{ Resource: addrs.Resource{ Mode: addrs.ManagedResourceMode, Type: "aws_instance", Name: "foo", }, - Module: child.Addr, + Module: child.Addr.Module(), }, }, }, @@ -2708,7 +2708,7 @@ func TestContext2Apply_moduleDestroyOrder(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"b"}`), - Dependencies: []addrs.AbsResource{mustResourceAddr("module.child.aws_instance.a")}, + Dependencies: []addrs.ConfigResource{mustResourceAddr("module.child.aws_instance.a")}, }, mustProviderConfig(`provider["registry.terraform.io/-/aws"]`), ) @@ -3170,8 +3170,8 @@ func TestContext2Apply_moduleProviderAliasTargets(t *testing.T) { }, ), Targets: []addrs.Targetable{ - addrs.AbsResource{ - Module: addrs.RootModuleInstance, + addrs.ConfigResource{ + Module: addrs.RootModule, Resource: addrs.Resource{ Mode: addrs.ManagedResourceMode, Type: "nonexistent", @@ -8025,7 +8025,7 @@ func TestContext2Apply_targetedDestroyCountDeps(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"i-abc123"}`), - Dependencies: []addrs.AbsResource{mustResourceAddr("aws_instance.foo")}, + Dependencies: []addrs.ConfigResource{mustResourceAddr("aws_instance.foo")}, }, mustProviderConfig(`provider["registry.terraform.io/-/aws"]`), ) @@ -8631,14 +8631,14 @@ func TestContext2Apply_createBefore_depends(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"baz","instance":"bar"}`), - Dependencies: []addrs.AbsResource{ - addrs.AbsResource{ + Dependencies: []addrs.ConfigResource{ + addrs.ConfigResource{ Resource: addrs.Resource{ Mode: addrs.ManagedResourceMode, Type: "aws_instance", Name: "web", }, - Module: addrs.RootModuleInstance, + Module: addrs.RootModule, }, }, }, @@ -8764,14 +8764,14 @@ func TestContext2Apply_singleDestroy(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"baz","instance":"bar"}`), - Dependencies: []addrs.AbsResource{ - addrs.AbsResource{ + Dependencies: []addrs.ConfigResource{ + addrs.ConfigResource{ Resource: addrs.Resource{ Mode: addrs.ManagedResourceMode, Type: "aws_instance", Name: "web", }, - Module: addrs.RootModuleInstance, + Module: addrs.RootModule, }, }, }, @@ -10639,22 +10639,22 @@ func TestContext2Apply_cbdCycle(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"a","require_new":"old","foo":"b"}`), - Dependencies: []addrs.AbsResource{ - addrs.AbsResource{ + Dependencies: []addrs.ConfigResource{ + addrs.ConfigResource{ Resource: addrs.Resource{ Mode: addrs.ManagedResourceMode, Type: "test_instance", Name: "b", }, - Module: addrs.RootModuleInstance, + Module: addrs.RootModule, }, - addrs.AbsResource{ + addrs.ConfigResource{ Resource: addrs.Resource{ Mode: addrs.ManagedResourceMode, Type: "test_instance", Name: "c", }, - Module: addrs.RootModuleInstance, + Module: addrs.RootModule, }, }, }, @@ -10672,14 +10672,14 @@ func TestContext2Apply_cbdCycle(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"b","require_new":"old","foo":"c"}`), - Dependencies: []addrs.AbsResource{ - addrs.AbsResource{ + Dependencies: []addrs.ConfigResource{ + addrs.ConfigResource{ Resource: addrs.Resource{ Mode: addrs.ManagedResourceMode, Type: "test_instance", Name: "c", }, - Module: addrs.RootModuleInstance, + Module: addrs.RootModule, }, }, }, diff --git a/terraform/context_refresh_test.go b/terraform/context_refresh_test.go index 099d0af423c2..ad87877e0a48 100644 --- a/terraform/context_refresh_test.go +++ b/terraform/context_refresh_test.go @@ -1985,10 +1985,10 @@ func TestRefresh_updateDependencies(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"foo"}`), - Dependencies: []addrs.AbsResource{ + Dependencies: []addrs.ConfigResource{ // Existing dependencies should not be removed during refresh { - Module: addrs.RootModuleInstance, + Module: addrs.RootModule, Resource: addrs.Resource{ Mode: addrs.ManagedResourceMode, Type: "aws_instance", diff --git a/terraform/eval_state.go b/terraform/eval_state.go index 16655080e9b3..7f626e96ff21 100644 --- a/terraform/eval_state.go +++ b/terraform/eval_state.go @@ -205,7 +205,7 @@ type EvalWriteState struct { // Dependencies are the inter-resource dependencies to be stored in the // state. - Dependencies *[]addrs.AbsResource + Dependencies *[]addrs.ConfigResource } func (n *EvalWriteState) Eval(ctx EvalContext) (interface{}, error) { @@ -538,7 +538,7 @@ type EvalRefreshDependencies struct { // Prior State State **states.ResourceInstanceObject // Dependencies to write to the new state - Dependencies *[]addrs.AbsResource + Dependencies *[]addrs.ConfigResource } func (n *EvalRefreshDependencies) Eval(ctx EvalContext) (interface{}, error) { @@ -548,7 +548,7 @@ func (n *EvalRefreshDependencies) Eval(ctx EvalContext) (interface{}, error) { return nil, nil } - depMap := make(map[string]addrs.AbsResource) + depMap := make(map[string]addrs.ConfigResource) for _, d := range *n.Dependencies { depMap[d.String()] = d } @@ -562,7 +562,7 @@ func (n *EvalRefreshDependencies) Eval(ctx EvalContext) (interface{}, error) { return nil, nil } - deps := make([]addrs.AbsResource, 0, len(depMap)) + deps := make([]addrs.ConfigResource, 0, len(depMap)) for _, d := range depMap { deps = append(deps, d) } diff --git a/terraform/graph_builder_apply_test.go b/terraform/graph_builder_apply_test.go index e755cacbf540..c9a328feb798 100644 --- a/terraform/graph_builder_apply_test.go +++ b/terraform/graph_builder_apply_test.go @@ -104,7 +104,7 @@ func TestApplyGraphBuilder_depCbd(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"B","test_list":["x"]}`), - Dependencies: []addrs.AbsResource{mustResourceAddr("test_object.A")}, + Dependencies: []addrs.ConfigResource{mustResourceAddr("test_object.A")}, }, mustProviderConfig(`provider["registry.terraform.io/-/test"]`), ) @@ -273,7 +273,7 @@ func TestApplyGraphBuilder_destroyStateOnly(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"bar"}`), - Dependencies: []addrs.AbsResource{mustResourceAddr("module.child.test_object.A")}, + Dependencies: []addrs.ConfigResource{mustResourceAddr("module.child.test_object.A")}, }, mustProviderConfig(`provider["registry.terraform.io/-/test"]`), ) @@ -378,7 +378,7 @@ func TestApplyGraphBuilder_moduleDestroy(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"foo","value":"foo"}`), - Dependencies: []addrs.AbsResource{mustResourceAddr("module.A.test_object.foo")}, + Dependencies: []addrs.ConfigResource{mustResourceAddr("module.A.test_object.foo")}, }, mustProviderConfig(`provider["registry.terraform.io/-/test"]`), ) @@ -566,14 +566,14 @@ func TestApplyGraphBuilder_updateFromOrphan(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"b_id","test_string":"a_id"}`), - Dependencies: []addrs.AbsResource{ - addrs.AbsResource{ + Dependencies: []addrs.ConfigResource{ + addrs.ConfigResource{ Resource: addrs.Resource{ Mode: addrs.ManagedResourceMode, Type: "test_object", Name: "a", }, - Module: root.Addr, + Module: root.Addr.Module(), }, }, }, @@ -670,14 +670,14 @@ func TestApplyGraphBuilder_updateFromCBDOrphan(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"b_id","test_string":"a_id"}`), - Dependencies: []addrs.AbsResource{ - addrs.AbsResource{ + Dependencies: []addrs.ConfigResource{ + addrs.ConfigResource{ Resource: addrs.Resource{ Mode: addrs.ManagedResourceMode, Type: "test_object", Name: "a", }, - Module: root.Addr, + Module: root.Addr.Module(), }, }, }, diff --git a/terraform/node_resource_abstract.go b/terraform/node_resource_abstract.go index b6b497814a20..72816dfd1396 100644 --- a/terraform/node_resource_abstract.go +++ b/terraform/node_resource_abstract.go @@ -36,7 +36,7 @@ type GraphNodeResourceInstance interface { // StateDependencies returns any inter-resource dependencies that are // stored in the state. - StateDependencies() []addrs.AbsResource + StateDependencies() []addrs.ConfigResource } // NodeAbstractResource represents a resource that has no associated @@ -99,7 +99,7 @@ type NodeAbstractResourceInstance struct { // interfaces if you're running those transforms, but also be explicitly // set if you already have that information. ResourceState *states.Resource - Dependencies []addrs.AbsResource + Dependencies []addrs.ConfigResource } var ( @@ -142,11 +142,6 @@ func (n *NodeAbstractResourceInstance) Name() string { return n.ResourceInstanceAddr().String() } -//// GraphNodeModuleInstance -//func (n *NodeAbstractResource) Path() addrs.ModuleInstance { -// return n.Addr.Module.UnkeyedInstanceShim() -//} - func (n *NodeAbstractResourceInstance) Path() addrs.ModuleInstance { return n.Addr.Module } @@ -271,7 +266,7 @@ func dottedInstanceAddr(tr addrs.ResourceInstance) string { } // StateDependencies returns the dependencies saved in the state. -func (n *NodeAbstractResourceInstance) StateDependencies() []addrs.AbsResource { +func (n *NodeAbstractResourceInstance) StateDependencies() []addrs.ConfigResource { if rs := n.ResourceState; rs != nil { if s := rs.Instance(n.Addr.Resource.Key); s != nil { if s.Current != nil { diff --git a/terraform/node_resource_apply_instance.go b/terraform/node_resource_apply_instance.go index 5d767dd20668..2a55f23a7ea0 100644 --- a/terraform/node_resource_apply_instance.go +++ b/terraform/node_resource_apply_instance.go @@ -24,7 +24,7 @@ type NodeApplyableResourceInstance struct { *NodeAbstractResourceInstance destroyNode GraphNodeDestroyerCBD - graphNodeDeposer // implementation of GraphNodeDeposer + graphNodeDeposer // implementation of GraphNodeDeposerConfig } var ( @@ -100,12 +100,7 @@ func (n *NodeApplyableResourceInstance) References() []*addrs.Reference { // GraphNodeAttachDependencies func (n *NodeApplyableResourceInstance) AttachDependencies(deps []addrs.ConfigResource) { - var shimmed []addrs.AbsResource - for _, r := range deps { - shimmed = append(shimmed, r.Absolute(r.Module.UnkeyedInstanceShim())) - } - - n.Dependencies = shimmed + n.Dependencies = deps } // GraphNodeEvalable diff --git a/terraform/node_resource_refresh.go b/terraform/node_resource_refresh.go index 4ceac6bbbf79..5b410822ab94 100644 --- a/terraform/node_resource_refresh.go +++ b/terraform/node_resource_refresh.go @@ -19,7 +19,7 @@ type nodeExpandRefreshableManagedResource struct { // We attach dependencies to the Resource during refresh, since the // instances are instantiated during DynamicExpand. - Dependencies []addrs.AbsResource + Dependencies []addrs.ConfigResource } var ( @@ -33,12 +33,7 @@ var ( // GraphNodeAttachDependencies func (n *nodeExpandRefreshableManagedResource) AttachDependencies(deps []addrs.ConfigResource) { - var shimmed []addrs.AbsResource - for _, r := range deps { - shimmed = append(shimmed, r.Absolute(r.Module.UnkeyedInstanceShim())) - } - - n.Dependencies = shimmed + n.Dependencies = deps } func (n *nodeExpandRefreshableManagedResource) References() []*addrs.Reference { @@ -69,7 +64,7 @@ type NodeRefreshableManagedResource struct { // We attach dependencies to the Resource during refresh, since the // instances are instantiated during DynamicExpand. - Dependencies []addrs.AbsResource + Dependencies []addrs.ConfigResource } var ( diff --git a/terraform/terraform_test.go b/terraform/terraform_test.go index edc251b1236f..7251e729c5e9 100644 --- a/terraform/terraform_test.go +++ b/terraform/terraform_test.go @@ -210,12 +210,12 @@ func mustResourceInstanceAddr(s string) addrs.AbsResourceInstance { return addr } -func mustResourceAddr(s string) addrs.AbsResource { +func mustResourceAddr(s string) addrs.ConfigResource { addr, diags := addrs.ParseAbsResourceStr(s) if diags.HasErrors() { panic(diags.Err()) } - return addr + return addr.Config() } func mustProviderConfig(s string) addrs.AbsProviderConfig { diff --git a/terraform/transform_destroy_cbd_test.go b/terraform/transform_destroy_cbd_test.go index 0831d7ad9728..fa63abadbc57 100644 --- a/terraform/transform_destroy_cbd_test.go +++ b/terraform/transform_destroy_cbd_test.go @@ -94,7 +94,7 @@ func TestCBDEdgeTransformer(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"B","test_list":["x"]}`), - Dependencies: []addrs.AbsResource{mustResourceAddr("test_object.A")}, + Dependencies: []addrs.ConfigResource{mustResourceAddr("test_object.A")}, }, mustProviderConfig(`provider["registry.terraform.io/-/test"]`), ) @@ -164,7 +164,7 @@ func TestCBDEdgeTransformerMulti(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"C","test_list":["x"]}`), - Dependencies: []addrs.AbsResource{ + Dependencies: []addrs.ConfigResource{ mustResourceAddr("test_object.A"), mustResourceAddr("test_object.B"), }, @@ -234,7 +234,7 @@ func TestCBDEdgeTransformer_depNonCBDCount(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"B","test_list":["x"]}`), - Dependencies: []addrs.AbsResource{mustResourceAddr("test_object.A")}, + Dependencies: []addrs.ConfigResource{mustResourceAddr("test_object.A")}, }, mustProviderConfig(`provider["registry.terraform.io/-/test"]`), ) @@ -243,7 +243,7 @@ func TestCBDEdgeTransformer_depNonCBDCount(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"B","test_list":["x"]}`), - Dependencies: []addrs.AbsResource{mustResourceAddr("test_object.A")}, + Dependencies: []addrs.ConfigResource{mustResourceAddr("test_object.A")}, }, mustProviderConfig(`provider["registry.terraform.io/-/test"]`), ) @@ -320,7 +320,7 @@ func TestCBDEdgeTransformer_depNonCBDCountBoth(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"B","test_list":["x"]}`), - Dependencies: []addrs.AbsResource{mustResourceAddr("test_object.A")}, + Dependencies: []addrs.ConfigResource{mustResourceAddr("test_object.A")}, }, mustProviderConfig(`provider["registry.terraform.io/-/test"]`), ) @@ -329,7 +329,7 @@ func TestCBDEdgeTransformer_depNonCBDCountBoth(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"B","test_list":["x"]}`), - Dependencies: []addrs.AbsResource{mustResourceAddr("test_object.A")}, + Dependencies: []addrs.ConfigResource{mustResourceAddr("test_object.A")}, }, mustProviderConfig(`provider["registry.terraform.io/-/test"]`), ) diff --git a/terraform/transform_destroy_edge_test.go b/terraform/transform_destroy_edge_test.go index 69b30adb3677..7696c5d36e81 100644 --- a/terraform/transform_destroy_edge_test.go +++ b/terraform/transform_destroy_edge_test.go @@ -28,7 +28,7 @@ func TestDestroyEdgeTransformer_basic(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"B","test_string":"x"}`), - Dependencies: []addrs.AbsResource{mustResourceAddr("test_object.A")}, + Dependencies: []addrs.ConfigResource{mustResourceAddr("test_object.A")}, }, mustProviderConfig(`provider["registry.terraform.io/-/test"]`), ) @@ -72,7 +72,7 @@ func TestDestroyEdgeTransformer_multi(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"B","test_string":"x"}`), - Dependencies: []addrs.AbsResource{mustResourceAddr("test_object.A")}, + Dependencies: []addrs.ConfigResource{mustResourceAddr("test_object.A")}, }, mustProviderConfig(`provider["registry.terraform.io/-/test"]`), ) @@ -81,7 +81,7 @@ func TestDestroyEdgeTransformer_multi(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"C","test_string":"x"}`), - Dependencies: []addrs.AbsResource{ + Dependencies: []addrs.ConfigResource{ mustResourceAddr("test_object.A"), mustResourceAddr("test_object.B"), }, @@ -138,7 +138,7 @@ func TestDestroyEdgeTransformer_module(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"a"}`), - Dependencies: []addrs.AbsResource{mustResourceAddr("module.child.test_object.b")}, + Dependencies: []addrs.ConfigResource{mustResourceAddr("module.child.test_object.b")}, }, mustProviderConfig(`provider["registry.terraform.io/-/test"]`), ) @@ -191,7 +191,7 @@ func TestDestroyEdgeTransformer_moduleOnly(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"b","test_string":"x"}`), - Dependencies: []addrs.AbsResource{mustResourceAddr("module.child.test_object.a")}, + Dependencies: []addrs.ConfigResource{mustResourceAddr("module.child.test_object.a")}, }, mustProviderConfig(`provider["registry.terraform.io/-/test"]`), ) @@ -200,7 +200,7 @@ func TestDestroyEdgeTransformer_moduleOnly(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"c","test_string":"x"}`), - Dependencies: []addrs.AbsResource{ + Dependencies: []addrs.ConfigResource{ mustResourceAddr("module.child.test_object.a"), mustResourceAddr("module.child.test_object.b"), }, From 2474b87ff4cd846804062f04f41856d803676b52 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 23 Mar 2020 16:28:19 -0400 Subject: [PATCH 08/15] remove UnkeyedInstanceShim from some provider nodes Remove the shims where they aren't necessary from the Init and Close provider nodes. This also removed some provider path checks from the builtin eval context, which cannot be resolved since the context may not be created with a ModuleInstance path. --- terraform/eval_context_builtin.go | 24 ++++++------------------ terraform/transform_provider.go | 9 --------- 2 files changed, 6 insertions(+), 27 deletions(-) diff --git a/terraform/eval_context_builtin.go b/terraform/eval_context_builtin.go index 1fbd8718206d..ff3dcf8b5bed 100644 --- a/terraform/eval_context_builtin.go +++ b/terraform/eval_context_builtin.go @@ -118,12 +118,6 @@ func (ctx *BuiltinEvalContext) Input() UIInput { } func (ctx *BuiltinEvalContext) InitProvider(addr addrs.AbsProviderConfig) (providers.Interface, error) { - //if !addr.Module.Equal(ctx.Path().Module()) { - // // This indicates incorrect use of InitProvider: it should be used - // // only from the module that the provider configuration belongs to. - // panic(fmt.Sprintf("%s initialized by wrong module %s", addr, ctx.Path())) - //} - // If we already initialized, it is an error if p := ctx.Provider(addr); p != nil { return nil, fmt.Errorf("%s is already initialized", addr) @@ -159,12 +153,6 @@ func (ctx *BuiltinEvalContext) ProviderSchema(addr addrs.AbsProviderConfig) *Pro } func (ctx *BuiltinEvalContext) CloseProvider(addr addrs.AbsProviderConfig) error { - if !addr.Module.Equal(ctx.Path().Module()) { - // This indicates incorrect use of CloseProvider: it should be used - // only from the module that the provider configuration belongs to. - panic(fmt.Sprintf("%s closed by wrong module %s", addr, ctx.Path())) - } - ctx.ProviderLock.Lock() defer ctx.ProviderLock.Unlock() @@ -227,13 +215,13 @@ func (ctx *BuiltinEvalContext) ProviderInput(pc addrs.AbsProviderConfig) map[str func (ctx *BuiltinEvalContext) SetProviderInput(pc addrs.AbsProviderConfig, c map[string]cty.Value) { absProvider := pc - if !absProvider.Module.Equal(ctx.Path().Module()) { - // This indicates incorrect use of InitProvider: it should be used - // only from the module that the provider configuration belongs to. - panic(fmt.Sprintf("%s initialized by wrong module %s", absProvider, ctx.Path())) - } + //if !absProvider.Module.Equal(ctx.Path().Module()) { + // // This indicates incorrect use of InitProvider: it should be used + // // only from the module that the provider configuration belongs to. + // panic(fmt.Sprintf("%s initialized by wrong module %s", absProvider, ctx.Path())) + //} - if !ctx.Path().IsRoot() { + if !pc.Module.IsRoot() { // Only root module provider configurations can have input. log.Printf("[WARN] BuiltinEvalContext: attempt to SetProviderInput for non-root module") return diff --git a/terraform/transform_provider.go b/terraform/transform_provider.go index be47cb627ebe..31cb8978de8e 100644 --- a/terraform/transform_provider.go +++ b/terraform/transform_provider.go @@ -470,11 +470,6 @@ func (n *graphNodeCloseProvider) Name() string { return n.Addr.String() + " (close)" } -// GraphNodeModuleInstance impl. -func (n *graphNodeCloseProvider) Path() addrs.ModuleInstance { - return n.Addr.Module.UnkeyedInstanceShim() -} - // GraphNodeModulePath func (n *graphNodeCloseProvider) ModulePath() addrs.Module { return n.Addr.Module @@ -534,10 +529,6 @@ func (n *graphNodeProxyProvider) ProviderAddr() addrs.AbsProviderConfig { return n.addr } -func (n *graphNodeProxyProvider) Path() addrs.ModuleInstance { - return n.addr.Module.UnkeyedInstanceShim() -} - func (n *graphNodeProxyProvider) ModulePath() addrs.Module { return n.addr.Module } From 7f0199bab02abf6425129d6f388b2f1ea1b5c66c Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 24 Mar 2020 15:19:12 -0400 Subject: [PATCH 09/15] cleanup some expanders --- terraform/node_data_refresh.go | 8 ++--- terraform/node_data_refresh_test.go | 8 +++-- terraform/node_module_expand.go | 43 +++++++++++++------------ terraform/node_resource_apply.go | 4 +-- terraform/node_resource_plan.go | 22 +++++-------- terraform/node_resource_refresh.go | 14 ++++---- terraform/node_resource_refresh_test.go | 8 +++-- 7 files changed, 55 insertions(+), 52 deletions(-) diff --git a/terraform/node_data_refresh.go b/terraform/node_data_refresh.go index d86e0e037fdf..ee50a24014d4 100644 --- a/terraform/node_data_refresh.go +++ b/terraform/node_data_refresh.go @@ -96,13 +96,13 @@ func (n *NodeRefreshableDataResource) DynamicExpand(ctx EvalContext) (*Graph, er expander := ctx.InstanceExpander() switch { case count >= 0: - expander.SetResourceCount(n.Addr.Module, n.ResourceAddr().Resource, count) + expander.SetResourceCount(n.Addr.Module, n.Addr.Resource, count) case forEachMap != nil: - expander.SetResourceForEach(n.Addr.Module, n.ResourceAddr().Resource, forEachMap) + expander.SetResourceForEach(n.Addr.Module, n.Addr.Resource, forEachMap) default: - expander.SetResourceSingle(n.Addr.Module, n.ResourceAddr().Resource) + expander.SetResourceSingle(n.Addr.Module, n.Addr.Resource) } - instanceAddrs := expander.ExpandResource(n.ResourceAddr().Absolute(path)) + instanceAddrs := expander.ExpandResource(n.Addr) // Our graph transformers require access to the full state, so we'll // temporarily lock it while we work on this. diff --git a/terraform/node_data_refresh_test.go b/terraform/node_data_refresh_test.go index de1b5a787a6f..df3231bc1584 100644 --- a/terraform/node_data_refresh_test.go +++ b/terraform/node_data_refresh_test.go @@ -38,11 +38,13 @@ func TestNodeRefreshableDataResourceDynamicExpand_scaleOut(t *testing.T) { }, }) + addr := addrs.RootModule.Resource(addrs.DataResourceMode, "aws_instance", "foo") n := &NodeRefreshableDataResource{ NodeAbstractResource: &NodeAbstractResource{ - Addr: addrs.RootModule.Resource(addrs.DataResourceMode, "aws_instance", "foo"), + Addr: addr, Config: m.Module.DataResources["data.aws_instance.foo"], }, + Addr: addr.Absolute(addrs.RootModuleInstance), } g, err := n.DynamicExpand(&MockEvalContext{ @@ -118,15 +120,17 @@ func TestNodeRefreshableDataResourceDynamicExpand_scaleIn(t *testing.T) { }, }) + addr := addrs.RootModule.Resource(addrs.DataResourceMode, "aws_instance", "foo") n := &NodeRefreshableDataResource{ NodeAbstractResource: &NodeAbstractResource{ - Addr: addrs.RootModule.Resource(addrs.DataResourceMode, "aws_instance", "foo"), + Addr: addr, Config: m.Module.DataResources["data.aws_instance.foo"], ResolvedProvider: addrs.AbsProviderConfig{ Provider: addrs.NewLegacyProvider("aws"), Module: addrs.RootModule, }, }, + Addr: addr.Absolute(addrs.RootModuleInstance), } g, err := n.DynamicExpand(&MockEvalContext{ diff --git a/terraform/node_module_expand.go b/terraform/node_module_expand.go index 0c62d47cf4cd..c337cdb2aaec 100644 --- a/terraform/node_module_expand.go +++ b/terraform/node_module_expand.go @@ -92,35 +92,36 @@ func (n *evalPrepareModuleExpansion) Eval(ctx EvalContext) (interface{}, error) _, call := n.Addr.Call() - count, countDiags := evaluateResourceCountExpression(n.ModuleCall.Count, ctx) - if countDiags.HasErrors() { - return nil, countDiags.Err() - } + // 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) + 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 - } + 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() - } + forEach, forEachDiags := evaluateResourceForEachExpression(n.ModuleCall.ForEach, ctx) + if forEachDiags.HasErrors() { + return nil, forEachDiags.Err() + } - if forEach != nil { - eachMode = states.EachMap - } + if forEach != nil { + eachMode = states.EachMap + } - // 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 _, path := range expander.ExpandModule(n.Addr.Parent()) { switch eachMode { case states.EachList: - expander.SetModuleCount(path, call, count) + expander.SetModuleCount(module, call, count) case states.EachMap: - expander.SetModuleForEach(path, call, forEach) + expander.SetModuleForEach(module, call, forEach) default: - expander.SetModuleSingle(path, call) + expander.SetModuleSingle(module, call) } } diff --git a/terraform/node_resource_apply.go b/terraform/node_resource_apply.go index 45ae02721aaf..1e53c9cf26d6 100644 --- a/terraform/node_resource_apply.go +++ b/terraform/node_resource_apply.go @@ -9,8 +9,8 @@ import ( ) // nodeExpandApplyableResource handles the first layer of resource -// expansion during apply. This is required because EvalTree does now have a -// context which which to expand the resource into multiple instances. +// expansion during apply. This is required because EvalTree does not have a +// context with which to expand the resource into multiple instances. // This type should be a drop in replacement for NodeApplyableResource, and // needs to mirror any non-evaluation methods exactly. // TODO: We may want to simplify this later by passing EvalContext to EvalTree, diff --git a/terraform/node_resource_plan.go b/terraform/node_resource_plan.go index fc90b7bbf9db..8024fd631910 100644 --- a/terraform/node_resource_plan.go +++ b/terraform/node_resource_plan.go @@ -99,6 +99,10 @@ func (n *NodePlannableResource) Path() addrs.ModuleInstance { return n.Addr.Module } +func (n *NodePlannableResource) Name() string { + return n.Addr.String() +} + // GraphNodeModuleInstance func (n *NodePlannableResource) ModuleInstance() addrs.ModuleInstance { return n.Addr.Module @@ -144,26 +148,16 @@ func (n *NodePlannableResource) ModifyCreateBeforeDestroy(v bool) error { func (n *NodePlannableResource) DynamicExpand(ctx EvalContext) (*Graph, error) { var diags tfdiags.Diagnostics + // We need to potentially rename an instance address in the state + // if we're transitioning whether "count" is set at all. + fixResourceCountSetTransition(ctx, n.Addr.Config(), n.Config.Count != nil) + // Our instance expander should already have been informed about the // expansion of this resource and of all of its containing modules, so // it can tell us which instance addresses we need to process. expander := ctx.InstanceExpander() instanceAddrs := expander.ExpandResource(n.ResourceAddr().Absolute(ctx.Path())) - // We need to potentially rename an instance address in the state - // if we're transitioning whether "count" is set at all. - // - // FIXME: We're re-evaluating count here, even though the InstanceExpander - // has already dealt with our expansion above, because we need it to - // call fixResourceCountSetTransition; the expander API and that function - // are not compatible yet. - count, countDiags := evaluateResourceCountExpression(n.Config.Count, ctx) - diags = diags.Append(countDiags) - if countDiags.HasErrors() { - return nil, diags.Err() - } - fixResourceCountSetTransition(ctx, n.Addr.Config(), count != -1) - // Our graph transformers require access to the full state, so we'll // temporarily lock it while we work on this. state := ctx.State().Lock() diff --git a/terraform/node_resource_refresh.go b/terraform/node_resource_refresh.go index 5b410822ab94..8284e8054c4b 100644 --- a/terraform/node_resource_refresh.go +++ b/terraform/node_resource_refresh.go @@ -97,20 +97,20 @@ func (n *NodeRefreshableManagedResource) DynamicExpand(ctx EvalContext) (*Graph, // Next we need to potentially rename an instance address in the state // if we're transitioning whether "count" is set at all. - fixResourceCountSetTransition(ctx, n.ResourceAddr(), count != -1) + fixResourceCountSetTransition(ctx, n.Addr.Config(), count != -1) // Inform our instance expander about our expansion results above, // and then use it to calculate the instance addresses we'll expand for. expander := ctx.InstanceExpander() switch { case count >= 0: - expander.SetResourceCount(n.Addr.Module, n.ResourceAddr().Resource, count) + expander.SetResourceCount(n.Addr.Module, n.Addr.Resource, count) case forEachMap != nil: - expander.SetResourceForEach(n.Addr.Module, n.ResourceAddr().Resource, forEachMap) + expander.SetResourceForEach(n.Addr.Module, n.Addr.Resource, forEachMap) default: - expander.SetResourceSingle(n.Addr.Module, n.ResourceAddr().Resource) + expander.SetResourceSingle(n.Addr.Module, n.Addr.Resource) } - instanceAddrs := expander.ExpandModuleResource(n.Addr.Module, n.ResourceAddr().Resource) + instanceAddrs := expander.ExpandResource(n.Addr) // Our graph transformers require access to the full state, so we'll // temporarily lock it while we work on this. @@ -136,7 +136,7 @@ func (n *NodeRefreshableManagedResource) DynamicExpand(ctx EvalContext) (*Graph, &ResourceCountTransformer{ Concrete: concreteResource, Schema: n.Schema, - Addr: n.ResourceAddr(), + Addr: n.Addr.Config(), InstanceAddrs: instanceAddrs, }, @@ -144,7 +144,7 @@ func (n *NodeRefreshableManagedResource) DynamicExpand(ctx EvalContext) (*Graph, // during a scale in. &OrphanResourceCountTransformer{ Concrete: concreteResource, - Addr: n.ResourceAddr(), + Addr: n.Addr.Config(), InstanceAddrs: instanceAddrs, State: state, }, diff --git a/terraform/node_resource_refresh_test.go b/terraform/node_resource_refresh_test.go index d1b0e4c7955f..f0ff42de21b9 100644 --- a/terraform/node_resource_refresh_test.go +++ b/terraform/node_resource_refresh_test.go @@ -40,11 +40,13 @@ func TestNodeRefreshableManagedResourceDynamicExpand_scaleOut(t *testing.T) { }, }).SyncWrapper() + cfgAddr := addrs.RootModule.Resource(addrs.ManagedResourceMode, "aws_instance", "foo") n := &NodeRefreshableManagedResource{ NodeAbstractResource: &NodeAbstractResource{ - Addr: addrs.RootModule.Resource(addrs.ManagedResourceMode, "aws_instance", "foo"), + Addr: cfgAddr, Config: m.Module.ManagedResources["aws_instance.foo"], }, + Addr: cfgAddr.Absolute(addrs.RootModuleInstance), } g, err := n.DynamicExpand(&MockEvalContext{ @@ -120,11 +122,13 @@ func TestNodeRefreshableManagedResourceDynamicExpand_scaleIn(t *testing.T) { }, }).SyncWrapper() + cfgAddr := addrs.RootModule.Resource(addrs.ManagedResourceMode, "aws_instance", "foo") n := &NodeRefreshableManagedResource{ NodeAbstractResource: &NodeAbstractResource{ - Addr: addrs.RootModule.Resource(addrs.ManagedResourceMode, "aws_instance", "foo"), + Addr: cfgAddr, Config: m.Module.ManagedResources["aws_instance.foo"], }, + Addr: cfgAddr.Absolute(addrs.RootModuleInstance), } g, err := n.DynamicExpand(&MockEvalContext{ From 8eb3f2cf52d72fcbe30d5bdf6a2ff4c2a234f0ee Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 25 Mar 2020 15:07:35 -0400 Subject: [PATCH 10/15] orphan resources needs to use AbsResource The expand logic was separated into nodeExpandRefreshableManagedResource, but the orphan logic wasn't updated. --- terraform/node_data_refresh.go | 2 +- terraform/node_resource_plan.go | 2 +- terraform/node_resource_refresh.go | 2 +- terraform/transform_orphan_count.go | 42 ++++++++++++++--------------- 4 files changed, 23 insertions(+), 25 deletions(-) diff --git a/terraform/node_data_refresh.go b/terraform/node_data_refresh.go index ee50a24014d4..5de33c66a4b0 100644 --- a/terraform/node_data_refresh.go +++ b/terraform/node_data_refresh.go @@ -147,7 +147,7 @@ func (n *NodeRefreshableDataResource) DynamicExpand(ctx EvalContext) (*Graph, er // directly as NodeDestroyableDataResource. &OrphanResourceCountTransformer{ Concrete: concreteResourceDestroyable, - Addr: n.ResourceAddr(), + Addr: n.Addr, InstanceAddrs: instanceAddrs, State: state, }, diff --git a/terraform/node_resource_plan.go b/terraform/node_resource_plan.go index 8024fd631910..1fcc09f03335 100644 --- a/terraform/node_resource_plan.go +++ b/terraform/node_resource_plan.go @@ -209,7 +209,7 @@ func (n *NodePlannableResource) DynamicExpand(ctx EvalContext) (*Graph, error) { // Add the count/for_each orphans &OrphanResourceCountTransformer{ Concrete: concreteResourceOrphan, - Addr: n.ResourceAddr(), + Addr: n.Addr, InstanceAddrs: instanceAddrs, State: state, }, diff --git a/terraform/node_resource_refresh.go b/terraform/node_resource_refresh.go index 8284e8054c4b..98603454df84 100644 --- a/terraform/node_resource_refresh.go +++ b/terraform/node_resource_refresh.go @@ -144,7 +144,7 @@ func (n *NodeRefreshableManagedResource) DynamicExpand(ctx EvalContext) (*Graph, // during a scale in. &OrphanResourceCountTransformer{ Concrete: concreteResource, - Addr: n.Addr.Config(), + Addr: n.Addr, InstanceAddrs: instanceAddrs, State: state, }, diff --git a/terraform/transform_orphan_count.go b/terraform/transform_orphan_count.go index ed3dc8f15cc7..8416b546230e 100644 --- a/terraform/transform_orphan_count.go +++ b/terraform/transform_orphan_count.go @@ -18,39 +18,37 @@ import ( type OrphanResourceCountTransformer struct { Concrete ConcreteResourceInstanceNodeFunc - Addr addrs.ConfigResource // Addr of the resource to look for orphans + Addr addrs.AbsResource // Addr of the resource to look for orphans InstanceAddrs []addrs.AbsResourceInstance // Addresses that currently exist in config State *states.State // Full global state } func (t *OrphanResourceCountTransformer) Transform(g *Graph) error { - resources := t.State.Resources(t.Addr) - if len(resources) == 0 { + rs := t.State.Resource(t.Addr) + if rs == nil { return nil // Resource doesn't exist in state, so nothing to do! } - for _, rs := range resources { - // This is an O(n*m) analysis, which we accept for now because the - // number of instances of a single resource ought to always be small in any - // reasonable Terraform configuration. - Have: - for key := range rs.Instances { - thisAddr := rs.Addr.Instance(key) - for _, wantAddr := range t.InstanceAddrs { - if wantAddr.Equal(thisAddr) { - continue Have - } + // This is an O(n*m) analysis, which we accept for now because the + // number of instances of a single resource ought to always be small in any + // reasonable Terraform configuration. +Have: + for key := range rs.Instances { + thisAddr := rs.Addr.Instance(key) + for _, wantAddr := range t.InstanceAddrs { + if wantAddr.Equal(thisAddr) { + continue Have } - // If thisAddr is not in t.InstanceAddrs then we've found an "orphan" + } + // If thisAddr is not in t.InstanceAddrs then we've found an "orphan" - abstract := NewNodeAbstractResourceInstance(thisAddr) - var node dag.Vertex = abstract - if f := t.Concrete; f != nil { - node = f(abstract) - } - log.Printf("[TRACE] OrphanResourceCountTransformer: adding %s as %T", thisAddr, node) - g.Add(node) + abstract := NewNodeAbstractResourceInstance(thisAddr) + var node dag.Vertex = abstract + if f := t.Concrete; f != nil { + node = f(abstract) } + log.Printf("[TRACE] OrphanResourceCountTransformer: adding %s as %T", thisAddr, node) + g.Add(node) } return nil From cd045f6f4e171c283e0778149771d7d3dbb32a6d Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 25 Mar 2020 15:16:33 -0400 Subject: [PATCH 11/15] enable count and for_each in configuration --- configs/module_call.go | 16 ---------------- configs/module_call_test.go | 2 -- 2 files changed, 18 deletions(-) diff --git a/configs/module_call.go b/configs/module_call.go index a484ffef9c0b..83d41d5f5aa5 100644 --- a/configs/module_call.go +++ b/configs/module_call.go @@ -68,26 +68,10 @@ func decodeModuleBlock(block *hcl.Block, override bool) (*ModuleCall, hcl.Diagno if attr, exists := content.Attributes["count"]; exists { mc.Count = attr.Expr - - // We currently parse this, but don't yet do anything with it. - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Reserved argument name in module block", - Detail: fmt.Sprintf("The name %q is reserved for use in a future version of Terraform.", attr.Name), - Subject: &attr.NameRange, - }) } if attr, exists := content.Attributes["for_each"]; exists { mc.ForEach = attr.Expr - - // We currently parse this, but don't yet do anything with it. - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Reserved argument name in module block", - Detail: fmt.Sprintf("The name %q is reserved for use in a future version of Terraform.", attr.Name), - Subject: &attr.NameRange, - }) } if attr, exists := content.Attributes["depends_on"]; exists { diff --git a/configs/module_call_test.go b/configs/module_call_test.go index 983798db5d0a..0b4b7371f215 100644 --- a/configs/module_call_test.go +++ b/configs/module_call_test.go @@ -20,8 +20,6 @@ func TestLoadModuleCall(t *testing.T) { file, diags := parser.LoadConfigFile("module-calls.tf") assertExactDiagnostics(t, diags, []string{ - `module-calls.tf:19,3-8: Reserved argument name in module block; The name "count" is reserved for use in a future version of Terraform.`, - `module-calls.tf:20,3-11: Reserved argument name in module block; The name "for_each" is reserved for use in a future version of Terraform.`, `module-calls.tf:22,3-13: Reserved argument name in module block; The name "depends_on" is reserved for use in a future version of Terraform.`, }) From 04a117b2a1aecb15c5cd41fcf63991dc37860705 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 25 Mar 2020 15:29:34 -0400 Subject: [PATCH 12/15] module expansion test simplify the test a bit and add a few more combinations to the config --- terraform/context_plan_test.go | 62 ++++++------------- .../child/main.tf | 0 .../main.tf | 18 +++--- 3 files changed, 29 insertions(+), 51 deletions(-) rename terraform/testdata/{plan-modules-count => plan-modules-expand}/child/main.tf (100%) rename terraform/testdata/{plan-modules-count => plan-modules-expand}/main.tf (51%) diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index 09c8e78d8694..9fb9820c8029 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -429,12 +429,9 @@ func TestContext2Plan_modules(t *testing.T) { checkVals(t, expected, ric.After) } } -func TestContext2Plan_moduleCount(t *testing.T) { - // This test is skipped with count disabled. - t.Skip() - //FIXME: add for_each and single modules to this test - - m := testModule(t, "plan-modules-count") +func TestContext2Plan_moduleExpand(t *testing.T) { + // Test a smattering of plan expansion behavior + m := testModule(t, "plan-modules-expand") p := testProvider("aws") p.DiffFn = testDiffFn ctx := testContext2(t, &ContextOpts{ @@ -451,31 +448,18 @@ func TestContext2Plan_moduleCount(t *testing.T) { t.Fatalf("unexpected errors: %s", diags.Err()) } - if len(plan.Changes.Resources) != 6 { - t.Error("expected 6 resource in plan, got", len(plan.Changes.Resources)) - } - schema := p.GetSchemaReturn.ResourceTypes["aws_instance"] ty := schema.ImpliedType() - expectFoo := objectVal(t, schema, map[string]cty.Value{ - "id": cty.UnknownVal(cty.String), - "foo": cty.StringVal("2"), - "type": cty.StringVal("aws_instance")}, - ) - - expectNum := objectVal(t, schema, map[string]cty.Value{ - "id": cty.UnknownVal(cty.String), - "num": cty.NumberIntVal(2), - "type": cty.StringVal("aws_instance"), - }) - - expectExpansion := objectVal(t, schema, map[string]cty.Value{ - "bar": cty.StringVal("baz"), - "id": cty.UnknownVal(cty.String), - "num": cty.NumberIntVal(2), - "type": cty.StringVal("aws_instance"), - }) + expected := map[string]struct{}{ + `aws_instance.foo["a"]`: struct{}{}, + `module.count_child[1].aws_instance.foo[0]`: struct{}{}, + `module.count_child[1].aws_instance.foo[1]`: struct{}{}, + `module.count_child[0].aws_instance.foo[0]`: struct{}{}, + `module.count_child[0].aws_instance.foo[1]`: struct{}{}, + `module.for_each_child["a"].aws_instance.foo[1]`: struct{}{}, + `module.for_each_child["a"].aws_instance.foo[0]`: struct{}{}, + } for _, res := range plan.Changes.Resources { if res.Action != plans.Create { @@ -486,22 +470,14 @@ func TestContext2Plan_moduleCount(t *testing.T) { t.Fatal(err) } - var expected cty.Value - switch i := ric.Addr.String(); i { - case "aws_instance.bar": - expected = expectFoo - case "aws_instance.foo": - expected = expectNum - case "module.child[0].aws_instance.foo[0]", - "module.child[0].aws_instance.foo[1]", - "module.child[1].aws_instance.foo[0]", - "module.child[1].aws_instance.foo[1]": - expected = expectExpansion - default: - t.Fatal("unknown instance:", i) + _, ok := expected[ric.Addr.String()] + if !ok { + t.Fatal("unexpected resource:", ric.Addr.String()) } - - checkVals(t, expected, ric.After) + delete(expected, ric.Addr.String()) + } + for addr := range expected { + t.Error("missing resource", addr) } } diff --git a/terraform/testdata/plan-modules-count/child/main.tf b/terraform/testdata/plan-modules-expand/child/main.tf similarity index 100% rename from terraform/testdata/plan-modules-count/child/main.tf rename to terraform/testdata/plan-modules-expand/child/main.tf diff --git a/terraform/testdata/plan-modules-count/main.tf b/terraform/testdata/plan-modules-expand/main.tf similarity index 51% rename from terraform/testdata/plan-modules-count/main.tf rename to terraform/testdata/plan-modules-expand/main.tf index eeb5fa0015bf..8cb4d96a9fb5 100644 --- a/terraform/testdata/plan-modules-count/main.tf +++ b/terraform/testdata/plan-modules-expand/main.tf @@ -1,6 +1,9 @@ locals { val = 2 bar = "baz" + m = { + "a" = "b" + } } variable "myvar" { @@ -8,21 +11,20 @@ variable "myvar" { } -module "child" { +module "count_child" { count = local.val foo = 2 bar = var.myvar source = "./child" } -output "out" { - value = module.child[*].out +module "for_each_child" { + for_each = aws_instance.foo + foo = 2 + bar = var.myvar + source = "./child" } resource "aws_instance" "foo" { - num = 2 -} - -resource "aws_instance" "bar" { - foo = "${aws_instance.foo.num}" + for_each = local.m } From 5810261add27ca589d843ee431b42267bdcfd9a0 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 25 Mar 2020 15:39:51 -0400 Subject: [PATCH 13/15] don't log path in EvalRaw eval nodes no longer always have a context path --- terraform/eval.go | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/terraform/eval.go b/terraform/eval.go index 51e89d8b5d23..2a8909aee521 100644 --- a/terraform/eval.go +++ b/terraform/eval.go @@ -45,28 +45,16 @@ func Eval(n EvalNode, ctx EvalContext) (interface{}, error) { // EvalRaw is like Eval except that it returns all errors, even if they // signal something normal such as EvalEarlyExitError. func EvalRaw(n EvalNode, ctx EvalContext) (interface{}, error) { - path := "unknown" - - // FIXME: restore the path here somehow or log this in another manner - // We cannot call Path here, since the context may not yet have the path - // set. - //if ctx != nil { - // path = ctx.Path().String() - //} - //if path == "" { - // path = "" - //} - - log.Printf("[TRACE] %s: eval: %T", path, n) + log.Printf("[TRACE] eval: %T", n) output, err := n.Eval(ctx) if err != nil { switch err.(type) { case EvalEarlyExitError: - log.Printf("[TRACE] %s: eval: %T, early exit err: %s", path, n, err) + log.Printf("[TRACE] eval: %T, early exit err: %s", n, err) case tfdiags.NonFatalError: - log.Printf("[WARN] %s: eval: %T, non-fatal err: %s", path, n, err) + log.Printf("[WARN] eval: %T, non-fatal err: %s", n, err) default: - log.Printf("[ERROR] %s: eval: %T, err: %s", path, n, err) + log.Printf("[ERROR] eval: %T, err: %s", n, err) } } From 4f1692cfaf5c9eb5010c3a31b9921de40076eed2 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 25 Mar 2020 15:53:54 -0400 Subject: [PATCH 14/15] comment updates --- terraform/eval_context_builtin.go | 6 ------ terraform/node_resource_apply.go | 2 -- terraform/node_resource_plan.go | 9 +-------- terraform/node_resource_refresh.go | 5 +++++ 4 files changed, 6 insertions(+), 16 deletions(-) diff --git a/terraform/eval_context_builtin.go b/terraform/eval_context_builtin.go index ff3dcf8b5bed..755d2d8f3ce1 100644 --- a/terraform/eval_context_builtin.go +++ b/terraform/eval_context_builtin.go @@ -215,12 +215,6 @@ func (ctx *BuiltinEvalContext) ProviderInput(pc addrs.AbsProviderConfig) map[str func (ctx *BuiltinEvalContext) SetProviderInput(pc addrs.AbsProviderConfig, c map[string]cty.Value) { absProvider := pc - //if !absProvider.Module.Equal(ctx.Path().Module()) { - // // This indicates incorrect use of InitProvider: it should be used - // // only from the module that the provider configuration belongs to. - // panic(fmt.Sprintf("%s initialized by wrong module %s", absProvider, ctx.Path())) - //} - if !pc.Module.IsRoot() { // Only root module provider configurations can have input. log.Printf("[WARN] BuiltinEvalContext: attempt to SetProviderInput for non-root module") diff --git a/terraform/node_resource_apply.go b/terraform/node_resource_apply.go index 1e53c9cf26d6..6b4250a30a80 100644 --- a/terraform/node_resource_apply.go +++ b/terraform/node_resource_apply.go @@ -13,8 +13,6 @@ import ( // context with which to expand the resource into multiple instances. // This type should be a drop in replacement for NodeApplyableResource, and // needs to mirror any non-evaluation methods exactly. -// TODO: We may want to simplify this later by passing EvalContext to EvalTree, -// and returning an EvalEquence. type nodeExpandApplyableResource struct { *NodeAbstractResource } diff --git a/terraform/node_resource_plan.go b/terraform/node_resource_plan.go index 1fcc09f03335..d78b451f13d5 100644 --- a/terraform/node_resource_plan.go +++ b/terraform/node_resource_plan.go @@ -11,17 +11,10 @@ import ( // nodeExpandPlannableResource handles the first layer of resource // expansion. We need this extra layer so DynamicExpand is called twice for // the resource, the first to expand the Resource for each module instance, and -// the second to expand each ResourceInstnace for the expanded Resources. -// -// Even though the Expander can handle this recursive expansion, the -// EvalWriteState nodes need to be expanded and Evaluated first, and our -// current graph doesn't allow evaluation within DynamicExpand, and doesn't -// call it recursively. +// the second to expand each ResourceInstance for the expanded Resources. type nodeExpandPlannableResource struct { *NodeAbstractResource - // TODO: can we eliminate the need for this flag and combine this with the - // apply expander? // ForceCreateBeforeDestroy might be set via our GraphNodeDestroyerCBD // during graph construction, if dependencies require us to force this // on regardless of what the configuration says. diff --git a/terraform/node_resource_refresh.go b/terraform/node_resource_refresh.go index 98603454df84..e874f47054ae 100644 --- a/terraform/node_resource_refresh.go +++ b/terraform/node_resource_refresh.go @@ -14,6 +14,11 @@ import ( "github.com/hashicorp/terraform/tfdiags" ) +// nodeExpandRefreshableResource handles the first layer of resource +// expansion durin refresh. We need this extra layer so DynamicExpand is called +// twice for the resource, the first to expand the Resource for each module +// instance, and the second to expand each ResourceInstance for the expanded +// Resources. type nodeExpandRefreshableManagedResource struct { *NodeAbstractResource From cec989d66045cf693e8504cfaee27e7271a3e137 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 26 Mar 2020 11:52:41 -0400 Subject: [PATCH 15/15] comment update and remove extra Name method This name method won't be called in the full graph, and remove it to prevent confusion with the parent node in logs. --- terraform/node_resource_apply.go | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/terraform/node_resource_apply.go b/terraform/node_resource_apply.go index 6b4250a30a80..88a378851e54 100644 --- a/terraform/node_resource_apply.go +++ b/terraform/node_resource_apply.go @@ -9,10 +9,9 @@ import ( ) // nodeExpandApplyableResource handles the first layer of resource -// expansion during apply. This is required because EvalTree does not have a -// context with which to expand the resource into multiple instances. -// This type should be a drop in replacement for NodeApplyableResource, and -// needs to mirror any non-evaluation methods exactly. +// expansion during apply. Even though the resource instances themselves are +// already expanded from the plan, we still need to expand the +// NodeApplyableResource nodes into their respective modules. type nodeExpandApplyableResource struct { *NodeAbstractResource } @@ -74,10 +73,6 @@ func (n *NodeApplyableResource) Path() addrs.ModuleInstance { return n.Addr.Module } -func (n *NodeApplyableResource) Name() string { - return n.NodeAbstractResource.Name() + " (prepare state)" -} - func (n *NodeApplyableResource) References() []*addrs.Reference { if n.Config == nil { log.Printf("[WARN] NodeApplyableResource %q: no configuration, so can't determine References", dag.VertexName(n))