Clean up PUT semantics #2114

erictune · 2014-11-01T23:28:56Z

PUTs are not doing what I expect them to do.

Bad Case 1

PUT /api/v1beta1/pods/a
Body:

        {
          "kind": "Pod",
          "apiVersion": "v1beta1",
          "id": "a",
          "desiredState": {
            "manifest": {
              "version": "v1beta1",
              "id": "a",
              "containers": [{ "name": "foo", "image": "bar/foo", }]
            }
          },
        }

I expected 200 since POST of the same body to "/api/v1beta1/pods".
Or, if that is the wrong way to do a PUT, then I would expect a 400, but not a 500. 500 is for server errors.

Here is the response body:

{"kind":"Status","creationTimestamp":null,"selfLink":"/api/v1beta1/pods/a","apiVersion":"v1beta1","status":"Failure","message":"100: Key not found (/registry/pods) [9]","code":500}

Bad Case 2

Previous cases are where no object existed.
Next case is an update (created object named a with a POST already)

PUT /api/v1beta1/pods/a
Same body as above.

I was expecting 200, but I got 500. I'd take a 400 or 404 over a 500, though.

Response body:

{"kind":"Status","creationTimestamp":null,"selfLink":"/api/v1beta1/pods/a","apiVersion":"v1beta1","status":"Failure","message":"105: Key already exists (/registry/pods/default/a) [10]","code":500}

The text was updated successfully, but these errors were encountered:

erictune · 2014-11-01T23:33:11Z

Edited. Same case was pasted in twice.

PUT /api/v1beta1/pods

is 404, which makes sense.

brendandburns · 2014-11-02T02:20:20Z

Put requires that the pod already exist. Have you created it with a POST
prior to attempting the put?

Brendan
On Nov 1, 2014 4:33 PM, "Eric Tune" notifications@github.com wrote:

Edited. Same case was pasted in twice.

PUT /api/v1beta1/pods

is 404, which makes sense.

—
Reply to this email directly or view it on GitHub
#2114 (comment)
.

erictune · 2014-11-02T06:37:11Z

Is that a kubernetes thing?

brendandburns · 2014-11-02T20:18:56Z

I had thought it was generally a RESTful thing (POST == create, PUT == update)

But looking around, it seems like there is disagreements about PUT in the RESTful context...

I could see supporting PUT for create, but it definitely makes the logic more complicated.

smarterclayton · 2014-11-02T22:07:12Z

The spec says that PUT allows create, and is optional. If you support both, there's always the case that you don't want the create behavior on PUT, which means you need to set something on create to do that (If-None-Match: Name, etc). Can be annoying. Since we do have strong naming and versioning, create-or-update seems more useful to clients...

The second case error is definitely wrong, probably because pod update is not using etcderr.InterpretUpdateError

