Fix prefixed global flags for kubectl plugins #94290

cmurphy · 2020-08-27T21:17:22Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

For normal kubectl commands, global flags can be used anywhere within
the command line:

  $ kubectl --kubeconfig admin.conf get pod
  No resources found in default namespace.
  $ kubectl get --kubeconfig admin.conf pod
  No resources found in default namespace.
  $ kubectl get pod --kubeconfig admin.conf
  No resources found in default namespace.

However, without this patch, when calling a plugin as a subcommand, the
flag can only work if it comes after the plugin subcommand:

  $ kubectl --kubeconfig admin.conf hns tree default
  Error: unknown command "hns" for "kubectl"
  Run 'kubectl --help' for usage.
  $ kubectl hns --kubeconfig admin.conf tree default
  default
  $ kubectl hns tree --kubeconfig admin.conf default
  default
  $ kubectl hns tree default --kubeconfig admin.conf
  default

This is an inconsistency that can lead to the user confusion regarding
whether they have installed the plugin correctly and so is an unfriendly
user experience. This patch corrects the issue for plugin handling by
relocating persistent flags to the end of the argument list before
attempting to search for the plugin binary.

Which issue(s) this PR fixes:

Fixes #93432

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Fixed parsing of prefixed global flags for kubectl plugins.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2020-08-27T21:17:31Z

Hi @cmurphy. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

eddiezane · 2020-08-28T20:15:15Z

/ok-to-test

brianpursley · 2020-08-31T14:15:15Z

@cmurphy I think you need to run hack/update-bazel.sh to fix the build error.

brianpursley · 2020-08-31T14:28:03Z

This is a tricky issue to solve completely and has been discussed several times before.
kubernetes/kubectl#884
#77666
#93432

I think this PR would help though, by relocating as much as possible after the plugin name. This would relocate global flags, and even non-global flags with no arg or that do not have a space before the arg.

There is still an edge case where if you provide a non-global flag with a space before the arg, the plugin it will fail.... ie. kubectl --foo bar myplugin. kubectl will think bar is the plugin name. Without knowing the plugin's flag set it is more difficult to try to solve this one.

I think this PR is an improvement though and fixes most cases.

/lgtm

cmurphy · 2020-08-31T18:41:11Z

@brianpursley commented on Aug 31, 2020, 7:28 AM PDT:

This is a tricky issue to solve completely and has been discussed several times before.
kubernetes/kubectl#884
#77666
#93432

Thanks for the review and the background information!

I think this PR would help though, by relocating as much as possible after the plugin name. This would relocate global flags, and even non-global flags with no arg or that do not have a space before the arg.

There is still an edge case where if you provide a non-global flag with a space before the arg, the plugin it will fail.... ie. kubectl --foo bar myplugin. kubectl will think bar is the plugin name. Without knowing the plugin's flag set it is more difficult to try to solve this one.

I think this is an oversight on my part, I don't think it should work this way. I think whether the flag-before-the-command has an argument or not, it should be treated the same way, and in this case I think if it is not a recognized global flag it should just be rejected because the "unknown command myplugin" is more confusing than an unrecognized flag. I'll try to fix this.

I think this PR is an improvement though and fixes most cases.

/lgtm

I should mention (and will add in a comment) that the logic and function names are largely inspired by similar functionality in cobra - I would have reused it more directly if any of it was exported.

k8s-ci-robot · 2020-09-04T02:57:50Z

New changes are detected. LGTM label has been removed.

brianpursley · 2020-09-04T13:04:16Z

pkg/kubectl/cmd/cmd.go

+	case strings.HasPrefix(flag, "--"):
+		return fs.Lookup(flag[2:])
+	case strings.HasPrefix(flag, "-"):
+		return fs.ShorthandLookup(flag[1:2])


Is this OK?

My concern is if someone accidentally does - when they meant -- and if this only takes the first character after the - then it could cause the wrong flag to be used instead of failing.

For example, in kubectl run there is an --image flag and a -i flag which is shorthand for --stdin.

