From dafc60e79b1a8ace4f602ee723ccd450ce362e59 Mon Sep 17 00:00:00 2001 From: Apoorva Jagtap <35304110+apoorvajagtap@users.noreply.github.com> Date: Wed, 1 Oct 2025 00:13:48 +0530 Subject: [PATCH] Remove keyValueArgs dependency and preserve user-defined kube-apiserver-arg (#1096) --- .../v1/cluster/mutator.go | 86 ++++++------------- .../v1/cluster/mutator_test.go | 82 ++++++++++-------- .../v1/cluster/validator.go | 4 +- 3 files changed, 73 insertions(+), 99 deletions(-) diff --git a/pkg/resources/provisioning.cattle.io/v1/cluster/mutator.go b/pkg/resources/provisioning.cattle.io/v1/cluster/mutator.go index 21796c4c7..92b3f428d 100644 --- a/pkg/resources/provisioning.cattle.io/v1/cluster/mutator.go +++ b/pkg/resources/provisioning.cattle.io/v1/cluster/mutator.go @@ -60,48 +60,10 @@ var gvr = schema.GroupVersionResource{ Resource: "clusters", } -// keyValueArg represents a key-value pair configuration argument. -type keyValueArg struct { - key string - value string -} - -type keyValueArgs []keyValueArg - -// 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 { - 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 - } - 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. -func (kv *keyValueArgs) update(key, val string) { - idx := slices.IndexFunc(*kv, func(arg keyValueArg) bool { - return arg.key == key - }) - if idx != -1 { - (*kv)[idx].value = val - } else { - *kv = append(*kv, keyValueArg{key: key, value: val}) - } -} - -// keyHasValue returns true if the given key-value pair exists in the slice of keyValueArg. -func (kv *keyValueArgs) keyHasValue(key, val string) bool { - for _, arg := range *kv { - if arg.key == key && arg.value == val { +// keyHasValue returns true if args list contains the exact "key=value" pair. +func keyHasValue(args []string, key, value string) bool { + for _, pair := range args { + if pair == fmt.Sprintf("%s=%s", key, value) { return true } } @@ -238,6 +200,7 @@ func (m *ProvisioningClusterMutator) handlePSACT(request *admission.Request, clu secretName := fmt.Sprintf(secretName, cluster.Name) mountPath := fmt.Sprintf(mountPath, getRuntime(cluster.Spec.KubernetesVersion)) + admissionConfigArg := fmt.Sprintf("%s=%s", kubeAPIAdmissionConfigOption, mountPath) templateName := cluster.Spec.DefaultPodSecurityAdmissionConfigurationTemplateName switch request.Operation { @@ -261,10 +224,12 @@ func (m *ProvisioningClusterMutator) handlePSACT(request *admission.Request, clu 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 - }) - setKubeAPIServerArgs(newArgs, cluster) + if slices.Contains(args, admissionConfigArg) { + args = slices.DeleteFunc(args, func(arg string) bool { + return arg == admissionConfigArg + }) + } + setKubeAPIServerArgs(args, cluster) } else { // Now, handle the case of PSACT being set when creating or updating the cluster template, err := m.psact.Get(templateName) @@ -296,7 +261,9 @@ func (m *ProvisioningClusterMutator) handlePSACT(request *admission.Request, clu if err != nil { return nil, fmt.Errorf("[provisioning cluster mutator] failed to get the kube-apiserver arguments: %w", err) } - args.update(kubeAPIAdmissionConfigOption, mountPath) + if !slices.Contains(args, admissionConfigArg) { + args = append([]string{admissionConfigArg}, args...) + } setKubeAPIServerArgs(args, cluster) } } @@ -335,36 +302,31 @@ func (m *ProvisioningClusterMutator) ensureSecret(namespace, name string, data m return nil } -// getKubeAPIServerArgs returns a slice of keyValueArg representing the parsed value of -// "kube-apiserver-arg" from the cluster's MachineGlobalConfig. +// getKubeAPIServerArgs returns []string 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 or an error is encountered during parsing. -func getKubeAPIServerArgs(cluster *v1.Cluster) (keyValueArgs, error) { +func getKubeAPIServerArgs(cluster *v1.Cluster) ([]string, error) { rawArgs, exists := cluster.Spec.RKEConfig.MachineGlobalConfig.Data["kube-apiserver-arg"] if !exists { - return keyValueArgs{}, nil + return []string{}, nil } - args, err := parseFromRawArgs(rawArgs) - if err != nil { - return keyValueArgs{}, err + args := convert.ToStringSlice(rawArgs) + if args == nil { + return []string{}, fmt.Errorf("failed to convert input into slice: invalid type: %T, expected interface{}", rawArgs) } return args, nil } // 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 setKubeAPIServerArgs(args keyValueArgs, cluster *v1.Cluster) { +// If the provided arg is an empty slice, setKubeAPIServerArg removes the existing kube-apiserver-arg from the cluster's MachineGlobalConfig. +func setKubeAPIServerArgs(args []string, cluster *v1.Cluster) { if len(args) == 0 { delete(cluster.Spec.RKEConfig.MachineGlobalConfig.Data, "kube-apiserver-arg") return } - parsed := make([]any, len(args)) - for i, arg := range args { - parsed[i] = arg.key + "=" + arg.value - } if cluster.Spec.RKEConfig.MachineGlobalConfig.Data == nil { - cluster.Spec.RKEConfig.MachineGlobalConfig.Data = make(map[string]interface{}) + cluster.Spec.RKEConfig.MachineGlobalConfig.Data = make(map[string]any) } - cluster.Spec.RKEConfig.MachineGlobalConfig.Data["kube-apiserver-arg"] = parsed + cluster.Spec.RKEConfig.MachineGlobalConfig.Data["kube-apiserver-arg"] = args } // machineSelectorFileForPSA generates an RKEProvisioningFiles that mounts the secret which contains diff --git a/pkg/resources/provisioning.cattle.io/v1/cluster/mutator_test.go b/pkg/resources/provisioning.cattle.io/v1/cluster/mutator_test.go index 1ad64a152..2481bf78a 100644 --- a/pkg/resources/provisioning.cattle.io/v1/cluster/mutator_test.go +++ b/pkg/resources/provisioning.cattle.io/v1/cluster/mutator_test.go @@ -2,7 +2,7 @@ package cluster import ( "encoding/json" - "errors" + "fmt" "reflect" "testing" @@ -18,17 +18,16 @@ import ( ) 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 + expected []string expectedErr error }{ { name: "cluster without kube-apiserver-arg", cluster: clusterWithoutKubeAPIServerArg(), - expected: keyValueArgs{}, + expected: []string{}, expectedErr: nil, }, { @@ -38,42 +37,49 @@ func Test_GetKubeAPIServerArgs(t *testing.T) { RKEConfig: &v1.RKEConfig{}, }, }, - expected: keyValueArgs{}, + expected: []string{}, expectedErr: nil, }, { name: "cluster with kube-apiserver-arg", cluster: clusterWithKubeAPIServerArg(), - expected: keyValueArgs{ - {key: "foo", value: "bar"}, - {key: "foo2", value: "bar2"}, + expected: []string{ + "foo=bar", + "foo2=bar2", }, expectedErr: nil, }, { name: "cluster with kube-apiserver-arg-2", cluster: clusterWithKubeAPIServerArg2(), - expected: keyValueArgs{ - {key: "foo", value: "bar"}, - {key: "foo2", value: "bar2"}, - {key: "foo3", value: "bar3=baz3"}, + expected: []string{ + "foo=bar", + "foo2=bar2", + "foo3=bar3=baz3", }, expectedErr: nil, }, { name: "cluster with duplicate keys in kube-apiserver-arg", cluster: clusterWithKubeAPIServerArg3(), - expected: keyValueArgs{ - {key: "foo", value: "bar"}, - {key: "foo2", value: "bar2=baz2"}, + expected: []string{ + "foo=bar", + "foo2=bar2", + "foo2=bar2=baz2", }, expectedErr: nil, }, { - name: "cluster with invalid data in kube-apiserver-arg", - cluster: clusterWithInvalidKubeAPIServerArg(), - expected: keyValueArgs{}, - expectedErr: errInvalidType, + name: "cluster with bool flag data in kube-apiserver-arg", + cluster: clusterWithBoolFlagKubeAPIServerArg(), + expected: []string{"foo", "foo2=bar2"}, + expectedErr: nil, + }, + { + name: "cluster with invalid data type in kube-apiserver-arg", + cluster: clusterWithInvalidKubeAPIServerArgType(), + expected: []string{}, + expectedErr: fmt.Errorf("failed to convert input into slice: invalid type: int, expected interface{}"), }, } for _, tt := range tests { @@ -85,7 +91,7 @@ func Test_GetKubeAPIServerArgs(t *testing.T) { } } if !reflect.DeepEqual(tt.expected, got) { - t.Errorf("got: %v, expected: %v", got, tt.expected) + t.Errorf("expected: %v, got: %v", tt.expected, got) } }) } @@ -94,15 +100,15 @@ func Test_GetKubeAPIServerArgs(t *testing.T) { func Test_SetKubeAPIServerArgs(t *testing.T) { tests := []struct { name string - arg keyValueArgs + args []string cluster *v1.Cluster expected *v1.Cluster }{ { name: "cluster that already has kube-apiserver-arg", - arg: keyValueArgs{ - {key: "foo", value: "bar"}, - {key: "foo2", value: "bar2"}, + args: []string{ + "foo=bar", + "foo2=bar2", }, cluster: &v1.Cluster{ Spec: v1.ClusterSpec{ @@ -123,9 +129,9 @@ func Test_SetKubeAPIServerArgs(t *testing.T) { }, { name: "cluster that does not have MachineGlobalConfig", - arg: keyValueArgs{ - {key: "foo", value: "bar"}, - {key: "foo2", value: "bar2"}, + args: []string{ + "foo=bar", + "foo2=bar2", }, cluster: &v1.Cluster{ Spec: v1.ClusterSpec{ @@ -136,9 +142,9 @@ func Test_SetKubeAPIServerArgs(t *testing.T) { }, { name: "cluster does not have kube-apiserver-arg but other args", - arg: keyValueArgs{ - {key: "foo", value: "bar"}, - {key: "foo2", value: "bar2"}, + args: []string{ + "foo=bar", + "foo2=bar2", }, cluster: &v1.Cluster{ Spec: v1.ClusterSpec{ @@ -162,9 +168,9 @@ func Test_SetKubeAPIServerArgs(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - 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"]) + setKubeAPIServerArgs(tt.args, tt.cluster) + got, _ := getKubeAPIServerArgs(tt.cluster) + expected, _ := getKubeAPIServerArgs(tt.expected) if !reflect.DeepEqual(got, expected) { t.Errorf("got: %v, expected: %v", got, expected) } @@ -485,15 +491,21 @@ func clusterWithKubeAPIServerArg3() *v1.Cluster { return cluster } -func clusterWithInvalidKubeAPIServerArg() *v1.Cluster { +func clusterWithBoolFlagKubeAPIServerArg() *v1.Cluster { cluster := clusterWithoutKubeAPIServerArg() var arg []string - arg = append(arg, "foo=bar") + arg = append(arg, "foo") arg = append(arg, "foo2=bar2") cluster.Spec.RKEConfig.MachineGlobalConfig.Data["kube-apiserver-arg"] = arg return cluster } +func clusterWithInvalidKubeAPIServerArgType() *v1.Cluster { + cluster := clusterWithoutKubeAPIServerArg() + cluster.Spec.RKEConfig.MachineGlobalConfig.Data["kube-apiserver-arg"] = 123 + return cluster +} + func machineSelectorFile3() rkev1.RKEProvisioningFiles { file := machineSelectorFile1() file.FileSources[0].Secret.Items[0].Hash = "expected-value-for-file-3" diff --git a/pkg/resources/provisioning.cattle.io/v1/cluster/validator.go b/pkg/resources/provisioning.cattle.io/v1/cluster/validator.go index 06991e04e..1dd039772 100644 --- a/pkg/resources/provisioning.cattle.io/v1/cluster/validator.go +++ b/pkg/resources/provisioning.cattle.io/v1/cluster/validator.go @@ -480,7 +480,7 @@ func (p *provisioningAdmitter) validatePSACT(request *admission.Request, respons if err != nil { return fmt.Errorf("[provisioning cluster validator] failed to get the kube-apiserver arguments: %w", err) } - if args.keyHasValue(kubeAPIAdmissionConfigOption, mountPath) { + if keyHasValue(args, kubeAPIAdmissionConfigOption, mountPath) { return fmt.Errorf("[provisioning cluster validator] admission-control-config-file under kube-apiserver-arg should not be set to %s", mountPath) } } else { @@ -526,7 +526,7 @@ func (p *provisioningAdmitter) validatePSACT(request *admission.Request, respons if err != nil { return fmt.Errorf("[provisioning cluster validator] failed to get the kube-apiserver arguments: %w", err) } - if !args.keyHasValue(kubeAPIAdmissionConfigOption, mountPath) { + if !keyHasValue(args, kubeAPIAdmissionConfigOption, mountPath) { return fmt.Errorf("[provisioning cluster validator] admission-control-config-file under kube-apiserver-arg should be set to %s", mountPath) } }