diff --git a/command/012_config_upgrade.go b/command/012_config_upgrade.go index 856c8349a2..4f0dc7c4a2 100644 --- a/command/012_config_upgrade.go +++ b/command/012_config_upgrade.go @@ -150,7 +150,7 @@ command and dealing with them before running this command again. Providers: c.providerResolver(), Provisioners: c.provisionerFactories(), } - newSources, upgradeDiags := upgrader.Upgrade(sources) + newSources, upgradeDiags := upgrader.Upgrade(sources, dir) diags = diags.Append(upgradeDiags) if upgradeDiags.HasErrors() { c.showDiagnostics(diags) diff --git a/configs/configupgrade/analysis.go b/configs/configupgrade/analysis.go index 4b21ea3d88..6928a3d927 100644 --- a/configs/configupgrade/analysis.go +++ b/configs/configupgrade/analysis.go @@ -25,6 +25,7 @@ type analysis struct { ResourceProviderType map[addrs.Resource]string ResourceHasCount map[addrs.Resource]bool VariableTypes map[string]string + ModuleDir string } // analyze processes the configuration files included inside the receiver diff --git a/configs/configupgrade/test-fixtures/valid/module-source/input/main.tf b/configs/configupgrade/test-fixtures/valid/module-source/input/main.tf new file mode 100644 index 0000000000..d111c977c9 --- /dev/null +++ b/configs/configupgrade/test-fixtures/valid/module-source/input/main.tf @@ -0,0 +1,3 @@ +module "foo" { + source = "foo" +} diff --git a/configs/configupgrade/test-fixtures/valid/module-source/want/main.tf b/configs/configupgrade/test-fixtures/valid/module-source/want/main.tf new file mode 100644 index 0000000000..d111c977c9 --- /dev/null +++ b/configs/configupgrade/test-fixtures/valid/module-source/want/main.tf @@ -0,0 +1,3 @@ +module "foo" { + source = "foo" +} diff --git a/configs/configupgrade/test-fixtures/valid/module-source/want/versions.tf b/configs/configupgrade/test-fixtures/valid/module-source/want/versions.tf new file mode 100644 index 0000000000..d9b6f790b9 --- /dev/null +++ b/configs/configupgrade/test-fixtures/valid/module-source/want/versions.tf @@ -0,0 +1,3 @@ +terraform { + required_version = ">= 0.12" +} diff --git a/configs/configupgrade/test-fixtures/valid/relative-module-source/input/main.tf b/configs/configupgrade/test-fixtures/valid/relative-module-source/input/main.tf new file mode 100644 index 0000000000..804345838e --- /dev/null +++ b/configs/configupgrade/test-fixtures/valid/relative-module-source/input/main.tf @@ -0,0 +1,3 @@ +module "foo" { + source = "module" +} diff --git a/configs/configupgrade/test-fixtures/valid/relative-module-source/input/module/main.tf b/configs/configupgrade/test-fixtures/valid/relative-module-source/input/module/main.tf new file mode 100644 index 0000000000..4fa198f174 --- /dev/null +++ b/configs/configupgrade/test-fixtures/valid/relative-module-source/input/module/main.tf @@ -0,0 +1,3 @@ +output "foo" { + value = "hello" +} diff --git a/configs/configupgrade/test-fixtures/valid/relative-module-source/want/main.tf b/configs/configupgrade/test-fixtures/valid/relative-module-source/want/main.tf new file mode 100644 index 0000000000..d5ec98f2b5 --- /dev/null +++ b/configs/configupgrade/test-fixtures/valid/relative-module-source/want/main.tf @@ -0,0 +1,10 @@ +module "foo" { + # TF-UPGRADE-TODO: In Terraform v0.11 and earlier, it was possible to + # reference a relative module source without a preceding ./, but it is no + # longer supported in Terraform v0.12. + # + # If the below module source is indeed a relative local path, add ./ to the + # start of the source string. If that is not the case, then leave it as-is + # and remove this TODO comment. + source = "module" +} diff --git a/configs/configupgrade/test-fixtures/valid/relative-module-source/want/module/main.tf b/configs/configupgrade/test-fixtures/valid/relative-module-source/want/module/main.tf new file mode 100644 index 0000000000..4fa198f174 --- /dev/null +++ b/configs/configupgrade/test-fixtures/valid/relative-module-source/want/module/main.tf @@ -0,0 +1,3 @@ +output "foo" { + value = "hello" +} diff --git a/configs/configupgrade/test-fixtures/valid/relative-module-source/want/versions.tf b/configs/configupgrade/test-fixtures/valid/relative-module-source/want/versions.tf new file mode 100644 index 0000000000..d9b6f790b9 --- /dev/null +++ b/configs/configupgrade/test-fixtures/valid/relative-module-source/want/versions.tf @@ -0,0 +1,3 @@ +terraform { + required_version = ">= 0.12" +} diff --git a/configs/configupgrade/upgrade.go b/configs/configupgrade/upgrade.go index e50a0dcb90..85fd580f9b 100644 --- a/configs/configupgrade/upgrade.go +++ b/configs/configupgrade/upgrade.go @@ -30,11 +30,12 @@ import ( // warnings are also represented as "TF-UPGRADE-TODO:" comments in the // generated source files so that users can visit them all and decide what to // do with them. -func (u *Upgrader) Upgrade(input ModuleSources) (ModuleSources, tfdiags.Diagnostics) { +func (u *Upgrader) Upgrade(input ModuleSources, dir string) (ModuleSources, tfdiags.Diagnostics) { ret := make(ModuleSources) var diags tfdiags.Diagnostics an, err := u.analyze(input) + an.ModuleDir = dir if err != nil { diags = diags.Append(err) return ret, diags diff --git a/configs/configupgrade/upgrade_body.go b/configs/configupgrade/upgrade_body.go index 7e770012a5..cc82e9a9fc 100644 --- a/configs/configupgrade/upgrade_body.go +++ b/configs/configupgrade/upgrade_body.go @@ -3,6 +3,8 @@ package configupgrade import ( "bytes" "fmt" + "os" + "path/filepath" "sort" "strconv" "strings" @@ -14,6 +16,7 @@ import ( "github.com/zclconf/go-cty/cty" "github.com/hashicorp/terraform/configs/configschema" + "github.com/hashicorp/terraform/registry/regsrc" "github.com/hashicorp/terraform/terraform" "github.com/hashicorp/terraform/tfdiags" ) @@ -657,6 +660,55 @@ func connectionBlockRule(filename string, resourceType string, an *analysis, adh } } +func moduleSourceRule(filename string, an *analysis) bodyItemRule { + return func(buf *bytes.Buffer, blockAddr string, item *hcl1ast.ObjectItem) tfdiags.Diagnostics { + var diags tfdiags.Diagnostics + val, ok := item.Val.(*hcl1ast.LiteralType) + if !ok { + diags = diags.Append(&hcl2.Diagnostic{ + Severity: hcl2.DiagError, + Summary: "Invalid source argument", + Detail: `The "source" argument must be a single string containing the module source.`, + Subject: hcl1PosRange(filename, item.Keys[0].Pos()).Ptr(), + }) + return diags + } + if val.Token.Type != hcl1token.STRING { + diags = diags.Append(&hcl2.Diagnostic{ + Severity: hcl2.DiagError, + Summary: "Invalid source argument", + Detail: `The "source" argument must be a single string containing the module source.`, + Subject: hcl1PosRange(filename, item.Keys[0].Pos()).Ptr(), + }) + return diags + } + + litVal := val.Token.Value().(string) + + if isMaybeRelativeLocalPath(litVal, an.ModuleDir) { + diags = diags.Append(&hcl2.Diagnostic{ + Severity: hcl2.DiagWarning, + Summary: "Possible relative module source", + Detail: "Terraform cannot determine the given module source, but it appears to be a relative path", + Subject: hcl1PosRange(filename, item.Keys[0].Pos()).Ptr(), + }) + buf.WriteString( + "# TF-UPGRADE-TODO: In Terraform v0.11 and earlier, it was possible to\n" + + "# reference a relative module source without a preceding ./, but it is no\n" + + "# longer supported in Terraform v0.12.\n" + + "#\n" + + "# If the below module source is indeed a relative local path, add ./ to the\n" + + "# start of the source string. If that is not the case, then leave it as-is\n" + + "# and remove this TODO comment.\n", + ) + } + newVal, exprDiags := upgradeExpr(val, filename, false, an) + diags = diags.Append(exprDiags) + buf.WriteString("source = " + string(newVal) + "\n") + return diags + } +} + // Prior to Terraform 0.12 providers were able to supply default connection // settings that would partially populate the "connection" block with // automatically-selected values. @@ -873,3 +925,42 @@ var resourceTypeAutomaticConnectionExprs = map[string]map[string]string{ )`, }, } + +// copied directly from internal/initwd/getter.go +var localSourcePrefixes = []string{ + "./", + "../", + ".\\", + "..\\", +} + +// isMaybeRelativeLocalPath tries to catch situations where a module source is +// an improperly-referenced relative path, such as "module" instead of +// "./module". This is a simple check that could return a false positive in the +// unlikely-yet-plausible case that a module source is for eg. a github +// repository that also looks exactly like an existing relative path. This +// should only be used to return a warning. +func isMaybeRelativeLocalPath(addr, dir string) bool { + for _, prefix := range localSourcePrefixes { + if strings.HasPrefix(addr, prefix) { + // it is _definitely_ a relative path + return false + } + } + + _, err := regsrc.ParseModuleSource(addr) + if err == nil { + // it is a registry source + return false + } + + possibleRelPath := filepath.Join(dir, addr) + _, err = os.Stat(possibleRelPath) + if err == nil { + // If there is no error, something exists at what would be the relative + // path, if the module source started with ./ + return true + } + + return false +} diff --git a/configs/configupgrade/upgrade_native.go b/configs/configupgrade/upgrade_native.go index 6cfd07d865..89bda85248 100644 --- a/configs/configupgrade/upgrade_native.go +++ b/configs/configupgrade/upgrade_native.go @@ -209,7 +209,7 @@ func (u *Upgrader) upgradeNativeSyntaxFile(filename string, src []byte, an *anal // start with the straightforward mapping of those and override // the special lifecycle arguments below. rules := justAttributesBodyRules(filename, body, an) - rules["source"] = noInterpAttributeRule(filename, cty.String, an) + rules["source"] = moduleSourceRule(filename, an) rules["version"] = noInterpAttributeRule(filename, cty.String, an) rules["providers"] = func(buf *bytes.Buffer, blockAddr string, item *hcl1ast.ObjectItem) tfdiags.Diagnostics { var diags tfdiags.Diagnostics diff --git a/configs/configupgrade/upgrade_test.go b/configs/configupgrade/upgrade_test.go index d76b1c08db..3501e730f2 100644 --- a/configs/configupgrade/upgrade_test.go +++ b/configs/configupgrade/upgrade_test.go @@ -53,7 +53,7 @@ func TestUpgradeValid(t *testing.T) { t.Fatal(err) } - gotSrc, diags := u.Upgrade(inputSrc) + gotSrc, diags := u.Upgrade(inputSrc, inputDir) if diags.HasErrors() { t.Error(diags.Err()) } @@ -101,7 +101,7 @@ func TestUpgradeRenameJSON(t *testing.T) { u := &Upgrader{ Providers: providers.ResolverFixed(testProviders), } - gotSrc, diags := u.Upgrade(inputSrc) + gotSrc, diags := u.Upgrade(inputSrc, inputDir) if diags.HasErrors() { t.Error(diags.Err()) } diff --git a/internal/initwd/getter.go b/internal/initwd/getter.go index 87fe7d82b3..50e2572afd 100644 --- a/internal/initwd/getter.go +++ b/internal/initwd/getter.go @@ -89,6 +89,10 @@ func (g reusingGetter) getWithGoGetter(instPath, addr string) (string, error) { return "", err } + if isMaybeRelativeLocalPath(realAddr) { + return "", &MaybeRelativePathErr{addr} + } + var realSubDir string realAddr, realSubDir = splitAddrSubdir(realAddr) if realSubDir != "" { @@ -187,17 +191,19 @@ func isRegistrySourceAddr(addr string) bool { return err == nil } -func isMaybeRelativeLocalPath(addr, path string) bool { - realAddr, err := getter.Detect(addr, path, getter.Detectors) - // this error will be handled by the next function - if err != nil { - return false - } else { - if strings.HasPrefix(realAddr, "file://") { - _, err := os.Stat(realAddr[7:]) - if err != nil { - return true - } +type MaybeRelativePathErr struct { + Addr string +} + +func (e *MaybeRelativePathErr) Error() string { + return fmt.Sprintf("Terraform cannot determine the module source for %s", e.Addr) +} + +func isMaybeRelativeLocalPath(addr string) bool { + if strings.HasPrefix(addr, "file://") { + _, err := os.Stat(addr[7:]) + if err != nil { + return true } } return false diff --git a/internal/initwd/module_install.go b/internal/initwd/module_install.go index acdb7e0fe4..543405b5b6 100644 --- a/internal/initwd/module_install.go +++ b/internal/initwd/module_install.go @@ -199,22 +199,6 @@ func (i *ModuleInstaller) installDescendentModules(rootMod *tfconfig.Module, roo diags = append(diags, mDiags...) return mod, v, diags - case isMaybeRelativeLocalPath(req.SourceAddr, instPath): - log.Printf( - "[TRACE] ModuleInstaller: %s looks like a local path but is missing ./ or ../", - req.SourceAddr, - ) - diags = diags.Append(tfdiags.Sourceless( - tfdiags.Error, - "Failed to locate local module source", - fmt.Sprintf( - "%s looks like a relative path, but Terraform cannot determine the module source. "+ - "Add ./ at the start of the source string if this is a relative path.", - req.SourceAddr, - ), - )) - return nil, nil, diags - default: log.Printf("[TRACE] ModuleInstaller: %s address %q will be handled by go-getter", key, req.SourceAddr) @@ -487,17 +471,36 @@ func (i *ModuleInstaller) installGoGetterModule(req *earlyconfig.ModuleRequest, modDir, err := getter.getWithGoGetter(instPath, req.SourceAddr) if err != nil { - // Errors returned by go-getter have very inconsistent quality as - // end-user error messages, but for now we're accepting that because - // we have no way to recognize any specific errors to improve them - // and masking the error entirely would hide valuable diagnostic - // information from the user. - diags = diags.Append(tfdiags.Sourceless( - tfdiags.Error, - "Failed to download module", - fmt.Sprintf("Error attempting to download module %q (%s:%d) source code from %q: %s", req.Name, req.CallPos.Filename, req.CallPos.Line, packageAddr, err), - )) + if err, ok := err.(*MaybeRelativePathErr); ok { + log.Printf( + "[TRACE] ModuleInstaller: %s looks like a local path but is missing ./ or ../", + req.SourceAddr, + ) + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Module not found", + fmt.Sprintf( + "The module address %q could not be resolved.\n\n"+ + "If you intended this as a path relative to the current "+ + "module, use \"./%s\" instead. The \"./\" prefix "+ + "indicates that the address is a relative filesystem path.", + req.SourceAddr, req.SourceAddr, + ), + )) + } else { + // Errors returned by go-getter have very inconsistent quality as + // end-user error messages, but for now we're accepting that because + // we have no way to recognize any specific errors to improve them + // and masking the error entirely would hide valuable diagnostic + // information from the user. + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Failed to download module", + fmt.Sprintf("Error attempting to download module %q (%s:%d) source code from %q: %s", req.Name, req.CallPos.Filename, req.CallPos.Line, packageAddr, err), + )) + } return nil, diags + } log.Printf("[TRACE] ModuleInstaller: %s %q was downloaded to %s", key, req.SourceAddr, modDir) diff --git a/internal/initwd/module_install_test.go b/internal/initwd/module_install_test.go index 9fc952f367..eb2e877c73 100644 --- a/internal/initwd/module_install_test.go +++ b/internal/initwd/module_install_test.go @@ -107,7 +107,7 @@ func TestModuleInstaller_error(t *testing.T) { if !diags.HasErrors() { t.Fatal("expected error") } else { - assertDiagnosticSummary(t, diags, "Failed to locate local module source") + assertDiagnosticSummary(t, diags, "Module not found") } }