Refactoring of module source addresses and module installation

It's been a long while since we gave close attention to the codepaths for
module source address parsing and external module package installation.
Due to their age, these codepaths often diverged from our modern practices
such as representing address types in the addrs package, and encapsulating
package installation details only in a particular location.

In particular, this refactor makes source address parsing a separate step
from module installation, which therefore makes the result of that parsing
available to other Terraform subsystems which work with the configuration
representation objects.

This also presented the opportunity to better encapsulate our use of
go-getter into a new package "getmodules" (echoing "getproviders"), which
is intended to be the only part of Terraform that directly interacts with
go-getter.

This is largely just a refactor of the existing functionality into a new
code organization, but there is one notable change in behavior here: the
source address parsing now happens during configuration loading rather
than module installation, which may cause errors about invalid addresses
to be returned in different situations than before. That counts as
backward compatible because we only promise to remain compatible with
configurations that are _valid_, which means that they can be initialized,
planned, and applied without any errors. This doesn't introduce any new
error cases, and instead just makes a pre-existing error case be detected
earlier.

Our module registry client is still using its own special module address
type from registry/regsrc for now, with a small shim from the new
addrs.ModuleSourceRegistry type. Hopefully in a later commit we'll also
rework the registry client to work with the new address type, but this
commit is already big enough as it is.

internal/addrs/module_source.go

