From 0986d0122302e89ec88d4258751f9c69f79c77a4 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 31 Oct 2017 13:13:24 -0400 Subject: [PATCH 01/16] add providers directly from the configuration The first step in only using the required provider nodes in a graph is to be able to specifically add them from the configuration. The MissingProviderTransformer was previously responsible for adding all providers. Now it is really just adding any that are missing from the config. --- terraform/graph_builder_apply.go | 7 +++ terraform/graph_builder_plan.go | 7 +++ terraform/transform_config.go | 75 ++++++++++++++++++++++++++++++++ terraform/transform_provider.go | 30 +++++++------ 4 files changed, 105 insertions(+), 14 deletions(-) diff --git a/terraform/graph_builder_apply.go b/terraform/graph_builder_apply.go index 430195c7f2..1b898926e3 100644 --- a/terraform/graph_builder_apply.go +++ b/terraform/graph_builder_apply.go @@ -87,6 +87,13 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { // Attach the state &AttachStateTransformer{State: b.State}, + // add configured providers + &ProviderConfigTransformer{ + Module: b.Module, + Providers: b.Providers, + Concrete: concreteProvider, + }, + // Create all the providers &MissingProviderTransformer{Providers: b.Providers, Concrete: concreteProvider}, &ProviderTransformer{}, diff --git a/terraform/graph_builder_plan.go b/terraform/graph_builder_plan.go index 429e424815..1b93a5ab38 100644 --- a/terraform/graph_builder_plan.go +++ b/terraform/graph_builder_plan.go @@ -93,6 +93,13 @@ func (b *PlanGraphBuilder) Steps() []GraphTransformer { // Add root variables &RootVariableTransformer{Module: b.Module}, + // add configured providers + &ProviderConfigTransformer{ + Module: b.Module, + Providers: b.Providers, + Concrete: b.ConcreteProvider, + }, + // Create all the providers &MissingProviderTransformer{Providers: b.Providers, Concrete: b.ConcreteProvider}, &ProviderTransformer{}, diff --git a/terraform/transform_config.go b/terraform/transform_config.go index 61bce8532a..7ec7744a7d 100644 --- a/terraform/transform_config.go +++ b/terraform/transform_config.go @@ -133,3 +133,78 @@ func (t *ConfigTransformer) transformSingle(g *Graph, m *module.Tree) error { return nil } + +type ProviderConfigTransformer struct { + Providers []string + Concrete ConcreteProviderNodeFunc + + // Module is the module to add resources from. + Module *module.Tree +} + +func (t *ProviderConfigTransformer) Transform(g *Graph) error { + // If no module is given, we don't do anything + if t.Module == nil { + return nil + } + + // If the module isn't loaded, that is simply an error + if !t.Module.Loaded() { + return errors.New("module must be loaded for ProviderConfigTransformer") + } + + // Start the transformation process + return t.transform(g, t.Module) +} + +func (t *ProviderConfigTransformer) transform(g *Graph, m *module.Tree) error { + // If no config, do nothing + if m == nil { + return nil + } + + // Add our resources + if err := t.transformSingle(g, m); err != nil { + return err + } + + // Transform all the children. + for _, c := range m.Children() { + if err := t.transform(g, c); err != nil { + return err + } + } + + return nil +} + +func (t *ProviderConfigTransformer) transformSingle(g *Graph, m *module.Tree) error { + log.Printf("[TRACE] ProviderConfigTransformer: Starting for path: %v", m.Path()) + + // Get the configuration for this module + conf := m.Config() + + // Build the path we're at + path := m.Path() + if len(path) > 0 { + path = append([]string{RootModuleName}, path...) + } + + // Write all the resources out + for _, p := range conf.ProviderConfigs { + name := p.Name + if p.Alias != "" { + name += "." + p.Alias + } + + v := t.Concrete(&NodeAbstractProvider{ + NameValue: name, + PathValue: path, + }).(dag.Vertex) + + // Add it to the graph + g.Add(v) + } + + return nil +} diff --git a/terraform/transform_provider.go b/terraform/transform_provider.go index b9695d5242..baf3016b9c 100644 --- a/terraform/transform_provider.go +++ b/terraform/transform_provider.go @@ -104,10 +104,10 @@ func (t *CloseProviderTransformer) Transform(g *Graph) error { return err } -// MissingProviderTransformer is a GraphTransformer that adds nodes -// for missing providers into the graph. Specifically, it creates provider -// configuration nodes for all the providers that we support. These are -// pruned later during an optimization pass. +// MissingProviderTransformer is a GraphTransformer that adds nodes for all +// required providers into the graph. Specifically, it creates provider +// configuration nodes for all the providers that we support. These are pruned +// later during an optimization pass. type MissingProviderTransformer struct { // Providers is the list of providers we support. Providers []string @@ -160,6 +160,18 @@ func (t *MissingProviderTransformer) Transform(g *Graph) error { } for _, p := range pv.ProvidedBy() { + // always add the parent nodes to check, since configured providers + // may have already been added for modules. + if len(path) > 0 { + // We'll need the parent provider as well, so let's + // add a dummy node to check to make sure that we add + // that parent provider. + check = append(check, &graphNodeProviderConsumerDummy{ + ProviderValue: p, + PathValue: path[:len(path)-1], + }) + } + key := providerMapKey(p, pv) if _, ok := m[key]; ok { // This provider already exists as a configure node @@ -185,16 +197,6 @@ func (t *MissingProviderTransformer) Transform(g *Graph) error { NameValue: p, PathValue: path, }).(dag.Vertex) - if len(path) > 0 { - // We'll need the parent provider as well, so let's - // add a dummy node to check to make sure that we add - // that parent provider. - check = append(check, &graphNodeProviderConsumerDummy{ - ProviderValue: p, - PathValue: path[:len(path)-1], - }) - } - m[key] = g.Add(v) } } From f0727501c1b7c7d5f57dcf404837c33c60b1f6c8 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 31 Oct 2017 15:58:24 -0400 Subject: [PATCH 02/16] WIP only add missing providers at the root level When looking for providers to connect to resources, walk up the resource path to find the appropriate provider. --- terraform/transform_provider.go | 56 +++++++++++++++++++++++----- terraform/transform_provider_test.go | 2 +- 2 files changed, 47 insertions(+), 11 deletions(-) diff --git a/terraform/transform_provider.go b/terraform/transform_provider.go index baf3016b9c..0bbbd1d991 100644 --- a/terraform/transform_provider.go +++ b/terraform/transform_provider.go @@ -27,6 +27,7 @@ type GraphNodeCloseProvider interface { // a provider must implement. ProvidedBy must return the name of the provider // to use. type GraphNodeProviderConsumer interface { + // TODO: make this return s string instead of a []string ProvidedBy() []string } @@ -41,18 +42,45 @@ func (t *ProviderTransformer) Transform(g *Graph) error { m := providerVertexMap(g) for _, v := range g.Vertices() { if pv, ok := v.(GraphNodeProviderConsumer); ok { - for _, p := range pv.ProvidedBy() { - target := m[providerMapKey(p, pv)] - if target == nil { - println(fmt.Sprintf("%#v\n\n%#v", m, providerMapKey(p, pv))) - err = multierror.Append(err, fmt.Errorf( - "%s: provider %s couldn't be found", - dag.VertexName(v), p)) - continue + p := pv.ProvidedBy()[0] + + key := providerMapKey(p, pv) + target := m[key] + + sp, ok := pv.(GraphNodeSubPath) + if !ok && target == nil { + // no target, and no path to walk up + err = multierror.Append(err, fmt.Errorf( + "%s: provider %s couldn't be found", + dag.VertexName(v), p)) + break + } + + // if we don't have a provider at this level, walk up the path looking for one + for i := 1; target == nil; i++ { + pathPrefix := "" + raw := normalizeModulePath(sp.Path()) + if len(raw) < i { + break } - g.Connect(dag.BasicEdge(v, target)) + raw = raw[:len(raw)-i] + + if len(raw) > len(rootModulePath) { + pathPrefix = modulePrefixStr(raw) + "." + } + key = pathPrefix + p + target = m[key] } + + if target == nil { + err = multierror.Append(err, fmt.Errorf( + "%s: provider %s couldn't be found", + dag.VertexName(v), p)) + break + } + + g.Connect(dag.BasicEdge(v, target)) } } @@ -173,12 +201,17 @@ func (t *MissingProviderTransformer) Transform(g *Graph) error { } key := providerMapKey(p, pv) + + // TODO: jbardin come back to this + // only adding root level missing providers + key = p if _, ok := m[key]; ok { // This provider already exists as a configure node continue } // If the provider has an alias in it, we just want the type + // TODO: jbardin -- stop adding aliased providers altogether ptype := p if idx := strings.IndexRune(p, '.'); idx != -1 { ptype = p[:idx] @@ -195,7 +228,10 @@ func (t *MissingProviderTransformer) Transform(g *Graph) error { // Add the missing provider node to the graph v := t.Concrete(&NodeAbstractProvider{ NameValue: p, - PathValue: path, + + // TODO: jbardin come back to this + // only adding root level missing providers + //PathValue: path, }).(dag.Vertex) m[key] = g.Add(v) } diff --git a/terraform/transform_provider_test.go b/terraform/transform_provider_test.go index b9f52a1f7a..29561a2469 100644 --- a/terraform/transform_provider_test.go +++ b/terraform/transform_provider_test.go @@ -339,7 +339,7 @@ func TestParentProviderTransformer_moduleGrandchild(t *testing.T) { actual := strings.TrimSpace(g.String()) expected := strings.TrimSpace(testTransformParentProviderModuleGrandchildStr) if actual != expected { - t.Fatalf("bad:\n\n%s", actual) + t.Fatalf("expected:\n%s\n\ngot:\n%s", expected, actual) } } From 1d54d4b10dc5f8a54c0fd2373a5e30d3f19886db Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 31 Oct 2017 17:41:44 -0400 Subject: [PATCH 03/16] WIP start only referring to provier by adddress Don't try to build a full adress based on the context path for providers in EvalContextBuiltin. Only use the name explicitly provided. --- terraform/eval_context_builtin.go | 23 +++++------------------ 1 file changed, 5 insertions(+), 18 deletions(-) diff --git a/terraform/eval_context_builtin.go b/terraform/eval_context_builtin.go index 212ca29ebe..3753beec7f 100644 --- a/terraform/eval_context_builtin.go +++ b/terraform/eval_context_builtin.go @@ -92,18 +92,13 @@ func (ctx *BuiltinEvalContext) InitProvider(n string) (ResourceProvider, error) ctx.ProviderLock.Lock() defer ctx.ProviderLock.Unlock() - providerPath := make([]string, len(ctx.Path())+1) - copy(providerPath, ctx.Path()) - providerPath[len(providerPath)-1] = n - key := PathCacheKey(providerPath) - typeName := strings.SplitN(n, ".", 2)[0] - p, err := ctx.Components.ResourceProvider(typeName, key) + p, err := ctx.Components.ResourceProvider(typeName, n) if err != nil { return nil, err } - ctx.ProviderCache[key] = p + ctx.ProviderCache[n] = p return p, nil } @@ -113,11 +108,7 @@ func (ctx *BuiltinEvalContext) Provider(n string) ResourceProvider { ctx.ProviderLock.Lock() defer ctx.ProviderLock.Unlock() - providerPath := make([]string, len(ctx.Path())+1) - copy(providerPath, ctx.Path()) - providerPath[len(providerPath)-1] = n - - return ctx.ProviderCache[PathCacheKey(providerPath)] + return ctx.ProviderCache[n] } func (ctx *BuiltinEvalContext) CloseProvider(n string) error { @@ -126,15 +117,11 @@ func (ctx *BuiltinEvalContext) CloseProvider(n string) error { ctx.ProviderLock.Lock() defer ctx.ProviderLock.Unlock() - providerPath := make([]string, len(ctx.Path())+1) - copy(providerPath, ctx.Path()) - providerPath[len(providerPath)-1] = n - var provider interface{} - provider = ctx.ProviderCache[PathCacheKey(providerPath)] + provider = ctx.ProviderCache[n] if provider != nil { if p, ok := provider.(ResourceProviderCloser); ok { - delete(ctx.ProviderCache, PathCacheKey(providerPath)) + delete(ctx.ProviderCache, n) return p.Close() } } From a14fd0344cf7a784482b400171910e681deafc54 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 1 Nov 2017 18:34:18 -0400 Subject: [PATCH 04/16] WIP reference providers by full name This turned out to be a big messy commit, since the way providers are referenced is tightly coupled throughout the code. That starts to unify how providers are referenced, using the format output node Name method. Add a new field to the internal resource data types called ResolvedProvider. This is set by a new setter method SetProvider when a resource is connected to a provider during graph creation. This allows us to later lookup the provider instance a resource is connected to, without requiring it to have the same module path. The InitProvider context method now takes 2 arguments, one if the provider type and the second is the full name of the provider. While the provider type could still be parsed from the full name, this makes it more explicit and, and changes to the name format won't effect this code. --- terraform/context_apply_test.go | 2 +- terraform/context_plan_test.go | 7 +- terraform/eval_context.go | 4 +- terraform/eval_context_builtin.go | 12 +- terraform/eval_context_mock.go | 2 +- terraform/eval_provider.go | 6 +- terraform/evaltree_provider.go | 27 ++-- terraform/graph_builder_import.go | 7 + terraform/graph_builder_refresh.go | 7 + terraform/node_data_refresh.go | 3 +- terraform/node_provider.go | 2 +- terraform/node_provider_abstract.go | 14 +- terraform/node_resource_abstract.go | 7 + terraform/node_resource_apply.go | 6 +- terraform/node_resource_destroy.go | 7 +- terraform/node_resource_plan.go | 2 + terraform/node_resource_plan_instance.go | 4 +- terraform/node_resource_refresh.go | 5 +- terraform/node_resource_validate.go | 3 +- terraform/transform_deposed.go | 36 +++-- terraform/transform_import_state.go | 43 +++--- terraform/transform_provider.go | 170 +++++++++++------------ terraform/transform_provider_test.go | 2 +- terraform/transform_reference.go | 7 +- terraform/transform_resource_count.go | 4 +- 25 files changed, 218 insertions(+), 171 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 22d7d6db38..0f1fd47a64 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -861,7 +861,7 @@ func TestContext2Apply_createBeforeDestroy(t *testing.T) { actual := strings.TrimSpace(state.String()) expected := strings.TrimSpace(testTerraformApplyCreateBeforeStr) if actual != expected { - t.Fatalf("bad: \n%s", actual) + t.Fatalf("expected:\n%s\ngot:\n%s", expected, actual) } } diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index 7dc860bb50..3eb213028c 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -687,13 +687,10 @@ func TestContext2Plan_moduleProviderDefaultsVar(t *testing.T) { } expected := []string{ - "root\n", - // this test originally verified that a parent provider config can - // partially override a child. That's no longer the case, so the child - // config is used in its entirety here. - //"root\nchild\n", "child\nchild\n", + "root\n", } + sort.Strings(calls) if !reflect.DeepEqual(calls, expected) { t.Fatalf("expected:\n%#v\ngot:\n%#v\n", expected, calls) } diff --git a/terraform/eval_context.go b/terraform/eval_context.go index 655916fc39..86481dedb4 100644 --- a/terraform/eval_context.go +++ b/terraform/eval_context.go @@ -22,11 +22,11 @@ type EvalContext interface { // Input is the UIInput object for interacting with the UI. Input() UIInput - // InitProvider initializes the provider with the given name and + // InitProvider initializes the provider with the given type and name, and // returns the implementation of the resource provider or an error. // // It is an error to initialize the same provider more than once. - InitProvider(string) (ResourceProvider, error) + InitProvider(typ string, name string) (ResourceProvider, error) // Provider gets the provider instance with the given name (already // initialized) or returns nil if the provider isn't initialized. diff --git a/terraform/eval_context_builtin.go b/terraform/eval_context_builtin.go index 3753beec7f..193421b78e 100644 --- a/terraform/eval_context_builtin.go +++ b/terraform/eval_context_builtin.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "log" - "strings" "sync" "github.com/hashicorp/terraform/config" @@ -79,12 +78,12 @@ func (ctx *BuiltinEvalContext) Input() UIInput { return ctx.InputValue } -func (ctx *BuiltinEvalContext) InitProvider(n string) (ResourceProvider, error) { +func (ctx *BuiltinEvalContext) InitProvider(typeName, name string) (ResourceProvider, error) { ctx.once.Do(ctx.init) // If we already initialized, it is an error - if p := ctx.Provider(n); p != nil { - return nil, fmt.Errorf("Provider '%s' already initialized", n) + if p := ctx.Provider(name); p != nil { + return nil, fmt.Errorf("Provider '%s' already initialized", name) } // Warning: make sure to acquire these locks AFTER the call to Provider @@ -92,13 +91,12 @@ func (ctx *BuiltinEvalContext) InitProvider(n string) (ResourceProvider, error) ctx.ProviderLock.Lock() defer ctx.ProviderLock.Unlock() - typeName := strings.SplitN(n, ".", 2)[0] - p, err := ctx.Components.ResourceProvider(typeName, n) + p, err := ctx.Components.ResourceProvider(typeName, name) if err != nil { return nil, err } - ctx.ProviderCache[n] = p + ctx.ProviderCache[name] = p return p, nil } diff --git a/terraform/eval_context_mock.go b/terraform/eval_context_mock.go index a148a54bfe..646451795d 100644 --- a/terraform/eval_context_mock.go +++ b/terraform/eval_context_mock.go @@ -107,7 +107,7 @@ func (c *MockEvalContext) Input() UIInput { return c.InputInput } -func (c *MockEvalContext) InitProvider(n string) (ResourceProvider, error) { +func (c *MockEvalContext) InitProvider(t, n string) (ResourceProvider, error) { c.InitProviderCalled = true c.InitProviderName = n return c.InitProviderProvider, c.InitProviderError diff --git a/terraform/eval_provider.go b/terraform/eval_provider.go index 328d72b5ea..61f6ff941b 100644 --- a/terraform/eval_provider.go +++ b/terraform/eval_provider.go @@ -52,11 +52,12 @@ func (n *EvalConfigProvider) Eval(ctx EvalContext) (interface{}, error) { // and returns nothing. The provider can be retrieved again with the // EvalGetProvider node. type EvalInitProvider struct { - Name string + TypeName string + Name string } func (n *EvalInitProvider) Eval(ctx EvalContext) (interface{}, error) { - return ctx.InitProvider(n.Name) + return ctx.InitProvider(n.TypeName, n.Name) } // EvalCloseProvider is an EvalNode implementation that closes provider @@ -129,6 +130,7 @@ func (n *EvalInputProvider) Eval(ctx EvalContext) (interface{}, error) { } } } + ctx.SetProviderInput(n.Name, confMap) return nil, nil diff --git a/terraform/evaltree_provider.go b/terraform/evaltree_provider.go index 55e5c52586..0c3da48f0d 100644 --- a/terraform/evaltree_provider.go +++ b/terraform/evaltree_provider.go @@ -1,17 +1,24 @@ package terraform import ( + "strings" + "github.com/hashicorp/terraform/config" ) // ProviderEvalTree returns the evaluation tree for initializing and // configuring providers. -func ProviderEvalTree(n string, config *config.ProviderConfig) EvalNode { +func ProviderEvalTree(n *NodeApplyableProvider, config *config.ProviderConfig) EvalNode { var provider ResourceProvider var resourceConfig *ResourceConfig + typeName := strings.SplitN(n.NameValue, ".", 2)[0] + seq := make([]EvalNode, 0, 5) - seq = append(seq, &EvalInitProvider{Name: n}) + seq = append(seq, &EvalInitProvider{ + TypeName: typeName, + Name: n.Name(), + }) // Input stuff seq = append(seq, &EvalOpFilter{ @@ -19,7 +26,7 @@ func ProviderEvalTree(n string, config *config.ProviderConfig) EvalNode { Node: &EvalSequence{ Nodes: []EvalNode{ &EvalGetProvider{ - Name: n, + Name: n.Name(), Output: &provider, }, &EvalInterpolateProvider{ @@ -27,12 +34,12 @@ func ProviderEvalTree(n string, config *config.ProviderConfig) EvalNode { Output: &resourceConfig, }, &EvalBuildProviderConfig{ - Provider: n, + Provider: n.NameValue, Config: &resourceConfig, Output: &resourceConfig, }, &EvalInputProvider{ - Name: n, + Name: n.NameValue, Provider: &provider, Config: &resourceConfig, }, @@ -45,7 +52,7 @@ func ProviderEvalTree(n string, config *config.ProviderConfig) EvalNode { Node: &EvalSequence{ Nodes: []EvalNode{ &EvalGetProvider{ - Name: n, + Name: n.Name(), Output: &provider, }, &EvalInterpolateProvider{ @@ -53,7 +60,7 @@ func ProviderEvalTree(n string, config *config.ProviderConfig) EvalNode { Output: &resourceConfig, }, &EvalBuildProviderConfig{ - Provider: n, + Provider: n.NameValue, Config: &resourceConfig, Output: &resourceConfig, }, @@ -71,7 +78,7 @@ func ProviderEvalTree(n string, config *config.ProviderConfig) EvalNode { Node: &EvalSequence{ Nodes: []EvalNode{ &EvalGetProvider{ - Name: n, + Name: n.Name(), Output: &provider, }, &EvalInterpolateProvider{ @@ -79,7 +86,7 @@ func ProviderEvalTree(n string, config *config.ProviderConfig) EvalNode { Output: &resourceConfig, }, &EvalBuildProviderConfig{ - Provider: n, + Provider: n.NameValue, Config: &resourceConfig, Output: &resourceConfig, }, @@ -94,7 +101,7 @@ func ProviderEvalTree(n string, config *config.ProviderConfig) EvalNode { Node: &EvalSequence{ Nodes: []EvalNode{ &EvalConfigProvider{ - Provider: n, + Provider: n.Name(), Config: &resourceConfig, }, }, diff --git a/terraform/graph_builder_import.go b/terraform/graph_builder_import.go index 7070c59e40..e064ae6292 100644 --- a/terraform/graph_builder_import.go +++ b/terraform/graph_builder_import.go @@ -52,6 +52,13 @@ func (b *ImportGraphBuilder) Steps() []GraphTransformer { // Add the import steps &ImportStateTransformer{Targets: b.ImportTargets}, + // add configured providers + &ProviderConfigTransformer{ + Module: b.Module, + Providers: b.Providers, + Concrete: concreteProvider, + }, + // Provider-related transformations &MissingProviderTransformer{Providers: b.Providers, Concrete: concreteProvider}, &ProviderTransformer{}, diff --git a/terraform/graph_builder_refresh.go b/terraform/graph_builder_refresh.go index beb5a4a445..da506170b5 100644 --- a/terraform/graph_builder_refresh.go +++ b/terraform/graph_builder_refresh.go @@ -126,6 +126,13 @@ func (b *RefreshGraphBuilder) Steps() []GraphTransformer { // Add root variables &RootVariableTransformer{Module: b.Module}, + // add configured providers + &ProviderConfigTransformer{ + Module: b.Module, + Providers: b.Providers, + Concrete: concreteProvider, + }, + // Create all the providers &MissingProviderTransformer{Providers: b.Providers, Concrete: concreteProvider}, &ProviderTransformer{}, diff --git a/terraform/node_data_refresh.go b/terraform/node_data_refresh.go index 45129b3cbf..15d9b8f9cb 100644 --- a/terraform/node_data_refresh.go +++ b/terraform/node_data_refresh.go @@ -27,6 +27,7 @@ func (n *NodeRefreshableDataResource) DynamicExpand(ctx EvalContext) (*Graph, er concreteResource := func(a *NodeAbstractResource) dag.Vertex { // Add the config and state since we don't do that via transforms a.Config = n.Config + a.ResolvedProvider = n.ResolvedProvider return &NodeRefreshableDataResourceInstance{ NodeAbstractResource: a, @@ -185,7 +186,7 @@ func (n *NodeRefreshableDataResourceInstance) EvalTree() EvalNode { // provider configurations that need this data during // refresh/plan. &EvalGetProvider{ - Name: n.ProvidedBy()[0], + Name: n.ResolvedProvider, Output: &provider, }, diff --git a/terraform/node_provider.go b/terraform/node_provider.go index 8e2c176fa9..2071ab168f 100644 --- a/terraform/node_provider.go +++ b/terraform/node_provider.go @@ -7,5 +7,5 @@ type NodeApplyableProvider struct { // GraphNodeEvalable func (n *NodeApplyableProvider) EvalTree() EvalNode { - return ProviderEvalTree(n.NameValue, n.ProviderConfig()) + return ProviderEvalTree(n, n.ProviderConfig()) } diff --git a/terraform/node_provider_abstract.go b/terraform/node_provider_abstract.go index 4a2c000877..3230558e8c 100644 --- a/terraform/node_provider_abstract.go +++ b/terraform/node_provider_abstract.go @@ -24,13 +24,17 @@ type NodeAbstractProvider struct { Config *config.ProviderConfig } -func (n *NodeAbstractProvider) Name() string { - result := fmt.Sprintf("provider.%s", n.NameValue) - if len(n.PathValue) > 1 { - result = fmt.Sprintf("%s.%s", modulePrefixStr(n.PathValue), result) +func ResolveProviderName(name string, path []string) string { + name = fmt.Sprintf("provider.%s", name) + if len(path) > 1 { + name = fmt.Sprintf("%s.%s", modulePrefixStr(path), name) } - return result + return name +} + +func (n *NodeAbstractProvider) Name() string { + return ResolveProviderName(n.NameValue, n.PathValue) } // GraphNodeSubPath diff --git a/terraform/node_resource_abstract.go b/terraform/node_resource_abstract.go index 50bb70792a..e46c320099 100644 --- a/terraform/node_resource_abstract.go +++ b/terraform/node_resource_abstract.go @@ -33,6 +33,9 @@ type NodeAbstractResource struct { ResourceState *ResourceState // ResourceState is the ResourceState for this Targets []ResourceAddress // Set from GraphNodeTargetable + + // The address of the provider this resource will use + ResolvedProvider string } func (n *NodeAbstractResource) Name() string { @@ -170,6 +173,10 @@ func (n *NodeAbstractResource) StateReferences() []string { return deps } +func (n *NodeAbstractResource) SetProvider(p string) { + n.ResolvedProvider = p +} + // GraphNodeProviderConsumer func (n *NodeAbstractResource) ProvidedBy() []string { // If we have a config we prefer that above all else diff --git a/terraform/node_resource_apply.go b/terraform/node_resource_apply.go index 3599782b9d..807b1f4169 100644 --- a/terraform/node_resource_apply.go +++ b/terraform/node_resource_apply.go @@ -135,7 +135,7 @@ func (n *NodeApplyableResource) evalTreeDataResource( }, &EvalGetProvider{ - Name: n.ProvidedBy()[0], + Name: n.ResolvedProvider, Output: &provider, }, @@ -242,7 +242,7 @@ func (n *NodeApplyableResource) evalTreeManagedResource( Output: &resourceConfig, }, &EvalGetProvider{ - Name: n.ProvidedBy()[0], + Name: n.ResolvedProvider, Output: &provider, }, &EvalReadState{ @@ -283,7 +283,7 @@ func (n *NodeApplyableResource) evalTreeManagedResource( }, &EvalGetProvider{ - Name: n.ProvidedBy()[0], + Name: n.ResolvedProvider, Output: &provider, }, &EvalReadState{ diff --git a/terraform/node_resource_destroy.go b/terraform/node_resource_destroy.go index c2efd2c384..cffb9ae60b 100644 --- a/terraform/node_resource_destroy.go +++ b/terraform/node_resource_destroy.go @@ -102,8 +102,9 @@ func (n *NodeDestroyResource) DynamicExpand(ctx EvalContext) (*Graph, error) { // We want deposed resources in the state to be destroyed steps = append(steps, &DeposedTransformer{ - State: state, - View: n.Addr.stateId(), + State: state, + View: n.Addr.stateId(), + ResolvedProvider: n.ResolvedProvider, }) // Target @@ -188,7 +189,7 @@ func (n *NodeDestroyResource) EvalTree() EvalNode { &EvalInstanceInfo{Info: info}, &EvalGetProvider{ - Name: n.ProvidedBy()[0], + Name: n.ResolvedProvider, Output: &provider, }, &EvalReadState{ diff --git a/terraform/node_resource_plan.go b/terraform/node_resource_plan.go index 52bbf88a1b..1afae7a048 100644 --- a/terraform/node_resource_plan.go +++ b/terraform/node_resource_plan.go @@ -27,6 +27,7 @@ func (n *NodePlannableResource) DynamicExpand(ctx EvalContext) (*Graph, error) { concreteResource := func(a *NodeAbstractResource) dag.Vertex { // Add the config and state since we don't do that via transforms a.Config = n.Config + a.ResolvedProvider = n.ResolvedProvider return &NodePlannableResourceInstance{ NodeAbstractResource: a, @@ -37,6 +38,7 @@ func (n *NodePlannableResource) DynamicExpand(ctx EvalContext) (*Graph, error) { concreteResourceOrphan := func(a *NodeAbstractResource) dag.Vertex { // Add the config and state since we don't do that via transforms a.Config = n.Config + a.ResolvedProvider = n.ResolvedProvider return &NodePlannableResourceOrphan{ NodeAbstractResource: a, diff --git a/terraform/node_resource_plan_instance.go b/terraform/node_resource_plan_instance.go index b52956908b..25a76a99fc 100644 --- a/terraform/node_resource_plan_instance.go +++ b/terraform/node_resource_plan_instance.go @@ -97,7 +97,7 @@ func (n *NodePlannableResourceInstance) evalTreeDataResource( }, &EvalGetProvider{ - Name: n.ProvidedBy()[0], + Name: n.ResolvedProvider, Output: &provider, }, @@ -143,7 +143,7 @@ func (n *NodePlannableResourceInstance) evalTreeManagedResource( Output: &resourceConfig, }, &EvalGetProvider{ - Name: n.ProvidedBy()[0], + Name: n.ResolvedProvider, Output: &provider, }, // Re-run validation to catch any errors we missed, e.g. type diff --git a/terraform/node_resource_refresh.go b/terraform/node_resource_refresh.go index cd4fe9201a..d504e7de41 100644 --- a/terraform/node_resource_refresh.go +++ b/terraform/node_resource_refresh.go @@ -30,6 +30,7 @@ func (n *NodeRefreshableManagedResource) DynamicExpand(ctx EvalContext) (*Graph, concreteResource := func(a *NodeAbstractResource) dag.Vertex { // Add the config and state since we don't do that via transforms a.Config = n.Config + a.ResolvedProvider = n.ResolvedProvider return &NodeRefreshableManagedResourceInstance{ NodeAbstractResource: a, @@ -149,7 +150,7 @@ func (n *NodeRefreshableManagedResourceInstance) evalTreeManagedResource() EvalN return &EvalSequence{ Nodes: []EvalNode{ &EvalGetProvider{ - Name: n.ProvidedBy()[0], + Name: n.ResolvedProvider, Output: &provider, }, &EvalReadState{ @@ -220,7 +221,7 @@ func (n *NodeRefreshableManagedResourceInstance) evalTreeManagedResourceNoState( Output: &resourceConfig, }, &EvalGetProvider{ - Name: n.ProvidedBy()[0], + Name: n.ResolvedProvider, Output: &provider, }, // Re-run validation to catch any errors we missed, e.g. type diff --git a/terraform/node_resource_validate.go b/terraform/node_resource_validate.go index f528f24b19..0df223d9ea 100644 --- a/terraform/node_resource_validate.go +++ b/terraform/node_resource_validate.go @@ -39,6 +39,7 @@ func (n *NodeValidatableResource) DynamicExpand(ctx EvalContext) (*Graph, error) concreteResource := func(a *NodeAbstractResource) dag.Vertex { // Add the config and state since we don't do that via transforms a.Config = n.Config + a.ResolvedProvider = n.ResolvedProvider return &NodeValidatableResourceInstance{ NodeAbstractResource: a, @@ -108,7 +109,7 @@ func (n *NodeValidatableResourceInstance) EvalTree() EvalNode { Config: &n.Config.RawConfig, }, &EvalGetProvider{ - Name: n.ProvidedBy()[0], + Name: n.ResolvedProvider, Output: &provider, }, &EvalInterpolate{ diff --git a/terraform/transform_deposed.go b/terraform/transform_deposed.go index 2148cef479..fd920fbdaf 100644 --- a/terraform/transform_deposed.go +++ b/terraform/transform_deposed.go @@ -12,6 +12,9 @@ type DeposedTransformer struct { // View, if non-empty, is the ModuleState.View used around the state // to find deposed resources. View string + + // The provider used by the resourced which were deposed + ResolvedProvider string } func (t *DeposedTransformer) Transform(g *Graph) error { @@ -33,14 +36,16 @@ func (t *DeposedTransformer) Transform(g *Graph) error { if len(rs.Deposed) == 0 { continue } + deposed := rs.Deposed for i, _ := range deposed { g.Add(&graphNodeDeposedResource{ - Index: i, - ResourceName: k, - ResourceType: rs.Type, - Provider: rs.Provider, + Index: i, + ResourceName: k, + ResourceType: rs.Type, + ProviderName: rs.Provider, + ResolvedProvider: t.ResolvedProvider, }) } } @@ -50,10 +55,11 @@ func (t *DeposedTransformer) Transform(g *Graph) error { // graphNodeDeposedResource is the graph vertex representing a deposed resource. type graphNodeDeposedResource struct { - Index int - ResourceName string - ResourceType string - Provider string + Index int + ResourceName string + ResourceType string + ProviderName string + ResolvedProvider string } func (n *graphNodeDeposedResource) Name() string { @@ -61,7 +67,11 @@ func (n *graphNodeDeposedResource) Name() string { } func (n *graphNodeDeposedResource) ProvidedBy() []string { - return []string{resourceProvider(n.ResourceName, n.Provider)} + return []string{resourceProvider(n.ResourceName, n.ProviderName)} +} + +func (n *graphNodeDeposedResource) SetProvider(p string) { + n.ResolvedProvider = p } // GraphNodeEvalable impl. @@ -81,7 +91,7 @@ func (n *graphNodeDeposedResource) EvalTree() EvalNode { Node: &EvalSequence{ Nodes: []EvalNode{ &EvalGetProvider{ - Name: n.ProvidedBy()[0], + Name: n.ResolvedProvider, Output: &provider, }, &EvalReadStateDeposed{ @@ -98,7 +108,7 @@ func (n *graphNodeDeposedResource) EvalTree() EvalNode { &EvalWriteStateDeposed{ Name: n.ResourceName, ResourceType: n.ResourceType, - Provider: n.Provider, + Provider: n.ProviderName, State: &state, Index: n.Index, }, @@ -114,7 +124,7 @@ func (n *graphNodeDeposedResource) EvalTree() EvalNode { Node: &EvalSequence{ Nodes: []EvalNode{ &EvalGetProvider{ - Name: n.ProvidedBy()[0], + Name: n.ResolvedProvider, Output: &provider, }, &EvalReadStateDeposed{ @@ -147,7 +157,7 @@ func (n *graphNodeDeposedResource) EvalTree() EvalNode { &EvalWriteStateDeposed{ Name: n.ResourceName, ResourceType: n.ResourceType, - Provider: n.Provider, + Provider: n.ProviderName, State: &state, Index: n.Index, }, diff --git a/terraform/transform_import_state.go b/terraform/transform_import_state.go index 081df2f84d..762bf1ded5 100644 --- a/terraform/transform_import_state.go +++ b/terraform/transform_import_state.go @@ -21,9 +21,9 @@ func (t *ImportStateTransformer) Transform(g *Graph) error { } nodes = append(nodes, &graphNodeImportState{ - Addr: addr, - ID: target.ID, - Provider: target.Provider, + Addr: addr, + ID: target.ID, + ProviderName: target.Provider, }) } @@ -36,9 +36,10 @@ func (t *ImportStateTransformer) Transform(g *Graph) error { } type graphNodeImportState struct { - Addr *ResourceAddress // Addr is the resource address to import to - ID string // ID is the ID to import as - Provider string // Provider string + Addr *ResourceAddress // Addr is the resource address to import to + ID string // ID is the ID to import as + ProviderName string // Provider string + ResolvedProvider string // provider node address states []*InstanceState } @@ -48,7 +49,11 @@ func (n *graphNodeImportState) Name() string { } func (n *graphNodeImportState) ProvidedBy() []string { - return []string{resourceProvider(n.Addr.Type, n.Provider)} + return []string{resourceProvider(n.Addr.Type, n.ProviderName)} +} + +func (n *graphNodeImportState) SetProvider(p string) { + n.ResolvedProvider = p } // GraphNodeSubPath @@ -72,7 +77,7 @@ func (n *graphNodeImportState) EvalTree() EvalNode { return &EvalSequence{ Nodes: []EvalNode{ &EvalGetProvider{ - Name: n.ProvidedBy()[0], + Name: n.ResolvedProvider, Output: &provider, }, &EvalImportState{ @@ -149,10 +154,11 @@ func (n *graphNodeImportState) DynamicExpand(ctx EvalContext) (*Graph, error) { // is safe. for i, state := range n.states { g.Add(&graphNodeImportStateSub{ - Target: addrs[i], - Path_: n.Path(), - State: state, - Provider: n.Provider, + Target: addrs[i], + Path_: n.Path(), + State: state, + ProviderName: n.ProviderName, + ResolvedProvider: n.ResolvedProvider, }) } @@ -170,10 +176,11 @@ func (n *graphNodeImportState) DynamicExpand(ctx EvalContext) (*Graph, error) { // and is part of the subgraph. This node is responsible for refreshing // and adding a resource to the state once it is imported. type graphNodeImportStateSub struct { - Target *ResourceAddress - State *InstanceState - Path_ []string - Provider string + Target *ResourceAddress + State *InstanceState + Path_ []string + ProviderName string + ResolvedProvider string } func (n *graphNodeImportStateSub) Name() string { @@ -216,7 +223,7 @@ func (n *graphNodeImportStateSub) EvalTree() EvalNode { return &EvalSequence{ Nodes: []EvalNode{ &EvalGetProvider{ - Name: resourceProvider(info.Type, n.Provider), + Name: n.ResolvedProvider, Output: &provider, }, &EvalRefresh{ @@ -233,7 +240,7 @@ func (n *graphNodeImportStateSub) EvalTree() EvalNode { &EvalWriteState{ Name: key.String(), ResourceType: info.Type, - Provider: resourceProvider(info.Type, n.Provider), + Provider: resourceProvider(info.Type, n.ProviderName), State: &state, }, }, diff --git a/terraform/transform_provider.go b/terraform/transform_provider.go index 0bbbd1d991..37bb6be6ca 100644 --- a/terraform/transform_provider.go +++ b/terraform/transform_provider.go @@ -10,10 +10,12 @@ import ( ) // GraphNodeProvider is an interface that nodes that can be a provider -// must implement. The ProviderName returned is the name of the provider -// they satisfy. +// must implement. +// ProviderName returns the name of the provider this satisfies. +// Name returns the full name of the provider in the config. type GraphNodeProvider interface { ProviderName() string + Name() string } // GraphNodeCloseProvider is an interface that nodes that can be a close @@ -29,6 +31,8 @@ type GraphNodeCloseProvider interface { type GraphNodeProviderConsumer interface { // TODO: make this return s string instead of a []string ProvidedBy() []string + // Set the resolved provider address for this resource. + SetProvider(string) } // ProviderTransformer is a GraphTransformer that maps resources to @@ -58,19 +62,16 @@ func (t *ProviderTransformer) Transform(g *Graph) error { // if we don't have a provider at this level, walk up the path looking for one for i := 1; target == nil; i++ { - pathPrefix := "" - raw := normalizeModulePath(sp.Path()) - if len(raw) < i { + path := normalizeModulePath(sp.Path()) + if len(path) < i { break } - raw = raw[:len(raw)-i] - - if len(raw) > len(rootModulePath) { - pathPrefix = modulePrefixStr(raw) + "." - } - key = pathPrefix + p + key = ResolveProviderName(p, path[:len(path)-i]) target = m[key] + if target != nil { + break + } } if target == nil { @@ -80,6 +81,7 @@ func (t *ProviderTransformer) Transform(g *Graph) error { break } + pv.SetProvider(key) g.Connect(dag.BasicEdge(v, target)) } } @@ -95,36 +97,32 @@ type CloseProviderTransformer struct{} func (t *CloseProviderTransformer) Transform(g *Graph) error { pm := providerVertexMap(g) - cpm := closeProviderVertexMap(g) + cpm := make(map[string]*graphNodeCloseProvider) var err error - for _, v := range g.Vertices() { - if pv, ok := v.(GraphNodeProviderConsumer); ok { - for _, p := range pv.ProvidedBy() { - key := p - source := cpm[key] - if source == nil { - // Create a new graphNodeCloseProvider and add it to the graph - source = &graphNodeCloseProvider{ProviderNameValue: p} - g.Add(source) + for _, v := range pm { + p := v.(GraphNodeProvider) - // Close node needs to depend on provider - provider, ok := pm[key] - if !ok { - err = multierror.Append(err, fmt.Errorf( - "%s: provider %s couldn't be found for closing", - dag.VertexName(v), p)) - continue - } - g.Connect(dag.BasicEdge(source, provider)) + // get the close provider of this type if we alread created it + closer := cpm[p.ProviderName()] - // Make sure we also add the new graphNodeCloseProvider to the map - // so we don't create and add any duplicate graphNodeCloseProviders. - cpm[key] = source - } + if closer == nil { + // create a closer for this provider type + closer = &graphNodeCloseProvider{ProviderNameValue: p.ProviderName()} + g.Add(closer) + cpm[p.ProviderName()] = closer + } - // Close node depends on all nodes provided by the provider - g.Connect(dag.BasicEdge(source, v)) + // Close node depends on the provider itself + // this is added unconditionally, so it will connect to all instances + // of the provider. Extra edges will be removed by transitive + // reduction. + g.Connect(dag.BasicEdge(closer, p)) + + // connect all the provider's resources to the close node + for _, s := range g.UpEdges(p).List() { + if _, ok := s.(GraphNodeResource); ok { + g.Connect(dag.BasicEdge(closer, s)) } } } @@ -187,54 +185,47 @@ func (t *MissingProviderTransformer) Transform(g *Graph) error { } } - for _, p := range pv.ProvidedBy() { - // always add the parent nodes to check, since configured providers - // may have already been added for modules. - if len(path) > 0 { - // We'll need the parent provider as well, so let's - // add a dummy node to check to make sure that we add - // that parent provider. - check = append(check, &graphNodeProviderConsumerDummy{ - ProviderValue: p, - PathValue: path[:len(path)-1], - }) - } + p := pv.ProvidedBy()[0] + // always add the parent nodes to check, since configured providers + // may have already been added for modules. + if len(path) > 0 { + // We'll need the parent provider as well, so let's + // add a dummy node to check to make sure that we add + // that parent provider. + check = append(check, &graphNodeProviderConsumerDummy{ + ProviderValue: p, + PathValue: path[:len(path)-1], + }) + } - key := providerMapKey(p, pv) + key := providerMapKey(p, pv) + if _, ok := m[key]; ok { + // This provider already exists as a configure node + continue + } - // TODO: jbardin come back to this - // only adding root level missing providers - key = p - if _, ok := m[key]; ok { - // This provider already exists as a configure node + // If the provider has an alias in it, we just want the type + // TODO: jbardin -- stop adding aliased providers altogether + ptype := p + if idx := strings.IndexRune(p, '.'); idx != -1 { + ptype = p[:idx] + } + + if !t.AllowAny { + if _, ok := supported[ptype]; !ok { + // If we don't support the provider type, skip it. + // Validation later will catch this as an error. continue } - - // If the provider has an alias in it, we just want the type - // TODO: jbardin -- stop adding aliased providers altogether - ptype := p - if idx := strings.IndexRune(p, '.'); idx != -1 { - ptype = p[:idx] - } - - if !t.AllowAny { - if _, ok := supported[ptype]; !ok { - // If we don't support the provider type, skip it. - // Validation later will catch this as an error. - continue - } - } - - // Add the missing provider node to the graph - v := t.Concrete(&NodeAbstractProvider{ - NameValue: p, - - // TODO: jbardin come back to this - // only adding root level missing providers - //PathValue: path, - }).(dag.Vertex) - m[key] = g.Add(v) } + + // Add the missing provider node to the graph + provider := t.Concrete(&NodeAbstractProvider{ + NameValue: p, + PathValue: path, + }).(dag.Vertex) + + m[key] = g.Add(provider) } return nil @@ -274,15 +265,14 @@ func (t *ParentProviderTransformer) Transform(g *Graph) error { } path = normalizeModulePath(path) - // Build the key with path.name i.e. "child.subchild.aws" - key := fmt.Sprintf("%s.%s", strings.Join(path, "."), pn.ProviderName()) + key := ResolveProviderName(pn.ProviderName(), path) m[key] = raw // Determine the parent if we're non-root. This is length 1 since // the 0 index should be "root" since we normalize above. if len(path) > 1 { path = path[:len(path)-1] - key := fmt.Sprintf("%s.%s", strings.Join(path, "."), pn.ProviderName()) + key := ResolveProviderName(pn.ProviderName(), path) parentMap[raw] = key } } @@ -323,23 +313,19 @@ func (t *PruneProviderTransformer) Transform(g *Graph) error { // providerMapKey is a helper that gives us the key to use for the // maps returned by things such as providerVertexMap. func providerMapKey(k string, v dag.Vertex) string { - pathPrefix := "" + // we create a dummy provider to + var path []string if sp, ok := v.(GraphNodeSubPath); ok { - raw := normalizeModulePath(sp.Path()) - if len(raw) > len(rootModulePath) { - pathPrefix = modulePrefixStr(raw) + "." - } + path = normalizeModulePath(sp.Path()) } - - return pathPrefix + k + return ResolveProviderName(k, path) } func providerVertexMap(g *Graph) map[string]dag.Vertex { m := make(map[string]dag.Vertex) for _, v := range g.Vertices() { if pv, ok := v.(GraphNodeProvider); ok { - key := providerMapKey(pv.ProviderName(), v) - m[key] = v + m[pv.Name()] = v } } @@ -416,3 +402,5 @@ func (n *graphNodeProviderConsumerDummy) Path() []string { func (n *graphNodeProviderConsumerDummy) ProvidedBy() []string { return []string{n.ProviderValue} } + +func (n *graphNodeProviderConsumerDummy) SetProvider(string) {} diff --git a/terraform/transform_provider_test.go b/terraform/transform_provider_test.go index 29561a2469..a577d2c121 100644 --- a/terraform/transform_provider_test.go +++ b/terraform/transform_provider_test.go @@ -256,7 +256,7 @@ func TestMissingProviderTransformer_moduleGrandchild(t *testing.T) { actual := strings.TrimSpace(g.String()) expected := strings.TrimSpace(testTransformMissingProviderModuleGrandchildStr) if actual != expected { - t.Fatalf("bad:\n\n%s", actual) + t.Fatalf("expected:\n%s\n\ngot:\n%s", expected, actual) } } diff --git a/terraform/transform_reference.go b/terraform/transform_reference.go index 24ddc7a0a4..2560e5ad64 100644 --- a/terraform/transform_reference.go +++ b/terraform/transform_reference.go @@ -335,8 +335,13 @@ func ReferenceFromInterpolatedVar(v config.InterpolatedVariable) []string { } func modulePrefixStr(p []string) string { + // strip "root" + if len(p) > 0 && p[0] == rootModulePath[0] { + p = p[1:] + } + parts := make([]string, 0, len(p)*2) - for _, p := range p[1:] { + for _, p := range p { parts = append(parts, "module", p) } diff --git a/terraform/transform_resource_count.go b/terraform/transform_resource_count.go index cda35cb7bd..e528b37b46 100644 --- a/terraform/transform_resource_count.go +++ b/terraform/transform_resource_count.go @@ -37,7 +37,9 @@ func (t *ResourceCountTransformer) Transform(g *Graph) error { addr.Index = index // Build the abstract node and the concrete one - abstract := &NodeAbstractResource{Addr: addr} + abstract := &NodeAbstractResource{ + Addr: addr, + } var node dag.Vertex = abstract if f := t.Concrete; f != nil { node = f(abstract) From 8388dfd2f9808262fd9e57c9892962049f02c6ec Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 2 Nov 2017 09:26:10 -0400 Subject: [PATCH 05/16] update GraphBuilder_targetModule test The updates CLoseProviderTransformer connects all provider instances, which is technically the correct behavior. --- terraform/graph_builder_apply_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/terraform/graph_builder_apply_test.go b/terraform/graph_builder_apply_test.go index a1c02dfa88..432cf4fc3f 100644 --- a/terraform/graph_builder_apply_test.go +++ b/terraform/graph_builder_apply_test.go @@ -84,7 +84,7 @@ func TestApplyGraphBuilder(t *testing.T) { actual := strings.TrimSpace(g.String()) expected := strings.TrimSpace(testApplyGraphBuilderStr) if actual != expected { - t.Fatalf("bad: %s", actual) + t.Fatalf("expected:\n%s\n\ngot:\n%s", expected, actual) } } @@ -515,6 +515,7 @@ provider.aws (close) aws_instance.other module.child.aws_instance.create module.child.aws_instance.other + module.child.provider.aws provider.aws provisioner.exec (close) module.child.aws_instance.create From 12a4a29cbd1928bb6052939ee63e1347851ca9a8 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 2 Nov 2017 09:38:15 -0400 Subject: [PATCH 06/16] update missing provider transform test The CloserProviderTransformer requires that the resources be connected to their provider first, so that it cen get the correct dependencies, and adding the ProviderTransformer changed the test output slightly. --- terraform/transform_provider.go | 2 +- terraform/transform_provider_test.go | 11 ++++++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/terraform/transform_provider.go b/terraform/transform_provider.go index 37bb6be6ca..0f34dbcfb6 100644 --- a/terraform/transform_provider.go +++ b/terraform/transform_provider.go @@ -121,7 +121,7 @@ func (t *CloseProviderTransformer) Transform(g *Graph) error { // connect all the provider's resources to the close node for _, s := range g.UpEdges(p).List() { - if _, ok := s.(GraphNodeResource); ok { + if _, ok := s.(GraphNodeProviderConsumer); ok { g.Connect(dag.BasicEdge(closer, s)) } } diff --git a/terraform/transform_provider_test.go b/terraform/transform_provider_test.go index a577d2c121..d5adca1f74 100644 --- a/terraform/transform_provider_test.go +++ b/terraform/transform_provider_test.go @@ -178,6 +178,13 @@ func TestMissingProviderTransformer(t *testing.T) { } } + { + transform := &ProviderTransformer{} + if err := transform.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + { transform := &CloseProviderTransformer{} if err := transform.Transform(&g); err != nil { @@ -188,7 +195,7 @@ func TestMissingProviderTransformer(t *testing.T) { actual := strings.TrimSpace(g.String()) expected := strings.TrimSpace(testTransformMissingProviderBasicStr) if actual != expected { - t.Fatalf("bad:\n\n%s", actual) + t.Fatalf("expected:\n%s\n\ngot:\n%s", expected, actual) } } @@ -413,7 +420,9 @@ provider.aws (close) const testTransformMissingProviderBasicStr = ` aws_instance.web + provider.aws foo_instance.web + provider.foo provider.aws provider.aws (close) aws_instance.web From 2f91007999cd6e91c247eb852f1fb0277b8130b9 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 2 Nov 2017 10:02:51 -0400 Subject: [PATCH 07/16] group the provider transformations The series of provider transformations is important, and often repeated. Group these together in a single transform function. --- terraform/graph_builder_apply.go | 15 ++------------ terraform/graph_builder_import.go | 14 +------------ terraform/graph_builder_plan.go | 14 +------------ terraform/graph_builder_refresh.go | 14 +------------ terraform/transform_destroy_edge.go | 7 +------ terraform/transform_provider.go | 32 +++++++++++++++++++++++++++++ 6 files changed, 38 insertions(+), 58 deletions(-) diff --git a/terraform/graph_builder_apply.go b/terraform/graph_builder_apply.go index 1b898926e3..614da2c852 100644 --- a/terraform/graph_builder_apply.go +++ b/terraform/graph_builder_apply.go @@ -87,19 +87,8 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { // Attach the state &AttachStateTransformer{State: b.State}, - // add configured providers - &ProviderConfigTransformer{ - Module: b.Module, - Providers: b.Providers, - Concrete: concreteProvider, - }, - - // Create all the providers - &MissingProviderTransformer{Providers: b.Providers, Concrete: concreteProvider}, - &ProviderTransformer{}, - &DisableProviderTransformer{}, - &ParentProviderTransformer{}, - &AttachProviderConfigTransformer{Module: b.Module}, + // add providers + TransformProviders(b.Providers, concreteProvider, b.Module), // Destruction ordering &DestroyEdgeTransformer{Module: b.Module, State: b.State}, diff --git a/terraform/graph_builder_import.go b/terraform/graph_builder_import.go index e064ae6292..07a1eaf830 100644 --- a/terraform/graph_builder_import.go +++ b/terraform/graph_builder_import.go @@ -52,19 +52,7 @@ func (b *ImportGraphBuilder) Steps() []GraphTransformer { // Add the import steps &ImportStateTransformer{Targets: b.ImportTargets}, - // add configured providers - &ProviderConfigTransformer{ - Module: b.Module, - Providers: b.Providers, - Concrete: concreteProvider, - }, - - // Provider-related transformations - &MissingProviderTransformer{Providers: b.Providers, Concrete: concreteProvider}, - &ProviderTransformer{}, - &DisableProviderTransformer{}, - &ParentProviderTransformer{}, - &AttachProviderConfigTransformer{Module: mod}, + TransformProviders(b.Providers, concreteProvider, mod), // This validates that the providers only depend on variables &ImportProviderValidateTransformer{}, diff --git a/terraform/graph_builder_plan.go b/terraform/graph_builder_plan.go index 1b93a5ab38..5d625e051e 100644 --- a/terraform/graph_builder_plan.go +++ b/terraform/graph_builder_plan.go @@ -93,19 +93,7 @@ func (b *PlanGraphBuilder) Steps() []GraphTransformer { // Add root variables &RootVariableTransformer{Module: b.Module}, - // add configured providers - &ProviderConfigTransformer{ - Module: b.Module, - Providers: b.Providers, - Concrete: b.ConcreteProvider, - }, - - // Create all the providers - &MissingProviderTransformer{Providers: b.Providers, Concrete: b.ConcreteProvider}, - &ProviderTransformer{}, - &DisableProviderTransformer{}, - &ParentProviderTransformer{}, - &AttachProviderConfigTransformer{Module: b.Module}, + TransformProviders(b.Providers, b.ConcreteProvider, b.Module), // Provisioner-related transformations. Only add these if requested. GraphTransformIf( diff --git a/terraform/graph_builder_refresh.go b/terraform/graph_builder_refresh.go index da506170b5..9638d4c8f3 100644 --- a/terraform/graph_builder_refresh.go +++ b/terraform/graph_builder_refresh.go @@ -126,19 +126,7 @@ func (b *RefreshGraphBuilder) Steps() []GraphTransformer { // Add root variables &RootVariableTransformer{Module: b.Module}, - // add configured providers - &ProviderConfigTransformer{ - Module: b.Module, - Providers: b.Providers, - Concrete: concreteProvider, - }, - - // Create all the providers - &MissingProviderTransformer{Providers: b.Providers, Concrete: concreteProvider}, - &ProviderTransformer{}, - &DisableProviderTransformer{}, - &ParentProviderTransformer{}, - &AttachProviderConfigTransformer{Module: b.Module}, + TransformProviders(b.Providers, concreteProvider, b.Module), // Add the local values &LocalTransformer{Module: b.Module}, diff --git a/terraform/transform_destroy_edge.go b/terraform/transform_destroy_edge.go index d9bd362f2f..a06ff292e2 100644 --- a/terraform/transform_destroy_edge.go +++ b/terraform/transform_destroy_edge.go @@ -127,12 +127,7 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error { &AttachResourceConfigTransformer{Module: t.Module}, &AttachStateTransformer{State: t.State}, - // Add providers since they can affect destroy order as well - &MissingProviderTransformer{AllowAny: true, Concrete: providerFn}, - &ProviderTransformer{}, - &DisableProviderTransformer{}, - &ParentProviderTransformer{}, - &AttachProviderConfigTransformer{Module: t.Module}, + TransformProviders(nil, providerFn, t.Module), // Add all the variables. We can depend on resources through // variables due to module parameters, and we need to properly diff --git a/terraform/transform_provider.go b/terraform/transform_provider.go index 0f34dbcfb6..fac7e15142 100644 --- a/terraform/transform_provider.go +++ b/terraform/transform_provider.go @@ -6,9 +6,41 @@ import ( "strings" "github.com/hashicorp/go-multierror" + "github.com/hashicorp/terraform/config/module" "github.com/hashicorp/terraform/dag" ) +func TransformProviders(providers []string, concrete ConcreteProviderNodeFunc, mod *module.Tree) GraphTransformer { + // If we have no providers, let the MissingProviderTransformer add anything required. + // This is used by the destroy edge transformer's internal dependency graph. + allowAny := providers == nil + + return GraphTransformMulti( + // Add providers from the config + &ProviderConfigTransformer{ + Module: mod, + Providers: providers, + Concrete: concrete, + }, + // Add any remaining missing providers + &MissingProviderTransformer{ + AllowAny: allowAny, + Providers: providers, + Concrete: concrete, + }, + // Connect the providers + &ProviderTransformer{}, + // Disable unused providers + &DisableProviderTransformer{}, + // Connect provider to their parent provider nodes + &ParentProviderTransformer{}, + // Attach configuration to each provider instance + &AttachProviderConfigTransformer{ + Module: mod, + }, + ) +} + // GraphNodeProvider is an interface that nodes that can be a provider // must implement. // ProviderName returns the name of the provider this satisfies. From 94ee4d91119d6e1004237153c03c131e0fc8b5ac Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 2 Nov 2017 11:15:26 -0400 Subject: [PATCH 08/16] Add new test and update graph outputs The the grandChild missing test has a provider declared in a child module which is missing in a grandchildmodule. Verify that the grandchild gets connected to the child provider, and they all are connected to the root providers. Update some test outputs to match the expected behavior of only adding missing providers at the root level. --- .../main.tf | 3 + .../sub/main.tf | 5 ++ .../sub/subsub/main.tf | 2 + terraform/transform_provider_test.go | 65 +++++++++++++++++-- 4 files changed, 70 insertions(+), 5 deletions(-) create mode 100644 terraform/test-fixtures/transform-provider-missing-grandchild/main.tf create mode 100644 terraform/test-fixtures/transform-provider-missing-grandchild/sub/main.tf create mode 100644 terraform/test-fixtures/transform-provider-missing-grandchild/sub/subsub/main.tf diff --git a/terraform/test-fixtures/transform-provider-missing-grandchild/main.tf b/terraform/test-fixtures/transform-provider-missing-grandchild/main.tf new file mode 100644 index 0000000000..385674a891 --- /dev/null +++ b/terraform/test-fixtures/transform-provider-missing-grandchild/main.tf @@ -0,0 +1,3 @@ +module "sub" { + source = "./sub" +} diff --git a/terraform/test-fixtures/transform-provider-missing-grandchild/sub/main.tf b/terraform/test-fixtures/transform-provider-missing-grandchild/sub/main.tf new file mode 100644 index 0000000000..65adf2d1cc --- /dev/null +++ b/terraform/test-fixtures/transform-provider-missing-grandchild/sub/main.tf @@ -0,0 +1,5 @@ +provider "foo" {} + +module "subsub" { + source = "./subsub" +} diff --git a/terraform/test-fixtures/transform-provider-missing-grandchild/sub/subsub/main.tf b/terraform/test-fixtures/transform-provider-missing-grandchild/sub/subsub/main.tf new file mode 100644 index 0000000000..fd865a5250 --- /dev/null +++ b/terraform/test-fixtures/transform-provider-missing-grandchild/sub/subsub/main.tf @@ -0,0 +1,2 @@ +resource "foo_instance" "one" {} +resource "bar_instance" "two" {} diff --git a/terraform/transform_provider_test.go b/terraform/transform_provider_test.go index d5adca1f74..1e5e0f82e2 100644 --- a/terraform/transform_provider_test.go +++ b/terraform/transform_provider_test.go @@ -3,6 +3,8 @@ package terraform import ( "strings" "testing" + + "github.com/hashicorp/terraform/dag" ) func TestProviderTransformer(t *testing.T) { @@ -199,6 +201,46 @@ func TestMissingProviderTransformer(t *testing.T) { } } +func TestMissingProviderTransformer_grandchildMissing(t *testing.T) { + mod := testModule(t, "transform-provider-missing-grandchild") + + concrete := func(a *NodeAbstractProvider) dag.Vertex { return a } + + g := Graph{Path: RootModulePath} + { + tf := &ConfigTransformer{Module: mod} + if err := tf.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + + { + transform := &AttachResourceConfigTransformer{Module: mod} + if err := transform.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + + { + transform := TransformProviders([]string{"aws", "foo", "bar"}, concrete, mod) + if err := transform.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + { + transform := &TransitiveReductionTransformer{} + if err := transform.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + + actual := strings.TrimSpace(g.String()) + expected := strings.TrimSpace(testTransformMissingGrandchildProviderStr) + if actual != expected { + t.Fatalf("expected:\n%s\n\ngot:\n%s", expected, actual) + } +} + func TestMissingProviderTransformer_moduleChild(t *testing.T) { g := Graph{Path: RootModulePath} @@ -304,7 +346,7 @@ func TestParentProviderTransformer(t *testing.T) { actual := strings.TrimSpace(g.String()) expected := strings.TrimSpace(testTransformParentProviderStr) if actual != expected { - t.Fatalf("bad:\n\n%s", actual) + t.Fatalf("expected:\n%s\n\ngot:\n%s", expected, actual) } } @@ -433,6 +475,23 @@ provider.foo (close) provider.foo ` +const testTransformMissingGrandchildProviderStr = ` +module.sub.module.subsub.bar_instance.two + module.sub.module.subsub.provider.bar + provider.bar +module.sub.module.subsub.foo_instance.one + module.sub.module.subsub.provider.foo + provider.foo +module.sub.module.subsub.provider.bar + provider.bar +module.sub.module.subsub.provider.foo + provider.foo +module.sub.provider.foo (disabled) + provider.foo +provider.bar +provider.foo +` + const testTransformMissingProviderModuleChildStr = ` module.moo.foo_instance.qux (import id: bar) module.moo.provider.foo @@ -448,16 +507,12 @@ provider.foo const testTransformParentProviderStr = ` module.moo.foo_instance.qux (import id: bar) -module.moo.provider.foo provider.foo provider.foo ` const testTransformParentProviderModuleGrandchildStr = ` module.a.module.b.foo_instance.qux (import id: bar) -module.a.module.b.provider.foo - module.a.provider.foo -module.a.provider.foo provider.foo provider.foo ` From 341ec39174a74cdb956a2b33d9d853cd2e7c5bb3 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 2 Nov 2017 11:21:07 -0400 Subject: [PATCH 09/16] udpate more tests to remove intermediate providers The new missing provider transformer doesn't add these nay longer --- terraform/transform_provider_test.go | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/terraform/transform_provider_test.go b/terraform/transform_provider_test.go index 1e5e0f82e2..f98ccc0ea5 100644 --- a/terraform/transform_provider_test.go +++ b/terraform/transform_provider_test.go @@ -478,49 +478,41 @@ provider.foo (close) const testTransformMissingGrandchildProviderStr = ` module.sub.module.subsub.bar_instance.two module.sub.module.subsub.provider.bar - provider.bar module.sub.module.subsub.foo_instance.one module.sub.module.subsub.provider.foo - provider.foo module.sub.module.subsub.provider.bar - provider.bar + provider.bar (disabled) module.sub.module.subsub.provider.foo - provider.foo + provider.foo (disabled) module.sub.provider.foo (disabled) - provider.foo -provider.bar -provider.foo + provider.foo (disabled) +provider.bar (disabled) +provider.foo (disabled) ` const testTransformMissingProviderModuleChildStr = ` module.moo.foo_instance.qux (import id: bar) -module.moo.provider.foo provider.foo ` const testTransformMissingProviderModuleGrandchildStr = ` module.a.module.b.foo_instance.qux (import id: bar) -module.a.module.b.provider.foo -module.a.provider.foo provider.foo ` const testTransformParentProviderStr = ` module.moo.foo_instance.qux (import id: bar) - provider.foo provider.foo ` const testTransformParentProviderModuleGrandchildStr = ` module.a.module.b.foo_instance.qux (import id: bar) - provider.foo provider.foo ` const testTransformProviderModuleChildStr = ` module.moo.foo_instance.qux (import id: bar) - module.moo.provider.foo -module.moo.provider.foo + provider.foo provider.foo ` From a782568645286b82dd6b2718837928fc8d3ceb29 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 2 Nov 2017 14:55:11 -0400 Subject: [PATCH 10/16] simplify MissingProvider and ParentProvider transf Simplify the MissingProviderTransformer so that it only adds missing providers at the root level. There's no need for the multitple providers added at every level of the path ParentProviderTransformer then only needs to connect providers with the equivalent type at the root level. --- terraform/transform_provider.go | 121 +++++++++++--------------------- 1 file changed, 40 insertions(+), 81 deletions(-) diff --git a/terraform/transform_provider.go b/terraform/transform_provider.go index fac7e15142..d70f6e8fad 100644 --- a/terraform/transform_provider.go +++ b/terraform/transform_provider.go @@ -10,6 +10,7 @@ import ( "github.com/hashicorp/terraform/dag" ) +// TODO: return the transformers and append them to the list, so we don't lose the log steps func TransformProviders(providers []string, concrete ConcreteProviderNodeFunc, mod *module.Tree) GraphTransformer { // If we have no providers, let the MissingProviderTransformer add anything required. // This is used by the destroy edge transformer's internal dependency graph. @@ -127,6 +128,7 @@ func (t *ProviderTransformer) Transform(g *Graph) error { // in the graph are evaluated. type CloseProviderTransformer struct{} +// FIXME: this doesn't close providers if the root provider is disabled func (t *CloseProviderTransformer) Transform(g *Graph) error { pm := providerVertexMap(g) cpm := make(map[string]*graphNodeCloseProvider) @@ -192,96 +194,62 @@ func (t *MissingProviderTransformer) Transform(g *Graph) error { supported[v] = struct{}{} } - // Get the map of providers we already have in our graph + var err error m := providerVertexMap(g) - - // Go through all the provider consumers and make sure we add - // that provider if it is missing. We use a for loop here instead - // of "range" since we'll modify check as we go to add more to check. - check := g.Vertices() - for i := 0; i < len(check); i++ { - v := check[i] - + for _, v := range g.Vertices() { pv, ok := v.(GraphNodeProviderConsumer) if !ok { continue } - // If this node has a subpath, then we use that as a prefix - // into our map to check for an existing provider. + p := pv.ProvidedBy()[0] + var path []string if sp, ok := pv.(GraphNodeSubPath); ok { - raw := normalizeModulePath(sp.Path()) - if len(raw) > len(rootModulePath) { - path = raw - } - } - - p := pv.ProvidedBy()[0] - // always add the parent nodes to check, since configured providers - // may have already been added for modules. - if len(path) > 0 { - // We'll need the parent provider as well, so let's - // add a dummy node to check to make sure that we add - // that parent provider. - check = append(check, &graphNodeProviderConsumerDummy{ - ProviderValue: p, - PathValue: path[:len(path)-1], - }) + path = sp.Path() } key := providerMapKey(p, pv) - if _, ok := m[key]; ok { - // This provider already exists as a configure node - continue + + provider := m[key] + + // if we don't have a provider at this level, walk up the path looking for one + for i := 1; provider == nil && len(path) >= i; i++ { + key = ResolveProviderName(p, normalizeModulePath(path[:len(path)-i])) + provider = m[key] } - // If the provider has an alias in it, we just want the type - // TODO: jbardin -- stop adding aliased providers altogether - ptype := p - if idx := strings.IndexRune(p, '.'); idx != -1 { - ptype = p[:idx] - } - - if !t.AllowAny { - if _, ok := supported[ptype]; !ok { - // If we don't support the provider type, skip it. - // Validation later will catch this as an error. + if provider != nil { + // we found a provider, but make sure there's a top-level provider too + if _, ok := m[ResolveProviderName(p, nil)]; ok { continue } } - // Add the missing provider node to the graph - provider := t.Concrete(&NodeAbstractProvider{ + // always add a new top level provider + provider = t.Concrete(&NodeAbstractProvider{ NameValue: p, - PathValue: path, }).(dag.Vertex) + key = ResolveProviderName(p, nil) m[key] = g.Add(provider) + + pv.SetProvider(key) } - return nil + return err } // ParentProviderTransformer connects provider nodes to their parents. // // This works by finding nodes that are both GraphNodeProviders and // GraphNodeSubPath. It then connects the providers to their parent -// path. +// path. The parent provider is always at the root level. type ParentProviderTransformer struct{} func (t *ParentProviderTransformer) Transform(g *Graph) error { - // Make a mapping of path to dag.Vertex, where path is: "path.name" - m := make(map[string]dag.Vertex) - - // Also create a map that maps a provider to its parent - parentMap := make(map[dag.Vertex]string) - for _, raw := range g.Vertices() { - // If it is the flat version, then make it the non-flat version. - // We eventually want to get rid of the flat version entirely so - // this is a stop-gap while it still exists. - var v dag.Vertex = raw - + pm := providerVertexMap(g) + for _, v := range g.Vertices() { // Only care about providers pn, ok := v.(GraphNodeProvider) if !ok || pn.ProviderName() == "" { @@ -289,33 +257,22 @@ func (t *ParentProviderTransformer) Transform(g *Graph) error { } // Also require a subpath, if there is no subpath then we - // just totally ignore it. The expectation of this transform is - // that it is used with a graph builder that is already flattened. - var path []string - if pn, ok := raw.(GraphNodeSubPath); ok { - path = pn.Path() + // can't have a parent. + if pn, ok := v.(GraphNodeSubPath); ok { + if len(normalizeModulePath(pn.Path())) <= 1 { + continue + } } - path = normalizeModulePath(path) - key := ResolveProviderName(pn.ProviderName(), path) - m[key] = raw - - // Determine the parent if we're non-root. This is length 1 since - // the 0 index should be "root" since we normalize above. - if len(path) > 1 { - path = path[:len(path)-1] - key := ResolveProviderName(pn.ProviderName(), path) - parentMap[raw] = key - } - } - - // Connect! - for v, key := range parentMap { - if parent, ok := m[key]; ok { + // this provider may be disabled, but we can only get it's name from + // the ProviderName string + name := ResolveProviderName(strings.SplitN(pn.ProviderName(), " ", 2)[0], nil) + parent := pm[name] + if parent != nil { g.Connect(dag.BasicEdge(v, parent)) } - } + } return nil } @@ -357,7 +314,9 @@ func providerVertexMap(g *Graph) map[string]dag.Vertex { m := make(map[string]dag.Vertex) for _, v := range g.Vertices() { if pv, ok := v.(GraphNodeProvider); ok { - m[pv.Name()] = v + // TODO: The Name may have meta info, like " (disabled)" + name := strings.SplitN(pv.Name(), " ", 2)[0] + m[name] = v } } From 838ffafc18ff872ca51b60a16e64433b3347d33b Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 2 Nov 2017 15:22:30 -0400 Subject: [PATCH 11/16] stop adding implicit provider configs Now that providers in the graph can adopt resources without an explicit provider, there's no need to add the implicit configs to the module.Tree when loading. --- config/module/tree.go | 44 ------------------------------------------- 1 file changed, 44 deletions(-) diff --git a/config/module/tree.go b/config/module/tree.go index 1ed03d07c5..8deaf2d254 100644 --- a/config/module/tree.go +++ b/config/module/tree.go @@ -365,50 +365,6 @@ func (t *Tree) inheritProviderConfigs(stack []*Tree) { } } - // Search for implicit provider configs - // This adds an empty config is no inherited config is found, so that - // there is always a provider config present. - // This is done in the root module as well, just to set the providers. - for missing := range missingProviders { - // first create an empty provider config - pc := &config.ProviderConfig{ - Name: missing, - } - - // walk up the stack looking for matching providers - for i := len(stack) - 2; i >= 0; i-- { - pt := stack[i] - var parentProvider *config.ProviderConfig - for _, p := range pt.config.ProviderConfigs { - if p.FullName() == missing { - parentProvider = p - break - } - } - - if parentProvider == nil { - continue - } - - pc.Path = pt.Path() - pc.Path = append([]string{RootName}, pt.path...) - pc.RawConfig = parentProvider.RawConfig - pc.Inherited = true - log.Printf("[TRACE] provider %q inheriting config from %q", - strings.Join(append(t.Path(), pc.FullName()), "."), - strings.Join(append(pt.Path(), parentProvider.FullName()), "."), - ) - break - } - - // always set a provider config - if pc.RawConfig == nil { - pc.RawConfig, _ = config.NewRawConfig(map[string]interface{}{}) - } - - t.config.ProviderConfigs = append(t.config.ProviderConfigs, pc) - } - // After allowing the empty implicit configs to be created in root, there's nothing left to inherit if len(stack) == 1 { return From 05ef30be2c34799f2d167305f14da8dea972e38f Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 2 Nov 2017 15:25:18 -0400 Subject: [PATCH 12/16] udpate test graph outputs Some of the test graph outputs have changes now that implicit providers are no longer added in the config. --- terraform/graph_builder_apply_test.go | 6 +----- terraform/module_dependencies_test.go | 18 +++++++----------- terraform/transform_provider_test.go | 12 ++++-------- 3 files changed, 12 insertions(+), 24 deletions(-) diff --git a/terraform/graph_builder_apply_test.go b/terraform/graph_builder_apply_test.go index 432cf4fc3f..ccc14399ad 100644 --- a/terraform/graph_builder_apply_test.go +++ b/terraform/graph_builder_apply_test.go @@ -497,16 +497,13 @@ meta.count-boundary (count boundary fixup) aws_instance.other module.child.aws_instance.create module.child.aws_instance.other - module.child.provider.aws module.child.provisioner.exec provider.aws module.child.aws_instance.create - module.child.provider.aws module.child.provisioner.exec + provider.aws module.child.aws_instance.other module.child.aws_instance.create - module.child.provider.aws -module.child.provider.aws provider.aws module.child.provisioner.exec provider.aws @@ -515,7 +512,6 @@ provider.aws (close) aws_instance.other module.child.aws_instance.create module.child.aws_instance.other - module.child.provider.aws provider.aws provisioner.exec (close) module.child.aws_instance.create diff --git a/terraform/module_dependencies_test.go b/terraform/module_dependencies_test.go index 39e4e7de59..8bcbef062a 100644 --- a/terraform/module_dependencies_test.go +++ b/terraform/module_dependencies_test.go @@ -73,8 +73,7 @@ func TestModuleTreeDependencies(t *testing.T) { Providers: moduledeps.Providers{ "foo": moduledeps.ProviderDependency{ Constraints: discovery.AllVersions, - //Reason: moduledeps.ProviderDependencyImplicit, - Reason: moduledeps.ProviderDependencyExplicit, + Reason: moduledeps.ProviderDependencyImplicit, }, "foo.baz": moduledeps.ProviderDependency{ Constraints: discovery.AllVersions, @@ -119,28 +118,25 @@ func TestModuleTreeDependencies(t *testing.T) { Providers: moduledeps.Providers{ "foo": moduledeps.ProviderDependency{ Constraints: discovery.AllVersions, - //Reason: moduledeps.ProviderDependencyInherited, - Reason: moduledeps.ProviderDependencyExplicit, + Reason: moduledeps.ProviderDependencyInherited, }, "baz": moduledeps.ProviderDependency{ Constraints: discovery.AllVersions, - //Reason: moduledeps.ProviderDependencyImplicit, - Reason: moduledeps.ProviderDependencyExplicit, + Reason: moduledeps.ProviderDependencyImplicit, }, }, Children: []*moduledeps.Module{ { Name: "grandchild", Providers: moduledeps.Providers{ + "bar": moduledeps.ProviderDependency{ + Constraints: discovery.AllVersions, + Reason: moduledeps.ProviderDependencyInherited, + }, "foo": moduledeps.ProviderDependency{ Constraints: discovery.AllVersions, Reason: moduledeps.ProviderDependencyExplicit, }, - "bar": moduledeps.ProviderDependency{ - Constraints: discovery.AllVersions, - //Reason: moduledeps.ProviderDependencyInherited, - Reason: moduledeps.ProviderDependencyExplicit, - }, }, }, }, diff --git a/terraform/transform_provider_test.go b/terraform/transform_provider_test.go index f98ccc0ea5..42995b8a3b 100644 --- a/terraform/transform_provider_test.go +++ b/terraform/transform_provider_test.go @@ -477,16 +477,12 @@ provider.foo (close) const testTransformMissingGrandchildProviderStr = ` module.sub.module.subsub.bar_instance.two - module.sub.module.subsub.provider.bar + provider.bar module.sub.module.subsub.foo_instance.one - module.sub.module.subsub.provider.foo -module.sub.module.subsub.provider.bar - provider.bar (disabled) -module.sub.module.subsub.provider.foo + module.sub.provider.foo +module.sub.provider.foo provider.foo (disabled) -module.sub.provider.foo (disabled) - provider.foo (disabled) -provider.bar (disabled) +provider.bar provider.foo (disabled) ` From 241dae7ead06aa48c7427a49c330faade420dcfe Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 2 Nov 2017 15:25:59 -0400 Subject: [PATCH 13/16] resintate disabled tests Reinstate the disabled tests that required some sort of inheritance during graph evaluation. --- terraform/context_apply_test.go | 174 ++++++++++++++--------------- terraform/context_validate_test.go | 90 ++++++++------- 2 files changed, 130 insertions(+), 134 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 0f1fd47a64..00ad665234 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -2639,107 +2639,105 @@ module.child: `) } -//// FIXME: how do we handle this one? -//func TestContext2Apply_moduleOrphanProvider(t *testing.T) { -// m := testModule(t, "apply-module-orphan-provider-inherit") -// p := testProvider("aws") -// p.ApplyFn = testApplyFn -// p.DiffFn = testDiffFn +func TestContext2Apply_moduleOrphanProvider(t *testing.T) { + m := testModule(t, "apply-module-orphan-provider-inherit") + p := testProvider("aws") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn -// p.ConfigureFn = func(c *ResourceConfig) error { -// if _, ok := c.Get("value"); !ok { -// return fmt.Errorf("value is not found") -// } + p.ConfigureFn = func(c *ResourceConfig) error { + if _, ok := c.Get("value"); !ok { + return fmt.Errorf("value is not found") + } -// return nil -// } + return nil + } -// // Create a state with an orphan module -// state := &State{ -// Modules: []*ModuleState{ -// &ModuleState{ -// Path: []string{"root", "child"}, -// Resources: map[string]*ResourceState{ -// "aws_instance.bar": &ResourceState{ -// Type: "aws_instance", -// Primary: &InstanceState{ -// ID: "bar", -// }, -// }, -// }, -// }, -// }, -// } + // Create a state with an orphan module + state := &State{ + Modules: []*ModuleState{ + &ModuleState{ + Path: []string{"root", "child"}, + Resources: map[string]*ResourceState{ + "aws_instance.bar": &ResourceState{ + Type: "aws_instance", + Primary: &InstanceState{ + ID: "bar", + }, + }, + }, + }, + }, + } -// ctx := testContext2(t, &ContextOpts{ -// Module: m, -// State: state, -// ProviderResolver: ResourceProviderResolverFixed( -// map[string]ResourceProviderFactory{ -// "aws": testProviderFuncFixed(p), -// }, -// ), -// }) + ctx := testContext2(t, &ContextOpts{ + Module: m, + State: state, + ProviderResolver: ResourceProviderResolverFixed( + map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + ), + }) -// if _, err := ctx.Plan(); err != nil { -// t.Fatalf("err: %s", err) -// } + if _, err := ctx.Plan(); err != nil { + t.Fatalf("err: %s", err) + } -// if _, err := ctx.Apply(); err != nil { -// t.Fatalf("err: %s", err) -// } -//} + if _, err := ctx.Apply(); err != nil { + t.Fatalf("err: %s", err) + } +} -//// FIXME: how do we handle this one? -//func TestContext2Apply_moduleOrphanGrandchildProvider(t *testing.T) { -// m := testModule(t, "apply-module-orphan-provider-inherit") -// p := testProvider("aws") -// p.ApplyFn = testApplyFn -// p.DiffFn = testDiffFn +func TestContext2Apply_moduleOrphanGrandchildProvider(t *testing.T) { + m := testModule(t, "apply-module-orphan-provider-inherit") + p := testProvider("aws") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn -// p.ConfigureFn = func(c *ResourceConfig) error { -// if _, ok := c.Get("value"); !ok { -// return fmt.Errorf("value is not found") -// } + p.ConfigureFn = func(c *ResourceConfig) error { + if _, ok := c.Get("value"); !ok { + return fmt.Errorf("value is not found") + } -// return nil -// } + return nil + } -// // Create a state with an orphan module that is nested (grandchild) -// state := &State{ -// Modules: []*ModuleState{ -// &ModuleState{ -// Path: []string{"root", "parent", "child"}, -// Resources: map[string]*ResourceState{ -// "aws_instance.bar": &ResourceState{ -// Type: "aws_instance", -// Primary: &InstanceState{ -// ID: "bar", -// }, -// }, -// }, -// }, -// }, -// } + // Create a state with an orphan module that is nested (grandchild) + state := &State{ + Modules: []*ModuleState{ + &ModuleState{ + Path: []string{"root", "parent", "child"}, + Resources: map[string]*ResourceState{ + "aws_instance.bar": &ResourceState{ + Type: "aws_instance", + Primary: &InstanceState{ + ID: "bar", + }, + }, + }, + }, + }, + } -// ctx := testContext2(t, &ContextOpts{ -// Module: m, -// State: state, -// ProviderResolver: ResourceProviderResolverFixed( -// map[string]ResourceProviderFactory{ -// "aws": testProviderFuncFixed(p), -// }, -// ), -// }) + ctx := testContext2(t, &ContextOpts{ + Module: m, + State: state, + ProviderResolver: ResourceProviderResolverFixed( + map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + ), + }) -// if _, err := ctx.Plan(); err != nil { -// t.Fatalf("err: %s", err) -// } + if _, err := ctx.Plan(); err != nil { + t.Fatalf("err: %s", err) + } -// if _, err := ctx.Apply(); err != nil { -// t.Fatalf("err: %s", err) -// } -//} + if _, err := ctx.Apply(); err != nil { + t.Fatalf("err: %s", err) + } +} func TestContext2Apply_moduleGrandchildProvider(t *testing.T) { m := testModule(t, "apply-module-grandchild-provider-inherit") diff --git a/terraform/context_validate_test.go b/terraform/context_validate_test.go index f2fb0895d8..8cf847e6db 100644 --- a/terraform/context_validate_test.go +++ b/terraform/context_validate_test.go @@ -321,55 +321,53 @@ func TestContext2Validate_moduleDepsShouldNotCycle(t *testing.T) { } } -//// FIXME: provider must still exist in config, but we should be able to locate -//// it elsewhere -//func TestContext2Validate_moduleProviderInheritOrphan(t *testing.T) { -// m := testModule(t, "validate-module-pc-inherit-orphan") -// p := testProvider("aws") -// c := testContext2(t, &ContextOpts{ -// Module: m, -// ProviderResolver: ResourceProviderResolverFixed( -// map[string]ResourceProviderFactory{ -// "aws": testProviderFuncFixed(p), -// }, -// ), -// State: &State{ -// Modules: []*ModuleState{ -// &ModuleState{ -// Path: []string{"root", "child"}, -// Resources: map[string]*ResourceState{ -// "aws_instance.bar": &ResourceState{ -// Type: "aws_instance", -// Primary: &InstanceState{ -// ID: "bar", -// }, -// }, -// }, -// }, -// }, -// }, -// }) +func TestContext2Validate_moduleProviderInheritOrphan(t *testing.T) { + m := testModule(t, "validate-module-pc-inherit-orphan") + p := testProvider("aws") + c := testContext2(t, &ContextOpts{ + Module: m, + ProviderResolver: ResourceProviderResolverFixed( + map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + ), + State: &State{ + Modules: []*ModuleState{ + &ModuleState{ + Path: []string{"root", "child"}, + Resources: map[string]*ResourceState{ + "aws_instance.bar": &ResourceState{ + Type: "aws_instance", + Primary: &InstanceState{ + ID: "bar", + }, + }, + }, + }, + }, + }, + }) -// p.ValidateFn = func(c *ResourceConfig) ([]string, []error) { -// v, ok := c.Get("set") -// if !ok { -// return nil, []error{fmt.Errorf("not set")} -// } -// if v != "bar" { -// return nil, []error{fmt.Errorf("bad: %#v", v)} -// } + p.ValidateFn = func(c *ResourceConfig) ([]string, []error) { + v, ok := c.Get("set") + if !ok { + return nil, []error{fmt.Errorf("not set")} + } + if v != "bar" { + return nil, []error{fmt.Errorf("bad: %#v", v)} + } -// return nil, nil -// } + return nil, nil + } -// w, e := c.Validate() -// if len(w) > 0 { -// t.Fatalf("bad: %#v", w) -// } -// if len(e) > 0 { -// t.Fatalf("bad: %s", e) -// } -//} + w, e := c.Validate() + if len(w) > 0 { + t.Fatalf("bad: %#v", w) + } + if len(e) > 0 { + t.Fatalf("bad: %s", e) + } +} func TestContext2Validate_moduleProviderVar(t *testing.T) { m := testModule(t, "validate-module-pc-vars") From 523b1213419e4de2cfa0a05f1df5bf5e0fd1f8f1 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 2 Nov 2017 15:38:53 -0400 Subject: [PATCH 14/16] fix get test working directory use a temp dir and cleanup --- command/get_test.go | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/command/get_test.go b/command/get_test.go index 69980db059..9c9043e774 100644 --- a/command/get_test.go +++ b/command/get_test.go @@ -5,6 +5,7 @@ import ( "strings" "testing" + "github.com/hashicorp/terraform/helper/copy" "github.com/mitchellh/cli" ) @@ -57,14 +58,10 @@ func TestGet_multipleArgs(t *testing.T) { } func TestGet_noArgs(t *testing.T) { - cwd, err := os.Getwd() - if err != nil { - t.Fatalf("err: %s", err) - } - if err := os.Chdir(testFixturePath("get")); err != nil { - t.Fatalf("err: %s", err) - } - defer os.Chdir(cwd) + td := tempDir(t) + copy.CopyDir(testFixturePath("get"), td) + defer os.RemoveAll(td) + defer testChdir(t, td)() ui := new(cli.MockUi) c := &GetCommand{ From b4e92406795dc9910766e9d047ccf4627d36ae02 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 2 Nov 2017 15:43:45 -0400 Subject: [PATCH 15/16] remove implicit provier tests from config/module No longer needed --- .../child/grandchild/main.tf | 2 - .../child/main.tf | 9 -- .../implicit-grandparent-providers/main.tf | 7 -- .../implicit-parent-providers/child/main.tf | 1 - .../implicit-parent-providers/main.tf | 7 -- config/module/tree_test.go | 111 ------------------ 6 files changed, 137 deletions(-) delete mode 100644 config/module/test-fixtures/implicit-grandparent-providers/child/grandchild/main.tf delete mode 100644 config/module/test-fixtures/implicit-grandparent-providers/child/main.tf delete mode 100644 config/module/test-fixtures/implicit-grandparent-providers/main.tf delete mode 100644 config/module/test-fixtures/implicit-parent-providers/child/main.tf delete mode 100644 config/module/test-fixtures/implicit-parent-providers/main.tf diff --git a/config/module/test-fixtures/implicit-grandparent-providers/child/grandchild/main.tf b/config/module/test-fixtures/implicit-grandparent-providers/child/grandchild/main.tf deleted file mode 100644 index 492e4d0e40..0000000000 --- a/config/module/test-fixtures/implicit-grandparent-providers/child/grandchild/main.tf +++ /dev/null @@ -1,2 +0,0 @@ -resource "bar_resource" "in_grandchild" {} - diff --git a/config/module/test-fixtures/implicit-grandparent-providers/child/main.tf b/config/module/test-fixtures/implicit-grandparent-providers/child/main.tf deleted file mode 100644 index 1ab8b908e3..0000000000 --- a/config/module/test-fixtures/implicit-grandparent-providers/child/main.tf +++ /dev/null @@ -1,9 +0,0 @@ -resource "foo_resource" "in_child" {} - -provider "bar" { - value = "from child" -} - -module "grandchild" { - source = "./grandchild" -} diff --git a/config/module/test-fixtures/implicit-grandparent-providers/main.tf b/config/module/test-fixtures/implicit-grandparent-providers/main.tf deleted file mode 100644 index 2c06ab5553..0000000000 --- a/config/module/test-fixtures/implicit-grandparent-providers/main.tf +++ /dev/null @@ -1,7 +0,0 @@ -provider "foo" { - value = "from root" -} - -module "child" { - source = "./child" -} diff --git a/config/module/test-fixtures/implicit-parent-providers/child/main.tf b/config/module/test-fixtures/implicit-parent-providers/child/main.tf deleted file mode 100644 index 2601241294..0000000000 --- a/config/module/test-fixtures/implicit-parent-providers/child/main.tf +++ /dev/null @@ -1 +0,0 @@ -resource "foo_instance" "bar" {} diff --git a/config/module/test-fixtures/implicit-parent-providers/main.tf b/config/module/test-fixtures/implicit-parent-providers/main.tf deleted file mode 100644 index 2c06ab5553..0000000000 --- a/config/module/test-fixtures/implicit-parent-providers/main.tf +++ /dev/null @@ -1,7 +0,0 @@ -provider "foo" { - value = "from root" -} - -module "child" { - source = "./child" -} diff --git a/config/module/tree_test.go b/config/module/tree_test.go index 2e23f1aad1..acc622c3f8 100644 --- a/config/module/tree_test.go +++ b/config/module/tree_test.go @@ -611,117 +611,6 @@ func TestTreeProviders_basic(t *testing.T) { } } -func TestTreeProviders_implicit(t *testing.T) { - storage := testStorage(t, nil) - tree := NewTree("", testConfig(t, "implicit-parent-providers")) - - storage.Mode = GetModeGet - if err := tree.Load(storage); err != nil { - t.Fatalf("err: %s", err) - } - - var child *Tree - for _, c := range tree.Children() { - if c.Name() == "child" { - child = c - } - } - - if child == nil { - t.Fatal("could not find module 'child'") - } - - // child should have inherited foo - providers := child.config.ProviderConfigsByFullName() - foo := providers["foo"] - - if foo == nil { - t.Fatal("could not find provider 'foo' in child module") - } - - if !reflect.DeepEqual([]string{RootName}, foo.Path) { - t.Fatalf(`expected foo scope of {"root"}, got %#v`, foo.Path) - } - - expected := map[string]interface{}{ - "value": "from root", - } - - if !reflect.DeepEqual(expected, foo.RawConfig.RawMap()) { - t.Fatalf(`expected "foo" config %#v, got: %#v`, expected, foo.RawConfig.RawMap()) - } -} - -func TestTreeProviders_implicitMultiLevel(t *testing.T) { - storage := testStorage(t, nil) - tree := NewTree("", testConfig(t, "implicit-grandparent-providers")) - - storage.Mode = GetModeGet - if err := tree.Load(storage); err != nil { - t.Fatalf("err: %s", err) - } - - var child, grandchild *Tree - for _, c := range tree.Children() { - if c.Name() == "child" { - child = c - } - } - - if child == nil { - t.Fatal("could not find module 'child'") - } - - for _, c := range child.Children() { - if c.Name() == "grandchild" { - grandchild = c - } - } - if grandchild == nil { - t.Fatal("could not find module 'grandchild'") - } - - // child should have inherited foo - providers := child.config.ProviderConfigsByFullName() - foo := providers["foo"] - - if foo == nil { - t.Fatal("could not find provider 'foo' in child module") - } - - if !reflect.DeepEqual([]string{RootName}, foo.Path) { - t.Fatalf(`expected foo scope of {"root"}, got %#v`, foo.Path) - } - - expected := map[string]interface{}{ - "value": "from root", - } - - if !reflect.DeepEqual(expected, foo.RawConfig.RawMap()) { - t.Fatalf(`expected "foo" config %#v, got: %#v`, expected, foo.RawConfig.RawMap()) - } - - // grandchild should have inherited bar - providers = grandchild.config.ProviderConfigsByFullName() - bar := providers["bar"] - - if bar == nil { - t.Fatal("could not find provider 'bar' in grandchild module") - } - - if !reflect.DeepEqual([]string{RootName, "child"}, bar.Path) { - t.Fatalf(`expected bar scope of {"root", "child"}, got %#v`, bar.Path) - } - - expected = map[string]interface{}{ - "value": "from child", - } - - if !reflect.DeepEqual(expected, bar.RawConfig.RawMap()) { - t.Fatalf(`expected "bar" config %#v, got: %#v`, expected, bar.RawConfig.RawMap()) - } -} - func TestTreeLoad_conflictingSubmoduleNames(t *testing.T) { storage := testStorage(t, nil) tree := NewTree("", testConfig(t, "conficting-submodule-names")) From ecdba2b0b2224b7e8f83866f671297f77331325a Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 2 Nov 2017 16:38:32 -0400 Subject: [PATCH 16/16] add transform step logs from GraphTransformMulti --- terraform/transform.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/terraform/transform.go b/terraform/transform.go index f4a431a674..0e47f208a7 100644 --- a/terraform/transform.go +++ b/terraform/transform.go @@ -1,6 +1,8 @@ package terraform import ( + "log" + "github.com/hashicorp/terraform/dag" ) @@ -40,6 +42,9 @@ func (t *graphTransformerMulti) Transform(g *Graph) error { if err := t.Transform(g); err != nil { return err } + log.Printf( + "[TRACE] Graph after step %T:\n\n%s", + t, g.StringWithNodeTypes()) } return nil