From 199157a51a86e4f002664dab9be3021f74affa56 Mon Sep 17 00:00:00 2001 From: Pam Selle Date: Tue, 16 Jun 2020 13:52:41 -0400 Subject: [PATCH] Validation for provider blocks in expanding modules (nested) (#25248) * Refactor provider validation into separate func & recurse Refactors the validate provider functions into a separate function that can recursively search above a module to check and see if any parents of the module contain count/for_each configs to be considered --- configs/configload/loader_load.go | 96 +++++++++++-------- configs/configload/loader_load_test.go | 8 +- .../.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 + 11 files changed, 154 insertions(+), 42 deletions(-) create mode 100644 configs/configload/testdata/expand-modules/more-nested-provider/.terraform/modules/modules.json create mode 100644 configs/configload/testdata/expand-modules/more-nested-provider/child/main.tf create mode 100644 configs/configload/testdata/expand-modules/more-nested-provider/child2/main.tf create mode 100644 configs/configload/testdata/expand-modules/more-nested-provider/child3/main.tf create mode 100644 configs/configload/testdata/expand-modules/more-nested-provider/root.tf create mode 100644 configs/configload/testdata/expand-modules/nested-provider/.terraform/modules/modules.json create mode 100644 configs/configload/testdata/expand-modules/nested-provider/child/main.tf create mode 100644 configs/configload/testdata/expand-modules/nested-provider/child2/main.tf create mode 100644 configs/configload/testdata/expand-modules/nested-provider/root.tf diff --git a/configs/configload/loader_load.go b/configs/configload/loader_load.go index c529e8324b..ba9b29ffd4 100644 --- a/configs/configload/loader_load.go +++ b/configs/configload/loader_load.go @@ -103,50 +103,70 @@ 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 - } + 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 { + 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 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 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 + // 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. diff --git a/configs/configload/loader_load_test.go b/configs/configload/loader_load_test.go index 0f60fb5340..c65e40f258 100644 --- a/configs/configload/loader_load_test.go +++ b/configs/configload/loader_load_test.go @@ -86,24 +86,24 @@ 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"} + 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: %s", err) + t.Fatalf("unexpected error from NewLoader at path %s: %s", p, err) } _, diags := loader.LoadConfig(fixtureDir) if !diags.HasErrors() { - t.Fatalf("success; want error") + 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\ngot:\n%s\n\nwant: containing %q", got, want) + t.Fatalf("wrong error at path %s \ngot:\n%s\n\nwant: containing %q", p, 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 new file mode 100644 index 0000000000..203e0876d5 --- /dev/null +++ b/configs/configload/testdata/expand-modules/more-nested-provider/.terraform/modules/modules.json @@ -0,0 +1,34 @@ +{ + "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 new file mode 100644 index 0000000000..b4bbb38c13 --- /dev/null +++ b/configs/configload/testdata/expand-modules/more-nested-provider/child/main.tf @@ -0,0 +1,4 @@ +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 new file mode 100644 index 0000000000..d107faad87 --- /dev/null +++ b/configs/configload/testdata/expand-modules/more-nested-provider/child2/main.tf @@ -0,0 +1,4 @@ +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 new file mode 100644 index 0000000000..01cd854232 --- /dev/null +++ b/configs/configload/testdata/expand-modules/more-nested-provider/child3/main.tf @@ -0,0 +1,7 @@ +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 new file mode 100644 index 0000000000..71b90f6d67 --- /dev/null +++ b/configs/configload/testdata/expand-modules/more-nested-provider/root.tf @@ -0,0 +1,4 @@ +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 new file mode 100644 index 0000000000..28f813039e --- /dev/null +++ b/configs/configload/testdata/expand-modules/nested-provider/.terraform/modules/modules.json @@ -0,0 +1,24 @@ +{ + "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 new file mode 100644 index 0000000000..b4bbb38c13 --- /dev/null +++ b/configs/configload/testdata/expand-modules/nested-provider/child/main.tf @@ -0,0 +1,4 @@ +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 new file mode 100644 index 0000000000..01cd854232 --- /dev/null +++ b/configs/configload/testdata/expand-modules/nested-provider/child2/main.tf @@ -0,0 +1,7 @@ +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 new file mode 100644 index 0000000000..71b90f6d67 --- /dev/null +++ b/configs/configload/testdata/expand-modules/nested-provider/root.tf @@ -0,0 +1,4 @@ +module "child" { + count = 1 + source = "./child" +}