From 0c30caec7d2ef99597c380a8bd140943d481f9be Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 16 Dec 2016 16:47:32 -0800 Subject: [PATCH] config: smarter provider alias usage validation Fixes #4789 This improves the validation that valid provider aliases are used. Previously, we required that provider aliases be defined in every module they're used. This isn't correct because the alias may be used in a parent module and inherited. This removes that validation and creates the validation that a provider alias must be defined in the used module or _any parent_. This allows inheritance to work properly. We've always had this type of validation for aliases because we believe its a good UX tradeoff: typo-ing an alias is really painful, so we require declaration of alias usage. It may add a small burden to declare, but since relatively few aliases are used, it improves the scenario where a user fat-fingers an alias name. --- config/config.go | 9 -- config/config_test.go | 7 -- .../validate-alias-bad/child/main.tf | 3 + .../test-fixtures/validate-alias-bad/main.tf | 3 + .../validate-alias-good/child/main.tf | 3 + .../test-fixtures/validate-alias-good/main.tf | 5 + config/module/tree.go | 8 ++ config/module/tree_test.go | 44 +++++++ config/module/validate_provider_alias.go | 118 ++++++++++++++++++ 9 files changed, 184 insertions(+), 16 deletions(-) create mode 100644 config/module/test-fixtures/validate-alias-bad/child/main.tf create mode 100644 config/module/test-fixtures/validate-alias-bad/main.tf create mode 100644 config/module/test-fixtures/validate-alias-good/child/main.tf create mode 100644 config/module/test-fixtures/validate-alias-good/main.tf create mode 100644 config/module/validate_provider_alias.go diff --git a/config/config.go b/config/config.go index 34f80d4f36..e1543a1afc 100644 --- a/config/config.go +++ b/config/config.go @@ -553,15 +553,6 @@ func (c *Config) Validate() error { // Validate DependsOn errs = append(errs, c.validateDependsOn(n, r.DependsOn, resources, modules)...) - // Verify provider points to a provider that is configured - if r.Provider != "" { - if _, ok := providerSet[r.Provider]; !ok { - errs = append(errs, fmt.Errorf( - "%s: resource depends on non-configured provider '%s'", - n, r.Provider)) - } - } - // Verify provisioners don't contain any splats for _, p := range r.Provisioners { // This validation checks that there are now splat variables diff --git a/config/config_test.go b/config/config_test.go index 61dafa65da..c73ed6100e 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -456,13 +456,6 @@ func TestConfigValidate_providerMultiRefGood(t *testing.T) { } } -func TestConfigValidate_providerMultiRefBad(t *testing.T) { - c := testConfig(t, "validate-provider-multi-ref-bad") - if err := c.Validate(); err == nil { - t.Fatal("should not be valid") - } -} - func TestConfigValidate_provConnSplatOther(t *testing.T) { c := testConfig(t, "validate-prov-conn-splat-other") if err := c.Validate(); err != nil { diff --git a/config/module/test-fixtures/validate-alias-bad/child/main.tf b/config/module/test-fixtures/validate-alias-bad/child/main.tf new file mode 100644 index 0000000000..48a39a45da --- /dev/null +++ b/config/module/test-fixtures/validate-alias-bad/child/main.tf @@ -0,0 +1,3 @@ +resource "aws_instance" "foo" { + provider = "aws.foo" +} diff --git a/config/module/test-fixtures/validate-alias-bad/main.tf b/config/module/test-fixtures/validate-alias-bad/main.tf new file mode 100644 index 0000000000..0f6991c536 --- /dev/null +++ b/config/module/test-fixtures/validate-alias-bad/main.tf @@ -0,0 +1,3 @@ +module "child" { + source = "./child" +} diff --git a/config/module/test-fixtures/validate-alias-good/child/main.tf b/config/module/test-fixtures/validate-alias-good/child/main.tf new file mode 100644 index 0000000000..48a39a45da --- /dev/null +++ b/config/module/test-fixtures/validate-alias-good/child/main.tf @@ -0,0 +1,3 @@ +resource "aws_instance" "foo" { + provider = "aws.foo" +} diff --git a/config/module/test-fixtures/validate-alias-good/main.tf b/config/module/test-fixtures/validate-alias-good/main.tf new file mode 100644 index 0000000000..a4e32d4464 --- /dev/null +++ b/config/module/test-fixtures/validate-alias-good/main.tf @@ -0,0 +1,5 @@ +provider "aws" { alias = "foo" } + +module "child" { + source = "./child" +} diff --git a/config/module/tree.go b/config/module/tree.go index 8a322650be..777389bdc8 100644 --- a/config/module/tree.go +++ b/config/module/tree.go @@ -267,6 +267,14 @@ func (t *Tree) Validate() error { return newErr } + // If we're the root, we do extra validation. This validation usually + // requires the entire tree (since children don't have parent pointers). + if len(t.path) == 0 { + if err := t.validateProviderAlias(); err != nil { + return err + } + } + // Get the child trees children := t.Children() diff --git a/config/module/tree_test.go b/config/module/tree_test.go index d46d5ed27e..4df3f8cff1 100644 --- a/config/module/tree_test.go +++ b/config/module/tree_test.go @@ -1,6 +1,7 @@ package module import ( + "fmt" "os" "reflect" "strings" @@ -267,6 +268,49 @@ func TestTreeName(t *testing.T) { } } +// This is a table-driven test for tree validation. This is the preferred +// way to test Validate. Non table-driven tests exist historically but +// that style shouldn't be done anymore. +func TestTreeValidate_table(t *testing.T) { + cases := []struct { + Name string + Fixture string + Err string + }{ + { + "provider alias in child", + "validate-alias-good", + "", + }, + + { + "undefined provider alias in child", + "validate-alias-bad", + "alias must be defined", + }, + } + + for i, tc := range cases { + t.Run(fmt.Sprintf("%d-%s", i, tc.Name), func(t *testing.T) { + tree := NewTree("", testConfig(t, tc.Fixture)) + if err := tree.Load(testStorage(t), GetModeGet); err != nil { + t.Fatalf("err: %s", err) + } + + err := tree.Validate() + if (err != nil) != (tc.Err != "") { + t.Fatalf("err: %s", err) + } + if err == nil { + return + } + if !strings.Contains(err.Error(), tc.Err) { + t.Fatalf("err should contain %q: %s", tc.Err, err) + } + }) + } +} + func TestTreeValidate_badChild(t *testing.T) { tree := NewTree("", testConfig(t, "validate-child-bad")) diff --git a/config/module/validate_provider_alias.go b/config/module/validate_provider_alias.go new file mode 100644 index 0000000000..090d4f7e39 --- /dev/null +++ b/config/module/validate_provider_alias.go @@ -0,0 +1,118 @@ +package module + +import ( + "fmt" + "strings" + + "github.com/hashicorp/go-multierror" + "github.com/hashicorp/terraform/dag" +) + +// validateProviderAlias validates that all provider alias references are +// defined at some point in the parent tree. This improves UX by catching +// alias typos at the slight cost of requiring a declaration of usage. This +// is usually a good tradeoff since not many aliases are used. +func (t *Tree) validateProviderAlias() error { + // If we're not the root, don't perform this validation. We must be the + // root since we require full tree visibilty. + if len(t.path) != 0 { + return nil + } + + // We'll use a graph to keep track of defined aliases at each level. + // As long as a parent defines an alias, it is okay. + var g dag.AcyclicGraph + t.buildProviderAliasGraph(&g, nil) + + // Go through the graph and check that the usage is all good. + var err error + for _, v := range g.Vertices() { + pv, ok := v.(*providerAliasVertex) + if !ok { + // This shouldn't happen, just ignore it. + continue + } + + // If we're not using any aliases, fast track and just continue + if len(pv.Used) == 0 { + continue + } + + // Grab the ancestors since we're going to have to check if our + // parents define any of our aliases. + var parents []*providerAliasVertex + ancestors, _ := g.Ancestors(v) + for _, raw := range ancestors.List() { + if pv, ok := raw.(*providerAliasVertex); ok { + parents = append(parents, pv) + } + } + for k, _ := range pv.Used { + // Check if we define this + if _, ok := pv.Defined[k]; ok { + continue + } + + // Check for a parent + found := false + for _, parent := range parents { + _, found = parent.Defined[k] + if found { + break + } + } + if found { + continue + } + + // We didn't find the alias, error! + err = multierror.Append(err, fmt.Errorf( + "module %s: provider alias must be defined by the module or a parent: %s", + strings.Join(pv.Path, "."), k)) + } + } + + return err +} + +func (t *Tree) buildProviderAliasGraph(g *dag.AcyclicGraph, parent dag.Vertex) { + // Add all our defined aliases + defined := make(map[string]struct{}) + for _, p := range t.config.ProviderConfigs { + defined[p.FullName()] = struct{}{} + } + + // Add all our used aliases + used := make(map[string]struct{}) + for _, r := range t.config.Resources { + if r.Provider != "" { + used[r.Provider] = struct{}{} + } + } + + // Add it to the graph + vertex := &providerAliasVertex{ + Path: t.Path(), + Defined: defined, + Used: used, + } + g.Add(vertex) + + // Connect to our parent if we have one + if parent != nil { + g.Connect(dag.BasicEdge(vertex, parent)) + } + + // Build all our children + for _, c := range t.Children() { + c.buildProviderAliasGraph(g, vertex) + } +} + +// providerAliasVertex is the vertex for the graph that keeps track of +// defined provider aliases. +type providerAliasVertex struct { + Path []string + Defined map[string]struct{} + Used map[string]struct{} +}