pkg/apiserver: use correct HTTP status code for bad method #7155
Conversation
|
I think this change is safe, but would want someone from the API team to confirm. @bgrant0607 - Could you or someone in API LGTM it? Thank you. |
| w.WriteHeader(http.StatusForbidden) | ||
| fmt.Fprintf(w, "This is a read-only endpoint.") | ||
| w.Header().Set("Allow", "GET") | ||
| http.Error(w, "This is a read-only endpoint.", http.StatusMethodNotAllowed) |
There was a problem hiding this comment.
Should use NewMethodNotSupported:
https://github.com/GoogleCloudPlatform/kubernetes/blob/a927c239fe5845ce2b515b932f5d7dfa7a9c88d5/pkg/api/errors/errors.go#L197
We want to return Status json structs for all errors.
There was a problem hiding this comment.
I see a few other places in this file where we do the same thing (return a plain text response instead of a json status object). Feel free to update them all while you are it :)
Ref #1990
There was a problem hiding this comment.
|
So 403 was intentional in our early discussions, because of this I've in the past typically reserved 405 for "not implemented", but in this case, the 403 is because the user is accessing the core server via a path that only allows a subset of requests. Can you describe more why you think it's 405? |
|
That's a good point. It is an authorization policy rather than an unimplemented or invalid verb/path. We still should return structured error responses, though. |
|
This may be pointless if we rip out the readonly service, though. #4567 |
|
I'm persuaded by @smarterclayton explanation. No recent activity. Closing. |
405 is more accurate than 403 according to http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html.
But I am not sure if this breaks the existing API.