configs/configupgrade: detect possible relative module sources (#20646)

* configs/configupgrade: detect possible relative module sources

If a module source appears to be a relative local path but does not have
a preceding ./, print a #TODO message for the user.

* internal/initwd: limit go-getter detectors to those supported by terraform
* internal/initwd: move isMaybeRelativeLocalPath check into getWithGoGetter

To avoid making two calls to getter.Detect, which potentially makes
non-trivial API calls, the "isMaybeRelativeLocalPath" check was moved to
a later step and a custom error type was added so user-friendly
diagnostics could be displayed in the event that a possible relative local
path was detected.

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)

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

configs/configupgrade/test-fixtures/valid/module-source/input/main.tf

@@ -0,0 +1,3 @@
+module "foo" {
+  source = "foo"
+}

configs/configupgrade/test-fixtures/valid/module-source/want/main.tf

@@ -0,0 +1,3 @@
+module "foo" {
+  source = "foo"
+}

configs/configupgrade/test-fixtures/valid/module-source/want/versions.tf

@@ -0,0 +1,3 @@
+terraform {
+  required_version = ">= 0.12"
+}

configs/configupgrade/test-fixtures/valid/relative-module-source/input/main.tf

@@ -0,0 +1,3 @@
+module "foo" {
+  source = "module"
+}

configs/configupgrade/test-fixtures/valid/relative-module-source/input/module/main.tf

@@ -0,0 +1,3 @@
+output "foo" {
+  value = "hello"
+}

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

configs/configupgrade/test-fixtures/valid/relative-module-source/want/module/main.tf

@@ -0,0 +1,3 @@
+output "foo" {
+  value = "hello"
+}

configs/configupgrade/test-fixtures/valid/relative-module-source/want/versions.tf

@@ -0,0 +1,3 @@
+terraform {
+  required_version = ">= 0.12"
+}

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

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

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

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())
 	}

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 {
+type MaybeRelativePathErr struct {
-	realAddr, err := getter.Detect(addr, path, getter.Detectors)
+	Addr string
-	// this error will be handled by the next function
+}
-	if err != nil {
+
-		return false
+func (e *MaybeRelativePathErr) Error() string {
-	} else {
+	return fmt.Sprintf("Terraform cannot determine the module source for %s", e.Addr)
-		if strings.HasPrefix(realAddr, "file://") {
+}
-			_, err := os.Stat(realAddr[7:])
+
-			if err != nil {
+func isMaybeRelativeLocalPath(addr string) bool {
-				return true
+	if strings.HasPrefix(addr, "file://") {
-			}
+		_, err := os.Stat(addr[7:])
+		if err != nil {
+			return true
 		}
 	}
 	return false

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
+		if err, ok := err.(*MaybeRelativePathErr); ok {
-		// end-user error messages, but for now we're accepting that because
+			log.Printf(
-		// we have no way to recognize any specific errors to improve them
+				"[TRACE] ModuleInstaller: %s looks like a local path but is missing ./ or ../",
-		// and masking the error entirely would hide valuable diagnostic
+				req.SourceAddr,
-		// information from the user.
+			)
-		diags = diags.Append(tfdiags.Sourceless(
+			diags = diags.Append(tfdiags.Sourceless(
-			tfdiags.Error,
+				tfdiags.Error,
-			"Failed to download module",
+				"Module not found",
-			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),
+				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)

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