10000 API reference has incorrect response schema for many delete calls · Issue #30537 · kubernetes/kubernetes · GitHub
[go: up one dir, main page]

Skip to content

API reference has incorrect response schema for many delete calls #30537

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
randalloveson opened this issue Aug 12, 2016 · 13 comments
Closed

API reference has incorrect response schema for many delete calls #30537

randalloveson opened this issue Aug 12, 2016 · 13 comments
Labels
area/api Indicates an issue on api area. area/apiserver area/swagger lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. priority/backlog Higher priority than priority/awaiting-more-evidence. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.

Comments

@randalloveson
Copy link

The API reference seems to assume a response schema of unversioned.Status for all delete calls, individual or collection. In reality, the response schema differers from resource to resource, and for some it is correct, but for many the response schema is actually an individual or list of the given resource. For example, the delete call for the Pod resource returns a Pod and a PodList for the individual and collection delete calls, respectively.

Short term, we should update the docs to reflect reality. Long term, wouldn't it make sense to unify the API so all the resources behave the same? Currently it is left up to the resource implemenation:

https://github.com/kubernetes/kubernetes/blob/master/pkg/apiserver/resthandler.go#L807-L827

@k8s-github-robot k8s-github-robot added area/controller-manager sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Aug 12, 2016
@mikedanese
Copy link
Member

Delete returns inconsistently across types. Too late to do anything about this?

cc @bgrant0607 @kubernetes/api-review-team

@bgrant0607 bgrant0607 added area/swagger area/api Indicates an issue on api area. labels Aug 12, 2016
@bgrant0607
Copy link
Member

IIRC, part of the problem was that we couldn't express in Swagger 1.2 that one type was returned in the case of success and another was returned in the case of failure.

cc @lavalamp @mbohlool

@bgrant0607 bgrant0607 added the priority/backlog Higher priority than priority/awaiting-more-evidence. label Aug 12, 2016
@randalloveson
Copy link
Author

@bgrant0607 it would be nice to sum up that quirk in a note somewhere, since it's true of all the API calls. However, this issue is about calls where the documentation specifies an incorrect response schema for the successful responses, and that successful delete response's schemas are inconsistent from resource type to resource type.

@mikedanese
Copy link
Member
mikedanese commented Aug 12, 2016

From my experimenting:

  • Succesful deletion of a single CertificateSigningRequest returns unversioned.Status
  • Succesful deletion of all CertificateSigningRequest returns printed CertificateSigningRequestList
  • Failed deletion of a single Pod returns unversioned.Status
  • Succesful deletion of a pod returns a Pod

@randalloveson
Copy link
Author
randalloveson commented Aug 12, 2016

I think REST dictates that for a synchronous delete, the body for a 200 response should be the entity deleted. So the Pod API is an example of good RESTful behavior, and the CertificateSigningRequest API is incorrect.

EDIT: Either way the documentation does not reflect the real behavior.

@bgrant0607
Copy link
Member

@randalloveson Yes, unfortunately there are many quirks. Please add this to #25716.

If you're interested in helping, check out SIG API machinery.
https://github.com/kubernetes/community/

@lavalamp
Copy link
Member

I added this to the umbrella issue.

Sounds like pod is doing this correctly, all resources should be going through the same code so it's a bit weird that it's different for other resources.

@loewenstein
Copy link

Actually the swagger spec is wrong in this regard and that is IMHO an even worse situation than broken documentation.

See https://github.com/kubernetes/kubernetes/blob/master/api/swagger-spec/v1.json#L7861 for the Pod example of wrong return type.

What should I expect when this is going to be resolved?

The client binding I am using is completely generated from swagger and hence it is wrong.

@fejta-bot
Copy link

Issues go stale after 30d 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

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 19, 2017
@fejta-bot
Copy link

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

@k8s-ci-robot k8s-ci-robot add 8000 ed 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 18, 2018
@bgrant0607
Copy link
Member

@mbohlool is this still an issue?

@fejta-bot
Copy link

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

@roycaihw
Copy link
Member
roycaihw commented Mar 8, 2018

@bgrant0607 The problem still exists because our openapi v2 spec cannot represent OneOf. Tracking openapi v3 support in #60937

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Indicates an issue on api area. area/apiserver area/swagger lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. priority/backlog Higher priority than priority/awaiting-more-evidence. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.
Projects
None yet
Development

No branches or pull requests

9 participants
0