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

@@ -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

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 {

config/module/test-fixtures/validate-alias-bad/child/main.tf

@@ -0,0 +1,3 @@
+resource "aws_instance" "foo" {
+    provider = "aws.foo"
+}

config/module/test-fixtures/validate-alias-bad/main.tf

@@ -0,0 +1,3 @@
+module "child" {
+    source = "./child"
+}

config/module/test-fixtures/validate-alias-good/child/main.tf

@@ -0,0 +1,3 @@
+resource "aws_instance" "foo" {
+    provider = "aws.foo"
+}

config/module/test-fixtures/validate-alias-good/main.tf

@@ -0,0 +1,5 @@
+provider "aws" { alias = "foo" }
+
+module "child" {
+    source = "./child"
+}

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()
 

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"))
 

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{}
+}