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

@@ -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") == "" {

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
+}

config/module/tree.go

@@ -173,172 +173,16 @@ 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, mode)
-	s := newModuleStorage(storage)
 
-	// Reset the children if we have any
+	children, err := t.getChildren(s)
-	t.children = nil
+	if err != nil {
-
+		return err
-	modules := t.Modules()
-
-	children := make(map[string]*Tree)
-
-	// 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)
-		}
-
-		// Determine the path to this child
-		path := make([]string, len(t.path), len(t.path)+1)
-		copy(path, t.path)
-		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)
-
-		// 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 != ""
-		}
-
-		// 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
-		// 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 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)
-				if err != nil {
-					return fmt.Errorf("module %s: %s", m.Name, err)
-				}
-				child.path = path
-				child.parent = t
-				child.version = 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)
-		}
-
-		log.Printf("[TRACE] detected module source %q", source)
-
-		// Check if the detector introduced something new.
-		// For example, the registry always adds a subdir of `//*`,
-		// indicating that we need to strip off the first component from the
-		// tar archive, though we may not yet know what it is called.
-		source, detectedSubDir := getter.SourceDirSubdir(source)
-		if detectedSubDir != "" {
-			subDir = filepath.Join(detectedSubDir, subDir)
-		}
-
-		log.Printf("[TRACE] getting module source %q", source)
-
-		dir, ok, err := s.getStorage(key, source, mode)
-		if err != nil {
-			return err
-		}
-		if !ok {
-			return fmt.Errorf(
-				"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)
-			if err != nil {
-				return err
-			}
-
-			// +1 to account for the pathsep
-			if len(dir)+1 > len(fullDir) {
-				return fmt.Errorf("invalid module storage path %q", fullDir)
-			}
-			subDir = fullDir[len(dir)+1:]
-		}
-
-		rec := moduleRecord{
-			Source: m.Source,
-			Key:    key,
-			Dir:    dir,
-			Root:   subDir,
-		}
-
-		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)
-		}
-		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.
 	for _, c := range children {
-		if err := c.Load(s, mode); err != nil {
+		if err := c.Load(storage, mode); err != nil {
 			return err
 		}
 	}
@@ -354,6 +198,145 @@ func (t *Tree) Load(storage getter.Storage, mode GetMode) error {
 	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 t.Modules() {
+		if _, ok := children[m.Name]; ok {
+			return nil, fmt.Errorf(
+				"module %s: duplicated. module names must be unique", m.Name)
+		}
+
+		// Determine the path to this child
+		path := make([]string, len(t.path), len(t.path)+1)
+		copy(path, t.path)
+		path = append(path, m.Name)
+
+		log.Printf("[TRACE] module source: %q", 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)
+		if mod.Version != "" {
+			key += "." + mod.Version
+		}
+
+		// 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 nil, err
+			}
+		}
+
+		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 {
+				fullDir := filepath.Join(mod.Dir, subDir)
+
+				child, err := NewTreeModule(m.Name, fullDir)
+				if err != nil {
+					return nil, fmt.Errorf("module %s: %s", m.Name, err)
+				}
+				child.path = path
+				child.parent = t
+				child.version = mod.Version
+				child.source = m.Source
+				children[m.Name] = child
+				continue
+			}
+		}
+
+		// 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)
+
+		// Check if the detector introduced something new.
+		// For example, the registry always adds a subdir of `//*`,
+		// indicating that we need to strip off the first component from the
+		// tar archive, though we may not yet know what it is called.
+		source, detectedSubDir := getter.SourceDirSubdir(source)
+		if detectedSubDir != "" {
+			subDir = filepath.Join(detectedSubDir, subDir)
+		}
+
+		dir, ok, err := s.getStorage(key, source)
+		if err != nil {
+			return nil, err
+		}
+		if !ok {
+			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)
+
+		// expand and record the subDir for later
+		fullDir := dir
+		if subDir != "" {
+			fullDir, err = getter.SubdirGlob(dir, subDir)
+			if err != nil {
+				return nil, err
+			}
+
+			// +1 to account for the pathsep
+			if len(dir)+1 > len(fullDir) {
+				return nil, fmt.Errorf("invalid module storage path %q", fullDir)
+			}
+			subDir = fullDir[len(dir)+1:]
+		}
+
+		// add new info to the module record
+		mod.Key = key
+		mod.Dir = dir
+		mod.Root = subDir
+
+		// 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 nil, fmt.Errorf("module %s: %s", m.Name, err)
+		}
+		child.path = path
+		child.parent = t
+		child.version = mod.Version
+		child.source = m.Source
+		children[m.Name] = child
+	}
+
+	return children, nil
+}
+
 // inheritProviderConfig resolves all provider config inheritance after the
 // tree is loaded.
 //