Merge pull request #16160 from hashicorp/jbardin/get-subdir

Handle module source subdirectories in Terraform

config/module/get_test.go

@@ -16,11 +16,29 @@ import (
 	version "github.com/hashicorp/go-version"
 )
 
-// map of module names and version for test module.
+// Map of module names and location of test modules.
-// only one version for now, as we only lookup latest from the registry
+// Only one version for now, as we only lookup latest from the registry.
-var testMods = map[string]string{
+type testMod struct {
-	"registry/foo/bar": "0.2.3",
+	location string
-	"registry/foo/baz": "1.10.0",
+	version  string
+}
+
+// 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": {
+		location: "file:///download/registry/foo/bar/0.2.3//*?archive=tar.gz",
+		version:  "0.2.3",
+	},
+	"registry/foo/baz": {
+		location: "file:///download/registry/foo/baz/1.10.0//*?archive=tar.gz",
+		version:  "1.10.0",
+	},
+	"registry/local/sub": {
+		location: "test-fixtures/registry-tar-subdir/foo.tgz//*?archive=tar.gz",
+		version:  "0.1.2",
+	},
 }
 
 func latestVersion(versions []string) string {
@@ -56,13 +74,19 @@ func mockRegistry() *httptest.Server {
 				return
 			}
 
-			version, ok := testMods[matches[1]]
+			mod, ok := testMods[matches[1]]
 			if !ok {
 				w.WriteHeader(http.StatusNotFound)
 				return
 			}
 
-			location := fmt.Sprintf("%s/download/%s/%s", server.URL, matches[1], version)
+			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(xTerraformGet, location)
 			w.WriteHeader(http.StatusNoContent)
 			// no body
@@ -90,12 +114,12 @@ func TestDetectRegistry(t *testing.T) {
 	}{
 		{
 			source:   "registry/foo/bar",
-			location: "download/registry/foo/bar/0.2.3",
+			location: testMods["registry/foo/bar"].location,
 			found:    true,
 		},
 		{
 			source:   "registry/foo/baz",
-			location: "download/registry/foo/baz/1.10.0",
+			location: testMods["registry/foo/baz"].location,
 			found:    true,
 		},
 		// this should not be found, but not stop detection
@@ -177,7 +201,7 @@ func TestDetectors(t *testing.T) {
 	}{
 		{
 			source:   "registry/foo/bar",
-			location: "download/registry/foo/bar/0.2.3",
+			location: "file:///download/registry/foo/bar/0.2.3//*?archive=tar.gz",
 		},
 		// this should not be found, but not stop detection
 		{
@@ -248,6 +272,53 @@ func TestDetectors(t *testing.T) {
 	}
 }
 
+// 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()
+	defer server.Close()
+
+	regDetector := &registryDetector{
+		api:    server.URL + "/v1/modules/",
+		client: server.Client(),
+	}
+
+	origDetectors := detectors
+	defer func() {
+		detectors = origDetectors
+	}()
+
+	detectors = []getter.Detector{
+		new(getter.GitHubDetector),
+		new(getter.BitBucketDetector),
+		new(getter.S3Detector),
+		new(localDetector),
+		regDetector,
+	}
+
+	storage := testStorage(t)
+	tree := NewTree("", testConfig(t, "registry-tar-subdir"))
+
+	if err := tree.Load(storage, GetModeGet); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	if !tree.Loaded() {
+		t.Fatal("should be loaded")
+	}
+
+	if err := tree.Load(storage, GetModeNone); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	actual := strings.TrimSpace(tree.String())
+	expected := strings.TrimSpace(treeLoadSubdirStr)
+	if actual != expected {
+		t.Fatalf("got: \n\n%s\nexpected: \n\n%s", actual, expected)
+	}
+}
+
 func TestAccRegistryDiscover(t *testing.T) {
 	if os.Getenv("TF_ACC") == "" {
 		t.Skip("skipping ACC test")
@@ -272,3 +343,30 @@ func TestAccRegistryDiscover(t *testing.T) {
 		t.Fatalf("url doesn't contain 'consul': %s", u.String())
 	}
 }
+
+func TestAccRegistryLoad(t *testing.T) {
+	if os.Getenv("TF_ACC") == "" {
+		t.Skip("skipping ACC test")
+	}
+
+	storage := testStorage(t)
+	tree := NewTree("", testConfig(t, "registry-load"))
+
+	if err := tree.Load(storage, GetModeGet); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	if !tree.Loaded() {
+		t.Fatal("should be loaded")
+	}
+
+	if err := tree.Load(storage, GetModeNone); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	// TODO expand this further by fetching some metadata from the registry
+	actual := strings.TrimSpace(tree.String())
+	if !strings.Contains(actual, "(path: vault)") {
+		t.Fatal("missing vault module, got:\n", actual)
+	}
+}

config/module/module_test.go

@@ -2,6 +2,7 @@ package module
 
 import (
 	"io/ioutil"
+	"log"
 	"os"
 	"path/filepath"
 	"testing"
@@ -10,6 +11,12 @@ import (
 	"github.com/hashicorp/terraform/config"
 )
 
+func init() {
+	if os.Getenv("TF_LOG") == "" {
+		log.SetOutput(ioutil.Discard)
+	}
+}
+
 const fixtureDir = "./test-fixtures"
 
 func tempDir(t *testing.T) string {

config/module/test-fixtures/basic-tar-subdir/main.tf

@@ -0,0 +1,3 @@
+module "foo" {
+    source = "./foo.tgz//sub"
+}

config/module/test-fixtures/registry-load/main.tf

@@ -0,0 +1,3 @@
+module "vault" {
+    source = "hashicorp/vault/aws"
+}

config/module/test-fixtures/registry-tar-subdir/main.tf

@@ -0,0 +1,4 @@
+module "foo" {
+	// the mock test registry will redirect this to the local tar file
+    source = "registry/local/sub"
+}

config/module/test-fixtures/tar-subdir-to-parent/main.tf

@@ -0,0 +1,4 @@
+module "foo" {
+	// the module in sub references sibling module baz via "../baz"
+    source = "./foo.tgz//sub"
+}

config/module/tree.go

@@ -3,7 +3,12 @@ package module
 import (
 	"bufio"
 	"bytes"
+	"encoding/json"
 	"fmt"
+	"io/ioutil"
+	"log"
+	"os"
+	"path/filepath"
 	"strings"
 	"sync"
 
@@ -176,13 +181,73 @@ func (t *Tree) Load(s getter.Storage, mode GetMode) error {
 		copy(path, t.path)
 		path = append(path, m.Name)
 
-		source, err := getter.Detect(m.Source, t.config.Dir, detectors)
+		// 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("0.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
+		// reference sibling modules from the same archive or repo.
+		source, subDir := getter.SourceDirSubdir(m.Source)
+
+		// 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
+		}
+
+		// 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 := t.getSubdir(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.
+				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)
+				if err != nil {
+					return fmt.Errorf("module %s: %s", m.Name, err)
+				}
+				// Set the path of this child
+				children[m.Name].path = path
+				continue
+			}
+		}
+
+		log.Printf("[TRACE] module source: %q", source)
+
+		source, err = getter.Detect(source, t.config.Dir, detectors)
 		if err != nil {
 			return fmt.Errorf("module %s: %s", m.Name, err)
 		}
-		// Get the directory where this module is so we can load it
+
-		key := strings.Join(path, ".")
+		log.Printf("[TRACE] detected module source %q", source)
-		key = fmt.Sprintf("module.%s-%s", key, m.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.
+		//
+		// TODO: This can cause us to lose the previously detected subdir. It
+		// was never an issue before, since none of the supported detectors
+		// previously had this behavior, but we may want to add this ability to
+		// registry modules.
+		source, subDir2 := getter.SourceDirSubdir(source)
+		if subDir2 != "" {
+			subDir = subDir2
+		}
+
+		log.Printf("[TRACE] getting module source %q", source)
 
 		dir, ok, err := getStorage(s, key, source, mode)
 		if err != nil {
@@ -193,12 +258,31 @@ func (t *Tree) Load(s getter.Storage, mode GetMode) error {
 				"module %s: not found, may need to be downloaded using 'terraform get'", m.Name)
 		}
 
-		children[m.Name], err = NewTreeModule(m.Name, dir)
+		// expand and record the subDir for later
-		if err != nil {
+		if subDir != "" {
-			return fmt.Errorf(
+			fullDir, err := getter.SubdirGlob(dir, subDir)
-				"module %s: %s", m.Name, err)
+			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:]
+
+			if err := t.recordSubdir(dir, subDir); err != nil {
+				return err
+			}
+			dir = fullDir
 		}
 
+		// Load the configurations.Dir(source)
+		children[m.Name], 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
 	}
@@ -216,6 +300,65 @@ func (t *Tree) Load(s getter.Storage, mode GetMode) error {
 	return nil
 }
 
+func subdirRecordsPath(dir string) string {
+	const filename = "module-subdir.json"
+	// Get the parent directory.
+	// The current FolderStorage implementation needed to be able to create
+	// this directory, so we can be reasonably certain we can use it.
+	parent := filepath.Dir(filepath.Clean(dir))
+	return filepath.Join(parent, filename)
+}
+
+// unmarshal the records file in the parent directory. Always returns a valid map.
+func loadSubdirRecords(dir string) (map[string]string, error) {
+	records := map[string]string{}
+
+	recordsPath := subdirRecordsPath(dir)
+	data, err := ioutil.ReadFile(recordsPath)
+	if err != nil && !os.IsNotExist(err) {
+		return records, err
+	}
+
+	if len(data) == 0 {
+		return records, nil
+	}
+
+	if err := json.Unmarshal(data, &records); err != nil {
+		return records, err
+	}
+	return records, nil
+}
+
+func (t *Tree) getSubdir(dir string) (string, error) {
+	records, err := loadSubdirRecords(dir)
+	if err != nil {
+		return "", err
+	}
+
+	return records[dir], nil
+}
+
+// Mark the location of a detected subdir in a top-level file so we
+// can skip detection when not updating the module.
+func (t *Tree) recordSubdir(dir, subdir string) error {
+	records, err := loadSubdirRecords(dir)
+	if err != nil {
+		// if there was a problem with the file, we will attempt to write a new
+		// one. Any non-data related error should surface there.
+		log.Printf("[WARN] error reading subdir records: %s", err)
+	}
+
+	records[dir] = subdir
+
+	js, err := json.Marshal(records)
+	if err != nil {
+		return err
+	}
+
+	recordsPath := subdirRecordsPath(dir)
+	return ioutil.WriteFile(recordsPath, js, 0644)
+}
+
 // Path is the full path to this tree.
 func (t *Tree) Path() []string {
 	return t.path

config/module/tree_test.go

@@ -2,7 +2,9 @@ package module
 
 import (
 	"fmt"
+	"io/ioutil"
 	"os"
+	"path/filepath"
 	"reflect"
 	"strings"
 	"testing"
@@ -209,40 +211,120 @@ func TestTreeLoad_parentRef(t *testing.T) {
 }
 
 func TestTreeLoad_subdir(t *testing.T) {
-	storage := testStorage(t)
+	fixtures := []string{
-	tree := NewTree("", testConfig(t, "basic-subdir"))
+		"basic-subdir",
+		"basic-tar-subdir",
 
-	if tree.Loaded() {
+		// Passing a subpath to go getter extracts only this subpath. The old
-		t.Fatal("should not be loaded")
+		// internal code would keep the entire directory structure, allowing a
+		// top-level module to reference others through its parent directory.
+		// TODO: this can be removed as a breaking change in a major release.
+		"tar-subdir-to-parent",
 	}
 
-	// This should error because we haven't gotten things yet
+	for _, tc := range fixtures {
-	if err := tree.Load(storage, GetModeNone); err == nil {
+		t.Run(tc, func(t *testing.T) {
-		t.Fatal("should error")
+			storage := testStorage(t)
+			tree := NewTree("", testConfig(t, tc))
+
+			if tree.Loaded() {
+				t.Fatal("should not be loaded")
+			}
+
+			// This should error because we haven't gotten things yet
+			if err := tree.Load(storage, GetModeNone); err == nil {
+				t.Fatal("should error")
+			}
+
+			if tree.Loaded() {
+				t.Fatal("should not be loaded")
+			}
+
+			// This should get things
+			if err := tree.Load(storage, GetModeGet); err != nil {
+				t.Fatalf("err: %s", err)
+			}
+
+			if !tree.Loaded() {
+				t.Fatal("should be loaded")
+			}
+
+			// This should no longer error
+			if err := tree.Load(storage, GetModeNone); err != nil {
+				t.Fatalf("err: %s", err)
+			}
+
+			actual := strings.TrimSpace(tree.String())
+			expected := strings.TrimSpace(treeLoadSubdirStr)
+			if actual != expected {
+				t.Fatalf("bad: \n\n%s", actual)
+			}
+		})
+	}
+}
+
+func TestTree_recordSubDir(t *testing.T) {
+	td, err := ioutil.TempDir("", "tf-module")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.RemoveAll(td)
+
+	dir := filepath.Join(td, "0131bf0fef686e090b16bdbab4910ddf")
+
+	subDir := "subDirName"
+
+	tree := Tree{}
+
+	// record and read the subdir path
+	if err := tree.recordSubdir(dir, subDir); err != nil {
+		t.Fatal(err)
+	}
+	actual, err := tree.getSubdir(dir)
+	if err != nil {
+		t.Fatal(err)
 	}
 
-	if tree.Loaded() {
+	if actual != subDir {
-		t.Fatal("should not be loaded")
+		t.Fatalf("expected subDir %q, got %q", subDir, actual)
 	}
 
-	// This should get things
+	// overwrite the path, and nmake sure we get the new one
-	if err := tree.Load(storage, GetModeGet); err != nil {
+	subDir = "newSubDir"
-		t.Fatalf("err: %s", err)
+	if err := tree.recordSubdir(dir, subDir); err != nil {
+		t.Fatal(err)
+	}
+	actual, err = tree.getSubdir(dir)
+	if err != nil {
+		t.Fatal(err)
 	}
 
-	if !tree.Loaded() {
+	if actual != subDir {
-		t.Fatal("should be loaded")
+		t.Fatalf("expected subDir %q, got %q", subDir, actual)
 	}
 
-	// This should no longer error
+	// create a fake entry
-	if err := tree.Load(storage, GetModeNone); err != nil {
+	if err := ioutil.WriteFile(subdirRecordsPath(dir), []byte("BAD DATA"), 0644); err != nil {
-		t.Fatalf("err: %s", err)
+		t.Fatal(err)
 	}
 
-	actual := strings.TrimSpace(tree.String())
+	// this should fail because there aare now 2 entries
-	expected := strings.TrimSpace(treeLoadSubdirStr)
+	actual, err = tree.getSubdir(dir)
-	if actual != expected {
+	if err == nil {
-		t.Fatalf("bad: \n\n%s", actual)
+		t.Fatal("expected multiple subdir entries")
+	}
+
+	// writing the subdir entry should remove the incorrect value
+	if err := tree.recordSubdir(dir, subDir); err != nil {
+		t.Fatal(err)
+	}
+	actual, err = tree.getSubdir(dir)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if actual != subDir {
+		t.Fatalf("expected subDir %q, got %q", subDir, actual)
 	}
 }