From f9fb2b4c9eb4cc348a89f08a494b162b6b039fd5 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 8 Sep 2017 18:18:47 -0400 Subject: [PATCH] move local module detection ahead of the registry The getter.FileDetector was intended to be the final detector, only converting a path to a file URL and returning a true in all cases. We want to check for a local module before checking the registry so no local modules that happen to match a registry module are broken. Wrap the getter.FileDetector to check the module source's existence before delegating the search to the registry. --- config/module/get.go | 46 +++++++++++-- config/module/get_test.go | 125 ++++++++++++++++++++++++++++++++--- config/module/module_test.go | 3 + 3 files changed, 159 insertions(+), 15 deletions(-) diff --git a/config/module/get.go b/config/module/get.go index e6eb1afbd..ba9d29259 100644 --- a/config/module/get.go +++ b/config/module/get.go @@ -5,6 +5,7 @@ import ( "io/ioutil" "log" "net/http" + "net/url" "os" "regexp" "strings" @@ -86,8 +87,8 @@ var detectors = []getter.Detector{ new(getter.GitHubDetector), new(getter.BitBucketDetector), new(getter.S3Detector), + new(localDetector), new(registryDetector), - new(getter.FileDetector), } // these prefixes can't be registry IDs @@ -121,8 +122,6 @@ func (d registryDetector) Detect(src, _ string) (string, bool, error) { } // Lookup the module in the registry. -// Since existing module sources may match a registry ID format, we only log -// registry errors and continue discovery. func (d registryDetector) lookupModule(src string) (string, bool, error) { if d.api == "" { d.api = registryAPI @@ -137,7 +136,7 @@ func (d registryDetector) lookupModule(src string) (string, bool, error) { // determine if it's truly valid. resp, err := d.client.Get(fmt.Sprintf("%s/%s/download", d.api, src)) if err != nil { - log.Println("[WARN] error looking up module %q: %s", src, err) + log.Printf("[WARN] error looking up module %q: %s", src, err) return "", false, nil } defer resp.Body.Close() @@ -145,7 +144,7 @@ func (d registryDetector) lookupModule(src string) (string, bool, error) { // there should be no body, but save it for logging body, err := ioutil.ReadAll(resp.Body) if err != nil { - fmt.Println("[WARN] error reading response body from registry: %s", err) + fmt.Printf("[WARN] error reading response body from registry: %s", err) return "", false, nil } @@ -169,3 +168,40 @@ func (d registryDetector) lookupModule(src string) (string, bool, error) { return location, true, nil } + +// localDetector wraps the default getter.FileDetector and checks if the module +// exists in the local filesystem. The default FileDetector only converts paths +// into file URLs, and returns found. We want to first check for a local module +// before passing it off to the registryDetector so we don't inadvertently +// replace a local module with a registry module of the same name. +type localDetector struct{} + +func (d localDetector) Detect(src, wd string) (string, bool, error) { + localSrc, ok, err := new(getter.FileDetector).Detect(src, wd) + if err != nil { + return src, ok, err + } + + if !ok { + return "", false, nil + } + + u, err := url.Parse(localSrc) + if err != nil { + return "", false, err + } + + _, err = os.Stat(u.Path) + + // just continue detection if it doesn't exist + if os.IsNotExist(err) { + return "", false, nil + } + + // return any other errors + if err != nil { + return "", false, err + } + + return localSrc, true, nil +} diff --git a/config/module/get_test.go b/config/module/get_test.go index 039c8b6b8..206d291bc 100644 --- a/config/module/get_test.go +++ b/config/module/get_test.go @@ -4,11 +4,14 @@ import ( "fmt" "net/http" "net/http/httptest" + "os" + "path/filepath" "regexp" "sort" "strings" "testing" + getter "github.com/hashicorp/go-getter" version "github.com/hashicorp/go-version" ) @@ -79,52 +82,52 @@ func TestDetectRegistry(t *testing.T) { } for _, tc := range []struct { - module string + source string location string found bool err bool }{ { - module: "registry/foo/bar", + source: "registry/foo/bar", location: "download/registry/foo/bar/0.2.3", found: true, }, { - module: "registry/foo/baz", + source: "registry/foo/baz", location: "download/registry/foo/baz/1.10.0", found: true, }, // this should not be found, but not stop detection { - module: "registry/foo/notfound", + source: "registry/foo/notfound", found: false, }, // a full url should not be detected { - module: "http://example.com/registry/foo/notfound", + source: "http://example.com/registry/foo/notfound", found: false, }, // paths should not be detected { - module: "./local/foo/notfound", + source: "./local/foo/notfound", found: false, }, { - module: "/local/foo/notfound", + source: "/local/foo/notfound", found: false, }, // wrong number of parts can't be regisry IDs { - module: "something/registry/foo/notfound", + source: "something/registry/foo/notfound", found: false, }, } { - t.Run(tc.module, func(t *testing.T) { - loc, ok, err := detector.Detect(tc.module, "") + 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) } @@ -141,3 +144,105 @@ func TestDetectRegistry(t *testing.T) { } } + +// check that the full set of detectors works as expected +func TestDetectors(t *testing.T) { + server := mockRegistry() + defer server.Close() + + wd, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + + regDetector := ®istryDetector{ + api: server.URL + "/v1/modules/", + client: server.Client(), + } + + detectors := []getter.Detector{ + new(getter.GitHubDetector), + new(getter.BitBucketDetector), + new(getter.S3Detector), + new(localDetector), + regDetector, + } + + for _, tc := range []struct { + source string + location string + fixture string + err bool + }{ + { + source: "registry/foo/bar", + location: "download/registry/foo/bar/0.2.3", + }, + // this should not be found, but not stop detection + { + 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", + err: true, + }, + { + source: "/registry/foo/bar", + err: true, + }, + + // wrong number of parts can't be regisry IDs + { + source: "something/registry/foo/notfound", + err: true, + }, + + // make sure a local module that looks like a registry id takes precedence + { + source: "namespace/identifier/provider", + fixture: "discover-subdirs", + // this should be found locally + location: "file://" + filepath.Join(wd, fixtureDir, "discover-subdirs/namespace/identifier/provider"), + }, + } { + + 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) + } + }) + + } +} diff --git a/config/module/module_test.go b/config/module/module_test.go index 89fee6ec5..99f5edad7 100644 --- a/config/module/module_test.go +++ b/config/module/module_test.go @@ -13,6 +13,7 @@ import ( const fixtureDir = "./test-fixtures" func tempDir(t *testing.T) string { + t.Helper() dir, err := ioutil.TempDir("", "tf") if err != nil { t.Fatalf("err: %s", err) @@ -25,6 +26,7 @@ func tempDir(t *testing.T) string { } func testConfig(t *testing.T, n string) *config.Config { + t.Helper() c, err := config.LoadDir(filepath.Join(fixtureDir, n)) if err != nil { t.Fatalf("err: %s", err) @@ -34,5 +36,6 @@ func testConfig(t *testing.T, n string) *config.Config { } func testStorage(t *testing.T) getter.Storage { + t.Helper() return &getter.FolderStorage{StorageDir: tempDir(t)} }