From 1da126365c8ea6cc31ae1166f6168af6ff20c9ca Mon Sep 17 00:00:00 2001 From: James Humphries Date: Wed, 27 Aug 2025 11:06:38 +0100 Subject: [PATCH] Improved missing subpath detection in module installation (#3184) Signed-off-by: James Humphries Signed-off-by: James Humphries Co-authored-by: Andrei Ciobanu --- internal/initwd/module_install.go | 53 ++++++++++++----- internal/initwd/module_install_test.go | 81 ++++++++++++++++++++++++-- 2 files changed, 115 insertions(+), 19 deletions(-) diff --git a/internal/initwd/module_install.go b/internal/initwd/module_install.go index d8bad2988a..b2dddcb26d 100644 --- a/internal/initwd/module_install.go +++ b/internal/initwd/module_install.go @@ -85,24 +85,44 @@ func NewModuleInstaller(modsDir string, loader *configload.Loader, registryClien } } -// isSubDirNonExistant checks if the error is due to a non-existent subdirectory +// isSubDirNonExistent checks if the error is due to a non-existent subdirectory // within an otherwise valid module. This helps distinguish between a genuine // OpenTofu bug when failing to get the module and a user configuration error where they've specified a // submodule path that doesn't exist. -func isSubDirNonExistant(modDir string) bool { - parent := filepath.Dir(modDir) - if parent == modDir || parent == "." || parent == "/" { - return false +func isSubDirNonExistent(modDir string) (isNonExistent bool, missingDir string) { + modDir = filepath.Clean(modDir) + + _, err := os.Stat(modDir) + // If the directory exists, this isn't a missing subdir case + if err == nil { + return false, "" + } + // If it's not a "does not exist" error, return false (unexpected error) + if !os.IsNotExist(err) { + return false, "" } - // Check if parent directory exists but the subdir doesn't - if _, err := os.Stat(parent); err == nil { - if _, err := os.Stat(modDir); os.IsNotExist(err) { - return true + var missingParts []string + current := modDir + + for { + parent := filepath.Dir(current) + if parent == current { + return false, "" // reached root without finding it } - } - return false + missingParts = append([]string{filepath.Base(current)}, missingParts...) + + info, err := os.Stat(parent) + if err == nil && info.IsDir() { // return the missing parts! + if len(missingParts) > 0 { + return true, missingParts[0] + } + return true, filepath.Base(current) + } + + current = parent + } } // InstallModules analyses the root module in the given directory and installs @@ -816,15 +836,17 @@ 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, req.Call) if mod == nil { + + isMissingSubDir, missingDir := isSubDirNonExistent(modDir) // nil indicates missing or unreadable directory, so we'll // discard the returned diags and return a more specific // error message here. - if subDir != "" && isSubDirNonExistant(modDir) { + if subDir != "" && isMissingSubDir { // This may be a user error, or a submodule may have been removed between unpinned versions of the module (ie a module update) diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "Module subdirectory not found", - Detail: fmt.Sprintf("The subdirectory %q does not exist in module %q. Please verify that the subdirectory path is correct.", subDir, instPath), + Detail: fmt.Sprintf("Cannot find directory %q in module %q. The requested subdirectory was %q.", missingDir, instPath, subDir), Subject: req.CallRange.Ptr(), }) } else { @@ -941,12 +963,13 @@ func (i *ModuleInstaller) installGoGetterModule(ctx context.Context, req *config // nil indicates missing or unreadable directory, so we'll // discard the returned diags and return a more specific // error message here. - if addr.Subdir != "" && isSubDirNonExistant(modDir) { + isNonExistent, missingDir := isSubDirNonExistent(modDir) + if addr.Subdir != "" && isNonExistent { // This is a user configuration error - they referenced a submodule that doesn't exist diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "Module subdirectory not found", - Detail: fmt.Sprintf("The subdirectory %q does not exist in the downloaded module. Verify that the subdirectory path is correct.", addr.Subdir), + Detail: fmt.Sprintf("Cannot find directory %q in the module path %q. The requested subdirectory was %q.", missingDir, instPath, addr.Subdir), Subject: req.CallRange.Ptr(), }) } else { diff --git a/internal/initwd/module_install_test.go b/internal/initwd/module_install_test.go index cc060d18a0..a0c12d0bbb 100644 --- a/internal/initwd/module_install_test.go +++ b/internal/initwd/module_install_test.go @@ -167,7 +167,7 @@ func TestModuleInstaller_packageEscapeError(t *testing.T) { t.Fatal(err) } final := bytes.ReplaceAll(template, []byte("%%BASE%%"), []byte(filepath.ToSlash(dir))) - err = os.WriteFile(rootFilename, final, 0644) + err = os.WriteFile(rootFilename, final, 0o644) if err != nil { t.Fatal(err) } @@ -207,7 +207,7 @@ func TestModuleInstaller_explicitPackageBoundary(t *testing.T) { t.Fatal(err) } final := bytes.ReplaceAll(template, []byte("%%BASE%%"), []byte(filepath.ToSlash(dir))) - err = os.WriteFile(rootFilename, final, 0644) + err = os.WriteFile(rootFilename, final, 0o644) if err != nil { t.Fatal(err) } @@ -619,7 +619,6 @@ func TestLoaderInstallModules_registry(t *testing.T) { gotTraces[path] = varDesc }) assertResultDeepEqual(t, gotTraces, wantTraces) - } func TestLoaderInstallModules_goGetter(t *testing.T) { @@ -747,7 +746,6 @@ func TestLoaderInstallModules_goGetter(t *testing.T) { gotTraces[path] = varDesc }) assertResultDeepEqual(t, gotTraces, wantTraces) - } func TestModuleInstaller_fromTests(t *testing.T) { @@ -898,6 +896,81 @@ func TestLoadInstallModules_registryFromTest(t *testing.T) { } } +func TestIsSubDirNonExistent(t *testing.T) { + tmpDir := t.TempDir() + + // Create test directory structure + existingDir := filepath.Join(tmpDir, "existing") + nestedExistingDir := filepath.Join(existingDir, "nested", "deep") + err := os.MkdirAll(nestedExistingDir, 0o755) + if err != nil { + t.Fatal(err) + } + + tests := []struct { + name string + path string + wantExists bool + wantMissingDir string + }{ + { + name: "directory exists", + path: existingDir, + wantExists: false, + wantMissingDir: "", + }, + { + name: "nested directory exists", + path: nestedExistingDir, + wantExists: false, + wantMissingDir: "", + }, + { + name: "single missing directory with existing parent", + path: filepath.Join(existingDir, "missing"), + wantExists: true, + wantMissingDir: "missing", + }, + { + name: "nested missing directory", + path: filepath.Join(existingDir, "missing", "nested"), + wantExists: true, + wantMissingDir: "missing", + }, + { + name: "deeply nested missing with existing parent", + path: filepath.Join(nestedExistingDir, "missing"), + wantExists: true, + wantMissingDir: "missing", + }, + { + name: "multiple missing levels", + path: filepath.Join(existingDir, "missing", "also", "missingtoo"), + wantExists: true, + wantMissingDir: "missing", + }, + { + name: "completely non-existent path", + path: filepath.Join(tmpDir, "completely", "missing", "path"), + wantExists: true, + wantMissingDir: "completely", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotExists, gotMissingDir := isSubDirNonExistent(tt.path) + + if gotExists != tt.wantExists { + t.Errorf("isSubDirNonExistent() exists = %v, want %v", gotExists, tt.wantExists) + } + if gotMissingDir != tt.wantMissingDir { + t.Errorf("isSubDirNonExistent() missingDir = %q, want %q", gotMissingDir, tt.wantMissingDir) + } + }) + } +} + // TestModuleInstaller_nonExistentSubmodule ensures that the error message returned when a module does not exist in an existing module // is not that it's a bug in OpenTofu. See issue https://github.com/opentofu/opentofu/issues/3142 for more information //