add provider validation

Add validation which was removed from the configload package, along with
additional validation checks. The output is slightly different, as
instead of validating whether the modules are allowed to have provider
configurations, we validate the various combinations of provider
structures themselves.

configs/config_build.go

@@ -22,6 +22,9 @@ func BuildConfig(root *Module, walker ModuleWalker) (*Config, hcl.Diagnostics) {
 	}
 	cfg.Root = cfg // Root module is self-referential.
 	cfg.Children, diags = buildChildModules(cfg, walker)
+
+	diags = append(diags, validateProviderConfigs(nil, cfg, false)...)
+
 	return cfg, diags
 }
 

configs/config_build_test.go

@@ -2,6 +2,7 @@ package configs
 
 import (
 	"fmt"
+	"io/ioutil"
 	"path/filepath"
 	"reflect"
 	"sort"
@@ -154,3 +155,127 @@ func TestBuildConfigChildModuleBackend(t *testing.T) {
 		t.Fatalf("wrong result\ngot: %swant: %s", spew.Sdump(got), spew.Sdump(want))
 	}
 }
+
+func TestBuildConfigInvalidModules(t *testing.T) {
+	testDir := "testdata/config-diagnostics"
+	dirs, err := ioutil.ReadDir(testDir)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	for _, info := range dirs {
+		name := info.Name()
+		t.Run(name, func(t *testing.T) {
+			parser := NewParser(nil)
+			path := filepath.Join(testDir, name)
+
+			mod, diags := parser.LoadConfigDir(path)
+			if diags.HasErrors() {
+				// these tests should only trigger errors that are caught in
+				// the config loader.
+				t.Errorf("error loading config dir")
+				for _, diag := range diags {
+					t.Logf("- %s", diag)
+				}
+			}
+
+			readDiags := func(data []byte, _ error) []string {
+				var expected []string
+				for _, s := range strings.Split(string(data), "\n") {
+					msg := strings.TrimSpace(s)
+					msg = strings.ReplaceAll(msg, `\n`, "\n")
+					if msg != "" {
+						expected = append(expected, msg)
+					}
+				}
+				return expected
+			}
+
+			// Load expected errors and warnings.
+			// Each line in the file is matched as a substring against the
+			// diagnostic outputs.
+			// Capturing part of the path and source range in the message lets
+			// us also ensure the diagnostic is being attributed to the
+			// expected location in the source, but is not required.
+			// The literal characters `\n` are replaced with newlines, but
+			// otherwise the string is unchanged.
+			expectedErrs := readDiags(ioutil.ReadFile(filepath.Join(testDir, name, "errors")))
+			expectedWarnings := readDiags(ioutil.ReadFile(filepath.Join(testDir, name, "warnings")))
+
+			_, buildDiags := BuildConfig(mod, ModuleWalkerFunc(
+				func(req *ModuleRequest) (*Module, *version.Version, hcl.Diagnostics) {
+					// for simplicity, these tests will treat all source
+					// addresses as relative to the root module
+					sourcePath := filepath.Join(path, req.SourceAddr)
+					mod, diags := parser.LoadConfigDir(sourcePath)
+					version, _ := version.NewVersion("1.0.0")
+					return mod, version, diags
+				},
+			))
+
+			// we can make this less repetitive later if we want
+			for _, msg := range expectedErrs {
+				found := false
+				for _, diag := range buildDiags {
+					if diag.Severity == hcl.DiagError && strings.Contains(diag.Error(), msg) {
+						found = true
+						break
+					}
+				}
+
+				if !found {
+					t.Errorf("Expected error diagnostic containing %q", msg)
+				}
+			}
+
+			for _, diag := range buildDiags {
+				if diag.Severity != hcl.DiagError {
+					continue
+				}
+				found := false
+				for _, msg := range expectedErrs {
+					if strings.Contains(diag.Error(), msg) {
+						found = true
+						break
+					}
+				}
+
+				if !found {
+					t.Errorf("Unexpected error: %q", diag)
+				}
+			}
+
+			for _, msg := range expectedWarnings {
+				found := false
+				for _, diag := range buildDiags {
+					if diag.Severity == hcl.DiagWarning && strings.Contains(diag.Error(), msg) {
+						found = true
+						break
+					}
+				}
+
+				if !found {
+					t.Errorf("Expected warning diagnostic containing %q", msg)
+				}
+			}
+
+			for _, diag := range buildDiags {
+				if diag.Severity != hcl.DiagWarning {
+					continue
+				}
+				found := false
+				for _, msg := range expectedWarnings {
+					if strings.Contains(diag.Error(), msg) {
+						found = true
+						break
+					}
+				}
+
+				if !found {
+					t.Errorf("Unexpected warning: %q", diag)
+				}
+			}
+
+		})
+	}
+}

