From 0d10564a74a40ba8b2560259ba5ebecda778984c Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 25 Oct 2017 17:32:43 -0400 Subject: [PATCH] remove the registryDetector The detection of registry modules will have to happen in mutliple phases. The go-getter interface requires that the detector return the final URL, while we won't know that until we verify which version we need. This leaves the regisry sources broken, to be re-integrated in a following commit. --- config/module/get.go | 92 ----------------- config/module/get_test.go | 208 +++----------------------------------- config/module/registry.go | 98 ++++++++++++++++-- config/module/tree.go | 2 +- registry/regsrc/module.go | 6 ++ 5 files changed, 106 insertions(+), 300 deletions(-) diff --git a/config/module/get.go b/config/module/get.go index 26a31fa845..58515ab363 100644 --- a/config/module/get.go +++ b/config/module/get.go @@ -1,16 +1,10 @@ package module import ( - "fmt" "io/ioutil" - "net/http" "os" - "regexp" - "strings" "github.com/hashicorp/go-getter" - - cleanhttp "github.com/hashicorp/go-cleanhttp" ) // GetMode is an enum that describes how modules are loaded. @@ -63,89 +57,3 @@ func GetCopy(dst, src string) error { // Copy to the final location return copyDir(dst, tmpDir) } - -const ( - registryAPI = "https://registry.terraform.io/v1/modules" -) - -var detectors = []getter.Detector{ - new(getter.GitHubDetector), - new(getter.BitBucketDetector), - new(getter.S3Detector), - new(registryDetector), - new(getter.FileDetector), -} - -// these prefixes can't be registry IDs -// "http", "../", "./", "/", "getter::", etc -var oldSkipRegistry = regexp.MustCompile(`^(http|[.]{1,2}/|/|[A-Za-z0-9]+::)`).MatchString - -// registryDetector implements getter.Detector to detect Terraform Registry modules. -// If a path looks like a registry module identifier, attempt to locate it in -// the registry. If it's not found, pass it on in case it can be found by -// other means. -type registryDetector struct { - // override the default registry URL - api string - - client *http.Client -} - -func (d registryDetector) Detect(src, _ string) (string, bool, error) { - // the namespace can't start with "http", a relative or absolute path, or - // contain a go-getter "forced getter" - if oldSkipRegistry(src) { - return "", false, nil - } - - // there are 3 parts to a registry ID - if len(strings.Split(src, "/")) != 3 { - return "", false, nil - } - - return d.lookupModule(src) -} - -// Lookup the module in the registry. -func (d registryDetector) lookupModule(src string) (string, bool, error) { - if d.api == "" { - d.api = registryAPI - } - - if d.client == nil { - d.client = cleanhttp.DefaultClient() - } - - // src is already partially validated in Detect. We know it's a path, and - // if it can be parsed as a URL we will hand it off to the registry to - // determine if it's truly valid. - resp, err := d.client.Get(fmt.Sprintf("%s/%s/download", d.api, src)) - if err != nil { - return "", false, fmt.Errorf("error looking up module %q: %s", src, err) - } - defer resp.Body.Close() - - // there should be no body, but save it for logging - body, err := ioutil.ReadAll(resp.Body) - if err != nil { - return "", false, fmt.Errorf("error reading response body from registry: %s", err) - } - - switch resp.StatusCode { - case http.StatusOK, http.StatusNoContent: - // OK - case http.StatusNotFound: - return "", false, fmt.Errorf("module %q not found in registry", src) - default: - // anything else is an error: - return "", false, fmt.Errorf("error getting download location for %q: %s resp:%s", src, resp.Status, body) - } - - // the download location is in the X-Terraform-Get header - location := resp.Header.Get(xTerraformGet) - if location == "" { - return "", false, fmt.Errorf("failed to get download URL for %q: %s resp:%s", src, resp.Status, body) - } - - return location, true, nil -} diff --git a/config/module/get_test.go b/config/module/get_test.go index 6b94ac3725..be0aea902d 100644 --- a/config/module/get_test.go +++ b/config/module/get_test.go @@ -8,14 +8,13 @@ import ( "net/http/httptest" "net/url" "os" - "path/filepath" "regexp" "sort" "strings" "testing" - getter "github.com/hashicorp/go-getter" version "github.com/hashicorp/go-version" + "github.com/hashicorp/terraform/registry/regsrc" "github.com/hashicorp/terraform/registry/response" ) @@ -179,199 +178,10 @@ func mockTLSRegistry() *httptest.Server { return server } -func setResetRegDetector(server *httptest.Server) func() { - regDetector := ®istryDetector{ - api: server.URL + "/v1/modules", - client: server.Client(), - } - - origDetectors := detectors - detectors = []getter.Detector{ - new(getter.GitHubDetector), - new(getter.BitBucketDetector), - new(getter.S3Detector), - regDetector, - new(getter.FileDetector), - } - - return func() { - detectors = origDetectors - } -} - -func TestDetectRegistry(t *testing.T) { - server := mockRegistry() - defer server.Close() - - detector := registryDetector{ - api: server.URL + "/v1/modules", - client: server.Client(), - } - - for _, tc := range []struct { - source string - location string - found bool - err bool - }{ - { - source: "registry/foo/bar", - location: testMods["registry/foo/bar"][0].location, - found: true, - }, - { - source: "registry/foo/baz", - location: testMods["registry/foo/baz"][0].location, - found: true, - }, - // this should not be found, and is no longer valid as a local source - { - source: "registry/foo/notfound", - err: true, - }, - - // a full url should not be detected - { - source: "http://example.com/registry/foo/notfound", - found: false, - }, - - // paths should not be detected - { - source: "./local/foo/notfound", - found: false, - }, - { - source: "/local/foo/notfound", - found: false, - }, - - // wrong number of parts can't be regisry IDs - { - source: "something/registry/foo/notfound", - found: false, - }, - } { - - t.Run(tc.source, func(t *testing.T) { - loc, ok, err := detector.Detect(tc.source, "") - if (err == nil) == tc.err { - t.Fatalf("expected error? %t; got error: %v", tc.err, err) - } - - if ok != tc.found { - t.Fatalf("expected OK == %t", tc.found) - } - - loc = strings.TrimPrefix(loc, server.URL+"/") - if strings.TrimPrefix(loc, server.URL) != tc.location { - t.Fatalf("expected location: %q, got %q", tc.location, loc) - } - }) - - } -} - -// check that the full set of detectors works as expected -func TestDetectors(t *testing.T) { - server := mockRegistry() - defer server.Close() - defer setResetRegDetector(server)() - - wd, err := os.Getwd() - if err != nil { - t.Fatal(err) - } - - for _, tc := range []struct { - source string - location string - fixture string - err bool - }{ - { - source: "registry/foo/bar", - location: "file:///download/registry/foo/bar/0.2.3//*?archive=tar.gz", - }, - // this should not be found, and is no longer a valid local source - { - source: "registry/foo/notfound", - err: true, - }, - // a full url should be unchanged - { - source: "http://example.com/registry/foo/notfound?" + - "checksum=sha256:f19056b80a426d797ff9e470da069c171a6c6befa83e2da7f6c706207742acab", - location: "http://example.com/registry/foo/notfound?" + - "checksum=sha256:f19056b80a426d797ff9e470da069c171a6c6befa83e2da7f6c706207742acab", - }, - - // forced getters will return untouched - { - source: "git::http://example.com/registry/foo/notfound?param=value", - location: "git::http://example.com/registry/foo/notfound?param=value", - }, - - // local paths should be detected as such, even if they're match - // registry modules. - { - source: "./registry/foo/bar", - location: "file://" + filepath.Join(wd, "registry/foo/bar"), - }, - { - source: "/registry/foo/bar", - location: "file:///registry/foo/bar", - }, - - // Wrong number of parts can't be registry IDs. - // This is returned as a local path for now, but may return an error at - // some point. - { - source: "something/here/registry/foo/notfound", - location: "file://" + filepath.Join(wd, "something/here/registry/foo/notfound"), - }, - - // make sure a local module that looks like a registry id can be found - { - source: "namespace/identifier/provider", - fixture: "discover-subdirs", - err: true, - }, - - // The registry takes precedence over local paths if they don't start - // with a relative or absolute path - { - source: "exists-in-registry/identifier/provider", - fixture: "discover-registry-local", - // registry should take precidence - location: "file:///registry/exists", - }, - } { - - t.Run(tc.source, func(t *testing.T) { - dir := wd - if tc.fixture != "" { - dir = filepath.Join(wd, fixtureDir, tc.fixture) - if err := os.Chdir(dir); err != nil { - t.Fatal(err) - } - defer os.Chdir(wd) - } - - loc, err := getter.Detect(tc.source, dir, detectors) - if (err == nil) == tc.err { - t.Fatalf("expected error? %t; got error :%v", tc.err, err) - } - - loc = strings.TrimPrefix(loc, server.URL+"/") - if strings.TrimPrefix(loc, server.URL) != tc.location { - t.Fatalf("expected location: %q, got %q", tc.location, loc) - } - }) - - } -} - +/* +// FIXME: verifying the behavior in these tests is still important, so they +// need to be updated. +// // GitHub archives always contain the module source in a single subdirectory, // so the registry will return a path with with a `//*` suffix. We need to make // sure this doesn't intefere with our internal handling of `//` subdir. @@ -441,6 +251,7 @@ func TestRegisryModuleSubdir(t *testing.T) { t.Fatalf("got: \n\n%s\nexpected: \n\n%s", actual, expected) } } +*/ func TestAccRegistryDiscover(t *testing.T) { if os.Getenv("TF_ACC") == "" { @@ -448,7 +259,12 @@ func TestAccRegistryDiscover(t *testing.T) { } // simply check that we get a valid github URL for this from the registry - loc, err := getter.Detect("hashicorp/consul/aws", "./", detectors) + module, err := regsrc.ParseModuleSource("hashicorp/consul/aws") + if err != nil { + t.Fatal(err) + } + + loc, err := lookupModuleLocation(nil, module, "") if err != nil { t.Fatal(err) } diff --git a/config/module/registry.go b/config/module/registry.go index a1fef61619..012ef7e146 100644 --- a/config/module/registry.go +++ b/config/module/registry.go @@ -3,6 +3,7 @@ package module import ( "encoding/json" "fmt" + "io/ioutil" "log" "net/http" "net/url" @@ -28,13 +29,14 @@ const ( ) var ( - client *http.Client - tfVersion = version.String() + httpClient *http.Client + tfVersion = version.String() + regDisco = disco.NewDisco() ) func init() { - client = cleanhttp.DefaultPooledClient() - client.Timeout = requestTimeout + httpClient = cleanhttp.DefaultPooledClient() + httpClient.Timeout = requestTimeout } type errModuleNotFound string @@ -43,13 +45,16 @@ func (e errModuleNotFound) Error() string { return `module "` + string(e) + `" not found` } -// Lookup module versions in the registry. -func lookupModuleVersions(regDisco *disco.Disco, module *regsrc.Module) (*response.ModuleVersions, error) { +func discoverRegURL(d *disco.Disco, module *regsrc.Module) string { + if d == nil { + d = regDisco + } + if module.RawHost == nil { module.RawHost = regsrc.NewFriendlyHost(defaultRegistry) } - regURL := regDisco.DiscoverServiceURL(svchost.Hostname(module.RawHost.Normalized()), serviceID) + regURL := d.DiscoverServiceURL(svchost.Hostname(module.RawHost.Normalized()), serviceID) if regURL == nil { regURL = &url.URL{ Scheme: "https", @@ -64,7 +69,14 @@ func lookupModuleVersions(regDisco *disco.Disco, module *regsrc.Module) (*respon service += "/" } - location := fmt.Sprintf("%s%s/%s/%s/versions", service, module.RawNamespace, module.RawName, module.RawProvider) + return service +} + +// Lookup module versions in the registry. +func lookupModuleVersions(d *disco.Disco, module *regsrc.Module) (*response.ModuleVersions, error) { + service := discoverRegURL(d, module) + + location := fmt.Sprintf("%s%s/versions", service, module.Module()) log.Printf("[DEBUG] fetching module versions from %q", location) req, err := http.NewRequest("GET", location, nil) @@ -74,11 +86,15 @@ func lookupModuleVersions(regDisco *disco.Disco, module *regsrc.Module) (*respon req.Header.Set(xTerraformVersion, tfVersion) + if d == nil { + d = regDisco + } + // if discovery required a custom transport, then we should use that too - client := client - if regDisco.Transport != nil { + client := httpClient + if d.Transport != nil { client = &http.Client{ - Transport: regDisco.Transport, + Transport: d.Transport, Timeout: requestTimeout, } } @@ -107,3 +123,63 @@ func lookupModuleVersions(regDisco *disco.Disco, module *regsrc.Module) (*respon return &versions, nil } + +// lookup the location of a specific module version in the registry +func lookupModuleLocation(d *disco.Disco, module *regsrc.Module, version string) (string, error) { + service := discoverRegURL(d, module) + + var download string + if version == "" { + download = fmt.Sprintf("%s%s/download", service, module.Module()) + } else { + download = fmt.Sprintf("%s%s/%s/download", service, module.Module(), version) + } + + log.Printf("[DEBUG] looking up module location from %q", download) + + req, err := http.NewRequest("GET", download, nil) + if err != nil { + return "", err + } + + req.Header.Set(xTerraformVersion, tfVersion) + + // if discovery required a custom transport, then we should use that too + client := httpClient + if regDisco.Transport != nil { + client = &http.Client{ + Transport: regDisco.Transport, + Timeout: requestTimeout, + } + } + + resp, err := client.Do(req) + if err != nil { + return "", err + } + defer resp.Body.Close() + + // there should be no body, but save it for logging + body, err := ioutil.ReadAll(resp.Body) + if err != nil { + return "", fmt.Errorf("error reading response body from registry: %s", err) + } + + switch resp.StatusCode { + case http.StatusOK, http.StatusNoContent: + // OK + case http.StatusNotFound: + return "", fmt.Errorf("module %q version %q not found", module, version) + default: + // anything else is an error: + return "", fmt.Errorf("error getting download location for %q: %s resp:%s", module, resp.Status, body) + } + + // the download location is in the X-Terraform-Get header + location := resp.Header.Get(xTerraformGet) + if location == "" { + return "", fmt.Errorf("failed to get download URL for %q: %s resp:%s", module, resp.Status, body) + } + + return location, nil +} diff --git a/config/module/tree.go b/config/module/tree.go index e10dbec1a0..e3ea555fd1 100644 --- a/config/module/tree.go +++ b/config/module/tree.go @@ -270,7 +270,7 @@ func (t *Tree) Load(storage getter.Storage, mode GetMode) error { } } - source, err := getter.Detect(rawSource, t.config.Dir, detectors) + source, err := getter.Detect(rawSource, t.config.Dir, getter.Detectors) if err != nil { return fmt.Errorf("module %s: %s", m.Name, err) } diff --git a/registry/regsrc/module.go b/registry/regsrc/module.go index 05a7bfa32d..b6671c8a4a 100644 --- a/registry/regsrc/module.go +++ b/registry/regsrc/module.go @@ -132,6 +132,12 @@ func (m *Module) String() string { return m.formatWithPrefix(hostPrefix, true) } +// Module returns just the registry ID of the module, without a hostname or +// suffix. +func (m *Module) Module() string { + return fmt.Sprintf("%s/%s/%s", m.RawNamespace, m.RawName, m.RawProvider) +} + // Equal compares the module source against another instance taking // normalization into account. func (m *Module) Equal(other *Module) bool {