add PATCH verb to apiserver #4706

mikedanese · 2015-02-22T06:02:07Z

This add's a PATCH verb to all resources who's RESTStorage implements RESTGetter and RESTUpdater. Patch implements the merge patch defined in rfc7386. I'm opening this to discuss what how the pkg/client.Client Patch api should look, or if it should be included in the Client api. The difficulty is that when a patch is converted into an api type, information is lost to zero values of fields, thus a patch must be applied as a json string or a map[string]interface{} in order to maintain all information. I will be refactoring kubectl update --patch as well.

 curl localhost:8080/api/v1beta1/pods/web \
    --request PATCH  \
    -H "Content-Type:application/merge-patch+json" \
    -d '{"labels":{"new":"label"}}'

Fixes #4578

vmarmol · 2015-02-23T01:11:37Z

Assigning to @bgrant0607, feel free to rebalance. Maybe @smarterclayton is also interested.

smarterclayton · 2015-02-23T02:18:40Z

pkg/apiserver/resthandler.go

+		ctx := ctxFn(req)
+		ctx = api.WithNamespace(ctx, namespace)
+
+		original, err := r.Get(ctx, name)


This results in two successive reads that are not controlled by the resource version, which I think is wrong. The client has to be able to specify which version to check against. So there's at least a check missing here for the input version against the get version.

I'm mentally debating whether passing a structured type down into Update would be more appropriate (delta, with attached api version). Even a generic patch resource being passed down would work. I mildly dislike reading here then reading again on update - I'd prefer to do it exactly once, in the storage. That's something we could fix later, but it still seems messy.

Makes sense. We could validate that the client passes a resource version in their patch. The intent was to add PATCH without needing to add an extra Patch method to the registry's RESTs. What's the reason that the registry does a get from etcd after update?

----- Original Message -----

