From 8b9cbe62025da49a31518870f2aea0ce9797d3ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Filip=20K=C5=99epinsk=C3=BD?= Date: Thu, 11 May 2023 20:12:38 +0200 Subject: [PATCH 1/2] fix false positive kubectl plugin unit tests - test.args should be passed instead of the os.Args of the test framework to prevent simple invocation of kubectl without args that could manifest in false positive test runs - plugin execution should have a different test path - tests should invoke functioning kubectl commands instead of the mock ones to ensure the correct subcommand is executed without a failure --- .../src/k8s.io/kubectl/pkg/cmd/cmd_test.go | 95 ++++++++++++++----- 1 file changed, 69 insertions(+), 26 deletions(-) diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/cmd_test.go b/staging/src/k8s.io/kubectl/pkg/cmd/cmd_test.go index 3a2bdeac8ee..f7d92f93a20 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/cmd_test.go +++ b/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" ) @@ -79,7 +80,7 @@ func TestKubectlSubcommandShadowPlugin(t *testing.T) { }, { 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}) - if err := root.Execute(); err != nil { - t.Fatalf("unexpected error: %v", err) + // 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.err != nil && pluginsHandler.err.Error() != test.expectError) || + (pluginsHandler.err == nil && len(test.expectError) > 0) { t.Fatalf("unexpected error: expected %q to occur, but got %q", test.expectError, pluginsHandler.err) } + if pluginsHandler.lookedup && !pluginsHandler.executed && len(test.expectError) == 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) } @@ -166,13 +184,13 @@ func TestKubectlCommandHandlesPlugins(t *testing.T) { }{ { 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}) - if err := root.Execute(); err != nil { - t.Fatalf("unexpected error: %v", err) + // 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.err != nil && pluginsHandler.err.Error() != test.expectError) || + (pluginsHandler.err == nil && len(test.expectError) > 0) { t.Fatalf("unexpected error: expected %q to occur, but got %q", test.expectError, pluginsHandler.err) } + if pluginsHandler.lookedup && !pluginsHandler.executed && len(test.expectError) == 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,18 +296,21 @@ func TestKubectlCommandHandlesPlugins(t *testing.T) { type testPluginHandler struct { pluginsDirectory string + validPrefixes []string + + // lookup results + lookedup bool + err 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 - filename = fmt.Sprintf("%s-%s", "kubectl", filename) + h.lookedup = true dir, err := os.Stat(h.pluginsDirectory) if err != nil { @@ -291,17 +329,22 @@ func (h *testPluginHandler) Lookup(filename string) (string, bool) { return "", false } - for _, p := range plugins { - if p.Name() == filename { - return fmt.Sprintf("%s/%s", h.pluginsDirectory, p.Name()), true + filenameWithSuportedPrefix := "" + for _, prefix := range h.validPrefixes { + for _, p := range plugins { + 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.err = 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 From bafae5c0764a222e336de1d7ce2645186bb455dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Filip=20K=C5=99epinsk=C3=BD?= Date: Mon, 15 May 2023 16:41:54 +0200 Subject: [PATCH 2/2] rename err to lookupErr --- .../src/k8s.io/kubectl/pkg/cmd/cmd_test.go | 54 +++++++++---------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/cmd_test.go b/staging/src/k8s.io/kubectl/pkg/cmd/cmd_test.go index f7d92f93a20..c3f4a6d8be3 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/cmd_test.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/cmd_test.go @@ -61,11 +61,11 @@ func TestNormalizationFuncGlobalExistence(t *testing.T) { func TestKubectlSubcommandShadowPlugin(t *testing.T) { tests := []struct { - name string - args []string - expectPlugin string - expectPluginArgs []string - expectError string + name string + args []string + expectPlugin string + expectPluginArgs []string + expectLookupError string }{ { name: "test that a plugin executable is found based on command args when builtin subcommand does not exist", @@ -74,9 +74,9 @@ func TestKubectlSubcommandShadowPlugin(t *testing.T) { expectPluginArgs: []string{"--bar", "--bar2", "--namespace", "test-namespace"}, }, { - 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\"", + 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"}, + expectLookupError: "unable to find a plugin executable \"kubectl-create-foo2\"", }, { name: "test that normal commands are able to be executed, when builtin subcommand exists", @@ -148,12 +148,12 @@ func TestKubectlSubcommandShadowPlugin(t *testing.T) { } } - if (pluginsHandler.err != nil && pluginsHandler.err.Error() != test.expectError) || - (pluginsHandler.err == nil && len(test.expectError) > 0) { - t.Fatalf("unexpected error: expected %q to occur, but got %q", test.expectError, pluginsHandler.err) + if (pluginsHandler.lookupErr != nil && pluginsHandler.lookupErr.Error() != test.expectLookupError) || + (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.expectError) == 0 { + 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") } @@ -176,11 +176,11 @@ func TestKubectlSubcommandShadowPlugin(t *testing.T) { func TestKubectlCommandHandlesPlugins(t *testing.T) { tests := []struct { - name string - args []string - expectPlugin string - expectPluginArgs []string - expectError string + name string + args []string + expectPlugin string + expectPluginArgs []string + expectLookupError string }{ { name: "test that normal commands are able to be executed, when no plugin overshadows them", @@ -269,12 +269,12 @@ func TestKubectlCommandHandlesPlugins(t *testing.T) { } } - if (pluginsHandler.err != nil && pluginsHandler.err.Error() != test.expectError) || - (pluginsHandler.err == nil && len(test.expectError) > 0) { - t.Fatalf("unexpected error: expected %q to occur, but got %q", test.expectError, pluginsHandler.err) + if (pluginsHandler.lookupErr != nil && pluginsHandler.lookupErr.Error() != test.expectLookupError) || + (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.expectError) == 0 { + 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") } @@ -299,8 +299,8 @@ type testPluginHandler struct { validPrefixes []string // lookup results - lookedup bool - err error + lookedup bool + lookupErr error // execution results executed bool @@ -314,18 +314,18 @@ func (h *testPluginHandler) Lookup(filename string) (string, bool) { 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 } @@ -339,7 +339,7 @@ func (h *testPluginHandler) Lookup(filename string) (string, bool) { } } - h.err = fmt.Errorf("unable to find a plugin executable %q", filenameWithSuportedPrefix) + h.lookupErr = fmt.Errorf("unable to find a plugin executable %q", filenameWithSuportedPrefix) return "", false }