Proposal: Deprecate the usage of NoOptDefVal on flags

[rikatz]

What would you like to be added: Not added, but removed 😓

We use NoOptDefVal on flags like dry-run, validate and others (mapped below). This generates confusion on the command, as we accept string flags as key=value or key value, but when using NoOptDefVal this is not possible.

This separation of a value makes sense, for instance, let's take validate flag as an example:

kubectl create deploy --image=nginx --validate strict

In this case, do I want to create a deploy called strict and default value, or do I want to do strict validation but I forgot the deployment name? It is not possible to get it.

We already stated that this behavior will be deprecated on the dry-run flag, but I think we should move forward and do with all the other string flags.

If you pass a string flag, you should know what value you want (and not be susceptible to future behavior change if we want to change the "default hidden value".

This will also help to generate consistency in command usage, knowing that string flags can or can't be separated by "=" and that anything else is the real command arg.

Why is this needed: Generate some consistency on command execution

I will try to remember the next sig-cli meeting to bring this subject, but also anyone looking at this feel free to comment here and tell me if it is worth to discussing this or if this behavior is this way and will not change, and close the issue :)

Commands and flags currently with this behavior

• validate and dry-run flags on create * command
• cascade flag on delete * command
• set-raw-bytes flag on config set command
• insecure-skip-tls-verify and embed-certs flag on config set-cluster command
• embed-certs flag on config set-credentials command

[varshaprasad96]

@rikatz I've been looking at this, and agree to the deprecation of NoOptDefVal for consistency. I can work on this if there is no strong opinion against doing so in the community meeting.

/assign @varshaprasad96

[mpuckett159]

/triage accepted I agree that this is an issue. This will be complicated and has more far reaching implications. Require deprecation process (and KEP). This affects more than just string flags also affects bool flags.

[apelisse]

I've clearly been abusing this, as someone who introduced it for both dry-run and validation. I'm not sure HOW I would have done without it.

[rikatz]

@apelisse I don't think there was another way of doing it! But I think right now at least IMHO is a behavior that we should avoid, for command consistency, and make sure that if you want "--validation", you either call the flag a bool (validation=true,false) or a string (validation=type). Or, let's say, in the future if we say that dry-run is not a bool anymore, call the flag "dry-run-mode=client" (yikes, I know, it sucks, but I cannot also think on a different way)

[salavessa]

I just bumped into a highly undesired behavior performing a kubectl delete sts my-sts --cascade orphan (tested in kubectl versions v1.26.6 and v1.27.3).

By omitting the equals sign, --cascade is assuming the default option background instead of the orphan value and the command above actually calls a delete on both my-sts and orphan.

One thing that makes this even worse is that the bash auto-completion mechanism always sets --cascade<WHITE_SPACE> (exact same behavior for --dry-run).

Below is a snippet of commands with what's been described:

$ kubectl delete sts my-sts --cascade orphan --dry-run server
W0701 19:49:02.551736    8944 helpers.go:663] --dry-run is deprecated and can be replaced with --dry-run=client.
statefulset.apps "my-sts" deleted (dry run)
statefulset.apps "orphan" deleted (dry run)
statefulset.apps "server" deleted (dry run)

$ kubectl delete sts my-sts --cascade orphan --dry-run=server
statefulset.apps "my-sts" deleted (server dry run)
Error from server (NotFound): statefulsets.apps "orphan" not found

$ kubectl delete sts my-sts --cascade=orphan --dry-run=server
statefulset.apps "my-sts" deleted (server dry run)

Hope this will get sorted at some point.

Thanks!

[marckhouzam]

One thing that makes this even worse is that the bash auto-completion mechanism always sets --cascade<WHITE_SPACE> (exact same behavior for --dry-run)

@salavessa I consider this a bug in cobra. would you be willing to open an issue? spf13/cobra

[marckhouzam]

One thing that makes this even worse is that the bash auto-completion mechanism always sets --cascade<WHITE_SPACE> (exact same behavior for --dry-run)

@salavessa I consider this a bug in cobra. would you be willing to open an issue? spf13/cobra

So, after thinking about it some more, I don’t think cobra can do much better.

In your example @salavessa, if you trigger completion: kubectl delete sts my-sts --cascade<TAB>, cobra won’t be able to know if you want to use the default value for cascade, in which case you want to put the space or if you want to use your own value, in which case you want to use an =.

Maybe not adding the space in this case would be slightly more informative?

[mpuckett159]

I think this is something that needs to be solved with user alerting, ie. the interactive delete flag that @ardaguclu is working on, coupled with the kuberc stuff I've been working in. That will force users to verify destructive actions and would catch something like this, assuming the user doesn't just pass a autoyes to everything.

[salavessa]

In your example @salavessa, if you trigger completion: kubectl delete sts my-sts --cascade<TAB>, cobra won’t be able to know if you want to use the default value for cascade, in which case you want to put the space or if you want to use your own value, in which case you want to use an =.

Maybe not adding the space in this case would be slightly more informative?

@marckhouzam

I'm not very concerned about the use of --cascade<SPACE> with auto-complete. What I believe is inconsistent is that --cascade orphan is not doing the same as --cascade=orphan.

For --cascade it feels redundant to have it with a default value because the delete will perform exactly the same way with or without it, i.e. kubectl delete sts my-sts and kubectl delete sts my-sts --cascade do exactly the same.

What I would expect is that --cascade to behave the same way as --namespace option in terms of reading a value with a space or with the equals sign, and it would only use the "default" value when --cascade it's not present:

$ kubectl get pod --namespace
error: flag needs an argument: --namespace
See 'kubectl get --help' for usage.

$ kubectl get pod --namespace dummy
No resources found in dummy namespace.

$ kubectl get pod --namespace=dummy
No resources found in dummy namespace.

For --dry-run it's a bit different because the default option when the option is present (--dry-run=client) is not the same of when the option is not present (--dry-run=none), i.e. currently kubectl delete sts my-sts behaves differently from kubectl delete sts my-sts --dry-run. But I still believe that --dry-run client should behave the same as --dry-run=client.

Thanks

[k8s-triage-robot]

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

• Confirm that this issue is still relevant with /triage accepted (org members only)
• Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle rotten
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[devoxel]

I just bumped into a highly undesired behavior performing a kubectl delete sts my-sts --cascade orphan (tested in kubectl versions v1.26.6 and v1.27.3).

I did this in production and caused a (small internal) outage.

In most CLI's I have used ["--cascade=orphan"] would be the same as ["--cascade","orphan"]. It's a pretty razor thin margin that forgetting a single character means potentially wiping out your whole production workload.