Support Patch request in client

[grantr]

Patch is similar to Update but submits the patch data in the request body instead of the object content. Patch data is accompanied by a patch type specifying the patch strategy, e.g. types.MergePatchType.

A subresource can be patched by passing the subresource path as variadic string arguments.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

If they are not already assigned, you can assign the PR to them by writing /assign 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:

• OWNERS

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

[DirectXMan12]

I'd like to see this accompanied by (or at least with a plan for) how we'd make it easy for users to use this. Building patches isn't necessarily obvious or straightforward, and the difference between the different types isn't always clear to users.

I think we want to try and make sure interfaces that we expose like this are useful for people who aren't a-priori versed in how patch works.

Additionally, it might be worth thinking about how this signature would/could/should change when server-side apply merges.

[DirectXMan12]

(I also think we want a better story in general for acting on subresources, so it might be worth having a broader discussion about that @pwittrock -- once you hit polymorphic subresources, it gets weird). I don't have any immediate thoughts on the subject off the top of my head -- need to spend some time mulling it over. Would love to hear any external ideas.

[grantr]

I think we want to try and make sure interfaces that we expose like this are useful for people who aren't a-priori versed in how patch works.

👍 on the intent. In the meantime, could there be a separate client interface that implements methods like this? This gives an escape hatch to users who need it, and makes it clear that this interface doesn't offer the same compatibility guarantees as the "blessed" methods.

type ExperimentalClient interface{
  Patch(...)
  UpdateStatus(...)
  Watch(...)
}
client.Experimental().Patch(...)

I haven't thought about it too carefully, but maybe a nice Patch interface could take two objects, the original and the update, and generate a patch internally based on the object diff (and optional filters restricting the patch scope).

client.Patch(ctx, objA, objAPrime)
client.Patch(ctx, objA, objAPrime, client.PatchFilter("metadata", "status"))

Not sure how patch types would integrate, but maybe those are going away with server-side apply.

[DirectXMan12]

I haven't thought about it too carefully, but maybe a nice Patch interface could take two objects, the original and the update, and generate a patch internally based on the object diff (and optional filters restricting the patch scope).

We have that a little bit already in one of the built-in kubernetes utilities, and it's not horrible (I don't recall if the filters thing exists, but the base concept exists -- it converts both to json then does the diff that way using a json diff library).

Other wild options off the top of my head (that I'm not sure how much I like): a series of generated objects with setters that record the changes (would mostly apply to kubebuilder instead), or generated diff methods, but most of this seems irrelevant with server-side apply (which is listed as alpha for 1.12, I believe).

In the meantime, could there be a separate client interface that implements methods like this?

Maybe? I do realize that this is a real-world problem for some users. The only thing that makes me hesitant is the tendency of people to use experimental interfaces anyway and then complain when they break ;-).

This already exists in another sense -- you can always manually construct a generated client (not trying to be cheeky here). Patch is mostly a performance optimization -- you avoid the potential for conflicts causing requeues, but it doesn't gain you much else in controller code, since you can always requeue if an update fails.

At any rate, in light of server-side apply coming soon, suggesting an "experimental" or alternate client in some capacity might be the best option.

anyone else have any thoughts on the matter?

[droot]

Good discussion and very timely because I am starting work on adding support for updating "Status" subresource :) because this is turning out to be a very common ask.
Status seems to be a very simple one and fits nicely in our current structure

  err := client.UpdateStatus(ctx, object)

Re: Patch
I am also trying to understand the use of Patch over Update from best practices pt. of view. @grantr what is your primary motivation in your use-case ? I concur with @DirectXMan12 (quoting below) and would like steer controller authors towards that approach.

This already exists in another sense -- you can always manually construct a generated client (not trying to be cheeky here). Patch is mostly a performance optimization -- you avoid the potential for conflicts causing requeues, but it doesn't gain you much else in controller code, since you can always requeue if an update fails.

Initially when we started with controller-runtime, main goal for client pkg was to implement an interface which serves common needs of controller authors instead of becoming a general purpose kubernetes client. For example, Watch method is never be needed in the client interface because higher abstractions in controller-runtime eliminates the need for that. I think having clarity on goals/non-goals for the client interface will help with some of these discussions ?

[grantr]

@grantr what is your primary motivation in your use-case ?

A controller in https://github.com/knative/serving is using Patch to add a label to a resource it doesn't control. As @DirectXMan12 states, this is a performance optimization, but a useful one as judged by the knative/serving authors. if Patch fit naturally into controller-runtime that would be one less reason not to adopt it.

I understand the counter-arguments, and I'm fine with the answer being "use a normal client-go client" as long as there's a reasonably straightforward way to create/retrieve a client-go client using the controller-runtime config. Documentation to this effect would also be useful. Similar to #88, it should be clear to the user when additional capabilities exist (with greater responsibility).

I think having clarity on goals/non-goals for the client interface will help with some of these discussions ?

Yes definitely!

[DirectXMan12]

let's start adding doc issues for things like this -- we should have a section of "so you're an advanced user that needs an escape hatch until the functionality lands in controller-runtime (if ever)"

[adracus]

Any update on this? I guess patching really is something that is very useful to a broad user base.
I'd be up to continue the work on this PR / file a new one / discuss here so we can get this / similar functionality in.

[grantr]

@adracus I'm not working on this currently but I do think it's a useful feature, especially combined with the recently added unstructured object support.

[DirectXMan12]

Ultimately, what we're probably going to want to do here is have a client.Apply that uses server-side patch when available, and generates a patch otherwise. That probably covers most use case for patch

[adracus]

Alright, I'll bring this PR back to speed again :)

[adracus]

PR is at #235