@@ -49,7 +49,14 @@ var moduleSourceLocalPrefixes = []string{
 func ParseModuleSource(raw string) (ModuleSource, error) {
 	for _, prefix := range moduleSourceLocalPrefixes {
 		if strings.HasPrefix(raw, prefix) {
-			return parseModuleSourceLocal(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
 		}
 	}
 
@@ -74,7 +81,14 @@ func ParseModuleSource(raw string) (ModuleSource, error) {
 	// nonsense will probably interpreted as _something_ here
 	// and then fail during installation instead. We can't
 	// really improve this situation for historical reasons.
-	return parseModuleSourceRemote(raw)
+	remoteAddr, err := parseModuleSourceRemote(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 ModuleSourceRemote.
+		return nil, err
+	}
+	return remoteAddr, nil
 }
 
 // ModuleSourceLocal is a ModuleSource representing a local path reference
@@ -145,7 +159,7 @@ func (s ModuleSourceLocal) String() string {
 }
 
 func (s ModuleSourceLocal) ForDisplay() string {
-	return s.String() // the two string representations are identical for this address type
+	return string(s)
 }
 
 // ModuleSourceRegistry is a ModuleSource representing a module listed in a
@@ -387,6 +401,10 @@ func parseModuleSourceRemote(raw string) (ModuleSourceRemote, error) {
 	// aim to remove the network requests over time, if possible.
 	norm, moreSubDir, err := getmodules.NormalizePackageAddress(raw)
 	if err != nil {
+		// We must pass through the returned error directly here because
+		// the getmodules package has some special error types it uses
+		// for certain cases where the UI layer might want to include a
+		// more helpful error message.
 		return ModuleSourceRemote{}, err
 	}
 

internal/addrs/module_source_test.go

@@ -282,13 +282,26 @@ func TestParseModuleSource(t *testing.T) {
 			wantErr: `subdirectory path "../invalid" leads outside of the module package`,
 		},
 
+		"relative path without the needed prefix": {
+			input: "boop/bloop",
+			// For this case we return a generic error message from the addrs
+			// layer, but using a specialized error type which our module
+			// installer checks for and produces an extra hint for users who
+			// were intending to write a local path which then got
+			// misinterpreted as a remote source due to the missing prefix.
+			// However, the main message is generic here because this is really
+			// just a general "this string doesn't match any of our source
+			// address patterns" situation, not _necessarily_ about relative
+			// local paths.
+			wantErr: `Terraform cannot detect a supported external module source type for boop/bloop`,
+		},
+
 		"go-getter will accept all sorts of garbage": {
 			input: "dfgdfgsd:dgfhdfghdfghdfg/dfghdfghdfg",
 			want: ModuleSourceRemote{
 				// Unfortunately go-getter doesn't actually reject a totally
-				// invalid address like this until getting time, so it's
+				// invalid address like this until getting time, as long as
-				// pretty difficult to make remote address parsing actually
+				// it looks somewhat like a URL.
-				// return an error in practice.
 				PackageAddr: ModulePackage("dfgdfgsd:dgfhdfghdfghdfg/dfghdfghdfg"),
 			},
 		},

internal/command/jsonconfig/config.go

@@ -291,7 +291,13 @@ func marshalModuleCall(c *configs.Config, mc *configs.ModuleCall, schemas *terra
 	}
 
 	ret := moduleCall{
-		Source:            mc.SourceAddr,
+		// We're intentionally echoing back exactly what the user entered
+		// here, rather than the normalized version in SourceAddr, because
+		// historically we only _had_ the raw address and thus it would be
+		// a (admittedly minor) breaking change to start normalizing them
+		// now, in case consumers of this data are expecting a particular
+		// non-normalized syntax.
+		Source:            mc.SourceAddrRaw,
 		VersionConstraint: mc.Version.Required.String(),
 	}
 	cExp := marshalExpression(mc.Count)

internal/configs/config.go

@@ -55,10 +55,12 @@ type Config struct {
 	CallRange hcl.Range
 
 	// SourceAddr is the source address that the referenced module was requested
-	// from, as specified in configuration.
+	// from, as specified in configuration. SourceAddrRaw is the same
+	// information, but as the raw string the user originally entered.
 	//
-	// This field is meaningless for the root module, where its contents are undefined.
+	// These fields are meaningless for the root module, where their contents are undefined.
-	SourceAddr string
+	SourceAddr    addrs.ModuleSource
+	SourceAddrRaw string
 
 	// SourceAddrRange is the location in the configuration source where the
 	// SourceAddr value was set, for use in diagnostic messages.
@@ -82,7 +84,7 @@ type Config struct {
 // determine which modules require which providers.
 type ModuleRequirements struct {
 	Name         string
-	SourceAddr   string
+	SourceAddr   addrs.ModuleSource
 	SourceDir    string
 	Requirements getproviders.Requirements
 	Children     map[string]*ModuleRequirements

internal/configs/config_build.go

@@ -145,7 +145,7 @@ type ModuleRequest struct {
 
 	// SourceAddr is the source address string provided by the user in
 	// configuration.
-	SourceAddr string
+	SourceAddr addrs.ModuleSource
 
 	// SourceAddrRange is the source range for the SourceAddr value as it
 	// was provided in configuration. This can and should be used to generate

internal/configs/config_build_test.go

@@ -30,7 +30,7 @@ func TestBuildConfig(t *testing.T) {
 			// SourceAddr as a path relative to our fixture directory.
 			// A "real" implementation of ModuleWalker should accept the
 			// various different source address syntaxes Terraform supports.
-			sourcePath := filepath.Join("testdata/config-build", req.SourceAddr)
+			sourcePath := filepath.Join("testdata/config-build", req.SourceAddr.String())
 
 			mod, diags := parser.LoadConfigDir(sourcePath)
 			version, _ := version.NewVersion(fmt.Sprintf("1.0.%d", versionI))
@@ -86,7 +86,7 @@ func TestBuildConfigDiags(t *testing.T) {
 			// SourceAddr as a path relative to our fixture directory.
 			// A "real" implementation of ModuleWalker should accept the
 			// various different source address syntaxes Terraform supports.
-			sourcePath := filepath.Join("testdata/nested-errors", req.SourceAddr)
+			sourcePath := filepath.Join("testdata/nested-errors", req.SourceAddr.String())
 
 			mod, diags := parser.LoadConfigDir(sourcePath)
 			version, _ := version.NewVersion(fmt.Sprintf("1.0.%d", versionI))
@@ -130,7 +130,7 @@ func TestBuildConfigChildModuleBackend(t *testing.T) {
 			// SourceAddr as a path relative to our fixture directory.
 			// A "real" implementation of ModuleWalker should accept the
 			// various different source address syntaxes Terraform supports.
-			sourcePath := filepath.Join("testdata/nested-backend-warning", req.SourceAddr)
+			sourcePath := filepath.Join("testdata/nested-backend-warning", req.SourceAddr.String())
 
 			mod, diags := parser.LoadConfigDir(sourcePath)
 			version, _ := version.NewVersion("1.0.0")
@@ -206,7 +206,7 @@ func TestBuildConfigInvalidModules(t *testing.T) {
 				func(req *ModuleRequest) (*Module, *version.Version, hcl.Diagnostics) {
 					// for simplicity, these tests will treat all source
 					// addresses as relative to the root module
-					sourcePath := filepath.Join(path, req.SourceAddr)
+					sourcePath := filepath.Join(path, req.SourceAddr.String())
 					mod, diags := parser.LoadConfigDir(sourcePath)
 					version, _ := version.NewVersion("1.0.0")
 					return mod, version, diags

internal/configs/config_test.go

@@ -221,7 +221,7 @@ func TestConfigProviderRequirementsByModule(t *testing.T) {
 	assertNoDiagnostics(t, diags)
 	want := &ModuleRequirements{
 		Name:       "",
-		SourceAddr: "",
+		SourceAddr: nil,
 		SourceDir:  "testdata/provider-reqs",
 		Requirements: getproviders.Requirements{
 			// Only the root module's version is present here
@@ -235,7 +235,7 @@ func TestConfigProviderRequirementsByModule(t *testing.T) {
 		Children: map[string]*ModuleRequirements{
 			"kinder": {
 				Name:       "kinder",
-				SourceAddr: "./child",
+				SourceAddr: addrs.ModuleSourceLocal("./child"),
 				SourceDir:  "testdata/provider-reqs/child",
 				Requirements: getproviders.Requirements{
 					nullProvider:       getproviders.MustParseVersionConstraints("= 2.0.1"),
@@ -244,7 +244,7 @@ func TestConfigProviderRequirementsByModule(t *testing.T) {
 				Children: map[string]*ModuleRequirements{
 					"nested": {
 						Name:       "nested",
-						SourceAddr: "./grandchild",
+						SourceAddr: addrs.ModuleSourceLocal("./grandchild"),
 						SourceDir:  "testdata/provider-reqs/child/grandchild",
 						Requirements: getproviders.Requirements{
 							grandchildProvider: nil,

internal/configs/configload/loader_load.go

@@ -55,7 +55,7 @@ func (l *Loader) moduleWalkerLoad(req *configs.ModuleRequest) (*configs.Module,
 	var diags hcl.Diagnostics
 
 	// Check for inconsistencies between manifest and config
-	if req.SourceAddr != record.SourceAddr {
+	if req.SourceAddr.String() != record.SourceAddr {
 		diags = append(diags, &hcl.Diagnostic{
 			Severity: hcl.DiagError,
 			Summary:  "Module source has changed",

internal/configs/module_call.go

@@ -6,13 +6,16 @@ import (
 	"github.com/hashicorp/hcl/v2"
 	"github.com/hashicorp/hcl/v2/gohcl"
 	"github.com/hashicorp/hcl/v2/hclsyntax"
+	"github.com/hashicorp/terraform/internal/addrs"
+	"github.com/hashicorp/terraform/internal/getmodules"
 )
 
 // ModuleCall represents a "module" block in a module or file.
 type ModuleCall struct {
 	Name string
 
-	SourceAddr      string
+	SourceAddr      addrs.ModuleSource
+	SourceAddrRaw   string
 	SourceAddrRange hcl.Range
 	SourceSet       bool
 
@@ -57,10 +60,41 @@ func decodeModuleBlock(block *hcl.Block, override bool) (*ModuleCall, hcl.Diagno
 	}
 
 	if attr, exists := content.Attributes["source"]; exists {
-		valDiags := gohcl.DecodeExpression(attr.Expr, nil, &mc.SourceAddr)
+		valDiags := gohcl.DecodeExpression(attr.Expr, nil, &mc.SourceAddrRaw)
 		diags = append(diags, valDiags...)
 		mc.SourceAddrRange = attr.Expr.Range()
 		mc.SourceSet = true
+
+		addr, err := addrs.ParseModuleSource(mc.SourceAddrRaw)
+		mc.SourceAddr = addr
+		if err != 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
+			// we'll detect them only during module installation. There are
+			// still a _few_ purely-syntax errors we can catch at parsing time,
+			// though, mostly related to remote package sub-paths and local
+			// paths.
+			switch err := err.(type) {
+			case *getmodules.MaybeRelativePathErr:
+				diags = append(diags, &hcl.Diagnostic{
+					Severity: hcl.DiagError,
+					Summary:  "Invalid module source address",
+					Detail: fmt.Sprintf(
+						"Terraform failed to determine your intended installation method for remote module package %q.\n\nIf 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.",
+						err.Addr, err.Addr,
+					),
+					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 attr, exists := content.Attributes["version"]; exists {

internal/configs/module_call_test.go

@@ -6,6 +6,7 @@ import (
 
 	"github.com/go-test/deep"
 	"github.com/hashicorp/hcl/v2"
+	"github.com/hashicorp/terraform/internal/addrs"
 )
 
 func TestLoadModuleCall(t *testing.T) {
@@ -26,9 +27,10 @@ func TestLoadModuleCall(t *testing.T) {
 	gotModules := file.ModuleCalls
 	wantModules := []*ModuleCall{
 		{
-			Name:       "foo",
+			Name:          "foo",
-			SourceAddr: "./foo",
+			SourceAddr:    addrs.ModuleSourceLocal("./foo"),
-			SourceSet:  true,
+			SourceAddrRaw: "./foo",
+			SourceSet:     true,
 			SourceAddrRange: hcl.Range{
 				Filename: "module-calls.tf",
 				Start:    hcl.Pos{Line: 3, Column: 12, Byte: 27},
@@ -41,9 +43,15 @@ func TestLoadModuleCall(t *testing.T) {
 			},
 		},
 		{
-			Name:       "bar",
+			Name: "bar",
-			SourceAddr: "hashicorp/bar/aws",
+			SourceAddr: addrs.ModuleSourceRegistry{
-			SourceSet:  true,
+				Host:         addrs.DefaultModuleRegistryHost,
+				Namespace:    "hashicorp",
+				Name:         "bar",
+				TargetSystem: "aws",
+			},
+			SourceAddrRaw: "hashicorp/bar/aws",
+			SourceSet:     true,
 			SourceAddrRange: hcl.Range{
 				Filename: "module-calls.tf",
 				Start:    hcl.Pos{Line: 8, Column: 12, Byte: 113},
@@ -56,9 +64,12 @@ func TestLoadModuleCall(t *testing.T) {
 			},
 		},
 		{
-			Name:       "baz",
+			Name: "baz",
-			SourceAddr: "git::https://example.com/",
+			SourceAddr: addrs.ModuleSourceRemote{
-			SourceSet:  true,
+				PackageAddr: addrs.ModulePackage("git::https://example.com/"),
+			},
+			SourceAddrRaw: "git::https://example.com/",
+			SourceSet:     true,
 			SourceAddrRange: hcl.Range{
 				Filename: "module-calls.tf",
 				Start:    hcl.Pos{Line: 15, Column: 12, Byte: 193},

internal/configs/module_merge.go

@@ -150,6 +150,7 @@ func (mc *ModuleCall) merge(omc *ModuleCall) hcl.Diagnostics {
 
 	if omc.SourceSet {
 		mc.SourceAddr = omc.SourceAddr
+		mc.SourceAddrRaw = omc.SourceAddrRaw
 		mc.SourceAddrRange = omc.SourceAddrRange
 		mc.SourceSet = omc.SourceSet
 	}

internal/configs/module_merge_test.go

@@ -81,8 +81,9 @@ func TestModuleOverrideModule(t *testing.T) {
 
 	got := mod.ModuleCalls["example"]
 	want := &ModuleCall{
-		Name:       "example",
+		Name:          "example",
-		SourceAddr: "./example2-a_override",
+		SourceAddr:    addrs.ModuleSourceLocal("./example2-a_override"),
+		SourceAddrRaw: "./example2-a_override",
 		SourceAddrRange: hcl.Range{
 			Filename: "testdata/valid-modules/override-module/a_override.tf",
 			Start: hcl.Pos{

internal/configs/parser_test.go

@@ -87,9 +87,9 @@ func testNestedModuleConfigFromDir(t *testing.T, path string) (*Config, hcl.Diag
 
 			// Build a full path by walking up the module tree, prepending each
 			// source address path until we hit the root
-			paths := []string{req.SourceAddr}
+			paths := []string{req.SourceAddr.String()}
 			for config := req.Parent; config != nil && config.Parent != nil; config = config.Parent {
-				paths = append([]string{config.SourceAddr}, paths...)
+				paths = append([]string{config.SourceAddr.String()}, paths...)
 			}
 			paths = append([]string{path}, paths...)
 			sourcePath := filepath.Join(paths...)

internal/configs/testdata/config-build/child_a/child_a.tf

@@ -1,4 +1,7 @@
 
 module "child_c" {
-  source = "child_c"
+  # In the unit test where this fixture is used, we treat the source strings
+  # as relative paths from the fixture directory rather than as source
+  # addresses as we would in a real module walker.
+  source = "./child_c"
 }

internal/configs/testdata/config-build/child_b/child_b.tf

@@ -1,7 +1,7 @@
 
 module "child_c" {
   # In the unit test where this fixture is used, we treat the source strings
-  # as absolute paths rather than as source addresses as we would in a real
+  # as relative paths from the fixture directory rather than as source
-  # module walker.
+  # addresses as we would in a real module walker.
-  source = "child_c"
+  source = "./child_c"
 }

internal/configs/testdata/config-build/root.tf

@@ -1,9 +1,9 @@
 
 module "child_a" {
-  source = "child_a"
+  source = "./child_a"
 }
 
 module "child_b" {
-  source = "child_b"
+  source = "./child_b"
 }
 

internal/configs/testdata/nested-errors/child_a/child_a.tf

@@ -1,4 +1,7 @@
 
 module "child_c" {
-  source = "child_c"
+  # Note: this test case has an unrealistic module loader that resolves all
+  # sources as relative to the fixture directory, rather than to the
+  # current module directory as Terraform normally would.
+  source = "./child_c"
 }

internal/configs/testdata/nested-errors/root.tf

@@ -1,3 +1,3 @@
 module "child_a" {
-  source = "child_a"
+  source = "./child_a"
 }

internal/earlyconfig/config.go

@@ -56,7 +56,7 @@ type Config struct {
 	// from, as specified in configuration.
 	//
 	// This field is meaningless for the root module, where its contents are undefined.
-	SourceAddr string
+	SourceAddr addrs.ModuleSource
 
 	// Version is the specific version that was selected for this module,
 	// based on version constraints given in configuration.

internal/earlyconfig/config_build.go

@@ -56,10 +56,22 @@ func buildChildModules(parent *Config, walker ModuleWalker) (map[string]*Config,
 			}
 		}
 
+		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 we didn't have a valid source address then we can't continue
+			// down the module tree with this one.
+			continue
+		}
+
 		req := ModuleRequest{
 			Name:               call.Name,
 			Path:               path,
-			SourceAddr:         call.Source,
+			SourceAddr:         sourceAddr,
 			VersionConstraints: vc,
 			Parent:             parent,
 			CallPos:            call.Pos,
@@ -80,7 +92,7 @@ func buildChildModules(parent *Config, walker ModuleWalker) (map[string]*Config,
 			Path:       path,
 			Module:     mod,
 			CallPos:    call.Pos,
-			SourceAddr: call.Source,
+			SourceAddr: sourceAddr,
 			Version:    ver,
 		}
 
@@ -111,7 +123,7 @@ type ModuleRequest struct {
 
 	// SourceAddr is the source address string provided by the user in
 	// configuration.
-	SourceAddr string
+	SourceAddr addrs.ModuleSource
 
 	// VersionConstraint is the version constraint applied to the module in
 	// configuration.

internal/earlyconfig/config_test.go

@@ -75,7 +75,7 @@ func testConfig(t *testing.T, baseDir string) *Config {
 // location information from the call.
 func testModuleWalkerFunc(req *ModuleRequest) (*tfconfig.Module, *version.Version, tfdiags.Diagnostics) {
 	callFilename := req.CallPos.Filename
-	sourcePath := req.SourceAddr
+	sourcePath := req.SourceAddr.String()
 	finalPath := filepath.Join(filepath.Dir(callFilename), sourcePath)
 	log.Printf("[TRACE] %s in %s -> %s", sourcePath, callFilename, finalPath)
 

internal/getmodules/file_detector.go

@@ -0,0 +1,65 @@
+package getmodules
+
+import (
+	"fmt"
+	"path/filepath"
+	"runtime"
+)
+
+// fileDetector is a replacement for go-getter's own file detector which
+// better meets Terraform's needs: specifically, it rejects relative filesystem
+// paths with a somewhat-decent error message.
+//
+// This is a replacement for some historical hackery we did where we tried to
+// avoid calling into go-getter altogether in this situation. This is,
+// therefore, a copy of getter.FileDetector but with the "not absolute path"
+// case replaced with a similar result as Terraform's old heuristic would've
+// returned: a custom error type that the caller can react to in order to
+// produce a hint error message if desired.
+type fileDetector struct{}
+
+func (d *fileDetector) Detect(src, pwd string) (string, bool, error) {
+	if len(src) == 0 {
+		return "", false, nil
+	}
+
+	if !filepath.IsAbs(src) {
+		return "", true, &MaybeRelativePathErr{src}
+	}
+
+	return fmtFileURL(src), true, nil
+}
+
+func fmtFileURL(path string) string {
+	if runtime.GOOS == "windows" {
+		// Make sure we're using "/" on Windows. URLs are "/"-based.
+		path = filepath.ToSlash(path)
+		return fmt.Sprintf("file://%s", path)
+	}
+
+	// Make sure that we don't start with "/" since we add that below.
+	if path[0] == '/' {
+		path = path[1:]
+	}
+	return fmt.Sprintf("file:///%s", path)
+}
+
+// MaybeRelativePathErr is the error type returned by NormalizePackageAddress
+// if the source address looks like it might be intended to be a relative
+// filesystem path but without the required "./" or "../" prefix.
+//
+// Specifically, NormalizePackageAddress will return a pointer to this type,
+// so the error type will be *MaybeRelativePathErr.
+//
+// It has a name starting with "Maybe" because in practice we can get here
+// with any string that isn't recognized as one of the supported schemes:
+// treating the address as a local filesystem path is our fallback for when
+// everything else fails, but it could just as easily be a typo in an attempt
+// to use one of the other schemes and thus not a filesystem path at all.
+type MaybeRelativePathErr struct {
+	Addr string
+}
+
+func (e *MaybeRelativePathErr) Error() string {
+	return fmt.Sprintf("Terraform cannot detect a supported external module source type for %s", e.Addr)
+}

internal/getmodules/getter.go

@@ -48,7 +48,7 @@ var goGetterDetectors = []getter.Detector{
 
 	new(getter.GCSDetector),
 	new(getter.S3Detector),
-	new(getter.FileDetector),
+	new(fileDetector),
 }
 
 var goGetterNoDetectors = []getter.Detector{}

internal/getmodules/installer.go

@@ -17,6 +17,12 @@ type PackageFetcher struct {
 	getter reusingGetter
 }
 
+func NewPackageFetcher() *PackageFetcher {
+	return &PackageFetcher{
+		getter: reusingGetter{},
+	}
+}
+
 // FetchPackage downloads or otherwise retrieves the filesystem inside the
 // package at the given address into the given local installation directory.
 //

internal/getmodules/package.go

@@ -35,6 +35,14 @@ import (
 // detect whether to use Git or Mercurial, because earlier versions of
 // BitBucket used to support both.
 func NormalizePackageAddress(given string) (packageAddr, subDir string, err error) {
+	// Because we're passing go-getter no base directory here, the file
+	// detector will return an error if the user entered a relative filesystem
+	// path without a "../" or "./" prefix and thus ended up in here.
+	//
+	// go-getter's error message for that case is very poor, and so we'll
+	// try to heuristically detect that situation and return a better error
+	// message.
+
 	// NOTE: We're passing an empty string to the "current working directory"
 	// here because that's only relevant for relative filesystem paths,
 	// but Terraform handles relative filesystem paths itself outside of

internal/initwd/from_module.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/hashicorp/terraform/internal/copy"
 	"github.com/hashicorp/terraform/internal/earlyconfig"
+	"github.com/hashicorp/terraform/internal/getmodules"
 
 	version "github.com/hashicorp/go-version"
 	"github.com/hashicorp/terraform-config-inspect/tfconfig"
@@ -142,8 +143,8 @@ func DirFromModule(rootDir, modulesDir, sourceAddr string, reg *registry.Client,
 	wrapHooks := installHooksInitDir{
 		Wrapped: hooks,
 	}
-	getter := reusingGetter{}
+	fetcher := getmodules.NewPackageFetcher()
-	_, instDiags := inst.installDescendentModules(fakeRootModule, rootDir, instManifest, true, wrapHooks, getter)
+	_, instDiags := inst.installDescendentModules(fakeRootModule, rootDir, instManifest, true, wrapHooks, fetcher)
 	diags = append(diags, instDiags...)
 	if instDiags.HasErrors() {
 		return diags
@@ -193,7 +194,7 @@ func DirFromModule(rootDir, modulesDir, sourceAddr string, reg *registry.Client,
 			if mod != nil {
 				for _, mc := range mod.ModuleCalls {
 					if pathTraversesUp(mc.Source) {
-						packageAddr, givenSubdir := splitAddrSubdir(sourceAddr)
+						packageAddr, givenSubdir := getmodules.SplitPackageSubdir(sourceAddr)
 						newSubdir := filepath.Join(givenSubdir, mc.Source)
 						if pathTraversesUp(newSubdir) {
 							// This should never happen in any reasonable

internal/initwd/from_module_test.go

@@ -8,6 +8,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
 	version "github.com/hashicorp/go-version"
 	"github.com/hashicorp/terraform/internal/configs"
 	"github.com/hashicorp/terraform/internal/configs/configload"
@@ -57,7 +58,7 @@ func TestDirFromModule_registry(t *testing.T) {
 		{
 			Name:        "Download",
 			ModuleAddr:  "root",
-			PackageAddr: "hashicorp/module-installer-acctest/aws",
+			PackageAddr: "registry.terraform.io/hashicorp/module-installer-acctest/aws",
 			Version:     v,
 		},
 		{
@@ -78,8 +79,8 @@ func TestDirFromModule_registry(t *testing.T) {
 		},
 	}
 
-	if assertResultDeepEqual(t, hooks.Calls, wantCalls) {
+	if diff := cmp.Diff(wantCalls, hooks.Calls); diff != "" {
-		return
+		t.Fatalf("wrong installer calls\n%s", diff)
 	}
 
 	loader, err := configload.NewLoader(&configload.Config{

internal/initwd/getter.go

@@ -1,214 +0,0 @@
-package initwd
-
-import (
-	"fmt"
-	"log"
-	"os"
-	"path/filepath"
-	"strings"
-
-	cleanhttp "github.com/hashicorp/go-cleanhttp"
-	getter "github.com/hashicorp/go-getter"
-	"github.com/hashicorp/terraform/internal/copy"
-	"github.com/hashicorp/terraform/internal/registry/regsrc"
-)
-
-// We configure our own go-getter detector and getter sets here, because
-// the set of sources we support is part of Terraform's documentation and
-// so we don't want any new sources introduced in go-getter to sneak in here
-// and work even though they aren't documented. This also insulates us from
-// any meddling that might be done by other go-getter callers linked into our
-// executable.
-
-var goGetterDetectors = []getter.Detector{
-	new(getter.GitHubDetector),
-	new(getter.GitDetector),
-	new(getter.BitBucketDetector),
-	new(getter.GCSDetector),
-	new(getter.S3Detector),
-	new(getter.FileDetector),
-}
-
-var goGetterNoDetectors = []getter.Detector{}
-
-var goGetterDecompressors = map[string]getter.Decompressor{
-	"bz2": new(getter.Bzip2Decompressor),
-	"gz":  new(getter.GzipDecompressor),
-	"xz":  new(getter.XzDecompressor),
-	"zip": new(getter.ZipDecompressor),
-
-	"tar.bz2":  new(getter.TarBzip2Decompressor),
-	"tar.tbz2": new(getter.TarBzip2Decompressor),
-
-	"tar.gz": new(getter.TarGzipDecompressor),
-	"tgz":    new(getter.TarGzipDecompressor),
-
-	"tar.xz": new(getter.TarXzDecompressor),
-	"txz":    new(getter.TarXzDecompressor),
-}
-
-var goGetterGetters = map[string]getter.Getter{
-	"file":  new(getter.FileGetter),
-	"gcs":   new(getter.GCSGetter),
-	"git":   new(getter.GitGetter),
-	"hg":    new(getter.HgGetter),
-	"s3":    new(getter.S3Getter),
-	"http":  getterHTTPGetter,
-	"https": getterHTTPGetter,
-}
-
-var getterHTTPClient = cleanhttp.DefaultClient()
-
-var getterHTTPGetter = &getter.HttpGetter{
-	Client: getterHTTPClient,
-	Netrc:  true,
-}
-
-// A reusingGetter is a helper for the module installer that remembers
-// the final resolved addresses of all of the sources it has already been
-// asked to install, and will copy from a prior installation directory if
-// it has the same resolved source address.
-//
-// The keys in a reusingGetter are resolved and trimmed source addresses
-// (with a scheme always present, and without any "subdir" component),
-// and the values are the paths where each source was previously installed.
-type reusingGetter map[string]string
-
-// getWithGoGetter retrieves the package referenced in the given address
-// into the installation path and then returns the full path to any subdir
-// indicated in the address.
-//
-// The errors returned by this function are those surfaced by the underlying
-// go-getter library, which have very inconsistent quality as
-// end-user-actionable error messages. At this time we do not have any
-// reasonable way to improve these error messages at this layer because
-// the underlying errors are not separately recognizable.
-func (g reusingGetter) getWithGoGetter(instPath, addr string) (string, error) {
-	packageAddr, subDir := splitAddrSubdir(addr)
-
-	log.Printf("[DEBUG] will download %q to %s", packageAddr, instPath)
-
-	realAddr, err := getter.Detect(packageAddr, instPath, goGetterDetectors)
-	if err != nil {
-		return "", err
-	}
-
-	if isMaybeRelativeLocalPath(realAddr) {
-		return "", &MaybeRelativePathErr{addr}
-	}
-
-	var realSubDir string
-	realAddr, realSubDir = splitAddrSubdir(realAddr)
-	if realSubDir != "" {
-		subDir = filepath.Join(realSubDir, subDir)
-	}
-
-	if realAddr != packageAddr {
-		log.Printf("[TRACE] go-getter detectors rewrote %q to %q", packageAddr, realAddr)
-	}
-
-	if prevDir, exists := g[realAddr]; exists {
-		log.Printf("[TRACE] copying previous install %s to %s", prevDir, instPath)
-		err := os.Mkdir(instPath, os.ModePerm)
-		if err != nil {
-			return "", fmt.Errorf("failed to create directory %s: %s", instPath, err)
-		}
-		err = copy.CopyDir(instPath, prevDir)
-		if err != nil {
-			return "", fmt.Errorf("failed to copy from %s to %s: %s", prevDir, instPath, err)
-		}
-	} else {
-		log.Printf("[TRACE] fetching %q to %q", realAddr, instPath)
-		client := getter.Client{
-			Src: realAddr,
-			Dst: instPath,
-			Pwd: instPath,
-
-			Mode: getter.ClientModeDir,
-
-			Detectors:     goGetterNoDetectors, // we already did detection above
-			Decompressors: goGetterDecompressors,
-			Getters:       goGetterGetters,
-		}
-		err = client.Get()
-		if err != nil {
-			return "", err
-		}
-		// Remember where we installed this so we might reuse this directory
-		// on subsequent calls to avoid re-downloading.
-		g[realAddr] = instPath
-	}
-
-	// Our subDir string can contain wildcards until this point, so that
-	// e.g. a subDir of * can expand to one top-level directory in a .tar.gz
-	// archive. Now that we've expanded the archive successfully we must
-	// resolve that into a concrete path.
-	var finalDir string
-	if subDir != "" {
-		finalDir, err = getter.SubdirGlob(instPath, subDir)
-		log.Printf("[TRACE] expanded %q to %q", subDir, finalDir)
-		if err != nil {
-			return "", err
-		}
-	} else {
-		finalDir = instPath
-	}
-
-	// If we got this far then we have apparently succeeded in downloading
-	// the requested object!
-	return filepath.Clean(finalDir), nil
-}
-
-// splitAddrSubdir splits the given address (which is assumed to be a
-// registry address or go-getter-style address) into a package portion
-// and a sub-directory portion.
-//
-// The package portion defines what should be downloaded and then the
-// sub-directory portion, if present, specifies a sub-directory within
-// the downloaded object (an archive, VCS repository, etc) that contains
-// the module's configuration files.
-//
-// The subDir portion will be returned as empty if no subdir separator
-// ("//") is present in the address.
-func splitAddrSubdir(addr string) (packageAddr, subDir string) {
-	return getter.SourceDirSubdir(addr)
-}
-
-var localSourcePrefixes = []string{
-	"./",
-	"../",
-	".\\",
-	"..\\",
-}
-
-func isLocalSourceAddr(addr string) bool {
-	for _, prefix := range localSourcePrefixes {
-		if strings.HasPrefix(addr, prefix) {
-			return true
-		}
-	}
-	return false
-}
-
-func isRegistrySourceAddr(addr string) bool {
-	_, err := regsrc.ParseModuleSource(addr)
-	return err == nil
-}
-
-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
-}

internal/initwd/module_install.go

@@ -12,6 +12,7 @@ import (
 	"github.com/hashicorp/terraform-config-inspect/tfconfig"
 	"github.com/hashicorp/terraform/internal/addrs"
 	"github.com/hashicorp/terraform/internal/earlyconfig"
+	"github.com/hashicorp/terraform/internal/getmodules"
 	"github.com/hashicorp/terraform/internal/modsdir"
 	"github.com/hashicorp/terraform/internal/registry"
 	"github.com/hashicorp/terraform/internal/registry/regsrc"
@@ -28,8 +29,8 @@ type ModuleInstaller struct {
 	moduleVersions map[string]*response.ModuleVersions
 
 	// The keys in moduleVersionsUrl are the moduleVersion struct below and
-	// addresses and the values are the download URLs.
+	// addresses and the values are underlying remote source addresses.
-	moduleVersionsUrl map[moduleVersion]string
+	moduleVersionsUrl map[moduleVersion]addrs.ModuleSourceRemote
 }
 
 type moduleVersion struct {
@@ -42,7 +43,7 @@ func NewModuleInstaller(modsDir string, reg *registry.Client) *ModuleInstaller {
 		modsDir:           modsDir,
 		reg:               reg,
 		moduleVersions:    make(map[string]*response.ModuleVersions),
-		moduleVersionsUrl: make(map[moduleVersion]string),
+		moduleVersionsUrl: make(map[moduleVersion]addrs.ModuleSourceRemote),
 	}
 }
 
@@ -92,14 +93,14 @@ func (i *ModuleInstaller) InstallModules(rootDir string, upgrade bool, hooks Mod
 		return nil, diags
 	}
 
-	getter := reusingGetter{}
+	fetcher := getmodules.NewPackageFetcher()
-	cfg, instDiags := i.installDescendentModules(rootMod, rootDir, manifest, upgrade, hooks, getter)
+	cfg, instDiags := i.installDescendentModules(rootMod, rootDir, manifest, upgrade, hooks, fetcher)
 	diags = append(diags, instDiags...)
 
 	return cfg, diags
 }
 
-func (i *ModuleInstaller) installDescendentModules(rootMod *tfconfig.Module, rootDir string, manifest modsdir.Manifest, upgrade bool, hooks ModuleInstallHooks, getter reusingGetter) (*earlyconfig.Config, tfdiags.Diagnostics) {
+func (i *ModuleInstaller) installDescendentModules(rootMod *tfconfig.Module, rootDir string, manifest modsdir.Manifest, upgrade bool, hooks ModuleInstallHooks, fetcher *getmodules.PackageFetcher) (*earlyconfig.Config, tfdiags.Diagnostics) {
 	var diags tfdiags.Diagnostics
 
 	if hooks == nil {
@@ -131,7 +132,7 @@ func (i *ModuleInstaller) installDescendentModules(rootMod *tfconfig.Module, roo
 				case !recorded:
 					log.Printf("[TRACE] ModuleInstaller: %s is not yet installed", key)
 					replace = true
-				case record.SourceAddr != req.SourceAddr:
+				case record.SourceAddr != req.SourceAddr.String():
 					log.Printf("[TRACE] ModuleInstaller: %s source address has changed from %q to %q", key, record.SourceAddr, req.SourceAddr)
 					replace = true
 				case record.Version != nil && !req.VersionConstraints.Check(record.Version):
@@ -196,33 +197,32 @@ func (i *ModuleInstaller) installDescendentModules(rootMod *tfconfig.Module, roo
 			// If we get down here then it's finally time to actually install
 			// the module. There are some variants to this process depending
 			// on what type of module source address we have.
-			switch {
 
-			case isLocalSourceAddr(req.SourceAddr):
+			switch addr := req.SourceAddr.(type) {
-				log.Printf("[TRACE] ModuleInstaller: %s has local path %q", key, req.SourceAddr)
+
+			case addrs.ModuleSourceLocal:
+				log.Printf("[TRACE] ModuleInstaller: %s has local path %q", key, addr.String())
 				mod, mDiags := i.installLocalModule(req, key, manifest, hooks)
 				mDiags = maybeImproveLocalInstallError(req, mDiags)
 				diags = append(diags, mDiags...)
 				return mod, nil, diags
 
-			case isRegistrySourceAddr(req.SourceAddr):
+			case addrs.ModuleSourceRegistry:
-				addr, err := regsrc.ParseModuleSource(req.SourceAddr)
+				log.Printf("[TRACE] ModuleInstaller: %s is a registry module at %s", key, addr.String())
-				if err != nil {
+				mod, v, mDiags := i.installRegistryModule(req, key, instPath, addr, manifest, hooks, fetcher)
-					// Should never happen because isRegistrySourceAddr already validated
-					panic(err)
-				}
-				log.Printf("[TRACE] ModuleInstaller: %s is a registry module at %s", key, addr)
-
-				mod, v, mDiags := i.installRegistryModule(req, key, instPath, addr, manifest, hooks, getter)
 				diags = append(diags, mDiags...)
 				return mod, v, diags
 
-			default:
+			case addrs.ModuleSourceRemote:
-				log.Printf("[TRACE] ModuleInstaller: %s address %q will be handled by go-getter", key, req.SourceAddr)
+				log.Printf("[TRACE] ModuleInstaller: %s address %q will be handled by go-getter", key, addr.String())
-
+				mod, mDiags := i.installGoGetterModule(req, key, instPath, manifest, hooks, fetcher)
-				mod, mDiags := i.installGoGetterModule(req, key, instPath, manifest, hooks, getter)
 				diags = append(diags, mDiags...)
 				return mod, nil, diags
+
+			default:
+				// Shouldn't get here, because there are no other implementations
+				// of addrs.ModuleSource.
+				panic(fmt.Sprintf("unsupported module source address %#v", addr))
 			}
 
 		},
@@ -262,7 +262,7 @@ func (i *ModuleInstaller) installLocalModule(req *earlyconfig.ModuleRequest, key
 	// For local sources we don't actually need to modify the
 	// filesystem at all because the parent already wrote
 	// the files we need, and so we just load up what's already here.
-	newDir := filepath.Join(parentRecord.Dir, req.SourceAddr)
+	newDir := filepath.Join(parentRecord.Dir, req.SourceAddr.String())
 
 	log.Printf("[TRACE] ModuleInstaller: %s uses directory from parent: %s", key, newDir)
 	// it is possible that the local directory is a symlink
@@ -293,7 +293,7 @@ func (i *ModuleInstaller) installLocalModule(req *earlyconfig.ModuleRequest, key
 	manifest[key] = modsdir.Record{
 		Key:        key,
 		Dir:        newDir,
-		SourceAddr: req.SourceAddr,
+		SourceAddr: req.SourceAddr.String(),
 	}
 	log.Printf("[DEBUG] Module installer: %s installed at %s", key, newDir)
 	hooks.Install(key, nil, newDir)
@@ -301,41 +301,31 @@ func (i *ModuleInstaller) installLocalModule(req *earlyconfig.ModuleRequest, key
 	return mod, diags
 }
 
-func (i *ModuleInstaller) installRegistryModule(req *earlyconfig.ModuleRequest, key string, instPath string, addr *regsrc.Module, manifest modsdir.Manifest, hooks ModuleInstallHooks, getter reusingGetter) (*tfconfig.Module, *version.Version, tfdiags.Diagnostics) {
+func (i *ModuleInstaller) installRegistryModule(req *earlyconfig.ModuleRequest, key string, instPath string, addr addrs.ModuleSourceRegistry, manifest modsdir.Manifest, hooks ModuleInstallHooks, fetcher *getmodules.PackageFetcher) (*tfconfig.Module, *version.Version, tfdiags.Diagnostics) {
 	var diags tfdiags.Diagnostics
 
-	hostname, err := addr.SvcHost()
+	hostname := addr.Host
-	if err != nil {
-		// If it looks like the user was trying to use punycode then we'll generate
-		// a specialized error for that case. We require the unicode form of
-		// hostname so that hostnames are always human-readable in configuration
-		// and punycode can't be used to hide a malicious module hostname.
-		if strings.HasPrefix(addr.RawHost.Raw, "xn--") {
-			diags = diags.Append(tfdiags.Sourceless(
-				tfdiags.Error,
-				"Invalid module registry hostname",
-				fmt.Sprintf("The hostname portion of the module %q source address (at %s:%d) is not an acceptable hostname. Internationalized domain names must be given in unicode form rather than ASCII (\"punycode\") form.", req.Name, req.CallPos.Filename, req.CallPos.Line),
-			))
-		} else {
-			diags = diags.Append(tfdiags.Sourceless(
-				tfdiags.Error,
-				"Invalid module registry hostname",
-				fmt.Sprintf("The hostname portion of the module %q source address (at %s:%d) is not a valid hostname.", req.Name, req.CallPos.Filename, req.CallPos.Line),
-			))
-		}
-		return nil, nil, diags
-	}
-
 	reg := i.reg
 	var resp *response.ModuleVersions
 	var exists bool
 
+	// A registry entry isn't _really_ a module package, but we'll pretend it's
+	// one for the sake of this reporting by just trimming off any source
+	// directory.
+	packageAddr := addr // shallow copy
+	packageAddr.Subdir = ""
+
+	// Our registry client is still using the legacy model of addresses, so
+	// we'll shim it here for now.
+	regsrcAddr := regsrc.ModuleFromModuleSourceAddr(packageAddr)
+
 	// check if we've already looked up this module from the registry
-	if resp, exists = i.moduleVersions[addr.String()]; exists {
+	if resp, exists = i.moduleVersions[packageAddr.String()]; exists {
 		log.Printf("[TRACE] %s using already found available versions of %s at %s", key, addr, hostname)
 	} else {
+		var err error
 		log.Printf("[DEBUG] %s listing available versions of %s at %s", key, addr, hostname)
-		resp, err = reg.ModuleVersions(addr)
+		resp, err = reg.ModuleVersions(regsrcAddr)
 		if err != nil {
 			if registry.IsModuleNotFound(err) {
 				diags = diags.Append(tfdiags.Sourceless(
@@ -352,7 +342,7 @@ func (i *ModuleInstaller) installRegistryModule(req *earlyconfig.ModuleRequest,
 			}
 			return nil, nil, diags
 		}
-		i.moduleVersions[addr.String()] = resp
+		i.moduleVersions[packageAddr.String()] = resp
 	}
 
 	// The response might contain information about dependencies to allow us
@@ -425,17 +415,16 @@ func (i *ModuleInstaller) installRegistryModule(req *earlyconfig.ModuleRequest,
 	}
 
 	// Report up to the caller that we're about to start downloading.
-	packageAddr, _ := splitAddrSubdir(req.SourceAddr)
+	hooks.Download(key, packageAddr.String(), latestMatch)
-	hooks.Download(key, packageAddr, latestMatch)
 
 	// If we manage to get down here then we've found a suitable version to
 	// install, so we need to ask the registry where we should download it from.
 	// The response to this is a go-getter-style address string.
 
 	// first check the cache for the download URL
-	moduleAddr := moduleVersion{module: addr.String(), version: latestMatch.String()}
+	moduleAddr := moduleVersion{module: packageAddr.String(), version: latestMatch.String()}
 	if _, exists := i.moduleVersionsUrl[moduleAddr]; !exists {
-		url, err := reg.ModuleLocation(addr, latestMatch.String())
+		realAddrRaw, err := reg.ModuleLocation(regsrcAddr, latestMatch.String())
 		if err != nil {
 			log.Printf("[ERROR] %s from %s %s: %s", key, addr, latestMatch, err)
 			diags = diags.Append(tfdiags.Sourceless(
@@ -445,14 +434,37 @@ func (i *ModuleInstaller) installRegistryModule(req *earlyconfig.ModuleRequest,
 			))
 			return nil, nil, diags
 		}
-		i.moduleVersionsUrl[moduleVersion{module: addr.String(), version: latestMatch.String()}] = url
+		realAddr, err := addrs.ParseModuleSource(realAddrRaw)
+		if err != nil {
+			diags = diags.Append(tfdiags.Sourceless(
+				tfdiags.Error,
+				"Invalid package location from module registry",
+				fmt.Sprintf("Module registry %s returned invalid source location %q for %s %s: %s.", hostname, realAddrRaw, addr, latestMatch, err),
+			))
+			return nil, nil, diags
+		}
+		switch realAddr := realAddr.(type) {
+		// Only a remote source address is allowed here: a registry isn't
+		// allowed to return a local path (because it doesn't know what
+		// its being called from) and we also don't allow recursively pointing
+		// at another registry source for simplicity's sake.
+		case addrs.ModuleSourceRemote:
+			i.moduleVersionsUrl[moduleAddr] = realAddr
+		default:
+			diags = diags.Append(tfdiags.Sourceless(
+				tfdiags.Error,
+				"Invalid package location from module registry",
+				fmt.Sprintf("Module registry %s returned invalid source location %q for %s %s: must be a direct remote package address.", hostname, realAddrRaw, addr, latestMatch),
+			))
+			return nil, nil, diags
+		}
 	}
 
 	dlAddr := i.moduleVersionsUrl[moduleAddr]
 
-	log.Printf("[TRACE] ModuleInstaller: %s %s %s is available at %q", key, addr, latestMatch, dlAddr)
+	log.Printf("[TRACE] ModuleInstaller: %s %s %s is available at %q", key, packageAddr, latestMatch, dlAddr.PackageAddr)
 
-	modDir, err := getter.getWithGoGetter(instPath, dlAddr)
+	err := fetcher.FetchPackage(instPath, dlAddr.PackageAddr.String())
 	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
@@ -467,13 +479,14 @@ func (i *ModuleInstaller) installRegistryModule(req *earlyconfig.ModuleRequest,
 		return nil, nil, diags
 	}
 
-	log.Printf("[TRACE] ModuleInstaller: %s %q was downloaded to %s", key, dlAddr, modDir)
+	log.Printf("[TRACE] ModuleInstaller: %s %q was downloaded to %s", key, dlAddr.PackageAddr, instPath)
 
-	if addr.RawSubmodule != "" {
+	// Incorporate any subdir information from the original path into the
-		// Append the user's requested subdirectory to any subdirectory that
+	// address returned by the registry in order to find the final directory
-		// was implied by any of the nested layers we expanded within go-getter.
+	// of the target module.
-		modDir = filepath.Join(modDir, addr.RawSubmodule)
+	finalAddr := dlAddr.FromRegistry(addr)
-	}
+	subDir := filepath.FromSlash(finalAddr.Subdir)
+	modDir := filepath.Join(instPath, subDir)
 
 	log.Printf("[TRACE] ModuleInstaller: %s should now be at %s", key, modDir)
 
@@ -499,7 +512,7 @@ func (i *ModuleInstaller) installRegistryModule(req *earlyconfig.ModuleRequest,
 		Key:        key,
 		Version:    latestMatch,
 		Dir:        modDir,
-		SourceAddr: req.SourceAddr,
+		SourceAddr: req.SourceAddr.String(),
 	}
 	log.Printf("[DEBUG] Module installer: %s installed at %s", key, modDir)
 	hooks.Install(key, latestMatch, modDir)
@@ -507,12 +520,13 @@ func (i *ModuleInstaller) installRegistryModule(req *earlyconfig.ModuleRequest,
 	return mod, latestMatch, diags
 }
 
-func (i *ModuleInstaller) installGoGetterModule(req *earlyconfig.ModuleRequest, key string, instPath string, manifest modsdir.Manifest, hooks ModuleInstallHooks, getter reusingGetter) (*tfconfig.Module, tfdiags.Diagnostics) {
+func (i *ModuleInstaller) installGoGetterModule(req *earlyconfig.ModuleRequest, key string, instPath string, manifest modsdir.Manifest, hooks ModuleInstallHooks, fetcher *getmodules.PackageFetcher) (*tfconfig.Module, tfdiags.Diagnostics) {
 	var diags tfdiags.Diagnostics
 
 	// Report up to the caller that we're about to start downloading.
-	packageAddr, _ := splitAddrSubdir(req.SourceAddr)
+	addr := req.SourceAddr.(addrs.ModuleSourceRemote)
-	hooks.Download(key, packageAddr, nil)
+	packageAddr := addr.PackageAddr
+	hooks.Download(key, packageAddr.String(), nil)
 
 	if len(req.VersionConstraints) != 0 {
 		diags = diags.Append(tfdiags.Sourceless(
@@ -523,9 +537,11 @@ func (i *ModuleInstaller) installGoGetterModule(req *earlyconfig.ModuleRequest,
 		return nil, diags
 	}
 
-	modDir, err := getter.getWithGoGetter(instPath, req.SourceAddr)
+	err := fetcher.FetchPackage(instPath, packageAddr.String())
 	if err != nil {
-		if _, ok := err.(*MaybeRelativePathErr); ok {
+		// go-getter generates a poor error for an invalid relative path, so
+		// we'll detect that case and generate a better one.
+		if _, ok := err.(*getmodules.MaybeRelativePathErr); ok {
 			log.Printf(
 				"[TRACE] ModuleInstaller: %s looks like a local path but is missing ./ or ../",
 				req.SourceAddr,
@@ -554,10 +570,12 @@ func (i *ModuleInstaller) installGoGetterModule(req *earlyconfig.ModuleRequest,
 			))
 		}
 		return nil, diags
-
 	}
 
-	log.Printf("[TRACE] ModuleInstaller: %s %q was downloaded to %s", key, req.SourceAddr, modDir)
+	subDir := filepath.FromSlash(addr.Subdir)
+	modDir := filepath.Join(instPath, subDir)
+
+	log.Printf("[TRACE] ModuleInstaller: %s %q was downloaded to %s", key, addr, modDir)
 
 	mod, mDiags := earlyconfig.LoadModule(modDir)
 	if mod == nil {
@@ -579,7 +597,7 @@ func (i *ModuleInstaller) installGoGetterModule(req *earlyconfig.ModuleRequest,
 	manifest[key] = modsdir.Record{
 		Key:        key,
 		Dir:        modDir,
-		SourceAddr: req.SourceAddr,
+		SourceAddr: req.SourceAddr.String(),
 	}
 	log.Printf("[DEBUG] Module installer: %s installed at %s", key, modDir)
 	hooks.Install(key, nil, modDir)
@@ -627,7 +645,7 @@ func maybeImproveLocalInstallError(req *earlyconfig.ModuleRequest, diags tfdiags
 	// to see if any of the local paths "escaped" the package.
 	type Step struct {
 		Path       addrs.Module
-		SourceAddr string
+		SourceAddr addrs.ModuleSource
 	}
 	var packageDefiner Step
 	var localRefs []Step
@@ -637,13 +655,13 @@ func maybeImproveLocalInstallError(req *earlyconfig.ModuleRequest, diags tfdiags
 	})
 	current := req.Parent // an earlyconfig.Config where Children isn't populated yet
 	for {
-		if current == nil {
+		if current == nil || current.SourceAddr == 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) {
+		if _, ok := current.SourceAddr.(addrs.ModuleSourceLocal); !ok {
 			// We've found the package definer, then!
 			packageDefiner = Step{
 				Path:       current.Path,
@@ -673,9 +691,7 @@ func maybeImproveLocalInstallError(req *earlyconfig.ModuleRequest, diags tfdiags
 	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"
+		rel := step.SourceAddr.String()
-		// 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!
@@ -719,3 +735,18 @@ func maybeImproveLocalInstallError(req *earlyconfig.ModuleRequest, diags tfdiags
 	// echo back what we were given.
 	return diags
 }
+
+func splitAddrSubdir(addr addrs.ModuleSource) (string, string) {
+	switch addr := addr.(type) {
+	case addrs.ModuleSourceRegistry:
+		subDir := addr.Subdir
+		addr.Subdir = ""
+		return addr.String(), subDir
+	case addrs.ModuleSourceRemote:
+		return addr.PackageAddr.String(), addr.Subdir
+	case nil:
+		panic("splitAddrSubdir on nil addrs.ModuleSource")
+	default:
+		return addr.String(), ""
+	}
+}

internal/initwd/module_install_test.go

@@ -10,7 +10,9 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/davecgh/go-spew/spew"
 	"github.com/go-test/deep"
+	"github.com/google/go-cmp/cmp"
 	version "github.com/hashicorp/go-version"
 	"github.com/hashicorp/terraform/internal/configs"
 	"github.com/hashicorp/terraform/internal/configs/configload"
@@ -101,7 +103,7 @@ func TestModuleInstaller_error(t *testing.T) {
 	if !diags.HasErrors() {
 		t.Fatal("expected error")
 	} else {
-		assertDiagnosticSummary(t, diags, "Module not found")
+		assertDiagnosticSummary(t, diags, "Invalid module source address")
 	}
 }
 
@@ -322,7 +324,7 @@ func TestLoaderInstallModules_registry(t *testing.T) {
 		{
 			Name:        "Download",
 			ModuleAddr:  "acctest_child_a",
-			PackageAddr: "hashicorp/module-installer-acctest/aws", // intentionally excludes the subdir because we're downloading the whole package here
+			PackageAddr: "registry.terraform.io/hashicorp/module-installer-acctest/aws", // intentionally excludes the subdir because we're downloading the whole package here
 			Version:     v,
 		},
 		{
@@ -344,7 +346,7 @@ func TestLoaderInstallModules_registry(t *testing.T) {
 		{
 			Name:        "Download",
 			ModuleAddr:  "acctest_child_b",
-			PackageAddr: "hashicorp/module-installer-acctest/aws", // intentionally excludes the subdir because we're downloading the whole package here
+			PackageAddr: "registry.terraform.io/hashicorp/module-installer-acctest/aws", // intentionally excludes the subdir because we're downloading the whole package here
 			Version:     v,
 		},
 		{
@@ -358,7 +360,7 @@ func TestLoaderInstallModules_registry(t *testing.T) {
 		{
 			Name:        "Download",
 			ModuleAddr:  "acctest_root",
-			PackageAddr: "hashicorp/module-installer-acctest/aws",
+			PackageAddr: "registry.terraform.io/hashicorp/module-installer-acctest/aws",
 			Version:     v,
 		},
 		{
@@ -385,16 +387,16 @@ func TestLoaderInstallModules_registry(t *testing.T) {
 		},
 	}
 
-	if assertResultDeepEqual(t, hooks.Calls, wantCalls) {
+	if diff := cmp.Diff(wantCalls, hooks.Calls); diff != "" {
-		return
+		t.Fatalf("wrong installer calls\n%s", diff)
 	}
 
 	//check that the registry reponses were cached
-	if _, ok := inst.moduleVersions["hashicorp/module-installer-acctest/aws"]; !ok {
+	if _, ok := inst.moduleVersions["registry.terraform.io/hashicorp/module-installer-acctest/aws"]; !ok {
-		t.Fatal("module versions cache was not populated")
+		t.Errorf("module versions cache was not populated\ngot: %s\nwant: key hashicorp/module-installer-acctest/aws", spew.Sdump(inst.moduleVersions))
 	}
-	if _, ok := inst.moduleVersionsUrl[moduleVersion{module: "hashicorp/module-installer-acctest/aws", version: "0.0.1"}]; !ok {
+	if _, ok := inst.moduleVersionsUrl[moduleVersion{module: "registry.terraform.io/hashicorp/module-installer-acctest/aws", version: "0.0.1"}]; !ok {
-		t.Fatal("module download url cache was not populated")
+		t.Errorf("module download url cache was not populated\ngot: %s", spew.Sdump(inst.moduleVersionsUrl))
 	}
 
 	loader, err := configload.NewLoader(&configload.Config{
@@ -463,7 +465,7 @@ func TestLoaderInstallModules_goGetter(t *testing.T) {
 		{
 			Name:        "Download",
 			ModuleAddr:  "acctest_child_a",
-			PackageAddr: "github.com/hashicorp/terraform-aws-module-installer-acctest?ref=v0.0.1", // intentionally excludes the subdir because we're downloading the whole repo here
+			PackageAddr: "git::https://github.com/hashicorp/terraform-aws-module-installer-acctest.git?ref=v0.0.1", // intentionally excludes the subdir because we're downloading the whole repo here
 		},
 		{
 			Name:       "Install",
@@ -483,7 +485,7 @@ func TestLoaderInstallModules_goGetter(t *testing.T) {
 		{
 			Name:        "Download",
 			ModuleAddr:  "acctest_child_b",
-			PackageAddr: "github.com/hashicorp/terraform-aws-module-installer-acctest?ref=v0.0.1", // intentionally excludes the subdir because we're downloading the whole package here
+			PackageAddr: "git::https://github.com/hashicorp/terraform-aws-module-installer-acctest.git?ref=v0.0.1", // intentionally excludes the subdir because we're downloading the whole package here
 		},
 		{
 			Name:       "Install",
@@ -495,7 +497,7 @@ func TestLoaderInstallModules_goGetter(t *testing.T) {
 		{
 			Name:        "Download",
 			ModuleAddr:  "acctest_root",
-			PackageAddr: "github.com/hashicorp/terraform-aws-module-installer-acctest?ref=v0.0.1",
+			PackageAddr: "git::https://github.com/hashicorp/terraform-aws-module-installer-acctest.git?ref=v0.0.1",
 		},
 		{
 			Name:       "Install",
@@ -520,8 +522,8 @@ func TestLoaderInstallModules_goGetter(t *testing.T) {
 		},
 	}
 
-	if assertResultDeepEqual(t, hooks.Calls, wantCalls) {
+	if diff := cmp.Diff(wantCalls, hooks.Calls); diff != "" {
-		return
+		t.Fatalf("wrong installer calls\n%s", diff)
 	}
 
 	loader, err := configload.NewLoader(&configload.Config{
@@ -649,7 +651,7 @@ func assertDiagnosticCount(t *testing.T, diags tfdiags.Diagnostics, want int) bo
 	if len(diags) != 0 {
 		t.Errorf("wrong number of diagnostics %d; want %d", len(diags), want)
 		for _, diag := range diags {
-			t.Logf("- %s", diag)
+			t.Logf("- %#v", diag)
 		}
 		return true
 	}
@@ -667,7 +669,7 @@ func assertDiagnosticSummary(t *testing.T, diags tfdiags.Diagnostics, want strin
 
 	t.Errorf("missing diagnostic summary %q", want)
 	for _, diag := range diags {
-		t.Logf("- %s", diag)
+		t.Logf("- %#v", diag)
 	}
 	return true
 }

internal/modsdir/manifest.go

@@ -26,6 +26,9 @@ type Record struct {
 	// This is used only to detect if the source was changed in configuration
 	// since the module was last installed, which means that the installer
 	// must re-install it.
+	//
+	// This should always be the result of calling method String on an
+	// addrs.ModuleSource value, to get a suitably-normalized result.
 	SourceAddr string `json:"Source"`
 
 	// Version is the exact version of the module, which results from parsing
@@ -89,6 +92,20 @@ func ReadManifestSnapshot(r io.Reader) (Manifest, error) {
 			}
 		}
 
+		// Historically we didn't normalize the module source addresses when
+		// writing them into the manifest, and so we'll make a best effort
+		// to normalize them back in on read so that we can just gracefully
+		// upgrade on the next "terraform init".
+		if record.SourceAddr != "" {
+			if addr, err := addrs.ParseModuleSource(record.SourceAddr); err == nil {
+				// This is a best effort sort of thing. If the source
+				// address isn't valid then we'll just leave it as-is
+				// and let another component detect that downstream,
+				// to preserve the old behavior in that case.
+				record.SourceAddr = addr.String()
+			}
+		}
+
 		// Ensure Windows is using the proper modules path format after
 		// reading the modules manifest Dir records
 		record.Dir = filepath.FromSlash(record.Dir)

internal/registry/regsrc/module.go

@@ -6,7 +6,8 @@ import (
 	"regexp"
 	"strings"
 
-	"github.com/hashicorp/terraform-svchost"
+	svchost "github.com/hashicorp/terraform-svchost"
+	"github.com/hashicorp/terraform/internal/addrs"
 )
 
 var (
@@ -89,6 +90,30 @@ func NewModule(host, namespace, name, provider, submodule string) (*Module, erro
 	return m, nil
 }
 
+// ModuleFromModuleSourceAddr is an adapter to automatically transform the
+// modern representation of registry module addresses,
+// addrs.ModuleSourceRegistry, into the legacy representation regsrc.Module.
+//
+// Note that the new-style model always does normalization during parsing and
+// does not preserve the raw user input at all, and so although the fields
+// of regsrc.Module are all called "Raw...", initializing a Module indirectly
+// through an addrs.ModuleSourceRegistry will cause those values to be the
+// normalized ones, not the raw user input.
+//
+// Use this only for temporary shims to call into existing code that still
+// uses regsrc.Module. Eventually all other subsystems should be updated to
+// use addrs.ModuleSourceRegistry instead, and then package regsrc can be
+// removed altogether.
+func ModuleFromModuleSourceAddr(addr addrs.ModuleSourceRegistry) *Module {
+	return &Module{
+		RawHost:      NewFriendlyHost(addr.Host.String()),
+		RawNamespace: addr.Namespace,
+		RawName:      addr.Name,
+		RawProvider:  addr.TargetSystem, // this field was never actually enforced to be a provider address, so now has a more general name
+		RawSubmodule: addr.Subdir,
+	}
+}
+
 // ParseModuleSource attempts to parse source as a Terraform registry module
 // source. If the string is not found to be in a valid format,
 // ErrInvalidModuleSource is returned. Note that this can only be used on