From f82700bc560dae1200741da72f0ac3b18daa6709 Mon Sep 17 00:00:00 2001 From: Pam Selle Date: Fri, 8 May 2020 11:35:28 -0400 Subject: [PATCH] Disallow provider configuration in expanding modules (#24892) Validate providers in expanding modules. Expanding modules cannot have provider configurations with non-empty configs, which includes having a version configured. If an empty or alias-only block is passed, the provider must be passed through the providers argument on the module call --- configs/configload/loader_load.go | 55 ++++++++++++++++++- configs/configload/loader_load_test.go | 46 +++++++++++++++- .../.terraform/modules/modules.json | 14 +++++ .../no-provider-passed/child/main.tf | 7 +++ .../expand-modules/no-provider-passed/root.tf | 9 +++ .../.terraform/modules/modules.json | 14 +++++ .../provider-configured/child/main.tf | 8 +++ .../provider-configured/root.tf | 11 ++++ .../valid/.terraform/modules/modules.json | 19 +++++++ .../valid/child-with-alias/main.tf | 8 +++ .../expand-modules/valid/child/main.tf | 7 +++ .../testdata/expand-modules/valid/root.tf | 20 +++++++ 12 files changed, 213 insertions(+), 5 deletions(-) create mode 100644 configs/configload/testdata/expand-modules/no-provider-passed/.terraform/modules/modules.json create mode 100644 configs/configload/testdata/expand-modules/no-provider-passed/child/main.tf create mode 100644 configs/configload/testdata/expand-modules/no-provider-passed/root.tf create mode 100644 configs/configload/testdata/expand-modules/provider-configured/.terraform/modules/modules.json create mode 100644 configs/configload/testdata/expand-modules/provider-configured/child/main.tf create mode 100644 configs/configload/testdata/expand-modules/provider-configured/root.tf create mode 100644 configs/configload/testdata/expand-modules/valid/.terraform/modules/modules.json create mode 100644 configs/configload/testdata/expand-modules/valid/child-with-alias/main.tf create mode 100644 configs/configload/testdata/expand-modules/valid/child/main.tf create mode 100644 configs/configload/testdata/expand-modules/valid/root.tf diff --git a/configs/configload/loader_load.go b/configs/configload/loader_load.go index 80b2de1b5f..c529e8324b 100644 --- a/configs/configload/loader_load.go +++ b/configs/configload/loader_load.go @@ -17,7 +17,7 @@ import ( // in spite of the errors. // // LoadConfig performs the basic syntax and uniqueness validations that are -// required to process the individual modules, and also detects +// required to process the individual modules func (l *Loader) LoadConfig(rootDir string) (*configs.Config, hcl.Diagnostics) { rootMod, diags := l.parser.LoadConfigDir(rootDir) if rootMod == nil { @@ -31,8 +31,7 @@ func (l *Loader) LoadConfig(rootDir string) (*configs.Config, hcl.Diagnostics) { } // moduleWalkerLoad is a configs.ModuleWalkerFunc for loading modules that -// are presumed to have already been installed. A different function -// (moduleWalkerInstall) is used for installation. +// are presumed to have already been installed. func (l *Loader) moduleWalkerLoad(req *configs.ModuleRequest) (*configs.Module, *version.Version, hcl.Diagnostics) { // Since we're just loading here, we expect that all referenced modules // will be already installed and described in our manifest. However, we @@ -101,5 +100,55 @@ 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 { + if mc.Count != nil || mc.ForEach != 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(), + }) + } + } + } + } + } return mod, record.Version, 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.` diff --git a/configs/configload/loader_load_test.go b/configs/configload/loader_load_test.go index 8a56c7551b..0f60fb5340 100644 --- a/configs/configload/loader_load_test.go +++ b/configs/configload/loader_load_test.go @@ -1,6 +1,7 @@ package configload import ( + "fmt" "path/filepath" "reflect" "sort" @@ -75,8 +76,49 @@ func TestLoaderLoadConfig_addVersion(t *testing.T) { t.Fatalf("success; want error") } got := diags.Error() - want := "Module requirements have changed" - if strings.Contains(got, want) { + want := "Module version requirements have changed" + if !strings.Contains(got, want) { 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"} + 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: %s", err) + } + + _, diags := loader.LoadConfig(fixtureDir) + if !diags.HasErrors() { + t.Fatalf("success; want error") + } + got := diags.Error() + want := "Module does not support count" + if !strings.Contains(got, want) { + t.Fatalf("wrong error\ngot:\n%s\n\nwant: containing %q", 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) +} diff --git a/configs/configload/testdata/expand-modules/no-provider-passed/.terraform/modules/modules.json b/configs/configload/testdata/expand-modules/no-provider-passed/.terraform/modules/modules.json new file mode 100644 index 0000000000..8c0d923676 --- /dev/null +++ b/configs/configload/testdata/expand-modules/no-provider-passed/.terraform/modules/modules.json @@ -0,0 +1,14 @@ +{ + "Modules": [ + { + "Key": "", + "Source": "", + "Dir": "testdata/expand-modules/no-provider-passed" + }, + { + "Key": "child", + "Source": "./child", + "Dir": "testdata/expand-modules/no-provider-passed/child" + } + ] +} diff --git a/configs/configload/testdata/expand-modules/no-provider-passed/child/main.tf b/configs/configload/testdata/expand-modules/no-provider-passed/child/main.tf new file mode 100644 index 0000000000..a5c3c47b17 --- /dev/null +++ b/configs/configload/testdata/expand-modules/no-provider-passed/child/main.tf @@ -0,0 +1,7 @@ +provider "aws" { +} + +output "my_output" { + value = "my output" +} + diff --git a/configs/configload/testdata/expand-modules/no-provider-passed/root.tf b/configs/configload/testdata/expand-modules/no-provider-passed/root.tf new file mode 100644 index 0000000000..195cfeb5d4 --- /dev/null +++ b/configs/configload/testdata/expand-modules/no-provider-passed/root.tf @@ -0,0 +1,9 @@ +provider "aws" { + alias = "usw2" + region = "us-west-2" +} +module "child" { + count = 1 + source = "./child" + # To make this test fail, add a valid providers {} block passing "aws" to the child +} diff --git a/configs/configload/testdata/expand-modules/provider-configured/.terraform/modules/modules.json b/configs/configload/testdata/expand-modules/provider-configured/.terraform/modules/modules.json new file mode 100644 index 0000000000..b7a474ea32 --- /dev/null +++ b/configs/configload/testdata/expand-modules/provider-configured/.terraform/modules/modules.json @@ -0,0 +1,14 @@ +{ + "Modules": [ + { + "Key": "", + "Source": "", + "Dir": "testdata/expand-modules/provider-configured" + }, + { + "Key": "child", + "Source": "./child", + "Dir": "testdata/expand-modules/provider-configured/child" + } + ] +} diff --git a/configs/configload/testdata/expand-modules/provider-configured/child/main.tf b/configs/configload/testdata/expand-modules/provider-configured/child/main.tf new file mode 100644 index 0000000000..b77075fc0e --- /dev/null +++ b/configs/configload/testdata/expand-modules/provider-configured/child/main.tf @@ -0,0 +1,8 @@ +provider "aws" { + version = "1.0" +} + +output "my_output" { + value = "my output" +} + diff --git a/configs/configload/testdata/expand-modules/provider-configured/root.tf b/configs/configload/testdata/expand-modules/provider-configured/root.tf new file mode 100644 index 0000000000..953d4ab554 --- /dev/null +++ b/configs/configload/testdata/expand-modules/provider-configured/root.tf @@ -0,0 +1,11 @@ +provider "aws" { + region = "us-west-2" +} + +module "child" { + count = 1 + source = "./child" + providers = { + aws = aws.w2 + } +} diff --git a/configs/configload/testdata/expand-modules/valid/.terraform/modules/modules.json b/configs/configload/testdata/expand-modules/valid/.terraform/modules/modules.json new file mode 100644 index 0000000000..0bdb37d5bd --- /dev/null +++ b/configs/configload/testdata/expand-modules/valid/.terraform/modules/modules.json @@ -0,0 +1,19 @@ +{ + "Modules": [ + { + "Key": "", + "Source": "", + "Dir": "testdata/expand-modules/valid" + }, + { + "Key": "child", + "Source": "./child", + "Dir": "testdata/expand-modules/valid/child" + }, + { + "Key": "child_with_alias", + "Source": "./child-with-alias", + "Dir": "testdata/expand-modules/valid/child-with-alias" + } + ] +} diff --git a/configs/configload/testdata/expand-modules/valid/child-with-alias/main.tf b/configs/configload/testdata/expand-modules/valid/child-with-alias/main.tf new file mode 100644 index 0000000000..3a59131cb9 --- /dev/null +++ b/configs/configload/testdata/expand-modules/valid/child-with-alias/main.tf @@ -0,0 +1,8 @@ +provider "aws" { + alias = "east" +} + +output "my_output" { + value = "my output" +} + diff --git a/configs/configload/testdata/expand-modules/valid/child/main.tf b/configs/configload/testdata/expand-modules/valid/child/main.tf new file mode 100644 index 0000000000..a5c3c47b17 --- /dev/null +++ b/configs/configload/testdata/expand-modules/valid/child/main.tf @@ -0,0 +1,7 @@ +provider "aws" { +} + +output "my_output" { + value = "my output" +} + diff --git a/configs/configload/testdata/expand-modules/valid/root.tf b/configs/configload/testdata/expand-modules/valid/root.tf new file mode 100644 index 0000000000..27205a508a --- /dev/null +++ b/configs/configload/testdata/expand-modules/valid/root.tf @@ -0,0 +1,20 @@ +provider "aws" { + region = "us-east-1" + alias = "east" +} + +module "child" { + count = 1 + source = "./child" + providers = { + aws = aws.east + } +} + +module "child_with_alias" { + for_each = toset(["a", "b"]) + source = "./child-with-alias" + providers = { + aws.east = aws.east + } +} \ No newline at end of file