diff --git a/internal/addrs/module_source.go b/internal/addrs/module_source.go index bad32ba38..c42bc9f04 100644 --- a/internal/addrs/module_source.go +++ b/internal/addrs/module_source.go @@ -46,18 +46,36 @@ var moduleSourceLocalPrefixes = []string{ "..\\", } +// ParseModuleSource parses a module source address as given in the "source" +// argument inside a "module" block in the configuration. +// +// For historical reasons this syntax is a bit overloaded, supporting three +// different address types: +// - Local paths starting with either ./ or ../, which are special because +// Terraform considers them to belong to the same "package" as the caller. +// - Module registry addresses, given as either NAMESPACE/NAME/SYSTEM or +// HOST/NAMESPACE/NAME/SYSTEM, in which case the remote registry serves +// as an indirection over the third address type that follows. +// - Various URL-like and other heuristically-recognized strings which +// we currently delegate to the external library go-getter. +// +// There is some ambiguity between the module registry addresses and go-getter's +// very liberal heuristics and so this particular function will typically treat +// an invalid registry address as some other sort of remote source address +// rather than returning an error. If you know that you're expecting a +// registry address in particular, use ParseModuleSourceRegistry instead, which +// can therefore expose more detailed error messages about registry address +// parsing in particular. func ParseModuleSource(raw string) (ModuleSource, error) { - for _, prefix := range moduleSourceLocalPrefixes { - if strings.HasPrefix(raw, prefix) { - localAddr, err := parseModuleSourceLocal(raw) - if err != nil { - // This is to make sure we really return a nil ModuleSource in - // this case, rather than an interface containing the zero - // value of ModuleSourceLocal. - return nil, err - } - return localAddr, nil + if isModuleSourceLocal(raw) { + localAddr, err := parseModuleSourceLocal(raw) + if err != nil { + // This is to make sure we really return a nil ModuleSource in + // this case, rather than an interface containing the zero + // value of ModuleSourceLocal. + return nil, err } + return localAddr, nil } // For historical reasons, whether an address is a registry @@ -71,7 +89,7 @@ func ParseModuleSource(raw string) (ModuleSource, error) { // the registry source parse error gets returned to the caller, // which is annoying but has been true for many releases // without it posing a serious problem in practice.) - if ret, err := parseModuleSourceRegistry(raw); err == nil { + if ret, err := ParseModuleSourceRegistry(raw); err == nil { return ret, nil } @@ -150,6 +168,15 @@ func parseModuleSourceLocal(raw string) (ModuleSourceLocal, error) { return ModuleSourceLocal(clean), nil } +func isModuleSourceLocal(raw string) bool { + for _, prefix := range moduleSourceLocalPrefixes { + if strings.HasPrefix(raw, prefix) { + return true + } + } + return false +} + func (s ModuleSourceLocal) moduleSource() {} func (s ModuleSourceLocal) String() string { @@ -195,6 +222,30 @@ const DefaultModuleRegistryHost = svchost.Hostname("registry.terraform.io") var moduleRegistryNamePattern = regexp.MustCompile("^[0-9A-Za-z](?:[0-9A-Za-z-_]{0,62}[0-9A-Za-z])?$") var moduleRegistryTargetSystemPattern = regexp.MustCompile("^[0-9a-z]{1,64}$") +// ParseModuleSourceRegistry is a variant of ParseModuleSource which only +// accepts module registry addresses, and will reject any other address type. +// +// Use this instead of ParseModuleSource if you know from some other surrounding +// context that an address is intended to be a registry address rather than +// some other address type, which will then allow for better error reporting +// due to the additional information about user intent. +func ParseModuleSourceRegistry(raw string) (ModuleSource, error) { + // Before we delegate to the "real" function we'll just make sure this + // doesn't look like a local source address, so we can return a better + // error message for that situation. + if isModuleSourceLocal(raw) { + return ModuleSourceRegistry{}, fmt.Errorf("can't use local directory %q as a module registry address", raw) + } + + ret, err := parseModuleSourceRegistry(raw) + if err != nil { + // This is to make sure we return a nil ModuleSource, rather than + // a non-nil ModuleSource containing a zero-value ModuleSourceRegistry. + return nil, err + } + return ret, nil +} + func parseModuleSourceRegistry(raw string) (ModuleSourceRegistry, error) { var err error @@ -298,11 +349,10 @@ func parseModuleRegistryTargetSystem(given string) (string, error) { // Similar to the names in provider source addresses, we defined these // to be compatible with what filesystems and typical remote systems // like GitHub allow in names. Unfortunately we didn't end up defining - // these exactly equivalently: provider names can only use dashes as - // punctuation, whereas module names can use underscores. So here we're - // using some regular expressions from the original module source - // implementation, rather than using the IDNA rules as we do in - // ParseProviderPart. + // these exactly equivalently: provider names can't use dashes or + // underscores. So here we're using some regular expressions from the + // original module source implementation, rather than using the IDNA rules + // as we do in ParseProviderPart. if !moduleRegistryTargetSystemPattern.MatchString(given) { return "", fmt.Errorf("must be between one and 64 ASCII letters or digits") diff --git a/internal/addrs/module_source_test.go b/internal/addrs/module_source_test.go index 480ca50fb..9604c3374 100644 --- a/internal/addrs/module_source_test.go +++ b/internal/addrs/module_source_test.go @@ -488,10 +488,14 @@ func TestParseModuleSourceRegistry(t *testing.T) { input: `foo/var/baz/qux`, wantErr: `invalid module registry hostname: must contain at least one dot`, }, - "invalid target system": { + "invalid target system characters": { input: `foo/var/no-no-no`, wantErr: `invalid target system "no-no-no": must be between one and 64 ASCII letters or digits`, }, + "invalid target system length": { + input: `foo/var/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaah`, + wantErr: `invalid target system "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaah": must be between one and 64 ASCII letters or digits`, + }, "invalid namespace": { input: `boop!/var/baz`, wantErr: `invalid namespace "boop!": must be between one and 64 characters, including ASCII letters, digits, dashes, and underscores, where dashes and underscores may not be the prefix or suffix`, @@ -518,11 +522,23 @@ func TestParseModuleSourceRegistry(t *testing.T) { input: `bitbucket.org/HashiCorp/Consul/aws`, wantErr: `can't use "bitbucket.org" as a module registry host, because it's reserved for installing directly from version control repositories`, }, + "local path from current dir": { + // Can't use a local path when we're specifically trying to parse + // a _registry_ source address. + input: `./boop`, + wantErr: `can't use local directory "./boop" as a module registry address`, + }, + "local path from parent dir": { + // Can't use a local path when we're specifically trying to parse + // a _registry_ source address. + input: `../boop`, + wantErr: `can't use local directory "../boop" as a module registry address`, + }, } for name, test := range tests { t.Run(name, func(t *testing.T) { - addr, err := parseModuleSourceRegistry(test.input) + addrI, err := ParseModuleSourceRegistry(test.input) if test.wantErr != "" { switch { @@ -538,6 +554,11 @@ func TestParseModuleSourceRegistry(t *testing.T) { t.Fatalf("unexpected error: %s", err.Error()) } + addr, ok := addrI.(ModuleSourceRegistry) + if !ok { + t.Fatalf("wrong address type %T; want %T", addrI, addr) + } + if got, want := addr.String(), test.wantString; got != want { t.Errorf("wrong String() result\ngot: %s\nwant: %s", got, want) } diff --git a/internal/configs/module_call.go b/internal/configs/module_call.go index 5d887dac0..3ec42ec01 100644 --- a/internal/configs/module_call.go +++ b/internal/configs/module_call.go @@ -59,15 +59,35 @@ func decodeModuleBlock(block *hcl.Block, override bool) (*ModuleCall, hcl.Diagno }) } + haveVersionArg := false + if attr, exists := content.Attributes["version"]; exists { + var versionDiags hcl.Diagnostics + mc.Version, versionDiags = decodeVersionConstraint(attr) + diags = append(diags, versionDiags...) + haveVersionArg = true + } + if attr, exists := content.Attributes["source"]; exists { mc.SourceSet = true mc.SourceAddrRange = attr.Expr.Range() valDiags := gohcl.DecodeExpression(attr.Expr, nil, &mc.SourceAddrRaw) diags = append(diags, valDiags...) if !valDiags.HasErrors() { - addr, err := addrs.ParseModuleSource(mc.SourceAddrRaw) + var addr addrs.ModuleSource + var err error + if haveVersionArg { + addr, err = addrs.ParseModuleSourceRegistry(mc.SourceAddrRaw) + } else { + addr, err = addrs.ParseModuleSource(mc.SourceAddrRaw) + } mc.SourceAddr = addr if err != nil { + // NOTE: We leave mc.SourceAddr as nil for any situation where the + // source attribute is invalid, so any code which tries to carefully + // use the partial result of a failed config decode must be + // resilient to that. + mc.SourceAddr = nil + // NOTE: In practice it's actually very unlikely to end up here, // because our source address parser can turn just about any string // into some sort of remote package address, and so for most errors @@ -87,25 +107,27 @@ func decodeModuleBlock(block *hcl.Block, override bool) (*ModuleCall, hcl.Diagno Subject: mc.SourceAddrRange.Ptr(), }) default: - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid module source address", - Detail: fmt.Sprintf("Failed to parse module source address: %s.", err), - Subject: mc.SourceAddrRange.Ptr(), - }) + if haveVersionArg { + // In this case we'll include some extra context that + // we assumed a registry source address due to the + // version argument. + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid registry module source address", + Detail: fmt.Sprintf("Failed to parse module registry address: %s.\n\nTerraform assumed that you intended a module registry source address because you also set the argument \"version\", which applies only to registry modules.", err), + Subject: mc.SourceAddrRange.Ptr(), + }) + } else { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid module source address", + Detail: fmt.Sprintf("Failed to parse module source address: %s.", err), + Subject: mc.SourceAddrRange.Ptr(), + }) + } } } } - // NOTE: We leave mc.SourceAddr as nil for any situation where the - // source attribute is invalid, so any code which tries to carefully - // use the partial result of a failed config decode must be - // resilient to that. - } - - if attr, exists := content.Attributes["version"]; exists { - var versionDiags hcl.Diagnostics - mc.Version, versionDiags = decodeVersionConstraint(attr) - diags = append(diags, versionDiags...) } if attr, exists := content.Attributes["count"]; exists { diff --git a/internal/configs/testdata/error-files/module-invalid-registry-source-with-module.tf b/internal/configs/testdata/error-files/module-invalid-registry-source-with-module.tf new file mode 100644 index 000000000..0029be8f4 --- /dev/null +++ b/internal/configs/testdata/error-files/module-invalid-registry-source-with-module.tf @@ -0,0 +1,5 @@ + +module "test" { + source = "---.com/HashiCorp/Consul/aws" # ERROR: Invalid registry module source address + version = "1.0.0" # Makes Terraform assume "source" is a module address +} diff --git a/internal/configs/testdata/error-files/module-local-source-with-version.tf b/internal/configs/testdata/error-files/module-local-source-with-version.tf new file mode 100644 index 000000000..f570d65fe --- /dev/null +++ b/internal/configs/testdata/error-files/module-local-source-with-version.tf @@ -0,0 +1,5 @@ + +module "test" { + source = "../boop" # ERROR: Invalid registry module source address + version = "1.0.0" # Makes Terraform assume "source" is a module address +} diff --git a/internal/earlyconfig/config_build.go b/internal/earlyconfig/config_build.go index 807d9cd77..dd84cf9cc 100644 --- a/internal/earlyconfig/config_build.go +++ b/internal/earlyconfig/config_build.go @@ -43,7 +43,10 @@ func buildChildModules(parent *Config, walker ModuleWalker) (map[string]*Config, path[len(path)-1] = call.Name var vc version.Constraints + haveVersionArg := false if strings.TrimSpace(call.Version) != "" { + haveVersionArg = true + var err error vc, err = version.NewConstraint(call.Version) if err != nil { @@ -56,13 +59,27 @@ func buildChildModules(parent *Config, walker ModuleWalker) (map[string]*Config, } } - sourceAddr, err := addrs.ParseModuleSource(call.Source) + var sourceAddr addrs.ModuleSource + var err error + if haveVersionArg { + sourceAddr, err = addrs.ParseModuleSourceRegistry(call.Source) + } else { + sourceAddr, err = addrs.ParseModuleSource(call.Source) + } if err != nil { - diags = diags.Append(wrapDiagnostic(tfconfig.Diagnostic{ - Severity: tfconfig.DiagError, - Summary: "Invalid module source address", - Detail: fmt.Sprintf("Module %q (declared at %s line %d) has invalid source address %q: %s.", callName, call.Pos.Filename, call.Pos.Line, call.Source, err), - })) + if haveVersionArg { + diags = diags.Append(wrapDiagnostic(tfconfig.Diagnostic{ + Severity: tfconfig.DiagError, + Summary: "Invalid registry module source address", + Detail: fmt.Sprintf("Module %q (declared at %s line %d) has invalid source address %q: %s.\n\nTerraform assumed that you intended a module registry source address because you also set the argument \"version\", which applies only to registry modules.", callName, call.Pos.Filename, call.Pos.Line, call.Source, err), + })) + } else { + diags = diags.Append(wrapDiagnostic(tfconfig.Diagnostic{ + Severity: tfconfig.DiagError, + Summary: "Invalid module source address", + Detail: fmt.Sprintf("Module %q (declared at %s line %d) has invalid source address %q: %s.", callName, call.Pos.Filename, call.Pos.Line, call.Source, err), + })) + } // If we didn't have a valid source address then we can't continue // down the module tree with this one. continue diff --git a/internal/initwd/module_install.go b/internal/initwd/module_install.go index c5355ee96..e9ee49531 100644 --- a/internal/initwd/module_install.go +++ b/internal/initwd/module_install.go @@ -547,7 +547,7 @@ func (i *ModuleInstaller) installGoGetterModule(ctx context.Context, req *earlyc diags = diags.Append(tfdiags.Sourceless( tfdiags.Error, "Invalid version constraint", - fmt.Sprintf("Cannot apply a version constraint to module %q (at %s:%d) because it has a non Registry URL.", req.Name, req.CallPos.Filename, req.CallPos.Line), + fmt.Sprintf("Cannot apply a version constraint to module %q (at %s:%d) because it doesn't come from a module registry.", req.Name, req.CallPos.Filename, req.CallPos.Line), )) return nil, diags } diff --git a/internal/initwd/module_install_test.go b/internal/initwd/module_install_test.go index 534ef5840..b05c5619f 100644 --- a/internal/initwd/module_install_test.go +++ b/internal/initwd/module_install_test.go @@ -192,7 +192,12 @@ func TestModuleInstaller_invalid_version_constraint_error(t *testing.T) { if !diags.HasErrors() { t.Fatal("expected error") } else { - assertDiagnosticSummary(t, diags, "Invalid version constraint") + // We use the presence of the "version" argument as a heuristic for + // user intent to use a registry module, and so we intentionally catch + // this as an invalid registry module address rather than an invalid + // version constraint, so we can surface the specific address parsing + // error instead of a generic version constraint error. + assertDiagnosticSummary(t, diags, "Invalid registry module source address") } } @@ -210,7 +215,12 @@ func TestModuleInstaller_invalidVersionConstraintGetter(t *testing.T) { if !diags.HasErrors() { t.Fatal("expected error") } else { - assertDiagnosticSummary(t, diags, "Invalid version constraint") + // We use the presence of the "version" argument as a heuristic for + // user intent to use a registry module, and so we intentionally catch + // this as an invalid registry module address rather than an invalid + // version constraint, so we can surface the specific address parsing + // error instead of a generic version constraint error. + assertDiagnosticSummary(t, diags, "Invalid registry module source address") } } @@ -228,7 +238,12 @@ func TestModuleInstaller_invalidVersionConstraintLocal(t *testing.T) { if !diags.HasErrors() { t.Fatal("expected error") } else { - assertDiagnosticSummary(t, diags, "Invalid version constraint") + // We use the presence of the "version" argument as a heuristic for + // user intent to use a registry module, and so we intentionally catch + // this as an invalid registry module address rather than an invalid + // version constraint, so we can surface the specific address parsing + // error instead of a generic version constraint error. + assertDiagnosticSummary(t, diags, "Invalid registry module source address") } }