Skip to content

Commit

Permalink
Fix inconsistent loading of config dropins when config file does not …
Browse files Browse the repository at this point in the history 
…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 <[email protected]>
  • Loading branch information
@brandond
brandond committed Jul 29, 2024
1 parent ff06b10 commit f646604
Show file tree
Hide file tree
Showing 7 changed files with 180 additions and 84 deletions.
96 changes: 53 additions & 43 deletions pkg/configfilearg/parser.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
}
}
Expand All @@ -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) {
Expand Down Expand Up @@ -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...)
}

Expand Down Expand Up @@ -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 {
Expand Down
140 changes: 99 additions & 41 deletions pkg/configfilearg/parser_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,80 +120,69 @@ 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",
fields: fields{
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 {
Expand All @@ -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)
}
})
}
}
Expand Down Expand Up @@ -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,
},
{
Expand Down Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand Down
Loading

0 comments on commit f646604

Please sign in to comment.