[Release-1.26] CLI + Config Enhancement (#7403)

* Handle multiple arguments with StringSlice flags (#7380)
* Cleanup server setup with util function

Signed-off-by: Derek Nola <[email protected]>

* Enable FindString to search dotD config files (#7323)
* Address multiple arg cases

Signed-off-by: Derek Nola <[email protected]>

pkg/cli/server/server.go

@@ -116,7 +116,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
@@ -247,14 +247,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
@@ -273,14 +271,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
@@ -315,14 +311,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
@@ -349,12 +343,10 @@ func run(app *cli.Context, cfg *cmds.Server, leaderControllers server.CustomCont
 
 	serverConfig.ControlConfig.Skips = map[string]bool{}
 	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

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) {

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,20 +378,20 @@ 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",
 				DefaultConfig: "./testdata/data.yaml",
 			},
 			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) {

pkg/configfilearg/testdata/data.yaml

@@ -1,4 +1,5 @@
 foo-bar: baz
+alice: bob
 a-slice:
 - 1
 - "2"

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

@@ -11,4 +11,5 @@ c-slice:
 - two
 d-slice:
 - one
-- two
+- two
+f-string: beta

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)
+			}
+		})
+	}
+}

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
+}

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)
+			}
+		})
+	}
+}