Support selectors for kubectl patch

[tnozicka]

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR adds 3 selector flags for kubectl patch that are common on other commands:

• --label to select object using labels
• --field-selector to select objects by fields
• --all to select all objects within a namespace

Which issue(s) this PR fixes:

Fixes kubernetes/kubectl#909

Special notes for your reviewer:

Does this PR introduce a user-facing change?

`kubectl patch` now support `--all`, `--label` and `--field-selector`, similarly to other existing `kubectl` commands.

/sig cli
/priority important-longterm

[k8s-ci-robot]

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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: tnozicka
Once this PR has been reviewed and has the lgtm label, please assign seans3 for approval. For more information see the Kubernetes Code Review Process.

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

[tnozicka]

Job execution failed: Pod got deleted unexpectedly
/retest

[tnozicka]

@ardaguclu @mpuckett159 gentle ping

[ardaguclu]

field and label selectors in patch command sounds reasonable to me but I'm strongly reluctant to add all in here. Because this flag already creates confusions to users in other commands and I don't think, we'd want to add more.

In addition to that all flag is also mutually exclusive with the other selectors

kubernetes/staging/src/k8s.io/cli-runtime/pkg/resource/builder.go

Line 585 in 1cfb932

b.errs = append(b.errs, fmt.Errorf("setting 'all' parameter but found a non empty selector. "))

[mpuckett159]

Agreed don't want to add all because it is highly confusing to users as it does not actually mean "all."

[tnozicka]

Well, it means "select all instances" so you don't have to create dummy selector when you want to select all instances of the object. Would you prefer the flag name to be different? If so which one and how'd you see the consistency with other commands and reusing the builders?

[ardaguclu]

Could you please add unit tests for these 2 flags in here

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

Line 17 in 00c3094

package patch

and integration tests preferably in

kubernetes/test/cmd/core.sh

Line 423 in 76ee378

kubectl patch "${kube_flags[@]}" pod pod-with-precision -p='{"metadata":{"annotations":{"patchkey": "patchvalue"}}}'

[mpuckett159]

I am hesitant to approve this. SIG-CLI generally does not prefer to add more flags to existing imperative commands as it encourages people to continue using them for use cases they are not designed for. Especially with these particular flags, because they encourage users to make sweeping changes to large amounts of objects at once, when these commands are only meant to be introductory tools, or for quick testing and proof of concept scenarios. We have been trying to curb the expansion of these flags for some time. I don't want to close this immediately to allow for some discussions.

[tnozicka]

Especially with these particular flags, because they encourage users to make sweeping changes to large amounts of objects at once, when these commands are only meant to be introductory tools, or for quick testing and proof of concept scenarios.

I have to admit I am a bit surprised to hear kubectl patch being called an "introductory tool". To my understanding kubectl patch along with kubectl create, kubectl replace, kubectl delete and the rest of them mirror the REST API methods (with some variations) and are essential for kubectl and for both developers and administrators when operating a Kubernetes cluster.

because they encourage users to make sweeping changes to large amounts of objects at once

I don't think it's about encouraging modifying multiple objects at once but more about operating a real clusters where there is a lot of instances of those objects and sometimes you need to make changes that aren't exposed in higher level objects or are maintenance based. Obviously, there are some commands that allow avoiding doing the patches manually, like kubectl label, for a small subset of common tasks, but that hardly diminishes the importance of kubectl patch. Especially, with CRDs there is no way kubectl can have dedicated commands which makes the generic ones even more important. Here are some examples where kubectl patch is used for operating procedures:

• https://kubernetes.io/blog/2021/05/14/using-finalizers-to-control-deletion/#understanding-finalizers
  (good example for --all if you want to remove finalizers from say all configmaps)
• https://kubernetes.io/docs/tasks/administer-cluster/change-default-storage-class/#changing-the-default-storageclass
• https://docs.openshift.com/container-platform/4.14/registry/securing-exposing-registry.html
• https://docs.openshift.com/container-platform/4.14/networking/ovn_kubernetes_network_provider/rollback-to-openshift-sdn.html
• https://stackoverflow.com/questions/52080612/how-to-edit-all-the-deployment-of-kubernetes-at-a-time
• Bulk changing PVCs (e.g. to change size and allows in place resize) that are not managed by StatefulSets (or a similar case in general)
• Developers that want to bulk clear some fields on objects
• Admins that needs to do changes on multiple object at once

[mpuckett159]

Sorry I meant to add this link to my previous comment. Please see this blog post for SIG-CLI's position regarding patch and other commands like it.

https://kubernetes.io/docs/concepts/overview/working-with-objects/object-management/

As well as this presentation that some of the SIG-CLI leads and myself gave regarding this issue at the most recent KubeCon.

https://youtu.be/RggqaCSdOGA?si=tjj9g_vfA9M5NNjq&t=558

I know that this is not generally what people like to hear, but unfortunately it is very common for a lot of reasons, and there isn't much we can do to prevent people from using these commands in the ways you've described above without removing the commands entirely from the code base, which is not something that project is willing to do.

Thank you for providing the links and examples though, this is helpful for us to find more pain points that lead to people relying on imperative commands rather than using the preferred declarative workflow for production systems.

[tnozicka]

Thanks for the links, now I know where the view is coming from. While I am the first person to tell people to use GitOps and prefer kubectl apply --server-side it isn't, and likely never will be, "one fits all" cases. It's generally meant for .spec + .metadata and only the resources you manage / define. I'll list a few examples to show that point:

• Objects that aren't created by the user but he needs to edit the spec, add annotations, labels, or something else. That includes Nodes, default ConfigMaps or Secrets, Namespaces, global CRDs created by operators, and others. Unless all controllers would use SSA (they don't) this won't work.
• Object that are created by other Objects and you don't define them in your Object. Even better, they have variable instance count so storing a manifest for each isn't conceptionaly doable and is often one shot edit. A concrete case is e.g. PVCs for StatefulSets.
• Editing .status. (I don't see a meaningful way to use apply there unless we want to tel people to replace kubectl patch with kubectl get -o yaml | yq '.make-my-change' | kubectl apply --server-side -f -
• Development flows where for + kubectl get -o yaml + yq + kubectl apply is far from the effective path.
• Admins needing to unstuck cluster when they hit a bug or roadblock.

While I agree that we should tell people to deploy apps using kubectl apply --server-side those are not the only actions needing to be done on Kubernetes clusters and kubectl patch is still valuable and maps directly to an API concept that it exposes (contrary to some generator commands) which was the founding base of what kubectl should do.

I suppose mostly you could do bulk patch with kubectl apply like:

for o in $( kubectl get my-resource -o name ); do
  kubectl get my-resource/"${o}" -o yaml | yq 'del(.status.conditions)' | kubectl apply --server-side -f -
done

but that needs extra binary, makes a ton of extra API calls on large clusters (terribly slow) and misses extra concepts like UID preconditions.

/cc @soltysh
(as I saw you on the stage in the referenced KubeCon recording :) )

[k8s-triage-robot]

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

This bot triages PRs 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 PR is closed

You can:

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

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

/lifecycle stale

[tnozicka]

/remove-lifecycle stale

[k8s-ci-robot]

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-sigs/prow repository.