Merge pull request #117952 from atiratree/fix-kubectl-plugin-tests

fix false positive kubectl plugin unit tests

staging/src/k8s.io/kubectl/pkg/cmd/cmd_test.go

@@ -28,6 +28,7 @@ import (
 
 	"k8s.io/cli-runtime/pkg/genericclioptions"
 	"k8s.io/cli-runtime/pkg/genericiooptions"
+	"k8s.io/kubectl/pkg/cmd/plugin"
 	cmdtesting "k8s.io/kubectl/pkg/cmd/testing"
 	cmdutil "k8s.io/kubectl/pkg/cmd/util"
 )
@@ -64,7 +65,7 @@ func TestKubectlSubcommandShadowPlugin(t *testing.T) {
 		args              []string
 		expectPlugin      string
 		expectPluginArgs  []string
-		expectError      string
+		expectLookupError string
 	}{
 		{
 			name:             "test that a plugin executable is found based on command args when builtin subcommand does not exist",
@@ -75,11 +76,11 @@ func TestKubectlSubcommandShadowPlugin(t *testing.T) {
 		{
 			name:              "test that a plugin executable is not found based on command args when also builtin subcommand does not exist",
 			args:              []string{"kubectl", "create", "foo2", "--bar", "--bar2", "--namespace", "test-namespace"},
-			expectError: "unable to find a plugin executable \"kubectl-create-foo2\"",
+			expectLookupError: "unable to find a plugin executable \"kubectl-create-foo2\"",
 		},
 		{
 			name:             "test that normal commands are able to be executed, when builtin subcommand exists",
-			args:             []string{"kubectl", "create", "role", "--bar", "--bar2", "--namespace", "test-namespace"},
+			args:             []string{"kubectl", "create", "job", "foo", "--image=busybox", "--dry-run=client", "--namespace", "test-namespace"},
 			expectPlugin:     "",
 			expectPluginArgs: []string{},
 		},
@@ -87,7 +88,7 @@ func TestKubectlSubcommandShadowPlugin(t *testing.T) {
 		// just to retest them also when feature is enabled.
 		{
 			name:             "test that normal commands are able to be executed, when no plugin overshadows them",
-			args:             []string{"kubectl", "get", "foo"},
+			args:             []string{"kubectl", "config", "get-clusters"},
 			expectPlugin:     "",
 			expectPluginArgs: []string{},
 		},
@@ -99,7 +100,7 @@ func TestKubectlSubcommandShadowPlugin(t *testing.T) {
 		},
 		{
 			name: "test that a plugin does not execute over an existing command by the same name",
-			args: []string{"kubectl", "version"},
+			args: []string{"kubectl", "version", "--client=true"},
 		},
 		{
 			name: "test that a plugin does not execute over Cobra's help command",
@@ -107,11 +108,11 @@ func TestKubectlSubcommandShadowPlugin(t *testing.T) {
 		},
 		{
 			name: "test that a plugin does not execute over Cobra's __complete command",
-			args: []string{"kubectl", cobra.ShellCompRequestCmd},
+			args: []string{"kubectl", cobra.ShellCompRequestCmd, "de"},
 		},
 		{
 			name: "test that a plugin does not execute over Cobra's __completeNoDesc command",
-			args: []string{"kubectl", cobra.ShellCompNoDescRequestCmd},
+			args: []string{"kubectl", cobra.ShellCompNoDescRequestCmd, "de"},
 		},
 		{
 			name: "test that a flag does not break Cobra's help command",
@@ -132,22 +133,39 @@ func TestKubectlSubcommandShadowPlugin(t *testing.T) {
 			t.Run(test.name, func(t *testing.T) {
 				pluginsHandler := &testPluginHandler{
 					pluginsDirectory: "plugin/testdata",
+					validPrefixes:    plugin.ValidPluginFilenamePrefixes,
 				}
 				ioStreams, _, _, _ := genericiooptions.NewTestIOStreams()
-
 				root := NewDefaultKubectlCommandWithArgs(KubectlOptions{PluginHandler: pluginsHandler, Arguments: test.args, IOStreams: ioStreams})
+				// original plugin handler (DefaultPluginHandler) is implemented by exec call so no additional actions are expected on the cobra command if we activate the plugin flow
+				if !pluginsHandler.lookedup && !pluginsHandler.executed {
+					// args must be set, otherwise Execute will use os.Args (args used for starting the test) and test.args would not be passed
+					// to the command which might invoke only "kubectl" without any additional args and give false positives
+					root.SetArgs(test.args[1:])
+					// Important note! Incorrect command or command failing validation might just call os.Exit(1) which would interrupt execution of the test
 					if err := root.Execute(); err != nil {
 						t.Fatalf("unexpected error: %v", err)
 					}
+				}
 
-				if pluginsHandler.err != nil && pluginsHandler.err.Error() != test.expectError {
+				if (pluginsHandler.lookupErr != nil && pluginsHandler.lookupErr.Error() != test.expectLookupError) ||
-					t.Fatalf("unexpected error: expected %q to occur, but got %q", test.expectError, pluginsHandler.err)
+					(pluginsHandler.lookupErr == nil && len(test.expectLookupError) > 0) {
+					t.Fatalf("unexpected error: expected %q to occur, but got %q", test.expectLookupError, pluginsHandler.lookupErr)
+				}
+
+				if pluginsHandler.lookedup && !pluginsHandler.executed && len(test.expectLookupError) == 0 {
+					// we have to fail here, because we have found the plugin, but not executed the plugin, nor the command (this would normally result in an error: unknown command)
+					t.Fatalf("expected plugin execution, but did not occur")
 				}
 
 				if pluginsHandler.executedPlugin != test.expectPlugin {
 					t.Fatalf("unexpected plugin execution: expected %q, got %q", test.expectPlugin, pluginsHandler.executedPlugin)
 				}
 
+				if pluginsHandler.executed && len(test.expectPlugin) == 0 {
+					t.Fatalf("unexpected plugin execution: expected no plugin, got %q", pluginsHandler.executedPlugin)
+				}
+
 				if !cmp.Equal(pluginsHandler.withArgs, test.expectPluginArgs, cmpopts.EquateEmpty()) {
 					t.Fatalf("unexpected plugin execution args: expected %q, got %q", test.expectPluginArgs, pluginsHandler.withArgs)
 				}
@@ -162,17 +180,17 @@ func TestKubectlCommandHandlesPlugins(t *testing.T) {
 		args              []string
 		expectPlugin      string
 		expectPluginArgs  []string
-		expectError      string
+		expectLookupError string
 	}{
 		{
 			name:             "test that normal commands are able to be executed, when no plugin overshadows them",
-			args:             []string{"kubectl", "get", "foo"},
+			args:             []string{"kubectl", "config", "get-clusters"},
 			expectPlugin:     "",
 			expectPluginArgs: []string{},
 		},
 		{
 			name:             "test that normal commands are able to be executed, when no plugin overshadows them and shadowing feature is not enabled",
-			args:             []string{"kubectl", "create", "foo"},
+			args:             []string{"kubectl", "create", "job", "foo", "--image=busybox", "--dry-run=client"},
 			expectPlugin:     "",
 			expectPluginArgs: []string{},
 		},
@@ -184,7 +202,7 @@ func TestKubectlCommandHandlesPlugins(t *testing.T) {
 		},
 		{
 			name: "test that a plugin does not execute over an existing command by the same name",
-			args: []string{"kubectl", "version"},
+			args: []string{"kubectl", "version", "--client=true"},
 		},
 		// The following tests make sure that commands added by Cobra cannot be shadowed by a plugin
 		// See https://github.com/kubernetes/kubectl/issues/1116
@@ -194,11 +212,11 @@ func TestKubectlCommandHandlesPlugins(t *testing.T) {
 		},
 		{
 			name: "test that a plugin does not execute over Cobra's __complete command",
-			args: []string{"kubectl", cobra.ShellCompRequestCmd},
+			args: []string{"kubectl", cobra.ShellCompRequestCmd, "de"},
 		},
 		{
 			name: "test that a plugin does not execute over Cobra's __completeNoDesc command",
-			args: []string{"kubectl", cobra.ShellCompNoDescRequestCmd},
+			args: []string{"kubectl", cobra.ShellCompNoDescRequestCmd, "de"},
 		},
 		// The following tests make sure that commands added by Cobra cannot be shadowed by a plugin
 		// even when a flag is specified first.  This can happen when using aliases.
@@ -236,22 +254,39 @@ func TestKubectlCommandHandlesPlugins(t *testing.T) {
 		t.Run(test.name, func(t *testing.T) {
 			pluginsHandler := &testPluginHandler{
 				pluginsDirectory: "plugin/testdata",
+				validPrefixes:    plugin.ValidPluginFilenamePrefixes,
 			}
 			ioStreams, _, _, _ := genericiooptions.NewTestIOStreams()
-
 			root := NewDefaultKubectlCommandWithArgs(KubectlOptions{PluginHandler: pluginsHandler, Arguments: test.args, IOStreams: ioStreams})
+			// original plugin handler (DefaultPluginHandler) is implemented by exec call so no additional actions are expected on the cobra command if we activate the plugin flow
+			if !pluginsHandler.lookedup && !pluginsHandler.executed {
+				// args must be set, otherwise Execute will use os.Args (args used for starting the test) and test.args would not be passed
+				// to the command which might invoke only "kubectl" without any additional args and give false positives
+				root.SetArgs(test.args[1:])
+				// Important note! Incorrect command or command failing validation might just call os.Exit(1) which would interrupt execution of the test
 				if err := root.Execute(); err != nil {
 					t.Fatalf("unexpected error: %v", err)
 				}
+			}
 
-			if pluginsHandler.err != nil && pluginsHandler.err.Error() != test.expectError {
+			if (pluginsHandler.lookupErr != nil && pluginsHandler.lookupErr.Error() != test.expectLookupError) ||
-				t.Fatalf("unexpected error: expected %q to occur, but got %q", test.expectError, pluginsHandler.err)
+				(pluginsHandler.lookupErr == nil && len(test.expectLookupError) > 0) {
+				t.Fatalf("unexpected error: expected %q to occur, but got %q", test.expectLookupError, pluginsHandler.lookupErr)
+			}
+
+			if pluginsHandler.lookedup && !pluginsHandler.executed && len(test.expectLookupError) == 0 {
+				// we have to fail here, because we have found the plugin, but not executed the plugin, nor the command (this would normally result in an error: unknown command)
+				t.Fatalf("expected plugin execution, but did not occur")
 			}
 
 			if pluginsHandler.executedPlugin != test.expectPlugin {
 				t.Fatalf("unexpected plugin execution: expected %q, got %q", test.expectPlugin, pluginsHandler.executedPlugin)
 			}
 
+			if pluginsHandler.executed && len(test.expectPlugin) == 0 {
+				t.Fatalf("unexpected plugin execution: expected no plugin, got %q", pluginsHandler.executedPlugin)
+			}
+
 			if !cmp.Equal(pluginsHandler.withArgs, test.expectPluginArgs, cmpopts.EquateEmpty()) {
 				t.Fatalf("unexpected plugin execution args: expected %q, got %q", test.expectPluginArgs, pluginsHandler.withArgs)
 			}
@@ -261,47 +296,55 @@ func TestKubectlCommandHandlesPlugins(t *testing.T) {
 
 type testPluginHandler struct {
 	pluginsDirectory string
+	validPrefixes    []string
+
+	// lookup results
+	lookedup  bool
+	lookupErr error
 
 	// execution results
+	executed       bool
 	executedPlugin string
 	withArgs       []string
 	withEnv        []string
-
-	err error
 }
 
 func (h *testPluginHandler) Lookup(filename string) (string, bool) {
-	// append supported plugin prefix to the filename
+	h.lookedup = true
-	filename = fmt.Sprintf("%s-%s", "kubectl", filename)
 
 	dir, err := os.Stat(h.pluginsDirectory)
 	if err != nil {
-		h.err = err
+		h.lookupErr = err
 		return "", false
 	}
 
 	if !dir.IsDir() {
-		h.err = fmt.Errorf("expected %q to be a directory", h.pluginsDirectory)
+		h.lookupErr = fmt.Errorf("expected %q to be a directory", h.pluginsDirectory)
 		return "", false
 	}
 
 	plugins, err := os.ReadDir(h.pluginsDirectory)
 	if err != nil {
-		h.err = err
+		h.lookupErr = err
 		return "", false
 	}
 
+	filenameWithSuportedPrefix := ""
+	for _, prefix := range h.validPrefixes {
 		for _, p := range plugins {
-		if p.Name() == filename {
+			filenameWithSuportedPrefix = fmt.Sprintf("%s-%s", prefix, filename)
+			if p.Name() == filenameWithSuportedPrefix {
 				return fmt.Sprintf("%s/%s", h.pluginsDirectory, p.Name()), true
 			}
 		}
+	}
 
-	h.err = fmt.Errorf("unable to find a plugin executable %q", filename)
+	h.lookupErr = fmt.Errorf("unable to find a plugin executable %q", filenameWithSuportedPrefix)
 	return "", false
 }
 
 func (h *testPluginHandler) Execute(executablePath string, cmdArgs, env []string) error {
+	h.executed = true
 	h.executedPlugin = executablePath
 	h.withArgs = cmdArgs
 	h.withEnv = env