[backport v2.10] Refactors keyValueArgs handling for improved efficiency (#928)

pkg/resources/provisioning.cattle.io/v1/cluster/mutator.go

@@ -68,26 +68,22 @@ type keyValueArg struct {
 
 type keyValueArgs []keyValueArg
 
-// newKeyValueArgs initializes and returns an empty keyValueArgs slice.
-func newKeyValueArgs() *keyValueArgs {
-	return &keyValueArgs{}
-}
-
-// parseFromRawArgs converts an interface representing a slice of "key=value" strings into a slice of keyValueArg.
-func (kv *keyValueArgs) parseFromRawArgs(input interface{}) {
+// parseFromRawArgs converts an interface representing a slice of "key=value" strings & returns a slice of keyValueArg.
+func parseFromRawArgs(input interface{}) (keyValueArgs, error) {
 	parsed := convert.ToInterfaceSlice(input)
 	if parsed == nil {
-		logrus.Errorf("failed to convert input into slice: invalid type: %v", input)
-		return
+		return nil, fmt.Errorf("failed to convert input into slice: invalid type: %T, expected interface{}", input)
 	}
+	args := keyValueArgs{}
 	for _, arg := range parsed {
 		key, val, found := strings.Cut(convert.ToString(arg), "=")
 		if !found {
 			logrus.Warnf("skipping argument [%s] which does not have right format", arg)
 			continue
 		}
-		kv.update(key, val)
+		args.update(key, val)
 	}
+	return args, nil
 }
 
 // update updates the value for the given key if it exists in the slice; otherwise it appends a new key-value pair.
@@ -261,11 +257,14 @@ func (m *ProvisioningClusterMutator) handlePSACT(request *admission.Request, clu
 			}
 			// drop relevant fields if they exist in the cluster
 			dropMachineSelectorFile(machineSelectorFileForPSA(secretName, mountPath, ""), cluster, true)
-			args := getKubeAPIServerArg(cluster)
-			newArgs := slices.DeleteFunc(*args, func(arg keyValueArg) bool {
+			args, err := getKubeAPIServerArgs(cluster)
+			if err != nil {
+				return nil, fmt.Errorf("[provisioning cluster mutator] failed to get the kube-apiserver arguments: %w", err)
+			}
+			newArgs := slices.DeleteFunc(args, func(arg keyValueArg) bool {
 				return arg.key == kubeAPIAdmissionConfigOption && arg.value == mountPath
 			})
-			setKubeAPIServerArg(newArgs, cluster)
+			setKubeAPIServerArgs(newArgs, cluster)
 		} else {
 			// Now, handle the case of PSACT being set when creating or updating the cluster
 			template, err := m.psact.Get(templateName)
@@ -293,9 +292,12 @@ func (m *ProvisioningClusterMutator) handlePSACT(request *admission.Request, clu
 			dropMachineSelectorFile(machineSelectorFileForPSA(secretName, mountPath, ""), cluster, true)
 			hash := sha256.Sum256(fileContent)
 			addMachineSelectorFile(machineSelectorFileForPSA(secretName, mountPath, base64.StdEncoding.EncodeToString(hash[:])), cluster)
-			args := getKubeAPIServerArg(cluster)
+			args, err := getKubeAPIServerArgs(cluster)
+			if err != nil {
+				return nil, fmt.Errorf("[provisioning cluster mutator] failed to get the kube-apiserver arguments: %w", err)
+			}
 			args.update(kubeAPIAdmissionConfigOption, mountPath)
-			setKubeAPIServerArg(*args, cluster)
+			setKubeAPIServerArgs(args, cluster)
 		}
 	}
 	return admission.ResponseAllowed(), nil
@@ -333,20 +335,24 @@ func (m *ProvisioningClusterMutator) ensureSecret(namespace, name string, data m
 	return nil
 }
 
-// getKubeAPIServerArg returns a slice of keyValueArg representing the parsed value of
+// getKubeAPIServerArgs returns a slice of keyValueArg representing the parsed value of
 // "kube-apiserver-arg" from the cluster's MachineGlobalConfig.
-// An empty slice is returned if "kube-apiserver-arg" is not set in the cluster.
-func getKubeAPIServerArg(cluster *v1.Cluster) *keyValueArgs {
-	args := newKeyValueArgs()
-	if rawArgs, exists := cluster.Spec.RKEConfig.MachineGlobalConfig.Data["kube-apiserver-arg"]; exists {
-		args.parseFromRawArgs(rawArgs)
+// An empty slice is returned if "kube-apiserver-arg" is not set or an error is encountered during parsing.
+func getKubeAPIServerArgs(cluster *v1.Cluster) (keyValueArgs, error) {
+	rawArgs, exists := cluster.Spec.RKEConfig.MachineGlobalConfig.Data["kube-apiserver-arg"]
+	if !exists {
+		return keyValueArgs{}, nil
+	}
+	args, err := parseFromRawArgs(rawArgs)
+	if err != nil {
+		return keyValueArgs{}, err
 	}
-	return args
+	return args, nil
 }
 
-// setKubeAPIServerArg uses the provided arg to overwrite the value of kube-apiserver-arg under the cluster's MachineGlobalConfig.
+// setKubeAPIServerArgs uses the provided arg to overwrite the value of kube-apiserver-arg under the cluster's MachineGlobalConfig.
 // If the provided arg is an empty map, setKubeAPIServerArg removes the existing kube-apiserver-arg from the cluster's MachineGlobalConfig.
-func setKubeAPIServerArg(args []keyValueArg, cluster *v1.Cluster) {
+func setKubeAPIServerArgs(args keyValueArgs, cluster *v1.Cluster) {
 	if len(args) == 0 {
 		delete(cluster.Spec.RKEConfig.MachineGlobalConfig.Data, "kube-apiserver-arg")
 		return

pkg/resources/provisioning.cattle.io/v1/cluster/mutator_test.go

@@ -2,6 +2,7 @@ package cluster
 
 import (
 	"encoding/json"
+	"errors"
 	"reflect"
 	"testing"
 
@@ -16,16 +17,19 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 )
 
-func Test_GetKubeAPIServerArg(t *testing.T) {
+func Test_GetKubeAPIServerArgs(t *testing.T) {
+	errInvalidType := errors.New("failed to convert input into slice: invalid type: []string, expected interface{}")
 	tests := []struct {
-		name     string
-		cluster  *v1.Cluster
-		expected *keyValueArgs
+		name        string
+		cluster     *v1.Cluster
+		expected    keyValueArgs
+		expectedErr error
 	}{
 		{
-			name:     "cluster without kube-apiserver-arg",
-			cluster:  clusterWithoutKubeAPIServerArg(),
-			expected: &keyValueArgs{},
+			name:        "cluster without kube-apiserver-arg",
+			cluster:     clusterWithoutKubeAPIServerArg(),
+			expected:    keyValueArgs{},
+			expectedErr: nil,
 		},
 		{
 			name: "cluster without MachineGlobalConfig",
@@ -34,45 +38,60 @@ func Test_GetKubeAPIServerArg(t *testing.T) {
 					RKEConfig: &v1.RKEConfig{},
 				},
 			},
-			expected: &keyValueArgs{},
+			expected:    keyValueArgs{},
+			expectedErr: nil,
 		},
 		{
 			name:    "cluster with kube-apiserver-arg",
 			cluster: clusterWithKubeAPIServerArg(),
-			expected: &keyValueArgs{
+			expected: keyValueArgs{
 				{key: "foo", value: "bar"},
 				{key: "foo2", value: "bar2"},
 			},
+			expectedErr: nil,
 		},
 		{
 			name:    "cluster with kube-apiserver-arg-2",
 			cluster: clusterWithKubeAPIServerArg2(),
-			expected: &keyValueArgs{
+			expected: keyValueArgs{
 				{key: "foo", value: "bar"},
 				{key: "foo2", value: "bar2"},
 				{key: "foo3", value: "bar3=baz3"},
 			},
+			expectedErr: nil,
 		},
 		{
 			name:    "cluster with duplicate keys in kube-apiserver-arg",
 			cluster: clusterWithKubeAPIServerArg3(),
-			expected: &keyValueArgs{
+			expected: keyValueArgs{
 				{key: "foo", value: "bar"},
 				{key: "foo2", value: "bar2=baz2"},
 			},
+			expectedErr: nil,
+		},
+		{
+			name:        "cluster with invalid data in kube-apiserver-arg",
+			cluster:     clusterWithInvalidKubeAPIServerArg(),
+			expected:    keyValueArgs{},
+			expectedErr: errInvalidType,
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got := getKubeAPIServerArg(tt.cluster)
-			if !reflect.DeepEqual(*tt.expected, *got) {
-				t.Errorf("got: [%v], expected: [%v]", *got, *tt.expected)
+			got, err := getKubeAPIServerArgs(tt.cluster)
+			if tt.expectedErr != nil {
+				if err == nil || err.Error() != tt.expectedErr.Error() {
+					t.Errorf("expected getKubeAPIServerArgs() error = %v, got %v", tt.expectedErr, err)
+				}
+			}
+			if !reflect.DeepEqual(tt.expected, got) {
+				t.Errorf("got: %v, expected: %v", got, tt.expected)
 			}
 		})
 	}
 }
 
-func Test_SetKubeAPIServerArg(t *testing.T) {
+func Test_SetKubeAPIServerArgs(t *testing.T) {
 	tests := []struct {
 		name     string
 		arg      keyValueArgs
@@ -143,13 +162,11 @@ func Test_SetKubeAPIServerArg(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got := newKeyValueArgs()
-			expected := newKeyValueArgs()
-			setKubeAPIServerArg(tt.arg, tt.cluster)
-			got.parseFromRawArgs(tt.cluster.Spec.RKEConfig.MachineGlobalConfig.Data["kube-apiserver-arg"])
-			expected.parseFromRawArgs(tt.expected.Spec.RKEConfig.MachineGlobalConfig.Data["kube-apiserver-arg"])
-			if !reflect.DeepEqual(*got, *expected) {
-				t.Errorf("got: %v, expected: %v", *got, *expected)
+			setKubeAPIServerArgs(tt.arg, tt.cluster)
+			got, _ := parseFromRawArgs(tt.cluster.Spec.RKEConfig.MachineGlobalConfig.Data["kube-apiserver-arg"])
+			expected, _ := parseFromRawArgs(tt.expected.Spec.RKEConfig.MachineGlobalConfig.Data["kube-apiserver-arg"])
+			if !reflect.DeepEqual(got, expected) {
+				t.Errorf("got: %v, expected: %v", got, expected)
 			}
 		})
 	}
@@ -468,6 +485,15 @@ func clusterWithKubeAPIServerArg3() *v1.Cluster {
 	return cluster
 }
 
+func clusterWithInvalidKubeAPIServerArg() *v1.Cluster {
+	cluster := clusterWithoutKubeAPIServerArg()
+	var arg []string
+	arg = append(arg, "foo=bar")
+	arg = append(arg, "foo2=bar2")
+	cluster.Spec.RKEConfig.MachineGlobalConfig.Data["kube-apiserver-arg"] = arg
+	return cluster
+}
+
 func machineSelectorFile3() rkev1.RKEProvisioningFiles {
 	file := machineSelectorFile1()
 	file.FileSources[0].Secret.Items[0].Hash = "expected-value-for-file-3"

pkg/resources/provisioning.cattle.io/v1/cluster/validator.go

@@ -458,7 +458,10 @@ func (p *provisioningAdmitter) validatePSACT(request *admission.Request, respons
 				return fmt.Errorf("[provisioning cluster validator] machineSelectorFile for PSA should not be in the cluster Spec")
 			}
 			// validate that the flags are not set
-			args := getKubeAPIServerArg(cluster)
+			args, err := getKubeAPIServerArgs(cluster)
+			if err != nil {
+				return fmt.Errorf("[provisioning cluster validator] failed to get the kube-apiserver arguments: %w", err)
+			}
 			if args.keyHasValue(kubeAPIAdmissionConfigOption, mountPath) {
 				return fmt.Errorf("[provisioning cluster validator] admission-control-config-file under kube-apiserver-arg should not be set to %s", mountPath)
 			}
@@ -501,7 +504,10 @@ func (p *provisioningAdmitter) validatePSACT(request *admission.Request, respons
 				return fmt.Errorf("[provisioning cluster validator] machineSelectorFile for PSA should be in the cluster Spec")
 			}
 			// validate that the flags are set
-			args := getKubeAPIServerArg(cluster)
+			args, err := getKubeAPIServerArgs(cluster)
+			if err != nil {
+				return fmt.Errorf("[provisioning cluster validator] failed to get the kube-apiserver arguments: %w", err)
+			}
 			if !args.keyHasValue(kubeAPIAdmissionConfigOption, mountPath) {
 				return fmt.Errorf("[provisioning cluster validator] admission-control-config-file under kube-apiserver-arg should be set to %s", mountPath)
 			}