From 698cb0d59dc4bbbf94d56515c9a262a74dad768e Mon Sep 17 00:00:00 2001 From: Liam Cervante Date: Mon, 14 Oct 2024 10:36:32 +0200 Subject: [PATCH] init: detect and report invalid submodules (#35839) --- internal/initwd/module_install.go | 49 +++++++++++++++---- internal/initwd/module_install_test.go | 42 +++++++++++++++- .../invalid-registry-modules/.gitignore | 1 + .../testdata/invalid-registry-modules/root.tf | 28 +++++++++++ 4 files changed, 109 insertions(+), 11 deletions(-) create mode 100644 internal/initwd/testdata/invalid-registry-modules/.gitignore create mode 100644 internal/initwd/testdata/invalid-registry-modules/root.tf diff --git a/internal/initwd/module_install.go b/internal/initwd/module_install.go index 6599c770b8..9672d14227 100644 --- a/internal/initwd/module_install.go +++ b/internal/initwd/module_install.go @@ -659,16 +659,45 @@ func (i *ModuleInstaller) installRegistryModule(ctx context.Context, req *config // Finally we are ready to try actually loading the module. mod, mDiags := i.loader.Parser().LoadConfigDir(modDir) if mod == nil { - // nil indicates missing or unreadable directory, so we'll - // discard the returned diags and return a more specific - // error message here. For registry modules this actually - // indicates a bug in the code above, since it's not the - // user's responsibility to create the directory in this case. - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Unreadable module directory", - Detail: fmt.Sprintf("The directory %s could not be read. This is a bug in Terraform and should be reported.", modDir), - }) + // a nil module indicates a missing or unreadable directory, typically + // this would indicate that Terraform has done something wrong. + // However, if the subDir is not empty then it is possible that the + // module was properly downloaded but the user is trying to read a + // subdirectory that doesn't exist. In this case, it's not a problem + // with Terraform. + if len(subDir) > 0 { + // Let's make this error message as precise as possible. + _, instErr := os.Stat(instPath) + _, subErr := os.Stat(modDir) + if instErr == nil && os.IsNotExist(subErr) { + // Then the root directory the module was downloaded to could + // be loaded fine, but the subdirectory does not exist. This + // definitely means the user is trying to read a subdirectory + // that doesn't exist. + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Unreadable module subdirectory", + Detail: fmt.Sprintf("The directory %s does not exist. The target submodule %s does not exist within the target module.", modDir, subDir), + }) + } else { + // There's something else gone wrong here, so we'll report it + // as a bug in Terraform. + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Unreadable module directory", + Detail: fmt.Sprintf("The directory %s could not be read. This is a bug in Terraform and should be reported.", modDir), + }) + } + } else { + // If there is no subDir, then somehow the module was downloaded but + // could not be read even at the root directory it was downloaded into. + // This is definitely something that Terraform is doing wrong. + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Unreadable module directory", + Detail: fmt.Sprintf("The directory %s could not be read. This is a bug in Terraform and should be reported.", modDir), + }) + } } else if vDiags := mod.CheckCoreVersionRequirements(req.Path, req.SourceAddr); vDiags.HasErrors() { // If the core version requirements are not met, we drop any other // diagnostics, as they may reflect language changes from future diff --git a/internal/initwd/module_install_test.go b/internal/initwd/module_install_test.go index 217b6cfd8f..dde949ebda 100644 --- a/internal/initwd/module_install_test.go +++ b/internal/initwd/module_install_test.go @@ -431,6 +431,45 @@ func TestModuleInstaller_symlink(t *testing.T) { assertResultDeepEqual(t, gotTraces, wantTraces) } +func TestLoaderInstallModules_invalidRegistry(t *testing.T) { + if os.Getenv("TF_ACC") == "" { + t.Skip("this test accesses registry.terraform.io and github.com; set TF_ACC=1 to run it") + } + + fixtureDir := filepath.Clean("testdata/invalid-registry-modules") + tmpDir, done := tempChdir(t, fixtureDir) + // the module installer runs filepath.EvalSymlinks() on the destination + // directory before copying files, and the resultant directory is what is + // returned by the install hooks. Without this, tests could fail on machines + // where the default temp dir was a symlink. + dir, err := filepath.EvalSymlinks(tmpDir) + if err != nil { + t.Error(err) + } + + defer done() + + hooks := &testInstallHooks{} + modulesDir := filepath.Join(dir, ".terraform/modules") + + loader, close := configload.NewLoaderForTests(t) + defer close() + inst := NewModuleInstaller(modulesDir, loader, registry.NewClient(nil, nil)) + _, diags := inst.InstallModules(context.Background(), dir, "tests", false, false, hooks) + + if !diags.HasErrors() { + t.Fatal("expected error") + } else { + assertDiagnosticCount(t, diags, 1) + assertDiagnosticSummary(t, diags, "Unreadable module subdirectory") + + // the diagnostic should specifically call out the submodule that failed + if !strings.Contains(diags[0].Description().Detail, "target submodule modules/child_c") { + t.Errorf("unmatched error detail") + } + } +} + func TestLoaderInstallModules_registry(t *testing.T) { if os.Getenv("TF_ACC") == "" { t.Skip("this test accesses registry.terraform.io and github.com; set TF_ACC=1 to run it") @@ -980,13 +1019,14 @@ func assertDiagnosticSummary(t *testing.T, diags tfdiags.Diagnostics, want strin for _, diag := range diags { if diag.Description().Summary == want { + t.Logf("matching diagnostic detail %q", diag.Description().Detail) return false } } t.Errorf("missing diagnostic summary %q", want) for _, diag := range diags { - t.Logf("- %#v", diag) + t.Logf("- %#v", diag.Description().Summary) } return true } diff --git a/internal/initwd/testdata/invalid-registry-modules/.gitignore b/internal/initwd/testdata/invalid-registry-modules/.gitignore new file mode 100644 index 0000000000..6e0db03a8b --- /dev/null +++ b/internal/initwd/testdata/invalid-registry-modules/.gitignore @@ -0,0 +1 @@ +.terraform/* diff --git a/internal/initwd/testdata/invalid-registry-modules/root.tf b/internal/initwd/testdata/invalid-registry-modules/root.tf new file mode 100644 index 0000000000..856910f45d --- /dev/null +++ b/internal/initwd/testdata/invalid-registry-modules/root.tf @@ -0,0 +1,28 @@ +# This fixture indirectly depends on a github repo at: +# https://github.com/hashicorp/terraform-aws-module-installer-acctest +# ...and expects its v0.0.1 tag to be pointing at the following commit: +# d676ab2559d4e0621d59e3c3c4cbb33958ac4608 +# +# This repository is accessed indirectly via: +# https://registry.terraform.io/modules/hashicorp/module-installer-acctest/aws/0.0.1 +# +# Since the tag's id is included in a downloaded archive, it is expected to +# have the following id: +# 853d03855b3290a3ca491d4c3a7684572dd42237 +# (this particular assumption is encoded in the tests that use this fixture) + + +variable "v" { + description = "in local caller for registry-modules" + default = "" +} + +// see ../registry-modules/root.tf for more info, and for valid usages to the +// acceptance test module + +// this sub module is not available in the registry, we should see an error +// message in the output +module "acctest_child_c" { + source = "hashicorp/module-installer-acctest/aws//modules/child_c" + version = "0.0.1" +}