From affe2c329561f40f13c0e94f4570321977527a77 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Tue, 30 Nov 2021 14:30:44 -0800 Subject: [PATCH] addrs: Expose the registry address parser's error messages Previously we ended up losing all of the error message detail produced by the registry address parser, because we treated any registry address failure as cause to parse the address as a go-getter-style remote address instead. That led to terrible feedback in the situation where the user _was_ trying to write a module address but it was invalid in some way. Although we can't really tighten this up in the default case due to our compatibility promises, it's never been valid to use the "version" argument with anything other than a registry address and so as a compromise here we'll use the presence of "version" as a heuristic for user intent to parse the source address as a registry address, and thus we can return a registry-address-specific error message in that case and thus give more direct feedback about what was wrong. This unfortunately won't help someone trying to install from the registry _without_ a version constraint, but I didn't want to let perfect be the enemy of the good here, particularly since we recommend using version constraints with registry modules anyway; indeed, that's one of the main benefits of using a registry rather than a remote source directly. --- internal/addrs/module_source.go | 82 +++++++++++++++---- internal/addrs/module_source_test.go | 25 +++++- internal/configs/module_call.go | 56 +++++++++---- ...ule-invalid-registry-source-with-module.tf | 5 ++ .../module-local-source-with-version.tf | 5 ++ internal/earlyconfig/config_build.go | 29 +++++-- internal/initwd/module_install.go | 2 +- internal/initwd/module_install_test.go | 21 ++++- 8 files changed, 180 insertions(+), 45 deletions(-) create mode 100644 internal/configs/testdata/error-files/module-invalid-registry-source-with-module.tf create mode 100644 internal/configs/testdata/error-files/module-local-source-with-version.tf 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") } }