From 803d18ad55f34d0d4f1c3f89a520089befcc220b Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 23 Oct 2017 18:07:49 -0400 Subject: [PATCH 01/13] add more metadata to the Tree and the storage path Submodules were located by using their module path as the storage key. Now that modules may have versions, a submodule needs to know how to locate the corect source depending on the versions of its ancestors in the tree. Add a version field to each Tree, and a pointer back to the parent Tree to step back through the ancestors. The new versionedPathKey method uses this information to build a unique key for each module, dependent on the ancestor versions. Not only do stored modules need to know their version if it exists, but any relative source needs to know all the ancestor versions in order to resolve correctly. --- config/module/storage.go | 45 +++++++++++- config/module/tree.go | 154 +++++++++++++++++++++++++++++++-------- 2 files changed, 163 insertions(+), 36 deletions(-) diff --git a/config/module/storage.go b/config/module/storage.go index d701cd9d4c18..2873e61efe97 100644 --- a/config/module/storage.go +++ b/config/module/storage.go @@ -23,12 +23,14 @@ type moduleManifest struct { // This is compared for equality using '==', so all fields needs to remain // comparable. type moduleRecord struct { - // Source is the module source string, minus any subdirectory. - // If it is sourced from a registry, it will include the hostname if it is - // supplied in configuration. + // Source is the module source string from the config, minus any + // subdirectory. Source string - // Version is the exact version string that is stored in this Key. + // Key is the locally unique identifier for this module. + Key string + + // Version is the exact version string for the stored module. Version string // Dir is the directory name returned by the FileStorage. This is what @@ -138,6 +140,41 @@ func (m moduleStorage) recordModule(rec moduleRecord) error { return ioutil.WriteFile(manifestPath, js, 0644) } +// load the manifest from dir, and return all module versions matching the +// provided source. Records with no version info will be skipped, as they need +// to be uniquely identified by other means. +func (m moduleStorage) moduleVersions(source string) ([]moduleRecord, error) { + manifest, err := m.loadManifest() + if err != nil { + return manifest.Modules, err + } + + var matching []moduleRecord + + for _, m := range manifest.Modules { + if m.Source == source && m.Version != "" { + matching = append(matching, m) + } + } + + return matching, nil +} + +func (m moduleStorage) moduleDir(key string) (string, error) { + manifest, err := m.loadManifest() + if err != nil { + return "", err + } + + for _, m := range manifest.Modules { + if m.Key == key { + return m.Dir, nil + } + } + + return "", nil +} + // return only the root directory of the module stored in dir. func (m moduleStorage) getModuleRoot(dir string) (string, error) { manifest, err := m.loadManifest() diff --git a/config/module/tree.go b/config/module/tree.go index 9fe29f1a7d95..691c5fab5438 100644 --- a/config/module/tree.go +++ b/config/module/tree.go @@ -27,6 +27,17 @@ type Tree struct { children map[string]*Tree path []string lock sync.RWMutex + + // version is the final version of the config loaded for the Tree's module + version string + // source is the "source" string used to load this module. It's possible + // for a module source to change, but the path remains the same, preventing + // it from being reloaded. + source string + // parent allows us to walk back up the tree and determine if there are any + // versioned ancestor modules which may effect the stored location of + // submodules + parent *Tree } // NewTree returns a new Tree for the given config structure. @@ -174,7 +185,6 @@ func (t *Tree) Load(storage getter.Storage, mode GetMode) error { // Go through all the modules and get the directory for them. for _, m := range modules { - if _, ok := children[m.Name]; ok { return fmt.Errorf( "module %s: duplicated. module names must be unique", m.Name) @@ -185,24 +195,53 @@ func (t *Tree) Load(storage getter.Storage, mode GetMode) error { copy(path, t.path) path = append(path, m.Name) - // The key is the string that will be hashed to uniquely id the Source. - // The leading digit can be incremented to force re-fetch all existing - // modules. - key := fmt.Sprintf("1.root.%s-%s", strings.Join(path, "."), m.Source) - log.Printf("[TRACE] module source: %q", m.Source) // Split out the subdir if we have one. - // Terraform keeps the entire requested tree for now, so that modules can + // Terraform keeps the entire requested tree, so that modules can // reference sibling modules from the same archive or repo. - source, subDir := getter.SourceDirSubdir(m.Source) + rawSource, subDir := getter.SourceDirSubdir(m.Source) + + // The key is the string that will be used to uniquely id the Source in + // the local storage. The prefix digit can be incremented to + // invalidate the local module storage. + key := "1." + t.versionedPathKey(m) + + // we can't calculate a key without a version, so lookup if we have any + // matching modules stored. + var dir, version string + var found bool + // only registry modules have a version, and only full URLs are globally unique + + // TODO: This needs to only check for registry modules, and lookup + // versions if we don't find them here. Don't continue on as if + // a registry identifier could be some other source. + if mode != GetModeUpdate { + versions, err := s.moduleVersions(rawSource) + if err != nil { + log.Println("[ERROR] error looking up versions for %q: %s", m.Source, err) + return err + } - // First check if we we need to download anything. - // This is also checked by the getter.Storage implementation, but we - // want to be able to short-circuit the detection as well, since some - // detectors may need to make external calls. - dir, found, err := s.Dir(key) - if err != nil { - return err + match, err := newestRecord(versions, m.Version) + if err != nil { + // not everything has a recorded version, or a constraint, so just log this + log.Printf("[INFO] no matching version for %q<%s>, %s", m.Source, m.Version, err) + } + + dir = match.Dir + version = match.Version + found = dir != "" + } + + // It wasn't a versioned module, check for the exact key. + // This replaces the Storgae.Dir method with our manifest lookup. + var err error + if !found { + dir, err = s.moduleDir(key) + if err != nil { + return err + } + found = dir != "" } // looks like we already have it @@ -212,25 +251,28 @@ func (t *Tree) Load(storage getter.Storage, mode GetMode) error { subDir, err := s.getModuleRoot(dir) if err != nil { // If there's a problem with the subdir record, we'll let the - // recordSubdir method fix it up. Any other errors filesystem - // errors will turn up again below. + // recordSubdir method fix it up. Any other filesystem errors + // will turn up again below. log.Println("[WARN] error reading subdir record:", err) } else { dir := filepath.Join(dir, subDir) // Load the configurations.Dir(source) - children[m.Name], err = NewTreeModule(m.Name, dir) + child, err := NewTreeModule(m.Name, dir) if err != nil { return fmt.Errorf("module %s: %s", m.Name, err) } - // Set the path of this child - children[m.Name].path = path + child.path = path + child.parent = t + child.version = version + child.source = m.Source + children[m.Name] = child continue } } - log.Printf("[TRACE] module source: %q", source) + log.Printf("[TRACE] module source: %q", rawSource) - source, err = getter.Detect(source, t.config.Dir, detectors) + source, err := getter.Detect(rawSource, t.config.Dir, detectors) if err != nil { return fmt.Errorf("module %s: %s", m.Name, err) } @@ -257,9 +299,12 @@ func (t *Tree) Load(storage getter.Storage, mode GetMode) error { "module %s: not found, may need to be downloaded using 'terraform get'", m.Name) } + log.Printf("[TRACE] %q stored in %q", source, dir) + // expand and record the subDir for later + fullDir := dir if subDir != "" { - fullDir, err := getter.SubdirGlob(dir, subDir) + fullDir, err = getter.SubdirGlob(dir, subDir) if err != nil { return err } @@ -268,22 +313,29 @@ func (t *Tree) Load(storage getter.Storage, mode GetMode) error { if len(dir)+1 > len(fullDir) { return fmt.Errorf("invalid module storage path %q", fullDir) } - subDir = fullDir[len(dir)+1:] + } - if err := s.recordModuleRoot(dir, subDir); err != nil { - return err - } - dir = fullDir + rec := moduleRecord{ + Source: m.Source, + Key: key, + Dir: dir, + Root: subDir, } - // Load the configurations.Dir(source) - children[m.Name], err = NewTreeModule(m.Name, dir) + if err := s.recordModule(rec); err != nil { + return err + } + + child, err := NewTreeModule(m.Name, fullDir) if err != nil { return fmt.Errorf("module %s: %s", m.Name, err) } - // Set the path of this child - children[m.Name].path = path + child.path = path + child.parent = t + child.version = version + child.source = m.Source + children[m.Name] = child } // Go through all the children and load them. @@ -600,6 +652,44 @@ func (t *Tree) Validate() error { return newErr.ErrOrNil() } +// versionedPathKey returns a path string with every levels full name, version +// and source encoded. This is to provide a unique key for our module storage, +// since submodules need to know which versions of their ancestor modules they +// are loaded from. +func (t *Tree) versionedPathKey(m *Module) string { + path := make([]string, len(t.path)+1) + path[len(path)-1] = m.Name + // We're going to load these in order for easier reading and debugging, but + // in practice they only need to be unique and consistent. + + p := t + i := len(path) - 2 + for ; i >= 0; i-- { + if p == nil { + break + } + // we may have been loaded under a blank Tree, so always check for a name + // too. + if p.name == "" { + break + } + seg := p.name + if p.version != "" { + seg += "#" + p.version + } + + if p.source != "" { + seg += ";" + p.source + } + + path[i] = seg + p = p.parent + } + + key := strings.Join(path, "|") + return key +} + // treeError is an error use by Tree.Validate to accumulates all // validation errors. type treeError struct { From 8d8f77c094de90e4b0d41b9d4cc3c29b8d89fab2 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 19 Oct 2017 16:32:19 -0400 Subject: [PATCH 02/13] Use the new regsrc and response packages Adds basic detector for registry module source strings. While this isn't a thorough validation, this will eliminate anything that is definitely not a registry module, and split out our host and module id strings. lookupModuleVersions interrogates the registry for the available versions of a particular module and the tree of dependencies. --- config/module/detector_test.go | 112 +++++++++++++++++++++++++++++++++ config/module/get.go | 7 +-- config/module/get_test.go | 2 +- config/module/registry.go | 95 ++++++++++++++++++++++++++++ config/module/versions.go | 95 ++++++++++++++++++++++++++++ config/module/versions_test.go | 60 ++++++++++++++++++ 6 files changed, 366 insertions(+), 5 deletions(-) create mode 100644 config/module/detector_test.go create mode 100644 config/module/registry.go create mode 100644 config/module/versions.go create mode 100644 config/module/versions_test.go diff --git a/config/module/detector_test.go b/config/module/detector_test.go new file mode 100644 index 000000000000..10cc057fcc4f --- /dev/null +++ b/config/module/detector_test.go @@ -0,0 +1,112 @@ +package module + +import ( + "fmt" + "testing" + + "github.com/hashicorp/terraform/registry/regsrc" +) + +func TestParseRegistrySource(t *testing.T) { + for _, tc := range []struct { + source string + host string + id string + err bool + notRegistry bool + }{ + { // simple source id + source: "namespace/id/provider", + id: "namespace/id/provider", + }, + { // source with hostname + source: "registry.com/namespace/id/provider", + host: "registry.com", + id: "namespace/id/provider", + }, + { // source with hostname and port + source: "registry.com:4443/namespace/id/provider", + host: "registry.com:4443", + id: "namespace/id/provider", + }, + { // too many parts + source: "registry.com/namespace/id/provider/extra", + notRegistry: true, + }, + { // local path + source: "./local/file/path", + notRegistry: true, + }, + { // local path with hostname + source: "./registry.com/namespace/id/provider", + notRegistry: true, + }, + { // full URL + source: "https://example.com/foo/bar/baz", + notRegistry: true, + }, + { // punycode host not allowed in source + source: "xn--80akhbyknj4f.com/namespace/id/provider", + err: true, + }, + { // simple source id with subdir + source: "namespace/id/provider//subdir", + id: "namespace/id/provider", + }, + { // source with hostname and subdir + source: "registry.com/namespace/id/provider//subdir", + host: "registry.com", + id: "namespace/id/provider", + }, + { // source with hostname + source: "registry.com/namespace/id/provider", + host: "registry.com", + id: "namespace/id/provider", + }, + { // we special case github + source: "github.com/namespace/id/provider", + notRegistry: true, + }, + { // we special case github ssh + source: "git@github.com:namespace/id/provider", + notRegistry: true, + }, + { // we special case bitbucket + source: "bitbucket.org/namespace/id/provider", + notRegistry: true, + }, + } { + t.Run(tc.source, func(t *testing.T) { + mod, err := regsrc.ParseModuleSource(tc.source) + if tc.notRegistry { + if err != regsrc.ErrInvalidModuleSource { + t.Fatalf("%q should not be a registry source, got err %v", tc.source, err) + } + return + } + + if tc.err { + if err == nil { + t.Fatal("expected error") + } + return + } + + if err != nil { + t.Fatal(err) + } + + id := fmt.Sprintf("%s/%s/%s", mod.RawNamespace, mod.RawName, mod.RawProvider) + + if tc.host != "" { + if mod.RawHost.Normalized() != tc.host { + t.Fatalf("expected host %q, got %q", tc.host, mod.RawHost) + } + } + + if tc.id != id { + t.Fatalf("expected id %q, got %q", tc.id, id) + } + }) + } +} diff --git a/config/module/get.go b/config/module/get.go index ac078666d8e4..26a31fa8456f 100644 --- a/config/module/get.go +++ b/config/module/get.go @@ -65,8 +65,7 @@ func GetCopy(dst, src string) error { } const ( - registryAPI = "https://registry.terraform.io/v1/modules" - xTerraformGet = "X-Terraform-Get" + registryAPI = "https://registry.terraform.io/v1/modules" ) var detectors = []getter.Detector{ @@ -79,7 +78,7 @@ var detectors = []getter.Detector{ // these prefixes can't be registry IDs // "http", "../", "./", "/", "getter::", etc -var skipRegistry = regexp.MustCompile(`^(http|[.]{1,2}/|/|[A-Za-z0-9]+::)`).MatchString +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 @@ -95,7 +94,7 @@ type registryDetector struct { 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 skipRegistry(src) { + if oldSkipRegistry(src) { return "", false, nil } diff --git a/config/module/get_test.go b/config/module/get_test.go index b73520c43e6a..de2a13bab572 100644 --- a/config/module/get_test.go +++ b/config/module/get_test.go @@ -91,7 +91,7 @@ func mockRegistry() *httptest.Server { location = fmt.Sprintf("file://%s/%s", wd, location) } - w.Header().Set(xTerraformGet, location) + w.Header().Set("X-Terraform-Get", location) w.WriteHeader(http.StatusNoContent) // no body return diff --git a/config/module/registry.go b/config/module/registry.go new file mode 100644 index 000000000000..2753871944fb --- /dev/null +++ b/config/module/registry.go @@ -0,0 +1,95 @@ +package module + +import ( + "encoding/json" + "fmt" + "log" + "net/http" + "net/url" + "time" + + cleanhttp "github.com/hashicorp/go-cleanhttp" + + "github.com/hashicorp/terraform/registry/regsrc" + "github.com/hashicorp/terraform/registry/response" + "github.com/hashicorp/terraform/svchost" + "github.com/hashicorp/terraform/svchost/disco" + "github.com/hashicorp/terraform/version" +) + +const ( + defaultRegistry = "registry.terraform.io" + defaultApiPath = "/v1/modules" + registryServiceID = "registry.v1" + xTerraformGet = "X-Terraform-Get" + xTerraformVersion = "X-Terraform-Version" + requestTimeout = 10 * time.Second + serviceID = "modules.v1" +) + +var ( + client *http.Client + tfVersion = version.String() + regDisco = disco.NewDisco() +) + +func init() { + client = cleanhttp.DefaultPooledClient() + client.Timeout = requestTimeout +} + +type errModuleNotFound string + +func (e errModuleNotFound) Error() string { + return `module "` + string(e) + `" not found` +} + +// Lookup module versions in the registry. +func lookupModuleVersions(module *regsrc.Module) (*response.ModuleVersions, error) { + if module.RawHost == nil { + module.RawHost = regsrc.NewFriendlyHost(defaultRegistry) + } + + regUrl := regDisco.DiscoverServiceURL(svchost.Hostname(module.RawHost.Normalized()), serviceID) + if regUrl == nil { + regUrl = &url.URL{ + Scheme: "https", + Host: module.RawHost.String(), + Path: defaultApiPath, + } + } + + location := fmt.Sprintf("%s/%s/%s/%s/versions", regUrl, module.RawNamespace, module.RawName, module.RawProvider) + log.Printf("[DEBUG] fetching module versions from %q", location) + + req, err := http.NewRequest("GET", location, nil) + if err != nil { + return nil, err + } + + req.Header.Set(xTerraformVersion, tfVersion) + + resp, err := client.Do(req) + if err != nil { + return nil, err + } + defer resp.Body.Close() + + switch resp.StatusCode { + case http.StatusOK: + // OK + case http.StatusNotFound: + return nil, errModuleNotFound(module.String()) + default: + return nil, fmt.Errorf("error looking up module versions: %s", resp.Status) + } + + var versions response.ModuleVersions + + dec := json.NewDecoder(resp.Body) + if err := dec.Decode(&versions); err != nil { + return nil, err + } + + return &versions, nil +} diff --git a/config/module/versions.go b/config/module/versions.go new file mode 100644 index 000000000000..8348d4b19535 --- /dev/null +++ b/config/module/versions.go @@ -0,0 +1,95 @@ +package module + +import ( + "errors" + "fmt" + "sort" + + version "github.com/hashicorp/go-version" + "github.com/hashicorp/terraform/registry/response" +) + +const anyVersion = ">=0.0.0" + +// return the newest version that satisfies the provided constraint +func newest(versions []string, constraint string) (string, error) { + if constraint == "" { + constraint = anyVersion + } + cs, err := version.NewConstraint(constraint) + if err != nil { + return "", err + } + + switch len(versions) { + case 0: + return "", errors.New("no versions found") + case 1: + v, err := version.NewVersion(versions[0]) + if err != nil { + return "", err + } + + if !cs.Check(v) { + return "", fmt.Errorf("no version found matching constraint %q", constraint) + } + return versions[0], nil + } + + sort.Slice(versions, func(i, j int) bool { + // versions should have already been validated + // sort invalid version strings to the end + iv, err := version.NewVersion(versions[i]) + if err != nil { + return true + } + jv, err := version.NewVersion(versions[j]) + if err != nil { + return true + } + return iv.GreaterThan(jv) + }) + + // versions are now in order, so just find the first which satisfies the + // constraint + for i := range versions { + v, err := version.NewVersion(versions[i]) + if err != nil { + continue + } + if cs.Check(v) { + return versions[i], nil + } + } + + return "", nil +} + +// return the newest *moduleVersion that matches the given constraint +// TODO: reconcile these two types and newest* functions +func newestVersion(moduleVersions []*response.ModuleVersion, constraint string) (*response.ModuleVersion, error) { + var versions []string + modules := make(map[string]*response.ModuleVersion) + + for _, m := range moduleVersions { + versions = append(versions, m.Version) + modules[m.Version] = m + } + + match, err := newest(versions, constraint) + return modules[match], err +} + +// return the newest moduleRecord that matches the given constraint +func newestRecord(moduleVersions []moduleRecord, constraint string) (moduleRecord, error) { + var versions []string + modules := make(map[string]moduleRecord) + + for _, m := range moduleVersions { + versions = append(versions, m.Version) + modules[m.Version] = m + } + + match, err := newest(versions, constraint) + return modules[match], err +} diff --git a/config/module/versions_test.go b/config/module/versions_test.go new file mode 100644 index 000000000000..b7ff6e6271e9 --- /dev/null +++ b/config/module/versions_test.go @@ -0,0 +1,60 @@ +package module + +import ( + "testing" + + "github.com/hashicorp/terraform/registry/response" +) + +func TestNewestModuleVersion(t *testing.T) { + mpv := &response.ModuleProviderVersions{ + Source: "registry/test/module", + Versions: []*response.ModuleVersion{ + {Version: "0.0.4"}, + {Version: "0.3.1"}, + {Version: "2.0.1"}, + {Version: "1.2.0"}, + }, + } + + m, err := newestVersion(mpv.Versions, "") + if err != nil { + t.Fatal(err) + } + + expected := "2.0.1" + if m.Version != expected { + t.Fatalf("expected version %q, got %q", expected, m.Version) + } + + // now with a constraint + m, err = newestVersion(mpv.Versions, "~>1.0") + if err != nil { + t.Fatal(err) + } + + expected = "1.2.0" + if m.Version != expected { + t.Fatalf("expected version %q, got %q", expected, m.Version) + } +} + +func TestNewestInvalidModuleVersion(t *testing.T) { + mpv := &response.ModuleProviderVersions{ + Source: "registry/test/module", + Versions: []*response.ModuleVersion{ + {Version: "WTF"}, + {Version: "2.0.1"}, + }, + } + + m, err := newestVersion(mpv.Versions, "") + if err != nil { + t.Fatal(err) + } + + expected := "2.0.1" + if m.Version != expected { + t.Fatalf("expected version %q, got %q", expected, m.Version) + } +} From 9fdf0e13c4dcc44601e59c015fa697ef2bdf6516 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 24 Oct 2017 16:00:42 -0400 Subject: [PATCH 03/13] failing test for changing intermediate modules This test highlights how changing an intermediate source path prevents reloading of submodules. While this is somewhat of an edge case now, it becomes quite common in the cacse where module versions are updated. --- .../change-intermediate-source/a/b/main.tf | 1 + .../change-intermediate-source/a/main.tf | 3 + .../change-intermediate-source/c/b/main.tf | 1 + .../change-intermediate-source/c/main.tf | 3 + .../change-intermediate-source/main.tf | 3 + .../main.tf.disabled | 3 + config/module/tree_test.go | 64 +++++++++++++++++++ 7 files changed, 78 insertions(+) create mode 100644 config/module/test-fixtures/change-intermediate-source/a/b/main.tf create mode 100644 config/module/test-fixtures/change-intermediate-source/a/main.tf create mode 100644 config/module/test-fixtures/change-intermediate-source/c/b/main.tf create mode 100644 config/module/test-fixtures/change-intermediate-source/c/main.tf create mode 100644 config/module/test-fixtures/change-intermediate-source/main.tf create mode 100644 config/module/test-fixtures/change-intermediate-source/main.tf.disabled diff --git a/config/module/test-fixtures/change-intermediate-source/a/b/main.tf b/config/module/test-fixtures/change-intermediate-source/a/b/main.tf new file mode 100644 index 000000000000..b91fad57a43c --- /dev/null +++ b/config/module/test-fixtures/change-intermediate-source/a/b/main.tf @@ -0,0 +1 @@ +resource "test_resource" "a-b" {} diff --git a/config/module/test-fixtures/change-intermediate-source/a/main.tf b/config/module/test-fixtures/change-intermediate-source/a/main.tf new file mode 100644 index 000000000000..acbf03927ea4 --- /dev/null +++ b/config/module/test-fixtures/change-intermediate-source/a/main.tf @@ -0,0 +1,3 @@ +module "b" { + source = "./b" +} diff --git a/config/module/test-fixtures/change-intermediate-source/c/b/main.tf b/config/module/test-fixtures/change-intermediate-source/c/b/main.tf new file mode 100644 index 000000000000..5a3565b33f61 --- /dev/null +++ b/config/module/test-fixtures/change-intermediate-source/c/b/main.tf @@ -0,0 +1 @@ +resource "test_resource" "c-b" {} diff --git a/config/module/test-fixtures/change-intermediate-source/c/main.tf b/config/module/test-fixtures/change-intermediate-source/c/main.tf new file mode 100644 index 000000000000..acbf03927ea4 --- /dev/null +++ b/config/module/test-fixtures/change-intermediate-source/c/main.tf @@ -0,0 +1,3 @@ +module "b" { + source = "./b" +} diff --git a/config/module/test-fixtures/change-intermediate-source/main.tf b/config/module/test-fixtures/change-intermediate-source/main.tf new file mode 100644 index 000000000000..2326ee22acca --- /dev/null +++ b/config/module/test-fixtures/change-intermediate-source/main.tf @@ -0,0 +1,3 @@ +module "a" { + source = "./a" +} diff --git a/config/module/test-fixtures/change-intermediate-source/main.tf.disabled b/config/module/test-fixtures/change-intermediate-source/main.tf.disabled new file mode 100644 index 000000000000..52dabca93346 --- /dev/null +++ b/config/module/test-fixtures/change-intermediate-source/main.tf.disabled @@ -0,0 +1,3 @@ +module "a" { + source = "./c" +} diff --git a/config/module/tree_test.go b/config/module/tree_test.go index 705f90859a92..26a172977502 100644 --- a/config/module/tree_test.go +++ b/config/module/tree_test.go @@ -724,7 +724,71 @@ func TestTreeLoad_conflictingSubmoduleNames(t *testing.T) { t.Fatal("found wrong resource in b/c:", res.Name) } } + } + } +} + +// changing the source for a module but not the module "path" +func TestTreeLoad_changeIntermediateSource(t *testing.T) { + // copy the config to our tempdir this time, since we're going to edit it + td, err := ioutil.TempDir("", "tf") + if err != nil { + t.Fatalf("err: %s", err) + } + defer os.RemoveAll(td) + + if err := copyDir(td, filepath.Join(fixtureDir, "change-intermediate-source")); err != nil { + t.Fatal(err) + } + + wd, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + if err := os.Chdir(td); err != nil { + t.Fatal(err) + } + defer os.Chdir(wd) + + if err := os.MkdirAll(".terraform/modules", 0777); err != nil { + t.Fatal(err) + } + storage := &getter.FolderStorage{StorageDir: ".terraform/modules"} + cfg, err := config.LoadDir("./") + if err != nil { + t.Fatal(err) + } + tree := NewTree("", cfg) + if err := tree.Load(storage, GetModeGet); err != nil { + t.Fatalf("load failed: %s", err) + } + + // now we change the source of our module, without changing its path + if err := os.Rename("main.tf.disabled", "main.tf"); err != nil { + t.Fatal(err) + } + + // reload the tree + cfg, err = config.LoadDir("./") + if err != nil { + t.Fatal(err) + } + tree = NewTree("", cfg) + if err := tree.Load(storage, GetModeGet); err != nil { + t.Fatalf("load failed: %s", err) + } + // check for our resource in b + for _, c := range tree.Children() { + for _, gc := range c.Children() { + if len(gc.config.Resources) != 1 { + t.Fatalf("expected 1 resource in %s, got %d", gc.name, len(gc.config.Resources)) + } + res := gc.config.Resources[0] + expected := "c-b" + if res.Name != expected { + t.Fatalf("expexted resource %q, got %q", expected, res.Name) + } } } } From 750b46f7fbfe0fe4ce8d04e9727d24ac80739db9 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 24 Oct 2017 17:08:36 -0400 Subject: [PATCH 04/13] missing current module source in versionedPathKey The reason versionedPathKey didn't solve the previous test case was a missing source string for the current module. --- config/module/tree.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/config/module/tree.go b/config/module/tree.go index 691c5fab5438..e10dbec1a047 100644 --- a/config/module/tree.go +++ b/config/module/tree.go @@ -270,8 +270,6 @@ func (t *Tree) Load(storage getter.Storage, mode GetMode) error { } } - log.Printf("[TRACE] module source: %q", rawSource) - source, err := getter.Detect(rawSource, t.config.Dir, detectors) if err != nil { return fmt.Errorf("module %s: %s", m.Name, err) @@ -658,7 +656,7 @@ func (t *Tree) Validate() error { // are loaded from. func (t *Tree) versionedPathKey(m *Module) string { path := make([]string, len(t.path)+1) - path[len(path)-1] = m.Name + path[len(path)-1] = m.Name + ";" + m.Source // We're going to load these in order for easier reading and debugging, but // in practice they only need to be unique and consistent. From 439dcac608339acdfa7e11dc7d029fbed0135437 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 25 Oct 2017 11:49:41 -0400 Subject: [PATCH 05/13] a series of test commits wire up HTTP so we can test the mock discovery service test lookupModuleVersions Add a versions endpoint to the mock registry, and use that to verify the lookupModuleVersions behavior. lookupModuleVersions takes a Disco as the argument so a custom Transport can be injected, and uses that transport for its own client if it set. test looking up modules with default registry Add registry.terrform.io to the hostname that the mock registry resolves to localhost. ACC test looking up module versions Lookup a basic module for the available version in the default registry. --- config/module/get_test.go | 150 ++++++++++++++++++++++++-------- config/module/registry.go | 26 ++++-- config/module/registry_test.go | 152 +++++++++++++++++++++++++++++++++ 3 files changed, 286 insertions(+), 42 deletions(-) create mode 100644 config/module/registry_test.go diff --git a/config/module/get_test.go b/config/module/get_test.go index de2a13bab572..6b94ac372535 100644 --- a/config/module/get_test.go +++ b/config/module/get_test.go @@ -1,7 +1,9 @@ package module import ( + "encoding/json" "fmt" + "io" "net/http" "net/http/httptest" "net/url" @@ -14,6 +16,7 @@ import ( getter "github.com/hashicorp/go-getter" version "github.com/hashicorp/go-version" + "github.com/hashicorp/terraform/registry/response" ) // Map of module names and location of test modules. @@ -26,22 +29,28 @@ type testMod struct { // All the locationes from the mockRegistry start with a file:// scheme. If // the the location string here doesn't have a scheme, the mockRegistry will // find the absolute path and return a complete URL. -var testMods = map[string]testMod{ - "registry/foo/bar": { +var testMods = map[string][]testMod{ + "registry/foo/bar": {{ location: "file:///download/registry/foo/bar/0.2.3//*?archive=tar.gz", version: "0.2.3", - }, - "registry/foo/baz": { + }}, + "registry/foo/baz": {{ location: "file:///download/registry/foo/baz/1.10.0//*?archive=tar.gz", version: "1.10.0", - }, - "registry/local/sub": { + }}, + "registry/local/sub": {{ location: "test-fixtures/registry-tar-subdir/foo.tgz//*?archive=tar.gz", version: "0.1.2", - }, - "exists-in-registry/identifier/provider": { + }}, + "exists-in-registry/identifier/provider": {{ location: "file:///registry/exists", version: "0.2.0", + }}, + "test-versions/name/provider": { + {version: "2.2.0"}, + {version: "2.1.1"}, + {version: "1.2.2"}, + {version: "1.2.1"}, }, } @@ -59,45 +68,114 @@ func latestVersion(versions []string) string { return col[len(col)-1].String() } -// Just enough like a registry to exercise our code. -// Returns the location of the latest version -func mockRegistry() *httptest.Server { +func mockRegHandler() http.Handler { mux := http.NewServeMux() - server := httptest.NewServer(mux) + + download := func(w http.ResponseWriter, r *http.Request) { + p := strings.TrimLeft(r.URL.Path, "/") + // handle download request + re := regexp.MustCompile(`^([-a-z]+/\w+/\w+)/download$`) + // download lookup + matches := re.FindStringSubmatch(p) + if len(matches) != 2 { + w.WriteHeader(http.StatusBadRequest) + return + } + + versions, ok := testMods[matches[1]] + if !ok { + http.NotFound(w, r) + return + } + mod := versions[0] + + location := mod.location + if !strings.HasPrefix(location, "file:///") { + // we can't use filepath.Abs because it will clean `//` + wd, _ := os.Getwd() + location = fmt.Sprintf("file://%s/%s", wd, location) + } + + w.Header().Set("X-Terraform-Get", location) + w.WriteHeader(http.StatusNoContent) + // no body + return + } + + versions := func(w http.ResponseWriter, r *http.Request) { + p := strings.TrimLeft(r.URL.Path, "/") + re := regexp.MustCompile(`^([-a-z]+/\w+/\w+)/versions$`) + matches := re.FindStringSubmatch(p) + if len(matches) != 2 { + w.WriteHeader(http.StatusBadRequest) + return + } + + name := matches[1] + versions, ok := testMods[name] + if !ok { + http.NotFound(w, r) + return + } + + // only adding the single requested module for now + // this is the minimal that any regisry is epected to support + mpvs := &response.ModuleProviderVersions{ + Source: name, + } + + for _, v := range versions { + mv := &response.ModuleVersion{ + Version: v.version, + } + mpvs.Versions = append(mpvs.Versions, mv) + } + + resp := response.ModuleVersions{ + Modules: []*response.ModuleProviderVersions{mpvs}, + } + + js, err := json.Marshal(resp) + if err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + w.Header().Set("Content-Type", "application/json") + w.Write(js) + } mux.Handle("/v1/modules/", http.StripPrefix("/v1/modules/", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - p := strings.TrimLeft(r.URL.Path, "/") - // handle download request - download := regexp.MustCompile(`^([-a-z]+/\w+/\w+)/download$`) - - // download lookup - matches := download.FindStringSubmatch(p) - if len(matches) != 2 { - w.WriteHeader(http.StatusBadRequest) + if strings.HasSuffix(r.URL.Path, "/download") { + download(w, r) return } - mod, ok := testMods[matches[1]] - if !ok { - w.WriteHeader(http.StatusNotFound) + if strings.HasSuffix(r.URL.Path, "/versions") { + versions(w, r) return } - location := mod.location - if !strings.HasPrefix(location, "file:///") { - // we can't use filepath.Abs because it will clean `//` - wd, _ := os.Getwd() - location = fmt.Sprintf("file://%s/%s", wd, location) - } - - w.Header().Set("X-Terraform-Get", location) - w.WriteHeader(http.StatusNoContent) - // no body - return + http.NotFound(w, r) })), ) + mux.HandleFunc("/.well-known/terraform.json", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + io.WriteString(w, `{"modules.v1":"/v1/modules/"}`) + }) + return mux +} + +// Just enough like a registry to exercise our code. +// Returns the location of the latest version +func mockRegistry() *httptest.Server { + server := httptest.NewServer(mockRegHandler()) + return server +} + +func mockTLSRegistry() *httptest.Server { + server := httptest.NewTLSServer(mockRegHandler()) return server } @@ -138,12 +216,12 @@ func TestDetectRegistry(t *testing.T) { }{ { source: "registry/foo/bar", - location: testMods["registry/foo/bar"].location, + location: testMods["registry/foo/bar"][0].location, found: true, }, { source: "registry/foo/baz", - location: testMods["registry/foo/baz"].location, + location: testMods["registry/foo/baz"][0].location, found: true, }, // this should not be found, and is no longer valid as a local source diff --git a/config/module/registry.go b/config/module/registry.go index 2753871944fb..a1fef61619b0 100644 --- a/config/module/registry.go +++ b/config/module/registry.go @@ -30,7 +30,6 @@ const ( var ( client *http.Client tfVersion = version.String() - regDisco = disco.NewDisco() ) func init() { @@ -45,21 +44,27 @@ func (e errModuleNotFound) Error() string { } // Lookup module versions in the registry. -func lookupModuleVersions(module *regsrc.Module) (*response.ModuleVersions, error) { +func lookupModuleVersions(regDisco *disco.Disco, module *regsrc.Module) (*response.ModuleVersions, error) { if module.RawHost == nil { module.RawHost = regsrc.NewFriendlyHost(defaultRegistry) } - regUrl := regDisco.DiscoverServiceURL(svchost.Hostname(module.RawHost.Normalized()), serviceID) - if regUrl == nil { - regUrl = &url.URL{ + regURL := regDisco.DiscoverServiceURL(svchost.Hostname(module.RawHost.Normalized()), serviceID) + if regURL == nil { + regURL = &url.URL{ Scheme: "https", Host: module.RawHost.String(), Path: defaultApiPath, } } - location := fmt.Sprintf("%s/%s/%s/%s/versions", regUrl, module.RawNamespace, module.RawName, module.RawProvider) + service := regURL.String() + + if service[len(service)-1] != '/' { + service += "/" + } + + location := fmt.Sprintf("%s%s/%s/%s/versions", service, module.RawNamespace, module.RawName, module.RawProvider) log.Printf("[DEBUG] fetching module versions from %q", location) req, err := http.NewRequest("GET", location, nil) @@ -69,6 +74,15 @@ func lookupModuleVersions(module *regsrc.Module) (*response.ModuleVersions, erro req.Header.Set(xTerraformVersion, tfVersion) + // if discovery required a custom transport, then we should use that too + client := client + if regDisco.Transport != nil { + client = &http.Client{ + Transport: regDisco.Transport, + Timeout: requestTimeout, + } + } + resp, err := client.Do(req) if err != nil { return nil, err diff --git a/config/module/registry_test.go b/config/module/registry_test.go new file mode 100644 index 000000000000..2c784ae5de5c --- /dev/null +++ b/config/module/registry_test.go @@ -0,0 +1,152 @@ +package module + +import ( + "context" + "net" + "net/http" + "net/http/httptest" + "net/url" + "testing" + "time" + + cleanhttp "github.com/hashicorp/go-cleanhttp" + version "github.com/hashicorp/go-version" + "github.com/hashicorp/terraform/registry/regsrc" + "github.com/hashicorp/terraform/svchost/disco" +) + +// Return a transport to use for this test server. +// This not only loads the tls.Config from the test server for proper cert +// validation, but also inserts a Dialer that resolves localhost and +// example.com to 127.0.0.1 with the correct port, since 127.0.0.1 on its own +// isn't a valid registry hostname. +// TODO: cert validation not working here, so we use don't verify for now. +func mockTransport(server *httptest.Server) *http.Transport { + u, _ := url.Parse(server.URL) + _, port, _ := net.SplitHostPort(u.Host) + + transport := cleanhttp.DefaultTransport() + transport.TLSClientConfig = server.TLS + transport.TLSClientConfig.InsecureSkipVerify = true + transport.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) { + host, _, _ := net.SplitHostPort(addr) + switch host { + case "example.com", "localhost", "localhost.localdomain", "registry.terraform.io": + addr = "127.0.0.1" + if port != "" { + addr += ":" + port + } + } + return (&net.Dialer{ + Timeout: 30 * time.Second, + KeepAlive: 30 * time.Second, + DualStack: true, + }).DialContext(ctx, network, addr) + } + return transport +} + +func TestMockDiscovery(t *testing.T) { + server := mockTLSRegistry() + defer server.Close() + + regDisco := disco.NewDisco() + regDisco.Transport = mockTransport(server) + + regURL := regDisco.DiscoverServiceURL("example.com", serviceID) + + if regURL == nil { + t.Fatal("no registry service discovered") + } + + if regURL.Host != "example.com" { + t.Fatal("expected registry host example.com, got:", regURL.Host) + } +} + +func TestLookupModuleVersions(t *testing.T) { + server := mockTLSRegistry() + defer server.Close() + regDisco := disco.NewDisco() + regDisco.Transport = mockTransport(server) + + // test with and without a hostname + for _, src := range []string{ + "example.com/test-versions/name/provider", + "test-versions/name/provider", + } { + modsrc, err := regsrc.ParseModuleSource(src) + if err != nil { + t.Fatal(err) + } + + resp, err := lookupModuleVersions(regDisco, modsrc) + if err != nil { + t.Fatal(err) + } + + if len(resp.Modules) != 1 { + t.Fatal("expected 1 module, got", len(resp.Modules)) + } + + mod := resp.Modules[0] + name := "test-versions/name/provider" + if mod.Source != name { + t.Fatalf("expected module name %q, got %q", name, mod.Source) + } + + if len(mod.Versions) != 4 { + t.Fatal("expected 4 versions, got", len(mod.Versions)) + } + + for _, v := range mod.Versions { + _, err := version.NewVersion(v.Version) + if err != nil { + t.Fatalf("invalid version %q: %s", v.Version, err) + } + } + } +} + +func TestACCLookupModuleVersions(t *testing.T) { + server := mockTLSRegistry() + defer server.Close() + regDisco := disco.NewDisco() + + // test with and without a hostname + for _, src := range []string{ + "terraform-aws-modules/vpc/aws", + defaultRegistry + "/terraform-aws-modules/vpc/aws", + } { + modsrc, err := regsrc.ParseModuleSource(src) + if err != nil { + t.Fatal(err) + } + + resp, err := lookupModuleVersions(regDisco, modsrc) + if err != nil { + t.Fatal(err) + } + + if len(resp.Modules) != 1 { + t.Fatal("expected 1 module, got", len(resp.Modules)) + } + + mod := resp.Modules[0] + name := "terraform-aws-modules/vpc/aws" + if mod.Source != name { + t.Fatalf("expected module name %q, got %q", name, mod.Source) + } + + if len(mod.Versions) == 0 { + t.Fatal("expected multiple versions, got 0") + } + + for _, v := range mod.Versions { + _, err := version.NewVersion(v.Version) + if err != nil { + t.Fatalf("invalid version %q: %s", v.Version, err) + } + } + } +} From 152918d129568fd95f976063d9b07636c78d6332 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 25 Oct 2017 17:32:43 -0400 Subject: [PATCH 06/13] 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 26a31fa8456f..58515ab36339 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 6b94ac372535..be0aea902d45 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 a1fef61619b0..012ef7e14622 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 e10dbec1a047..e3ea555fd101 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 05a7bfa32d13..b6671c8a4a43 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 { From f56633c96d267ed8dbf916c91bb609754cf26c5e Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 25 Oct 2017 19:20:05 -0400 Subject: [PATCH 07/13] Lookup registry module versions during Tree.Load. Registry modules can't be handled directly by the getter.Storage implementation, which doesn't know how to handle versions. First see if we have a matching module stored that satisfies our constraints. If not, and we're getting or updating, we can look it up in the registry. This essentially takes the place of a "registry detector" for go-getter, but required the intermediate step of resolving the version dependency. This also starts breaking up the huge Tree.Load method into more manageable parts. It was sorely needed, as indicated by the difficulty encountered in this refactor. There's still a lot that can be done to improve this, but at least there are now a few easier to read methods when we come back to it. --- config/module/get_test.go | 28 ++++--- config/module/storage.go | 97 ++++++++++++++++++++-- config/module/tree.go | 165 +++++++++++++++++--------------------- 3 files changed, 183 insertions(+), 107 deletions(-) diff --git a/config/module/get_test.go b/config/module/get_test.go index be0aea902d45..01b61639d9bd 100644 --- a/config/module/get_test.go +++ b/config/module/get_test.go @@ -16,6 +16,7 @@ import ( version "github.com/hashicorp/go-version" "github.com/hashicorp/terraform/registry/regsrc" "github.com/hashicorp/terraform/registry/response" + "github.com/hashicorp/terraform/svchost/disco" ) // Map of module names and location of test modules. @@ -73,7 +74,7 @@ func mockRegHandler() http.Handler { download := func(w http.ResponseWriter, r *http.Request) { p := strings.TrimLeft(r.URL.Path, "/") // handle download request - re := regexp.MustCompile(`^([-a-z]+/\w+/\w+)/download$`) + re := regexp.MustCompile(`^([-a-z]+/\w+/\w+).*/download$`) // download lookup matches := re.FindStringSubmatch(p) if len(matches) != 2 { @@ -178,17 +179,19 @@ func mockTLSRegistry() *httptest.Server { return server } -/* -// 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. func TestRegistryGitHubArchive(t *testing.T) { - server := mockRegistry() + server := mockTLSRegistry() defer server.Close() - defer setResetRegDetector(server)() + d := regDisco + + regDisco = disco.NewDisco() + regDisco.Transport = mockTransport(server) + defer func() { + regDisco = d + }() storage := testStorage(t) tree := NewTree("", testConfig(t, "registry-tar-subdir")) @@ -226,9 +229,15 @@ func TestRegistryGitHubArchive(t *testing.T) { // Test that the //subdir notation can be used with registry modules func TestRegisryModuleSubdir(t *testing.T) { - server := mockRegistry() + server := mockTLSRegistry() defer server.Close() - defer setResetRegDetector(server)() + + d := regDisco + regDisco = disco.NewDisco() + regDisco.Transport = mockTransport(server) + defer func() { + regDisco = d + }() storage := testStorage(t) tree := NewTree("", testConfig(t, "registry-subdir")) @@ -251,7 +260,6 @@ 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") == "" { diff --git a/config/module/storage.go b/config/module/storage.go index 2873e61efe97..815888f87303 100644 --- a/config/module/storage.go +++ b/config/module/storage.go @@ -2,6 +2,7 @@ package module import ( "encoding/json" + "fmt" "io/ioutil" "log" "os" @@ -9,6 +10,7 @@ import ( "reflect" getter "github.com/hashicorp/go-getter" + "github.com/hashicorp/terraform/registry/regsrc" ) const manifestName = "modules.json" @@ -44,6 +46,12 @@ type moduleRecord struct { // independent from any subdirectory in the original source string, which // may traverse further into the module tree. Root string + + // url is the location of the module source + url string + + // Registry is true if this module is sourced from a registry + registry bool } // moduleStorage implements methods to record and fetch metadata about the @@ -53,20 +61,20 @@ type moduleRecord struct { type moduleStorage struct { getter.Storage storageDir string + mode GetMode } -func newModuleStorage(s getter.Storage) moduleStorage { +func newModuleStorage(s getter.Storage, mode GetMode) moduleStorage { return moduleStorage{ Storage: s, storageDir: storageDir(s), + mode: mode, } } // The Tree needs to know where to store the module manifest. // Th Storage abstraction doesn't provide access to the storage root directory, // so we extract it here. -// TODO: This needs to be replaced by refactoring the getter.Storage usage for -// modules. func storageDir(s getter.Storage) string { // get the StorageDir directly if possible switch t := s.(type) { @@ -74,6 +82,8 @@ func storageDir(s getter.Storage) string { return t.StorageDir case moduleStorage: return t.storageDir + case nil: + return "" } // this should be our UI wrapper which is exported here, so we need to @@ -201,11 +211,11 @@ func (m moduleStorage) recordModuleRoot(dir, root string) error { return m.recordModule(rec) } -func (m moduleStorage) getStorage(key string, src string, mode GetMode) (string, bool, error) { +func (m moduleStorage) getStorage(key string, src string) (string, bool, error) { // Get the module with the level specified if we were told to. - if mode > GetModeNone { + if m.mode > GetModeNone { log.Printf("[DEBUG] fetching %q with key %q", src, key) - if err := m.Storage.Get(key, src, mode == GetModeUpdate); err != nil { + if err := m.Storage.Get(key, src, m.mode == GetModeUpdate); err != nil { return "", false, err } } @@ -215,3 +225,78 @@ func (m moduleStorage) getStorage(key string, src string, mode GetMode) (string, log.Printf("[DEBUG] found %q in %q: %t", src, dir, found) return dir, found, err } + +// find a stored module that's not from a registry +func (m moduleStorage) findModule(key string) (string, error) { + if m.mode == GetModeUpdate { + return "", nil + } + + return m.moduleDir(key) +} + +// find a registry module +func (m moduleStorage) findRegistryModule(mSource, constraint string) (moduleRecord, error) { + rec := moduleRecord{ + Source: mSource, + } + // detect if we have a registry source + mod, err := regsrc.ParseModuleSource(mSource) + switch err { + case nil: + //ok + case regsrc.ErrInvalidModuleSource: + return rec, nil + default: + return rec, err + } + rec.registry = true + + log.Printf("[TRACE] %q is a registry module", mod.Module()) + + versions, err := m.moduleVersions(mod.String()) + if err != nil { + log.Println("[ERROR] error looking up versions for %q: %s", mod.Module(), err) + return rec, err + } + + match, err := newestRecord(versions, constraint) + if err != nil { + // TODO: does this allow previously unversioned modules? + log.Printf("[INFO] no matching version for %q<%s>, %s", mod.Module(), constraint, err) + } + + rec.Dir = match.Dir + rec.Version = match.Version + found := rec.Dir != "" + + // we need to lookup available versions + // Only on Get if it's not found, on unconditionally on Update + if (m.mode == GetModeGet && !found) || (m.mode == GetModeUpdate) { + resp, err := lookupModuleVersions(nil, mod) + if err != nil { + return rec, err + } + + if len(resp.Modules) == 0 { + return rec, fmt.Errorf("module %q not found in registry", mod.Module()) + } + + match, err := newestVersion(resp.Modules[0].Versions, constraint) + if err != nil { + return rec, err + } + + if match == nil { + return rec, fmt.Errorf("no versions for %q found matching %q", mod.Module(), constraint) + } + + rec.Version = match.Version + + rec.url, err = lookupModuleLocation(nil, mod, rec.Version) + if err != nil { + return rec, err + } + } + return rec, nil +} diff --git a/config/module/tree.go b/config/module/tree.go index e3ea555fd101..519fe31e7dab 100644 --- a/config/module/tree.go +++ b/config/module/tree.go @@ -173,20 +173,38 @@ func (t *Tree) Load(storage getter.Storage, mode GetMode) error { t.lock.Lock() defer t.lock.Unlock() - // discover where our modules are going to be stored - s := newModuleStorage(storage) + s := newModuleStorage(storage, mode) - // Reset the children if we have any - t.children = nil + children, err := t.getChildren(s) + if err != nil { + return err + } + + // Go through all the children and load them. + for _, c := range children { + if err := c.Load(storage, mode); err != nil { + return err + } + } + + // Set our tree up + t.children = children + + // if we're the root module, we can now set the provider inheritance + if len(t.path) == 0 { + t.inheritProviderConfigs(nil) + } - modules := t.Modules() + return nil +} +func (t *Tree) getChildren(s moduleStorage) (map[string]*Tree, error) { children := make(map[string]*Tree) // Go through all the modules and get the directory for them. - for _, m := range modules { + for _, m := range t.Modules() { if _, ok := children[m.Name]; ok { - return fmt.Errorf( + return nil, fmt.Errorf( "module %s: duplicated. module names must be unique", m.Name) } @@ -196,83 +214,67 @@ func (t *Tree) Load(storage getter.Storage, mode GetMode) error { path = append(path, m.Name) log.Printf("[TRACE] module source: %q", m.Source) - // Split out the subdir if we have one. - // Terraform keeps the entire requested tree, so that modules can - // reference sibling modules from the same archive or repo. - rawSource, subDir := getter.SourceDirSubdir(m.Source) + + // Lookup the local location of the module. + // dir is the local directory where the module is stored + mod, err := s.findRegistryModule(m.Source, m.Version) + if err != nil { + return nil, err + } // The key is the string that will be used to uniquely id the Source in // the local storage. The prefix digit can be incremented to // invalidate the local module storage. key := "1." + t.versionedPathKey(m) - - // we can't calculate a key without a version, so lookup if we have any - // matching modules stored. - var dir, version string - var found bool - // only registry modules have a version, and only full URLs are globally unique - - // TODO: This needs to only check for registry modules, and lookup - // versions if we don't find them here. Don't continue on as if - // a registry identifier could be some other source. - if mode != GetModeUpdate { - versions, err := s.moduleVersions(rawSource) - if err != nil { - log.Println("[ERROR] error looking up versions for %q: %s", m.Source, err) - return err - } - - match, err := newestRecord(versions, m.Version) - if err != nil { - // not everything has a recorded version, or a constraint, so just log this - log.Printf("[INFO] no matching version for %q<%s>, %s", m.Source, m.Version, err) - } - - dir = match.Dir - version = match.Version - found = dir != "" + if mod.Version != "" { + key += "." + mod.Version } - // It wasn't a versioned module, check for the exact key. - // This replaces the Storgae.Dir method with our manifest lookup. - var err error - if !found { - dir, err = s.moduleDir(key) + // Check for the exact key if it's not a registry module + if !mod.registry { + mod.Dir, err = s.findModule(key) if err != nil { - return err + return nil, err } - found = dir != "" } - // looks like we already have it - // In order to load the Tree we need to find out if there was another - // subDir stored from discovery. - if found && mode != GetModeUpdate { - subDir, err := s.getModuleRoot(dir) + if mod.Dir != "" { + // We found it locally, but in order to load the Tree we need to + // find out if there was another subDir stored from detection. + subDir, err := s.getModuleRoot(mod.Dir) if err != nil { // If there's a problem with the subdir record, we'll let the // recordSubdir method fix it up. Any other filesystem errors // will turn up again below. log.Println("[WARN] error reading subdir record:", err) } else { - dir := filepath.Join(dir, subDir) - // Load the configurations.Dir(source) - child, err := NewTreeModule(m.Name, dir) + fullDir := filepath.Join(mod.Dir, subDir) + + child, err := NewTreeModule(m.Name, fullDir) if err != nil { - return fmt.Errorf("module %s: %s", m.Name, err) + return nil, fmt.Errorf("module %s: %s", m.Name, err) } child.path = path child.parent = t - child.version = version + child.version = mod.Version child.source = m.Source children[m.Name] = child continue } } - source, err := getter.Detect(rawSource, t.config.Dir, getter.Detectors) - if err != nil { - return fmt.Errorf("module %s: %s", m.Name, err) + // Split out the subdir if we have one. + // Terraform keeps the entire requested tree, so that modules can + // reference sibling modules from the same archive or repo. + rawSource, subDir := getter.SourceDirSubdir(m.Source) + + // we haven't found a source, so fallback to the go-getter detectors + source := mod.url + if source == "" { + source, err = getter.Detect(rawSource, t.config.Dir, getter.Detectors) + if err != nil { + return nil, fmt.Errorf("module %s: %s", m.Name, err) + } } log.Printf("[TRACE] detected module source %q", source) @@ -286,15 +288,12 @@ func (t *Tree) Load(storage getter.Storage, mode GetMode) error { subDir = filepath.Join(detectedSubDir, subDir) } - log.Printf("[TRACE] getting module source %q", source) - - dir, ok, err := s.getStorage(key, source, mode) + dir, ok, err := s.getStorage(key, source) if err != nil { - return err + return nil, err } if !ok { - return fmt.Errorf( - "module %s: not found, may need to be downloaded using 'terraform get'", m.Name) + return nil, fmt.Errorf("module %s: not found, may need to run 'terraform init'", m.Name) } log.Printf("[TRACE] %q stored in %q", source, dir) @@ -304,54 +303,38 @@ func (t *Tree) Load(storage getter.Storage, mode GetMode) error { if subDir != "" { fullDir, err = getter.SubdirGlob(dir, subDir) if err != nil { - return err + return nil, err } // +1 to account for the pathsep if len(dir)+1 > len(fullDir) { - return fmt.Errorf("invalid module storage path %q", fullDir) + return nil, fmt.Errorf("invalid module storage path %q", fullDir) } subDir = fullDir[len(dir)+1:] } - rec := moduleRecord{ - Source: m.Source, - Key: key, - Dir: dir, - Root: subDir, - } + // add new info to the module record + mod.Key = key + mod.Dir = dir + mod.Root = subDir - if err := s.recordModule(rec); err != nil { - return err + // record the module in our manifest + if err := s.recordModule(mod); err != nil { + return nil, err } child, err := NewTreeModule(m.Name, fullDir) if err != nil { - return fmt.Errorf("module %s: %s", m.Name, err) + return nil, fmt.Errorf("module %s: %s", m.Name, err) } child.path = path child.parent = t - child.version = version + child.version = mod.Version child.source = m.Source children[m.Name] = child } - // Go through all the children and load them. - for _, c := range children { - if err := c.Load(s, mode); err != nil { - return err - } - } - - // Set our tree up - t.children = children - - // if we're the root module, we can now set the provider inheritance - if len(t.path) == 0 { - t.inheritProviderConfigs(nil) - } - - return nil + return children, nil } // inheritProviderConfig resolves all provider config inheritance after the From 4ab03bfa81c8f0247d4e74f5898e9c392f76a63a Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 26 Oct 2017 18:48:39 -0400 Subject: [PATCH 08/13] ProviderConfig needs an Inherited flag If a provider configuration is inherited from another module, any interpolations in that config won't have variables declared locally. Let the config only be validated in it's original location. --- config/config.go | 8 ++++++++ config/module/tree.go | 1 + 2 files changed, 9 insertions(+) diff --git a/config/config.go b/config/config.go index c8c7c9ae3156..96d63bf432aa 100644 --- a/config/config.go +++ b/config/config.go @@ -74,6 +74,10 @@ type ProviderConfig struct { // it can be copied into child module providers yet still interpolated in // the correct scope. Path []string + + // Inherited is used to skip validation of this config, since any + // interpolated variables won't be declared at this level. + Inherited bool } // A resource represents a single Terraform resource in the configuration. @@ -813,6 +817,10 @@ func (c *Config) rawConfigs() map[string]*RawConfig { } for _, pc := range c.ProviderConfigs { + // this was an inherited config, so we don't validate it at this level. + if pc.Inherited { + continue + } source := fmt.Sprintf("provider config '%s'", pc.Name) result[source] = pc.RawConfig } diff --git a/config/module/tree.go b/config/module/tree.go index 519fe31e7dab..dbbe0a480adc 100644 --- a/config/module/tree.go +++ b/config/module/tree.go @@ -395,6 +395,7 @@ func (t *Tree) inheritProviderConfigs(stack []*Tree) { pc.Path = pt.Path() pc.Path = append([]string{RootName}, pt.path...) pc.RawConfig = parentProvider.RawConfig + pc.Inherited = true log.Printf("[TRACE] provider %q inheriting config from %q", strings.Join(append(t.Path(), pc.FullName()), "."), strings.Join(append(pt.Path(), parentProvider.FullName()), "."), From 74dec5e90d4a99e0d11929db67205c49cde96e21 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 27 Oct 2017 08:57:06 -0400 Subject: [PATCH 09/13] flag off registry ACC test Was missing the TF_ACC check --- config/module/registry_test.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/config/module/registry_test.go b/config/module/registry_test.go index 2c784ae5de5c..6387c1812580 100644 --- a/config/module/registry_test.go +++ b/config/module/registry_test.go @@ -6,6 +6,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "os" "testing" "time" @@ -108,9 +109,10 @@ func TestLookupModuleVersions(t *testing.T) { } } -func TestACCLookupModuleVersions(t *testing.T) { - server := mockTLSRegistry() - defer server.Close() +func TestAccLookupModuleVersions(t *testing.T) { + if os.Getenv("TF_ACC") == "" { + t.Skip() + } regDisco := disco.NewDisco() // test with and without a hostname From e36093959ae42ed0f3e3c1a2d6f9dcf6d3cf4e84 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 27 Oct 2017 08:57:37 -0400 Subject: [PATCH 10/13] update test for error condition --- config/module/detector_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/module/detector_test.go b/config/module/detector_test.go index 10cc057fcc4f..8f727f4dae89 100644 --- a/config/module/detector_test.go +++ b/config/module/detector_test.go @@ -30,8 +30,8 @@ func TestParseRegistrySource(t *testing.T) { id: "namespace/id/provider", }, { // too many parts - source: "registry.com/namespace/id/provider/extra", - notRegistry: true, + source: "registry.com/namespace/id/provider/extra", + err: true, }, { // local path source: "./local/file/path", From e658390cb6dc8a831d6cb509cfe1adeb71d94fc9 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 27 Oct 2017 08:58:16 -0400 Subject: [PATCH 11/13] use URLs rather than strings is registry functions Parse all the registry strings as urls, and compine with path.Join to for better validation. --- config/module/registry.go | 37 ++++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/config/module/registry.go b/config/module/registry.go index 012ef7e14622..b9ca08c8d243 100644 --- a/config/module/registry.go +++ b/config/module/registry.go @@ -7,6 +7,8 @@ import ( "log" "net/http" "net/url" + "path" + "strings" "time" cleanhttp "github.com/hashicorp/go-cleanhttp" @@ -45,7 +47,7 @@ func (e errModuleNotFound) Error() string { return `module "` + string(e) + `" not found` } -func discoverRegURL(d *disco.Disco, module *regsrc.Module) string { +func discoverRegURL(d *disco.Disco, module *regsrc.Module) *url.URL { if d == nil { d = regDisco } @@ -63,23 +65,27 @@ func discoverRegURL(d *disco.Disco, module *regsrc.Module) string { } } - service := regURL.String() - - if service[len(service)-1] != '/' { - service += "/" + if !strings.HasSuffix(regURL.Path, "/") { + regURL.Path += "/" } - return service + return regURL } // 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) + p, err := url.Parse(path.Join(module.Module(), "versions")) + if err != nil { + return nil, err + } + + service = service.ResolveReference(p) - req, err := http.NewRequest("GET", location, nil) + log.Printf("[DEBUG] fetching module versions from %q", service) + + req, err := http.NewRequest("GET", service.String(), nil) if err != nil { return nil, err } @@ -128,16 +134,21 @@ func lookupModuleVersions(d *disco.Disco, module *regsrc.Module) (*response.Modu func lookupModuleLocation(d *disco.Disco, module *regsrc.Module, version string) (string, error) { service := discoverRegURL(d, module) - var download string + var p *url.URL + var err error if version == "" { - download = fmt.Sprintf("%s%s/download", service, module.Module()) + p, err = url.Parse(path.Join(module.Module(), "download")) } else { - download = fmt.Sprintf("%s%s/%s/download", service, module.Module(), version) + p, err = url.Parse(path.Join(module.Module(), version, "download")) + } + if err != nil { + return "", err } + download := service.ResolveReference(p) log.Printf("[DEBUG] looking up module location from %q", download) - req, err := http.NewRequest("GET", download, nil) + req, err := http.NewRequest("GET", download.String(), nil) if err != nil { return "", err } From 32c2efe29cf70ccab16e73217d87444ea67b3e7a Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 27 Oct 2017 09:01:45 -0400 Subject: [PATCH 12/13] add example for the reason behind versionedPathKey --- config/module/tree.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/config/module/tree.go b/config/module/tree.go index dbbe0a480adc..24b40cba18cc 100644 --- a/config/module/tree.go +++ b/config/module/tree.go @@ -638,6 +638,9 @@ func (t *Tree) Validate() error { // and source encoded. This is to provide a unique key for our module storage, // since submodules need to know which versions of their ancestor modules they // are loaded from. +// For example, if module A has a subdirectory B, if module A's source or +// version is updated B's storage key must reflect this change in order for the +// correct version of B's source to be loaded. func (t *Tree) versionedPathKey(m *Module) string { path := make([]string, len(t.path)+1) path[len(path)-1] = m.Name + ";" + m.Source From 68bf48edc4ea9a27865e45583b576fabb178ddc5 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 27 Oct 2017 09:07:25 -0400 Subject: [PATCH 13/13] fix vet error --- config/module/storage.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/module/storage.go b/config/module/storage.go index 815888f87303..eaee3746a72a 100644 --- a/config/module/storage.go +++ b/config/module/storage.go @@ -256,7 +256,7 @@ func (m moduleStorage) findRegistryModule(mSource, constraint string) (moduleRec versions, err := m.moduleVersions(mod.String()) if err != nil { - log.Println("[ERROR] error looking up versions for %q: %s", mod.Module(), err) + log.Printf("[ERROR] error looking up versions for %q: %s", mod.Module(), err) return rec, err }