diff --git a/internal/initwd/module_install.go b/internal/initwd/module_install.go index 5b9c7110c..d1f3fd9e8 100644 --- a/internal/initwd/module_install.go +++ b/internal/initwd/module_install.go @@ -4,6 +4,7 @@ import ( "fmt" "log" "os" + "path" "path/filepath" "strings" @@ -200,6 +201,7 @@ func (i *ModuleInstaller) installDescendentModules(rootMod *tfconfig.Module, roo case isLocalSourceAddr(req.SourceAddr): log.Printf("[TRACE] ModuleInstaller: %s has local path %q", key, req.SourceAddr) mod, mDiags := i.installLocalModule(req, key, manifest, hooks) + mDiags = maybeImproveLocalInstallError(req, mDiags) diags = append(diags, mDiags...) return mod, nil, diags @@ -588,3 +590,132 @@ func (i *ModuleInstaller) installGoGetterModule(req *earlyconfig.ModuleRequest, func (i *ModuleInstaller) packageInstallPath(modulePath addrs.Module) string { return filepath.Join(i.modsDir, strings.Join(modulePath, ".")) } + +// maybeImproveLocalInstallError is a helper function which can recognize +// some specific situations where it can return a more helpful error message +// and thus replace the given errors with those if so. +// +// If this function can't do anything about a particular situation then it +// will just return the given diags verbatim. +// +// This function's behavior is only reasonable for errors returned from the +// ModuleInstaller.installLocalModule function. +func maybeImproveLocalInstallError(req *earlyconfig.ModuleRequest, diags tfdiags.Diagnostics) tfdiags.Diagnostics { + if !diags.HasErrors() { + return diags + } + + // The main situation we're interested in detecting here is whether the + // current module or any of its ancestors use relative paths that reach + // outside of the "package" established by the nearest non-local ancestor. + // That's never really valid, but unfortunately we historically didn't + // have any explicit checking for it and so now for compatibility in + // situations where things just happened to "work" we treat this as an + // error only in situations where installation would've failed anyway, + // so we can give a better error about it than just a generic + // "directory not found" or whatever. + // + // Since it's never actually valid to relative out of the containing + // package, we just assume that any failed local package install which + // does so was caused by that, because to stop doing it should always + // improve the situation, even if it leads to another error describing + // a different problem. + + // To decide this we need to find the subset of our ancestors that + // belong to the same "package" as our request, along with the closest + // ancestor that defined that package, and then we can work forwards + // to see if any of the local paths "escaped" the package. + type Step struct { + Path addrs.Module + SourceAddr string + } + var packageDefiner Step + var localRefs []Step + localRefs = append(localRefs, Step{ + Path: req.Path, + SourceAddr: req.SourceAddr, + }) + current := req.Parent // an earlyconfig.Config where Children isn't populated yet + for { + if current == nil { + // We've reached the root module, in which case we aren't + // in an external "package" at all and so our special case + // can't apply. + return diags + } + if !isLocalSourceAddr(current.SourceAddr) { + // We've found the package definer, then! + packageDefiner = Step{ + Path: current.Path, + SourceAddr: current.SourceAddr, + } + break + } + + localRefs = append(localRefs, Step{ + Path: current.Path, + SourceAddr: current.SourceAddr, + }) + current = current.Parent + } + // Our localRefs list is reversed because we were traversing up the tree, + // so we'll flip it the other way and thus walk "downwards" through it. + for i, j := 0, len(localRefs)-1; i < j; i, j = i+1, j-1 { + localRefs[i], localRefs[j] = localRefs[j], localRefs[i] + } + + // Our method here is to start with a known base path prefix and + // then apply each of the local refs to it in sequence until one of + // them causes us to "lose" the prefix. If that happens, we've found + // an escape to report. This is not an exact science but good enough + // heuristic for choosing a better error message. + const prefix = "*/" // NOTE: this can find a false negative if the user chooses "*" as a directory name, but we consider that unlikely + packageAddr, startPath := splitAddrSubdir(packageDefiner.SourceAddr) + currentPath := path.Join(prefix, startPath) + for _, step := range localRefs { + // We're working in the simpler space of "path" rather than "filepath" + // for our heuristic here, so we need to slashify Windows-ish paths. + rel := filepath.ToSlash(step.SourceAddr) + + nextPath := path.Join(currentPath, rel) + if !strings.HasPrefix(nextPath, prefix) { // ESCAPED! + escapeeAddr := step.Path.String() + + var newDiags tfdiags.Diagnostics + + // First we'll copy over any non-error diagnostics from the source diags + for _, diag := range diags { + if diag.Severity() != tfdiags.Error { + newDiags = newDiags.Append(diag) + } + } + + // ...but we'll replace any errors with this more precise error. + var suggestion string + if strings.HasPrefix(packageAddr, "/") || filepath.VolumeName(packageAddr) != "" { + // It might be somewhat surprising that Terraform treats + // absolute filesystem paths as "external" even though it + // treats relative paths as local, so if it seems like that's + // what the user was doing then we'll add an additional note + // about it. + suggestion = "\n\nTerraform treats absolute filesystem paths as external modules which establish a new module package. To treat this directory as part of the same package as its caller, use a local path starting with either \"./\" or \"../\"." + } + newDiags = newDiags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Local module path escapes module package", + fmt.Sprintf( + "The given source directory for %s would be outside of its containing package %q. Local source addresses starting with \"../\" must stay within the same package that the calling module belongs to.%s", + escapeeAddr, packageAddr, suggestion, + ), + )) + + return newDiags + } + + currentPath = nextPath + } + + // If we get down here then we have nothing useful to do, so we'll just + // echo back what we were given. + return diags +} diff --git a/internal/initwd/module_install_test.go b/internal/initwd/module_install_test.go index cab2a7007..8e0ec64dc 100644 --- a/internal/initwd/module_install_test.go +++ b/internal/initwd/module_install_test.go @@ -1,6 +1,7 @@ package initwd import ( + "bytes" "flag" "fmt" "io/ioutil" @@ -104,6 +105,74 @@ func TestModuleInstaller_error(t *testing.T) { } } +func TestModuleInstaller_packageEscapeError(t *testing.T) { + fixtureDir := filepath.Clean("testdata/load-module-package-escape") + dir, done := tempChdir(t, fixtureDir) + defer done() + + // For this particular test we need an absolute path in the root module + // that must actually resolve to our temporary directory in "dir", so + // we need to do a little rewriting. We replace the arbitrary placeholder + // %%BASE%% with the temporary directory path. + { + rootFilename := filepath.Join(dir, "package-escape.tf") + template, err := ioutil.ReadFile(rootFilename) + if err != nil { + t.Fatal(err) + } + final := bytes.ReplaceAll(template, []byte("%%BASE%%"), []byte(filepath.ToSlash(dir))) + err = ioutil.WriteFile(rootFilename, final, 0644) + if err != nil { + t.Fatal(err) + } + } + + hooks := &testInstallHooks{} + + modulesDir := filepath.Join(dir, ".terraform/modules") + inst := NewModuleInstaller(modulesDir, nil) + _, diags := inst.InstallModules(".", false, hooks) + + if !diags.HasErrors() { + t.Fatal("expected error") + } else { + assertDiagnosticSummary(t, diags, "Local module path escapes module package") + } +} + +func TestModuleInstaller_explicitPackageBoundary(t *testing.T) { + fixtureDir := filepath.Clean("testdata/load-module-package-prefix") + dir, done := tempChdir(t, fixtureDir) + defer done() + + // For this particular test we need an absolute path in the root module + // that must actually resolve to our temporary directory in "dir", so + // we need to do a little rewriting. We replace the arbitrary placeholder + // %%BASE%% with the temporary directory path. + { + rootFilename := filepath.Join(dir, "package-prefix.tf") + template, err := ioutil.ReadFile(rootFilename) + if err != nil { + t.Fatal(err) + } + final := bytes.ReplaceAll(template, []byte("%%BASE%%"), []byte(filepath.ToSlash(dir))) + err = ioutil.WriteFile(rootFilename, final, 0644) + if err != nil { + t.Fatal(err) + } + } + + hooks := &testInstallHooks{} + + modulesDir := filepath.Join(dir, ".terraform/modules") + inst := NewModuleInstaller(modulesDir, nil) + _, diags := inst.InstallModules(".", false, hooks) + + if diags.HasErrors() { + t.Fatalf("unexpected errors\n%s", diags.Err().Error()) + } +} + func TestModuleInstaller_invalid_version_constraint_error(t *testing.T) { fixtureDir := filepath.Clean("testdata/invalid-version-constraint") dir, done := tempChdir(t, fixtureDir) diff --git a/internal/initwd/testdata/load-module-package-escape/child/package-escape-child.tf b/internal/initwd/testdata/load-module-package-escape/child/package-escape-child.tf new file mode 100644 index 000000000..935a02de1 --- /dev/null +++ b/internal/initwd/testdata/load-module-package-escape/child/package-escape-child.tf @@ -0,0 +1,8 @@ +module "grandchild" { + # NOTE: This seems like it ought to work because there is indeed a + # ../grandchild directory, but our caller loaded us as an external + # module using an absolute path and so we're actually isolated from + # the parent directory in a separate "module package", and so we + # can't traverse out to find the grandchild module. + source = "../grandchild" +} diff --git a/internal/initwd/testdata/load-module-package-escape/grandchild/package-escape-grandchild.tf b/internal/initwd/testdata/load-module-package-escape/grandchild/package-escape-grandchild.tf new file mode 100644 index 000000000..734d0d638 --- /dev/null +++ b/internal/initwd/testdata/load-module-package-escape/grandchild/package-escape-grandchild.tf @@ -0,0 +1,2 @@ +# This is intentionally empty, just here to be referred to by the "child" +# module in ../child . diff --git a/internal/initwd/testdata/load-module-package-escape/package-escape.tf b/internal/initwd/testdata/load-module-package-escape/package-escape.tf new file mode 100644 index 000000000..7a665a4e6 --- /dev/null +++ b/internal/initwd/testdata/load-module-package-escape/package-escape.tf @@ -0,0 +1,9 @@ +module "child" { + # NOTE: For this test we need a working absolute path so that Terraform + # will see this a an "external" module and thus establish a separate + # package for it, but we won't know which temporary directory this + # will be in at runtime, so we'll rewrite this file inside the test + # code to replace %%BASE%% with the actual path. %%BASE%% is not normal + # Terraform syntax and won't work outside of this test. + source = "%%BASE%%/child" +} diff --git a/internal/initwd/testdata/load-module-package-prefix/package-prefix.tf b/internal/initwd/testdata/load-module-package-prefix/package-prefix.tf new file mode 100644 index 000000000..08d5ced60 --- /dev/null +++ b/internal/initwd/testdata/load-module-package-prefix/package-prefix.tf @@ -0,0 +1,15 @@ +module "child" { + # NOTE: For this test we need a working absolute path so that Terraform + # will see this a an "external" module and thus establish a separate + # package for it, but we won't know which temporary directory this + # will be in at runtime, so we'll rewrite this file inside the test + # code to replace %%BASE%% with the actual path. %%BASE%% is not normal + # Terraform syntax and won't work outside of this test. + # + # Note that we're intentionally using the special // delimiter to + # tell Terraform that it should treat the "package" directory as a + # whole as a module package, with all of its descendents "downloaded" + # (copied) together into ./.terraform/modules/child so that child + # can refer to ../grandchild successfully. + source = "%%BASE%%/package//child" +} diff --git a/internal/initwd/testdata/load-module-package-prefix/package/child/package-prefix-child.tf b/internal/initwd/testdata/load-module-package-prefix/package/child/package-prefix-child.tf new file mode 100644 index 000000000..84b67acdf --- /dev/null +++ b/internal/initwd/testdata/load-module-package-prefix/package/child/package-prefix-child.tf @@ -0,0 +1,9 @@ +module "grandchild" { + # NOTE: This only works because our caller told Terraform to treat + # the parent directory as a whole as a module package, and so + # the "./terraform/modules/child" directory should contain both + # "child" and "grandchild" sub directories that we can traverse between. + # This is the same as local paths between different directories inside + # a single git repository or distribution archive. + source = "../grandchild" +} diff --git a/internal/initwd/testdata/load-module-package-prefix/package/grandchild/package-prefix-grandchild.tf b/internal/initwd/testdata/load-module-package-prefix/package/grandchild/package-prefix-grandchild.tf new file mode 100644 index 000000000..734d0d638 --- /dev/null +++ b/internal/initwd/testdata/load-module-package-prefix/package/grandchild/package-prefix-grandchild.tf @@ -0,0 +1,2 @@ +# This is intentionally empty, just here to be referred to by the "child" +# module in ../child . diff --git a/website/docs/language/modules/sources.html.md b/website/docs/language/modules/sources.html.md index a4164571f..94da03653 100644 --- a/website/docs/language/modules/sources.html.md +++ b/website/docs/language/modules/sources.html.md @@ -74,6 +74,15 @@ that other sources are: the files are already present on local disk (possibly as a result of installing a parent module) and so can just be used directly. Their source code is automatically updated if the parent module is upgraded. +Note that Terraform does not consider an _absolute_ filesystem path (starting +with a slash, a drive letter, or similar) to be a local path. Instead, +Terraform will treat that in a similar way as a remote module and copy it into +the local module cache. An absolute path is a "package" in the sense described +in [Modules in Package Sub-directories](#modules-in-package-sub-directories). +We don't recommend using absolute filesystem paths to refer to Terraform +modules, because it will tend to couple your configuration to the filesystem +layout of a particular computer. + ## Terraform Registry A module registry is the native way of distributing Terraform modules for use