Use some constants for tokens. Refactor tokenfile creation to function. Reorder some test cases to make lookups follow creates so they succeed. Add expected status code to test cases (some are not quite what expected, so filed bugs kubernetes#2112, kubernetes#2113, kubernetes#2114) Check expected status codes. Close Body after each iterations so that we don't run out of file handles when I add even more test cases in the next PR. Handle that it is unpredictable whether status 200 or 202 is returned.

roberthbailey · 2014-12-10T00:33:52Z

Hey Brian -- is this fixed with go-restful?

smarterclayton · 2014-12-10T00:38:27Z

I don't see how it would affect it

bgrant0607 · 2014-12-10T05:54:47Z

@roberthbailey No. Why do you ask?

brendandburns · 2014-12-10T06:57:42Z

We were triaging bugs today, and I thought the go-restful conversion might
have fixed this, but I guess not.

Brendan
On Dec 9, 2014 9:55 PM, "bgrant0607" notifications@github.com wrote:

@roberthbailey https://github.com/roberthbailey No. Why do you ask?

—
Reply to this email directly or view it on GitHub
#2114 (comment)
.

smarterclayton · 2014-12-10T13:49:39Z

I took a todo to look at this after v1veta3 if no one gets there first

On Dec 10, 2014, at 1:58 AM, Brendan Burns notifications@github.com wrote:

We were triaging bugs today, and I thought the go-restful conversion might
have fixed this, but I guess not.

Brendan
On Dec 9, 2014 9:55 PM, "bgrant0607" notifications@github.com wrote:

@roberthbailey https://github.com/roberthbailey No. Why do you ask?

—
Reply to this email directly or view it on GitHub
#2114 (comment)
.

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

smarterclayton · 2014-12-15T04:05:06Z

Noticed a few more behaviors we need to define:

how does a client unambiguously signal a PUT overwrite (write this value no matter what is on the server)? Omit resource version? This is for fire and forget endpoints - make sure this replication controller looks like X, don't care what else happens
- put allows creates, which is useful in some of these cases. We'd need to clearly handle the difference between update existing and update of new (which AtomicUpdate could handle) - one problem is that our own logic doesn't distinguish that today (but it should)
how does a client easily handle retry on collision when an automated process also update the object? We started hitting this on a few resources, esp those where you want to mutate a few properties but status is being updated.
- seems like a variation of etcdtools.AtomicUpdate, but from the client: client.Foo(namespace).AtomicUpdate(name, func(existing *Foo) {}). Would do get then mutate and then retry on conflict
409 conflict returns an api.Status, which means you have to reread via GET. Not the end of the world, but would be more difficult to do in arbitrary clients. Might be better to return 409 and the latest resource, but that would be ugly to plumb
the spec/status discussion also applies to these cases. Spent a lot of time contemplating the generic rest client - I'd argue that clearly defined root rest resources for each type is better for clients. I.e. /pods and /podstatus are easy to work with generic code for (and can have different resource representations), but /pods/spec and /pods/status are a lot harder to write generic code for (reuse existing libraries and patterns).

On Dec 10, 2014, at 1:58 AM, Brendan Burns notifications@github.com wrote:

We were triaging bugs today, and I thought the go-restful conversion might
have fixed this, but I guess not.

Brendan
On Dec 9, 2014 9:55 PM, "bgrant0607" notifications@github.com wrote:

@roberthbailey https://github.com/roberthbailey No. Why do you ask?

—
Reply to this email directly or view it on GitHub
#2114 (comment)
.

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

lavalamp · 2014-12-15T19:54:04Z

how does a client unambiguously signal a PUT overwrite (write this value no matter what is on the server)? Omit resource version? This is for fire and forget endpoints - make sure this replication controller looks like X, don't care what else happens

Right now this is the only behavior available-- if you give the right resource version, server overwrites all fields.

409 conflict returns an api.Status, which means you have to reread via GET. Not the end of the world, but would be more difficult to do in arbitrary clients. Might be better to return 409 and the latest resource, but that would be ugly to plumb

This would be cool, esp combined with a client-side AtomicUpdate...

bgrant0607 · 2014-12-16T22:20:34Z

There was some discussion of updates here.

I guess omission of resourceVersion would be similar to omission of If-match, though it feels error-prone to me. The client library or command-line tool could emulate this by just GETing the resourceVersion, but otherwise taking the body from the user. That's what kubecfg did IIRC. I'd be ok with supporting this (I think of it as update --force) in the apiserver. That goes for creation via PUT, as well -- it could be emulated in the client right now, though it doesn't seem hard to support it in the apiserver.

Automatic retry on collision: One option would be PATCH. Short of that, I think we need a PATCH-like client library method that would GET, re-merge, and PUT again on conflict.

The more general form of this is #1702. In order to remove previously configured fields by omission we'd need to keep track of configured fields in annotations.

What are the difficulties with /pod/spec and /pod/status? Perhaps we should discuss that in #2726.

bgrant0607 · 2014-12-17T18:56:14Z

/cc @nikhiljindal

bgrant0607 · 2015-10-06T22:27:43Z

AllowCreateOnUpdate is currently false for most resources (except services, endpoints, events, limitranges). I think we should enable it for most resources (namespaces TBD) and use the uid as a precondition, if set, in addition to resourceVersion.

fejta-bot · 2017-12-22T00:27:37Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

Prevent issues from auto-closing with an /lifecycle frozen comment.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or @fejta.
/lifecycle stale

fejta-bot · 2018-01-21T01:15:23Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or @fejta.
/lifecycle rotten
/remove-lifecycle stale

bgrant0607 · 2018-02-07T06:50:33Z

/remove-lifecycle rotten

fejta-bot · 2018-06-14T18:53:26Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot · 2018-07-14T19:39:11Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten
/remove-lifecycle stale

fejta-bot · 2018-08-13T20:25:23Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

OCPBUGS-38838: Re-enable 1.31 kubectl tests

erictune mentioned this issue Nov 1, 2014

Improve integration test #2116

Merged

bgrant0607 added area/api Indicates an issue on api area. kind/bug Categorizes issue or PR as related to a bug. labels Nov 3, 2014

erictune mentioned this issue Nov 13, 2014

Automatic API generation via go-restful #2282

Merged

bgrant0607 self-assigned this Dec 4, 2014

roberthbailey added the priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. label Dec 10, 2014

bgrant0607 added the area/usability label Dec 10, 2014

bgrant0607 added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. labels Dec 16, 2014

bgrant0607 added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Feb 5, 2015

bgrant0607 removed this from the v1.0 milestone Jul 1, 2015

bgrant0607 removed this from the v1.0-post milestone Jul 24, 2015

bgrant0607 mentioned this issue Sep 17, 2015

kubectl replace-or-create would be helpful #14040

Closed

calebamiles modified the milestone: v1.6 Mar 8, 2017

bgrant0607 added the triaged label Mar 9, 2017

ethernetdan modified the milestone: v1.6 Mar 10, 2017

bgrant0607 mentioned this issue Aug 9, 2017

API Conventions: PUT does not create new resource kubernetes/community#876

Open

This was referenced Sep 20, 2017

Proposal for re-architecting apply kubernetes/community#1028

Merged

Support updating config map and secret with --from-file #30558

Closed

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 22, 2017

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 21, 2018

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Feb 7, 2018

spiffxp removed the triaged label Mar 16, 2018

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 14, 2018

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jul 14, 2018

k8s-ci-robot closed this as completed Aug 13, 2018

gaziqbal mentioned this issue Feb 27, 2019

CSharp PUT wrappers kubernetes-client/csharp#248

Closed

Nuru mentioned this issue Feb 12, 2021

Replace does not work for services #91459

Closed

danwinship pushed a commit to danwinship/kubernetes that referenced this issue Oct 18, 2024

Merge pull request kubernetes#2114 from ardaguclu/ocpbugs-38838

81c1851

OCPBUGS-38838: Re-enable 1.31 kubectl tests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up PUT semantics #2114

Clean up PUT semantics #2114

erictune commented Nov 1, 2014

erictune commented Nov 1, 2014

brendandburns commented Nov 2, 2014

erictune commented Nov 2, 2014

brendandburns commented Nov 2, 2014

smarterclayton commented Nov 2, 2014

roberthbailey commented Dec 10, 2014

smarterclayton commented Dec 10, 2014

bgrant0607 commented Dec 10, 2014

brendandburns commented Dec 10, 2014

smarterclayton commented Dec 10, 2014

smarterclayton commented Dec 15, 2014

lavalamp commented Dec 15, 2014

bgrant0607 commented Dec 16, 2014

bgrant0607 commented Dec 17, 2014

bgrant0607 commented Oct 6, 2015

fejta-bot commented Dec 22, 2017

fejta-bot commented Jan 21, 2018

bgrant0607 commented Feb 7, 2018

fejta-bot commented Jun 14, 2018

fejta-bot commented Jul 14, 2018

fejta-bot commented Aug 13, 2018

Clean up PUT semantics #2114

Clean up PUT semantics #2114

Comments

erictune commented Nov 1, 2014

Bad Case 1

Bad Case 2

erictune commented Nov 1, 2014

brendandburns commented Nov 2, 2014

erictune commented Nov 2, 2014

brendandburns commented Nov 2, 2014

smarterclayton commented Nov 2, 2014

roberthbailey commented Dec 10, 2014

smarterclayton commented Dec 10, 2014

bgrant0607 commented Dec 10, 2014

brendandburns commented Dec 10, 2014

smarterclayton commented Dec 10, 2014

smarterclayton commented Dec 15, 2014

lavalamp commented Dec 15, 2014

bgrant0607 commented Dec 16, 2014

bgrant0607 commented Dec 17, 2014

bgrant0607 commented Oct 6, 2015

fejta-bot commented Dec 22, 2017

fejta-bot commented Jan 21, 2018

bgrant0607 commented Feb 7, 2018

fejta-bot commented Jun 14, 2018

fejta-bot commented Jul 14, 2018

fejta-bot commented Aug 13, 2018