OpenAPI: Use more clear language in recommending error responses #12376
Conversation
| Credentials have timed out. If possible, the client should refresh credentials and retry. | ||
| This is an optional status response type that the REST Catalog can issue when the | ||
| token has expired. The client MAY request a new access token and retry the request. | ||
| 401 UnauthorizedResponse SHOULD be preferred over this response type on token expiry. |
There was a problem hiding this comment.
I like the new descriptions for both status codes. With the new changes, it seems we actually deprecated it without saying it explicitly. Should we just deprecate it then? cc @danielcweeks @rdblue
There was a problem hiding this comment.
Thank @flyrain !
Yeah I was actually debating on the last line, but decided to keep it to facilitate this discussion. I believe a deprecation would result in us removing the status code in a subsequent release, after which the status code handling could also be removed from the Iceberg REST client representations which I think may cause confusion as 419 responses from Iceberg REST Catalog servers that are running on older versions of the spec won't be handled as expected. Hence, I'm hesitant to suggest deprecating the status code.
I still think it's still fair to express a preference as we did here, so as to not confuse the REST Catalog server implementer on which status code to choose to use on token expiry.
There was a problem hiding this comment.
From my POV the "401 should be preferred" language is already effectively deprecating 419.
However, from the spec evolution perspective, I do not think it would be valid to remove 419 from the v1 REST spec.
All in all, I think the "should" language adds enough clarity for both clients and servers.
There was a problem hiding this comment.
may cause confusion as 419 responses from Iceberg REST Catalog servers that are running on older versions of the spec won't be handled as expected.
Can you elaborate it? I think the clients are OK to treat a 419 as a 401, or just a normal failure in that case. To retry or not is the client side decision. I believe either behavior(client retry or not retry) is acceptable. My only concern to deprecate it is the SigV4 use case @danielcweeks and @rdblue mentioned, if SigV4 clients must depend on 419, we need to keep it. But I don't have much context, will appreciate any inputs.
There was a problem hiding this comment.
To retry or not is the client side decision
Yes I agree with you @flyrain . Although I think it'll be confusing for application developers if for example, PyIceberg removed retry handling for 419 status on the next version after the 419 status code is deprecated on the REST Catalog. The application developer would expect 419 status codes from their REST Catalog server to result in retries, and on version upgrade they their PyIceberg application no longer will. Hence, as a community, we may need to keep 419 status code handling for backward compatibility.
There was a problem hiding this comment.
The application developer would expect 419 status codes from their REST Catalog server to result in retries
yeah, that's an edge case, in which a server happened to return 419, plus an application happened to rely on 419 for retrying. As we all agreed that 419-based retry is not reliable and not recommended, application shouldn't do that afterward. These are very subtle things though. I'm fine with not deprecate it, but I still prefer to give users more clear message, that 419 shouldn't be used. Deprecating it makes it more clear.
There was a problem hiding this comment.
Brought this up in last community catalog meeting. The general agreement was to deprecate 419. Meeting link: https://www.youtube.com/watch?v=vbV2XxOTyT4.
This is a follow up item from a recent discussion on the mailing list[1], where the community decided that 401 response should be preferred over 419 response on token expiry. [1] mailing list discussion: https://lists.apache.org/thread/443skhqr59j3fj0ovg4tyxh9d4f4gysc [2] REST Catalog Spec update PR: apache/iceberg#12376
| description: | ||
| Unauthorized. Authentication is required and has failed or has not yet been provided. | ||
| Unauthorized. The REST Catalog SHOULD respond with the 401 UnauthorizedResponse when | ||
| the access token provided is expired, revoked, malformed, or invalid for other reasons. |
There was a problem hiding this comment.
If an access token is malformed, wouldn't a 400 / BadRequestErrorResponse be a more appropriate response? I'm wondering if we should drop the malformed here?
There was a problem hiding this comment.
Hi @mrcnc thank you for the review!
IHMO I think if the access token is malformed, I'd still consider 401 to be the appropriate respones type vs 400 which I think would be more appropriate when the format of the Request itself is malformed.
I took this verbiage directly from https://www.rfc-editor.org/rfc/rfc6750 under the invalid_token section:
invalid_token
The access token provided is expired, revoked, malformed, or
invalid for other reasons. The resource SHOULD respond with
the HTTP 401 (Unauthorized) status code. The client MAY
request a new access token and retry the protected resource
request.
This is a follow up item from a recent discussion on the mailing list[1], where the community decided that 401 response should be preferred over 419 response on token expiry. [1] mailing list discussion: https://lists.apache.org/thread/443skhqr59j3fj0ovg4tyxh9d4f4gysc [2] REST Catalog Spec update PR: apache/iceberg#12376
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
Hi @sungwy, are you still working on it? |
|
Hi @sungwy, thanks for the reply. I'm OK with moving forward with this PR. We could deprecate it in a followup one. |
|
@flyrain : feel free to merge (from my POV) |
|
Merging it in :) |
This is a follow up item from a recent discussion on the mailing list[1], where the community decided that 401 response should be preferred over 419 response on token expiry. [1] mailing list discussion: https://lists.apache.org/thread/443skhqr59j3fj0ovg4tyxh9d4f4gysc [2] REST Catalog Spec update PR: apache/iceberg#12376
4 participants
The existence of the 419 AuthenticationTimeoutResponse status code caused confusion in the Iceberg community on the following two questions:
The updated description follows the RFC language of using SHOULD and MAY to be make these more clear for the REST catalog as well as the client responding to the status response.