From 7aaffac223c62bfb262907e2d0d2a01eea1bf227 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 10 Feb 2021 10:20:40 -0500 Subject: [PATCH] configload should not be doing validation The configload package should only be responsible for locating and loading the configuration, and not be further inspecting the config source itself. Moving the validating into the configs package. --- configs/configload/loader_load.go | 78 ------------------- configs/configload/loader_load_test.go | 63 --------------- .../.terraform/modules/modules.json | 34 -------- .../more-nested-provider/child/main.tf | 4 - .../more-nested-provider/child2/main.tf | 4 - .../more-nested-provider/child3/main.tf | 7 -- .../more-nested-provider/root.tf | 4 - .../.terraform/modules/modules.json | 24 ------ .../nested-provider/child/main.tf | 4 - .../nested-provider/child2/main.tf | 7 -- .../expand-modules/nested-provider/root.tf | 4 - .../.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 | 7 -- .../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 ----- 21 files changed, 349 deletions(-) delete mode 100644 configs/configload/testdata/expand-modules/more-nested-provider/.terraform/modules/modules.json delete mode 100644 configs/configload/testdata/expand-modules/more-nested-provider/child/main.tf delete mode 100644 configs/configload/testdata/expand-modules/more-nested-provider/child2/main.tf delete mode 100644 configs/configload/testdata/expand-modules/more-nested-provider/child3/main.tf delete mode 100644 configs/configload/testdata/expand-modules/more-nested-provider/root.tf delete mode 100644 configs/configload/testdata/expand-modules/nested-provider/.terraform/modules/modules.json delete mode 100644 configs/configload/testdata/expand-modules/nested-provider/child/main.tf delete mode 100644 configs/configload/testdata/expand-modules/nested-provider/child2/main.tf delete mode 100644 configs/configload/testdata/expand-modules/nested-provider/root.tf delete mode 100644 configs/configload/testdata/expand-modules/no-provider-passed/.terraform/modules/modules.json delete mode 100644 configs/configload/testdata/expand-modules/no-provider-passed/child/main.tf delete mode 100644 configs/configload/testdata/expand-modules/no-provider-passed/root.tf delete mode 100644 configs/configload/testdata/expand-modules/provider-configured/.terraform/modules/modules.json delete mode 100644 configs/configload/testdata/expand-modules/provider-configured/child/main.tf delete mode 100644 configs/configload/testdata/expand-modules/provider-configured/root.tf delete mode 100644 configs/configload/testdata/expand-modules/valid/.terraform/modules/modules.json delete mode 100644 configs/configload/testdata/expand-modules/valid/child-with-alias/main.tf delete mode 100644 configs/configload/testdata/expand-modules/valid/child/main.tf delete 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 eab38495c2..e3c9bdca6c 100644 --- a/configs/configload/loader_load.go +++ b/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.` diff --git a/configs/configload/loader_load_test.go b/configs/configload/loader_load_test.go index b7f396cf22..845b227a38 100644 --- a/configs/configload/loader_load_test.go +++ b/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) - } -} diff --git a/configs/configload/testdata/expand-modules/more-nested-provider/.terraform/modules/modules.json b/configs/configload/testdata/expand-modules/more-nested-provider/.terraform/modules/modules.json deleted file mode 100644 index 203e0876d5..0000000000 --- a/configs/configload/testdata/expand-modules/more-nested-provider/.terraform/modules/modules.json +++ /dev/null @@ -1,34 +0,0 @@ -{ - "Modules": [ - { - "Key": "", - "Source": "", - "Dir": "testdata/expand-modules/nested-provider" - }, - { - "Key": "child", - "Source": "./child", - "Dir": "testdata/expand-modules/nested-provider/child" - }, - { - "Key": "child2", - "Source": "./child2", - "Dir": "testdata/expand-modules/nested-provider/child2" - }, - { - "Key": "child3", - "Source": "./child3", - "Dir": "testdata/expand-modules/nested-provider/child3" - }, - { - "Key": "child.child2", - "Source": "../child2", - "Dir": "testdata/expand-modules/nested-provider/child2" - }, - { - "Key": "child.child2.child3", - "Source": "../child3", - "Dir": "testdata/expand-modules/nested-provider/child3" - } - ] -} diff --git a/configs/configload/testdata/expand-modules/more-nested-provider/child/main.tf b/configs/configload/testdata/expand-modules/more-nested-provider/child/main.tf deleted file mode 100644 index b4bbb38c13..0000000000 --- a/configs/configload/testdata/expand-modules/more-nested-provider/child/main.tf +++ /dev/null @@ -1,4 +0,0 @@ -module "child2" { - source = "../child2" - -} diff --git a/configs/configload/testdata/expand-modules/more-nested-provider/child2/main.tf b/configs/configload/testdata/expand-modules/more-nested-provider/child2/main.tf deleted file mode 100644 index d107faad87..0000000000 --- a/configs/configload/testdata/expand-modules/more-nested-provider/child2/main.tf +++ /dev/null @@ -1,4 +0,0 @@ -module "child3" { - source = "../child3" - -} diff --git a/configs/configload/testdata/expand-modules/more-nested-provider/child3/main.tf b/configs/configload/testdata/expand-modules/more-nested-provider/child3/main.tf deleted file mode 100644 index 01cd854232..0000000000 --- a/configs/configload/testdata/expand-modules/more-nested-provider/child3/main.tf +++ /dev/null @@ -1,7 +0,0 @@ -provider "aws" { - -} - -output "my_output" { - value = "my output" -} \ No newline at end of file diff --git a/configs/configload/testdata/expand-modules/more-nested-provider/root.tf b/configs/configload/testdata/expand-modules/more-nested-provider/root.tf deleted file mode 100644 index 71b90f6d67..0000000000 --- a/configs/configload/testdata/expand-modules/more-nested-provider/root.tf +++ /dev/null @@ -1,4 +0,0 @@ -module "child" { - count = 1 - source = "./child" -} diff --git a/configs/configload/testdata/expand-modules/nested-provider/.terraform/modules/modules.json b/configs/configload/testdata/expand-modules/nested-provider/.terraform/modules/modules.json deleted file mode 100644 index 28f813039e..0000000000 --- a/configs/configload/testdata/expand-modules/nested-provider/.terraform/modules/modules.json +++ /dev/null @@ -1,24 +0,0 @@ -{ - "Modules": [ - { - "Key": "", - "Source": "", - "Dir": "testdata/expand-modules/nested-provider" - }, - { - "Key": "child", - "Source": "./child", - "Dir": "testdata/expand-modules/nested-provider/child" - }, - { - "Key": "child2", - "Source": "./child2", - "Dir": "testdata/expand-modules/nested-provider/child2" - }, - { - "Key": "child.child2", - "Source": "../child2", - "Dir": "testdata/expand-modules/nested-provider/child2" - } - ] -} diff --git a/configs/configload/testdata/expand-modules/nested-provider/child/main.tf b/configs/configload/testdata/expand-modules/nested-provider/child/main.tf deleted file mode 100644 index b4bbb38c13..0000000000 --- a/configs/configload/testdata/expand-modules/nested-provider/child/main.tf +++ /dev/null @@ -1,4 +0,0 @@ -module "child2" { - source = "../child2" - -} diff --git a/configs/configload/testdata/expand-modules/nested-provider/child2/main.tf b/configs/configload/testdata/expand-modules/nested-provider/child2/main.tf deleted file mode 100644 index 01cd854232..0000000000 --- a/configs/configload/testdata/expand-modules/nested-provider/child2/main.tf +++ /dev/null @@ -1,7 +0,0 @@ -provider "aws" { - -} - -output "my_output" { - value = "my output" -} \ No newline at end of file diff --git a/configs/configload/testdata/expand-modules/nested-provider/root.tf b/configs/configload/testdata/expand-modules/nested-provider/root.tf deleted file mode 100644 index 71b90f6d67..0000000000 --- a/configs/configload/testdata/expand-modules/nested-provider/root.tf +++ /dev/null @@ -1,4 +0,0 @@ -module "child" { - count = 1 - source = "./child" -} 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 deleted file mode 100644 index 8c0d923676..0000000000 --- a/configs/configload/testdata/expand-modules/no-provider-passed/.terraform/modules/modules.json +++ /dev/null @@ -1,14 +0,0 @@ -{ - "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 deleted file mode 100644 index a5c3c47b17..0000000000 --- a/configs/configload/testdata/expand-modules/no-provider-passed/child/main.tf +++ /dev/null @@ -1,7 +0,0 @@ -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 deleted file mode 100644 index 195cfeb5d4..0000000000 --- a/configs/configload/testdata/expand-modules/no-provider-passed/root.tf +++ /dev/null @@ -1,9 +0,0 @@ -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 deleted file mode 100644 index b7a474ea32..0000000000 --- a/configs/configload/testdata/expand-modules/provider-configured/.terraform/modules/modules.json +++ /dev/null @@ -1,14 +0,0 @@ -{ - "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 deleted file mode 100644 index 61ff4b572a..0000000000 --- a/configs/configload/testdata/expand-modules/provider-configured/child/main.tf +++ /dev/null @@ -1,7 +0,0 @@ -provider "aws" { - region = "us-west-2" -} - -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 deleted file mode 100644 index 953d4ab554..0000000000 --- a/configs/configload/testdata/expand-modules/provider-configured/root.tf +++ /dev/null @@ -1,11 +0,0 @@ -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 deleted file mode 100644 index 0bdb37d5bd..0000000000 --- a/configs/configload/testdata/expand-modules/valid/.terraform/modules/modules.json +++ /dev/null @@ -1,19 +0,0 @@ -{ - "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 deleted file mode 100644 index 3a59131cb9..0000000000 --- a/configs/configload/testdata/expand-modules/valid/child-with-alias/main.tf +++ /dev/null @@ -1,8 +0,0 @@ -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 deleted file mode 100644 index a5c3c47b17..0000000000 --- a/configs/configload/testdata/expand-modules/valid/child/main.tf +++ /dev/null @@ -1,7 +0,0 @@ -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 deleted file mode 100644 index 27205a508a..0000000000 --- a/configs/configload/testdata/expand-modules/valid/root.tf +++ /dev/null @@ -1,20 +0,0 @@ -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