Mark --pod as deprecated for kubectl exec and port-forward

[yuexiao-wang]

Signed-off-by: yuexiao-wang wang.yuexiao@zte.com.cn

What this PR does / why we need it:

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged):
fixes kubernetes/kubectl#104

Special notes for your reviewer:
@apelisse @php-coder

Release note:

Deprecation: The flag `pod ` of kubectl exec and port-forward is deprecated and will be removed in a future release.

[rootfs]

Is there a deprecation announcement?
/unassign

[dims]

/ok-to-test

[apelisse]

Hi @yuexiao-wang, thanks for working on this.

Did you test this code? What happened?

[apelisse]

@yuexiao-wang I tried to run it, and it didn't do what I expected:

1. It still printed the option when running kubectl help port-forward or kubectl help exec
2. It printed two warnings when running kubectl exec -p random_pod_name, I don't think that's expected

Would you mind looking into these two problems?

Also I might remember that @alexandercampbell was testing the deprecation flag somehow, maybe you could look for his code and find how he was doing it.

Thank you!

[yuexiao-wang]

Thanks for review. I will check it

[yuexiao-wang]

@apelisse I am sure that it is a issue of spf13/cobra used by kubernetes.
Because these deprecated flag dosen't work such as kubectl export --container-port, kubectl get --experimental-use-openapi-print-columns.

We can find this issue of cobra as follows:
spf13/cobra#463
The PR is merged to fix the issue in cobra as below:
spf13/cobra#466

I find cobra is updated in kompose:
kubernetes/kompose#663

Should we update cobra in kubernetes?

According to https://github.com/kubernetes/community/blob/master/contributors/devel/godep.md, I encounter some problems when update cobra in my development enviroment

[yuexiao-wang]

/retest

[apelisse]

I don't think that issue is related. You could try to apply that one pull-request in your vendor/github.com/spf13/pflag and test to see if it fixes it?

[supereagle]

Add TODO comment, and relate an issue to record this.

[yuexiao-wang]

@apelisse Sorry, I am respond late.

1. How do use pflag in kubectl ?
2. I use the newest version of cobra to fix it in my example. but I update cobra in vendor failed

[apelisse]

@yuexiao-wang what behavior are you expecting from kubectl (with regard to this pull-request)? Can you describe it? I want to be sure we're expecting the same thing :-)

[yuexiao-wang]

@apelisse I expect that kubectl can show deprecated messages in help output. for example
$ kubectl.sh exec -h | grep -e '-p'
-p POD_NAME is DEPRECATED and will be removed in a future version. Use exec POD_NAME instead.

I use MarkDeprecated from spf13/cobra to mark the option as deprected, but it doesn't work.
Accord to you suggestion, I find spf13/pflag isn't used in kubectl.

I think it need to update spf13/cobra, then my PR can work to show deprecated option

[yuexiao-wang]

If I understand you incorrectly, please inform me.

[mengqiy]

@yuexiao-wang I think you need another PR to update the vendor/ to pick up the fix in spf13/cobra#463

[yuexiao-wang]

@mengqiy Thanks for review. I am no familiar to update cobra in vendor. Should I read https://github.com/kubernetes/community/blob/master/contributors/devel/godep.md?
I encounter some problems when update cobra in my development enviroment
Therefore I don't create another PR to update cobra in vendor

[apelisse]

$ kubectl.sh exec -h | grep -e '-p'
-p POD_NAME is DEPRECATED and will be removed in a future version. Use exec POD_NAME instead.

@yuexiao-wang I agree that this is probably what we want. Have you looked at the code for spf13/cobra and spf13/flag? Are you sure the latest version is going to do what you expect? If yes, can you point me to the piece of code that will?

[mengqiy]

@yuexiao-wang If we can confirm latest cobra has the fix, then @apelisse and I can help with updating the vendor.

[yuexiao-wang]

I confirm that the latest cobra don't support to show deprecated message in the help output, and they have planed to support this feature in v2.0.0.

spf13/cobra#466 fixes the issue to show deprecated message when run cli command.
Now K8s call printDeprecationWarning to show deprecated message

[yuexiao-wang]

@apelisse Could you any suggestion about kubernetes/kubectl#104

[apelisse]

@yuexiao-wang Thank you for investigating that issue. All of this makes sense. What do you think we should be doing?

[mengqiy]

they have planed to support this feature in v2.0.0.

When? AFAIK Cobra is now 0.0.1, isn't it?

[yuexiao-wang]

Yes, Cobra is now 0.0.1. The member didn't tell me the release time of cobra as follows.
spf13/cobra#566
This PR maybe closed to wait for cobra v2.0.0.

[apelisse]

I'm still waiting for a deeper analysis:

• This PR is trying to rightfully use the "MarkDeprecated" semantic from cobra, I think we should get this PR merged once it works properly,
• It doesn't work properly because it is redundant with our custom "deprecated" warning. That custom code should be removed,
• I think we should push harder on cobra: nothing prevents us from contributing to that project, and vendor any git hash that we want in kubernetes repo.

@yuexiao-wang are you willing to do that?

Thanks,
Antoine

[yuexiao-wang]

It doesn't work properly because it is redundant with our custom "deprecated" warning. That custom code should be removed,

I will do it when update vendor spf13/cobra at #53631

I think we should push harder on cobra: nothing prevents us from contributing to that project, and vendor any git hash that we want in kubernetes repo.

I am no familiar with codes of cobra and no sure to fix the issue in cobra.
I am willing to analyze cobra code.

[apelisse]

I will do it when update vendor spf13/cobra at #53631

OK Let's wait for that PR to get merged, and see what's needed then

