From 51b0aee36cc2145f45f5b04051a01eb6eb7be8bf Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Wed, 2 Jun 2021 12:26:35 -0700 Subject: [PATCH] addrs: ModuleRegistryPackage for representing module registry packages Previously we had a separation between ModuleSourceRemote and ModulePackage as a way to represent within the type system that there's an important difference between a module source address and a package address, because module packages often contain multiple modules and so a ModuleSourceRemote combines a ModulePackage with a subdirectory to represent one specific module. This commit applies that same strategy to ModuleSourceRegistry, creating a new type ModuleRegistryPackage to represent the different sort of package that we use for registry modules. Again, the main goal here is to try to reflect the conceptual modelling more directly in the type system so that we can more easily verify that uses of these different address types are correct. To make use of that, I've also lightly reworked initwd's module installer to use addrs.ModuleRegistryPackage directly, instead of a string representation thereof. This was in response to some earlier commits where I found myself accidentally mixing up package addresses and source addresses in the installRegistryModule method; with this new organization those bugs would've been caught at compile time, rather than only at unit and integration testing time. While in the area anyway, I also took this opportunity to fix some historical confusing names of fields in initwd.ModuleInstaller, to be clearer that they are only for registry packages and not for all module source address types. --- internal/addrs/module_package.go | 59 ++++++++++++++++++++++++ internal/addrs/module_source.go | 62 +++++++------------------- internal/addrs/module_source_test.go | 50 ++++++++++++--------- internal/configs/module_call_test.go | 10 +++-- internal/initwd/module_install.go | 33 +++++++------- internal/initwd/module_install_test.go | 16 +++++-- internal/registry/regsrc/module.go | 17 ++++++- 7 files changed, 153 insertions(+), 94 deletions(-) diff --git a/internal/addrs/module_package.go b/internal/addrs/module_package.go index e21ed6fc2..dc5a6621c 100644 --- a/internal/addrs/module_package.go +++ b/internal/addrs/module_package.go @@ -1,5 +1,11 @@ package addrs +import ( + "strings" + + svchost "github.com/hashicorp/terraform-svchost" +) + // A ModulePackage represents a physical location where Terraform can retrieve // a module package, which is an archive, repository, or other similar // container which delivers the source code for one or more Terraform modules. @@ -28,3 +34,56 @@ type ModulePackage string func (p ModulePackage) String() string { return string(p) } + +// A ModuleRegistryPackage is an extra indirection over a ModulePackage where +// we use a module registry to translate a more symbolic address (and +// associated version constraint given out of band) into a physical source +// location. +// +// ModuleRegistryPackage is distinct from ModulePackage because they have +// disjoint use-cases: registry package addresses are only used to query a +// registry in order to find a real module package address. These being +// distinct is intended to help future maintainers more easily follow the +// series of steps in the module installer, with the help of the type checker. +type ModuleRegistryPackage struct { + Host svchost.Hostname + Namespace string + Name string + TargetSystem string +} + +func (s ModuleRegistryPackage) String() string { + var buf strings.Builder + // Note: we're using the "display" form of the hostname here because + // for our service hostnames "for display" means something different: + // it means to render non-ASCII characters directly as Unicode + // characters, rather than using the "punycode" representation we + // use for internal processing, and so the "display" representation + // is actually what users would write in their configurations. + return s.Host.ForDisplay() + "/" + s.ForRegistryProtocol() + return buf.String() +} + +func (s ModuleRegistryPackage) ForDisplay() string { + if s.Host == DefaultModuleRegistryHost { + return s.ForRegistryProtocol() + } + return s.Host.ForDisplay() + "/" + s.ForRegistryProtocol() +} + +// ForRegistryProtocol returns a string representation of just the namespace, +// name, and target system portions of the address, always omitting the +// registry hostname and the subdirectory portion, if any. +// +// This is primarily intended for generating addresses to send to the +// registry in question via the registry protocol, since the protocol +// skips sending the registry its own hostname as part of identifiers. +func (s ModuleRegistryPackage) ForRegistryProtocol() string { + var buf strings.Builder + buf.WriteString(s.Namespace) + buf.WriteByte('/') + buf.WriteString(s.Name) + buf.WriteByte('/') + buf.WriteString(s.TargetSystem) + return buf.String() +} diff --git a/internal/addrs/module_source.go b/internal/addrs/module_source.go index 0f2c4aff2..bad32ba38 100644 --- a/internal/addrs/module_source.go +++ b/internal/addrs/module_source.go @@ -171,10 +171,11 @@ func (s ModuleSourceLocal) ForDisplay() string { // a concrete ModuleSourceRemote that Terraform will then download and // install. type ModuleSourceRegistry struct { - Host svchost.Hostname - Namespace string - Name string - TargetSystem string + // PackageAddr is the registry package that the target module belongs to. + // The module installer must translate this into a ModuleSourceRemote + // using the registry API and then take that underlying address's + // PackageAddr in order to find the actual package location. + PackageAddr ModuleRegistryPackage // If Subdir is non-empty then it represents a sub-directory within the // remote package that the registry address eventually resolves to. @@ -233,7 +234,9 @@ func parseModuleSourceRegistry(raw string) (ModuleSourceRegistry, error) { } ret := ModuleSourceRegistry{ - Host: host, + PackageAddr: ModuleRegistryPackage{ + Host: host, + }, Subdir: subDir, } @@ -242,7 +245,7 @@ func parseModuleSourceRegistry(raw string) (ModuleSourceRegistry, error) { return ret, fmt.Errorf("can't use %q as a module registry host, because it's reserved for installing directly from version control repositories", host) } - if ret.Namespace, err = parseModuleRegistryName(parts[0]); err != nil { + if ret.PackageAddr.Namespace, err = parseModuleRegistryName(parts[0]); err != nil { if strings.Contains(parts[0], ".") { // Seems like the user omitted one of the latter components in // an address with an explicit hostname. @@ -250,10 +253,10 @@ func parseModuleSourceRegistry(raw string) (ModuleSourceRegistry, error) { } return ret, fmt.Errorf("invalid namespace %q: %s", parts[0], err) } - if ret.Name, err = parseModuleRegistryName(parts[1]); err != nil { + if ret.PackageAddr.Name, err = parseModuleRegistryName(parts[1]); err != nil { return ret, fmt.Errorf("invalid module name %q: %s", parts[1], err) } - if ret.TargetSystem, err = parseModuleRegistryTargetSystem(parts[2]); err != nil { + if ret.PackageAddr.TargetSystem, err = parseModuleRegistryTargetSystem(parts[2]); err != nil { if strings.Contains(parts[2], "?") { // The user was trying to include a query string, probably? return ret, fmt.Errorf("module registry addresses may not include a query string portion") @@ -314,52 +317,17 @@ func parseModuleRegistryTargetSystem(given string) (string, error) { func (s ModuleSourceRegistry) moduleSource() {} func (s ModuleSourceRegistry) String() string { - var buf strings.Builder - // Note: we're using the "display" form of the hostname here because - // for our service hostnames "for display" means something different: - // it means to render non-ASCII characters directly as Unicode - // characters, rather than using the "punycode" representation we - // use for internal processing, and so the "display" representation - // is actually what users would write in their configurations. - buf.WriteString(s.Host.ForDisplay()) - buf.WriteByte('/') - buf.WriteString(s.ForRegistryProtocol()) if s.Subdir != "" { - buf.WriteString("//") - buf.WriteString(s.Subdir) + return s.PackageAddr.String() + "//" + s.Subdir } - return buf.String() + return s.PackageAddr.String() } func (s ModuleSourceRegistry) ForDisplay() string { - var buf strings.Builder - if s.Host != DefaultModuleRegistryHost { - buf.WriteString(s.Host.ForDisplay()) - buf.WriteByte('/') - } - buf.WriteString(s.ForRegistryProtocol()) if s.Subdir != "" { - buf.WriteString("//") - buf.WriteString(s.Subdir) + return s.PackageAddr.ForDisplay() + "//" + s.Subdir } - return buf.String() -} - -// ForRegistryProtocol returns a string representation of just the namespace, -// name, and target system portions of the address, always omitting the -// registry hostname and the subdirectory portion, if any. -// -// This is primarily intended for generating addresses to send to the -// registry in question via the registry protocol, since the protocol -// skips sending the registry its own hostname as part of identifiers. -func (s ModuleSourceRegistry) ForRegistryProtocol() string { - var buf strings.Builder - buf.WriteString(s.Namespace) - buf.WriteByte('/') - buf.WriteString(s.Name) - buf.WriteByte('/') - buf.WriteString(s.TargetSystem) - return buf.String() + return s.PackageAddr.ForDisplay() } // ModuleSourceRemote is a ModuleSource representing a remote location from diff --git a/internal/addrs/module_source_test.go b/internal/addrs/module_source_test.go index 6303ff20c..480ca50fb 100644 --- a/internal/addrs/module_source_test.go +++ b/internal/addrs/module_source_test.go @@ -59,21 +59,25 @@ func TestParseModuleSource(t *testing.T) { "main registry implied": { input: "hashicorp/subnets/cidr", want: ModuleSourceRegistry{ - Host: svchost.Hostname("registry.terraform.io"), - Namespace: "hashicorp", - Name: "subnets", - TargetSystem: "cidr", - Subdir: "", + PackageAddr: ModuleRegistryPackage{ + Host: svchost.Hostname("registry.terraform.io"), + Namespace: "hashicorp", + Name: "subnets", + TargetSystem: "cidr", + }, + Subdir: "", }, }, "main registry implied, subdir": { input: "hashicorp/subnets/cidr//examples/foo", want: ModuleSourceRegistry{ - Host: svchost.Hostname("registry.terraform.io"), - Namespace: "hashicorp", - Name: "subnets", - TargetSystem: "cidr", - Subdir: "examples/foo", + PackageAddr: ModuleRegistryPackage{ + Host: svchost.Hostname("registry.terraform.io"), + Namespace: "hashicorp", + Name: "subnets", + TargetSystem: "cidr", + }, + Subdir: "examples/foo", }, }, "main registry implied, escaping subdir": { @@ -88,21 +92,25 @@ func TestParseModuleSource(t *testing.T) { "custom registry": { input: "example.com/awesomecorp/network/happycloud", want: ModuleSourceRegistry{ - Host: svchost.Hostname("example.com"), - Namespace: "awesomecorp", - Name: "network", - TargetSystem: "happycloud", - Subdir: "", + PackageAddr: ModuleRegistryPackage{ + Host: svchost.Hostname("example.com"), + Namespace: "awesomecorp", + Name: "network", + TargetSystem: "happycloud", + }, + Subdir: "", }, }, "custom registry, subdir": { input: "example.com/awesomecorp/network/happycloud//examples/foo", want: ModuleSourceRegistry{ - Host: svchost.Hostname("example.com"), - Namespace: "awesomecorp", - Name: "network", - TargetSystem: "happycloud", - Subdir: "examples/foo", + PackageAddr: ModuleRegistryPackage{ + Host: svchost.Hostname("example.com"), + Namespace: "awesomecorp", + Name: "network", + TargetSystem: "happycloud", + }, + Subdir: "examples/foo", }, }, @@ -536,7 +544,7 @@ func TestParseModuleSourceRegistry(t *testing.T) { if got, want := addr.ForDisplay(), test.wantForDisplay; got != want { t.Errorf("wrong ForDisplay() result\ngot: %s\nwant: %s", got, want) } - if got, want := addr.ForRegistryProtocol(), test.wantForProtocol; got != want { + if got, want := addr.PackageAddr.ForRegistryProtocol(), test.wantForProtocol; got != want { t.Errorf("wrong ForRegistryProtocol() result\ngot: %s\nwant: %s", got, want) } }) diff --git a/internal/configs/module_call_test.go b/internal/configs/module_call_test.go index eaf923739..9c607f8e3 100644 --- a/internal/configs/module_call_test.go +++ b/internal/configs/module_call_test.go @@ -45,10 +45,12 @@ func TestLoadModuleCall(t *testing.T) { { Name: "bar", SourceAddr: addrs.ModuleSourceRegistry{ - Host: addrs.DefaultModuleRegistryHost, - Namespace: "hashicorp", - Name: "bar", - TargetSystem: "aws", + PackageAddr: addrs.ModuleRegistryPackage{ + Host: addrs.DefaultModuleRegistryHost, + Namespace: "hashicorp", + Name: "bar", + TargetSystem: "aws", + }, }, SourceAddrRaw: "hashicorp/bar/aws", SourceSet: true, diff --git a/internal/initwd/module_install.go b/internal/initwd/module_install.go index b640ae93f..83f4ed2bb 100644 --- a/internal/initwd/module_install.go +++ b/internal/initwd/module_install.go @@ -26,24 +26,24 @@ type ModuleInstaller struct { // The keys in moduleVersions are resolved and trimmed registry source // addresses and the values are the registry response. - moduleVersions map[string]*response.ModuleVersions + registryPackageVersions map[addrs.ModuleRegistryPackage]*response.ModuleVersions // The keys in moduleVersionsUrl are the moduleVersion struct below and // addresses and the values are underlying remote source addresses. - moduleVersionsUrl map[moduleVersion]addrs.ModuleSourceRemote + registryPackageSources map[moduleVersion]addrs.ModuleSourceRemote } type moduleVersion struct { - module string + module addrs.ModuleRegistryPackage version string } func NewModuleInstaller(modsDir string, reg *registry.Client) *ModuleInstaller { return &ModuleInstaller{ - modsDir: modsDir, - reg: reg, - moduleVersions: make(map[string]*response.ModuleVersions), - moduleVersionsUrl: make(map[moduleVersion]addrs.ModuleSourceRemote), + modsDir: modsDir, + reg: reg, + registryPackageVersions: make(map[addrs.ModuleRegistryPackage]*response.ModuleVersions), + registryPackageSources: make(map[moduleVersion]addrs.ModuleSourceRemote), } } @@ -304,7 +304,7 @@ func (i *ModuleInstaller) installLocalModule(req *earlyconfig.ModuleRequest, key 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 := addr.Host + hostname := addr.PackageAddr.Host reg := i.reg var resp *response.ModuleVersions var exists bool @@ -312,15 +312,14 @@ func (i *ModuleInstaller) installRegistryModule(req *earlyconfig.ModuleRequest, // 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 = "" + packageAddr := addr.PackageAddr // Our registry client is still using the legacy model of addresses, so // we'll shim it here for now. - regsrcAddr := regsrc.ModuleFromModuleSourceAddr(packageAddr) + regsrcAddr := regsrc.ModuleFromRegistryPackageAddr(packageAddr) // check if we've already looked up this module from the registry - if resp, exists = i.moduleVersions[packageAddr.String()]; exists { + if resp, exists = i.registryPackageVersions[packageAddr]; exists { log.Printf("[TRACE] %s using already found available versions of %s at %s", key, addr, hostname) } else { var err error @@ -342,7 +341,7 @@ func (i *ModuleInstaller) installRegistryModule(req *earlyconfig.ModuleRequest, } return nil, nil, diags } - i.moduleVersions[packageAddr.String()] = resp + i.registryPackageVersions[packageAddr] = resp } // The response might contain information about dependencies to allow us @@ -422,8 +421,8 @@ func (i *ModuleInstaller) installRegistryModule(req *earlyconfig.ModuleRequest, // The response to this is a go-getter-style address string. // first check the cache for the download URL - moduleAddr := moduleVersion{module: packageAddr.String(), version: latestMatch.String()} - if _, exists := i.moduleVersionsUrl[moduleAddr]; !exists { + moduleAddr := moduleVersion{module: packageAddr, version: latestMatch.String()} + if _, exists := i.registryPackageSources[moduleAddr]; !exists { realAddrRaw, err := reg.ModuleLocation(regsrcAddr, latestMatch.String()) if err != nil { log.Printf("[ERROR] %s from %s %s: %s", key, addr, latestMatch, err) @@ -449,7 +448,7 @@ func (i *ModuleInstaller) installRegistryModule(req *earlyconfig.ModuleRequest, // 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 + i.registryPackageSources[moduleAddr] = realAddr default: diags = diags.Append(tfdiags.Sourceless( tfdiags.Error, @@ -460,7 +459,7 @@ func (i *ModuleInstaller) installRegistryModule(req *earlyconfig.ModuleRequest, } } - dlAddr := i.moduleVersionsUrl[moduleAddr] + dlAddr := i.registryPackageSources[moduleAddr] log.Printf("[TRACE] ModuleInstaller: %s %s %s is available at %q", key, packageAddr, latestMatch, dlAddr.PackageAddr) diff --git a/internal/initwd/module_install_test.go b/internal/initwd/module_install_test.go index 25905d90e..223ca1ca7 100644 --- a/internal/initwd/module_install_test.go +++ b/internal/initwd/module_install_test.go @@ -14,6 +14,8 @@ import ( "github.com/go-test/deep" "github.com/google/go-cmp/cmp" version "github.com/hashicorp/go-version" + svchost "github.com/hashicorp/terraform-svchost" + "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/configs" "github.com/hashicorp/terraform/internal/configs/configload" "github.com/hashicorp/terraform/internal/copy" @@ -403,11 +405,17 @@ func TestLoaderInstallModules_registry(t *testing.T) { } //check that the registry reponses were cached - 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)) + packageAddr := addrs.ModuleRegistryPackage{ + Host: svchost.Hostname("registry.terraform.io"), + Namespace: "hashicorp", + Name: "module-installer-acctest", + TargetSystem: "aws", } - 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)) + if _, ok := inst.registryPackageVersions[packageAddr]; !ok { + t.Errorf("module versions cache was not populated\ngot: %s\nwant: key hashicorp/module-installer-acctest/aws", spew.Sdump(inst.registryPackageVersions)) + } + if _, ok := inst.registryPackageSources[moduleVersion{module: packageAddr, version: "0.0.1"}]; !ok { + t.Errorf("module download url cache was not populated\ngot: %s", spew.Sdump(inst.registryPackageSources)) } loader, err := configload.NewLoader(&configload.Config{ diff --git a/internal/registry/regsrc/module.go b/internal/registry/regsrc/module.go index 9ce6b282a..9f9999d34 100644 --- a/internal/registry/regsrc/module.go +++ b/internal/registry/regsrc/module.go @@ -105,12 +105,27 @@ func NewModule(host, namespace, name, provider, submodule string) (*Module, erro // use addrs.ModuleSourceRegistry instead, and then package regsrc can be // removed altogether. func ModuleFromModuleSourceAddr(addr addrs.ModuleSourceRegistry) *Module { + ret := ModuleFromRegistryPackageAddr(addr.PackageAddr) + ret.RawSubmodule = addr.Subdir + return ret +} + +// ModuleFromRegistryPackageAddr is similar to ModuleFromModuleSourceAddr, but +// it works with just the isolated registry package address, and not the +// full source address. +// +// The practical implication of that is that RawSubmodule will always be +// the empty string in results from this function, because "Submodule" maps +// to "Subdir" and that's a module source address concept, not a module +// package concept. In practice this typically doesn't matter because the +// registry client ignores the RawSubmodule field anyway; that's a concern +// for the higher-level module installer to deal with. +func ModuleFromRegistryPackageAddr(addr addrs.ModuleRegistryPackage) *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, } }