In this case it probably would not cause a bad problem because the command would fail, but if I used -image instead of --image, I think the code above would interpret -image as -i, right?

What if it just does this?

return fs.ShorthandLookup(flag[1:])

Or return an error if the length of the single-dash flag is not 1.

Just an idea

EDIT: Or just return null if a single-dash flag is more than 1 character in length might work too

Thanks for catching that, I handled it in the original version and mistakenly removed it

brianpursley · 2020-09-04T13:10:37Z

Looks pretty good I think. Just one minor comment.

Flag parsing is a little hard to imagine given all the scenarios, but I like that you have tests to cover them.

brianpursley · 2020-09-04T17:30:50Z

Looks good. Can you squash your commits into a single commit?

cmurphy · 2020-09-04T17:35:37Z

@brianpursley commented on Sep 4, 2020, 10:30 AM PDT:

Looks good. Can you squash your commits into a single commit?

done

brianpursley · 2020-09-04T17:47:52Z

/assign @seans3

brianpursley · 2020-09-04T18:13:41Z

note to other reviewers. I know we have discussed this topic before on several occasions (I linked above in a previous comment).

The reason I think this PR is fine because it improves the handling of global flags before the plugin name, which helps in the majority of the problem cases where people have aliased kubectl with a flag included.

It still cannot handle non-global flags before the plugin name, but I think that is just going to have to remain a known limitation.

eddiezane · 2020-09-23T16:35:06Z

/assign @soltysh

soltysh

@cmurphy I greatly appreciate your effort and time you spend putting this PR together but as mentioned in #92343 and several other places, which @brianpursley also mentioned sig-cli decided (see https://docs.google.com/document/d/1r0YElcXt6G5mOWxwZiXgGu_X6he3F--wKwg-9UBc29I/edit#heading=h.ji0nwpg7dyuz) that we're not going to re-schuffle flags but rather we'll put a warning in place. I've spent decent amount of time carefully examining your PR today and I was still able to find issues with global flags. Namely passing --v=2, -v=2 or -v 2 which works just fine with kubectl does not b/c -v is not registered as global flags. We could theoretically have a list of exceptions but I'm worried we'd be playing never-ending catch up game with that.

My proposal for this PR and the overall problem are as follows. For this PR I'd like to see the current test case fixed (see my comment below about comparing counts and not actual arguments passed to plugins) and a new test which will run plugin commands with no flags (1), make sure they get routed to, then run with flags after the commands and make sure they get routed to (2), then run with flags before the commands and make sure it errors (3).

Secondly, in the long run I think we should look into cobra flag parsing mechanism and check what we could do to make the flag parsing more predictable.

/hold

soltysh · 2020-10-28T12:44:38Z

pkg/kubectl/cmd/cmd_test.go

+			name:             "test that a plugin executable is found with flags before arg",
+			args:             []string{"kubectl", "--kubeconfig", "bar.conf", "foo"},
+			expectPlugin:     "plugin/testdata/kubectl-foo",
+			expectPluginArgs: []string{"--kubeconfig", "bar"},


bar.conf?

soltysh · 2020-10-28T12:45:20Z

pkg/kubectl/cmd/cmd_test.go

+			name:             "test that a plugin executable is found with flags with '=' before arg",
+			args:             []string{"kubectl", "--kubeconfig=bar.conf", "foo"},
+			expectPlugin:     "plugin/testdata/kubectl-foo",
+			expectPluginArgs: []string{"--kubeconfig=bar"},


--kubeconfig=bar.conf?

soltysh · 2020-10-28T12:46:39Z

pkg/kubectl/cmd/cmd_test.go

