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 48ed3533a..51e89d8b5 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 a682b3d6c..241662f7b 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 746b00fe1..b5e400be4 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 210a40d80..15bf80569 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 373819f7e..4c9f2f0c2 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 e980e0c6d..706b7e0ab 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 d53ebe97a..5025c98b6 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 | 36 ++++++------- 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, 101 insertions(+), 33 deletions(-) diff --git a/terraform/eval_context_builtin_test.go b/terraform/eval_context_builtin_test.go index bdcd09484..c6d27d498 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 181e55635..f1a195f6d 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 8f134d465..16655080e 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 } + // 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) - // 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) - } + 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 cbf3d2670..baeda03f5 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 6aa91da87..ce9fc0d22 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 060f27ea7..bb13234b6 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 2423ad04b..b323db732 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 73e059a64..df057b468 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 7eb8e4884..d594b9f1d 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 5aaf6c072..daafa982a 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 b323db732..378900d1c 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 e3aedab56..7c926a070 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 1af564b0c..a31936796 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 16c327fd1..3e280d16f 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 284fa9168..95606dd71 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 48157e038..b6aada940 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 19a141b52..2fbe2022a 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 9872a704d..482435057 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 b5e400be4..1fbd87182 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 0d323bfda..e755cacbf 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 a31936796..b6b497814 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 378900d1c..45ae02721 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 3e280d16f..c9e21e597 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 df057b468..fc90b7bbf 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 f516aaecf..d1b0e4c79 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 83a67ed0c..0228e3d1c 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 2fbe2022a..69b30adb3 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 | 61 ++++++++++++++----- terraform/node_resource_refresh.go | 78 +++++++++++++++++++------ terraform/transform_provider.go | 2 +- 5 files changed, 111 insertions(+), 38 deletions(-) diff --git a/terraform/graph_builder_refresh.go b/terraform/graph_builder_refresh.go index 587b90677..8bf4b78a7 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 35068c841..23aa4aacc 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 f6c3f6d92..d86e0e037 100644 --- a/terraform/node_data_refresh.go +++ b/terraform/node_data_refresh.go @@ -10,12 +10,45 @@ import ( "github.com/zclconf/go-cty/cty" ) -// NodeRefreshableDataResource represents a resource that is "refreshable". -type NodeRefreshableDataResource struct { +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 68a087708..4ceac6bbb 100644 --- a/terraform/node_resource_refresh.go +++ b/terraform/node_resource_refresh.go @@ -14,9 +14,7 @@ import ( "github.com/hashicorp/terraform/tfdiags" ) -// NodeRefreshableManagedResource represents a resource that is expandable into -// NodeRefreshableManagedResourceInstance. Resource count orphans are also added. -type NodeRefreshableManagedResource struct { +type nodeExpandRefreshableManagedResource struct { *NodeAbstractResource // We attach dependencies to the Resource during refresh, since the @@ -25,16 +23,16 @@ type NodeRefreshableManagedResource struct { } var ( - _ GraphNodeDynamicExpandable = (*NodeRefreshableManagedResource)(nil) - _ GraphNodeReferenceable = (*NodeRefreshableManagedResource)(nil) - _ GraphNodeReferencer = (*NodeRefreshableManagedResource)(nil) - _ GraphNodeConfigResource = (*NodeRefreshableManagedResource)(nil) - _ GraphNodeAttachResourceConfig = (*NodeRefreshableManagedResource)(nil) - _ GraphNodeAttachDependencies = (*NodeRefreshableManagedResource)(nil) + _ GraphNodeDynamicExpandable = (*nodeExpandRefreshableManagedResource)(nil) + _ GraphNodeReferenceable = (*nodeExpandRefreshableManagedResource)(nil) + _ GraphNodeReferencer = (*nodeExpandRefreshableManagedResource)(nil) + _ GraphNodeConfigResource = (*nodeExpandRefreshableManagedResource)(nil) + _ GraphNodeAttachResourceConfig = (*nodeExpandRefreshableManagedResource)(nil) + _ GraphNodeAttachDependencies = (*nodeExpandRefreshableManagedResource)(nil) ) // GraphNodeAttachDependencies -func (n *NodeRefreshableManagedResource) AttachDependencies(deps []addrs.ConfigResource) { +func (n *nodeExpandRefreshableManagedResource) AttachDependencies(deps []addrs.ConfigResource) { var shimmed []addrs.AbsResource for _, r := range deps { shimmed = append(shimmed, r.Absolute(r.Module.UnkeyedInstanceShim())) @@ -43,6 +41,50 @@ func (n *NodeRefreshableManagedResource) AttachDependencies(deps []addrs.ConfigR 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) +) + +func (n *NodeRefreshableManagedResource) Path() addrs.ModuleInstance { + return n.Addr.Module +} + // GraphNodeDynamicExpandable func (n *NodeRefreshableManagedResource) DynamicExpand(ctx EvalContext) (*Graph, error) { var diags tfdiags.Diagnostics @@ -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 6ca2b475b..be47cb627 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 784c3029b..d946a104d 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 11632df93..81a5b41d7 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 c0a94fc24..1ccaa8360 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 50e1c32ca..2e81933f4 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 95bc6984b..e84aca6f1 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 1642b4569..0a6454990 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 ff56bcfb8..c8f92bd0e 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 89082fb8e..1c5aee0a9 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 bff8c4c15..2a38a9c53 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 050be0ac9..c49599d82 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 c33885e68..4e87a4984 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 099d0af42..ad87877e0 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 16655080e..7f626e96f 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 e755cacbf..c9a328feb 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 b6b497814..72816dfd1 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 5d767dd20..2a55f23a7 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 4ceac6bbb..5b410822a 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 edc251b12..7251e729c 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 0831d7ad9..fa63abadb 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 69b30adb3..7696c5d36 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 1fbd87182..ff3dcf8b5 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 be47cb627..31cb8978d 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 | 45 +++++++++++++------------ 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, 56 insertions(+), 53 deletions(-) diff --git a/terraform/node_data_refresh.go b/terraform/node_data_refresh.go index d86e0e037..ee50a2401 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 de1b5a787..df3231bc1 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 0c62d47cf..c337cdb2a 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() - } - - if count >= 0 { // -1 signals "count not set" - eachMode = states.EachList - } - - forEach, forEachDiags := evaluateResourceForEachExpression(n.ModuleCall.ForEach, ctx) - if forEachDiags.HasErrors() { - return nil, forEachDiags.Err() - } - - if forEach != nil { - eachMode = states.EachMap - } - // 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()) { + for _, module := range expander.ExpandModule(n.Addr.Parent()) { + ctx = ctx.WithPath(module) + count, countDiags := evaluateResourceCountExpression(n.ModuleCall.Count, ctx) + if countDiags.HasErrors() { + return nil, countDiags.Err() + } + + if count >= 0 { // -1 signals "count not set" + eachMode = states.EachList + } + + forEach, forEachDiags := evaluateResourceForEachExpression(n.ModuleCall.ForEach, ctx) + if forEachDiags.HasErrors() { + return nil, forEachDiags.Err() + } + + if forEach != nil { + eachMode = states.EachMap + } + switch eachMode { case states.EachList: - 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 45ae02721..1e53c9cf2 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 fc90b7bbf..8024fd631 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 5b410822a..8284e8054 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 d1b0e4c79..f0ff42de2 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 | 44 ++++++++++++++--------------- 4 files changed, 24 insertions(+), 26 deletions(-) diff --git a/terraform/node_data_refresh.go b/terraform/node_data_refresh.go index ee50a2401..5de33c66a 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 8024fd631..1fcc09f03 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 8284e8054..98603454d 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 ed3dc8f15..8416b5462 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" - - 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) } + // 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) } 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 a484ffef9..83d41d5f5 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 983798db5..0b4b7371f 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 09c8e78d8..9fb9820c8 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 eeb5fa001..8cb4d96a9 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 51e89d8b5..2a8909aee 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 ff3dcf8b5..755d2d8f3 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 1e53c9cf2..6b4250a30 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 1fcc09f03..d78b451f1 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 98603454d..e874f4705 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 6b4250a30..88a378851 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))