addrs: Expose the registry address parser's error messages

Previously we ended up losing all of the error message detail produced by
the registry address parser, because we treated any registry address
failure as cause to parse the address as a go-getter-style remote address
instead.

That led to terrible feedback in the situation where the user _was_
trying to write a module address but it was invalid in some way.

Although we can't really tighten this up in the default case due to our
compatibility promises, it's never been valid to use the "version"
argument with anything other than a registry address and so as a
compromise here we'll use the presence of "version" as a heuristic for
user intent to parse the source address as a registry address, and thus
we can return a registry-address-specific error message in that case and
thus give more direct feedback about what was wrong.

This unfortunately won't help someone trying to install from the registry
_without_ a version constraint, but I didn't want to let perfect be the
enemy of the good here, particularly since we recommend using version
constraints with registry modules anyway; indeed, that's one of the main
benefits of using a registry rather than a remote source directly.

Author: apparentlymart

internal/addrs/module_source.go

@@ -46,18 +46,36 @@ var moduleSourceLocalPrefixes = []string{
 	"..\\",
 }
 
+// ParseModuleSource parses a module source address as given in the "source"
+// argument inside a "module" block in the configuration.
+//
+// For historical reasons this syntax is a bit overloaded, supporting three
+// different address types:
+//   - Local paths starting with either ./ or ../, which are special because
+//     Terraform considers them to belong to the same "package" as the caller.
+//   - Module registry addresses, given as either NAMESPACE/NAME/SYSTEM or
+//     HOST/NAMESPACE/NAME/SYSTEM, in which case the remote registry serves
+//     as an indirection over the third address type that follows.
+//   - Various URL-like and other heuristically-recognized strings which
+//     we currently delegate to the external library go-getter.
+//
+// There is some ambiguity between the module registry addresses and go-getter's
+// very liberal heuristics and so this particular function will typically treat
+// an invalid registry address as some other sort of remote source address
+// rather than returning an error. If you know that you're expecting a
+// registry address in particular, use ParseModuleSourceRegistry instead, which
+// can therefore expose more detailed error messages about registry address
+// parsing in particular.
 func ParseModuleSource(raw string) (ModuleSource, error) {
-	for _, prefix := range moduleSourceLocalPrefixes {
-		if strings.HasPrefix(raw, prefix) {
-			localAddr, err := parseModuleSourceLocal(raw)
-			if err != nil {
-				// This is to make sure we really return a nil ModuleSource in
-				// this case, rather than an interface containing the zero
-				// value of ModuleSourceLocal.
-				return nil, err
-			}
-			return localAddr, nil
+	if isModuleSourceLocal(raw) {
+		localAddr, err := parseModuleSourceLocal(raw)
+		if err != nil {
+			// This is to make sure we really return a nil ModuleSource in
+			// this case, rather than an interface containing the zero
+			// value of ModuleSourceLocal.
+			return nil, err
 		}
+		return localAddr, nil
 	}
 
 	// For historical reasons, whether an address is a registry
@@ -71,7 +89,7 @@ func ParseModuleSource(raw string) (ModuleSource, error) {
 	// the registry source parse error gets returned to the caller,
 	// which is annoying but has been true for many releases
 	// without it posing a serious problem in practice.)
-	if ret, err := parseModuleSourceRegistry(raw); err == nil {
+	if ret, err := ParseModuleSourceRegistry(raw); err == nil {
 		return ret, nil
 	}
 
@@ -150,6 +168,15 @@ func parseModuleSourceLocal(raw string) (ModuleSourceLocal, error) {
 	return ModuleSourceLocal(clean), nil
 }
 
+func isModuleSourceLocal(raw string) bool {
+	for _, prefix := range moduleSourceLocalPrefixes {
+		if strings.HasPrefix(raw, prefix) {
+			return true
+		}
+	}
+	return false
+}
+
 func (s ModuleSourceLocal) moduleSource() {}
 
 func (s ModuleSourceLocal) String() string {
@@ -195,6 +222,30 @@ const DefaultModuleRegistryHost = svchost.Hostname("registry.terraform.io")
 var moduleRegistryNamePattern = regexp.MustCompile("^[0-9A-Za-z](?:[0-9A-Za-z-_]{0,62}[0-9A-Za-z])?$")
 var moduleRegistryTargetSystemPattern = regexp.MustCompile("^[0-9a-z]{1,64}$")
 
+// ParseModuleSourceRegistry is a variant of ParseModuleSource which only
+// accepts module registry addresses, and will reject any other address type.
+//
+// Use this instead of ParseModuleSource if you know from some other surrounding
+// context that an address is intended to be a registry address rather than
+// some other address type, which will then allow for better error reporting
+// due to the additional information about user intent.
+func ParseModuleSourceRegistry(raw string) (ModuleSource, error) {
+	// Before we delegate to the "real" function we'll just make sure this
+	// doesn't look like a local source address, so we can return a better
+	// error message for that situation.
+	if isModuleSourceLocal(raw) {
+		return ModuleSourceRegistry{}, fmt.Errorf("can't use local directory %q as a module registry address", raw)
+	}
+
+	ret, err := parseModuleSourceRegistry(raw)
+	if err != nil {
+		// This is to make sure we return a nil ModuleSource, rather than
+		// a non-nil ModuleSource containing a zero-value ModuleSourceRegistry.
+		return nil, err
+	}
+	return ret, nil
+}
+
 func parseModuleSourceRegistry(raw string) (ModuleSourceRegistry, error) {
 	var err error
 
@@ -298,11 +349,10 @@ func parseModuleRegistryTargetSystem(given string) (string, error) {
 	// Similar to the names in provider source addresses, we defined these
 	// to be compatible with what filesystems and typical remote systems
 	// like GitHub allow in names. Unfortunately we didn't end up defining
-	// these exactly equivalently: provider names can only use dashes as
-	// punctuation, whereas module names can use underscores. So here we're
-	// using some regular expressions from the original module source
-	// implementation, rather than using the IDNA rules as we do in
-	// ParseProviderPart.
+	// these exactly equivalently: provider names can't use dashes or
+	// underscores. So here we're using some regular expressions from the
+	// original module source implementation, rather than using the IDNA rules
+	// as we do in ParseProviderPart.
 
 	if !moduleRegistryTargetSystemPattern.MatchString(given) {
 		return "", fmt.Errorf("must be between one and 64 ASCII letters or digits")

internal/addrs/module_source_test.go

@@ -488,10 +488,14 @@ func TestParseModuleSourceRegistry(t *testing.T) {
 			input:   `foo/var/baz/qux`,
 			wantErr: `invalid module registry hostname: must contain at least one dot`,
 		},
-		"invalid target system": {
+		"invalid target system characters": {
 			input:   `foo/var/no-no-no`,
 			wantErr: `invalid target system "no-no-no": must be between one and 64 ASCII letters or digits`,
 		},
+		"invalid target system length": {
+			input:   `foo/var/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaah`,
+			wantErr: `invalid target system "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaah": must be between one and 64 ASCII letters or digits`,
+		},
 		"invalid namespace": {
 			input:   `boop!/var/baz`,
 			wantErr: `invalid namespace "boop!": must be between one and 64 characters, including ASCII letters, digits, dashes, and underscores, where dashes and underscores may not be the prefix or suffix`,
@@ -518,11 +522,23 @@ func TestParseModuleSourceRegistry(t *testing.T) {
 			input:   `bitbucket.org/HashiCorp/Consul/aws`,
 			wantErr: `can't use "bitbucket.org" as a module registry host, because it's reserved for installing directly from version control repositories`,
 		},
+		"local path from current dir": {
+			// Can't use a local path when we're specifically trying to parse
+			// a _registry_ source address.
+			input:   `./boop`,
+			wantErr: `can't use local directory "./boop" as a module registry address`,
+		},
+		"local path from parent dir": {
+			// Can't use a local path when we're specifically trying to parse
+			// a _registry_ source address.
+			input:   `../boop`,
+			wantErr: `can't use local directory "../boop" as a module registry address`,
+		},
 	}
 
 	for name, test := range tests {
 		t.Run(name, func(t *testing.T) {
-			addr, err := parseModuleSourceRegistry(test.input)
+			addrI, err := ParseModuleSourceRegistry(test.input)
 
 			if test.wantErr != "" {
 				switch {
@@ -538,6 +554,11 @@ func TestParseModuleSourceRegistry(t *testing.T) {
 				t.Fatalf("unexpected error: %s", err.Error())
 			}
 
+			addr, ok := addrI.(ModuleSourceRegistry)
+			if !ok {
+				t.Fatalf("wrong address type %T; want %T", addrI, addr)
+			}
+
 			if got, want := addr.String(), test.wantString; got != want {
 				t.Errorf("wrong String() result\ngot:  %s\nwant: %s", got, want)
 			}

internal/configs/module_call.go

@@ -59,15 +59,35 @@ func decodeModuleBlock(block *hcl.Block, override bool) (*ModuleCall, hcl.Diagno
 		})
 	}
 
+	haveVersionArg := false
+	if attr, exists := content.Attributes["version"]; exists {
+		var versionDiags hcl.Diagnostics
+		mc.Version, versionDiags = decodeVersionConstraint(attr)
+		diags = append(diags, versionDiags...)
+		haveVersionArg = true
+	}
+
 	if attr, exists := content.Attributes["source"]; exists {
 		mc.SourceSet = true
 		mc.SourceAddrRange = attr.Expr.Range()
 		valDiags := gohcl.DecodeExpression(attr.Expr, nil, &mc.SourceAddrRaw)
 		diags = append(diags, valDiags...)
 		if !valDiags.HasErrors() {
-			addr, err := addrs.ParseModuleSource(mc.SourceAddrRaw)
+			var addr addrs.ModuleSource
+			var err error
+			if haveVersionArg {
+				addr, err = addrs.ParseModuleSourceRegistry(mc.SourceAddrRaw)
+			} else {
+				addr, err = addrs.ParseModuleSource(mc.SourceAddrRaw)
+			}
 			mc.SourceAddr = addr
 			if err != nil {
+				// NOTE: We leave mc.SourceAddr as nil for any situation where the
+				// source attribute is invalid, so any code which tries to carefully
+				// use the partial result of a failed config decode must be
+				// resilient to that.
+				mc.SourceAddr = nil
+
 				// NOTE: In practice it's actually very unlikely to end up here,
 				// because our source address parser can turn just about any string
 				// into some sort of remote package address, and so for most errors
@@ -87,25 +107,27 @@ func decodeModuleBlock(block *hcl.Block, override bool) (*ModuleCall, hcl.Diagno
 						Subject: mc.SourceAddrRange.Ptr(),
 					})
 				default:
-					diags = append(diags, &hcl.Diagnostic{
-						Severity: hcl.DiagError,
-						Summary:  "Invalid module source address",
-						Detail:   fmt.Sprintf("Failed to parse module source address: %s.", err),
-						Subject:  mc.SourceAddrRange.Ptr(),
-					})
+					if haveVersionArg {
+						// In this case we'll include some extra context that
+						// we assumed a registry source address due to the
+						// version argument.
+						diags = append(diags, &hcl.Diagnostic{
+							Severity: hcl.DiagError,
+							Summary:  "Invalid registry module source address",
+							Detail:   fmt.Sprintf("Failed to parse module registry address: %s.\n\nTerraform assumed that you intended a module registry source address because you also set the argument \"version\", which applies only to registry modules.", err),
+							Subject:  mc.SourceAddrRange.Ptr(),
+						})
+					} else {
+						diags = append(diags, &hcl.Diagnostic{
+							Severity: hcl.DiagError,
+							Summary:  "Invalid module source address",
+							Detail:   fmt.Sprintf("Failed to parse module source address: %s.", err),
+							Subject:  mc.SourceAddrRange.Ptr(),
+						})
+					}
 				}
 			}
 		}
-		// NOTE: We leave mc.SourceAddr as nil for any situation where the
-		// source attribute is invalid, so any code which tries to carefully
-		// use the partial result of a failed config decode must be
-		// resilient to that.
-	}
-
-	if attr, exists := content.Attributes["version"]; exists {
-		var versionDiags hcl.Diagnostics
-		mc.Version, versionDiags = decodeVersionConstraint(attr)
-		diags = append(diags, versionDiags...)
 	}
 
 	if attr, exists := content.Attributes["count"]; exists {

internal/configs/testdata/error-files/module-invalid-registry-source-with-module.tf

@@ -0,0 +1,5 @@
+
+module "test" {
+  source  = "---.com/HashiCorp/Consul/aws" # ERROR: Invalid registry module source address
+  version = "1.0.0" # Makes Terraform assume "source" is a module address
+}

internal/configs/testdata/error-files/module-local-source-with-version.tf

@@ -0,0 +1,5 @@
+
+module "test" {
+  source  = "../boop" # ERROR: Invalid registry module source address
+  version = "1.0.0" # Makes Terraform assume "source" is a module address
+}

internal/earlyconfig/config_build.go

@@ -43,7 +43,10 @@ func buildChildModules(parent *Config, walker ModuleWalker) (map[string]*Config,
 		path[len(path)-1] = call.Name
 
 		var vc version.Constraints
+		haveVersionArg := false
 		if strings.TrimSpace(call.Version) != "" {
+			haveVersionArg = true
+
 			var err error
 			vc, err = version.NewConstraint(call.Version)
 			if err != nil {
@@ -56,13 +59,27 @@ func buildChildModules(parent *Config, walker ModuleWalker) (map[string]*Config,
 			}
 		}
 
-		sourceAddr, err := addrs.ParseModuleSource(call.Source)
+		var sourceAddr addrs.ModuleSource
+		var err error
+		if haveVersionArg {
+			sourceAddr, err = addrs.ParseModuleSourceRegistry(call.Source)
+		} else {
+			sourceAddr, err = addrs.ParseModuleSource(call.Source)
+		}
 		if err != nil {
-			diags = diags.Append(wrapDiagnostic(tfconfig.Diagnostic{
-				Severity: tfconfig.DiagError,
-				Summary:  "Invalid module source address",
-				Detail:   fmt.Sprintf("Module %q (declared at %s line %d) has invalid source address %q: %s.", callName, call.Pos.Filename, call.Pos.Line, call.Source, err),
-			}))
+			if haveVersionArg {
+				diags = diags.Append(wrapDiagnostic(tfconfig.Diagnostic{
+					Severity: tfconfig.DiagError,
+					Summary:  "Invalid registry module source address",
+					Detail:   fmt.Sprintf("Module %q (declared at %s line %d) has invalid source address %q: %s.\n\nTerraform assumed that you intended a module registry source address because you also set the argument \"version\", which applies only to registry modules.", callName, call.Pos.Filename, call.Pos.Line, call.Source, err),
+				}))
+			} else {
+				diags = diags.Append(wrapDiagnostic(tfconfig.Diagnostic{
+					Severity: tfconfig.DiagError,
+					Summary:  "Invalid module source address",
+					Detail:   fmt.Sprintf("Module %q (declared at %s line %d) has invalid source address %q: %s.", callName, call.Pos.Filename, call.Pos.Line, call.Source, err),
+				}))
+			}
 			// If we didn't have a valid source address then we can't continue
 			// down the module tree with this one.
 			continue

internal/initwd/module_install.go

@@ -547,7 +547,7 @@ func (i *ModuleInstaller) installGoGetterModule(ctx context.Context, req *earlyc
 		diags = diags.Append(tfdiags.Sourceless(
 			tfdiags.Error,
 			"Invalid version constraint",
-			fmt.Sprintf("Cannot apply a version constraint to module %q (at %s:%d) because it has a non Registry URL.", req.Name, req.CallPos.Filename, req.CallPos.Line),
+			fmt.Sprintf("Cannot apply a version constraint to module %q (at %s:%d) because it doesn't come from a module registry.", req.Name, req.CallPos.Filename, req.CallPos.Line),
 		))
 		return nil, diags
 	}

internal/initwd/module_install_test.go

@@ -192,7 +192,12 @@ func TestModuleInstaller_invalid_version_constraint_error(t *testing.T) {
 	if !diags.HasErrors() {
 		t.Fatal("expected error")
 	} else {
-		assertDiagnosticSummary(t, diags, "Invalid version constraint")
+		// We use the presence of the "version" argument as a heuristic for
+		// user intent to use a registry module, and so we intentionally catch
+		// this as an invalid registry module address rather than an invalid
+		// version constraint, so we can surface the specific address parsing
+		// error instead of a generic version constraint error.
+		assertDiagnosticSummary(t, diags, "Invalid registry module source address")
 	}
 }
 
@@ -210,7 +215,12 @@ func TestModuleInstaller_invalidVersionConstraintGetter(t *testing.T) {
 	if !diags.HasErrors() {
 		t.Fatal("expected error")
 	} else {
-		assertDiagnosticSummary(t, diags, "Invalid version constraint")
+		// We use the presence of the "version" argument as a heuristic for
+		// user intent to use a registry module, and so we intentionally catch
+		// this as an invalid registry module address rather than an invalid
+		// version constraint, so we can surface the specific address parsing
+		// error instead of a generic version constraint error.
+		assertDiagnosticSummary(t, diags, "Invalid registry module source address")
 	}
 }
 
@@ -228,7 +238,12 @@ func TestModuleInstaller_invalidVersionConstraintLocal(t *testing.T) {
 	if !diags.HasErrors() {
 		t.Fatal("expected error")
 	} else {
-		assertDiagnosticSummary(t, diags, "Invalid version constraint")
+		// We use the presence of the "version" argument as a heuristic for
+		// user intent to use a registry module, and so we intentionally catch
+		// this as an invalid registry module address rather than an invalid
+		// version constraint, so we can surface the specific address parsing
+		// error instead of a generic version constraint error.
+		assertDiagnosticSummary(t, diags, "Invalid registry module source address")
 	}
 }