config: refactor to consolidate all File->Source loading

Previously the logic for reading ConfigFiles and produces Sources was split
between NewBuilder and Build. This commit moves all of the logic into NewBuilder
so that Build() can operate entirely on Sources.

This change is in preparation for logging warnings when files have an
unsupported extension.

It also reduces the scope of BuilderOpts, and gets us very close to removing
Builder.options.

agent/config/builder.go

@@ -81,8 +81,7 @@ type Builder struct {
 	err error
 }
 
-// NewBuilder returns a new configuration builder based on the given command
-// line flags.
+// NewBuilder returns a new configuration Builder from the BuilderOpts.
 func NewBuilder(opts BuilderOpts) (*Builder, error) {
 	newSource := func(name string, v interface{}) Source {
 		b, err := json.MarshalIndent(v, "", "    ")
@@ -97,7 +96,7 @@ func NewBuilder(opts BuilderOpts) (*Builder, error) {
 		Head:    []Source{DefaultSource(), DefaultEnterpriseSource()},
 	}
 
-	if b.boolVal(b.options.DevMode) {
+	if b.boolVal(opts.DevMode) {
 		b.Head = append(b.Head, DevSource())
 	}
 
@@ -106,17 +105,17 @@ func NewBuilder(opts BuilderOpts) (*Builder, error) {
 	// we need to merge all slice values defined in flags before we
 	// merge the config files since the flag values for slices are
 	// otherwise appended instead of prepended.
-	slices, values := b.splitSlicesAndValues(b.options.Config)
+	slices, values := b.splitSlicesAndValues(opts.Config)
 	b.Head = append(b.Head, newSource("flags.slices", slices))
-	for _, path := range b.options.ConfigFiles {
-		sources, err := b.ReadPath(path)
+	for _, path := range opts.ConfigFiles {
+		sources, err := sourcesFromPath(path, opts)
 		if err != nil {
 			return nil, err
 		}
 		b.Sources = append(b.Sources, sources...)
 	}
 	b.Tail = append(b.Tail, newSource("flags.values", values))
-	for i, s := range b.options.HCL {
+	for i, s := range opts.HCL {
 		b.Tail = append(b.Tail, Source{
 			Name:   fmt.Sprintf("flags-%d.hcl", i),
 			Format: "hcl",
@@ -124,16 +123,16 @@ func NewBuilder(opts BuilderOpts) (*Builder, error) {
 		})
 	}
 	b.Tail = append(b.Tail, NonUserSource(), DefaultConsulSource(), OverrideEnterpriseSource(), DefaultVersionSource())
-	if b.boolVal(b.options.DevMode) {
+	if b.boolVal(opts.DevMode) {
 		b.Tail = append(b.Tail, DevConsulSource())
 	}
 	return b, nil
 }
 
-// ReadPath reads a single config file or all files in a directory (but
-// not its sub-directories) and appends them to the list of config
-// sources.
-func (b *Builder) ReadPath(path string) ([]Source, error) {
+// sourcesFromPath reads a single config file or all files in a directory (but
+// not its sub-directories) and returns Sources created from the
+// files.
+func sourcesFromPath(path string, options BuilderOpts) ([]Source, error) {
 	f, err := os.Open(path)
 	if err != nil {
 		return nil, fmt.Errorf("config: Open failed on %s. %s", path, err)
@@ -146,7 +145,12 @@ func (b *Builder) ReadPath(path string) ([]Source, error) {
 	}
 
 	if !fi.IsDir() {
-		src, err := b.ReadFile(path)
+		if !shouldParseFile(path, options.ConfigFormat) {
+			// TODO: log warning
+			return nil, nil
+		}
+
+		src, err := newSourceFromFile(path, options.ConfigFormat)
 		if err != nil {
 			return nil, err
 		}
@@ -181,36 +185,46 @@ func (b *Builder) ReadPath(path string) ([]Source, error) {
 			continue
 		}
 
-		if b.shouldParseFile(fp) {
-			src, err := b.ReadFile(fp)
-			if err != nil {
-				return nil, err
-			}
-			sources = append(sources, src)
+		if !shouldParseFile(fp, options.ConfigFormat) {
+			// TODO: log warning
+			continue
 		}
+		src, err := newSourceFromFile(fp, options.ConfigFormat)
+		if err != nil {
+			return nil, err
+		}
+		sources = append(sources, src)
 	}
 	return sources, nil
 }
 
-// ReadFile parses a JSON or HCL config file and appends it to the list of
-// config sources.
-func (b *Builder) ReadFile(path string) (Source, error) {
+// newSourceFromFile creates a Source from the contents of the file at path.
+func newSourceFromFile(path string, format string) (Source, error) {
 	data, err := ioutil.ReadFile(path)
 	if err != nil {
-		return Source{}, fmt.Errorf("config: ReadFile failed on %s: %s", path, err)
+		return Source{}, fmt.Errorf("config: failed to read %s: %s", path, err)
 	}
-	return Source{Name: path, Data: string(data)}, nil
+	if format == "" {
+		format = formatFromFileExtension(path)
+	}
+	return Source{Name: path, Data: string(data), Format: format}, nil
 }
 
 // shouldParse file determines whether the file to be read is of a supported extension
-func (b *Builder) shouldParseFile(path string) bool {
-	srcFormat := FormatFrom(path)
+func shouldParseFile(path string, configFormat string) bool {
+	srcFormat := formatFromFileExtension(path)
+	return configFormat != "" || srcFormat == "hcl" || srcFormat == "json"
+}
 
-	// If config-format is not set, only read files with supported extensions
-	if b.options.ConfigFormat == "" && srcFormat != "hcl" && srcFormat != "json" {
-		return false
+func formatFromFileExtension(name string) string {
+	switch {
+	case strings.HasSuffix(name, ".json"):
+		return "json"
+	case strings.HasSuffix(name, ".hcl"):
+		return "hcl"
+	default:
+		return ""
 	}
-	return true
 }
 
 type byName []os.FileInfo
@@ -240,9 +254,7 @@ func (b *Builder) Build() (rt RuntimeConfig, err error) {
 	b.err = nil
 	b.Warnings = nil
 
-	// ----------------------------------------------------------------
-	// merge config sources as follows
-	//
+	// TODO: move to NewBuilder to remove Builder.options field
 	configFormat := b.options.ConfigFormat
 	if configFormat != "" && configFormat != "json" && configFormat != "hcl" {
 		return RuntimeConfig{}, fmt.Errorf("config: -config-format must be either 'hcl' or 'json'")
@@ -251,19 +263,7 @@ func (b *Builder) Build() (rt RuntimeConfig, err error) {
 	// build the list of config sources
 	var srcs []Source
 	srcs = append(srcs, b.Head...)
-	for _, src := range b.Sources {
-		// skip file if it should not be parsed
-		if !b.shouldParseFile(src.Name) {
-			continue
-		}
-
-		// if config-format is set, files of any extension will be interpreted in that format
-		src.Format = FormatFrom(src.Name)
-		if configFormat != "" {
-			src.Format = configFormat
-		}
-		srcs = append(srcs, src)
-	}
+	srcs = append(srcs, b.Sources...)
 	srcs = append(srcs, b.Tail...)
 
 	// parse the config sources into a configuration

agent/config/builder_test.go

@@ -0,0 +1,93 @@
+package config
+
+import (
+	"fmt"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestShouldParseFile(t *testing.T) {
+	var testcases = []struct {
+		filename     string
+		configFormat string
+		expected     bool
+	}{
+		{filename: "config.json", expected: true},
+		{filename: "config.hcl", expected: true},
+		{filename: "config", configFormat: "hcl", expected: true},
+		{filename: "config.js", configFormat: "json", expected: true},
+		{filename: "config.yaml", expected: false},
+	}
+
+	for _, tc := range testcases {
+		name := fmt.Sprintf("filename=%s, format=%s", tc.filename, tc.configFormat)
+		t.Run(name, func(t *testing.T) {
+			require.Equal(t, tc.expected, shouldParseFile(tc.filename, tc.configFormat))
+		})
+	}
+}
+
+func TestNewBuilder_PopulatesSourcesFromConfigFiles(t *testing.T) {
+	paths := setupConfigFiles(t)
+
+	b, err := NewBuilder(BuilderOpts{ConfigFiles: paths})
+	require.NoError(t, err)
+
+	expected := []Source{
+		{Name: paths[0], Format: "hcl", Data: "content a"},
+		{Name: paths[1], Format: "json", Data: "content b"},
+		{Name: filepath.Join(paths[3], "a.hcl"), Format: "hcl", Data: "content a"},
+		{Name: filepath.Join(paths[3], "b.json"), Format: "json", Data: "content b"},
+	}
+	require.Equal(t, expected, b.Sources)
+}
+
+func TestNewBuilder_PopulatesSourcesFromConfigFiles_WithConfigFormat(t *testing.T) {
+	paths := setupConfigFiles(t)
+
+	b, err := NewBuilder(BuilderOpts{ConfigFiles: paths, ConfigFormat: "hcl"})
+	require.NoError(t, err)
+
+	expected := []Source{
+		{Name: paths[0], Format: "hcl", Data: "content a"},
+		{Name: paths[1], Format: "hcl", Data: "content b"},
+		{Name: paths[2], Format: "hcl", Data: "content c"},
+		{Name: filepath.Join(paths[3], "a.hcl"), Format: "hcl", Data: "content a"},
+		{Name: filepath.Join(paths[3], "b.json"), Format: "hcl", Data: "content b"},
+		{Name: filepath.Join(paths[3], "c.yaml"), Format: "hcl", Data: "content c"},
+	}
+	require.Equal(t, expected, b.Sources)
+}
+
+// TODO: this would be much nicer with gotest.tools/fs
+func setupConfigFiles(t *testing.T) []string {
+	t.Helper()
+	path, err := ioutil.TempDir("", t.Name())
+	require.NoError(t, err)
+	t.Cleanup(func() { os.RemoveAll(path) })
+
+	subpath := filepath.Join(path, "sub")
+	err = os.Mkdir(subpath, 0755)
+	require.NoError(t, err)
+
+	for _, dir := range []string{path, subpath} {
+		err = ioutil.WriteFile(filepath.Join(dir, "a.hcl"), []byte("content a"), 0644)
+		require.NoError(t, err)
+
+		err = ioutil.WriteFile(filepath.Join(dir, "b.json"), []byte("content b"), 0644)
+		require.NoError(t, err)
+
+		err = ioutil.WriteFile(filepath.Join(dir, "c.yaml"), []byte("content c"), 0644)
+		require.NoError(t, err)
+	}
+	return []string{
+		filepath.Join(path, "a.hcl"),
+		filepath.Join(path, "b.json"),
+		filepath.Join(path, "c.yaml"),
+		subpath,
+	}
+}

agent/config/config.go

@@ -3,7 +3,6 @@ package config
 import (
 	"encoding/json"
 	"fmt"
-	"strings"
 
 	"github.com/hashicorp/consul/lib/decode"
 	"github.com/hashicorp/hcl"
@@ -21,17 +20,6 @@ type Source struct {
 	Data   string
 }
 
-func FormatFrom(name string) string {
-	switch {
-	case strings.HasSuffix(name, ".json"):
-		return "json"
-	case strings.HasSuffix(name, ".hcl"):
-		return "hcl"
-	default:
-		return ""
-	}
-}
-
 // Parse parses a config fragment in either JSON or HCL format.
 func Parse(data string, format string) (c Config, md mapstructure.Metadata, err error) {
 	var raw map[string]interface{}

agent/config/runtime_test.go

@@ -6003,16 +6003,11 @@ func TestFullConfig(t *testing.T) {
 				t.Fatalf("ParseFlags: %s", err)
 			}
 
-			// ensure that all fields are set to unique non-zero values
-			// if err := nonZero("Config", nil, c); err != nil {
-			// 	t.Fatal(err)
-			// }
-
 			b, err := NewBuilder(flags)
 			if err != nil {
 				t.Fatalf("NewBuilder: %s", err)
 			}
-			b.Sources = append(b.Sources, Source{Name: "full." + format, Data: data})
+			b.Sources = append(b.Sources, Source{Name: "full." + format, Data: data, Format: format})
 			b.Tail = append(b.Tail, tail[format]...)
 			b.Tail = append(b.Tail, VersionSource("JNtPSav3", "R909Hblt", "ZT1JOQLn"))