From 0ca0f3ded762db6a5089d53af1c701da510b4dc0 Mon Sep 17 00:00:00 2001 From: Brad Davidson Date: Mon, 29 Jul 2024 19:22:18 +0000 Subject: [PATCH 1/3] Don't set K3S_DATA_DIR env var This was only used to pass the bundled strongswan path through to the flannel ipsec backend, and is no longer needed. Ref: #719 Signed-off-by: Brad Davidson --- cmd/k3s/main.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/cmd/k3s/main.go b/cmd/k3s/main.go index a3401158bea1..df2f0a4c61ca 100644 --- a/cmd/k3s/main.go +++ b/cmd/k3s/main.go @@ -193,9 +193,6 @@ func stageAndRun(dataDir, cmd string, args []string, calledAsInternal bool) erro if err := os.Setenv("PATH", pathEnv); err != nil { return err } - if err := os.Setenv(version.ProgramUpper+"_DATA_DIR", dir); err != nil { - return err - } cmd, err = exec.LookPath(cmd) if err != nil { From c271c044571f8f3ea44b1717c59a872d16d1b752 Mon Sep 17 00:00:00 2001 From: Brad Davidson Date: Mon, 29 Jul 2024 19:22:50 +0000 Subject: [PATCH 2/3] Add K3S_DATA_DIR as env var for --data-dir flag Signed-off-by: Brad Davidson --- cmd/k3s/main.go | 7 +++++-- pkg/cli/cmds/agent.go | 3 +++ pkg/cli/cmds/certs.go | 6 +----- pkg/cli/cmds/root.go | 5 +---- pkg/cli/cmds/server.go | 1 + 5 files changed, 11 insertions(+), 11 deletions(-) diff --git a/cmd/k3s/main.go b/cmd/k3s/main.go index df2f0a4c61ca..6a17998505ba 100644 --- a/cmd/k3s/main.go +++ b/cmd/k3s/main.go @@ -86,11 +86,14 @@ func main() { } } -// findDataDir reads data-dir settings from the CLI args and config file. +// findDataDir reads data-dir settings from the environment, CLI args, and config file. // If not found, the default will be used, which varies depending on whether // k3s is being run as root or not. func findDataDir(args []string) string { - var dataDir string + dataDir := os.Getenv(version.ProgramUpper + "_DATA_DIR") + if dataDir != "" { + return dataDir + } fs := pflag.NewFlagSet("data-dir-set", pflag.ContinueOnError) fs.ParseErrorsWhitelist.UnknownFlags = true fs.SetOutput(io.Discard) diff --git a/pkg/cli/cmds/agent.go b/pkg/cli/cmds/agent.go index 16e0a196c106..751c6c31b144 100644 --- a/pkg/cli/cmds/agent.go +++ b/pkg/cli/cmds/agent.go @@ -266,11 +266,14 @@ func NewAgentCommand(action func(ctx *cli.Context) error) cli.Command { EnvVar: version.ProgramUpper + "_URL", Destination: &AgentConfig.ServerURL, }, + // Note that this is different from DataDirFlag used elswhere in the CLI, + // as this is bound to AgentConfig instead of ServerConfig. &cli.StringFlag{ Name: "data-dir,d", Usage: "(agent/data) Folder to hold state", Destination: &AgentConfig.DataDir, Value: "/var/lib/rancher/" + version.Program + "", + EnvVar: version.ProgramUpper + "_DATA_DIR", }, NodeNameFlag, WithNodeIDFlag, diff --git a/pkg/cli/cmds/certs.go b/pkg/cli/cmds/certs.go index 50834e69fc89..0c3ee9c7026d 100644 --- a/pkg/cli/cmds/certs.go +++ b/pkg/cli/cmds/certs.go @@ -28,6 +28,7 @@ var ( }, } CertRotateCACommandFlags = []cli.Flag{ + DataDirFlag, cli.StringFlag{ Name: "server,s", Usage: "(cluster) Server to connect to", @@ -35,11 +36,6 @@ var ( Value: "https://127.0.0.1:6443", Destination: &ServerConfig.ServerURL, }, - cli.StringFlag{ - Name: "data-dir,d", - Usage: "(data) Folder to hold state default /var/lib/rancher/" + version.Program + " or ${HOME}/.rancher/" + version.Program + " if not root", - Destination: &ServerConfig.DataDir, - }, cli.StringFlag{ Name: "path", Usage: "Path to directory containing new CA certificates", diff --git a/pkg/cli/cmds/root.go b/pkg/cli/cmds/root.go index 6ab66400fdbc..e57133baae5b 100644 --- a/pkg/cli/cmds/root.go +++ b/pkg/cli/cmds/root.go @@ -41,10 +41,7 @@ func NewApp() *cli.App { } app.Flags = []cli.Flag{ DebugFlag, - &cli.StringFlag{ - Name: "data-dir,d", - Usage: "(data) Folder to hold state (default: /var/lib/rancher/" + version.Program + " or ${HOME}/.rancher/" + version.Program + " if not root)", - }, + DataDirFlag, } return app diff --git a/pkg/cli/cmds/server.go b/pkg/cli/cmds/server.go index c7fec4f54139..91dafa30995d 100644 --- a/pkg/cli/cmds/server.go +++ b/pkg/cli/cmds/server.go @@ -117,6 +117,7 @@ var ( Name: "data-dir,d", Usage: "(data) Folder to hold state default /var/lib/rancher/" + version.Program + " or ${HOME}/.rancher/" + version.Program + " if not root", Destination: &ServerConfig.DataDir, + EnvVar: version.ProgramUpper + "_DATA_DIR", } ServerToken = &cli.StringFlag{ Name: "token,t", From cffe82ebc1ad282cb9a3079fc4219d520b8ea312 Mon Sep 17 00:00:00 2001 From: Brad Davidson Date: Mon, 29 Jul 2024 20:36:58 +0000 Subject: [PATCH 3/3] 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