+		},
+		{
+			name:             "test that a plugin executable is found with short flags with '=' before arg",
+			args:             []string{"kubectl", "-n=default", "foo"},


This looks wrong, shorthand does not pass argument with =, so this should be an error.

Oh my, I just checked this code and the test only verifies the length of args not their actual contents, see

kubernetes/pkg/kubectl/cmd/cmd_test.go

Line 106 in 1a645c2

if len(pluginsHandler.withArgs) != len(test.expectPluginArgs) {

Can you fix the test to actually compare the contents?

cmurphy · 2020-10-28T20:45:37Z

Thank you for the thorough review, @soltysh !

as mentioned in #92343 and several other places, which @brianpursley also mentioned sig-cli decided (see https://docs.google.com/document/d/1r0YElcXt6G5mOWxwZiXgGu_X6he3F--wKwg-9UBc29I/edit#heading=h.ji0nwpg7dyuz) that we're not going to re-schuffle flags but rather we'll put a warning in place. I've spent decent amount of time carefully examining your PR today and I was still able to find issues with global flags. Namely passing --v=2, -v=2 or -v 2 which works just fine with kubectl does not b/c -v is not registered as global flags. We could theoretically have a list of exceptions but I'm worried we'd be playing never-ending catch up game with that.

Could you clarify here - why is -v not registered as a global flag? From my experimentation, it seems like it is:

$ git diff
diff --git a/pkg/kubectl/cmd/cmd.go b/pkg/kubectl/cmd/cmd.go
index 9b01843796a..1cdede694bc 100644
--- a/pkg/kubectl/cmd/cmd.go
+++ b/pkg/kubectl/cmd/cmd.go
@@ -650,6 +650,8 @@ func NewKubectlCommand(in io.Reader, out, err io.Writer) *cobra.Command {
        cmds.AddCommand(apiresources.NewCmdAPIResources(f, ioStreams))
        cmds.AddCommand(options.NewCmdOptions(ioStreams.Out))
 
+       fmt.Printf("%#v\n", cmds.PersistentFlags().Lookup("v"))
+
        return cmds
 }
$ _output/bin/kubectl get pod
&pflag.Flag{Name:"v", Shorthand:"v", Usage:"number for the log level verbosity", Value:(*pflag.flagValueWrapper)(0xc0004278e0), DefValue:"0", Changed:false, NoOptDefVal:"", Deprecated:"", Hidden:false, ShorthandDeprecated:"", Annotations:map[string][]string(nil)}
No resources found in default namespace.

A plugin may still not respect -v even though it is a global flag, but it now gives an error message about the flag, the same as it would if the flag came at the end:

$ /usr/bin/kubectl version --client
Client Version: version.Info{Major:"1", Minor:"19", GitVersion:"v1.19.3", GitCommit:"1e11e4a2108024935ecfcb2912226cedeafd99df", GitTreeState:"clean", BuildDate:"2020-10-15T00:00:00Z", GoVersion:"go1.15.3", Compiler:"gc", Platform:"linux/amd64"}
$ /usr/bin/kubectl hns tree default --v=2
Error: unknown flag: --v
Usage:
  kubectl tree [flags]
...
$ _output/bin/kubectl version --client
Client Version: version.Info{Major:"1", Minor:"20+", GitVersion:"v1.20.0-alpha.0.1203+ee15e434cb2686", GitCommit:"ee15e434cb2686043c7d1c6b6eaf957a5ce6e02d", GitTreeState:"clean", BuildDate:"2020-10-28T20:10:17Z", GoVersion:"go1.15.3", Compiler:"gc", Platform:"linux/amd64"}
$ _output/bin/kubectl --v=2 hns tree default 
Error: unknown flag: --v

There was an issue with the unit tests where the registered name of the flag became "test.v" for reasons I don't understand so I didn't use it in the tests, but I could dig into that.

Regarding the concern about being able to tell the difference between a flag that has an argument and a flag without an argument, I think I have covered that as well:

$ _output/bin/kubectl --insecure-skip-tls-verify hns tree default # boolean flag
default
$ _output/bin/kubectl --insecure-skip-tls-verify=true hns tree default # boolean with =
default
$ _output/bin/kubectl --namespace=foo hns tree default # flag with arguments with =
default
$ _output/bin/kubectl --namespace foo hns tree default # flag with arguments without =
default
$ _output/bin/kubectl --namespace hns tree default # expected error since the plugin name is interpreted as belonging to the flag
Error: unknown command "tree" for "kubectl"
Run 'kubectl --help' for usage.

What case of flags did I miss where this is doing the wrong thing? Or is the last case in this example what's not acceptable?

I support just emitting an error as in the the other PR, that is certainly simpler and fixes the essence of the problem. I just want to make sure I understand where I went wrong here so I can learn :)

My proposal for this PR and the overall problem are as follows. For this PR I'd like to see the current test case fixed (see my comment below about comparing counts and not actual arguments passed to plugins) and a new test which will run plugin commands with no flags (1), make sure they get routed to, then run with flags after the commands and make sure they get routed to (2),

Happy to turn this PR into fixing the tests

then run with flags before the commands and make sure it errors (3).

This part I think should either wait for #92343 to merge, or perhaps just be part of that PR?

Thanks again for your time!

soltysh · 2020-11-17T20:16:38Z

Could you clarify here - why is -v not registered as a global flag? From my experimentation, it seems like it is:

IIRC the main issues is that -v (with single hyphen is coming from glog, not sure if this still applies to klog, but looking through

kubernetes/cmd/kubectl/kubectl.go

Line 46 in 6dddea5

logs.InitLogs()

kubernetes/staging/src/k8s.io/kubectl/pkg/util/logs/logs.go

Line 33 in 6dddea5

klog.InitFlags(flag.CommandLine)

you'll get to

kubernetes/vendor/k8s.io/klog/v2/klog.go

Line 433 in 6dddea5

flagset.Var(&logging.verbosity, "v", "number for the log level verbosity")

which is behind this. With kubectl in staging we can probably try separating these two somehow to not to rely on this, but we need to ensure that still the verbosity setting works as expected.

What case of flags did I miss where this is doing the wrong thing? Or is the last case in this example what's not acceptable?

I can't remember exactly, but when I checked this last time not all of the ones you mentioned worked as I expect them. I'd need to give it another try.

Happy to turn this PR into fixing the tests

yeah - fixing tests is always nice seen, the more tests the better for us.

This part I think should either wait for #92343 to merge, or perhaps just be part of that PR?

That merged by now.

Ensure the unit tests compare the actual plugin arguments rather than just the number of arguments, and add a test using a plugin with no flags provided.

k8s-ci-robot · 2020-11-19T01:04:48Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: brianpursley, cmurphy
To complete the pull request process, please assign seans3 after the PR has been reviewed.
You can assign the PR to them by writing /assign @seans3 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

staging/src/k8s.io/kubectl/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cmurphy · 2020-11-19T01:04:50Z

Removed the changes to the plugin flag handling, fixed the unit tests to do a real comparison of the argument list and added a test for when no flag is provided to the plugin.

I couldn't test the error case, because the plugin simply does an immediate exit 1 instead of returning, so the unit tests aren't able to handle it safely:

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

Line 321 in e64ebe0

os.Exit(1)

Moreover, the error handling introduced in #92343 doesn't work if there is only one prefixed flag and one command in the plugin name:

$ cat ~/bin/kubectl-foo
echo "My first command-line argument was $1"
$ _output/bin/kubectl version --client
Client Version: version.Info{Major:"1", Minor:"20+", GitVersion:"v1.20.0-beta.2.29+844561654d6f0c", GitCommit:"844561654d6f0ca2d6956af37c295d351e42a64c", GitTreeState:"clean", BuildDate:"2020-11-19T01:00:10Z", GoVersion:"go1.15.4", Compiler:"gc", Platform:"linux/amd64"}
$ _output/bin/kubectl --bar foo
Error: unknown flag: --bar
See 'kubectl --help' for usage.
$ _output/bin/kubectl --bar foo bar
Error: flags cannot be placed before plugin name: --bar

cmurphy · 2020-11-19T05:34:12Z

/retest

k8s-ci-robot added the area/kubectl label Aug 27, 2020

k8s-ci-robot requested review from brianpursley and smarterclayton August 27, 2020 21:17

k8s-ci-robot added sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 27, 2020

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 28, 2020

cmurphy force-pushed the kubectl-plugin-flags branch from 8e1e0a2 to 8c03f80 Compare August 30, 2020 00:58

k8s-ci-robot assigned brianpursley Aug 31, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 31, 2020

cmurphy force-pushed the kubectl-plugin-flags branch from 8c03f80 to 0b81f5a Compare September 4, 2020 02:57

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 4, 2020

brianpursley reviewed Sep 4, 2020

View reviewed changes

cmurphy force-pushed the kubectl-plugin-flags branch from 533e411 to ee15e43 Compare September 4, 2020 17:35

brianpursley approved these changes Sep 4, 2020

View reviewed changes

k8s-ci-robot assigned seans3 Sep 4, 2020

brianpursley removed their assignment Sep 4, 2020

k8s-ci-robot assigned soltysh Sep 23, 2020

sallyom mentioned this pull request Oct 14, 2020

kubectl run does not fail when you do not pass a NAME, when you pass a --command kubernetes/kubectl#943

Closed

soltysh requested changes Oct 28, 2020

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 28, 2020

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 17, 2020

Fix unit tests for kubectl plugins

1283b84

Ensure the unit tests compare the actual plugin arguments rather than just the number of arguments, and add a test using a plugin with no flags provided.

cmurphy force-pushed the kubectl-plugin-flags branch from ee15e43 to 1283b84 Compare November 19, 2020 01:04

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 19, 2020

cmurphy closed this Jan 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix prefixed global flags for kubectl plugins #94290

Fix prefixed global flags for kubectl plugins #94290

cmurphy commented Aug 27, 2020 •

edited

k8s-ci-robot commented Aug 27, 2020

eddiezane commented Aug 28, 2020

brianpursley commented Aug 31, 2020

brianpursley commented Aug 31, 2020

cmurphy commented Aug 31, 2020

k8s-ci-robot commented Sep 4, 2020

brianpursley Sep 4, 2020 •

edited

cmurphy Sep 4, 2020

brianpursley commented Sep 4, 2020

brianpursley commented Sep 4, 2020

cmurphy commented Sep 4, 2020

brianpursley commented Sep 4, 2020

brianpursley commented Sep 4, 2020 •

edited

eddiezane commented Sep 23, 2020

soltysh left a comment

soltysh Oct 28, 2020

soltysh Oct 28, 2020

soltysh Oct 28, 2020

soltysh Oct 28, 2020

cmurphy commented Oct 28, 2020

soltysh commented Nov 17, 2020

k8s-ci-robot commented Nov 19, 2020

cmurphy commented Nov 19, 2020

cmurphy commented Nov 19, 2020

Fix prefixed global flags for kubectl plugins #94290

Fix prefixed global flags for kubectl plugins #94290

Conversation

cmurphy commented Aug 27, 2020 • edited

k8s-ci-robot commented Aug 27, 2020

eddiezane commented Aug 28, 2020

brianpursley commented Aug 31, 2020

brianpursley commented Aug 31, 2020

cmurphy commented Aug 31, 2020

k8s-ci-robot commented Sep 4, 2020

brianpursley Sep 4, 2020 • edited

Choose a reason for hiding this comment

cmurphy Sep 4, 2020

Choose a reason for hiding this comment

brianpursley commented Sep 4, 2020

brianpursley commented Sep 4, 2020

cmurphy commented Sep 4, 2020

brianpursley commented Sep 4, 2020

brianpursley commented Sep 4, 2020 • edited

eddiezane commented Sep 23, 2020

soltysh left a comment

Choose a reason for hiding this comment

soltysh Oct 28, 2020

Choose a reason for hiding this comment

soltysh Oct 28, 2020

Choose a reason for hiding this comment

soltysh Oct 28, 2020

Choose a reason for hiding this comment

soltysh Oct 28, 2020

Choose a reason for hiding this comment

cmurphy commented Oct 28, 2020

soltysh commented Nov 17, 2020

k8s-ci-robot commented Nov 19, 2020

cmurphy commented Nov 19, 2020

cmurphy commented Nov 19, 2020

cmurphy commented Aug 27, 2020 •

edited

brianpursley Sep 4, 2020 •

edited

brianpursley commented Sep 4, 2020 •

edited