From b865512ff2d3d6e693df5d06b24a32989efe1a42 Mon Sep 17 00:00:00 2001 From: Derek Nola Date: Tue, 2 May 2023 15:09:08 -0700 Subject: [PATCH] [Release-1.24] CLI + Config Enhancement (#7407) * Handle multiple arguments with StringSlice flags (#7380) * Add helper function for multiple arguments in stringslice * Cleanup server setup with util function Signed-off-by: Derek Nola * Enable FindString to search dotD config files (#7323) * Enable FindString to search dotD config files * Address multiple arg cases Signed-off-by: Derek Nola --- pkg/cli/server/server.go | 48 ++++++++--------- pkg/configfilearg/parser.go | 36 +++++++++---- pkg/configfilearg/parser_test.go | 49 ++++++++++++++++-- pkg/configfilearg/testdata/data.yaml | 1 + .../testdata/data.yaml.d/01-data.yml | 3 +- pkg/daemons/control/deps/deps_test.go | 51 +++++++++++++++++++ pkg/util/client.go | 13 +++++ pkg/util/client_test.go | 39 ++++++++++++++ 8 files changed, 198 insertions(+), 42 deletions(-) create mode 100644 pkg/daemons/control/deps/deps_test.go create mode 100644 pkg/util/client_test.go diff --git a/pkg/cli/server/server.go b/pkg/cli/server/server.go index 1434fdcb1cd3..4cd562d02339 100644 --- a/pkg/cli/server/server.go +++ b/pkg/cli/server/server.go @@ -121,7 +121,7 @@ func run(app *cli.Context, cfg *cmds.Server, leaderControllers server.CustomCont serverConfig.ControlConfig.KubeConfigMode = cfg.KubeConfigMode serverConfig.ControlConfig.Rootless = cfg.Rootless serverConfig.ControlConfig.ServiceLBNamespace = cfg.ServiceLBNamespace - serverConfig.ControlConfig.SANs = cfg.TLSSan + serverConfig.ControlConfig.SANs = util.SplitStringSlice(cfg.TLSSan) serverConfig.ControlConfig.BindAddress = cfg.BindAddress serverConfig.ControlConfig.SupervisorPort = cfg.SupervisorPort serverConfig.ControlConfig.HTTPSPort = cfg.HTTPSPort @@ -251,14 +251,12 @@ func run(app *cli.Context, cfg *cmds.Server, leaderControllers server.CustomCont } cmds.ServerConfig.ClusterCIDR.Set(clusterCIDR) } - for _, cidr := range cmds.ServerConfig.ClusterCIDR { - for _, v := range strings.Split(cidr, ",") { - _, parsed, err := net.ParseCIDR(v) - if err != nil { - return errors.Wrapf(err, "invalid cluster-cidr %s", v) - } - serverConfig.ControlConfig.ClusterIPRanges = append(serverConfig.ControlConfig.ClusterIPRanges, parsed) + for _, cidr := range util.SplitStringSlice(cmds.ServerConfig.ClusterCIDR) { + _, parsed, err := net.ParseCIDR(cidr) + if err != nil { + return errors.Wrapf(err, "invalid cluster-cidr %s", cidr) } + serverConfig.ControlConfig.ClusterIPRanges = append(serverConfig.ControlConfig.ClusterIPRanges, parsed) } // set ClusterIPRange to the first IPv4 block, for legacy clients @@ -277,14 +275,12 @@ func run(app *cli.Context, cfg *cmds.Server, leaderControllers server.CustomCont } cmds.ServerConfig.ServiceCIDR.Set(serviceCIDR) } - for _, cidr := range cmds.ServerConfig.ServiceCIDR { - for _, v := range strings.Split(cidr, ",") { - _, parsed, err := net.ParseCIDR(v) - if err != nil { - return errors.Wrapf(err, "invalid service-cidr %s", v) - } - serverConfig.ControlConfig.ServiceIPRanges = append(serverConfig.ControlConfig.ServiceIPRanges, parsed) + for _, cidr := range util.SplitStringSlice(cmds.ServerConfig.ServiceCIDR) { + _, parsed, err := net.ParseCIDR(cidr) + if err != nil { + return errors.Wrapf(err, "invalid service-cidr %s", cidr) } + serverConfig.ControlConfig.ServiceIPRanges = append(serverConfig.ControlConfig.ServiceIPRanges, parsed) } // set ServiceIPRange to the first IPv4 block, for legacy clients @@ -319,14 +315,12 @@ func run(app *cli.Context, cfg *cmds.Server, leaderControllers server.CustomCont serverConfig.ControlConfig.ClusterDNS = clusterDNS serverConfig.ControlConfig.ClusterDNSs = []net.IP{serverConfig.ControlConfig.ClusterDNS} } else { - for _, ip := range cmds.ServerConfig.ClusterDNS { - for _, v := range strings.Split(ip, ",") { - parsed := net.ParseIP(v) - if parsed == nil { - return fmt.Errorf("invalid cluster-dns address %s", v) - } - serverConfig.ControlConfig.ClusterDNSs = append(serverConfig.ControlConfig.ClusterDNSs, parsed) + for _, ip := range util.SplitStringSlice(cmds.ServerConfig.ClusterDNS) { + parsed := net.ParseIP(ip) + if parsed == nil { + return fmt.Errorf("invalid cluster-dns address %s", ip) } + serverConfig.ControlConfig.ClusterDNSs = append(serverConfig.ControlConfig.ClusterDNSs, parsed) } // Set ClusterDNS to the first IPv4 address, for legacy clients // unless only IPv6 range given @@ -360,12 +354,10 @@ func run(app *cli.Context, cfg *cmds.Server, leaderControllers server.CustomCont } } serverConfig.ControlConfig.Disables = map[string]bool{} - for _, disable := range app.StringSlice("disable") { - for _, v := range strings.Split(disable, ",") { - v = strings.TrimSpace(v) - serverConfig.ControlConfig.Skips[v] = true - serverConfig.ControlConfig.Disables[v] = true - } + for _, disable := range util.SplitStringSlice(app.StringSlice("disable")) { + disable = strings.TrimSpace(disable) + serverConfig.ControlConfig.Skips[disable] = true + serverConfig.ControlConfig.Disables[disable] = true } if serverConfig.ControlConfig.Skips["servicelb"] { serverConfig.ControlConfig.DisableServiceLB = true diff --git a/pkg/configfilearg/parser.go b/pkg/configfilearg/parser.go index 764aaf0c6caf..f93e13911b83 100644 --- a/pkg/configfilearg/parser.go +++ b/pkg/configfilearg/parser.go @@ -98,28 +98,46 @@ func (p *Parser) stripInvalidFlags(command string, args []string) ([]string, err func (p *Parser) FindString(args []string, target string) (string, error) { configFile, isSet := p.findConfigFileFlag(args) + var lastVal string if configFile != "" { - bytes, err := readConfigFileData(configFile) + + _, err := os.Stat(configFile) if !isSet && os.IsNotExist(err) { return "", nil } else if err != nil { return "", err } - data := yaml.MapSlice{} - if err := yaml.Unmarshal(bytes, &data); err != nil { + files, 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 + } - for _, i := range data { - k, v := convert.ToString(i.Key), convert.ToString(i.Value) - if k == target { - return v, nil + 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 + } + } } } } - - return "", nil + return lastVal, nil } func (p *Parser) findConfigFileFlag(args []string) (string, bool) { diff --git a/pkg/configfilearg/parser_test.go b/pkg/configfilearg/parser_test.go index 776d43774f17..66faa7586810 100644 --- a/pkg/configfilearg/parser_test.go +++ b/pkg/configfilearg/parser_test.go @@ -221,6 +221,7 @@ func Test_UnitParser_findConfigFileFlag(t *testing.T) { func Test_UnitParser_Parse(t *testing.T) { testDataOutput := []string{ "--foo-bar=bar-foo", + "--alice=bob", "--a-slice=1", "--a-slice=1.5", "--a-slice=2", @@ -237,6 +238,7 @@ func Test_UnitParser_Parse(t *testing.T) { "--c-slice=three", "--d-slice=three", "--d-slice=four", + "--f-string=beta", "--e-slice=one", "--e-slice=two", } @@ -376,7 +378,7 @@ func Test_UnitParser_FindString(t *testing.T) { want: "", }, { - name: "A custom config yaml exists, target exists", + name: "A custom config exists, target exists", fields: fields{ FlagNames: []string{"-c", "--config"}, EnvName: "_TEST_ENV", @@ -384,12 +386,12 @@ func Test_UnitParser_FindString(t *testing.T) { }, args: args{ osArgs: []string{"-c", "./testdata/data.yaml"}, - target: "foo-bar", + target: "alice", }, - want: "baz", + want: "bob", }, { - name: "A custom config yaml exists, target does not exist", + name: "A custom config exists, target does not exist", fields: fields{ FlagNames: []string{"-c", "--config"}, EnvName: "_TEST_ENV", @@ -401,6 +403,45 @@ func Test_UnitParser_FindString(t *testing.T) { }, want: "", }, + { + name: "Multiple custom configs exist, target exists in a secondary config", + fields: fields{ + FlagNames: []string{"-c", "--config"}, + EnvName: "_TEST_ENV", + DefaultConfig: "./testdata/data.yaml", + }, + args: args{ + osArgs: []string{"-c", "./testdata/data.yaml"}, + target: "f-string", + }, + want: "beta", + }, + { + name: "Multiple custom configs exist, multiple targets exist in multiple secondary config, replacement", + fields: fields{ + FlagNames: []string{"-c", "--config"}, + EnvName: "_TEST_ENV", + DefaultConfig: "./testdata/data.yaml", + }, + args: args{ + osArgs: []string{"-c", "./testdata/data.yaml"}, + target: "foo-bar", + }, + want: "bar-foo", + }, + { + name: "Multiple custom configs exist, multiple targets exist in multiple secondary config, appending", + fields: fields{ + FlagNames: []string{"-c", "--config"}, + EnvName: "_TEST_ENV", + DefaultConfig: "./testdata/data.yaml", + }, + args: args{ + osArgs: []string{"-c", "./testdata/data.yaml"}, + target: "b-string", + }, + want: "one,two", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/configfilearg/testdata/data.yaml b/pkg/configfilearg/testdata/data.yaml index 35105341ad7a..712668e5ab31 100644 --- a/pkg/configfilearg/testdata/data.yaml +++ b/pkg/configfilearg/testdata/data.yaml @@ -1,4 +1,5 @@ foo-bar: baz +alice: bob a-slice: - 1 - "2" diff --git a/pkg/configfilearg/testdata/data.yaml.d/01-data.yml b/pkg/configfilearg/testdata/data.yaml.d/01-data.yml index d118ca5a00f7..9a0a098f7c73 100644 --- a/pkg/configfilearg/testdata/data.yaml.d/01-data.yml +++ b/pkg/configfilearg/testdata/data.yaml.d/01-data.yml @@ -11,4 +11,5 @@ c-slice: - two d-slice: - one -- two \ No newline at end of file +- two +f-string: beta \ No newline at end of file diff --git a/pkg/daemons/control/deps/deps_test.go b/pkg/daemons/control/deps/deps_test.go new file mode 100644 index 000000000000..297aa5aa0d21 --- /dev/null +++ b/pkg/daemons/control/deps/deps_test.go @@ -0,0 +1,51 @@ +package deps + +import ( + "net" + "reflect" + "testing" + + certutil "github.com/rancher/dynamiclistener/cert" +) + +func Test_UnitAddSANs(t *testing.T) { + type args struct { + altNames *certutil.AltNames + sans []string + } + tests := []struct { + name string + args args + want certutil.AltNames + }{ + { + name: "One IP, One DNS", + args: args{ + altNames: &certutil.AltNames{}, + sans: []string{"192.168.205.10", "192.168.205.10.nip.io"}, + }, + want: certutil.AltNames{ + IPs: []net.IP{net.ParseIP("192.168.205.10")}, + DNSNames: []string{"192.168.205.10.nip.io"}, + }, + }, + { + name: "Two IP, No DNS", + args: args{ + altNames: &certutil.AltNames{}, + sans: []string{"192.168.205.10", "10.168.21.15"}, + }, + want: certutil.AltNames{ + IPs: []net.IP{net.ParseIP("192.168.205.10"), net.ParseIP("10.168.21.15")}, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + addSANs(tt.args.altNames, tt.args.sans) + if !reflect.DeepEqual(*tt.args.altNames, tt.want) { + t.Errorf("addSANs() = %v, want %v", *tt.args.altNames, tt.want) + } + }) + } +} diff --git a/pkg/util/client.go b/pkg/util/client.go index 52c396e4c447..6c6fefd00060 100644 --- a/pkg/util/client.go +++ b/pkg/util/client.go @@ -1,6 +1,8 @@ package util import ( + "strings" + "github.com/k3s-io/k3s/pkg/datadir" clientset "k8s.io/client-go/kubernetes" "k8s.io/client-go/tools/clientcmd" @@ -27,3 +29,14 @@ func GetClientSet(file string) (clientset.Interface, error) { return clientset.NewForConfig(restConfig) } + +// SplitStringSlice is a helper function to handle StringSliceFlag containing multiple values +// By default, StringSliceFlag only supports repeated values, not multiple values +// e.g. --foo="bar,car" --foo=baz will result in []string{"bar", "car". "baz"} +func SplitStringSlice(ss []string) []string { + result := []string{} + for _, s := range ss { + result = append(result, strings.Split(s, ",")...) + } + return result +} diff --git a/pkg/util/client_test.go b/pkg/util/client_test.go new file mode 100644 index 000000000000..7559fb76d976 --- /dev/null +++ b/pkg/util/client_test.go @@ -0,0 +1,39 @@ +package util + +import ( + "reflect" + "testing" + + "github.com/urfave/cli" +) + +func Test_UnitSplitSliceString(t *testing.T) { + tests := []struct { + name string + arg cli.StringSlice + want []string + }{ + { + name: "Single Argument", + arg: cli.StringSlice{"foo"}, + want: []string{"foo"}, + }, + { + name: "Repeated Arguments", + arg: cli.StringSlice{"foo", "bar", "baz"}, + want: []string{"foo", "bar", "baz"}, + }, + { + name: "Multiple Arguments and Repeated Arguments", + arg: cli.StringSlice{"foo,bar", "zoo,clar", "baz"}, + want: []string{"foo", "bar", "zoo", "clar", "baz"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := SplitStringSlice(tt.arg); !reflect.DeepEqual(got, tt.want) { + t.Errorf("SplitSliceString() = %+v\nWant = %+v", got, tt.want) + } + }) + } +}