kubeadm: adapt the validation and utils for structured ExtraArgs

Use []kubeadm.Arg instead of map[string]string when
validating ExtraArgs in the API.

Add new GetArgValue() and SetArgValue() utilities
and tests in apis/kubeadm.

Add new utils for constucting commands from and to
a []kubeadm.Arg slice.

cmd/kubeadm/app/apis/kubeadm/argument.go

@@ -0,0 +1,63 @@
+/*
+Copyright 2023 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package kubeadm
+
+// GetArgValue traverses an argument slice backwards and returns the value
+// of the given argument name and the index where it was found.
+// If the argument does not exist an empty string and -1 are returned.
+// startIdx defines where the iteration starts. If startIdx is a negative
+// value or larger than the size of the argument slice the iteration
+// will start from the last element.
+func GetArgValue(args []Arg, name string, startIdx int) (string, int) {
+	if startIdx < 0 || startIdx > len(args)-1 {
+		startIdx = len(args) - 1
+	}
+	for i := startIdx; i >= 0; i-- {
+		arg := args[i]
+		if arg.Name == name {
+			return arg.Value, i
+		}
+	}
+	return "", -1
+}
+
+// SetArgValues updates the value of one or more arguments or adds a new
+// one if missing. The function works backwards in the argument list.
+// nArgs holds how many existing arguments with this name should be set.
+// If nArgs is less than 1, all of them will be updated.
+func SetArgValues(args []Arg, name, value string, nArgs int) []Arg {
+	var count int
+	var found bool
+	for i := len(args) - 1; i >= 0; i-- {
+		if args[i].Name == name {
+			found = true
+			args[i].Value = value
+			if nArgs < 1 {
+				continue
+			}
+			count++
+			if count >= nArgs {
+				return args
+			}
+		}
+	}
+	if found {
+		return args
+	}
+	args = append(args, Arg{Name: name, Value: value})
+	return args
+}

cmd/kubeadm/app/apis/kubeadm/argument_test.go

@@ -0,0 +1,123 @@
+/*
+Copyright 2023 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package kubeadm
+
+import (
+	"reflect"
+	"testing"
+)
+
+func TestGetArgValue(t *testing.T) {
+	var tests = []struct {
+		testName      string
+		args          []Arg
+		name          string
+		expectedValue string
+		startIdx      int
+		expectedIdx   int
+	}{
+		{
+			testName:      "argument exists with non-empty value",
+			args:          []Arg{{Name: "a", Value: "a1"}, {Name: "b", Value: "b1"}, {Name: "c", Value: "c1"}},
+			name:          "b",
+			expectedValue: "b1",
+			expectedIdx:   1,
+			startIdx:      -1,
+		},
+		{
+			testName:      "argument exists with non-empty value (offset index)",
+			args:          []Arg{{Name: "a", Value: "a1"}, {Name: "b", Value: "b1"}, {Name: "c", Value: "c1"}},
+			name:          "a",
+			expectedValue: "a1",
+			expectedIdx:   0,
+			startIdx:      0,
+		},
+		{
+			testName:      "argument exists with empty value",
+			args:          []Arg{{Name: "foo1", Value: ""}, {Name: "foo2", Value: ""}},
+			name:          "foo2",
+			expectedValue: "",
+			expectedIdx:   1,
+			startIdx:      -1,
+		},
+		{
+			testName:      "argument does not exists",
+			args:          []Arg{{Name: "foo", Value: "bar"}},
+			name:          "z",
+			expectedValue: "",
+			expectedIdx:   -1,
+			startIdx:      -1,
+		},
+	}
+
+	for _, rt := range tests {
+		t.Run(rt.testName, func(t *testing.T) {
+			value, idx := GetArgValue(rt.args, rt.name, rt.startIdx)
+			if idx != rt.expectedIdx {
+				t.Errorf("expected index: %v, got: %v", rt.expectedIdx, idx)
+			}
+			if value != rt.expectedValue {
+				t.Errorf("expected value: %s, got: %s", rt.expectedValue, value)
+			}
+		})
+	}
+}
+
+func TestSetArgValues(t *testing.T) {
+	var tests = []struct {
+		testName     string
+		args         []Arg
+		name         string
+		value        string
+		nArgs        int
+		expectedArgs []Arg
+	}{
+		{
+			testName:     "update 1 argument",
+			args:         []Arg{{Name: "foo", Value: "bar1"}, {Name: "foo", Value: "bar2"}},
+			name:         "foo",
+			value:        "zz",
+			nArgs:        1,
+			expectedArgs: []Arg{{Name: "foo", Value: "bar1"}, {Name: "foo", Value: "zz"}},
+		},
+		{
+			testName:     "update all arguments",
+			args:         []Arg{{Name: "foo", Value: "bar1"}, {Name: "foo", Value: "bar2"}},
+			name:         "foo",
+			value:        "zz",
+			nArgs:        -1,
+			expectedArgs: []Arg{{Name: "foo", Value: "zz"}, {Name: "foo", Value: "zz"}},
+		},
+		{
+			testName:     "add new argument",
+			args:         []Arg{{Name: "foo", Value: "bar1"}, {Name: "foo", Value: "bar2"}},
+			name:         "z",
+			value:        "zz",
+			nArgs:        -1,
+			expectedArgs: []Arg{{Name: "foo", Value: "bar1"}, {Name: "foo", Value: "bar2"}, {Name: "z", Value: "zz"}},
+		},
+	}
+
+	for _, rt := range tests {
+		t.Run(rt.testName, func(t *testing.T) {
+			args := SetArgValues(rt.args, rt.name, rt.value, rt.nArgs)
+			if !reflect.DeepEqual(args, rt.expectedArgs) {
+				t.Errorf("expected args: %#v, got: %#v", rt.expectedArgs, args)
+			}
+		})
+	}
+}

cmd/kubeadm/app/apis/kubeadm/validation/validation.go

@@ -64,6 +64,8 @@ func ValidateClusterConfiguration(c *kubeadm.ClusterConfiguration) field.ErrorLi
 	allErrs = append(allErrs, ValidateDNS(&c.DNS, field.NewPath("dns"))...)
 	allErrs = append(allErrs, ValidateNetworking(c, field.NewPath("networking"))...)
 	allErrs = append(allErrs, ValidateAPIServer(&c.APIServer, field.NewPath("apiServer"))...)
+	allErrs = append(allErrs, ValidateControllerManager(&c.ControllerManager, field.NewPath("controllerManager"))...)
+	allErrs = append(allErrs, ValidateScheduler(&c.Scheduler, field.NewPath("scheduler"))...)
 	allErrs = append(allErrs, ValidateAbsolutePath(c.CertificatesDir, field.NewPath("certificatesDir"))...)
 	allErrs = append(allErrs, ValidateFeatureGates(c.FeatureGates, field.NewPath("featureGates"))...)
 	allErrs = append(allErrs, ValidateHostPort(c.ControlPlaneEndpoint, field.NewPath("controlPlaneEndpoint"))...)
@@ -77,6 +79,21 @@ func ValidateClusterConfiguration(c *kubeadm.ClusterConfiguration) field.ErrorLi
 func ValidateAPIServer(a *kubeadm.APIServer, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
 	allErrs = append(allErrs, ValidateCertSANs(a.CertSANs, fldPath.Child("certSANs"))...)
+	allErrs = append(allErrs, ValidateExtraArgs(a.ExtraArgs, fldPath.Child("extraArgs"))...)
+	return allErrs
+}
+
+// ValidateControllerManager validates the controller manager object and collects all encountered errors
+func ValidateControllerManager(a *kubeadm.ControlPlaneComponent, fldPath *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+	allErrs = append(allErrs, ValidateExtraArgs(a.ExtraArgs, fldPath.Child("extraArgs"))...)
+	return allErrs
+}
+
+// ValidateScheduler validates the scheduler object and collects all encountered errors
+func ValidateScheduler(a *kubeadm.ControlPlaneComponent, fldPath *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+	allErrs = append(allErrs, ValidateExtraArgs(a.ExtraArgs, fldPath.Child("extraArgs"))...)
 	return allErrs
 }
 
@@ -115,6 +132,7 @@ func ValidateNodeRegistrationOptions(nro *kubeadm.NodeRegistrationOptions, fldPa
 		}
 	}
 	allErrs = append(allErrs, ValidateSocketPath(nro.CRISocket, fldPath.Child("criSocket"))...)
+	allErrs = append(allErrs, ValidateExtraArgs(nro.KubeletExtraArgs, fldPath.Child("kubeletExtraArgs"))...)
 	// TODO: Maybe validate .Taints as well in the future using something like validateNodeTaints() in pkg/apis/core/validation
 	return allErrs
 }
@@ -287,6 +305,7 @@ func ValidateEtcd(e *kubeadm.Etcd, fldPath *field.Path) field.ErrorList {
 		if len(e.Local.ImageRepository) > 0 {
 			allErrs = append(allErrs, ValidateImageRepository(e.Local.ImageRepository, localPath.Child("imageRepository"))...)
 		}
+		allErrs = append(allErrs, ValidateExtraArgs(e.Local.ExtraArgs, localPath.Child("extraArgs"))...)
 	}
 	if e.External != nil {
 		requireHTTPS := true
@@ -478,9 +497,10 @@ func getClusterNodeMask(c *kubeadm.ClusterConfiguration, isIPv6 bool) (int, erro
 		maskArg = "node-cidr-mask-size-ipv4"
 	}
 
-	if v, ok := c.ControllerManager.ExtraArgs[maskArg]; ok && v != "" {
+	maskValue, _ := kubeadm.GetArgValue(c.ControllerManager.ExtraArgs, maskArg, -1)
+	if len(maskValue) != 0 {
 		// assume it is an integer, if not it will fail later
-		maskSize, err = strconv.Atoi(v)
+		maskSize, err = strconv.Atoi(maskValue)
 		if err != nil {
 			return 0, errors.Wrapf(err, "could not parse the value of the kube-controller-manager flag %s as an integer", maskArg)
 		}
@@ -518,7 +538,8 @@ func ValidateNetworking(c *kubeadm.ClusterConfiguration, fldPath *field.Path) fi
 	}
 	if len(c.Networking.PodSubnet) != 0 {
 		allErrs = append(allErrs, ValidateIPNetFromString(c.Networking.PodSubnet, constants.MinimumAddressesInPodSubnet, fldPath.Child("podSubnet"))...)
-		if c.ControllerManager.ExtraArgs["allocate-node-cidrs"] != "false" {
+		val, _ := kubeadm.GetArgValue(c.ControllerManager.ExtraArgs, "allocate-node-cidrs", -1)
+		if val != "false" {
 			// Pod subnet was already validated, we need to validate now against the node-mask
 			allErrs = append(allErrs, ValidatePodSubnetNodeMask(c.Networking.PodSubnet, c, fldPath.Child("podSubnet"))...)
 		}
@@ -656,3 +677,16 @@ func ValidateResetConfiguration(c *kubeadm.ResetConfiguration) field.ErrorList {
 	allErrs = append(allErrs, ValidateAbsolutePath(c.CertificatesDir, field.NewPath("certificatesDir"))...)
 	return allErrs
 }
+
+// ValidateExtraArgs validates a set of arguments and collects all encountered errors
+func ValidateExtraArgs(args []kubeadm.Arg, fldPath *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+
+	for idx, arg := range args {
+		if len(arg.Name) == 0 {
+			allErrs = append(allErrs, field.Invalid(fldPath, fmt.Sprintf("index %d", idx), "argument has no name"))
+		}
+	}
+
+	return allErrs
+}

cmd/kubeadm/app/apis/kubeadm/validation/validation_test.go

@@ -254,24 +254,24 @@ func TestValidatePodSubnetNodeMask(t *testing.T) {
 	var tests = []struct {
 		name        string
 		subnet      string
-		cmExtraArgs map[string]string
+		cmExtraArgs []kubeadmapi.Arg
 		expected    bool
 	}{
 		// dual-stack:
 		{"dual IPv4 only, but mask too small. Default node-mask", "10.0.0.16/29", nil, false},
-		{"dual IPv4 only, but mask too small. Configured node-mask", "10.0.0.16/24", map[string]string{"node-cidr-mask-size-ipv4": "23"}, false},
+		{"dual IPv4 only, but mask too small. Configured node-mask", "10.0.0.16/24", []kubeadmapi.Arg{{Name: "node-cidr-mask-size-ipv4", Value: "23"}}, false},
 		{"dual IPv6 only, but mask too small. Default node-mask", "2001:db8::1/112", nil, false},
-		{"dual IPv6 only, but mask too small. Configured node-mask", "2001:db8::1/64", map[string]string{"node-cidr-mask-size-ipv6": "24"}, false},
+		{"dual IPv6 only, but mask too small. Configured node-mask", "2001:db8::1/64", []kubeadmapi.Arg{{Name: "node-cidr-mask-size-ipv6", Value: "24"}}, false},
 		{"dual IPv6 only, but mask difference greater than 16. Default node-mask", "2001:db8::1/12", nil, false},
-		{"dual IPv6 only, but mask difference greater than 16. Configured node-mask", "2001:db8::1/64", map[string]string{"node-cidr-mask-size-ipv6": "120"}, false},
+		{"dual IPv6 only, but mask difference greater than 16. Configured node-mask", "2001:db8::1/64", []kubeadmapi.Arg{{Name: "node-cidr-mask-size-ipv6", Value: "120"}}, false},
 		{"dual IPv4 only CIDR", "10.0.0.16/12", nil, true},
 		{"dual IPv6 only CIDR", "2001:db8::/48", nil, true},
 		{"dual, but IPv4 mask too small. Default node-mask", "10.0.0.16/29,2001:db8::/48", nil, false},
-		{"dual, but IPv4 mask too small. Configured node-mask", "10.0.0.16/24,2001:db8::/48", map[string]string{"node-cidr-mask-size-ipv4": "23"}, false},
+		{"dual, but IPv4 mask too small. Configured node-mask", "10.0.0.16/24,2001:db8::/48", []kubeadmapi.Arg{{Name: "node-cidr-mask-size-ipv4", Value: "23"}}, false},
 		{"dual, but IPv6 mask too small. Default node-mask", "2001:db8::1/112,10.0.0.16/16", nil, false},
-		{"dual, but IPv6 mask too small. Configured node-mask", "10.0.0.16/16,2001:db8::1/64", map[string]string{"node-cidr-mask-size-ipv6": "24"}, false},
+		{"dual, but IPv6 mask too small. Configured node-mask", "10.0.0.16/16,2001:db8::1/64", []kubeadmapi.Arg{{Name: "node-cidr-mask-size-ipv6", Value: "24"}}, false},
 		{"dual, but mask difference greater than 16. Default node-mask", "2001:db8::1/12,10.0.0.16/16", nil, false},
-		{"dual, but mask difference greater than 16. Configured node-mask", "10.0.0.16/16,2001:db8::1/64", map[string]string{"node-cidr-mask-size-ipv6": "120"}, false},
+		{"dual, but mask difference greater than 16. Configured node-mask", "10.0.0.16/16,2001:db8::1/64", []kubeadmapi.Arg{{Name: "node-cidr-mask-size-ipv6", Value: "120"}}, false},
 		{"dual IPv4 IPv6", "2001:db8::/48,10.0.0.16/12", nil, true},
 		{"dual IPv6 IPv4", "2001:db8::/48,10.0.0.16/12", nil, true},
 	}
@@ -1169,7 +1169,10 @@ func TestGetClusterNodeMask(t *testing.T) {
 			name: "dual ipv4 custom mask",
 			cfg: &kubeadmapi.ClusterConfiguration{
 				ControllerManager: kubeadmapi.ControlPlaneComponent{
-					ExtraArgs: map[string]string{"node-cidr-mask-size": "21", "node-cidr-mask-size-ipv4": "23"},
+					ExtraArgs: []kubeadmapi.Arg{
+						{Name: "node-cidr-mask-size", Value: "21"},
+						{Name: "node-cidr-mask-size-ipv4", Value: "23"},
+					},
 				},
 			},
 			isIPv6:       false,
@@ -1185,7 +1188,9 @@ func TestGetClusterNodeMask(t *testing.T) {
 			name: "dual ipv6 custom mask",
 			cfg: &kubeadmapi.ClusterConfiguration{
 				ControllerManager: kubeadmapi.ControlPlaneComponent{
-					ExtraArgs: map[string]string{"node-cidr-mask-size-ipv6": "83"},
+					ExtraArgs: []kubeadmapi.Arg{
+						{Name: "node-cidr-mask-size-ipv6", Value: "83"},
+					},
 				},
 			},
 			isIPv6:       true,
@@ -1195,7 +1200,9 @@ func TestGetClusterNodeMask(t *testing.T) {
 			name: "dual ipv4 custom mask",
 			cfg: &kubeadmapi.ClusterConfiguration{
 				ControllerManager: kubeadmapi.ControlPlaneComponent{
-					ExtraArgs: map[string]string{"node-cidr-mask-size-ipv4": "23"},
+					ExtraArgs: []kubeadmapi.Arg{
+						{Name: "node-cidr-mask-size-ipv4", Value: "23"},
+					},
 				},
 			},
 			isIPv6:       false,
@@ -1205,7 +1212,9 @@ func TestGetClusterNodeMask(t *testing.T) {
 			name: "dual ipv4 wrong mask",
 			cfg: &kubeadmapi.ClusterConfiguration{
 				ControllerManager: kubeadmapi.ControlPlaneComponent{
-					ExtraArgs: map[string]string{"node-cidr-mask-size-ipv4": "aa"},
+					ExtraArgs: []kubeadmapi.Arg{
+						{Name: "node-cidr-mask-size-ipv4", Value: "aa"},
+					},
 				},
 			},
 			isIPv6:        false,
@@ -1215,7 +1224,9 @@ func TestGetClusterNodeMask(t *testing.T) {
 			name: "dual ipv6 default mask and legacy flag",
 			cfg: &kubeadmapi.ClusterConfiguration{
 				ControllerManager: kubeadmapi.ControlPlaneComponent{
-					ExtraArgs: map[string]string{"node-cidr-mask-size": "23"},
+					ExtraArgs: []kubeadmapi.Arg{
+						{Name: "node-cidr-mask-size", Value: "23"},
+					},
 				},
 			},
 			isIPv6:       true,
@@ -1225,7 +1236,10 @@ func TestGetClusterNodeMask(t *testing.T) {
 			name: "dual ipv6 custom mask and legacy flag",
 			cfg: &kubeadmapi.ClusterConfiguration{
 				ControllerManager: kubeadmapi.ControlPlaneComponent{
-					ExtraArgs: map[string]string{"node-cidr-mask-size": "23", "node-cidr-mask-size-ipv6": "83"},
+					ExtraArgs: []kubeadmapi.Arg{
+						{Name: "node-cidr-mask-size", Value: "23"},
+						{Name: "node-cidr-mask-size-ipv6", Value: "83"},
+					},
 				},
 			},
 			isIPv6:       true,
@@ -1235,7 +1249,10 @@ func TestGetClusterNodeMask(t *testing.T) {
 			name: "dual ipv6 custom mask and wrong flag",
 			cfg: &kubeadmapi.ClusterConfiguration{
 				ControllerManager: kubeadmapi.ControlPlaneComponent{
-					ExtraArgs: map[string]string{"node-cidr-mask-size": "23", "node-cidr-mask-size-ipv6": "a83"},
+					ExtraArgs: []kubeadmapi.Arg{
+						{Name: "node-cidr-mask-size", Value: "23"},
+						{Name: "node-cidr-mask-size-ipv6", Value: "a83"},
+					},
 				},
 			},
 			isIPv6:        true,
@@ -1354,3 +1371,34 @@ func TestValidateAbsolutePath(t *testing.T) {
 		}
 	}
 }
+
+func TestValidateExtraArgs(t *testing.T) {
+	var tests = []struct {
+		name           string
+		args           []kubeadmapi.Arg
+		expectedErrors int
+	}{
+		{
+			name:           "valid argument",
+			args:           []kubeadmapi.Arg{{Name: "foo", Value: "bar"}},
+			expectedErrors: 0,
+		},
+		{
+			name:           "invalid one argument",
+			args:           []kubeadmapi.Arg{{Name: "", Value: "bar"}},
+			expectedErrors: 1,
+		},
+		{
+			name:           "invalid two arguments",
+			args:           []kubeadmapi.Arg{{Name: "", Value: "foo"}, {Name: "", Value: "bar"}},
+			expectedErrors: 2,
+		},
+	}
+
+	for _, tc := range tests {
+		actual := ValidateExtraArgs(tc.args, nil)
+		if len(actual) != tc.expectedErrors {
+			t.Errorf("case %q:\n\t expected errors: %v\n\t got: %v\n\t errors: %v", tc.name, tc.expectedErrors, len(actual), actual)
+		}
+	}
+}

cmd/kubeadm/app/util/arguments.go

@@ -24,41 +24,54 @@ import (
 	"github.com/pkg/errors"
 
 	"k8s.io/klog/v2"
+	kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
 )
 
-// BuildArgumentListFromMap takes two string-string maps, one with the base arguments and one
+// ArgumentsToCommand takes two Arg slices, one with the base arguments and one
 // with optional override arguments. In the return list override arguments will precede base
-// arguments
+// arguments. If an argument is present in the overrides, it will cause
-func BuildArgumentListFromMap(baseArguments map[string]string, overrideArguments map[string]string) []string {
+// all instances of the same argument in the base list to be discarded, leaving
+// only the instances of this argument in the overrides to be applied.
+func ArgumentsToCommand(base []kubeadmapi.Arg, overrides []kubeadmapi.Arg) []string {
 	var command []string
-	var keys []string
+	// Copy the base arguments into a new slice.
+	args := make([]kubeadmapi.Arg, len(base))
+	copy(args, base)
 
-	argsMap := make(map[string]string)
+	// Go trough the override arguments and delete all instances of arguments with the same name
-
+	// in the base list of arguments.
-	for k, v := range baseArguments {
+	for i := 0; i < len(overrides); i++ {
-		argsMap[k] = v
+	repeat:
+		for j := 0; j < len(args); j++ {
+			if overrides[i].Name == args[j].Name {
+				// Remove this existing argument and search for another argument
+				// with the same name in base.
+				args = append(args[:j], args[j+1:]...)
+				goto repeat
+			}
+		}
 	}
 
-	for k, v := range overrideArguments {
+	// Concatenate the overrides and the base arguments and then sort them.
-		argsMap[k] = v
+	args = append(args, overrides...)
-	}
+	sort.Slice(args, func(i, j int) bool {
+		if args[i].Name == args[j].Name {
+			return args[i].Value < args[j].Value
+		}
+		return args[i].Name < args[j].Name
+	})
 
-	for k := range argsMap {
+	for _, arg := range args {
-		keys = append(keys, k)
+		command = append(command, fmt.Sprintf("--%s=%s", arg.Name, arg.Value))
-	}
-
-	sort.Strings(keys)
-	for _, k := range keys {
-		command = append(command, fmt.Sprintf("--%s=%s", k, argsMap[k]))
 	}
 
 	return command
 }
 
-// ParseArgumentListToMap parses a CLI argument list in the form "--foo=bar" to a string-string map
+// ArgumentsFromCommand parses a CLI command in the form "--foo=bar" to an Arg slice
-func ParseArgumentListToMap(arguments []string) map[string]string {
+func ArgumentsFromCommand(command []string) []kubeadmapi.Arg {
-	resultingMap := map[string]string{}
+	args := []kubeadmapi.Arg{}
-	for i, arg := range arguments {
+	for i, arg := range command {
 		key, val, err := parseArgument(arg)
 
 		// Ignore if the first argument doesn't satisfy the criteria, it's most often the binary name
@@ -70,24 +83,16 @@ func ParseArgumentListToMap(arguments []string) map[string]string {
 			continue
 		}
 
-		resultingMap[key] = val
+		args = append(args, kubeadmapi.Arg{Name: key, Value: val})
 	}
-	return resultingMap
-}
 
-// ReplaceArgument gets a command list; converts it to a map for easier modification, runs the provided function that
+	sort.Slice(args, func(i, j int) bool {
-// returns a new modified map, and then converts the map back to a command string slice
+		if args[i].Name == args[j].Name {
-func ReplaceArgument(command []string, argMutateFunc func(map[string]string) map[string]string) []string {
+			return args[i].Value < args[j].Value
-	argMap := ParseArgumentListToMap(command)
+		}
-
+		return args[i].Name < args[j].Name
-	// Save the first command (the executable) if we're sure it's not an argument (i.e. no --)
+	})
-	var newCommand []string
+	return args
-	if len(command) > 0 && !strings.HasPrefix(command[0], "--") {
-		newCommand = append(newCommand, command[0])
-	}
-	newArgMap := argMutateFunc(argMap)
-	newCommand = append(newCommand, BuildArgumentListFromMap(newArgMap, map[string]string{})...)
-	return newCommand
 }
 
 // parseArgument parses the argument "--foo=bar" to "foo" and "bar"

cmd/kubeadm/app/util/arguments_test.go

@@ -20,23 +20,25 @@ import (
 	"reflect"
 	"sort"
 	"testing"
+
+	kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
 )
 
-func TestBuildArgumentListFromMap(t *testing.T) {
+func TestArgumentsToCommand(t *testing.T) {
 	var tests = []struct {
 		name      string
-		base      map[string]string
+		base      []kubeadmapi.Arg
-		overrides map[string]string
+		overrides []kubeadmapi.Arg
 		expected  []string
 	}{
 		{
 			name: "override an argument from the base",
-			base: map[string]string{
+			base: []kubeadmapi.Arg{
-				"admission-control": "NamespaceLifecycle",
+				{Name: "admission-control", Value: "NamespaceLifecycle"},
-				"allow-privileged":  "true",
+				{Name: "allow-privileged", Value: "true"},
 			},
-			overrides: map[string]string{
+			overrides: []kubeadmapi.Arg{
-				"admission-control": "NamespaceLifecycle,LimitRanger",
+				{Name: "admission-control", Value: "NamespaceLifecycle,LimitRanger"},
 			},
 			expected: []string{
 				"--admission-control=NamespaceLifecycle,LimitRanger",
@@ -44,12 +46,43 @@ func TestBuildArgumentListFromMap(t *testing.T) {
 			},
 		},
 		{
-			name: "add an argument that is not in base",
+			name: "override an argument from the base and add duplicate",
-			base: map[string]string{
+			base: []kubeadmapi.Arg{
-				"allow-privileged": "true",
+				{Name: "token-auth-file", Value: "/token"},
+				{Name: "tls-sni-cert-key", Value: "/some/path/"},
 			},
-			overrides: map[string]string{
+			overrides: []kubeadmapi.Arg{
-				"admission-control": "NamespaceLifecycle,LimitRanger",
+				{Name: "tls-sni-cert-key", Value: "/some/new/path"},
+				{Name: "tls-sni-cert-key", Value: "/some/new/path/subpath"},
+			},
+			expected: []string{
+				"--tls-sni-cert-key=/some/new/path",
+				"--tls-sni-cert-key=/some/new/path/subpath",
+				"--token-auth-file=/token",
+			},
+		},
+		{
+			name: "override all duplicate arguments from base",
+			base: []kubeadmapi.Arg{
+				{Name: "token-auth-file", Value: "/token"},
+				{Name: "tls-sni-cert-key", Value: "foo"},
+				{Name: "tls-sni-cert-key", Value: "bar"},
+			},
+			overrides: []kubeadmapi.Arg{
+				{Name: "tls-sni-cert-key", Value: "/some/new/path"},
+			},
+			expected: []string{
+				"--tls-sni-cert-key=/some/new/path",
+				"--token-auth-file=/token",
+			},
+		},
+		{
+			name: "add an argument that is not in base",
+			base: []kubeadmapi.Arg{
+				{Name: "allow-privileged", Value: "true"},
+			},
+			overrides: []kubeadmapi.Arg{
+				{Name: "admission-control", Value: "NamespaceLifecycle,LimitRanger"},
 			},
 			expected: []string{
 				"--admission-control=NamespaceLifecycle,LimitRanger",
@@ -58,12 +91,12 @@ func TestBuildArgumentListFromMap(t *testing.T) {
 		},
 		{
 			name: "allow empty strings in base",
-			base: map[string]string{
+			base: []kubeadmapi.Arg{
-				"allow-privileged":                   "true",
+				{Name: "allow-privileged", Value: "true"},
-				"something-that-allows-empty-string": "",
+				{Name: "something-that-allows-empty-string", Value: ""},
 			},
-			overrides: map[string]string{
+			overrides: []kubeadmapi.Arg{
-				"admission-control": "NamespaceLifecycle,LimitRanger",
+				{Name: "admission-control", Value: "NamespaceLifecycle,LimitRanger"},
 			},
 			expected: []string{
 				"--admission-control=NamespaceLifecycle,LimitRanger",
@@ -73,13 +106,13 @@ func TestBuildArgumentListFromMap(t *testing.T) {
 		},
 		{
 			name: "allow empty strings in overrides",
-			base: map[string]string{
+			base: []kubeadmapi.Arg{
-				"allow-privileged":                   "true",
+				{Name: "allow-privileged", Value: "true"},
-				"something-that-allows-empty-string": "foo",
+				{Name: "something-that-allows-empty-string", Value: "foo"},
 			},
-			overrides: map[string]string{
+			overrides: []kubeadmapi.Arg{
-				"admission-control":                  "NamespaceLifecycle,LimitRanger",
+				{Name: "admission-control", Value: "NamespaceLifecycle,LimitRanger"},
-				"something-that-allows-empty-string": "",
+				{Name: "something-that-allows-empty-string", Value: ""},
 			},
 			expected: []string{
 				"--admission-control=NamespaceLifecycle,LimitRanger",
@@ -91,19 +124,19 @@ func TestBuildArgumentListFromMap(t *testing.T) {
 
 	for _, rt := range tests {
 		t.Run(rt.name, func(t *testing.T) {
-			actual := BuildArgumentListFromMap(rt.base, rt.overrides)
+			actual := ArgumentsToCommand(rt.base, rt.overrides)
 			if !reflect.DeepEqual(actual, rt.expected) {
-				t.Errorf("failed BuildArgumentListFromMap:\nexpected:\n%v\nsaw:\n%v", rt.expected, actual)
+				t.Errorf("failed ArgumentsToCommand:\nexpected:\n%v\nsaw:\n%v", rt.expected, actual)
 			}
 		})
 	}
 }
 
-func TestParseArgumentListToMap(t *testing.T) {
+func TestArgumentsFromCommand(t *testing.T) {
 	var tests = []struct {
-		name        string
+		name     string
-		args        []string
+		args     []string
-		expectedMap map[string]string
+		expected []kubeadmapi.Arg
 	}{
 		{
 			name: "normal case",
@@ -111,9 +144,9 @@ func TestParseArgumentListToMap(t *testing.T) {
 				"--admission-control=NamespaceLifecycle,LimitRanger",
 				"--allow-privileged=true",
 			},
-			expectedMap: map[string]string{
+			expected: []kubeadmapi.Arg{
-				"admission-control": "NamespaceLifecycle,LimitRanger",
+				{Name: "admission-control", Value: "NamespaceLifecycle,LimitRanger"},
-				"allow-privileged":  "true",
+				{Name: "allow-privileged", Value: "true"},
 			},
 		},
 		{
@@ -123,10 +156,10 @@ func TestParseArgumentListToMap(t *testing.T) {
 				"--allow-privileged=true",
 				"--feature-gates=EnableFoo=true,EnableBar=false",
 			},
-			expectedMap: map[string]string{
+			expected: []kubeadmapi.Arg{
-				"admission-control": "NamespaceLifecycle,LimitRanger",
+				{Name: "admission-control", Value: "NamespaceLifecycle,LimitRanger"},
-				"allow-privileged":  "true",
+				{Name: "allow-privileged", Value: "true"},
-				"feature-gates":     "EnableFoo=true,EnableBar=false",
+				{Name: "feature-gates", Value: "EnableFoo=true,EnableBar=false"},
 			},
 		},
 		{
@@ -137,75 +170,32 @@ func TestParseArgumentListToMap(t *testing.T) {
 				"--allow-privileged=true",
 				"--feature-gates=EnableFoo=true,EnableBar=false",
 			},
-			expectedMap: map[string]string{
+			expected: []kubeadmapi.Arg{
-				"admission-control": "NamespaceLifecycle,LimitRanger",
+				{Name: "admission-control", Value: "NamespaceLifecycle,LimitRanger"},
-				"allow-privileged":  "true",
+				{Name: "allow-privileged", Value: "true"},
-				"feature-gates":     "EnableFoo=true,EnableBar=false",
+				{Name: "feature-gates", Value: "EnableFoo=true,EnableBar=false"},
+			},
+		},
+		{
+			name: "allow duplicate args",
+			args: []string{
+				"--admission-control=NamespaceLifecycle,LimitRanger",
+				"--tls-sni-cert-key=/some/path",
+				"--tls-sni-cert-key=/some/path/subpath",
+			},
+			expected: []kubeadmapi.Arg{
+				{Name: "admission-control", Value: "NamespaceLifecycle,LimitRanger"},
+				{Name: "tls-sni-cert-key", Value: "/some/path"},
+				{Name: "tls-sni-cert-key", Value: "/some/path/subpath"},
 			},
 		},
 	}
 
 	for _, rt := range tests {
 		t.Run(rt.name, func(t *testing.T) {
-			actualMap := ParseArgumentListToMap(rt.args)
+			actual := ArgumentsFromCommand(rt.args)
-			if !reflect.DeepEqual(actualMap, rt.expectedMap) {
+			if !reflect.DeepEqual(actual, rt.expected) {
-				t.Errorf("failed ParseArgumentListToMap:\nexpected:\n%v\nsaw:\n%v", rt.expectedMap, actualMap)
+				t.Errorf("failed ArgumentsFromCommand:\nexpected:\n%v\nsaw:\n%v", rt.expected, actual)
-			}
-		})
-	}
-}
-
-func TestReplaceArgument(t *testing.T) {
-	var tests = []struct {
-		name         string
-		args         []string
-		mutateFunc   func(map[string]string) map[string]string
-		expectedArgs []string
-	}{
-		{
-			name: "normal case",
-			args: []string{
-				"kube-apiserver",
-				"--admission-control=NamespaceLifecycle,LimitRanger",
-				"--allow-privileged=true",
-			},
-			mutateFunc: func(argMap map[string]string) map[string]string {
-				argMap["admission-control"] = "NamespaceLifecycle,LimitRanger,ResourceQuota"
-				return argMap
-			},
-			expectedArgs: []string{
-				"kube-apiserver",
-				"--admission-control=NamespaceLifecycle,LimitRanger,ResourceQuota",
-				"--allow-privileged=true",
-			},
-		},
-		{
-			name: "another normal case",
-			args: []string{
-				"kube-apiserver",
-				"--admission-control=NamespaceLifecycle,LimitRanger",
-				"--allow-privileged=true",
-			},
-			mutateFunc: func(argMap map[string]string) map[string]string {
-				argMap["new-arg-here"] = "foo"
-				return argMap
-			},
-			expectedArgs: []string{
-				"kube-apiserver",
-				"--admission-control=NamespaceLifecycle,LimitRanger",
-				"--allow-privileged=true",
-				"--new-arg-here=foo",
-			},
-		},
-	}
-
-	for _, rt := range tests {
-		t.Run(rt.name, func(t *testing.T) {
-			actualArgs := ReplaceArgument(rt.args, rt.mutateFunc)
-			sort.Strings(actualArgs)
-			sort.Strings(rt.expectedArgs)
-			if !reflect.DeepEqual(actualArgs, rt.expectedArgs) {
-				t.Errorf("failed ReplaceArgument:\nexpected:\n%v\nsaw:\n%v", rt.expectedArgs, actualArgs)
 			}
 		})
 	}
@@ -236,7 +226,7 @@ func TestRoundtrip(t *testing.T) {
 	for _, rt := range tests {
 		t.Run(rt.name, func(t *testing.T) {
 			// These two methods should be each other's opposite functions, test that by chaining the methods and see if you get the same result back
-			actual := BuildArgumentListFromMap(ParseArgumentListToMap(rt.args), map[string]string{})
+			actual := ArgumentsToCommand(ArgumentsFromCommand(rt.args), []kubeadmapi.Arg{})
 			sort.Strings(actual)
 			sort.Strings(rt.args)