diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/cmd.go b/staging/src/k8s.io/kubectl/pkg/cmd/cmd.go index cc554dea1a6..7907ffe6c46 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/cmd.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/cmd.go @@ -142,17 +142,12 @@ func NewDefaultKubectlCommandWithArgs(o KubectlOptions) *cobra.Command { } } } else if err == nil { - if cmdutil.CmdPluginAsSubcommand.IsEnabled() { + if !cmdutil.CmdPluginAsSubcommand.IsDisabled() { // Command exists(e.g. kubectl create), but it is not certain that // subcommand also exists (e.g. kubectl create networkpolicy) - if IsSubcommandPluginAllowed(foundCmd.Name()) { - var subcommand string - for _, arg := range foundArgs { // first "non-flag" argument as subcommand - if !strings.HasPrefix(arg, "-") { - subcommand = arg - break - } - } + // we also have to eliminate kubectl create -f + if IsSubcommandPluginAllowed(foundCmd.Name()) && len(foundArgs) >= 1 && !strings.HasPrefix(foundArgs[0], "-") { + subcommand := foundArgs[0] builtinSubcmdExist := false for _, subcmd := range foundCmd.Commands() { if subcmd.Name() == subcommand { 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 c3f4a6d8be3..84653461c97 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/cmd_test.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/cmd_test.go @@ -29,8 +29,6 @@ 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" ) func TestNormalizationFuncGlobalExistence(t *testing.T) { @@ -129,47 +127,45 @@ func TestKubectlSubcommandShadowPlugin(t *testing.T) { } for _, test := range tests { - cmdtesting.WithAlphaEnvs([]cmdutil.FeatureGate{cmdutil.CmdPluginAsSubcommand}, t, func(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) - } + 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.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.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.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.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.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 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) - } - }) + if !cmp.Equal(pluginsHandler.withArgs, test.expectPluginArgs, cmpopts.EquateEmpty()) { + t.Fatalf("unexpected plugin execution args: expected %q, got %q", test.expectPluginArgs, pluginsHandler.withArgs) + } }) } } diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/util/helpers.go b/staging/src/k8s.io/kubectl/pkg/cmd/util/helpers.go index 5d92b10b8e8..1f16632d9d9 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/util/helpers.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/util/helpers.go @@ -429,10 +429,20 @@ const ( CmdPluginAsSubcommand FeatureGate = "KUBECTL_ENABLE_CMD_SHADOW" ) +// IsEnabled returns true iff environment variable is set to true. +// All other cases, it returns false. func (f FeatureGate) IsEnabled() bool { return os.Getenv(string(f)) == "true" } +// IsDisabled returns true iff environment variable is set to false. +// All other cases, it returns true. +// This function is used for the cases where feature is enabled by default, +// but it may be needed to provide a way to ability to disable this feature. +func (f FeatureGate) IsDisabled() bool { + return os.Getenv(string(f)) == "false" +} + func AddValidateFlags(cmd *cobra.Command) { cmd.Flags().String( "validate", diff --git a/test/cmd/plugins.sh b/test/cmd/plugins.sh index 33a238ecf09..ed876bc7ed5 100755 --- a/test/cmd/plugins.sh +++ b/test/cmd/plugins.sh @@ -57,6 +57,14 @@ run_plugins_tests() { kube::test::if_has_string "${output_message}" 'Client Version' kube::test::if_has_not_string "${output_message}" 'overshadows an existing plugin' + # attempt to run a plugin as a subcommand of kubectl create in the user's PATH + output_message=$(PATH=${TEMP_PATH}:"test/fixtures/pkg/kubectl/plugins/create" kubectl create foo) + kube::test::if_has_string "${output_message}" 'plugin foo as a subcommand of kubectl create command' + + # ensure that a kubectl create cronjob builtin command supersedes a plugin that overshadows it + output_message=$(PATH=${TEMP_PATH}:"test/fixtures/pkg/kubectl/plugins/create" kubectl create cronjob --help) + kube::test::if_has_not_string "${output_message}" 'plugin cronjob as a subcommand of kubectl create command' + rm -fr "${TEMP_PATH}" set +o nounset diff --git a/test/fixtures/pkg/kubectl/plugins/create/kubectl-create-cronjob b/test/fixtures/pkg/kubectl/plugins/create/kubectl-create-cronjob new file mode 100755 index 00000000000..197df985065 --- /dev/null +++ b/test/fixtures/pkg/kubectl/plugins/create/kubectl-create-cronjob @@ -0,0 +1,3 @@ +#!/bin/bash + +echo "I am plugin cronjob as a subcommand of kubectl create command" diff --git a/test/fixtures/pkg/kubectl/plugins/create/kubectl-create-foo b/test/fixtures/pkg/kubectl/plugins/create/kubectl-create-foo new file mode 100755 index 00000000000..e74209d0e7e --- /dev/null +++ b/test/fixtures/pkg/kubectl/plugins/create/kubectl-create-foo @@ -0,0 +1,3 @@ +#!/bin/bash + +echo "I am plugin foo as a subcommand of kubectl create command"