From 7d79f0fd37c91f63b357bbee76debb900604e117 Mon Sep 17 00:00:00 2001 From: Artem Mikhalitsin Date: Mon, 3 Mar 2025 10:26:54 -0500 Subject: [PATCH] Fix arg parsing for plugins. Add missing global args. Signed-off-by: Artem Mikhalitsin --- pkg/cmd/load_plugins.go | 81 ++++++++++++++++++++++++++++++++++------- pkg/cmd/plugin_test.go | 38 +++++++++++++------ 2 files changed, 94 insertions(+), 25 deletions(-) diff --git a/pkg/cmd/load_plugins.go b/pkg/cmd/load_plugins.go index 3cf701242..40de2d901 100644 --- a/pkg/cmd/load_plugins.go +++ b/pkg/cmd/load_plugins.go @@ -64,7 +64,6 @@ func loadPlugins(baseCmd *cobra.Command, out io.Writer) { // Now we create commands for all of these. for _, plug := range found { - plug := plug md := plug.Metadata if md.Usage == "" { md.Usage = fmt.Sprintf("the %q plugin", md.Name) @@ -151,11 +150,36 @@ func callPluginExecutable(pluginName string, main string, argv []string, out io. // manuallyProcessArgs processes an arg array, removing special args. // // Returns two sets of args: known and unknown (in that order) +// Known args are flags that are consumed by the root `helm` command +// Unknown args are any args that will not be consumed by helm, and that may be +// processed by the plugin plus +// The single exception is -h or --help, which is passed to the plugin func manuallyProcessArgs(args []string) ([]string, []string) { known := []string{} unknown := []string{} - kvargs := []string{"--kube-context", "--namespace", "-n", "--kubeconfig", "--kube-apiserver", "--kube-token", "--kube-as-user", "--kube-as-group", "--kube-ca-file", "--registry-config", "--repository-cache", "--repository-config", "--kube-insecure-skip-tls-verify", "--kube-tls-server-name"} - knownArg := func(a string) bool { + boolArgs := []string{ + "--debug", + "--kube-insecure-skip-tls-verify", + } + kvargs := []string{ + "--burst-limit", + "--kube-apiserver", + "--kube-as-group", + "--kube-as-user", + "--kube-ca-file", + "--kube-context", + "--kube-tls-server-name", + "--kube-token", + "--kubeconfig", + "--namespace", "-n", + "--qps", + "--registry-config", + "--repository-cache", + "--repository-config"} + kvargs = append(kvargs, boolArgs...) // bool args can also by kv i.e. `--debug=true` + + // detects args with 'arg=val' syntax + isKnownKvWithEquals := func(a string) bool { for _, pre := range kvargs { if strings.HasPrefix(a, pre+"=") { return true @@ -163,31 +187,60 @@ func manuallyProcessArgs(args []string) ([]string, []string) { } return false } - - isKnown := func(v string) string { + // detects standalone boolean flags + isKnownBoolean := func(v string) bool { + for _, i := range boolArgs { + if i == v { + return true + } + } + return false + } + // detects boolean flags where the next arg can be parsed to boolean + isKnownBooleanWithNextValue := func(args []string, i int) bool { + if len(args)-i < 2 { // must be at least 2 args remaining + return false + } + a := args[i] + for _, known := range boolArgs { + if known == a { + possibleBool := args[i+1] // safe - checked that at least 2 args left in slice + _, err := strconv.ParseBool(possibleBool) + if err == nil { + return true + } + } + } + return false + } + isKnownKv := func(v string) bool { for _, i := range kvargs { if i == v { - return v + return true } } - return "" + return false } for i := 0; i < len(args); i++ { - switch a := args[i]; a { - case "--debug": + a := args[i] + if isKnownKvWithEquals(a) { // is it an argument using `arg=val` syntax? known = append(known, a) - case isKnown(a): + } else if isKnownBooleanWithNextValue(args, i) { // is it a boolean flag using `--boolFlag true` syntax? known = append(known, a) i++ if i < len(args) { known = append(known, args[i]) } - default: - if knownArg(a) { - known = append(known, a) - continue + } else if isKnownBoolean(a) { // is it a known boolean flag by itself? i.e. `--boolFlag` + known = append(known, a) + } else if isKnownKv(a) { + known = append(known, a) // is it a known kv flag? + i++ + if i < len(args) { + known = append(known, args[i]) } + } else { unknown = append(unknown, a) } } diff --git a/pkg/cmd/plugin_test.go b/pkg/cmd/plugin_test.go index 7c36698b1..309491258 100644 --- a/pkg/cmd/plugin_test.go +++ b/pkg/cmd/plugin_test.go @@ -25,6 +25,7 @@ import ( "github.com/spf13/cobra" "github.com/spf13/pflag" + "github.com/stretchr/testify/assert" release "helm.sh/helm/v4/pkg/release/v1" ) @@ -32,7 +33,16 @@ import ( func TestManuallyProcessArgs(t *testing.T) { input := []string{ "--debug", + "--debug=true", + "--debug", "true", + "--debug", "false", + "--kube-insecure-skip-tls-verify", + "--kube-insecure-skip-tls-verify=true", + "--kube-insecure-skip-tls-verify", "true", + "--kube-insecure-skip-tls-verify", "false", "--foo", "bar", + "--burst-limit", "123", + "--burst-limit=123", "--kubeconfig=/home/foo", "--kubeconfig", "/home/foo", "--kube-context=test1", @@ -44,12 +54,24 @@ func TestManuallyProcessArgs(t *testing.T) { "-n", "test2", "--namespace=test2", "--namespace", "test2", + "--qps", "22", + "--qps=22", "--home=/tmp", "command", + "--debug", // check a boolean flag at the end to test possible out of bounds exception } expectKnown := []string{ "--debug", + "--debug=true", + "--debug", "true", + "--debug", "false", + "--kube-insecure-skip-tls-verify", + "--kube-insecure-skip-tls-verify=true", + "--kube-insecure-skip-tls-verify", "true", + "--kube-insecure-skip-tls-verify", "false", + "--burst-limit", "123", + "--burst-limit=123", "--kubeconfig=/home/foo", "--kubeconfig", "/home/foo", "--kube-context=test1", @@ -61,6 +83,9 @@ func TestManuallyProcessArgs(t *testing.T) { "-n", "test2", "--namespace=test2", "--namespace", "test2", + "--qps", "22", + "--qps=22", + "--debug", } expectUnknown := []string{ @@ -69,17 +94,8 @@ func TestManuallyProcessArgs(t *testing.T) { known, unknown := manuallyProcessArgs(input) - for i, k := range known { - if k != expectKnown[i] { - t.Errorf("expected known flag %d to be %q, got %q", i, expectKnown[i], k) - } - } - for i, k := range unknown { - if k != expectUnknown[i] { - t.Errorf("expected unknown flag %d to be %q, got %q", i, expectUnknown[i], k) - } - } - + assert.Equal(t, expectKnown, known) + assert.Equal(t, expectUnknown, unknown) } func TestLoadPlugins(t *testing.T) {