diff --git a/config/config.go b/config/config.go index 34f80d4f3637..e1543a1afc3f 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 61dafa65da5f..c73ed6100e7a 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 000000000000..48a39a45dae2 --- /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 000000000000..0f6991c536ca --- /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 000000000000..48a39a45dae2 --- /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 000000000000..a4e32d4464ff --- /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 8a322650be47..777389bdc868 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 d46d5ed27e15..4df3f8cff1cc 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 000000000000..090d4f7e3984 --- /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{} +}