configs/provider_requirements.go

@@ -218,9 +218,12 @@ func decodeRequiredProvidersBlock(block *hcl.Block) (*RequiredProviders, hcl.Dia
 
 		}
 
-		// finally add the required provider as long as there were no errors
+		if diags.HasErrors() {
-		if !diags.HasErrors() {
+			continue
-			// if a source was not given, create an implied type
+		}
+
+		// We can add the required provider when there are no errors.
+		// If a source was not given, create an implied type.
 		if rp.Type.IsZero() {
 			pType, err := addrs.ParseProviderPart(rp.Name)
 			if err != nil {
@@ -237,7 +240,6 @@ func decodeRequiredProvidersBlock(block *hcl.Block) (*RequiredProviders, hcl.Dia
 
 		ret.RequiredProviders[rp.Name] = rp
 	}
-	}
 
 	return ret, diags
 }

configs/provider_validation.go

@@ -0,0 +1,243 @@
+package configs
+
+import (
+	"fmt"
+	"strings"
+
+	"github.com/hashicorp/hcl/v2"
+	"github.com/hashicorp/terraform/addrs"
+)
+
+// validateProviderConfigs walks the full configuration tree from the root
+// module outward, static validation rules to the various combinations of
+// provider configuration, required_providers values, and module call providers
+// mappings.
+//
+// To retain compatibility with previous terraform versions, empty "proxy
+// provider blocks" are still allowed within modules, though they will
+// generate warnings when the configuration is loaded. The new validation
+// however will generate an error if a suitable provider configuration is not
+// passed in through the module call.
+//
+// The call argument is the ModuleCall for the provided Config cfg. The
+// noProviderConfig argument is passed down the call stack, indicating that the
+// module call, or a parent module call, has used a feature that precludes
+// providers from being configured at all within the module.
+func validateProviderConfigs(call *ModuleCall, cfg *Config, noProviderConfig bool) (diags hcl.Diagnostics) {
+	for name, child := range cfg.Children {
+		mc := cfg.Module.ModuleCalls[name]
+
+		// if the module call has any of count, for_each or depends_on,
+		// providers are prohibited from being configured in this module, or
+		// any module beneath this module.
+		nope := noProviderConfig || mc.Count != nil || mc.ForEach != nil || mc.DependsOn != nil
+		diags = append(diags, validateProviderConfigs(mc, child, nope)...)
+	}
+
+	// nothing else to do in the root module
+	if call == nil {
+		return diags
+	}
+
+	// the set of provider configuration names passed into the module, with the
+	// source range of the provider assignment in the module call.
+	passedIn := map[string]PassedProviderConfig{}
+
+	// the set of empty configurations that could be proxy configurations, with
+	// the source range of the empty configuration block.
+	emptyConfigs := map[string]*hcl.Range{}
+
+	// the set of provider with a defined configuration, with the source range
+	// of the configuration block declaration.
+	configured := map[string]*hcl.Range{}
+
+	// the set of configuration_aliases defined in the required_providers
+	// block, with the fully qualified provider type.
+	configAliases := map[string]addrs.AbsProviderConfig{}
+
+	// the set of provider names defined in the required_providers block, and
+	// their provider types.
+	localNames := map[string]addrs.AbsProviderConfig{}
+
+	for _, passed := range call.Providers {
+		name := providerName(passed.InChild.Name, passed.InChild.Alias)
+		passedIn[name] = passed
+	}
+
+	mod := cfg.Module
+
+	for _, pc := range mod.ProviderConfigs {
+		name := providerName(pc.Name, pc.Alias)
+		// Validate the config against an empty schema to see if it's empty.
+		_, pcConfigDiags := pc.Config.Content(&hcl.BodySchema{})
+		if pcConfigDiags.HasErrors() || pc.Version.Required != nil {
+			configured[name] = &pc.DeclRange
+		} else {
+			emptyConfigs[name] = &pc.DeclRange
+		}
+	}
+
+	if mod.ProviderRequirements != nil {
+		for _, req := range mod.ProviderRequirements.RequiredProviders {
+			addr := addrs.AbsProviderConfig{
+				Module:   cfg.Path,
+				Provider: req.Type,
+			}
+			localNames[req.Name] = addr
+			for _, alias := range req.Aliases {
+				addr := addrs.AbsProviderConfig{
+					Module:   cfg.Path,
+					Provider: req.Type,
+					Alias:    alias.Alias,
+				}
+				configAliases[providerName(alias.LocalName, alias.Alias)] = addr
+			}
+		}
+	}
+
+	// there cannot be any configurations if no provider config is allowed
+	if len(configured) > 0 && noProviderConfig {
+		diags = append(diags, &hcl.Diagnostic{
+			Severity: hcl.DiagError,
+			Summary:  fmt.Sprintf("Module %s contains provider configuration", cfg.Path),
+			Detail:   "Providers cannot be configured within modules using count, for_each or depends_on.",
+		})
+	}
+
+	// now check that the user is not attempting to override a config
+	for name := range configured {
+		if passed, ok := passedIn[name]; ok {
+			diags = append(diags, &hcl.Diagnostic{
+				Severity: hcl.DiagError,
+				Summary:  "Cannot override provider configuration",
+				Detail:   fmt.Sprintf("Provider %s is configured within the module %s and cannot be overridden.", name, cfg.Path),
+				Subject:  &passed.InChild.NameRange,
+			})
+		}
+	}
+
+	// A declared alias requires either a matching configuration within the
+	// module, or one must be passed in.
+	for name, providerAddr := range configAliases {
+		_, confOk := configured[name]
+		_, passedOk := passedIn[name]
+
+		if confOk || passedOk {
+			continue
+		}
+
+		diags = append(diags, &hcl.Diagnostic{
+			Severity: hcl.DiagError,
+			Summary:  fmt.Sprintf("No configuration for provider %s", name),
+			Detail:   fmt.Sprintf("Configuration required for %s.", providerAddr),
+			Subject:  &call.DeclRange,
+		})
+	}
+
+	// You cannot pass in a provider that cannot be used
+	for name, passed := range passedIn {
+		providerAddr := addrs.AbsProviderConfig{
+			Module:   cfg.Path,
+			Provider: addrs.NewDefaultProvider(passed.InChild.Name),
+			Alias:    passed.InChild.Alias,
+		}
+
+		localAddr, localName := localNames[name]
+		if localName {
+			providerAddr = localAddr
+		}
+
+		aliasAddr, configAlias := configAliases[name]
+		if configAlias {
+			providerAddr = aliasAddr
+		}
+
+		_, emptyConfig := emptyConfigs[name]
+
+		if !(localName || configAlias || emptyConfig) {
+			severity := hcl.DiagError
+
+			// we still allow default configs, so switch to a warning if the incoming provider is a default
+			if providerAddr.Provider.IsDefault() {
+				severity = hcl.DiagWarning
+			}
+
+			diags = append(diags, &hcl.Diagnostic{
+				Severity: severity,
+				Summary:  fmt.Sprintf("Provider %s is undefined", name),
+				Detail: fmt.Sprintf("Module %s does not declare a provider named %s.\n", cfg.Path, name) +
+					fmt.Sprintf("If you wish to specify a provider configuration for the module, add an entry for %s in the required_providers block within the module.", name),
+				Subject: &passed.InChild.NameRange,
+			})
+		}
+
+		// The provider being passed in must also be of the correct type.
+		// While we would like to ensure required_providers exists here,
+		// implied default configuration is still allowed.
+		pTy := addrs.NewDefaultProvider(passed.InParent.Name)
+
+		// use the full address for a nice diagnostic output
+		parentAddr := addrs.AbsProviderConfig{
+			Module:   cfg.Parent.Path,
+			Provider: pTy,
+			Alias:    passed.InParent.Alias,
+		}
+
+		if cfg.Parent.Module.ProviderRequirements != nil {
+			req, defined := cfg.Parent.Module.ProviderRequirements.RequiredProviders[name]
+			if defined {
+				parentAddr.Provider = req.Type
+			}
+		}
+
+		if !providerAddr.Provider.Equals(parentAddr.Provider) {
+			diags = append(diags, &hcl.Diagnostic{
+				Severity: hcl.DiagError,
+				Summary:  fmt.Sprintf("Invalid type for provider %s", providerAddr),
+				Detail: fmt.Sprintf("Cannot use configuration from %s for %s. ", parentAddr, providerAddr) +
+					"The given provider configuration is for a different provider type.",
+				Subject: &passed.InChild.NameRange,
+			})
+		}
+	}
+
+	// Empty configurations are no longer needed
+	for name, src := range emptyConfigs {
+		detail := fmt.Sprintf("Remove the %s provider block from %s.", name, cfg.Path)
+
+		isAlias := strings.Contains(name, ".")
+		_, isConfigAlias := configAliases[name]
+		_, isLocalName := localNames[name]
+
+		if isAlias && !isConfigAlias {
+			localName := strings.Split(name, ".")[0]
+			detail = fmt.Sprintf("Remove the %s provider block from %s. Add %s to the list of configuration_aliases for %s in required_providers to define the provider configuration name.", name, cfg.Path, name, localName)
+		}
+
+		if !isAlias && !isLocalName {
+			// if there is no local name, add a note to include it in the
+			// required_provider block
+			detail += fmt.Sprintf("\nTo ensure the correct provider configuration is used, add %s to the required_providers configuration", name)
+		}
+
+		diags = append(diags, &hcl.Diagnostic{
+			Severity: hcl.DiagWarning,
+			Summary:  "Empty provider configuration blocks are not required",
+			Detail:   detail,
+			Subject:  src,
+		})
+	}
+
+	if diags.HasErrors() {
+		return diags
+	}
+
+	return diags
+}
+
+func providerName(name, alias string) string {
+	if alias != "" {
+		name = name + "." + alias
+	}
+	return name
+}

configs/testdata/config-diagnostics/empty-configs/main.tf

@@ -0,0 +1,20 @@
+terraform {
+  required_providers {
+    foo = {
+      source = "hashicorp/foo"
+    }
+    baz = {
+      source = "hashicorp/baz"
+    }
+  }
+}
+
+module "mod" {
+  source = "./mod"
+  providers = {
+    foo = foo
+    foo.bar = foo
+    baz = baz
+    baz.bing = baz
+  }
+}

configs/testdata/config-diagnostics/empty-configs/mod/main.tf

@@ -0,0 +1,22 @@
+terraform {
+  required_providers {
+    foo = {
+      source = "hashicorp/foo"
+      configuration_aliases = [ foo.bar ]
+    }
+  }
+}
+
+provider "foo" {
+}
+
+provider "foo" {
+  alias = "bar"
+}
+
+provider "baz" {
+}
+
+provider "baz" {
+  alias = "bing"
+}

configs/testdata/config-diagnostics/empty-configs/warnings

@@ -0,0 +1,4 @@
+empty-configs/mod/main.tf:10,1-15: Empty provider configuration blocks are not required; Remove the foo provider block from module.mod
+empty-configs/mod/main.tf:13,1-15: Empty provider configuration blocks are not required; Remove the foo.bar provider block from module.mod
+empty-configs/mod/main.tf:17,1-15: Empty provider configuration blocks are not required; Remove the baz provider block from module.mod.\nTo ensure the correct provider configuration is used, add baz to the required_providers configuration
+empty-configs/mod/main.tf:20,1-15: Empty provider configuration blocks are not required; Remove the baz.bing provider block from module.mod. Add baz.bing to the list of configuration_aliases for baz in required_providers to define the provider configuration name

configs/testdata/config-diagnostics/incorrect-type/errors

@@ -0,0 +1 @@
+incorrect-type/main.tf:15,5-8: Invalid type for provider module.mod.provider["example.com/vendor/foo"]; Cannot use configuration from provider["registry.terraform.io/hashicorp/foo"] for module.mod.provider["example.com/vendor/foo"]

configs/testdata/config-diagnostics/incorrect-type/main.tf

@@ -0,0 +1,18 @@
+terraform {
+  required_providers {
+    foo = {
+      source = "hashicorp/foo"
+    }
+    baz = {
+      source = "hashicorp/baz"
+    }
+  }
+}
+
+module "mod" {
+  source = "./mod"
+  providers = {
+    foo = foo
+    baz = baz
+  }
+}

configs/testdata/config-diagnostics/incorrect-type/mod/main.tf

@@ -0,0 +1,14 @@
+terraform {
+  required_providers {
+    foo = {
+      source = "example.com/vendor/foo"
+    }
+  }
+}
+
+resource "foo_resource" "a" {
+}
+
+// implied default provider baz
+resource "baz_resource" "a" {
+}

configs/testdata/config-diagnostics/incorrect-type/warnings

@@ -0,0 +1 @@
+incorrect-type/main.tf:16,5-8: Provider baz is undefined; Module module.mod does not declare a provider named baz.\nIf you wish to specify a provider configuration for the module

configs/testdata/config-diagnostics/nested-provider/child/child2/main.tf

@@ -0,0 +1,7 @@
+provider "aws" {
+  value = "foo"
+}
+
+output "my_output" {
+  value = "my output"
+}

configs/testdata/config-diagnostics/nested-provider/child/main.tf

@@ -0,0 +1,4 @@
+module "child2" {
+  // the test fixture treats these sources as relative to the root
+  source = "./child/child2"
+}

configs/testdata/config-diagnostics/nested-provider/errors

@@ -0,0 +1,3 @@
+Module module.child.module.child2 contains provider configuration; Providers cannot be configured within modules using count, for_each or depends_on
+
+

configs/testdata/config-diagnostics/nested-provider/root.tf

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

configs/testdata/config-diagnostics/override-provider/errors

@@ -0,0 +1 @@
+override-provider/main.tf:17,5-8: Cannot override provider configuration; Provider bar is configured within the module module.mod and cannot be overridden.

configs/testdata/config-diagnostics/override-provider/main.tf

@@ -0,0 +1,19 @@
+terraform {
+  required_providers {
+    bar = {
+      version = "~>1.0.0"
+    }
+  }
+}
+
+provider "bar" {
+  value = "not ok"
+}
+
+// this module configures its own provider, which cannot be overridden
+module "mod" {
+  source = "./mod"
+  providers = {
+    bar = bar
+  }
+}

configs/testdata/config-diagnostics/override-provider/mod/main.tf

@@ -0,0 +1,12 @@
+terraform {
+  required_providers {
+    bar = {
+      version = "~>1.0.0"
+    }
+  }
+}
+
+// this configuration cannot be overridden from an outside module
+provider "bar" {
+  value = "ok"
+}

configs/testdata/config-diagnostics/required-alias/errors

@@ -0,0 +1 @@
+required-alias/main.tf:1,1-13: No configuration for provider foo.bar; Configuration required for module.mod.provider["registry.terraform.io/hashicorp/foo"].bar

configs/testdata/config-diagnostics/required-alias/main.tf

@@ -0,0 +1,4 @@
+module "mod" {
+  source = "./mod"
+  // missing providers with foo.bar provider config
+}

configs/testdata/config-diagnostics/required-alias/mod/main.tf

@@ -0,0 +1,13 @@
+terraform {
+  required_providers {
+    foo = {
+      source = "hashicorp/foo"
+      version = "1.0.0"
+      configuration_aliases = [ foo.bar ]
+    }
+  }
+}
+
+resource "foo_resource" "a" {
+  provider = foo.bar
+}

configs/testdata/config-diagnostics/unexpected-provider/main.tf

@@ -0,0 +1,15 @@
+terraform {
+  required_providers {
+    foo = {
+      source = "hashicorp/foo"
+      version = "1.0.0"
+    }
+  }
+}
+
+module "mod" {
+  source = "./mod"
+  providers = {
+    foo = foo
+  }
+}

configs/testdata/config-diagnostics/unexpected-provider/mod/main.tf

@@ -0,0 +1,2 @@
+resource "foo_resource" "a" {
+}

configs/testdata/config-diagnostics/unexpected-provider/warnings

@@ -0,0 +1,2 @@
+unexpected-provider/main.tf:13,5-8: Provider foo is undefined; Module module.mod does not declare a provider named foo.
+

configs/testdata/config-diagnostics/with-depends-on/main.tf

@@ -0,0 +1,14 @@
+terraform {
+  required_providers {
+    foo = {
+      source = "hashicorp/foo"
+    }
+  }
+}
+
+module "mod2" {
+  source = "./mod1"
+  providers = {
+    foo = foo
+  }
+}

configs/testdata/config-diagnostics/with-depends-on/mod1/main.tf

@@ -0,0 +1,19 @@
+terraform {
+  required_providers {
+    foo = {
+      source = "hashicorp/foo"
+    }
+  }
+}
+
+resource "foo_resource" "a" {
+}
+
+module "mod2" {
+  depends_on = [foo_resource.a]
+  // test fixture source is from root
+  source = "./mod1/mod2"
+  providers = {
+    foo = foo
+  }
+}

configs/testdata/config-diagnostics/with-depends-on/mod1/mod2/main.tf

@@ -0,0 +1,15 @@
+terraform {
+  required_providers {
+    foo = {
+      source = "hashicorp/foo"
+    }
+  }
+}
+
+module "mod3" {
+  // test fixture source is from root
+  source = "./mod1/mod2/mod3"
+  providers = {
+    foo.bar = foo
+  }
+}

configs/testdata/config-diagnostics/with-depends-on/mod1/mod2/mod3/main.tf

@@ -0,0 +1,12 @@
+terraform {
+  required_providers {
+    foo = {
+      source = "hashicorp/foo"
+      configuration_aliases = [ foo.bar ]
+    }
+  }
+}
+
+resource "foo_resource" "a" {
+  providers = foo.bar
+}