+func PatchResource(r RESTPatcher, ctxFn ContextFunc, namer ScopeNamer,
codec runtime.Codec, resource string, admit admission.Interface)
restful.RouteFunction {

return func(req *restful.Request, res *restful.Response) {

w := res.ResponseWriter

// TODO: we either want to remove timeout or document it (if we
document, move timeout out of this function and declare it in
api_installer)

timeout := parseTimeout(req.Request.URL.Query().Get("timeout"))

namespace, name, err := namer.Name(req)

if err != nil {

notFound(w, req.Request)

return

}

ctx := ctxFn(req)

ctx = api.WithNamespace(ctx, namespace)

original, err := r.Get(ctx, name)

Makes sense. We could validate that the client passes a resource version in
their patch. The intent was to add PATCH without needing to add an extra
Patch method to the registry's RESTs. What's the reason that the registry
does a get from etcd after update?

Get before update - it's so that we know that what is stored so we can do a CAS. In the pod changes I made the REST logic happen between the initial get and the CAS, so that you had the existing state to compare to. There's a problem there which is merging - when we merge what the server has with the client's input, we have to make the decision at that time about what to preserve (that's why pod/status was going to exist, so it could have a different decision). Right now we throw away the status section and combine it with the spec (so that users who naively GET -> PUT overwrite don't overwrite status). If we didn't do that, all clients would have to do merges correctly (which they aren't) and some users should be prevented from changing status like pod.status.host, which they aren't today. Admission control probably would be better to run given both the old and the new value as well, and being able to calculate exact deltas for authorization is a nice to have.

Let's slate a time to sit down and go through all the edge cases here - I like the approach, it just highlights how we're not doing everything right for Update today.

Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/4706/files#r25143674

Agree with @smarterclayton. There's probably some refactoring of Update in which we pass in a function that takes the old object with status removed etc and merges the new object, such that both PATCH and PUT could produce such a function.

Yeah - same thought here. We needed that on client update as well (the way merge is handled is ugly right now).

On Feb 23, 2015, at 4:30 PM, Daniel Smith notifications@github.com wrote:

In pkg/apiserver/resthandler.go:

+func PatchResource(r RESTPatcher, ctxFn ContextFunc, namer ScopeNamer, codec runtime.Codec, resource string, admit admission.Interface) restful.RouteFunction {

return func(req *restful.Request, res *restful.Response) {

w := res.ResponseWriter

// TODO: we either want to remove timeout or document it (if we document, move timeout out of this function and declare it in api_installer)

timeout := parseTimeout(req.Request.URL.Query().Get("timeout"))

namespace, name, err := namer.Name(req)

if err != nil {

notFound(w, req.Request)

return

}

ctx := ctxFn(req)

ctx = api.WithNamespace(ctx, namespace)

original, err := r.Get(ctx, name)
Agree with @smarterclayton. There's probably some refactoring of Update in which we pass in a function that takes the old object with status removed etc and merges the new object, such that both PATCH and PUT could produce such a function.

—
Reply to this email directly or view it on GitHub.

@lavalamp The only issue with that is only a few Update implementations do reads before update. RESTs that use AtomicUpdate do read and most that use SetObj do not read. It might be nice to standardize on an Update mechanism that could skip the initial read when unnecessary.

My next set of changes to update will make implementing patch under AtomicUpdate feasible. For now, that doesn't need to block this change. We should leave a todo here to that effect.

On Mar 1, 2015, at 2:08 PM, Mike Danese notifications@github.com wrote:

In pkg/apiserver/resthandler.go:

+func PatchResource(r RESTPatcher, ctxFn ContextFunc, namer ScopeNamer, codec runtime.Codec, resource string, admit admission.Interface) restful.RouteFunction {

return func(req *restful.Request, res *restful.Response) {

w := res.ResponseWriter

// TODO: we either want to remove timeout or document it (if we document, move timeout out of this function and declare it in api_installer)

timeout := parseTimeout(req.Request.URL.Query().Get("timeout"))

namespace, name, err := namer.Name(req)

if err != nil {

notFound(w, req.Request)

return

}

ctx := ctxFn(req)

ctx = api.WithNamespace(ctx, namespace)

original, err := r.Get(ctx, name)
@lavalamp The only issue with that is only a few Update implementations do reads before update. RESTs that use AtomicUpdate do read and most that use SetObj do not read. It might be nice to standardize on an Update mechanism that could skip the initial read when unnecessary.

—
Reply to this email directly or view it on GitHub.

smarterclayton · 2015-02-23T02:24:48Z

I'd like to sketch out how patch fits in with the other changes going on in update (ie whether we should reorganize update so we can avoid double reads) and how we distinguish intent on PATCH to allow status.

mikedanese · 2015-02-23T04:38:44Z

We can hold this pending more discussion on the use case / api of PATCH.

bgrant0607 · 2015-02-23T06:23:39Z

@ghodss and @nikhiljindal may also be interested.

lavalamp · 2015-02-23T21:19:51Z

pkg/apiserver/resthandler.go

+		}
+
+		// PATCH requires same permission as UPDATE
+		err = admit.Admit(admission.NewAttributesRecord(obj, namespace, resource, "UPDATE"))


Might as well check this before doing the read/merge?

bgrant0607 · 2015-02-23T21:35:38Z

Work on status shouldn't hold up patch, IMO. I'll add some thoughts about status to #2726.

smarterclayton · 2015-02-23T22:09:42Z

What's the rush to get patch in? Agree it's nice, but I'd put the priority on getting our existing verbs working before we add new ones.

On Feb 23, 2015, at 4:36 PM, Brian Grant notifications@github.com wrote:

Work on status shouldn't hold up patch, IMO. I'll add some thoughts about status to #2726.

—
Reply to this email directly or view it on GitHub.

bgrant0607 · 2015-02-24T01:53:09Z

No huge rush, but configuration (reconcile) will depend heavily on patch, and I see it as a separate issue from status.

The refactoring you are doing, which admittedly is partly in support of status, is a bigger reason to go slowly with this.

smarterclayton · 2015-02-24T06:34:18Z

I should have a prelim status pull up tomorrow - we can circle the wagons on what we want to do with update, then if necessary we can do the simple patch now and implement the "correct" patch. I want to close on finalizers and acl so we know whether we need to implement per property policy rules in our engine, with that we probably have the last of the near term update reqts closed (except splitting into two objects which belongs below the storage interface anyway).

On Feb 23, 2015, at 5:53 PM, Brian Grant notifications@github.com wrote:

No huge rush, but configuration (reconcile) will depend heavily on patch, and I see it as a separate issue from status.

The refactoring you are doing, which admittedly is partly in support of status, is a bigger reason to go slowly with this.

—
Reply to this email directly or view it on GitHub.

mikedanese · 2015-03-03T19:22:37Z

One more question. Is the "patch body contains a resourceVersion" condition something we want to validate on input? If there is no resourceVersion should we just update against the current resource?

smarterclayton · 2015-03-03T19:32:04Z

I'm uncomfortable encouraging clients to not patch with resource version. However, arbitrary patch isn't unreasonable to support. I would not do anything specific around resource version right now, and we can add it in later.

On Mar 3, 2015, at 2:22 PM, Mike Danese notifications@github.com wrote:

One more question. Is the "patch body contains a resourceVersion" condition something we want to validate on input? If there is no resourceVersion should be just update against the current resource?

—
Reply to this email directly or view it on GitHub.

mikedanese · 2015-03-03T21:02:43Z

Current iteration will honor resource version when it is passed in the patch body, otherwise it will use the resourceVersion returned in the first Get so the get/patch/write cycle will be controlled by resourceVersion as well.

smarterclayton · 2015-03-10T15:57:22Z

LGTM, waiting for rerun of Travis, sorry for the long delay.

bgrant0607 · 2015-03-10T18:32:35Z

@ghodss How would this PR need to change in order to support custom list merging?

bgrant0607 · 2015-03-10T18:38:11Z

I'm going to merge this as-is (Shippable passed, Travis is backed up), but we've decided to change merge semantics: #4889. I'm counting on lack of documentation and kubectl integration to prevent rapid proliferation of usage. :-)

add PATCH verb to apiserver

ghodss · 2015-03-11T03:50:22Z

That's helpful actually - I will open a new PR that enhances this with list merging.

googlebot added the cla: yes label Feb 22, 2015

mikedanese force-pushed the PATCH branch 7 times, most recently from 7483ba1 to ed95937 Compare February 22, 2015 08:18

mikedanese changed the title ~~WIP: add PATCH verb to apiserver~~ add PATCH verb to apiserver Feb 22, 2015

mikedanese force-pushed the PATCH branch from ed95937 to f16c82d Compare February 22, 2015 16:57

vmarmol assigned lavalamp and bgrant0607 and unassigned lavalamp Feb 23, 2015

smarterclayton reviewed Feb 23, 2015
View reviewed changes

mikedanese mentioned this pull request Feb 23, 2015

Add PATCH verb to apiserver resources #4578

Closed

lavalamp reviewed Feb 23, 2015
View reviewed changes

bgrant0607 assigned smarterclayton and unassigned bgrant0607 Feb 23, 2015

mikedanese force-pushed the PATCH branch 3 times, most recently from 9be6299 to 4ce03bc Compare February 28, 2015 01:31

mikedanese force-pushed the PATCH branch 2 times, most recently from 54ada3f to ae2017c Compare March 1, 2015 17:32

mikedanese force-pushed the PATCH branch from ae2017c to 16c64a5 Compare March 3, 2015 19:52

mikedanese force-pushed the PATCH branch 3 times, most recently from 330cbcf to 329c132 Compare March 8, 2015 17:51

smarterclayton closed this Mar 10, 2015

smarterclayton reopened this Mar 10, 2015

smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 10, 2015

mikedanese force-pushed the PATCH branch 2 times, most recently from 4b861f8 to 587bfc4 Compare March 10, 2015 17:16

add PATCH to apiserver

46ec580

mikedanese force-pushed the PATCH branch from 587bfc4 to 46ec580 Compare March 10, 2015 17:57

bgrant0607 removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 10, 2015

bgrant0607 added a commit that referenced this pull request Mar 10, 2015

Merge pull request #4706 from mikedanese/PATCH

7aa060b

add PATCH verb to apiserver

bgrant0607 merged commit 7aa060b into kubernetes:master Mar 10, 2015

mikedanese deleted the PATCH branch March 10, 2015 18:41

satnam6502 mentioned this pull request Mar 19, 2015

PUT fails with an obscure error if resourceVersion is not specified #3085

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add PATCH verb to apiserver #4706

add PATCH verb to apiserver #4706

mikedanese commented Feb 22, 2015

vmarmol commented Feb 23, 2015

smarterclayton Feb 23, 2015

mikedanese Feb 23, 2015

smarterclayton Feb 23, 2015

lavalamp Feb 23, 2015

smarterclayton Feb 23, 2015

mikedanese Mar 1, 2015

smarterclayton Mar 1, 2015

smarterclayton commented Feb 23, 2015

mikedanese commented Feb 23, 2015

bgrant0607 commented Feb 23, 2015

lavalamp Feb 23, 2015

mikedanese Mar 3, 2015

bgrant0607 commented Feb 23, 2015

smarterclayton commented Feb 23, 2015

bgrant0607 commented Feb 24, 2015

smarterclayton commented Feb 24, 2015

mikedanese commented Mar 3, 2015

smarterclayton commented Mar 3, 2015

mikedanese commented Mar 3, 2015

smarterclayton commented Mar 10, 2015

bgrant0607 commented Mar 10, 2015

bgrant0607 commented Mar 10, 2015

ghodss commented Mar 11, 2015

add PATCH verb to apiserver #4706

add PATCH verb to apiserver #4706

Conversation

mikedanese commented Feb 22, 2015

vmarmol commented Feb 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Feb 23, 2015

mikedanese commented Feb 23, 2015

bgrant0607 commented Feb 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgrant0607 commented Feb 23, 2015

smarterclayton commented Feb 23, 2015

bgrant0607 commented Feb 24, 2015

smarterclayton commented Feb 24, 2015

mikedanese commented Mar 3, 2015

smarterclayton commented Mar 3, 2015

mikedanese commented Mar 3, 2015

smarterclayton commented Mar 10, 2015

bgrant0607 commented Mar 10, 2015

bgrant0607 commented Mar 10, 2015

ghodss commented Mar 11, 2015