From f646604010affc6a1d3233a8a0870bca46bf80cf Mon Sep 17 00:00:00 2001 From: Brad Davidson Date: Mon, 29 Jul 2024 20:36:58 +0000 Subject: [PATCH] Fix inconsistent loading of config dropins when config file does not exist FindString would silently skip parsing dropins if the main config file didn't exist. If a custom config file path was passed it would raise an error, but if we were parsing the default config file and it didn't exist it would just silently fail to load the dropins. Signed-off-by: Brad Davidson --- pkg/configfilearg/parser.go | 96 ++++++------ pkg/configfilearg/parser_test.go | 140 +++++++++++++----- .../testdata/dropin-only.yaml.d/01-data.yml | 15 ++ .../02-data-ignore-this.txt | 1 + .../testdata/dropin-only.yaml.d/02-data.yaml | 10 ++ .../invalid-dropin.yaml.d/01-data.yml | 1 + pkg/configfilearg/testdata/invalid.yaml | 1 + 7 files changed, 180 insertions(+), 84 deletions(-) create mode 100644 pkg/configfilearg/testdata/dropin-only.yaml.d/01-data.yml create mode 100644 pkg/configfilearg/testdata/dropin-only.yaml.d/02-data-ignore-this.txt create mode 100644 pkg/configfilearg/testdata/dropin-only.yaml.d/02-data.yaml create mode 100644 pkg/configfilearg/testdata/invalid-dropin.yaml.d/01-data.yml create mode 100644 pkg/configfilearg/testdata/invalid.yaml diff --git a/pkg/configfilearg/parser.go b/pkg/configfilearg/parser.go index 077b89922b98..c17a8f184ae7 100644 --- a/pkg/configfilearg/parser.go +++ b/pkg/configfilearg/parser.go @@ -42,12 +42,12 @@ func (p *Parser) Parse(args []string) ([]string, error) { return args, nil } - configFile, isSet := p.findConfigFileFlag(args) - if configFile != "" { + if configFile := p.findConfigFileFlag(args); configFile != "" { values, err := readConfigFile(configFile) - if !isSet && os.IsNotExist(err) { - return args, nil - } else if err != nil { + if err != nil { + if os.IsNotExist(err) { + return args, nil + } return nil, err } if len(args) > 1 { @@ -99,49 +99,50 @@ func (p *Parser) stripInvalidFlags(command string, args []string) ([]string, err return result, nil } +// FindString returns the string value of a flag, checking the CLI args, +// config file, and config file dropins. If the value is not found, +// an empty string is returned. It is not an error if no args, +// configfile, or dropins are present. func (p *Parser) FindString(args []string, target string) (string, error) { - // Check for --help or --version flags, which override any other flags if val, found := p.findOverrideFlag(args); found { return val, nil } - configFile, isSet := p.findConfigFileFlag(args) + var files []string var lastVal string - if configFile != "" { - _, err := os.Stat(configFile) - if !isSet && os.IsNotExist(err) { - return "", nil - } else if err != nil { + if configFile := p.findConfigFileFlag(args); configFile != "" { + if _, err := os.Stat(configFile); err == nil { + files = append(files, configFile) + } + + dropinFiles, err := dotDFiles(configFile) + if err != nil { return "", err } + files = append(files, dropinFiles...) + } - files, err := dotDFiles(configFile) + for _, file := range files { + bytes, err := readConfigFileData(file) if err != nil { return "", err } - files = append([]string{configFile}, files...) - for _, file := range files { - bytes, err := readConfigFileData(file) - if err != nil { - return "", err - } - data := yaml.MapSlice{} - if err := yaml.Unmarshal(bytes, &data); err != nil { - return "", err - } - for _, i := range data { - k, v := convert.ToString(i.Key), convert.ToString(i.Value) - isAppend := strings.HasSuffix(k, "+") - k = strings.TrimSuffix(k, "+") - if k == target { - if isAppend { - lastVal = lastVal + "," + v - } else { - lastVal = v - } + data := yaml.MapSlice{} + if err := yaml.Unmarshal(bytes, &data); err != nil { + return "", err + } + for _, i := range data { + k, v := convert.ToString(i.Key), convert.ToString(i.Value) + isAppend := strings.HasSuffix(k, "+") + k = strings.TrimSuffix(k, "+") + if k == target { + if isAppend { + lastVal = lastVal + "," + v + } else { + lastVal = v } } } @@ -161,26 +162,28 @@ func (p *Parser) findOverrideFlag(args []string) (string, bool) { return "", false } -func (p *Parser) findConfigFileFlag(args []string) (string, bool) { +// findConfigFileFlag returns the value of the config file env var or CLI flag. +// If neither are set, it returns the default value. +func (p *Parser) findConfigFileFlag(args []string) string { if envVal := os.Getenv(p.EnvName); p.EnvName != "" && envVal != "" { - return envVal, true + return envVal } for i, arg := range args { for _, flagName := range p.ConfigFlags { if flagName == arg { if len(args) > i+1 { - return args[i+1], true + return args[i+1] } // This is actually invalid, so we rely on the CLI parser after the fact flagging it as bad - return "", false + return "" } else if strings.HasPrefix(arg, flagName+"=") { - return arg[len(flagName)+1:], true + return arg[len(flagName)+1:] } } } - return p.DefaultConfig, false + return p.DefaultConfig } func (p *Parser) findStart(args []string) ([]string, []string, bool) { @@ -237,17 +240,23 @@ func dotDFiles(basefile string) (result []string, _ error) { return } +// readConfigFile returns a flattened arg list generated from the specified config +// file, and any config file dropins in the dropin directory that corresponds to that +// config file. The config file or at least one dropin must exist. func readConfigFile(file string) (result []string, _ error) { files, err := dotDFiles(file) if err != nil { return nil, err } - _, err = os.Stat(file) - if os.IsNotExist(err) && len(files) > 0 { - } else if err != nil { - return nil, err + if _, err = os.Stat(file); err != nil { + // If the config file doesn't exist and we have dropins that's fine. + // Other errors are bubbled up regardless of how many dropins we have. + if !(os.IsNotExist(err) && len(files) > 0) { + return nil, err + } } else { + // The config file exists, load it first. files = append([]string{file}, files...) } @@ -321,6 +330,7 @@ func toSlice(v interface{}) []interface{} { } } +// readConfigFileData returns the contents of a local or remote file func readConfigFileData(file string) ([]byte, error) { u, err := url.Parse(file) if err != nil { diff --git a/pkg/configfilearg/parser_test.go b/pkg/configfilearg/parser_test.go index 1dc4640ab9d8..84f7bac59c45 100644 --- a/pkg/configfilearg/parser_test.go +++ b/pkg/configfilearg/parser_test.go @@ -120,61 +120,52 @@ func Test_UnitParser_findConfigFileFlag(t *testing.T) { fields fields arg []string want string - found bool }{ { - name: "default case", - arg: nil, - found: false, + name: "default case", + arg: nil, }, { - name: "simple case", - arg: []string{"asdf", "-c", "value"}, - want: "value", - found: true, + name: "simple case", + arg: []string{"asdf", "-c", "value"}, + want: "value", }, { - name: "invalid args string", - arg: []string{"-c"}, - found: false, + name: "invalid args string", + arg: []string{"-c"}, }, { - name: "empty arg value", - arg: []string{"-c="}, - found: true, + name: "empty arg value", + arg: []string{"-c="}, }, { name: "empty arg value override default", fields: fields{ DefaultConfig: "def", }, - arg: []string{"-c="}, - found: true, + arg: []string{"-c="}, }, { fields: fields{ DefaultConfig: "def", }, - arg: []string{"-c"}, - found: false, - name: "invalid args always return no value", + arg: []string{"-c"}, + name: "invalid args always return no value", }, { fields: fields{ DefaultConfig: "def", }, - arg: []string{"-c", "value"}, - want: "value", - found: true, - name: "value override default", + arg: []string{"-c", "value"}, + want: "value", + name: "value override default", }, { fields: fields{ DefaultConfig: "def", }, - want: "def", - found: false, - name: "default gets used when nothing is passed", + want: "def", + name: "default gets used when nothing is passed", }, { name: "env override args", @@ -182,18 +173,16 @@ func Test_UnitParser_findConfigFileFlag(t *testing.T) { DefaultConfig: "def", env: "env", }, - arg: []string{"-c", "value"}, - want: "env", - found: true, + arg: []string{"-c", "value"}, + want: "env", }, { name: "garbage in start and end", fields: fields{ DefaultConfig: "def", }, - arg: []string{"before", "-c", "value", "after"}, - want: "value", - found: true, + arg: []string{"before", "-c", "value", "after"}, + want: "value", }, } for _, tt := range tests { @@ -207,13 +196,10 @@ func Test_UnitParser_findConfigFileFlag(t *testing.T) { defer os.Unsetenv(tt.fields.env) os.Setenv(p.EnvName, tt.fields.env) - got, found := p.findConfigFileFlag(tt.arg) + got := p.findConfigFileFlag(tt.arg) if got != tt.want { t.Errorf("Parser.findConfigFileFlag() got = %+v\nWant = %+v", got, tt.want) } - if found != tt.found { - t.Errorf("Parser.findConfigFileFlag() found = %+v\nWant = %+v", found, tt.found) - } }) } } @@ -286,13 +272,33 @@ func Test_UnitParser_Parse(t *testing.T) { want: []string{"server"}, }, { - name: "fail when missing config", + name: "ignore missing config when set", fields: fields{ After: []string{"server", "agent"}, FlagNames: []string{"-c", "--config"}, DefaultConfig: "missing", }, - arg: []string{"server", "-c=missing"}, + arg: []string{"server", "-c=missing"}, + want: []string{"server", "-c=missing"}, + }, + { + name: "fail when config cannot be parsed", + fields: fields{ + After: []string{"server", "agent"}, + FlagNames: []string{"-c", "--config"}, + DefaultConfig: "./testdata/invalid.yaml", + }, + arg: []string{"server"}, + wantErr: true, + }, + { + name: "fail when dropin cannot be parsed", + fields: fields{ + After: []string{"server", "agent"}, + FlagNames: []string{"-c", "--config"}, + DefaultConfig: "./testdata/invalid-dropin.yaml", + }, + arg: []string{"server"}, wantErr: true, }, { @@ -404,7 +410,59 @@ func Test_UnitParser_FindString(t *testing.T) { want: "", }, { - name: "Multiple custom configs exist, target exists in a secondary config", + name: "Default config file does not exist but dropin exists, target does not exist", + fields: fields{ + FlagNames: []string{"-c", "--config"}, + EnvName: "_TEST_ENV", + DefaultConfig: "./testdata/dropin-only.yaml", + }, + args: args{ + osArgs: []string{}, + target: "tls", + }, + want: "", + }, + { + name: "Default config file does not exist but dropin exists, target exists", + fields: fields{ + FlagNames: []string{"-c", "--config"}, + EnvName: "_TEST_ENV", + DefaultConfig: "./testdata/dropin-only.yaml", + }, + args: args{ + osArgs: []string{}, + target: "foo-bar", + }, + want: "bar-foo", + }, + { + name: "Custom config file does not exist but dropin exists, target does not exist", + fields: fields{ + FlagNames: []string{"-c", "--config"}, + EnvName: "_TEST_ENV", + DefaultConfig: "./testdata/defaultdata.yaml", + }, + args: args{ + osArgs: []string{"-c", "./testdata/dropin-only.yaml"}, + target: "tls", + }, + want: "", + }, + { + name: "Custom config file does not exist but dropin exists, target exists", + fields: fields{ + FlagNames: []string{"-c", "--config"}, + EnvName: "_TEST_ENV", + DefaultConfig: "./testdata/defaultdata.yaml", + }, + args: args{ + osArgs: []string{"-c", "./testdata/dropin-only.yaml"}, + target: "foo-bar", + }, + want: "bar-foo", + }, + { + name: "Multiple custom configs exist, target exists in a dropin config", fields: fields{ FlagNames: []string{"-c", "--config"}, EnvName: "_TEST_ENV", @@ -417,7 +475,7 @@ func Test_UnitParser_FindString(t *testing.T) { want: "beta", }, { - name: "Multiple custom configs exist, multiple targets exist in multiple secondary config, replacement", + name: "Multiple custom configs exist, multiple targets exist in multiple dropin config, replacement", fields: fields{ FlagNames: []string{"-c", "--config"}, EnvName: "_TEST_ENV", @@ -430,7 +488,7 @@ func Test_UnitParser_FindString(t *testing.T) { want: "bar-foo", }, { - name: "Multiple custom configs exist, multiple targets exist in multiple secondary config, appending", + name: "Multiple custom configs exist, multiple targets exist in multiple dropin config, appending", fields: fields{ FlagNames: []string{"-c", "--config"}, EnvName: "_TEST_ENV", diff --git a/pkg/configfilearg/testdata/dropin-only.yaml.d/01-data.yml b/pkg/configfilearg/testdata/dropin-only.yaml.d/01-data.yml new file mode 100644 index 000000000000..9a0a098f7c73 --- /dev/null +++ b/pkg/configfilearg/testdata/dropin-only.yaml.d/01-data.yml @@ -0,0 +1,15 @@ +foo-bar: get-overriden +a-slice: +- 1 +- "1.5" +- "2" +- "" +- three +b-string: one +c-slice: +- one +- two +d-slice: +- one +- two +f-string: beta \ No newline at end of file diff --git a/pkg/configfilearg/testdata/dropin-only.yaml.d/02-data-ignore-this.txt b/pkg/configfilearg/testdata/dropin-only.yaml.d/02-data-ignore-this.txt new file mode 100644 index 000000000000..f8048ad0559b --- /dev/null +++ b/pkg/configfilearg/testdata/dropin-only.yaml.d/02-data-ignore-this.txt @@ -0,0 +1 @@ +foo-bar: ignored diff --git a/pkg/configfilearg/testdata/dropin-only.yaml.d/02-data.yaml b/pkg/configfilearg/testdata/dropin-only.yaml.d/02-data.yaml new file mode 100644 index 000000000000..0947f7ec4bd7 --- /dev/null +++ b/pkg/configfilearg/testdata/dropin-only.yaml.d/02-data.yaml @@ -0,0 +1,10 @@ +foo-bar: bar-foo +b-string+: two +c-slice+: +- three +d-slice: +- three +- four +e-slice+: +- one +- two \ No newline at end of file diff --git a/pkg/configfilearg/testdata/invalid-dropin.yaml.d/01-data.yml b/pkg/configfilearg/testdata/invalid-dropin.yaml.d/01-data.yml new file mode 100644 index 000000000000..4bd4de28404f --- /dev/null +++ b/pkg/configfilearg/testdata/invalid-dropin.yaml.d/01-data.yml @@ -0,0 +1 @@ +!invalid diff --git a/pkg/configfilearg/testdata/invalid.yaml b/pkg/configfilearg/testdata/invalid.yaml new file mode 100644 index 000000000000..4bd4de28404f --- /dev/null +++ b/pkg/configfilearg/testdata/invalid.yaml @@ -0,0 +1 @@ +!invalid