KAFKA-3652: Return error response for unsupported version of ApiVersionsRequest#1310
KAFKA-3652: Return error response for unsupported version of ApiVersionsRequest#1310rajinisivaram wants to merge 2 commits into
Conversation
rajinisivaram
changed the title
| try { | ||
| // For unsupported version of ApiVersionsRequest, create a dummy request to enable an error response to be returned later | ||
| if (header.apiKey == ApiKeys.API_VERSIONS.id && !Protocol.apiVersionSupported(header.apiKey, header.apiVersion)) | ||
| new ApiVersionsRequest |
There was a problem hiding this comment.
It may be better to just respond with the error from here instead of creating this dummy request. But let's see what @junrao says.
There was a problem hiding this comment.
Yes, the logic here is a bit awkward, but probably the simplest. Sending a response directly from SocketServer seems to require more work.
|
Thanks for the PR @rajinisivaram, I had a quick look and it seems fine apart from one comment. Let's wait for @junrao to take a look as well. |
|
|
||
| public enum SaslState { | ||
| HANDSHAKE_REQUEST, AUTHENTICATE, COMPLETE, FAILED | ||
| GSSAPI_OR_HANDSHAKE_REQUEST, HANDSHAKE_REQUEST, AUTHENTICATE, COMPLETE, FAILED |
There was a problem hiding this comment.
Hmm, I am not quite sure what the new state GSSAPI_OR_HANDSHAKE_REQUEST is for. It's making the same call as the HANDSHAKE_REQUEST and there is no code to change saslState to GSSAPI_OR_HANDSHAKE_REQUEST.
There was a problem hiding this comment.
@junrao Oops, sorry, missed out the change of initiate state. In HANDSHAKE_REQUEST state, any invalid request is treated as a GSSAPI token. This made sense when only a SaslHandshakeRequest was expected prior to SASL tokens. Since SaslHandshakeRequest can now be preceded by any number of ApiVersionsRequests, I think it makes sense to revert to 0.9.0.x GSSAPI authentication only for the first token. GSSAPI_OR_HANDSHAKE_REQUEST is the initiate state which checks that. Have updated the PR.
| case HANDSHAKE_REQUEST: | ||
| handleKafkaRequest(clientToken); | ||
| break; | ||
| case GSSAPI_OR_HANDSHAKE_REQUEST: |
There was a problem hiding this comment.
Could we move this case to the first one after switch since it's the initial state?
There was a problem hiding this comment.
@junrao We are relying on fall-through for the second case, so it's not straightforward to make that change.
|
@rajinisivaram : Thanks for the latest patch. LGTM. Just left a couple of minor comments. |
|
@ijuma : Yes, your comments make sense. The patch LGTM then. |
…onsRequest Handle unsupported version of ApiVersionsRequest during SASL auth as well as normal operation by returning an error response. Author: Rajini Sivaram <rajinisivaram@googlemail.com> Reviewers: Ismael Juma <ismael@juma.me.uk>, Jun Rao <junrao@gmail.com> Closes #1310 from rajinisivaram/KAFKA-3652 (cherry picked from commit 64451af) Signed-off-by: Jun Rao <junrao@gmail.com>
…onsRequest Handle unsupported version of ApiVersionsRequest during SASL auth as well as normal operation by returning an error response. Author: Rajini Sivaram <rajinisivaram@googlemail.com> Reviewers: Ismael Juma <ismael@juma.me.uk>, Jun Rao <junrao@gmail.com> Closes apache#1310 from rajinisivaram/KAFKA-3652
3 participants
Handle unsupported version of ApiVersionsRequest during SASL auth as well as normal operation by returning an error response.