From 1a8da65314343e5cf4f48c5da22397a81e9cf822 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 27 May 2021 19:24:59 -0700 Subject: [PATCH] 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 | 24 +- internal/addrs/module_source_test.go | 19 +- internal/command/jsonconfig/config.go | 8 +- internal/configs/config.go | 10 +- internal/configs/config_build.go | 2 +- internal/configs/config_build_test.go | 8 +- internal/configs/config_test.go | 6 +- internal/configs/configload/loader_load.go | 2 +- internal/configs/module_call.go | 38 +++- internal/configs/module_call_test.go | 29 ++- internal/configs/module_merge.go | 1 + internal/configs/module_merge_test.go | 5 +- internal/configs/parser_test.go | 4 +- .../testdata/config-build/child_a/child_a.tf | 5 +- .../testdata/config-build/child_b/child_b.tf | 6 +- .../configs/testdata/config-build/root.tf | 4 +- .../testdata/nested-errors/child_a/child_a.tf | 5 +- .../configs/testdata/nested-errors/root.tf | 2 +- internal/earlyconfig/config.go | 2 +- internal/earlyconfig/config_build.go | 18 +- internal/earlyconfig/config_test.go | 2 +- internal/getmodules/file_detector.go | 65 ++++++ internal/getmodules/getter.go | 2 +- internal/getmodules/installer.go | 6 + internal/getmodules/package.go | 8 + internal/initwd/from_module.go | 7 +- internal/initwd/from_module_test.go | 7 +- internal/initwd/getter.go | 214 ------------------ internal/initwd/module_install.go | 189 +++++++++------- internal/initwd/module_install_test.go | 36 +-- internal/modsdir/manifest.go | 17 ++ internal/registry/regsrc/module.go | 27 ++- 32 files changed, 412 insertions(+), 366 deletions(-) create mode 100644 internal/getmodules/file_detector.go delete mode 100644 internal/initwd/getter.go 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