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

Errors not Included in OpenAPI #69014

Open
lswith opened this Issue Sep 24, 2018 · 6 comments

Comments

Projects
None yet
3 participants
@lswith

lswith commented Sep 24, 2018

Is this a BUG REPORT or FEATURE REQUEST?:
/kind bug

What happened:
I am using the swagger api and it doesn't include the possible errors that can be returned from the api. I am generating a client, so the possible errors are necessary.

What you expected to happen:
Swagger allows all possible return codes to be used. These should be included in the swagger api.

Environment:

  • Kubernetes version (use kubectl version):
    Client Version: version.Info{Major:"1", Minor:"11", GitVersion:"v1.11.2", GitCommit:"bb9ffb1654d4a729bb4cec18ff088eacc153c239", GitTreeState:"clean", BuildDate:"2018-08-08T16:31:10Z", GoVersion:"go1.10.3", Compiler:"gc", Platform:"darwin/amd64"}
    Server Version: version.Info{Major:"1", Minor:"10", GitVersion:"v1.10.6", GitCommit:"a21fdbd78dde8f5447f5f6c331f7eb6f80bd684e", GitTreeState:"clean", BuildDate:"2018-07-26T10:04:08Z", GoVersion:"go1.9.3", Compiler:"gc", Platform:"linux/amd64"}
@lswith

This comment has been minimized.

Show comment
Hide comment
@lswith

lswith Sep 24, 2018

/sig api-machinery

lswith commented Sep 24, 2018

/sig api-machinery

@lswith

This comment has been minimized.

Show comment
Hide comment
@lswith

lswith commented Sep 24, 2018

@roycaihw

This comment has been minimized.

Show comment
Hide comment
@roycaihw

roycaihw Sep 25, 2018

Member

Hi Luke, could you be more specific on which APIs are having the problem and what errors are you expecting?

To my knowledge there was a documentation bug that our openapi spec didn't reflect the 202 path in API server delete handler. The bug was fixed in 1.12 #63418

There is also a known issue that our openapi v2 spec cannot express OneOf concept for our API that may return different object for same path (e.g. API server may respond with the delete object or Status on a DELETE 200 #59501)

And I'm assuming you're using the openapi spec. The swagger 1.2 specs are deprecated and meant to be removed

Member

roycaihw commented Sep 25, 2018

Hi Luke, could you be more specific on which APIs are having the problem and what errors are you expecting?

To my knowledge there was a documentation bug that our openapi spec didn't reflect the 202 path in API server delete handler. The bug was fixed in 1.12 #63418

There is also a known issue that our openapi v2 spec cannot express OneOf concept for our API that may return different object for same path (e.g. API server may respond with the delete object or Status on a DELETE 200 #59501)

And I'm assuming you're using the openapi spec. The swagger 1.2 specs are deprecated and meant to be removed

@lswith

This comment has been minimized.

Show comment
Hide comment
@lswith

lswith Sep 28, 2018

I think the convention here: https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#response-status-kind

Kubernetes will always return the Status kind from any API endpoint when an error occurs.

Should be coded into the openapi spec. This is so that it can be consumed programmatically.

lswith commented Sep 28, 2018

I think the convention here: https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#response-status-kind

Kubernetes will always return the Status kind from any API endpoint when an error occurs.

Should be coded into the openapi spec. This is so that it can be consumed programmatically.

@roycaihw

This comment has been minimized.

Show comment
Hide comment
@roycaihw

roycaihw Sep 28, 2018

Member

I see. There is a TODO in apiserver installer about adding errors to the route documentation

// TODO: Add status documentation using Returns()
// Errors (see api/errors/errors.go as well as go-restful router):
// http.StatusNotFound, http.StatusMethodNotAllowed,
// http.StatusUnsupportedMediaType, http.StatusNotAcceptable,
// http.StatusBadRequest, http.StatusUnauthorized, http.StatusForbidden,
// http.StatusRequestTimeout, http.StatusConflict, http.StatusPreconditionFailed,
// http.StatusUnprocessableEntity, http.StatusInternalServerError,
// http.StatusServiceUnavailable
// and api error codes
// Note that if we specify a versioned Status object here, we may need to
// create one for the tests, also
// Success:
// http.StatusOK, http.StatusCreated, http.StatusAccepted, http.StatusNoContent
//
// test/integration/auth_test.go is currently the most comprehensive status code test

Note that currently the openapi spec has the 401 Unauthorized path documented (no response schema), which is added in OpenAPIConfig.CommonResponses:

if _, exists := c.OpenAPIConfig.CommonResponses[http.StatusUnauthorized]; !exists {
c.OpenAPIConfig.CommonResponses[http.StatusUnauthorized] = spec.Response{
ResponseProps: spec.ResponseProps{
Description: "Unauthorized",
},
}
}

I'd like to work on the TODO. One concern is although I think it's right to document the possible paths, this change will increase the size of the openapi spec by a fair amount. Do we have any scalability/performance concern around that? @mbohlool (We may also check if CI gives any signal when a PR is submitted)

/assign

Member

roycaihw commented Sep 28, 2018

I see. There is a TODO in apiserver installer about adding errors to the route documentation

// TODO: Add status documentation using Returns()
// Errors (see api/errors/errors.go as well as go-restful router):
// http.StatusNotFound, http.StatusMethodNotAllowed,
// http.StatusUnsupportedMediaType, http.StatusNotAcceptable,
// http.StatusBadRequest, http.StatusUnauthorized, http.StatusForbidden,
// http.StatusRequestTimeout, http.StatusConflict, http.StatusPreconditionFailed,
// http.StatusUnprocessableEntity, http.StatusInternalServerError,
// http.StatusServiceUnavailable
// and api error codes
// Note that if we specify a versioned Status object here, we may need to
// create one for the tests, also
// Success:
// http.StatusOK, http.StatusCreated, http.StatusAccepted, http.StatusNoContent
//
// test/integration/auth_test.go is currently the most comprehensive status code test

Note that currently the openapi spec has the 401 Unauthorized path documented (no response schema), which is added in OpenAPIConfig.CommonResponses:

if _, exists := c.OpenAPIConfig.CommonResponses[http.StatusUnauthorized]; !exists {
c.OpenAPIConfig.CommonResponses[http.StatusUnauthorized] = spec.Response{
ResponseProps: spec.ResponseProps{
Description: "Unauthorized",
},
}
}

I'd like to work on the TODO. One concern is although I think it's right to document the possible paths, this change will increase the size of the openapi spec by a fair amount. Do we have any scalability/performance concern around that? @mbohlool (We may also check if CI gives any signal when a PR is submitted)

/assign

@roycaihw

This comment has been minimized.

Show comment
Hide comment
@roycaihw

roycaihw Sep 28, 2018

Member

Also the api convention doc didn't reflect the 202 Accepted code that apiserver may response with. I will try update the doc when I get a chance: https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#http-status-codes

Member

roycaihw commented Sep 28, 2018

Also the api convention doc didn't reflect the 202 Accepted code that apiserver may response with. I will try update the doc when I get a chance: https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#http-status-codes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment