Merge pull request hashicorp#10807 from hashicorp/b-alias-validate

config: smarter provider alias usage validation

Author: mitchellh

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