[apelisse]

I've just checked-out the code of #53631, and rebased it on the most recent master. Compiled kubectl. Bug is still there:

> my-kubectl exec -h
Execute a command in a container.

Examples:
  # Get output from running 'date' from pod 123456-7890, using the first container by default
  kubectl exec 123456-7890 date
  
  # Get output from running 'date' in ruby-container from pod 123456-7890
  kubectl exec 123456-7890 -c ruby-container date
  
  # Switch to raw terminal mode, sends stdin to 'bash' in ruby-container from pod 123456-7890
  # and sends stdout/stderr from 'bash' back to the client
  kubectl exec 123456-7890 -c ruby-container -i -t -- bash -il
  
  # List contents of /usr from the first container of pod 123456-7890 and sort by modification time.
  # If the command you want to execute in the pod has any flags in common (e.g. -i),
  # you must use two dashes (--) to separate your command's flags/arguments.
  # Also note, do not surround your command and its flags/arguments with quotes
  # unless that is how you would execute it normally (i.e., do ls -t /usr, not "ls -t /usr").
  kubectl exec 123456-7890 -i -t -- ls -t /usr

Options:
  -c, --container='': Container name. If omitted, the first container in the pod will be chosen
  -p, --pod='': Pod name
  -i, --stdin=false: Pass stdin to the container
  -t, --tty=false: Stdin is a TTY

Usage:
  kubectl exec POD [-c CONTAINER] -- COMMAND [args...] [options]

Use "kubectl options" for a list of global command-line options (applies to all commands).

[apelisse]

@yuexiao-wang Any news on that? If not I'll probably close that PR

[quasoft]

Some analysis of this issue.

Since commit spf13/pflag@329ebf1 deprecated messages are hidden from help menu by default, but can be shown if the developer changes the flag's hidden property to false (cmd.Flag("pod").Hidden = false).

If Hidden is false deprecated messages are appended to the help menu with the FlagUsagesWrapped function:

https://github.com/spf13/pflag/blob/3ebe029320b2676d667ae88da602a5f854788a8a/flag.go#L719-L721

But this function is not called in our case, because kubectl uses a custom template for flags usage. It replaces the default template

https://github.com/spf13/cobra/blob/7c4570c3ebeb8129a1f7456d0908a8b676b6f9f1/command.go#L410-L411

with this one:

kubernetes/pkg/kubectl/cmd/templates/templates.go

Lines 51 to 54 in d244fa9

	SectionFlags = `{{ if or $visibleFlags.HasFlags $explicitlyExposedFlags.HasFlags}}Options:
	{{ if $visibleFlags.HasFlags}}{{trimRight (flagsUsages $visibleFlags)}}{{end}}{{ if $explicitlyExposedFlags.HasFlags}}{{trimRight (flagsUsages $explicitlyExposedFlags)}}{{end}}
	{{end}}`

and this one utilizes the flagsUsages function from templater.go

kubernetes/pkg/kubectl/cmd/templates/templater.go

Line 207 in 5e08ae0

func flagsUsages(f *flag.FlagSet) string {

which outputs flag usage with the Fprintf below, but ignores the deprecated message:

kubernetes/pkg/kubectl/cmd/templates/templater.go

Line 226 in 5e08ae0

fmt.Fprintf(x, format, flag.Shorthand, flag.Name, flag.DefValue, flag.Usage)

This can be fixed by changing a few lines in flagsUsages:

usage := flag.Usage
if len(flag.Deprecated) != 0 {
    usage += fmt.Sprintf(" (DEPRECATED: %s)", flag.Deprecated)
}
fmt.Fprintf(x, format, flag.Shorthand, flag.Name, flag.DefValue, usage)

Apart from that it seems that rebasing the PR of @yuexiao-wang with master effectively hides the --pod flag from the exec and port-forward help menus and only shows the deprecated message when command is executed.

Actually that was one of the suggested solutions with kubernetes/kubectl#104, so the change to flagsUsages decribed above might not be needed at all.

@apelisse If everyone agrees that hiding deprecated flags from the menu is just enough, would it be posible to reopen this PR?

[k8s-ci-robot]

@yuexiao-wang: PR needs rebase.

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.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yuexiao-wang
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: apelisse

If they are not already assigned, you can assign the PR to them by writing /assign @apelisse in a comment when ready.

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

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/kubectl/OWNERS

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

[apelisse]

Actually @quasoft, do you mind opening a new PR?

[k8s-ci-robot]

@yuexiao-wang: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-100-performance	adfb738	link	/test pull-kubernetes-e2e-gce-100-performance
pull-kubernetes-bazel-test	adfb738	link	/test pull-kubernetes-bazel-test
pull-kubernetes-integration	adfb738	link	/test pull-kubernetes-integration
pull-kubernetes-bazel-build	adfb738	link	/test pull-kubernetes-bazel-build
pull-kubernetes-e2e-gce	adfb738	link	/test pull-kubernetes-e2e-gce
pull-kubernetes-e2e-gce-device-plugin-gpu	adfb738	link	/test pull-kubernetes-e2e-gce-device-plugin-gpu
pull-kubernetes-e2e-kops-aws	adfb738	link	/test pull-kubernetes-e2e-kops-aws
pull-kubernetes-kubemark-e2e-gce-big	adfb738	link	/test pull-kubernetes-kubemark-e2e-gce-big
pull-kubernetes-node-e2e	adfb738	link	/test pull-kubernetes-node-e2e
pull-kubernetes-verify	adfb738	link	/test pull-kubernetes-verify
pull-kubernetes-typecheck	adfb738	link	/test pull-kubernetes-typecheck

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.