diff --git a/internal/addrs/module_source.go b/internal/addrs/module_source.go index c532a5f7b..0f2c4aff2 100644 --- a/internal/addrs/module_source.go +++ b/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 } diff --git a/internal/addrs/module_source_test.go b/internal/addrs/module_source_test.go index 1180a7e45..6303ff20c 100644 --- a/internal/addrs/module_source_test.go +++ b/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 - // pretty difficult to make remote address parsing actually - // return an error in practice. + // invalid address like this until getting time, as long as + // it looks somewhat like a URL. PackageAddr: ModulePackage("dfgdfgsd:dgfhdfghdfghdfg/dfghdfghdfg"), }, }, diff --git a/internal/command/jsonconfig/config.go b/internal/command/jsonconfig/config.go index c50c3f184..617181ba6 100644 --- a/internal/command/jsonconfig/config.go +++ b/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) diff --git a/internal/configs/config.go b/internal/configs/config.go index 694b8999f..93632612a 100644 --- a/internal/configs/config.go +++ b/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. - SourceAddr string + // These fields are meaningless for the root module, where their contents are undefined. + 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 diff --git a/internal/configs/config_build.go b/internal/configs/config_build.go index dada18947..e7ebe5021 100644 --- a/internal/configs/config_build.go +++ b/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 diff --git a/internal/configs/config_build_test.go b/internal/configs/config_build_test.go index a977e432b..8a2134b48 100644 --- a/internal/configs/config_build_test.go +++ b/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 diff --git a/internal/configs/config_test.go b/internal/configs/config_test.go index ad5134260..4677f3416 100644 --- a/internal/configs/config_test.go +++ b/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, diff --git a/internal/configs/configload/loader_load.go b/internal/configs/configload/loader_load.go index 2a31a22c2..323001de1 100644 --- a/internal/configs/configload/loader_load.go +++ b/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", diff --git a/internal/configs/module_call.go b/internal/configs/module_call.go index 0733db854..7213bea58 100644 --- a/internal/configs/module_call.go +++ b/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 { diff --git a/internal/configs/module_call_test.go b/internal/configs/module_call_test.go index 79ad89e62..a652bebb6 100644 --- a/internal/configs/module_call_test.go +++ b/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", - SourceAddr: "./foo", - SourceSet: true, + Name: "foo", + SourceAddr: addrs.ModuleSourceLocal("./foo"), + 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", - SourceAddr: "hashicorp/bar/aws", - SourceSet: true, + Name: "bar", + SourceAddr: addrs.ModuleSourceRegistry{ + 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", - SourceAddr: "git::https://example.com/", - SourceSet: true, + Name: "baz", + SourceAddr: addrs.ModuleSourceRemote{ + 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}, diff --git a/internal/configs/module_merge.go b/internal/configs/module_merge.go index bd6ebc457..d9b21cacc 100644 --- a/internal/configs/module_merge.go +++ b/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 } diff --git a/internal/configs/module_merge_test.go b/internal/configs/module_merge_test.go index 5c01a8435..c7db32316 100644 --- a/internal/configs/module_merge_test.go +++ b/internal/configs/module_merge_test.go @@ -81,8 +81,9 @@ func TestModuleOverrideModule(t *testing.T) { got := mod.ModuleCalls["example"] want := &ModuleCall{ - Name: "example", - SourceAddr: "./example2-a_override", + Name: "example", + 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{ diff --git a/internal/configs/parser_test.go b/internal/configs/parser_test.go index cb2239282..4eb558d1c 100644 --- a/internal/configs/parser_test.go +++ b/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...) diff --git a/internal/configs/testdata/config-build/child_a/child_a.tf b/internal/configs/testdata/config-build/child_a/child_a.tf index 6c5759701..d3eff0b49 100644 --- a/internal/configs/testdata/config-build/child_a/child_a.tf +++ b/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" } diff --git a/internal/configs/testdata/config-build/child_b/child_b.tf b/internal/configs/testdata/config-build/child_b/child_b.tf index 1adcb0650..d3eff0b49 100644 --- a/internal/configs/testdata/config-build/child_b/child_b.tf +++ b/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 - # module walker. - source = "child_c" + # as relative paths from the fixture directory rather than as source + # addresses as we would in a real module walker. + source = "./child_c" } diff --git a/internal/configs/testdata/config-build/root.tf b/internal/configs/testdata/config-build/root.tf index 7b3c4a75a..5e23e40a2 100644 --- a/internal/configs/testdata/config-build/root.tf +++ b/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" } diff --git a/internal/configs/testdata/nested-errors/child_a/child_a.tf b/internal/configs/testdata/nested-errors/child_a/child_a.tf index 6c5759701..d2fb89fae 100644 --- a/internal/configs/testdata/nested-errors/child_a/child_a.tf +++ 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" } diff --git a/internal/configs/testdata/nested-errors/root.tf b/internal/configs/testdata/nested-errors/root.tf index c9db02049..d596eb06d 100644 --- a/internal/configs/testdata/nested-errors/root.tf +++ b/internal/configs/testdata/nested-errors/root.tf @@ -1,3 +1,3 @@ module "child_a" { - source = "child_a" + source = "./child_a" } diff --git a/internal/earlyconfig/config.go b/internal/earlyconfig/config.go index 5ad9f6666..86d93c27b 100644 --- a/internal/earlyconfig/config.go +++ b/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. diff --git a/internal/earlyconfig/config_build.go b/internal/earlyconfig/config_build.go index 6e5683383..807d9cd77 100644 --- a/internal/earlyconfig/config_build.go +++ b/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. diff --git a/internal/earlyconfig/config_test.go b/internal/earlyconfig/config_test.go index 40fefb150..21aa71bee 100644 --- a/internal/earlyconfig/config_test.go +++ b/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) diff --git a/internal/getmodules/file_detector.go b/internal/getmodules/file_detector.go new file mode 100644 index 000000000..b28e2cdc9 --- /dev/null +++ b/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) +} diff --git a/internal/getmodules/getter.go b/internal/getmodules/getter.go index 44dc8254b..67b5c2760 100644 --- a/internal/getmodules/getter.go +++ b/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{} diff --git a/internal/getmodules/installer.go b/internal/getmodules/installer.go index a49ee0be8..087c0cd3d 100644 --- a/internal/getmodules/installer.go +++ b/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. // diff --git a/internal/getmodules/package.go b/internal/getmodules/package.go index d0e46dbab..a46065623 100644 --- a/internal/getmodules/package.go +++ b/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 diff --git a/internal/initwd/from_module.go b/internal/initwd/from_module.go index 28ac987e0..392370032 100644 --- a/internal/initwd/from_module.go +++ b/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{} - _, instDiags := inst.installDescendentModules(fakeRootModule, rootDir, instManifest, true, wrapHooks, getter) + fetcher := getmodules.NewPackageFetcher() + _, 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 diff --git a/internal/initwd/from_module_test.go b/internal/initwd/from_module_test.go index 186d981a2..8d5ab3056 100644 --- a/internal/initwd/from_module_test.go +++ b/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) { - return + if diff := cmp.Diff(wantCalls, hooks.Calls); diff != "" { + t.Fatalf("wrong installer calls\n%s", diff) } loader, err := configload.NewLoader(&configload.Config{ diff --git a/internal/initwd/getter.go b/internal/initwd/getter.go deleted file mode 100644 index a41e047aa..000000000 --- a/internal/initwd/getter.go +++ /dev/null @@ -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 -} diff --git a/internal/initwd/module_install.go b/internal/initwd/module_install.go index d1f3fd9e8..b640ae93f 100644 --- a/internal/initwd/module_install.go +++ b/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. - moduleVersionsUrl map[moduleVersion]string + // addresses and the values are underlying remote source addresses. + 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{} - cfg, instDiags := i.installDescendentModules(rootMod, rootDir, manifest, upgrade, hooks, getter) + fetcher := getmodules.NewPackageFetcher() + 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): - log.Printf("[TRACE] ModuleInstaller: %s has local path %q", key, req.SourceAddr) + switch addr := req.SourceAddr.(type) { + + 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): - addr, err := regsrc.ParseModuleSource(req.SourceAddr) - if err != nil { - // 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) + case addrs.ModuleSourceRegistry: + log.Printf("[TRACE] ModuleInstaller: %s is a registry module at %s", key, addr.String()) + mod, v, mDiags := i.installRegistryModule(req, key, instPath, addr, manifest, hooks, fetcher) diags = append(diags, mDiags...) return mod, v, diags - default: - log.Printf("[TRACE] ModuleInstaller: %s address %q will be handled by go-getter", key, req.SourceAddr) - - mod, mDiags := i.installGoGetterModule(req, key, instPath, manifest, hooks, getter) + case addrs.ModuleSourceRemote: + 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) 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() - 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 - } - + hostname := addr.Host 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, latestMatch) + hooks.Download(key, packageAddr.String(), 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 != "" { - // Append the user's requested subdirectory to any subdirectory that - // was implied by any of the nested layers we expanded within go-getter. - modDir = filepath.Join(modDir, addr.RawSubmodule) - } + // Incorporate any subdir information from the original path into the + // address returned by the registry in order to find the final directory + // of the target module. + 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) - hooks.Download(key, packageAddr, nil) + addr := req.SourceAddr.(addrs.ModuleSourceRemote) + 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" - // for our heuristic here, so we need to slashify Windows-ish paths. - rel := filepath.ToSlash(step.SourceAddr) + rel := step.SourceAddr.String() 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(), "" + } +} diff --git a/internal/initwd/module_install_test.go b/internal/initwd/module_install_test.go index 8e0ec64dc..9f34782d1 100644 --- a/internal/initwd/module_install_test.go +++ b/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) { - return + if diff := cmp.Diff(wantCalls, hooks.Calls); diff != "" { + 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 { - t.Fatal("module versions cache was not populated") + if _, ok := inst.moduleVersions["registry.terraform.io/hashicorp/module-installer-acctest/aws"]; !ok { + 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 { - t.Fatal("module download url cache was not populated") + if _, ok := inst.moduleVersionsUrl[moduleVersion{module: "registry.terraform.io/hashicorp/module-installer-acctest/aws", version: "0.0.1"}]; !ok { + 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) { - return + if diff := cmp.Diff(wantCalls, hooks.Calls); diff != "" { + 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 } diff --git a/internal/modsdir/manifest.go b/internal/modsdir/manifest.go index c347929d5..2821ce804 100644 --- a/internal/modsdir/manifest.go +++ b/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) diff --git a/internal/registry/regsrc/module.go b/internal/registry/regsrc/module.go index c3edd7d87..9ce6b282a 100644 --- a/internal/registry/regsrc/module.go +++ b/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