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 <brad.davidson@rancher.com>

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 {
-			return "", err
+	if configFile := p.findConfigFileFlag(args); configFile != "" {
+		if _, err := os.Stat(configFile); err == nil {
+			files = append(files, configFile)
 		}
 
-		files, err := dotDFiles(configFile)
+		dropinFiles, err := dotDFiles(configFile)
 		if err != nil {
 			return "", err
 		}
-		files = append([]string{configFile}, files...)
-		for _, file := range files {
-			bytes, err := readConfigFileData(file)
-			if err != nil {
-				return "", err
-			}
+		files = append(files, dropinFiles...)
+	}
 
-			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
-					}
+	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
 				}
 			}
 		}
@@ -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 {

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",

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

pkg/configfilearg/testdata/dropin-only.yaml.d/02-data-ignore-this.txt

@@ -0,0 +1 @@
+foo-bar: ignored

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

pkg/configfilearg/testdata/invalid-dropin.yaml.d/01-data.yml

@@ -0,0 +1 @@
+!invalid

pkg/configfilearg/testdata/invalid.yaml

@@ -0,0 +1 @@
+!invalid