From 99948bdc47c43c7c80c92283c489ef47ddf5e6ca Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 20 May 2022 10:52:49 -0700 Subject: [PATCH] configs: A test for the regression reported in #31081 5417975946305e3be4302945f77e9f0babf693df addressed a regression in the logic for catching when the newer module meta-arguments are used in conjunction with the legacy practice of including provider configurations inside modules, where the check started incorrectly catching situations where the legacy nested provider configuration was in the same module as the child call using one of those meta-arguments. This is a regression test to catch if a similar bug arises in the future. Since it's testing validation rules that apply to an entire configuration tree, it ended up in a rather idiosyncratic location under the "configload" package, rather than directly in "configs". The "configs" package only knows how to load one module at a time, so it's harder to write a test like this in that context. Due to it being further removed from the code it is testing, I included a test for the correct error too in order to increase the chance that we'll learn if future changes in the "configs" package invalidate this regression test. I've verified that this new test fails without the change made in the earlier commit. --- .../configs/configload/loader_load_test.go | 74 +++++++++++++++++++ .../.terraform/modules/modules.json | 19 +++++ .../child-provider-child-count.tf | 4 + .../child/child-provider-child-count-child.tf | 7 ++ .../.terraform/modules/modules.json | 19 +++++ .../child-provider-grandchild-count.tf | 3 + .../child-provider-grandchild-count-child.tf | 12 +++ 7 files changed, 138 insertions(+) create mode 100644 internal/configs/configload/testdata/child-provider-child-count/.terraform/modules/modules.json create mode 100644 internal/configs/configload/testdata/child-provider-child-count/child-provider-child-count.tf create mode 100644 internal/configs/configload/testdata/child-provider-child-count/child/child-provider-child-count-child.tf create mode 100644 internal/configs/configload/testdata/child-provider-grandchild-count/.terraform/modules/modules.json create mode 100644 internal/configs/configload/testdata/child-provider-grandchild-count/child-provider-grandchild-count.tf create mode 100644 internal/configs/configload/testdata/child-provider-grandchild-count/child/child-provider-grandchild-count-child.tf diff --git a/internal/configs/configload/loader_load_test.go b/internal/configs/configload/loader_load_test.go index ab8dd5dee630..ee2488c792ba 100644 --- a/internal/configs/configload/loader_load_test.go +++ b/internal/configs/configload/loader_load_test.go @@ -104,3 +104,77 @@ func TestLoaderLoadConfig_loadDiags(t *testing.T) { t.Fatal("expected config module") } } + +func TestLoaderLoadConfig_childProviderGrandchildCount(t *testing.T) { + // This test is focused on the specific situation where: + // - A child module contains a nested provider block, which is no longer + // recommended but supported for backward-compatibility. + // - A child of that child does _not_ contain a nested provider block, + // and is called with "count" (would also apply to "for_each" and + // "depends_on"). + // It isn't valid to use "count" with a module that _itself_ contains + // a provider configuration, but it _is_ valid for a module with a + // provider configuration to call another module with count. We previously + // botched this rule and so this is a regression test to cover the + // solution to that mistake: + // https://github.com/hashicorp/terraform/issues/31081 + + // Since this test is based on success rather than failure and it's + // covering a relatively large set of code where only a small part + // contributes to the test, we'll make sure to test both the success and + // failure cases here so that we'll have a better chance of noticing if a + // future change makes this succeed only because we've reorganized the code + // so that the check isn't happening at all anymore. + // + // If the "not okay" subtest fails, you should also be skeptical about + // whether the "okay" subtest is still valid, even if it happens to + // still be passing. + t.Run("okay", func(t *testing.T) { + fixtureDir := filepath.Clean("testdata/child-provider-grandchild-count") + loader, err := NewLoader(&Config{ + ModulesDir: filepath.Join(fixtureDir, ".terraform/modules"), + }) + if err != nil { + t.Fatalf("unexpected error from NewLoader: %s", err) + } + + cfg, diags := loader.LoadConfig(fixtureDir) + assertNoDiagnostics(t, diags) + if cfg == nil { + t.Fatalf("config is nil; want non-nil") + } + + var gotPaths []string + cfg.DeepEach(func(c *configs.Config) { + gotPaths = append(gotPaths, strings.Join(c.Path, ".")) + }) + sort.Strings(gotPaths) + wantPaths := []string{ + "", // root module + "child", + "child.grandchild", + } + + if !reflect.DeepEqual(gotPaths, wantPaths) { + t.Fatalf("wrong module paths\ngot: %swant %s", spew.Sdump(gotPaths), spew.Sdump(wantPaths)) + } + }) + t.Run("not okay", func(t *testing.T) { + fixtureDir := filepath.Clean("testdata/child-provider-child-count") + 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("loading succeeded; want an error") + } + if got, want := diags.Error(), "Module is incompatible with count, for_each, and depends_on"; !strings.Contains(got, want) { + t.Errorf("missing expected error\nwant substring: %s\ngot: %s", want, got) + } + }) + +} diff --git a/internal/configs/configload/testdata/child-provider-child-count/.terraform/modules/modules.json b/internal/configs/configload/testdata/child-provider-child-count/.terraform/modules/modules.json new file mode 100644 index 000000000000..3a80b67442bb --- /dev/null +++ b/internal/configs/configload/testdata/child-provider-child-count/.terraform/modules/modules.json @@ -0,0 +1,19 @@ +{ + "Modules": [ + { + "Key": "", + "Source": "", + "Dir": "." + }, + { + "Key": "child", + "Source": "./child", + "Dir": "testdata/child-provider-child-count/child" + }, + { + "Key": "child.grandchild", + "Source": "../grandchild", + "Dir": "testdata/child-provider-child-count/grandchild" + } + ] +} diff --git a/internal/configs/configload/testdata/child-provider-child-count/child-provider-child-count.tf b/internal/configs/configload/testdata/child-provider-child-count/child-provider-child-count.tf new file mode 100644 index 000000000000..5b39941a03c6 --- /dev/null +++ b/internal/configs/configload/testdata/child-provider-child-count/child-provider-child-count.tf @@ -0,0 +1,4 @@ +module "child" { + source = "./child" + count = 1 +} diff --git a/internal/configs/configload/testdata/child-provider-child-count/child/child-provider-child-count-child.tf b/internal/configs/configload/testdata/child-provider-child-count/child/child-provider-child-count-child.tf new file mode 100644 index 000000000000..524742c3fcfc --- /dev/null +++ b/internal/configs/configload/testdata/child-provider-child-count/child/child-provider-child-count-child.tf @@ -0,0 +1,7 @@ +provider "boop" { + blah = true +} + +module "grandchild" { + source = "../grandchild" +} diff --git a/internal/configs/configload/testdata/child-provider-grandchild-count/.terraform/modules/modules.json b/internal/configs/configload/testdata/child-provider-grandchild-count/.terraform/modules/modules.json new file mode 100644 index 000000000000..a9239e3a8b69 --- /dev/null +++ b/internal/configs/configload/testdata/child-provider-grandchild-count/.terraform/modules/modules.json @@ -0,0 +1,19 @@ +{ + "Modules": [ + { + "Key": "", + "Source": "", + "Dir": "." + }, + { + "Key": "child", + "Source": "./child", + "Dir": "testdata/child-provider-grandchild-count/child" + }, + { + "Key": "child.grandchild", + "Source": "../grandchild", + "Dir": "testdata/child-provider-grandchild-count/grandchild" + } + ] +} diff --git a/internal/configs/configload/testdata/child-provider-grandchild-count/child-provider-grandchild-count.tf b/internal/configs/configload/testdata/child-provider-grandchild-count/child-provider-grandchild-count.tf new file mode 100644 index 000000000000..1f95749fa7ea --- /dev/null +++ b/internal/configs/configload/testdata/child-provider-grandchild-count/child-provider-grandchild-count.tf @@ -0,0 +1,3 @@ +module "child" { + source = "./child" +} diff --git a/internal/configs/configload/testdata/child-provider-grandchild-count/child/child-provider-grandchild-count-child.tf b/internal/configs/configload/testdata/child-provider-grandchild-count/child/child-provider-grandchild-count-child.tf new file mode 100644 index 000000000000..8d3fe1023d2d --- /dev/null +++ b/internal/configs/configload/testdata/child-provider-grandchild-count/child/child-provider-grandchild-count-child.tf @@ -0,0 +1,12 @@ +provider "boop" { + blah = true +} + +module "grandchild" { + source = "../grandchild" + + # grandchild's caller (this file) has a legacy nested provider block, but + # grandchild itself does not and so it's valid to use "count" here even + # though it wouldn't be valid to call "child" (this file) with "count". + count = 2 +}