Merge pull request #47267 from fabianofranz/kubectl_plugins_v1_part3

Automatic merge from submit-queue (batch tested with PRs 49619, 49598, 47267, 49597, 49638)

Flag support in kubectl plugins

Adds support to flags in `kubectl` plugins. Flags are declared in the plugin descriptor and are passed to plugins through env vars, similar to global flags (which already works).

Fixes https://github.com/kubernetes/kubernetes/issues/49122

**Release note**:

```release-note
Added flag support to kubectl plugins
```
PTAL @monopole @kubernetes/sig-cli-pr-reviews

hack/make-rules/test-cmd-util.sh

@@ -4069,13 +4069,20 @@ run_plugins_tests() {
   kube::test::if_has_not_string "${output_message}" 'The first child'
 
   # plugin env
-  output_message=$(KUBECTL_PLUGINS_PATH=test/fixtures/pkg/kubectl/plugins kubectl plugin env 2>&1)
+  output_message=$(KUBECTL_PLUGINS_PATH=test/fixtures/pkg/kubectl/plugins kubectl plugin env -h 2>&1)
+  kube::test::if_has_string "${output_message}" "This is a flag 1"
+  kube::test::if_has_string "${output_message}" "This is a flag 2"
+  kube::test::if_has_string "${output_message}" "This is a flag 3"
+  output_message=$(KUBECTL_PLUGINS_PATH=test/fixtures/pkg/kubectl/plugins kubectl plugin env --test1=value1 -t value2 2>&1)
   kube::test::if_has_string "${output_message}" 'KUBECTL_PLUGINS_CURRENT_NAMESPACE'
   kube::test::if_has_string "${output_message}" 'KUBECTL_PLUGINS_CALLER'
   kube::test::if_has_string "${output_message}" 'KUBECTL_PLUGINS_DESCRIPTOR_COMMAND=./env.sh'
   kube::test::if_has_string "${output_message}" 'KUBECTL_PLUGINS_DESCRIPTOR_SHORT_DESC=The plugin envs plugin'
   kube::test::if_has_string "${output_message}" 'KUBECTL_PLUGINS_GLOBAL_FLAG_KUBECONFIG'
   kube::test::if_has_string "${output_message}" 'KUBECTL_PLUGINS_GLOBAL_FLAG_REQUEST_TIMEOUT=0'
+  kube::test::if_has_string "${output_message}" 'KUBECTL_PLUGINS_LOCAL_FLAG_TEST1=value1'
+  kube::test::if_has_string "${output_message}" 'KUBECTL_PLUGINS_LOCAL_FLAG_TEST2=value2'
+  kube::test::if_has_string "${output_message}" 'KUBECTL_PLUGINS_LOCAL_FLAG_TEST3=default'
 
   set +o nounset
   set +o errexit

pkg/kubectl/cmd/plugin.go

@@ -114,6 +114,10 @@ func NewCmdForPlugin(f cmdutil.Factory, plugin *plugins.Plugin, runner plugins.P
 		},
 	}
 
+	for _, flag := range plugin.Flags {
+		cmd.Flags().StringP(flag.Name, flag.Shorthand, flag.DefValue, flag.Desc)
+	}
+
 	for _, childPlugin := range plugin.Tree {
 		cmd.AddCommand(NewCmdForPlugin(f, childPlugin, runner, in, out, errout))
 	}
