Provider configuration_aliases and module validation

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/configload/loader_load.go

@@ -100,83 +100,5 @@ func (l *Loader) moduleWalkerLoad(req *configs.ModuleRequest) (*configs.Module,
 		}
 	}
 
-	// The providers associated with expanding modules must be present in the proxy/passed providers
-	// block. Guarding here for accessing the module call just in case.
-	if mc, exists := req.Parent.Module.ModuleCalls[req.Name]; exists {
-		var validateDiags hcl.Diagnostics
-		validateDiags = validateProviderConfigs(mc, mod, req.Parent, validateDiags)
-		diags = append(diags, validateDiags...)
-	}
 	return mod, record.Version, diags
 }
-
-func validateProviderConfigs(mc *configs.ModuleCall, mod *configs.Module, parent *configs.Config, diags hcl.Diagnostics) hcl.Diagnostics {
-	if mc.Count != nil || mc.ForEach != nil || mc.DependsOn != nil {
-		for key, pc := range mod.ProviderConfigs {
-			// Use these to track if a provider is configured (not allowed),
-			// or if we've found its matching proxy
-			var isConfigured bool
-			var foundMatchingProxy bool
-
-			// 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 {
-				isConfigured = true
-			}
-
-			// If it is empty or only has an alias,
-			// does this provider exist in our proxy configs?
-			for _, r := range mc.Providers {
-				// Must match on name and Alias
-				if pc.Name == r.InChild.Name && pc.Alias == r.InChild.Alias {
-					foundMatchingProxy = true
-					break
-				}
-			}
-			if isConfigured || !foundMatchingProxy {
-				if mc.Count != nil {
-					diags = append(diags, &hcl.Diagnostic{
-						Severity: hcl.DiagError,
-						Summary:  "Module does not support count",
-						Detail:   fmt.Sprintf(moduleProviderError, mc.Name, "count", key, pc.NameRange),
-						Subject:  mc.Count.Range().Ptr(),
-					})
-				}
-				if mc.ForEach != nil {
-					diags = append(diags, &hcl.Diagnostic{
-						Severity: hcl.DiagError,
-						Summary:  "Module does not support for_each",
-						Detail:   fmt.Sprintf(moduleProviderError, mc.Name, "for_each", key, pc.NameRange),
-						Subject:  mc.ForEach.Range().Ptr(),
-					})
-				}
-				if mc.DependsOn != nil {
-					diags = append(diags, &hcl.Diagnostic{
-						Severity: hcl.DiagError,
-						Summary:  "Module does not support depends_on",
-						Detail:   fmt.Sprintf(moduleProviderError, mc.Name, "depends_on", key, pc.NameRange),
-						Subject:  mc.SourceAddrRange.Ptr(),
-					})
-				}
-			}
-		}
-	}
-	// If this module has further parents, go through them recursively
-	if !parent.Path.IsRoot() {
-		// Use the path to get the name so we can look it up in the parent module calls
-		path := parent.Path
-		name := path[len(path)-1]
-		// This parent's module call, so we can check for count/for_each here,
-		// guarding with exists just in case. We pass the diags through to the recursive
-		// call so they will accumulate if needed.
-		if mc, exists := parent.Parent.Module.ModuleCalls[name]; exists {
-			return validateProviderConfigs(mc, mod, parent.Parent, diags)
-		}
-	}
-
-	return diags
-}
-
-var moduleProviderError = `Module "%s" cannot be used with %s because it contains a nested provider configuration for "%s", at %s.
-
-This module can be made compatible with %[2]s by changing it to receive all of its provider configurations from the calling module, by using the "providers" argument in the calling module block.`

configs/configload/loader_load_test.go

@@ -1,7 +1,6 @@
 package configload
 
 import (
-	"fmt"
 	"path/filepath"
 	"reflect"
 	"sort"
@@ -81,65 +80,3 @@ func TestLoaderLoadConfig_addVersion(t *testing.T) {
 		t.Fatalf("wrong error\ngot:\n%s\n\nwant: containing %q", got, want)
 	}
 }
-
-func TestLoaderLoadConfig_moduleExpand(t *testing.T) {
-	// We do not allow providers to be configured in expanding modules
-	// In addition, if a provider is present but an empty block, it is allowed,
-	// but IFF a provider is passed through the module call
-	paths := []string{"provider-configured", "no-provider-passed", "nested-provider", "more-nested-provider"}
-	for _, p := range paths {
-		fixtureDir := filepath.Clean(fmt.Sprintf("testdata/expand-modules/%s", p))
-		loader, err := NewLoader(&Config{
-			ModulesDir: filepath.Join(fixtureDir, ".terraform/modules"),
-		})
-		if err != nil {
-			t.Fatalf("unexpected error from NewLoader at path %s: %s", p, err)
-		}
-
-		_, diags := loader.LoadConfig(fixtureDir)
-		if !diags.HasErrors() {
-			t.Fatalf("success; want error at path %s", p)
-		}
-		got := diags.Error()
-		want := "Module does not support count"
-		if !strings.Contains(got, want) {
-			t.Fatalf("wrong error at path %s \ngot:\n%s\n\nwant: containing %q", p, got, want)
-		}
-	}
-}
-
-func TestLoaderLoadConfig_moduleExpandValid(t *testing.T) {
-	// This tests for when valid configs are passing a provider through as a proxy,
-	// either with or without an alias present.
-	fixtureDir := filepath.Clean("testdata/expand-modules/valid")
-	loader, err := NewLoader(&Config{
-		ModulesDir: filepath.Join(fixtureDir, ".terraform/modules"),
-	})
-	if err != nil {
-		t.Fatalf("unexpected error from NewLoader: %s", err)
-	}
-
-	_, diags := loader.LoadConfig(fixtureDir)
-	assertNoDiagnostics(t, diags)
-}
-
-func TestLoaderLoadConfig_moduleDependsOnProviders(t *testing.T) {
-	// We do not allow providers to be configured in module using depends_on.
-	fixtureDir := filepath.Clean("testdata/module-depends-on")
-	loader, err := NewLoader(&Config{
-		ModulesDir: filepath.Join(fixtureDir, ".terraform/modules"),
-	})
-	if err != nil {
-		t.Fatalf("unexpected error from NewLoader: %s", err)
-	}
-
-	_, diags := loader.LoadConfig(fixtureDir)
-	if !diags.HasErrors() {
-		t.Fatal("success; want error")
-	}
-	got := diags.Error()
-	want := "Module does not support depends_on"
-	if !strings.Contains(got, want) {
-		t.Fatalf("wrong error\ngot:\n%s\n\nwant: containing %q", got, want)
-	}
-}