init: detect and report invalid submodules (#35839)

Author: liamcervante

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

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
 }

internal/initwd/testdata/invalid-registry-modules/.gitignore

@@ -0,0 +1 @@
+.terraform/*

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"
+}