@@ -126,10 +130,14 @@ type flagsPluginEnvProvider struct {
 }
 
 func (p *flagsPluginEnvProvider) Env() (plugins.EnvList, error) {
-	prefix := "KUBECTL_PLUGINS_GLOBAL_FLAG_"
+	globalPrefix := "KUBECTL_PLUGINS_GLOBAL_FLAG_"
 	env := plugins.EnvList{}
-	p.cmd.Flags().VisitAll(func(flag *pflag.Flag) {
+	p.cmd.InheritedFlags().VisitAll(func(flag *pflag.Flag) {
-		env = append(env, plugins.FlagToEnv(flag, prefix))
+		env = append(env, plugins.FlagToEnv(flag, globalPrefix))
+	})
+	localPrefix := "KUBECTL_PLUGINS_LOCAL_FLAG_"
+	p.cmd.LocalFlags().VisitAll(func(flag *pflag.Flag) {
+		env = append(env, plugins.FlagToEnv(flag, localPrefix))
 	})
 	return env, nil
 }

pkg/kubectl/plugins/loader_test.go

@@ -232,7 +232,10 @@ func setupValidPlugins(nPlugins, nChildren int) (string, error) {
 		descriptor := fmt.Sprintf(`
 name: %[1]s
 shortDesc: The %[1]s test plugin
-command: echo %[1]s`, name)
+command: echo %[1]s
+flags:
+  - name: %[1]s-flag
+    desc: A flag for %[1]s`, name)
 
 		if nChildren > 0 {
 			descriptor += `

pkg/kubectl/plugins/plugins.go

@@ -19,6 +19,15 @@ package plugins
 import (
 	"fmt"
 	"strings"
+	"unicode"
+)
+
+var (
+	IncompletePluginError     = fmt.Errorf("incomplete plugin descriptor: name, shortDesc and command fields are required")
+	InvalidPluginNameError    = fmt.Errorf("plugin name can't contain spaces")
+	IncompleteFlagError       = fmt.Errorf("incomplete flag descriptor: name and desc fields are required")
+	InvalidFlagNameError      = fmt.Errorf("flag name can't contain spaces")
+	InvalidFlagShorthandError = fmt.Errorf("flag shorthand must be only one letter")
 )
 
 // Plugin is the representation of a CLI extension (plugin).
@@ -31,12 +40,13 @@ type Plugin struct {
 // PluginDescription holds everything needed to register a
 // plugin as a command. Usually comes from a descriptor file.
 type Description struct {
-	Name      string    `json:"name"`
+	Name      string  `json:"name"`
-	ShortDesc string    `json:"shortDesc"`
+	ShortDesc string  `json:"shortDesc"`
-	LongDesc  string    `json:"longDesc,omitempty"`
+	LongDesc  string  `json:"longDesc,omitempty"`
-	Example   string    `json:"example,omitempty"`
+	Example   string  `json:"example,omitempty"`
-	Command   string    `json:"command"`
+	Command   string  `json:"command"`
-	Tree      []*Plugin `json:"tree,omitempty"`
+	Flags     []Flag  `json:"flags,omitempty"`
+	Tree      Plugins `json:"tree,omitempty"`
 }
 
 // PluginSource holds the location of a given plugin in the filesystem.
@@ -45,17 +55,17 @@ type Source struct {
 	DescriptorName string `json:"-"`
 }
 
-var (
-	IncompleteError  = fmt.Errorf("incomplete plugin descriptor: name, shortDesc and command fields are required")
-	InvalidNameError = fmt.Errorf("plugin name can't contain spaces")
-)
-
 func (p Plugin) Validate() error {
 	if len(p.Name) == 0 || len(p.ShortDesc) == 0 || (len(p.Command) == 0 && len(p.Tree) == 0) {
-		return IncompleteError
+		return IncompletePluginError
 	}
 	if strings.Index(p.Name, " ") > -1 {
-		return InvalidNameError
+		return InvalidPluginNameError
+	}
+	for _, flag := range p.Flags {
+		if err := flag.Validate(); err != nil {
+			return err
+		}
 	}
 	for _, child := range p.Tree {
 		if err := child.Validate(); err != nil {
@@ -71,3 +81,33 @@ func (p Plugin) IsValid() bool {
 
 // Plugins is a list of plugins.
 type Plugins []*Plugin
+
+// Flag describes a single flag supported by a given plugin.
+type Flag struct {
+	Name      string `json:"name"`
+	Shorthand string `json:"shorthand,omitempty"`
+	Desc      string `json:"desc"`
+	DefValue  string `json:"defValue,omitempty"`
+}
+
+func (f Flag) Validate() error {
+	if len(f.Name) == 0 || len(f.Desc) == 0 {
+		return IncompleteFlagError
+	}
+	if strings.Index(f.Name, " ") > -1 {
+		return InvalidFlagNameError
+	}
+	return f.ValidateShorthand()
+}
+
+func (f Flag) ValidateShorthand() error {
+	length := len(f.Shorthand)
+	if length == 0 || (length == 1 && unicode.IsLetter(rune(f.Shorthand[0]))) {
+		return nil
+	}
+	return InvalidFlagShorthandError
+}
+
+func (f Flag) Shorthanded() bool {
+	return f.ValidateShorthand() == nil
+}

pkg/kubectl/plugins/plugins_test.go

@@ -16,55 +16,154 @@ limitations under the License.
 
 package plugins
 
-import (
+import "testing"
-	"strings"
-	"testing"
-)
 
 func TestPlugin(t *testing.T) {
 	tests := []struct {
-		plugin        Plugin
+		plugin      *Plugin
-		expectedErr   string
+		expectedErr error
-		expectedValid bool
 	}{
 		{
-			plugin: Plugin{
+			plugin: &Plugin{
 				Description: Description{
 					Name:      "test",
 					ShortDesc: "The test",
 					Command:   "echo 1",
 				},
 			},
-			expectedValid: true,
 		},
 		{
-			plugin: Plugin{
+			plugin: &Plugin{
 				Description: Description{
 					Name:      "test",
 					ShortDesc: "The test",
 				},
 			},
-			expectedErr: "incomplete",
+			expectedErr: IncompletePluginError,
 		},
 		{
-			plugin:      Plugin{},
+			plugin:      &Plugin{},
-			expectedErr: "incomplete",
+			expectedErr: IncompletePluginError,
+		},
+		{
+			plugin: &Plugin{
+				Description: Description{
+					Name:      "test spaces",
+					ShortDesc: "The test",
+					Command:   "echo 1",
+				},
+			},
+			expectedErr: InvalidPluginNameError,
+		},
+		{
+			plugin: &Plugin{
+				Description: Description{
+					Name:      "test",
+					ShortDesc: "The test",
+					Command:   "echo 1",
+					Flags: []Flag{
+						{
+							Name: "aflag",
+						},
+					},
+				},
+			},
+			expectedErr: IncompleteFlagError,
+		},
+		{
+			plugin: &Plugin{
+				Description: Description{
+					Name:      "test",
+					ShortDesc: "The test",
+					Command:   "echo 1",
+					Flags: []Flag{
+						{
+							Name: "a flag",
+							Desc: "Invalid flag",
+						},
+					},
+				},
+			},
+			expectedErr: InvalidFlagNameError,
+		},
+		{
+			plugin: &Plugin{
+				Description: Description{
+					Name:      "test",
+					ShortDesc: "The test",
+					Command:   "echo 1",
+					Flags: []Flag{
+						{
+							Name:      "aflag",
+							Desc:      "Invalid shorthand",
+							Shorthand: "aa",
+						},
+					},
+				},
+			},
+			expectedErr: InvalidFlagShorthandError,
+		},
+		{
+			plugin: &Plugin{
+				Description: Description{
+					Name:      "test",
+					ShortDesc: "The test",
+					Command:   "echo 1",
+					Flags: []Flag{
+						{
+							Name:      "aflag",
+							Desc:      "Invalid shorthand",
+							Shorthand: "2",
+						},
+					},
+				},
+			},
+			expectedErr: InvalidFlagShorthandError,
+		},
+		{
+			plugin: &Plugin{
+				Description: Description{
+					Name:      "test",
+					ShortDesc: "The test",
+					Command:   "echo 1",
+					Flags: []Flag{
+						{
+							Name:      "aflag",
+							Desc:      "A flag",
+							Shorthand: "a",
+						},
+					},
+					Tree: Plugins{
+						&Plugin{
+							Description: Description{
+								Name:      "child",
+								ShortDesc: "The child",
+								LongDesc:  "The child long desc",
+								Example:   "You can use it like this but you're not supposed to",
+								Command:   "echo 1",
+								Flags: []Flag{
+									{
+										Name: "childflag",
+										Desc: "A child flag",
+									},
+									{
+										Name:      "childshorthand",
+										Desc:      "A child shorthand flag",
+										Shorthand: "s",
+									},
+								},
+							},
+						},
+					},
+				},
+			},
 		},
 	}
 
 	for _, test := range tests {
-		if is := test.plugin.IsValid(); test.expectedValid != is {
-			t.Errorf("%s: expected valid=%v, got %v", test.plugin.Name, test.expectedValid, is)
-		}
 		err := test.plugin.Validate()
-		if len(test.expectedErr) > 0 {
+		if err != test.expectedErr {
-			if err == nil {
+			t.Errorf("%s: expected error %v, got %v", test.plugin.Name, test.expectedErr, err)
-				t.Errorf("%s: expected error, got none", test.plugin.Name)
-			} else if !strings.Contains(err.Error(), test.expectedErr) {
-				t.Errorf("%s: expected error containing %q, got %v", test.plugin.Name, test.expectedErr, err)
-			}
-		} else if err != nil {
-			t.Errorf("%s: expected no error, got %v", test.plugin.Name, err)
 		}
 	}
 }

test/fixtures/pkg/kubectl/plugins/env/plugin.yaml

@@ -1,3 +1,12 @@
 name: env
 shortDesc: "The plugin envs plugin"
 command: "./env.sh"
+flags: 
+  - name: "test1"
+    desc: "This is a flag 1"
+  - name: "test2"
+    desc: "This is a flag 2"
+    shorthand: "t"
+  - name: "test3"
+    desc: "This is a flag 3"
+    defValue: "default"