From 3297f60d03aac749a2d6c43480b9de0b2c5cd97a Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 6 Nov 2017 17:10:33 -0500 Subject: [PATCH 01/16] remove raw print statements --- config/loader_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/config/loader_test.go b/config/loader_test.go index 0bc3b8b55..e1ba75c96 100644 --- a/config/loader_test.go +++ b/config/loader_test.go @@ -851,7 +851,6 @@ func TestLoadFile_ignoreChanges(t *testing.T) { } actual := resourcesStr(c.Resources) - print(actual) if actual != strings.TrimSpace(ignoreChangesResourcesStr) { t.Fatalf("bad:\n%s", actual) } @@ -943,7 +942,6 @@ func TestLoad_hclAttributes(t *testing.T) { } actual := resourcesStr(c.Resources) - print(actual) if actual != strings.TrimSpace(jsonAttributeStr) { t.Fatalf("bad:\n%s", actual) } @@ -988,7 +986,6 @@ func TestLoad_jsonAttributes(t *testing.T) { } actual := resourcesStr(c.Resources) - print(actual) if actual != strings.TrimSpace(jsonAttributeStr) { t.Fatalf("bad:\n%s", actual) } From 57470f6bf51ed683732f512f3ca26b9d8f65e13d Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 6 Nov 2017 17:11:07 -0500 Subject: [PATCH 02/16] remove ProviderConfig Path and Inherited fields maving all inheritance into core --- config/config.go | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/config/config.go b/config/config.go index 96d63bf43..f36d90847 100644 --- a/config/config.go +++ b/config/config.go @@ -69,15 +69,6 @@ type ProviderConfig struct { Alias string Version string RawConfig *RawConfig - - // Path records where the Provider was declared in a module tree, so that - // it can be copied into child module providers yet still interpolated in - // the correct scope. - Path []string - - // Inherited is used to skip validation of this config, since any - // interpolated variables won't be declared at this level. - Inherited bool } // A resource represents a single Terraform resource in the configuration. @@ -817,10 +808,6 @@ func (c *Config) rawConfigs() map[string]*RawConfig { } for _, pc := range c.ProviderConfigs { - // this was an inherited config, so we don't validate it at this level. - if pc.Inherited { - continue - } source := fmt.Sprintf("provider config '%s'", pc.Name) result[source] = pc.RawConfig } From b15258dfec0ecbf8d714cf20f34d5afe9a2df132 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 6 Nov 2017 17:27:45 -0500 Subject: [PATCH 03/16] remove provider inheritance from Tree.Load Now that resources can be connected to providers with different paths in the core graph, handling the inheritance in config makes less sense. Removing this to make room for core to walk the Tree and connect resources directly to the proper provider instance. --- .../basic-parent-providers/a/a.tf | 13 --- .../basic-parent-providers/c/c.tf | 2 - .../basic-parent-providers/main.tf | 11 --- config/module/tree.go | 92 ------------------- config/module/tree_test.go | 64 ------------- 5 files changed, 182 deletions(-) delete mode 100644 config/module/test-fixtures/basic-parent-providers/a/a.tf delete mode 100644 config/module/test-fixtures/basic-parent-providers/c/c.tf delete mode 100644 config/module/test-fixtures/basic-parent-providers/main.tf diff --git a/config/module/test-fixtures/basic-parent-providers/a/a.tf b/config/module/test-fixtures/basic-parent-providers/a/a.tf deleted file mode 100644 index 8451bb1f9..000000000 --- a/config/module/test-fixtures/basic-parent-providers/a/a.tf +++ /dev/null @@ -1,13 +0,0 @@ -provider "top" {} - -provider "bottom" { - alias = "foo" - value = "from bottom" -} - -module "c" { - source = "../c" - providers = { - "bottom" = "bottom.foo" - } -} diff --git a/config/module/test-fixtures/basic-parent-providers/c/c.tf b/config/module/test-fixtures/basic-parent-providers/c/c.tf deleted file mode 100644 index d1e2809ce..000000000 --- a/config/module/test-fixtures/basic-parent-providers/c/c.tf +++ /dev/null @@ -1,2 +0,0 @@ -# Hello -provider "bottom" {} diff --git a/config/module/test-fixtures/basic-parent-providers/main.tf b/config/module/test-fixtures/basic-parent-providers/main.tf deleted file mode 100644 index b9009b6c6..000000000 --- a/config/module/test-fixtures/basic-parent-providers/main.tf +++ /dev/null @@ -1,11 +0,0 @@ -provider "top" { - alias = "foo" - value = "from top" -} - -module "a" { - source = "./a" - providers = { - "top" = "top.foo" - } -} diff --git a/config/module/tree.go b/config/module/tree.go index c649a4663..0bf6b93f7 100644 --- a/config/module/tree.go +++ b/config/module/tree.go @@ -188,11 +188,6 @@ func (t *Tree) Load(s *Storage) error { // Set our tree up t.children = children - // if we're the root module, we can now set the provider inheritance - if len(t.path) == 0 { - t.inheritProviderConfigs(nil) - } - return nil } @@ -348,93 +343,6 @@ func (t *Tree) getChildren(s *Storage) (map[string]*Tree, error) { return children, nil } -// inheritProviderConfig resolves all provider config inheritance after the -// tree is loaded. -// -// If there is a provider block without a config, look in the parent's Module -// block for a provider, and fetch that provider's configuration. If that -// doesn't exist, assume a default empty config. Implicit providers can still -// inherit their config all the way up from the root, so walk up the tree and -// copy the first matching provider into the module. -func (t *Tree) inheritProviderConfigs(stack []*Tree) { - // the recursive calls only append, so we don't need to worry about copying - // this slice. - stack = append(stack, t) - for _, c := range t.children { - c.inheritProviderConfigs(stack) - } - - if len(stack) == 1 { - return - } - - providers := make(map[string]*config.ProviderConfig) - missingProviders := make(map[string]bool) - - for _, p := range t.config.ProviderConfigs { - providers[p.FullName()] = p - } - - for _, r := range t.config.Resources { - p := r.ProviderFullName() - if _, ok := providers[p]; !(ok || strings.Contains(p, ".")) { - missingProviders[p] = true - } - } - - // get our parent's module config block - parent := stack[len(stack)-2] - var parentModule *config.Module - for _, m := range parent.config.Modules { - if m.Name == t.name { - parentModule = m - break - } - } - - if parentModule == nil { - panic("can't be a module without a parent module config") - } - - // now look for providers that need a config - for p, pc := range providers { - if len(pc.RawConfig.RawMap()) > 0 { - log.Printf("[TRACE] provider %q has a config, continuing", p) - continue - } - - // this provider has no config yet, check for one being passed in - parentProviderName, ok := parentModule.Providers[p] - if !ok { - continue - } - - var parentProvider *config.ProviderConfig - // there's a config for us in the parent module - for _, pp := range parent.config.ProviderConfigs { - if pp.FullName() == parentProviderName { - parentProvider = pp - break - } - } - - if parentProvider == nil { - // no config found, assume defaults - continue - } - - // Copy it in, but set an interpolation Scope. - // An interpolation Scope always need to have "root" - pc.Path = append([]string{RootName}, parent.path...) - pc.RawConfig = parentProvider.RawConfig - log.Printf("[TRACE] provider %q inheriting config from %q", - strings.Join(append(t.Path(), pc.FullName()), "."), - strings.Join(append(parent.Path(), parentProvider.FullName()), "."), - ) - } - -} - // Path is the full path to this tree. func (t *Tree) Path() []string { return t.path diff --git a/config/module/tree_test.go b/config/module/tree_test.go index acc622c3f..5e9abba4b 100644 --- a/config/module/tree_test.go +++ b/config/module/tree_test.go @@ -3,7 +3,6 @@ package module import ( "fmt" "io/ioutil" - "log" "os" "path/filepath" "reflect" @@ -548,69 +547,6 @@ func TestTreeValidate_unknownModule(t *testing.T) { } } -func TestTreeProviders_basic(t *testing.T) { - storage := testStorage(t, nil) - tree := NewTree("", testConfig(t, "basic-parent-providers")) - - storage.Mode = GetModeGet - if err := tree.Load(storage); err != nil { - t.Fatalf("err: %s", err) - } - - var a, b *Tree - for _, child := range tree.Children() { - if child.Name() == "a" { - a = child - } - } - - rootProviders := tree.config.ProviderConfigsByFullName() - topRaw := rootProviders["top.foo"] - - if a == nil { - t.Fatal("could not find module 'a'") - } - - for _, child := range a.Children() { - if child.Name() == "c" { - b = child - } - } - - if b == nil { - t.Fatal("could not find module 'c'") - } - - aProviders := a.config.ProviderConfigsByFullName() - bottomRaw := aProviders["bottom.foo"] - bProviders := b.config.ProviderConfigsByFullName() - bBottom := bProviders["bottom"] - - // compare the configs - // top.foo should have been copied to a.top - aTop := aProviders["top"] - if !reflect.DeepEqual(aTop.RawConfig.RawMap(), topRaw.RawConfig.RawMap()) { - log.Fatalf("expected config %#v, got %#v", - topRaw.RawConfig.RawMap(), - aTop.RawConfig.RawMap(), - ) - } - - if !reflect.DeepEqual(aTop.Path, []string{RootName}) { - log.Fatalf(`expected scope for "top": {"root"}, got %#v`, aTop.Path) - } - - if !reflect.DeepEqual(bBottom.RawConfig.RawMap(), bottomRaw.RawConfig.RawMap()) { - t.Fatalf("expected config %#v, got %#v", - bottomRaw.RawConfig.RawMap(), - bBottom.RawConfig.RawMap(), - ) - } - if !reflect.DeepEqual(bBottom.Path, []string{RootName, "a"}) { - t.Fatalf(`expected scope for "bottom": {"root", "a"}, got %#v`, bBottom.Path) - } -} - func TestTreeLoad_conflictingSubmoduleNames(t *testing.T) { storage := testStorage(t, nil) tree := NewTree("", testConfig(t, "conficting-submodule-names")) From 49e6ecfd7a27a06edc8975330600002de1e68c2c Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 6 Nov 2017 21:57:06 -0500 Subject: [PATCH 04/16] pass providers into modules via config Implement the adding of provider through the module/providers map in the configuration. The way this works is that we start walking the module tree from the top, and for any instance of a provider that can accept a configuration through the parent's module/provider map, we add a proxy node that provides the real name and a pointer to the actual parent provider node. Multiple proxies can be chained back to the original provider. When connecting resources to providers, if that provider is a proxy, we can then connect the resource directly to the proxied node. The proxies are later removed by the DisabledProviderTransformer. This should re-instate the 0.11 beta inheritance behavior, but will allow us to later store the actual concrete provider used by a resource, so that it can be re-connected if it's orphaned by removing its module configuration. --- terraform/eval_context_builtin.go | 7 +- terraform/node_provider_abstract.go | 2 +- terraform/transform_attach_config_provider.go | 62 ----- terraform/transform_config.go | 75 ------ terraform/transform_provider.go | 225 ++++++++++++++++-- terraform/transform_provider_disable.go | 12 +- terraform/transform_provider_test.go | 39 +++ 7 files changed, 259 insertions(+), 163 deletions(-) diff --git a/terraform/eval_context_builtin.go b/terraform/eval_context_builtin.go index 193421b78..1b6ee5a62 100644 --- a/terraform/eval_context_builtin.go +++ b/terraform/eval_context_builtin.go @@ -262,13 +262,8 @@ func (ctx *BuiltinEvalContext) InterpolateProvider( var cfg *config.RawConfig if pc != nil && pc.RawConfig != nil { - path := pc.Path - if len(path) == 0 { - path = ctx.Path() - } - scope := &InterpolationScope{ - Path: path, + Path: ctx.Path(), Resource: r, } diff --git a/terraform/node_provider_abstract.go b/terraform/node_provider_abstract.go index 3230558e8..2b346a44c 100644 --- a/terraform/node_provider_abstract.go +++ b/terraform/node_provider_abstract.go @@ -26,7 +26,7 @@ type NodeAbstractProvider struct { func ResolveProviderName(name string, path []string) string { name = fmt.Sprintf("provider.%s", name) - if len(path) > 1 { + if len(path) >= 1 { name = fmt.Sprintf("%s.%s", modulePrefixStr(path), name) } diff --git a/terraform/transform_attach_config_provider.go b/terraform/transform_attach_config_provider.go index 10506ea06..39cf097ae 100644 --- a/terraform/transform_attach_config_provider.go +++ b/terraform/transform_attach_config_provider.go @@ -1,10 +1,7 @@ package terraform import ( - "log" - "github.com/hashicorp/terraform/config" - "github.com/hashicorp/terraform/config/module" ) // GraphNodeAttachProvider is an interface that must be implemented by nodes @@ -19,62 +16,3 @@ type GraphNodeAttachProvider interface { // Sets the configuration AttachProvider(*config.ProviderConfig) } - -// AttachProviderConfigTransformer goes through the graph and attaches -// provider configuration structures to nodes that implement the interfaces -// above. -// -// The attached configuration structures are directly from the configuration. -// If they're going to be modified, a copy should be made. -type AttachProviderConfigTransformer struct { - Module *module.Tree // Module is the root module for the config -} - -func (t *AttachProviderConfigTransformer) Transform(g *Graph) error { - if err := t.attachProviders(g); err != nil { - return err - } - - return nil -} - -func (t *AttachProviderConfigTransformer) attachProviders(g *Graph) error { - // Go through and find GraphNodeAttachProvider - for _, v := range g.Vertices() { - // Only care about GraphNodeAttachProvider implementations - apn, ok := v.(GraphNodeAttachProvider) - if !ok { - continue - } - - // Determine what we're looking for - path := normalizeModulePath(apn.Path()) - path = path[1:] - name := apn.ProviderName() - log.Printf("[TRACE] Attach provider request: %#v %s", path, name) - - // Get the configuration. - tree := t.Module.Child(path) - if tree == nil { - continue - } - - // Go through the provider configs to find the matching config - for _, p := range tree.Config().ProviderConfigs { - // Build the name, which is "name.alias" if an alias exists - current := p.Name - if p.Alias != "" { - current += "." + p.Alias - } - - // If the configs match then attach! - if current == name { - log.Printf("[TRACE] Attaching provider config: %#v", p) - apn.AttachProvider(p) - break - } - } - } - - return nil -} diff --git a/terraform/transform_config.go b/terraform/transform_config.go index 7ec7744a7..61bce8532 100644 --- a/terraform/transform_config.go +++ b/terraform/transform_config.go @@ -133,78 +133,3 @@ 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 38821bae3..ac06fb5af 100644 --- a/terraform/transform_provider.go +++ b/terraform/transform_provider.go @@ -1,11 +1,13 @@ package terraform import ( + "errors" "fmt" "log" "strings" "github.com/hashicorp/go-multierror" + "github.com/hashicorp/terraform/config" "github.com/hashicorp/terraform/config/module" "github.com/hashicorp/terraform/dag" ) @@ -18,10 +20,6 @@ func TransformProviders(providers []string, concrete ConcreteProviderNodeFunc, m Providers: providers, Concrete: concrete, }, - // Attach configuration to each provider instance - &AttachProviderConfigTransformer{ - Module: mod, - }, // Add any remaining missing providers &MissingProviderTransformer{ Providers: providers, @@ -107,6 +105,13 @@ func (t *ProviderTransformer) Transform(g *Graph) error { break } + // see if this in an inherited provider + if p, ok := target.(*graphNodeProxyProvider); ok { + g.Remove(p) + target = p.Target + key = p.Target.Name() + } + log.Printf("[DEBUG] resource %s using provider %s", dag.VertexName(pv), key) pv.SetProvider(key) g.Connect(dag.BasicEdge(v, target)) @@ -349,21 +354,209 @@ func (n *graphNodeCloseProvider) RemoveIfNotTargeted() bool { return true } -// graphNodeProviderConsumerDummy is a struct that never enters the real -// graph (though it could to no ill effect). It implements -// GraphNodeProviderConsumer and GraphNodeSubpath as a way to force -// certain transformations. -type graphNodeProviderConsumerDummy struct { - ProviderValue string - PathValue []string +// graphNodeProxyProvider is a GraphNodeProvider implementation that is used to +// store the name and value of a provider node for inheritance between modules. +// These nodes are only used to store the data while loading the provider +// configurations, and are removed after all the resources have been connected +// to their providers. +type graphNodeProxyProvider struct { + NameValue string + Path []string + Target GraphNodeProvider } -func (n *graphNodeProviderConsumerDummy) Path() []string { - return n.PathValue +func (n *graphNodeProxyProvider) ProviderName() string { + return n.Target.ProviderName() } -func (n *graphNodeProviderConsumerDummy) ProvidedBy() string { - return n.ProviderValue +func (n *graphNodeProxyProvider) Name() string { + return ResolveProviderName(n.NameValue, n.Path) } -func (n *graphNodeProviderConsumerDummy) SetProvider(string) {} +// ProviderConfigTransformer adds all provider nodes from the configuration and +// attaches the configs. +type ProviderConfigTransformer struct { + Providers []string + Concrete ConcreteProviderNodeFunc + + // each provider node is stored here so that the proxy nodes can look up + // their targets by name. + providers map[string]GraphNodeProvider + + // 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") + } + + t.providers = make(map[string]GraphNodeProvider) + + // Start the transformation process + if err := t.transform(g, t.Module); err != nil { + return nil + } + + // finally attach the configs to the new nodes + return t.attachProviderConfigs(g) +} + +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...) + } + + // add all provider configs + for _, p := range conf.ProviderConfigs { + name := p.Name + if p.Alias != "" { + name += "." + p.Alias + } + + // if this is an empty config placeholder to accept a provier from a + // parent module, add a proxy and continue. + if t.addProxyProvider(g, m, p, name) { + continue + } + + v := t.Concrete(&NodeAbstractProvider{ + NameValue: name, + PathValue: path, + }) + + // Add it to the graph + g.Add(v) + t.providers[ResolveProviderName(name, path)] = v.(GraphNodeProvider) + } + + return nil +} + +// add a ProxyProviderConfig if this was inherited from a parent module. Return +// whether the proxy was added to the graph or not. +func (t *ProviderConfigTransformer) addProxyProvider(g *Graph, m *module.Tree, pc *config.ProviderConfig, name string) bool { + path := m.Path() + + // This isn't a proxy if there's a config, or we're at the root + if len(pc.RawConfig.RawMap()) > 0 || len(path) == 0 { + return false + } + + parentPath := path[:len(path)-1] + parent := t.Module.Child(parentPath) + if parent == nil { + return false + } + + var parentCfg *config.Module + for _, mod := range parent.Config().Modules { + if mod.Name == m.Name() { + parentCfg = mod + break + } + } + + if parentCfg == nil { + panic("immaculately conceived module " + m.Name()) + } + + parentProviderName, ok := parentCfg.Providers[name] + if !ok { + // this provider isn't listed in a parent module block, so we just have + // an empty config + return false + } + + // the parent module is passing in a provider + fullParentName := ResolveProviderName(parentProviderName, parentPath) + parentProvider := t.providers[fullParentName] + + if parentProvider == nil { + panic(fmt.Sprintf("missing provider %s in module %s", parentProviderName, m.Name())) + } + + v := &graphNodeProxyProvider{ + NameValue: name, + Path: path, + Target: parentProvider, + } + + // Add it to the graph + g.Add(v) + t.providers[v.NameValue] = v + return true +} + +func (t *ProviderConfigTransformer) attachProviderConfigs(g *Graph) error { + for _, v := range g.Vertices() { + // Only care about GraphNodeAttachProvider implementations + apn, ok := v.(GraphNodeAttachProvider) + if !ok { + continue + } + + // Determine what we're looking for + path := normalizeModulePath(apn.Path())[1:] + name := apn.ProviderName() + log.Printf("[TRACE] Attach provider request: %#v %s", path, name) + + // Get the configuration. + tree := t.Module.Child(path) + if tree == nil { + continue + } + + // Go through the provider configs to find the matching config + for _, p := range tree.Config().ProviderConfigs { + // Build the name, which is "name.alias" if an alias exists + current := p.Name + if p.Alias != "" { + current += "." + p.Alias + } + + // If the configs match then attach! + if current == name { + log.Printf("[TRACE] Attaching provider config: %#v", p) + apn.AttachProvider(p) + break + } + } + } + + return nil +} diff --git a/terraform/transform_provider_disable.go b/terraform/transform_provider_disable.go index d9919f3a7..656a6385c 100644 --- a/terraform/transform_provider_disable.go +++ b/terraform/transform_provider_disable.go @@ -7,9 +7,9 @@ import ( ) // DisableProviderTransformer "disables" any providers that are not actually -// used by anything. This avoids the provider being initialized and configured. -// This both saves resources but also avoids errors since configuration -// may imply initialization which may require auth. +// used by anything, and provider proxies. This avoids the provider being +// initialized and configured. This both saves resources but also avoids +// errors since configuration may imply initialization which may require auth. type DisableProviderTransformer struct{} func (t *DisableProviderTransformer) Transform(g *Graph) error { @@ -20,6 +20,12 @@ func (t *DisableProviderTransformer) Transform(g *Graph) error { continue } + // remove the proxy nodes now that we're done with them + if pn, ok := v.(*graphNodeProxyProvider); ok { + g.Remove(pn) + continue + } + // If we have dependencies, then don't disable if g.UpEdges(v).Len() > 0 { continue diff --git a/terraform/transform_provider_test.go b/terraform/transform_provider_test.go index 42995b8a3..36c9c5e07 100644 --- a/terraform/transform_provider_test.go +++ b/terraform/transform_provider_test.go @@ -445,6 +445,39 @@ func TestPruneProviderTransformer(t *testing.T) { } } +// the child module resource is attached to the configured parent provider +func TestProviderConfigTransformer_parentProviders(t *testing.T) { + mod := testModule(t, "transform-provider-inherit") + 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) + } + } + { + tf := &AttachResourceConfigTransformer{Module: mod} + if err := tf.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + + { + tf := TransformProviders([]string{"aws"}, concrete, mod) + if err := tf.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + + actual := strings.TrimSpace(g.String()) + expected := strings.TrimSpace(testTransformModuleProviderConfigStr) + if actual != expected { + t.Fatalf("expected:\n%s\n\ngot:\n%s", expected, actual) + } +} + const testTransformProviderBasicStr = ` aws_instance.web provider.aws @@ -545,3 +578,9 @@ provider.aws (close) provider.aws var.foo ` + +const testTransformModuleProviderConfigStr = ` +module.child.aws_instance.thing + provider.aws.foo +provider.aws.foo +` From b9b4912bfbe4755b561de21a54dfbd5363904e43 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 6 Nov 2017 22:21:18 -0500 Subject: [PATCH 05/16] complete passing providers through modules Here we complete the passing of providers between modules via the module/providers configuration, add another test and update broken test outputs. The DisbableProviderTransformer is being removed, since it was really only for provider configuration inheritance. Since configuration is no longer inherited, there's no need to keep around unused providers. The actually shouldn't be any unused providers going into the graph any longer, but put off verifying that condition for later. Replace it's usage with the PruneProviderTransformer, and use that to also remove the unneeded proxy provider nodes. --- .../child/grandchild/main.tf | 7 +++ .../child/main.tf | 10 +++ .../main.tf | 11 ++++ .../transform-provider-inherit/child/main.tf | 7 +++ .../transform-provider-inherit/main.tf | 11 ++++ terraform/transform_provider.go | 63 ++++++++++++------- terraform/transform_provider_disable.go | 56 ----------------- terraform/transform_provider_test.go | 41 +++++++++++- 8 files changed, 125 insertions(+), 81 deletions(-) create mode 100644 terraform/test-fixtures/transform-provider-grandchild-inherit/child/grandchild/main.tf create mode 100644 terraform/test-fixtures/transform-provider-grandchild-inherit/child/main.tf create mode 100644 terraform/test-fixtures/transform-provider-grandchild-inherit/main.tf create mode 100644 terraform/test-fixtures/transform-provider-inherit/child/main.tf create mode 100644 terraform/test-fixtures/transform-provider-inherit/main.tf delete mode 100644 terraform/transform_provider_disable.go diff --git a/terraform/test-fixtures/transform-provider-grandchild-inherit/child/grandchild/main.tf b/terraform/test-fixtures/transform-provider-grandchild-inherit/child/grandchild/main.tf new file mode 100644 index 000000000..58363ef0c --- /dev/null +++ b/terraform/test-fixtures/transform-provider-grandchild-inherit/child/grandchild/main.tf @@ -0,0 +1,7 @@ +provider "aws" { + alias = "baz" +} + +resource "aws_instance" "baz" { + provider = "aws.baz" +} diff --git a/terraform/test-fixtures/transform-provider-grandchild-inherit/child/main.tf b/terraform/test-fixtures/transform-provider-grandchild-inherit/child/main.tf new file mode 100644 index 000000000..6c1285513 --- /dev/null +++ b/terraform/test-fixtures/transform-provider-grandchild-inherit/child/main.tf @@ -0,0 +1,10 @@ +provider "aws" { + alias = "bar" +} + +module "grandchild" { + source = "./grandchild" + providers = { + "aws.baz" = "aws.bar" + } +} diff --git a/terraform/test-fixtures/transform-provider-grandchild-inherit/main.tf b/terraform/test-fixtures/transform-provider-grandchild-inherit/main.tf new file mode 100644 index 000000000..b9d39e9c7 --- /dev/null +++ b/terraform/test-fixtures/transform-provider-grandchild-inherit/main.tf @@ -0,0 +1,11 @@ +provider "aws" { + alias = "foo" + value = "config" +} + +module "child" { + source = "child" + providers = { + "aws.bar" = "aws.foo" + } +} diff --git a/terraform/test-fixtures/transform-provider-inherit/child/main.tf b/terraform/test-fixtures/transform-provider-inherit/child/main.tf new file mode 100644 index 000000000..fc598d7fc --- /dev/null +++ b/terraform/test-fixtures/transform-provider-inherit/child/main.tf @@ -0,0 +1,7 @@ +provider "aws" { + alias = "bar" +} + +resource "aws_instance" "thing" { + provider = "aws.bar" +} diff --git a/terraform/test-fixtures/transform-provider-inherit/main.tf b/terraform/test-fixtures/transform-provider-inherit/main.tf new file mode 100644 index 000000000..b9d39e9c7 --- /dev/null +++ b/terraform/test-fixtures/transform-provider-inherit/main.tf @@ -0,0 +1,11 @@ +provider "aws" { + alias = "foo" + value = "config" +} + +module "child" { + source = "child" + providers = { + "aws.bar" = "aws.foo" + } +} diff --git a/terraform/transform_provider.go b/terraform/transform_provider.go index ac06fb5af..5a2af25f7 100644 --- a/terraform/transform_provider.go +++ b/terraform/transform_provider.go @@ -27,8 +27,8 @@ func TransformProviders(providers []string, concrete ConcreteProviderNodeFunc, m }, // Connect the providers &ProviderTransformer{}, - // Disable unused providers - &DisableProviderTransformer{}, + // Remove unused providers and proxies + &PruneProviderTransformer{}, // Connect provider to their parent provider nodes &ParentProviderTransformer{}, ) @@ -108,8 +108,8 @@ func (t *ProviderTransformer) Transform(g *Graph) error { // see if this in an inherited provider if p, ok := target.(*graphNodeProxyProvider); ok { g.Remove(p) - target = p.Target - key = p.Target.Name() + target = p.Target() + key = target.(GraphNodeProvider).Name() } log.Printf("[DEBUG] resource %s using provider %s", dag.VertexName(pv), key) @@ -253,22 +253,29 @@ func (t *ParentProviderTransformer) Transform(g *Graph) error { return nil } -// PruneProviderTransformer is a GraphTransformer that prunes all the -// providers that aren't needed from the graph. A provider is unneeded if -// no resource or module is using that provider. +// PruneProviderTransformer removes any providers that are not actually used by +// anything, and provider proxies. This avoids the provider being initialized +// and configured. This both saves resources but also avoids errors since +// configuration may imply initialization which may require auth. type PruneProviderTransformer struct{} func (t *PruneProviderTransformer) Transform(g *Graph) error { for _, v := range g.Vertices() { - // We only care about the providers - if pn, ok := v.(GraphNodeProvider); !ok || pn.ProviderName() == "" { + // We only care about providers + pn, ok := v.(GraphNodeProvider) + if !ok || pn.ProviderName() == "" { continue } - // Does anything depend on this? If not, then prune it. - if s := g.UpEdges(v); s.Len() == 0 { - if nv, ok := v.(dag.NamedVertex); ok { - log.Printf("[DEBUG] Pruning provider with no dependencies: %s", nv.Name()) - } + + // ProxyProviders will have up edges, but we're now done with them in the graph + if _, ok := v.(*graphNodeProxyProvider); ok { + log.Printf("[DEBUG] pruning proxy provider %s", dag.VertexName(v)) + g.Remove(v) + } + + // Remove providers with no dependencies. + if g.UpEdges(v).Len() == 0 { + log.Printf("[DEBUG] pruning unused provider %s", dag.VertexName(v)) g.Remove(v) } } @@ -360,17 +367,27 @@ func (n *graphNodeCloseProvider) RemoveIfNotTargeted() bool { // configurations, and are removed after all the resources have been connected // to their providers. type graphNodeProxyProvider struct { - NameValue string - Path []string - Target GraphNodeProvider + nameValue string + path []string + target GraphNodeProvider } func (n *graphNodeProxyProvider) ProviderName() string { - return n.Target.ProviderName() + return n.Target().ProviderName() } func (n *graphNodeProxyProvider) Name() string { - return ResolveProviderName(n.NameValue, n.Path) + return ResolveProviderName(n.nameValue, n.path) +} + +// find the concrete provider instance +func (n *graphNodeProxyProvider) Target() GraphNodeProvider { + switch t := n.target.(type) { + case *graphNodeProxyProvider: + return t.Target() + default: + return n.target + } } // ProviderConfigTransformer adds all provider nodes from the configuration and @@ -511,14 +528,14 @@ func (t *ProviderConfigTransformer) addProxyProvider(g *Graph, m *module.Tree, p } v := &graphNodeProxyProvider{ - NameValue: name, - Path: path, - Target: parentProvider, + nameValue: name, + path: path, + target: parentProvider, } // Add it to the graph g.Add(v) - t.providers[v.NameValue] = v + t.providers[ResolveProviderName(name, path)] = v return true } diff --git a/terraform/transform_provider_disable.go b/terraform/transform_provider_disable.go deleted file mode 100644 index 656a6385c..000000000 --- a/terraform/transform_provider_disable.go +++ /dev/null @@ -1,56 +0,0 @@ -package terraform - -import ( - "fmt" - - "github.com/hashicorp/terraform/dag" -) - -// DisableProviderTransformer "disables" any providers that are not actually -// used by anything, and provider proxies. This avoids the provider being -// initialized and configured. This both saves resources but also avoids -// errors since configuration may imply initialization which may require auth. -type DisableProviderTransformer struct{} - -func (t *DisableProviderTransformer) Transform(g *Graph) error { - for _, v := range g.Vertices() { - // We only care about providers - pn, ok := v.(GraphNodeProvider) - if !ok || pn.ProviderName() == "" { - continue - } - - // remove the proxy nodes now that we're done with them - if pn, ok := v.(*graphNodeProxyProvider); ok { - g.Remove(pn) - continue - } - - // If we have dependencies, then don't disable - if g.UpEdges(v).Len() > 0 { - continue - } - - // Get the path - var path []string - if pn, ok := v.(GraphNodeSubPath); ok { - path = pn.Path() - } - - // Disable the provider by replacing it with a "disabled" provider - disabled := &NodeDisabledProvider{ - NodeAbstractProvider: &NodeAbstractProvider{ - NameValue: pn.ProviderName(), - PathValue: path, - }, - } - - if !g.Replace(v, disabled) { - panic(fmt.Sprintf( - "vertex disappeared from under us: %s", - dag.VertexName(v))) - } - } - - return nil -} diff --git a/terraform/transform_provider_test.go b/terraform/transform_provider_test.go index 36c9c5e07..f4277060a 100644 --- a/terraform/transform_provider_test.go +++ b/terraform/transform_provider_test.go @@ -478,6 +478,39 @@ func TestProviderConfigTransformer_parentProviders(t *testing.T) { } } +// the child module resource is attached to the configured grand-parent provider +func TestProviderConfigTransformer_grandparentProviders(t *testing.T) { + mod := testModule(t, "transform-provider-grandchild-inherit") + 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) + } + } + { + tf := &AttachResourceConfigTransformer{Module: mod} + if err := tf.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + + { + tf := TransformProviders([]string{"aws"}, concrete, mod) + if err := tf.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + + actual := strings.TrimSpace(g.String()) + expected := strings.TrimSpace(testTransformModuleProviderGrandparentStr) + if actual != expected { + t.Fatalf("expected:\n%s\n\ngot:\n%s", expected, actual) + } +} + const testTransformProviderBasicStr = ` aws_instance.web provider.aws @@ -514,9 +547,7 @@ module.sub.module.subsub.bar_instance.two module.sub.module.subsub.foo_instance.one module.sub.provider.foo module.sub.provider.foo - provider.foo (disabled) provider.bar -provider.foo (disabled) ` const testTransformMissingProviderModuleChildStr = ` @@ -584,3 +615,9 @@ module.child.aws_instance.thing provider.aws.foo provider.aws.foo ` + +const testTransformModuleProviderGrandparentStr = ` +module.child.module.grandchild.aws_instance.baz + provider.aws.foo +provider.aws.foo +` From 990acca758d477a693e7f46a9183648c620d7620 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 7 Nov 2017 10:15:11 -0500 Subject: [PATCH 06/16] remove commented out fields --- terraform/graph_walk_context.go | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/terraform/graph_walk_context.go b/terraform/graph_walk_context.go index c2cca1499..89f376e54 100644 --- a/terraform/graph_walk_context.go +++ b/terraform/graph_walk_context.go @@ -66,13 +66,12 @@ func (w *ContextGraphWalker) EnterPath(path []string) EvalContext { w.interpolaterVarLock.Unlock() ctx := &BuiltinEvalContext{ - StopContext: w.StopContext, - PathValue: path, - Hooks: w.Context.hooks, - InputValue: w.Context.uiInput, - Components: w.Context.components, - ProviderCache: w.providerCache, - //ProviderConfigCache: w.providerConfigCache, + StopContext: w.StopContext, + PathValue: path, + Hooks: w.Context.hooks, + InputValue: w.Context.uiInput, + Components: w.Context.components, + ProviderCache: w.providerCache, ProviderInputConfig: w.Context.providerInputConfig, ProviderLock: &w.providerLock, ProvisionerCache: w.provisionerCache, @@ -150,7 +149,6 @@ func (w *ContextGraphWalker) ExitEvalTree( func (w *ContextGraphWalker) init() { w.contexts = make(map[string]*BuiltinEvalContext, 5) w.providerCache = make(map[string]ResourceProvider, 5) - //w.providerConfigCache = make(map[string]*ResourceConfig, 5) w.provisionerCache = make(map[string]ResourceProvisioner, 5) w.interpolaterVars = make(map[string]map[string]interface{}, 5) } From b79adeae029a7573c9bebe60b72532a1b58780ad Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 7 Nov 2017 13:09:36 -0500 Subject: [PATCH 07/16] save resolved providers for resources to state Use the ResourceState.Provider field to store the full name of the provider used during apply. This field is only used when a resource is removed from the config, and will allow that resource to be removed by the exact same provider with which it was created. Modify the locations which might accept the alue of the ResourceState.Provider field to detect that the name is resolved. --- config/config.go | 4 +++- terraform/context_test.go | 1 + terraform/node_data_refresh.go | 4 +++- terraform/node_provider_abstract.go | 6 ++++++ terraform/node_resource_apply.go | 6 +++--- terraform/node_resource_destroy.go | 4 +++- terraform/node_resource_plan_instance.go | 4 ++-- terraform/node_resource_refresh.go | 2 +- terraform/transform_deposed.go | 4 ++-- terraform/transform_provider.go | 9 +++++++-- 10 files changed, 31 insertions(+), 13 deletions(-) diff --git a/config/config.go b/config/config.go index f36d90847..ebf30e0cd 100644 --- a/config/config.go +++ b/config/config.go @@ -261,7 +261,9 @@ func (r *Resource) ProviderFullName() string { // the provider name is inferred from the resource type name. func ResourceProviderFullName(resourceType, explicitProvider string) string { if explicitProvider != "" { - return explicitProvider + // check for an explicit provider name, or return the original + parts := strings.SplitAfter(explicitProvider, "provider.") + return parts[len(parts)-1] } idx := strings.IndexRune(resourceType, '_') diff --git a/terraform/context_test.go b/terraform/context_test.go index b7064bef0..eec91e3aa 100644 --- a/terraform/context_test.go +++ b/terraform/context_test.go @@ -147,6 +147,7 @@ func TestNewContextState(t *testing.T) { } func testContext2(t *testing.T, opts *ContextOpts) *Context { + t.Helper() // Enable the shadow graph opts.Shadow = true diff --git a/terraform/node_data_refresh.go b/terraform/node_data_refresh.go index 15d9b8f9c..7ab76c235 100644 --- a/terraform/node_data_refresh.go +++ b/terraform/node_data_refresh.go @@ -108,7 +108,9 @@ func (n *NodeRefreshableDataResourceInstance) EvalTree() EvalNode { // Get the state if we have it, if not we build it rs := n.ResourceState if rs == nil { - rs = &ResourceState{} + rs = &ResourceState{ + Provider: n.ResolvedProvider, + } } // If the config isn't empty we update the state diff --git a/terraform/node_provider_abstract.go b/terraform/node_provider_abstract.go index 2b346a44c..9e490f7b4 100644 --- a/terraform/node_provider_abstract.go +++ b/terraform/node_provider_abstract.go @@ -2,6 +2,7 @@ package terraform import ( "fmt" + "strings" "github.com/hashicorp/terraform/config" "github.com/hashicorp/terraform/dag" @@ -25,6 +26,11 @@ type NodeAbstractProvider struct { } func ResolveProviderName(name string, path []string) string { + if strings.Contains(name, "provider.") { + // already resolved + return name + } + name = fmt.Sprintf("provider.%s", name) if len(path) >= 1 { name = fmt.Sprintf("%s.%s", modulePrefixStr(path), name) diff --git a/terraform/node_resource_apply.go b/terraform/node_resource_apply.go index 807b1f416..9f6d69fd3 100644 --- a/terraform/node_resource_apply.go +++ b/terraform/node_resource_apply.go @@ -158,7 +158,7 @@ func (n *NodeApplyableResource) evalTreeDataResource( &EvalWriteState{ Name: stateId, ResourceType: n.Config.Type, - Provider: n.Config.Provider, + Provider: n.ResolvedProvider, Dependencies: stateDeps, State: &state, }, @@ -308,7 +308,7 @@ func (n *NodeApplyableResource) evalTreeManagedResource( &EvalWriteState{ Name: stateId, ResourceType: n.Config.Type, - Provider: n.Config.Provider, + Provider: n.ResolvedProvider, Dependencies: stateDeps, State: &state, }, @@ -332,7 +332,7 @@ func (n *NodeApplyableResource) evalTreeManagedResource( Else: &EvalWriteState{ Name: stateId, ResourceType: n.Config.Type, - Provider: n.Config.Provider, + Provider: n.ResolvedProvider, Dependencies: stateDeps, State: &state, }, diff --git a/terraform/node_resource_destroy.go b/terraform/node_resource_destroy.go index cffb9ae60..74a1e5494 100644 --- a/terraform/node_resource_destroy.go +++ b/terraform/node_resource_destroy.go @@ -149,7 +149,9 @@ func (n *NodeDestroyResource) EvalTree() EvalNode { // Get our state rs := n.ResourceState if rs == nil { - rs = &ResourceState{} + rs = &ResourceState{ + Provider: n.ResolvedProvider, + } } var diffApply *InstanceDiff diff --git a/terraform/node_resource_plan_instance.go b/terraform/node_resource_plan_instance.go index 25a76a99f..7d9fcddb5 100644 --- a/terraform/node_resource_plan_instance.go +++ b/terraform/node_resource_plan_instance.go @@ -112,7 +112,7 @@ func (n *NodePlannableResourceInstance) evalTreeDataResource( &EvalWriteState{ Name: stateId, ResourceType: n.Config.Type, - Provider: n.Config.Provider, + Provider: n.ResolvedProvider, Dependencies: stateDeps, State: &state, }, @@ -177,7 +177,7 @@ func (n *NodePlannableResourceInstance) evalTreeManagedResource( &EvalWriteState{ Name: stateId, ResourceType: n.Config.Type, - Provider: n.Config.Provider, + Provider: n.ResolvedProvider, Dependencies: stateDeps, State: &state, }, diff --git a/terraform/node_resource_refresh.go b/terraform/node_resource_refresh.go index d504e7de4..c18c95f96 100644 --- a/terraform/node_resource_refresh.go +++ b/terraform/node_resource_refresh.go @@ -251,7 +251,7 @@ func (n *NodeRefreshableManagedResourceInstance) evalTreeManagedResourceNoState( &EvalWriteState{ Name: stateID, ResourceType: n.Config.Type, - Provider: n.Config.Provider, + Provider: n.ResolvedProvider, Dependencies: stateDeps, State: &state, }, diff --git a/terraform/transform_deposed.go b/terraform/transform_deposed.go index c3c87a299..87a1f9c98 100644 --- a/terraform/transform_deposed.go +++ b/terraform/transform_deposed.go @@ -108,7 +108,7 @@ func (n *graphNodeDeposedResource) EvalTree() EvalNode { &EvalWriteStateDeposed{ Name: n.ResourceName, ResourceType: n.ResourceType, - Provider: n.ProviderName, + Provider: n.ResolvedProvider, State: &state, Index: n.Index, }, @@ -157,7 +157,7 @@ func (n *graphNodeDeposedResource) EvalTree() EvalNode { &EvalWriteStateDeposed{ Name: n.ResourceName, ResourceType: n.ResourceType, - Provider: n.ProviderName, + Provider: n.ResolvedProvider, State: &state, Index: n.Index, }, diff --git a/terraform/transform_provider.go b/terraform/transform_provider.go index 5a2af25f7..6d7ab28e9 100644 --- a/terraform/transform_provider.go +++ b/terraform/transform_provider.go @@ -52,7 +52,8 @@ type GraphNodeCloseProvider interface { // GraphNodeProviderConsumer is an interface that nodes that require // a provider must implement. ProvidedBy must return the name of the provider -// to use. +// to use. This may be a provider by type, type.alias or a fully resolved +// provider name type GraphNodeProviderConsumer interface { ProvidedBy() string // Set the resolved provider address for this resource. @@ -127,7 +128,6 @@ 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) @@ -286,6 +286,11 @@ 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 { + if strings.Contains(k, "provider.") { + // this is already resolved + return k + } + // we create a dummy provider to var path []string if sp, ok := v.(GraphNodeSubPath); ok { From 8dfaae1f2305041856fad8f9edf449a647916a7c Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 7 Nov 2017 13:49:10 -0500 Subject: [PATCH 08/16] rewrite all of the test state string for providers Now that the resolved provider is always stored in state, we need to udpate all the test data to match. There will probably be some more breakage once the provider field is properly diffed. --- terraform/context_apply_test.go | 41 +++++++++++++- terraform/context_input_test.go | 1 + terraform/context_test.go | 2 + terraform/terraform_test.go | 99 ++++++++++++++++++++++++++++++++- 4 files changed, 139 insertions(+), 4 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index c2e23dffb..52ed20daa 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -129,6 +129,7 @@ func TestContext2Apply_escape(t *testing.T) { checkStateString(t, state, ` aws_instance.bar: ID = foo + provider = provider.aws foo = "bar" type = aws_instance `) @@ -160,6 +161,7 @@ func TestContext2Apply_resourceCountOneList(t *testing.T) { actual := strings.TrimSpace(state.String()) expected := strings.TrimSpace(`null_resource.foo: ID = foo + provider = provider.null Outputs: @@ -578,6 +580,7 @@ amis_from_module = {eu-west-1:ami-789012 eu-west-2:ami-989484 us-west-1:ami-1234 module.test: null_resource.noop: ID = foo + provider = provider.null Outputs: @@ -745,6 +748,7 @@ func TestContext2Apply_providerWarning(t *testing.T) { expected := strings.TrimSpace(` aws_instance.foo: ID = foo + provider = provider.aws `) if actual != expected { t.Fatalf("got: \n%s\n\nexpected:\n%s", actual, expected) @@ -1032,11 +1036,13 @@ func TestContext2Apply_createBeforeDestroy_dependsNonCBD(t *testing.T) { checkStateString(t, state, ` aws_instance.bar: ID = foo + provider = provider.aws require_new = yes type = aws_instance value = foo aws_instance.foo: ID = foo + provider = provider.aws require_new = yes type = aws_instance `) @@ -1170,10 +1176,12 @@ func TestContext2Apply_createBeforeDestroy_deposedCount(t *testing.T) { checkStateString(t, state, ` aws_instance.bar.0: ID = foo + provider = provider.aws foo = bar type = aws_instance aws_instance.bar.1: ID = foo + provider = provider.aws foo = bar type = aws_instance `) @@ -1233,6 +1241,7 @@ func TestContext2Apply_createBeforeDestroy_deposedOnly(t *testing.T) { checkStateString(t, state, ` aws_instance.bar: ID = bar + provider = provider.aws `) } @@ -2007,6 +2016,7 @@ func TestContext2Apply_cancelBlock(t *testing.T) { checkStateString(t, state, ` aws_instance.foo: ID = foo + provider = provider.aws `) } @@ -2066,6 +2076,7 @@ func TestContext2Apply_cancelProvisioner(t *testing.T) { checkStateString(t, state, ` aws_instance.foo: (tainted) ID = foo + provider = provider.aws num = 2 type = aws_instance `) @@ -2448,10 +2459,12 @@ func TestContext2Apply_mapVariableOverride(t *testing.T) { expected := strings.TrimSpace(` aws_instance.bar: ID = foo + provider = provider.aws ami = overridden type = aws_instance aws_instance.foo: ID = foo + provider = provider.aws ami = image-1234 type = aws_instance `) @@ -2619,7 +2632,7 @@ func TestContext2Apply_moduleInheritAlias(t *testing.T) { module.child: aws_instance.foo: ID = foo - provider = aws.eu + provider = provider.aws.eu `) } @@ -3140,6 +3153,7 @@ func TestContext2Apply_moduleTarget(t *testing.T) { module.A: aws_instance.foo: ID = foo + provider = provider.aws foo = bar type = aws_instance @@ -3149,6 +3163,7 @@ module.A: module.B: aws_instance.bar: ID = foo + provider = provider.aws foo = foo type = aws_instance `) @@ -3929,6 +3944,7 @@ func TestContext2Apply_outputDependsOn(t *testing.T) { checkStateString(t, state, ` aws_instance.foo: ID = foo + provider = provider.aws Outputs: @@ -4560,6 +4576,7 @@ func TestContext2Apply_multiDepose_createBeforeDestroy(t *testing.T) { checkStateString(t, state, ` aws_instance.web: (1 deposed) ID = bar + provider = provider.aws Deposed ID 1 = foo `) @@ -4584,6 +4601,7 @@ aws_instance.web: (1 deposed) checkStateString(t, state, ` aws_instance.web: (2 deposed) ID = baz + provider = provider.aws Deposed ID 1 = foo Deposed ID 2 = bar `) @@ -4611,6 +4629,7 @@ aws_instance.web: (2 deposed) checkStateString(t, state, ` aws_instance.web: (1 deposed) ID = qux + provider = provider.aws Deposed ID 1 = bar `) @@ -4632,6 +4651,7 @@ aws_instance.web: (1 deposed) checkStateString(t, state, ` aws_instance.web: ID = quux + provider = provider.aws `) } @@ -4672,6 +4692,7 @@ func TestContext2Apply_provisionerFailContinue(t *testing.T) { checkStateString(t, state, ` aws_instance.foo: ID = foo + provider = provider.aws foo = bar type = aws_instance `) @@ -5067,6 +5088,7 @@ func TestContext2Apply_provisionerDestroyTainted(t *testing.T) { checkStateString(t, state, ` aws_instance.foo: ID = foo + provider = provider.aws foo = bar type = aws_instance `) @@ -7370,6 +7392,7 @@ func TestContext2Apply_targeted(t *testing.T) { checkStateString(t, state, ` aws_instance.foo: ID = foo + provider = provider.aws num = 2 type = aws_instance `) @@ -7402,10 +7425,13 @@ func TestContext2Apply_targetedCount(t *testing.T) { checkStateString(t, state, ` aws_instance.foo.0: ID = foo + provider = provider.aws aws_instance.foo.1: ID = foo + provider = provider.aws aws_instance.foo.2: ID = foo + provider = provider.aws `) } @@ -7436,6 +7462,7 @@ func TestContext2Apply_targetedCountIndex(t *testing.T) { checkStateString(t, state, ` aws_instance.foo.1: ID = foo + provider = provider.aws `) } @@ -7673,10 +7700,12 @@ func TestContext2Apply_targetedModule(t *testing.T) { module.child: aws_instance.bar: ID = foo + provider = provider.aws num = 2 type = aws_instance aws_instance.foo: ID = foo + provider = provider.aws num = 2 type = aws_instance `) @@ -7712,6 +7741,7 @@ func TestContext2Apply_targetedModuleDep(t *testing.T) { checkStateString(t, state, ` aws_instance.foo: ID = foo + provider = provider.aws foo = foo type = aws_instance @@ -7721,6 +7751,7 @@ aws_instance.foo: module.child: aws_instance.mod: ID = foo + provider = provider.aws Outputs: @@ -7795,6 +7826,7 @@ module.child1: module.child2: aws_instance.foo: ID = foo + provider = provider.aws Outputs: @@ -7836,6 +7868,7 @@ func TestContext2Apply_targetedModuleResource(t *testing.T) { module.child: aws_instance.foo: ID = foo + provider = provider.aws num = 2 type = aws_instance `) @@ -8289,6 +8322,7 @@ func TestContext2Apply_issue5254(t *testing.T) { expected := strings.TrimSpace(` template_file.child: ID = foo + provider = provider.template template = Hi type = template_file @@ -8296,6 +8330,7 @@ template_file.child: template_file.parent.* template_file.parent: ID = foo + provider = provider.template template = Hi type = template_file `) @@ -8371,6 +8406,7 @@ func TestContext2Apply_targetedWithTaintedInState(t *testing.T) { expected := strings.TrimSpace(` aws_instance.iambeingadded: ID = foo + provider = provider.aws aws_instance.ifailedprovisioners: (tainted) ID = ifailedprovisioners `) @@ -8416,6 +8452,7 @@ func TestContext2Apply_ignoreChangesCreate(t *testing.T) { expected := strings.TrimSpace(` aws_instance.foo: ID = foo + provider = provider.aws required_field = set type = aws_instance `) @@ -8560,6 +8597,7 @@ func TestContext2Apply_ignoreChangesWildcard(t *testing.T) { expected := strings.TrimSpace(` aws_instance.foo: ID = foo + provider = provider.aws required_field = set type = aws_instance `) @@ -8820,6 +8858,7 @@ func TestContext2Apply_targetedModuleRecursive(t *testing.T) { module.child.subchild: aws_instance.foo: ID = foo + provider = provider.aws num = 2 type = aws_instance `) diff --git a/terraform/context_input_test.go b/terraform/context_input_test.go index 8c2099855..0e94bf3c9 100644 --- a/terraform/context_input_test.go +++ b/terraform/context_input_test.go @@ -564,6 +564,7 @@ func TestContext2Input_varWithDefault(t *testing.T) { expectedStr := strings.TrimSpace(` aws_instance.foo: ID = foo + provider = provider.aws foo = 123 type = aws_instance `) diff --git a/terraform/context_test.go b/terraform/context_test.go index eec91e3aa..f528f2602 100644 --- a/terraform/context_test.go +++ b/terraform/context_test.go @@ -361,6 +361,7 @@ func testProvisioner() *MockResourceProvisioner { } func checkStateString(t *testing.T, state *State, expected string) { + t.Helper() actual := strings.TrimSpace(state.String()) expected = strings.TrimSpace(expected) @@ -381,6 +382,7 @@ func resourceState(resourceType, resourceID string) *ResourceState { // Test helper that gives a function 3 seconds to finish, assumes deadlock and // fails test if it does not. func testCheckDeadlock(t *testing.T, f func()) { + t.Helper() timeout := make(chan bool, 1) done := make(chan bool, 1) go func() { diff --git a/terraform/terraform_test.go b/terraform/terraform_test.go index 15e920796..4d645a9f7 100644 --- a/terraform/terraform_test.go +++ b/terraform/terraform_test.go @@ -220,11 +220,13 @@ func (h *HookRecordApplyOrder) PreApply( const testTerraformInputProviderStr = ` aws_instance.bar: ID = foo + provider = provider.aws bar = override foo = us-east-1 type = aws_instance aws_instance.foo: ID = foo + provider = provider.aws bar = baz num = 2 type = aws_instance @@ -233,6 +235,7 @@ aws_instance.foo: const testTerraformInputProviderOnlyStr = ` aws_instance.foo: ID = foo + provider = provider.aws foo = us-west-2 type = aws_instance ` @@ -240,6 +243,7 @@ aws_instance.foo: const testTerraformInputVarOnlyStr = ` aws_instance.foo: ID = foo + provider = provider.aws foo = us-east-1 type = aws_instance ` @@ -247,6 +251,7 @@ aws_instance.foo: const testTerraformInputVarOnlyUnsetStr = ` aws_instance.foo: ID = foo + provider = provider.aws bar = baz foo = foovalue type = aws_instance @@ -255,11 +260,13 @@ aws_instance.foo: const testTerraformInputVarsStr = ` aws_instance.bar: ID = foo + provider = provider.aws bar = override foo = us-east-1 type = aws_instance aws_instance.foo: ID = foo + provider = provider.aws bar = baz num = 2 type = aws_instance @@ -268,10 +275,12 @@ aws_instance.foo: const testTerraformApplyStr = ` aws_instance.bar: ID = foo + provider = provider.aws foo = bar type = aws_instance aws_instance.foo: ID = foo + provider = provider.aws num = 2 type = aws_instance ` @@ -279,11 +288,13 @@ aws_instance.foo: const testTerraformApplyDataBasicStr = ` data.null_data_source.testing: ID = yo + provider = provider.null ` const testTerraformApplyRefCountStr = ` aws_instance.bar: ID = foo + provider = provider.aws foo = 3 type = aws_instance @@ -291,20 +302,24 @@ aws_instance.bar: aws_instance.foo aws_instance.foo.0: ID = foo + provider = provider.aws aws_instance.foo.1: ID = foo + provider = provider.aws aws_instance.foo.2: ID = foo + provider = provider.aws ` const testTerraformApplyProviderAliasStr = ` aws_instance.bar: ID = foo - provider = aws.bar + provider = provider.aws.bar foo = bar type = aws_instance aws_instance.foo: ID = foo + provider = provider.aws num = 2 type = aws_instance ` @@ -312,9 +327,10 @@ aws_instance.foo: const testTerraformApplyProviderAliasConfigStr = ` another_instance.bar: ID = foo - provider = another.two + provider = provider.another.two another_instance.foo: ID = foo + provider = provider.another ` const testTerraformApplyEmptyModuleStr = ` @@ -335,6 +351,7 @@ aws_secret_key = ZZZZ const testTerraformApplyDependsCreateBeforeStr = ` aws_instance.lb: ID = foo + provider = provider.aws instance = foo type = aws_instance @@ -342,6 +359,7 @@ aws_instance.lb: aws_instance.web aws_instance.web: ID = foo + provider = provider.aws require_new = ami-new type = aws_instance ` @@ -349,6 +367,7 @@ aws_instance.web: const testTerraformApplyCreateBeforeStr = ` aws_instance.bar: ID = foo + provider = provider.aws require_new = xyz type = aws_instance ` @@ -356,6 +375,7 @@ aws_instance.bar: const testTerraformApplyCreateBeforeUpdateStr = ` aws_instance.bar: ID = foo + provider = provider.aws foo = baz type = aws_instance ` @@ -363,12 +383,14 @@ aws_instance.bar: const testTerraformApplyCancelStr = ` aws_instance.foo: ID = foo + provider = provider.aws num = 2 ` const testTerraformApplyComputeStr = ` aws_instance.bar: ID = foo + provider = provider.aws foo = computed_dynamical type = aws_instance @@ -376,6 +398,7 @@ aws_instance.bar: aws_instance.foo aws_instance.foo: ID = foo + provider = provider.aws dynamical = computed_dynamical num = 2 type = aws_instance @@ -429,10 +452,12 @@ const testTerraformApplyCountTaintedStr = ` const testTerraformApplyCountVariableStr = ` aws_instance.foo.0: ID = foo + provider = provider.aws foo = foo type = aws_instance aws_instance.foo.1: ID = foo + provider = provider.aws foo = foo type = aws_instance ` @@ -440,6 +465,7 @@ aws_instance.foo.1: const testTerraformApplyCountVariableRefStr = ` aws_instance.bar: ID = foo + provider = provider.aws foo = 2 type = aws_instance @@ -447,30 +473,37 @@ aws_instance.bar: aws_instance.foo aws_instance.foo.0: ID = foo + provider = provider.aws aws_instance.foo.1: ID = foo + provider = provider.aws ` const testTerraformApplyMinimalStr = ` aws_instance.bar: ID = foo + provider = provider.aws aws_instance.foo: ID = foo + provider = provider.aws ` const testTerraformApplyModuleStr = ` aws_instance.bar: ID = foo + provider = provider.aws foo = bar type = aws_instance aws_instance.foo: ID = foo + provider = provider.aws num = 2 type = aws_instance module.child: aws_instance.baz: ID = foo + provider = provider.aws foo = bar type = aws_instance ` @@ -478,6 +511,7 @@ module.child: const testTerraformApplyModuleBoolStr = ` aws_instance.bar: ID = foo + provider = provider.aws foo = 1 type = aws_instance @@ -500,10 +534,12 @@ module.child: const testTerraformApplyMultiProviderStr = ` aws_instance.bar: ID = foo + provider = provider.aws foo = bar type = aws_instance do_instance.foo: ID = foo + provider = provider.do num = 2 type = do_instance ` @@ -513,8 +549,10 @@ const testTerraformApplyModuleOnlyProviderStr = ` module.child: aws_instance.foo: ID = foo + provider = provider.aws test_instance.foo: ID = foo + provider = provider.test ` const testTerraformApplyModuleProviderAliasStr = ` @@ -522,7 +560,7 @@ const testTerraformApplyModuleProviderAliasStr = ` module.child: aws_instance.foo: ID = foo - provider = aws.eu + provider = module.child.provider.aws.eu ` const testTerraformApplyModuleVarRefExistingStr = ` @@ -533,6 +571,7 @@ aws_instance.foo: module.child: aws_instance.foo: ID = foo + provider = provider.aws type = aws_instance value = bar ` @@ -555,11 +594,13 @@ module.child: const testTerraformApplyProvisionerStr = ` aws_instance.bar: ID = foo + provider = provider.aws Dependencies: aws_instance.foo aws_instance.foo: ID = foo + provider = provider.aws dynamical = computed_dynamical num = 2 type = aws_instance @@ -570,13 +611,16 @@ const testTerraformApplyProvisionerModuleStr = ` module.child: aws_instance.bar: ID = foo + provider = provider.aws ` const testTerraformApplyProvisionerFailStr = ` aws_instance.bar: (tainted) ID = foo + provider = provider.aws aws_instance.foo: ID = foo + provider = provider.aws num = 2 type = aws_instance ` @@ -584,6 +628,7 @@ aws_instance.foo: const testTerraformApplyProvisionerFailCreateStr = ` aws_instance.bar: (tainted) ID = foo + provider = provider.aws ` const testTerraformApplyProvisionerFailCreateNoIdStr = ` @@ -593,6 +638,7 @@ const testTerraformApplyProvisionerFailCreateNoIdStr = ` const testTerraformApplyProvisionerFailCreateBeforeDestroyStr = ` aws_instance.bar: (1 deposed) ID = bar + provider = provider.aws require_new = abc Deposed ID 1 = foo (tainted) ` @@ -600,6 +646,7 @@ aws_instance.bar: (1 deposed) const testTerraformApplyProvisionerResourceRefStr = ` aws_instance.bar: ID = foo + provider = provider.aws num = 2 type = aws_instance ` @@ -607,6 +654,7 @@ aws_instance.bar: const testTerraformApplyProvisionerSelfRefStr = ` aws_instance.foo: ID = foo + provider = provider.aws foo = bar type = aws_instance ` @@ -614,14 +662,17 @@ aws_instance.foo: const testTerraformApplyProvisionerMultiSelfRefStr = ` aws_instance.foo.0: ID = foo + provider = provider.aws foo = number 0 type = aws_instance aws_instance.foo.1: ID = foo + provider = provider.aws foo = number 1 type = aws_instance aws_instance.foo.2: ID = foo + provider = provider.aws foo = number 2 type = aws_instance ` @@ -629,10 +680,12 @@ aws_instance.foo.2: const testTerraformApplyProvisionerMultiSelfRefSingleStr = ` aws_instance.foo.0: ID = foo + provider = provider.aws foo = number 0 type = aws_instance aws_instance.foo.1: ID = foo + provider = provider.aws foo = number 1 type = aws_instance @@ -640,6 +693,7 @@ aws_instance.foo.1: aws_instance.foo.0 aws_instance.foo.2: ID = foo + provider = provider.aws foo = number 2 type = aws_instance @@ -650,6 +704,7 @@ aws_instance.foo.2: const testTerraformApplyProvisionerDiffStr = ` aws_instance.bar: ID = foo + provider = provider.aws foo = bar type = aws_instance ` @@ -666,40 +721,47 @@ module.child.subchild: const testTerraformApplyErrorStr = ` aws_instance.bar: ID = bar + provider = provider.aws Dependencies: aws_instance.foo aws_instance.foo: ID = foo + provider = provider.aws num = 2 ` const testTerraformApplyErrorCreateBeforeDestroyStr = ` aws_instance.bar: ID = bar + provider = provider.aws require_new = abc ` const testTerraformApplyErrorDestroyCreateBeforeDestroyStr = ` aws_instance.bar: (1 deposed) ID = foo + provider = provider.aws Deposed ID 1 = bar ` const testTerraformApplyErrorPartialStr = ` aws_instance.bar: ID = bar + provider = provider.aws Dependencies: aws_instance.foo aws_instance.foo: ID = foo + provider = provider.aws num = 2 ` const testTerraformApplyResourceDependsOnModuleStr = ` aws_instance.a: ID = foo + provider = provider.aws Dependencies: module.child @@ -707,11 +769,13 @@ aws_instance.a: module.child: aws_instance.child: ID = foo + provider = provider.aws ` const testTerraformApplyResourceDependsOnModuleDeepStr = ` aws_instance.a: ID = foo + provider = provider.aws Dependencies: module.child @@ -719,6 +783,7 @@ aws_instance.a: module.child.grandchild: aws_instance.c: ID = foo + provider = provider.aws ` const testTerraformApplyResourceDependsOnModuleInModuleStr = ` @@ -726,17 +791,20 @@ const testTerraformApplyResourceDependsOnModuleInModuleStr = ` module.child: aws_instance.b: ID = foo + provider = provider.aws Dependencies: module.grandchild module.child.grandchild: aws_instance.c: ID = foo + provider = provider.aws ` const testTerraformApplyTaintStr = ` aws_instance.bar: ID = foo + provider = provider.aws num = 2 type = aws_instance ` @@ -744,6 +812,7 @@ aws_instance.bar: const testTerraformApplyTaintDepStr = ` aws_instance.bar: ID = bar + provider = provider.aws foo = foo num = 2 type = aws_instance @@ -752,6 +821,7 @@ aws_instance.bar: aws_instance.foo aws_instance.foo: ID = foo + provider = provider.aws num = 2 type = aws_instance ` @@ -759,6 +829,7 @@ aws_instance.foo: const testTerraformApplyTaintDepRequireNewStr = ` aws_instance.bar: ID = foo + provider = provider.aws foo = foo require_new = yes type = aws_instance @@ -767,6 +838,7 @@ aws_instance.bar: aws_instance.foo aws_instance.foo: ID = foo + provider = provider.aws num = 2 type = aws_instance ` @@ -774,10 +846,12 @@ aws_instance.foo: const testTerraformApplyOutputStr = ` aws_instance.bar: ID = foo + provider = provider.aws foo = bar type = aws_instance aws_instance.foo: ID = foo + provider = provider.aws num = 2 type = aws_instance @@ -789,10 +863,12 @@ foo_num = 2 const testTerraformApplyOutputAddStr = ` aws_instance.test.0: ID = foo + provider = provider.aws foo = foo0 type = aws_instance aws_instance.test.1: ID = foo + provider = provider.aws foo = foo1 type = aws_instance @@ -805,18 +881,22 @@ secondOutput = foo1 const testTerraformApplyOutputListStr = ` aws_instance.bar.0: ID = foo + provider = provider.aws foo = bar type = aws_instance aws_instance.bar.1: ID = foo + provider = provider.aws foo = bar type = aws_instance aws_instance.bar.2: ID = foo + provider = provider.aws foo = bar type = aws_instance aws_instance.foo: ID = foo + provider = provider.aws num = 2 type = aws_instance @@ -828,18 +908,22 @@ foo_num = [bar,bar,bar] const testTerraformApplyOutputMultiStr = ` aws_instance.bar.0: ID = foo + provider = provider.aws foo = bar type = aws_instance aws_instance.bar.1: ID = foo + provider = provider.aws foo = bar type = aws_instance aws_instance.bar.2: ID = foo + provider = provider.aws foo = bar type = aws_instance aws_instance.foo: ID = foo + provider = provider.aws num = 2 type = aws_instance @@ -851,18 +935,22 @@ foo_num = bar,bar,bar const testTerraformApplyOutputMultiIndexStr = ` aws_instance.bar.0: ID = foo + provider = provider.aws foo = bar type = aws_instance aws_instance.bar.1: ID = foo + provider = provider.aws foo = bar type = aws_instance aws_instance.bar.2: ID = foo + provider = provider.aws foo = bar type = aws_instance aws_instance.foo: ID = foo + provider = provider.aws num = 2 type = aws_instance @@ -874,6 +962,7 @@ foo_num = bar const testTerraformApplyUnknownAttrStr = ` aws_instance.foo: ID = foo + provider = provider.aws num = 2 type = aws_instance ` @@ -881,12 +970,14 @@ aws_instance.foo: const testTerraformApplyVarsStr = ` aws_instance.bar: ID = foo + provider = provider.aws bar = foo baz = override foo = us-west-2 type = aws_instance aws_instance.foo: ID = foo + provider = provider.aws bar = baz list = Hello,World map = Baz,Foo,Hello @@ -897,6 +988,7 @@ aws_instance.foo: const testTerraformApplyVarsEnvStr = ` aws_instance.bar: ID = foo + provider = provider.aws bar = Hello,World baz = Baz,Foo,Hello foo = baz @@ -1650,6 +1742,7 @@ STATE: const testTerraformInputHCL = ` hcl_instance.hcltest: ID = foo + provider = provider.hcl bar.w = z bar.x = y foo.# = 2 From c2f3522f7d6fc2e3c0303bb947d266e8e4bf1cf8 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 7 Nov 2017 20:42:00 -0500 Subject: [PATCH 09/16] write providers to state for data resources And update the test state strings Destroying with no config is no longer allowed, run an exlpicit destroy for the destroyOrder test. --- terraform/context_apply_test.go | 9 +++++---- terraform/node_data_refresh.go | 4 ++-- terraform/terraform_test.go | 2 ++ 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 52ed20daa..7ad55e4d4 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -5957,12 +5957,13 @@ func TestContext2Apply_destroyOrder(t *testing.T) { t.Logf("State 1: %s", state) - // Next, plan and apply config-less to force a destroy with "apply" + // Next, plan and apply a destroy h.Active = true ctx = testContext2(t, &ContextOpts{ - State: state, - Module: module.NewEmptyTree(), - Hooks: []Hook{h}, + Destroy: true, + State: state, + Module: m, + Hooks: []Hook{h}, ProviderResolver: ResourceProviderResolverFixed( map[string]ResourceProviderFactory{ "aws": testProviderFuncFixed(p), diff --git a/terraform/node_data_refresh.go b/terraform/node_data_refresh.go index 7ab76c235..d5ca641a6 100644 --- a/terraform/node_data_refresh.go +++ b/terraform/node_data_refresh.go @@ -148,7 +148,7 @@ func (n *NodeRefreshableDataResourceInstance) EvalTree() EvalNode { &EvalWriteState{ Name: stateId, ResourceType: rs.Type, - Provider: rs.Provider, + Provider: n.ResolvedProvider, Dependencies: rs.Dependencies, State: &state, // state is nil here }, @@ -210,7 +210,7 @@ func (n *NodeRefreshableDataResourceInstance) EvalTree() EvalNode { &EvalWriteState{ Name: stateId, ResourceType: rs.Type, - Provider: rs.Provider, + Provider: n.ResolvedProvider, Dependencies: rs.Dependencies, State: &state, }, diff --git a/terraform/terraform_test.go b/terraform/terraform_test.go index 4d645a9f7..0a84e34e6 100644 --- a/terraform/terraform_test.go +++ b/terraform/terraform_test.go @@ -1754,6 +1754,7 @@ hcl_instance.hcltest: const testTerraformRefreshDataRefDataStr = ` data.null_data_source.bar: ID = foo + provider = provider.null bar = yes type = null_data_source @@ -1761,6 +1762,7 @@ data.null_data_source.bar: data.null_data_source.foo data.null_data_source.foo: ID = foo + provider = provider.null foo = yes type = null_data_source ` From 3977fe8b2d54fb64fed90d7f6196f61b2b77fa35 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 7 Nov 2017 20:53:18 -0500 Subject: [PATCH 10/16] write provider to state for refresh nodes and update the test state strings --- terraform/context_plan_test.go | 6 ++++++ terraform/context_test.go | 4 ++++ terraform/node_resource_destroy.go | 2 +- terraform/node_resource_refresh.go | 2 +- 4 files changed, 12 insertions(+), 2 deletions(-) diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index 3eb213028..8240e435f 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -3527,28 +3527,34 @@ STATE: aws_instance.bar.0: ID = bar0 + provider = provider.aws Dependencies: aws_instance.foo.* aws_instance.bar.1: ID = bar1 + provider = provider.aws Dependencies: aws_instance.foo.* aws_instance.baz.0: ID = baz0 + provider = provider.aws Dependencies: aws_instance.bar.* aws_instance.baz.1: ID = baz1 + provider = provider.aws Dependencies: aws_instance.bar.* aws_instance.foo.0: ID = foo0 + provider = provider.aws aws_instance.foo.1: ID = foo1 + provider = provider.aws `) if actual != expected { t.Fatalf("bad:\n%s\n\nexpected\n\n%s", actual, expected) diff --git a/terraform/context_test.go b/terraform/context_test.go index f528f2602..7ef9245d7 100644 --- a/terraform/context_test.go +++ b/terraform/context_test.go @@ -416,15 +416,18 @@ root const testContextRefreshModuleStr = ` aws_instance.web: (tainted) ID = bar + provider = provider.aws module.child: aws_instance.web: ID = new + provider = provider.aws ` const testContextRefreshOutputStr = ` aws_instance.web: ID = foo + provider = provider.aws foo = bar Outputs: @@ -439,4 +442,5 @@ const testContextRefreshOutputPartialStr = ` const testContextRefreshTaintedStr = ` aws_instance.web: (tainted) ID = foo + provider = provider.aws ` diff --git a/terraform/node_resource_destroy.go b/terraform/node_resource_destroy.go index 74a1e5494..657bbee7f 100644 --- a/terraform/node_resource_destroy.go +++ b/terraform/node_resource_destroy.go @@ -275,7 +275,7 @@ func (n *NodeDestroyResource) EvalTree() EvalNode { &EvalWriteState{ Name: stateId, ResourceType: n.Addr.Type, - Provider: rs.Provider, + Provider: n.ResolvedProvider, Dependencies: rs.Dependencies, State: &state, }, diff --git a/terraform/node_resource_refresh.go b/terraform/node_resource_refresh.go index c18c95f96..dbb64edae 100644 --- a/terraform/node_resource_refresh.go +++ b/terraform/node_resource_refresh.go @@ -166,7 +166,7 @@ func (n *NodeRefreshableManagedResourceInstance) evalTreeManagedResource() EvalN &EvalWriteState{ Name: stateId, ResourceType: n.ResourceState.Type, - Provider: n.ResourceState.Provider, + Provider: n.ResolvedProvider, Dependencies: n.ResourceState.Dependencies, State: &state, }, From d613959cda3efdbcf147a6392213bc96e4363ac4 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 7 Nov 2017 20:58:04 -0500 Subject: [PATCH 11/16] write provider to state for import nodes and update the test state strings --- terraform/context_import_test.go | 26 +++++++++++++------------- terraform/transform_import_state.go | 2 +- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/terraform/context_import_test.go b/terraform/context_import_test.go index 7a57f5a31..2a258392b 100644 --- a/terraform/context_import_test.go +++ b/terraform/context_import_test.go @@ -801,13 +801,13 @@ func TestContextImport_customProvider(t *testing.T) { const testImportStr = ` aws_instance.foo: ID = foo - provider = aws + provider = provider.aws ` const testImportCountIndexStr = ` aws_instance.foo.0: ID = foo - provider = aws + provider = provider.aws ` const testImportCollisionStr = ` @@ -820,7 +820,7 @@ const testImportModuleStr = ` module.foo: aws_instance.foo: ID = foo - provider = aws + provider = provider.aws ` const testImportModuleDepth2Str = ` @@ -828,7 +828,7 @@ const testImportModuleDepth2Str = ` module.a.b: aws_instance.foo: ID = foo - provider = aws + provider = provider.aws ` const testImportModuleDiffStr = ` @@ -838,7 +838,7 @@ module.bar: module.foo: aws_instance.foo: ID = foo - provider = aws + provider = provider.aws ` const testImportModuleExistingStr = ` @@ -847,39 +847,39 @@ module.foo: ID = bar aws_instance.foo: ID = foo - provider = aws + provider = provider.aws ` const testImportMultiStr = ` aws_instance.foo: ID = foo - provider = aws + provider = provider.aws aws_instance_thing.foo: ID = bar - provider = aws + provider = provider.aws ` const testImportMultiSameStr = ` aws_instance.foo: ID = foo - provider = aws + provider = provider.aws aws_instance_thing.foo: ID = bar - provider = aws + provider = provider.aws aws_instance_thing.foo-1: ID = qux - provider = aws + provider = provider.aws ` const testImportRefreshStr = ` aws_instance.foo: ID = foo - provider = aws + provider = provider.aws foo = bar ` const testImportCustomProviderStr = ` aws_instance.foo: ID = foo - provider = aws.alias + provider = provider.aws.alias ` diff --git a/terraform/transform_import_state.go b/terraform/transform_import_state.go index abac3fd24..fcbff653f 100644 --- a/terraform/transform_import_state.go +++ b/terraform/transform_import_state.go @@ -240,7 +240,7 @@ func (n *graphNodeImportStateSub) EvalTree() EvalNode { &EvalWriteState{ Name: key.String(), ResourceType: info.Type, - Provider: resourceProvider(info.Type, n.ProviderName), + Provider: n.ResolvedProvider, State: &state, }, }, From 3c807e5427d9b3e983dae2c3b1f47a7985f83246 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 7 Nov 2017 21:18:01 -0500 Subject: [PATCH 12/16] update state test strings in command package --- command/import_test.go | 4 ++-- command/refresh_test.go | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/command/import_test.go b/command/import_test.go index 9f0f2d992..a75997fab 100644 --- a/command/import_test.go +++ b/command/import_test.go @@ -663,11 +663,11 @@ func TestImport_pluginDir(t *testing.T) { const testImportStr = ` test_instance.foo: ID = yay - provider = test + provider = provider.test ` const testImportCustomProviderStr = ` test_instance.foo: ID = yay - provider = test.alias + provider = provider.test.alias ` diff --git a/command/refresh_test.go b/command/refresh_test.go index 6106e53c5..bb04f39f5 100644 --- a/command/refresh_test.go +++ b/command/refresh_test.go @@ -802,8 +802,10 @@ foo = "bar" const testRefreshStr = ` test_instance.foo: ID = yes + provider = provider.test ` const testRefreshCwdStr = ` test_instance.foo: ID = yes + provider = provider.test ` From d62e9217aef15baec99ef7a99e24783959d0d51a Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 7 Nov 2017 21:23:08 -0500 Subject: [PATCH 13/16] update test state strings for backend/local --- backend/local/backend_apply_test.go | 3 +++ backend/local/backend_plan_test.go | 2 +- backend/local/backend_refresh_test.go | 5 +++++ backend/local/backend_test.go | 1 + 4 files changed, 10 insertions(+), 1 deletion(-) diff --git a/backend/local/backend_apply_test.go b/backend/local/backend_apply_test.go index 303f92969..3e692ea24 100644 --- a/backend/local/backend_apply_test.go +++ b/backend/local/backend_apply_test.go @@ -53,6 +53,7 @@ func TestLocal_applyBasic(t *testing.T) { checkState(t, b.StateOutPath, ` test_instance.foo: ID = yes + provider = provider.test `) } @@ -160,6 +161,7 @@ func TestLocal_applyError(t *testing.T) { checkState(t, b.StateOutPath, ` test_instance.foo: ID = foo + provider = provider.test `) } @@ -211,6 +213,7 @@ func TestLocal_applyBackendFail(t *testing.T) { checkState(t, "errored.tfstate", ` test_instance.foo: ID = yes + provider = provider.test `) } diff --git a/backend/local/backend_plan_test.go b/backend/local/backend_plan_test.go index b8698e8a4..244bdc130 100644 --- a/backend/local/backend_plan_test.go +++ b/backend/local/backend_plan_test.go @@ -220,7 +220,7 @@ func TestLocal_planDestroyNoConfig(t *testing.T) { } <-run.Done() if run.Err != nil { - t.Fatalf("err: %s", err) + t.Fatalf("err: %s", run.Err) } if !p.RefreshCalled { diff --git a/backend/local/backend_refresh_test.go b/backend/local/backend_refresh_test.go index c6cc0bc0a..1d7a6f1cc 100644 --- a/backend/local/backend_refresh_test.go +++ b/backend/local/backend_refresh_test.go @@ -37,6 +37,7 @@ func TestLocal_refresh(t *testing.T) { checkState(t, b.StateOutPath, ` test_instance.foo: ID = yes + provider = provider.test `) } @@ -64,6 +65,7 @@ func TestLocal_refreshNilModule(t *testing.T) { checkState(t, b.StateOutPath, ` test_instance.foo: ID = yes + provider = provider.test `) } @@ -94,6 +96,7 @@ func TestLocal_refreshNilModuleWithInput(t *testing.T) { checkState(t, b.StateOutPath, ` test_instance.foo: ID = yes + provider = provider.test `) } @@ -137,6 +140,7 @@ func TestLocal_refreshInput(t *testing.T) { checkState(t, b.StateOutPath, ` test_instance.foo: ID = yes + provider = provider.test `) } @@ -170,6 +174,7 @@ func TestLocal_refreshValidate(t *testing.T) { checkState(t, b.StateOutPath, ` test_instance.foo: ID = yes + provider = provider.test `) } diff --git a/backend/local/backend_test.go b/backend/local/backend_test.go index c22b92cd6..df71469ae 100644 --- a/backend/local/backend_test.go +++ b/backend/local/backend_test.go @@ -27,6 +27,7 @@ func TestLocal_backend(t *testing.T) { } func checkState(t *testing.T, path, expected string) { + t.Helper() // Read the state f, err := os.Open(path) if err != nil { From 09180a10ff8fc7df10bd351f5f4edc0ebcd0a9a9 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 7 Nov 2017 21:23:37 -0500 Subject: [PATCH 14/16] cannot destroy without a config --- backend/local/backend_plan_test.go | 42 ------------------------------ 1 file changed, 42 deletions(-) diff --git a/backend/local/backend_plan_test.go b/backend/local/backend_plan_test.go index 244bdc130..de71555a7 100644 --- a/backend/local/backend_plan_test.go +++ b/backend/local/backend_plan_test.go @@ -199,48 +199,6 @@ func TestLocal_planDestroy(t *testing.T) { } } -func TestLocal_planDestroyNoConfig(t *testing.T) { - b := TestLocal(t) - p := TestLocalProvider(t, b, "test") - terraform.TestStateFile(t, b.StatePath, testPlanState()) - - outDir := testTempDir(t) - defer os.RemoveAll(outDir) - planPath := filepath.Join(outDir, "plan.tfplan") - - op := testOperationPlan() - op.Destroy = true - op.PlanRefresh = true - op.Module = nil - op.PlanOutPath = planPath - - run, err := b.Operation(context.Background(), op) - if err != nil { - t.Fatalf("bad: %s", err) - } - <-run.Done() - if run.Err != nil { - t.Fatalf("err: %s", run.Err) - } - - if !p.RefreshCalled { - t.Fatal("refresh should be called") - } - - if run.PlanEmpty { - t.Fatal("plan should not be empty") - } - - plan := testReadPlan(t, planPath) - for _, m := range plan.Diff.Modules { - for _, r := range m.Resources { - if !r.Destroy { - t.Fatalf("bad: %#v", r) - } - } - } -} - func TestLocal_planOutPathNoChange(t *testing.T) { b := TestLocal(t) TestLocalProvider(t, b, "test") From 3801ca5bbfc0cbcb9ee698a37d10be78fde5e82e Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 7 Nov 2017 21:41:49 -0500 Subject: [PATCH 15/16] test that Refresh updates Provider fields in state --- terraform/context_apply_test.go | 1 - terraform/context_refresh_test.go | 50 +++++++++++++++++++ .../update-resource-provider/main.tf | 7 +++ 3 files changed, 57 insertions(+), 1 deletion(-) create mode 100644 terraform/test-fixtures/update-resource-provider/main.tf diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 7ad55e4d4..300c9d21d 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -8908,7 +8908,6 @@ func TestContext2Apply_destroyWithLocals(t *testing.T) { p.ApplyFn = testApplyFn p.DiffFn = func(info *InstanceInfo, s *InstanceState, c *ResourceConfig) (*InstanceDiff, error) { d, err := testDiffFn(info, s, c) - fmt.Println("DIFF:", d) return d, err } diff --git a/terraform/context_refresh_test.go b/terraform/context_refresh_test.go index 0065a458c..53e5534cd 100644 --- a/terraform/context_refresh_test.go +++ b/terraform/context_refresh_test.go @@ -1110,3 +1110,53 @@ func TestContext2Refresh_noDiffHookOnScaleOut(t *testing.T) { t.Fatal("PostDiff should not have been called") } } + +func TestContext2Refresh_updateProviderInState(t *testing.T) { + m := testModule(t, "update-resource-provider") + p := testProvider("aws") + + p.DiffFn = testDiffFn + p.ApplyFn = testApplyFn + + s := &State{ + Modules: []*ModuleState{ + &ModuleState{ + Path: rootModulePath, + Resources: map[string]*ResourceState{ + "aws_instance.bar": &ResourceState{ + Type: "aws_instance", + Primary: &InstanceState{ + ID: "foo", + }, + Provider: "provider.aws.baz", + }, + }, + }, + }, + } + + ctx := testContext2(t, &ContextOpts{ + Module: m, + ProviderResolver: ResourceProviderResolverFixed( + map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + ), + State: s, + }) + + expected := strings.TrimSpace(` +aws_instance.bar: + ID = foo + provider = provider.aws.foo`) + + state, err := ctx.Refresh() + if err != nil { + t.Fatal(err) + } + + actual := state.String() + if actual != expected { + t.Fatalf("expected:\n%s\n\ngot:\n%s", expected, actual) + } +} diff --git a/terraform/test-fixtures/update-resource-provider/main.tf b/terraform/test-fixtures/update-resource-provider/main.tf new file mode 100644 index 000000000..6c082d540 --- /dev/null +++ b/terraform/test-fixtures/update-resource-provider/main.tf @@ -0,0 +1,7 @@ +provider "aws" { + alias = "foo" +} + +resource "aws_instance" "bar" { + provider = "aws.foo" +} From b8b75486142818dfb90cae7f13d06f5f05f5bf7d Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 7 Nov 2017 22:00:58 -0500 Subject: [PATCH 16/16] remove module referencing existing provider --- terraform/context_apply_test.go | 75 ++++++++++++++++++- .../destroy-module-with-provider/main.tf | 11 +++ .../destroy-module-with-provider/mod/main.tf | 6 ++ 3 files changed, 89 insertions(+), 3 deletions(-) create mode 100644 terraform/test-fixtures/destroy-module-with-provider/main.tf create mode 100644 terraform/test-fixtures/destroy-module-with-provider/mod/main.tf diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 300c9d21d..094029044 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -9016,10 +9016,79 @@ func TestContext2Apply_providerWithLocals(t *testing.T) { t.Fatal("expected no state, got:", state) } - // Destroy won't work because the local value is removed before the - // provider. Once this is fixed this test will start to fail, and we - // can remove the invalid interpolation string; if providerRegion != "bar" { t.Fatalf("expected region %q, got: %q", "bar", providerRegion) } } + +func TestContext2Apply_destroyWithProviders(t *testing.T) { + m := testModule(t, "destroy-module-with-provider") + p := testProvider("aws") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn + + s := &State{ + Modules: []*ModuleState{ + &ModuleState{ + Path: rootModulePath, + }, + &ModuleState{ + Path: []string{"root", "child"}, + }, + &ModuleState{ + Path: []string{"root", "mod", "removed"}, + Resources: map[string]*ResourceState{ + "aws_instance.child": &ResourceState{ + Type: "aws_instance", + Primary: &InstanceState{ + ID: "bar", + }, + // this provider doesn't exist + Provider: "provider.aws.baz", + }, + }, + }, + }, + } + + ctx := testContext2(t, &ContextOpts{ + Module: m, + ProviderResolver: ResourceProviderResolverFixed( + map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + ), + State: s, + Destroy: true, + }) + + // test that we can't destroy if the provider is missing + if _, err := ctx.Plan(); err == nil { + t.Fatal("expected plan error, provider.aws.baz doesn't exist") + } + + // correct the state + s.Modules[2].Resources["aws_instance.child"].Provider = "provider.aws.bar" + + if _, err := ctx.Plan(); err != nil { + t.Fatal(err) + } + state, err := ctx.Apply() + if err != nil { + t.Fatalf("error during apply: %s", err) + } + + got := strings.TrimSpace(state.String()) + + // This should fail once modules are removed from the state entirely. + want := strings.TrimSpace(` + +module.child: + +module.mod.removed: + `) + + if got != want { + t.Fatalf("wrong final state\ngot:\n%s\nwant:\n%s", got, want) + } +} diff --git a/terraform/test-fixtures/destroy-module-with-provider/main.tf b/terraform/test-fixtures/destroy-module-with-provider/main.tf new file mode 100644 index 000000000..5c7c70331 --- /dev/null +++ b/terraform/test-fixtures/destroy-module-with-provider/main.tf @@ -0,0 +1,11 @@ +// this is the provider that should actually be used by orphaned resources +provider "aws" { + alias = "bar" +} + +module "mod" { + source = "./mod" + providers = { + "aws.foo" = "aws.bar" + } +} diff --git a/terraform/test-fixtures/destroy-module-with-provider/mod/main.tf b/terraform/test-fixtures/destroy-module-with-provider/mod/main.tf new file mode 100644 index 000000000..3e360ee46 --- /dev/null +++ b/terraform/test-fixtures/destroy-module-with-provider/mod/main.tf @@ -0,0 +1,6 @@ +provider "aws" { + alias = "foo" +} + +// removed module configuration referencing aws.foo, which was passed in by the +// root module