From 8bf270daa985792acd56657195b25af3eb146359 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 9 Nov 2017 23:22:40 -0500 Subject: [PATCH 1/3] rewrite the ProviderConfigTransformer It's become apparent that passing in a provider config for an implicitly used provider would be very useful. While the ProviderConfigTransformer efficiently added providers to the graph, the algorithm was reversed from what would be needed to allow overriding implicit providers. Change the ProviderConfigTransformer to fist add all configured provider, even if they are empty stubs. Then run through all providers being passed in from the parent, and replace the provider nodes we created with proxies, and add implicit proxies where none existed. The extra nodes will then be pruned later. --- terraform/transform_provider.go | 90 +++++++++++++++++++-------------- 1 file changed, 51 insertions(+), 39 deletions(-) diff --git a/terraform/transform_provider.go b/terraform/transform_provider.go index 31aeb3ca56..da2ae3f318 100644 --- a/terraform/transform_provider.go +++ b/terraform/transform_provider.go @@ -405,6 +405,8 @@ type ProviderConfigTransformer struct { // each provider node is stored here so that the proxy nodes can look up // their targets by name. providers map[string]GraphNodeProvider + // record providers that can be overriden with a proxy + proxiable map[string]bool // Module is the module to add resources from. Module *module.Tree @@ -422,6 +424,7 @@ func (t *ProviderConfigTransformer) Transform(g *Graph) error { } t.providers = make(map[string]GraphNodeProvider) + t.proxiable = make(map[string]bool) // Start the transformation process if err := t.transform(g, t.Module); err != nil { @@ -464,19 +467,13 @@ func (t *ProviderConfigTransformer) transformSingle(g *Graph, m *module.Tree) er path = append([]string{RootModuleName}, path...) } - // add all provider configs + // add all providers from the configuration 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, @@ -484,26 +481,29 @@ func (t *ProviderConfigTransformer) transformSingle(g *Graph, m *module.Tree) er // Add it to the graph g.Add(v) - t.providers[ResolveProviderName(name, path)] = v.(GraphNodeProvider) + fullName := ResolveProviderName(name, path) + t.providers[fullName] = v.(GraphNodeProvider) + t.proxiable[fullName] = len(p.RawConfig.RawMap()) == 0 } - return nil + // Now replace the provider nodes with proxy nodes if a provider was being + // passed in, and create implicit proxies if there was no config. Any extra + // proxies will be removed in the prune step. + return t.addProxyProviders(g, m) } -// 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 { +func (t *ProviderConfigTransformer) addProxyProviders(g *Graph, m *module.Tree) error { 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 + // can't add proxies at the root + if len(path) == 0 { + return nil } parentPath := path[:len(path)-1] parent := t.Module.Child(parentPath) if parent == nil { - return false + return nil } var parentCfg *config.Module @@ -515,35 +515,47 @@ func (t *ProviderConfigTransformer) addProxyProvider(g *Graph, m *module.Tree, p } if parentCfg == nil { - panic("immaculately conceived module " + m.Name()) + // this can't really happen during normal execution. + return fmt.Errorf("parent module config not found for %s", 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 - } + // Go through all the providers the parent is passing in, and add proxies to + // the parent provider nodes. + for name, parentName := range parentCfg.Providers { + fullName := ResolveProviderName(name, path) + fullParentName := ResolveProviderName(parentName, parentPath) + parentProvider := t.providers[fullParentName] - // the parent module is passing in a provider - fullParentName := ResolveProviderName(parentProviderName, parentPath) - parentProvider := t.providers[fullParentName] + if parentProvider == nil { + return fmt.Errorf("missing provider %s", fullParentName) + } - if parentProvider == nil { - log.Printf("[ERROR] missing provider %s in module %s", parentProviderName, m.Name()) - return false - } + proxy := &graphNodeProxyProvider{ + nameValue: name, + path: path, + target: parentProvider, + } - v := &graphNodeProxyProvider{ - nameValue: name, - path: path, - target: parentProvider, - } + concreteProvider := t.providers[fullName] - // Add it to the graph - g.Add(v) - t.providers[ResolveProviderName(name, path)] = v - return true + // replace the concrete node with the provider passed in + if concreteProvider != nil && t.proxiable[fullName] { + g.Replace(concreteProvider, proxy) + t.providers[fullName] = proxy + continue + } + + // aliased providers can't be implicitly passed in + if strings.Contains(name, ".") { + continue + } + + // There was no concrete provider, so add this as an implicit provider. + // The extra proxy will be pruned later if it's unused. + g.Add(proxy) + t.providers[fullName] = proxy + } + return nil } func (t *ProviderConfigTransformer) attachProviderConfigs(g *Graph) error { From b9b418bcb02959e673def7de51d093c2b83c80d2 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 10 Nov 2017 09:39:05 -0500 Subject: [PATCH 2/3] test passing in implicitly used provider --- .../main.tf | 10 ++++++ .../mod/main.tf | 2 ++ terraform/transform_provider_test.go | 34 +++++++++++++++++++ 3 files changed, 46 insertions(+) create mode 100644 terraform/test-fixtures/transform-provider-implicit-module/main.tf create mode 100644 terraform/test-fixtures/transform-provider-implicit-module/mod/main.tf diff --git a/terraform/test-fixtures/transform-provider-implicit-module/main.tf b/terraform/test-fixtures/transform-provider-implicit-module/main.tf new file mode 100644 index 0000000000..97d73f24dc --- /dev/null +++ b/terraform/test-fixtures/transform-provider-implicit-module/main.tf @@ -0,0 +1,10 @@ +provider "aws" { + alias = "foo" +} + +module "mod" { + source = "./mod" + providers = { + "aws" = "aws.foo" + } +} diff --git a/terraform/test-fixtures/transform-provider-implicit-module/mod/main.tf b/terraform/test-fixtures/transform-provider-implicit-module/mod/main.tf new file mode 100644 index 0000000000..01cf0803c0 --- /dev/null +++ b/terraform/test-fixtures/transform-provider-implicit-module/mod/main.tf @@ -0,0 +1,2 @@ +resource "aws_instance" "bar" { +} diff --git a/terraform/transform_provider_test.go b/terraform/transform_provider_test.go index f4277060a6..c6698b382f 100644 --- a/terraform/transform_provider_test.go +++ b/terraform/transform_provider_test.go @@ -511,6 +511,40 @@ func TestProviderConfigTransformer_grandparentProviders(t *testing.T) { } } +// pass a specific provider into a module using it implicitly +func TestProviderConfigTransformer_implicitModule(t *testing.T) { + mod := testModule(t, "transform-provider-implicit-module") + 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(`module.mod.aws_instance.bar + provider.aws.foo +provider.aws.foo`) + if actual != expected { + t.Fatalf("expected:\n%s\n\ngot:\n%s", expected, actual) + } +} + const testTransformProviderBasicStr = ` aws_instance.web provider.aws From 105b66e74d5b07d867d84a33ca500f2a2fda01c4 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 10 Nov 2017 09:52:00 -0500 Subject: [PATCH 3/3] error out when a referenced provider is missing --- .../transform-provider-invalid/main.tf | 11 +++++++ .../transform-provider-invalid/mod/main.tf | 2 ++ terraform/transform_provider.go | 3 +- terraform/transform_provider_test.go | 29 +++++++++++++++++++ 4 files changed, 44 insertions(+), 1 deletion(-) create mode 100644 terraform/test-fixtures/transform-provider-invalid/main.tf create mode 100644 terraform/test-fixtures/transform-provider-invalid/mod/main.tf diff --git a/terraform/test-fixtures/transform-provider-invalid/main.tf b/terraform/test-fixtures/transform-provider-invalid/main.tf new file mode 100644 index 0000000000..17e6304e02 --- /dev/null +++ b/terraform/test-fixtures/transform-provider-invalid/main.tf @@ -0,0 +1,11 @@ +provider "aws" { +} + +module "mod" { + source = "./mod" + + # aws.foo doesn't exist, and should report an error + providers = { + "aws" = "aws.foo" + } +} diff --git a/terraform/test-fixtures/transform-provider-invalid/mod/main.tf b/terraform/test-fixtures/transform-provider-invalid/mod/main.tf new file mode 100644 index 0000000000..03641197f0 --- /dev/null +++ b/terraform/test-fixtures/transform-provider-invalid/mod/main.tf @@ -0,0 +1,2 @@ +resource "aws_resource" "foo" { +} diff --git a/terraform/transform_provider.go b/terraform/transform_provider.go index da2ae3f318..19df21897e 100644 --- a/terraform/transform_provider.go +++ b/terraform/transform_provider.go @@ -428,7 +428,7 @@ func (t *ProviderConfigTransformer) Transform(g *Graph) error { // Start the transformation process if err := t.transform(g, t.Module); err != nil { - return nil + return err } // finally attach the configs to the new nodes @@ -524,6 +524,7 @@ func (t *ProviderConfigTransformer) addProxyProviders(g *Graph, m *module.Tree) for name, parentName := range parentCfg.Providers { fullName := ResolveProviderName(name, path) fullParentName := ResolveProviderName(parentName, parentPath) + parentProvider := t.providers[fullParentName] if parentProvider == nil { diff --git a/terraform/transform_provider_test.go b/terraform/transform_provider_test.go index c6698b382f..a418b5b678 100644 --- a/terraform/transform_provider_test.go +++ b/terraform/transform_provider_test.go @@ -545,6 +545,35 @@ provider.aws.foo`) } } +// error out when a non-existent provider is named in a module providers map +func TestProviderConfigTransformer_invalidProvider(t *testing.T) { + mod := testModule(t, "transform-provider-invalid") + 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) + err := tf.Transform(&g) + if err == nil { + t.Fatal("expected missing provider error") + } + if !strings.Contains(err.Error(), "provider.aws.foo") { + t.Fatalf("error should reference missing provider, got: %s", err) + } +} + const testTransformProviderBasicStr = ` aws_instance.web provider.aws