From 2c2da41b3d0c30b53967a50407877a5ef410de92 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 1 May 2020 18:45:15 -0400 Subject: [PATCH] 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 | 92 +++++++++++++++++------------------ agent/config/builder_test.go | 93 ++++++++++++++++++++++++++++++++++++ agent/config/config.go | 12 ----- agent/config/runtime_test.go | 7 +-- 4 files changed, 140 insertions(+), 64 deletions(-) create mode 100644 agent/config/builder_test.go diff --git a/agent/config/builder.go b/agent/config/builder.go index 3110c5a14..f1d787ebc 100644 --- a/agent/config/builder.go +++ b/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 diff --git a/agent/config/builder_test.go b/agent/config/builder_test.go new file mode 100644 index 000000000..aa6cec17f --- /dev/null +++ b/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, + } +} diff --git a/agent/config/config.go b/agent/config/config.go index c11e7fd2e..8e13b8049 100644 --- a/agent/config/config.go +++ b/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{} diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go index 913128086..2598946bb 100644 --- a/agent/config/runtime_test.go +++ b/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"))