KAFKA-18339: Remove raw unversioned direct SASL protocol (KIP-896) #18295
Conversation
github-actions
bot
added
triage
| // Raise an error prior to parsing if the api cannot be handled at this layer. This avoids | ||
| // unnecessary exposure to some of the more complex schema types. | ||
| if (apiKey != ApiKeys.API_VERSIONS && apiKey != ApiKeys.SASL_HANDSHAKE) | ||
| throw new IllegalSaslStateException("Unexpected Kafka request of type " + apiKey + " during SASL handshake."); | ||
| throw new InvalidRequestException("Unexpected Kafka request of type " + apiKey + " during SASL handshake."); |
There was a problem hiding this comment.
@rajinisivaram Do you know why we send IllegalSaslStateException here and InvalidRequestException for every other error? We seem to have special handling for Sasl related exceptions, but it doesn't seem to make sense in this case since we can't properly propagate an error if we get the wrong request type.
There was a problem hiding this comment.
change looks good to me for this case.
looks like we defined IllegalSaslStateException to indicate unexpected Kafka requests/state prior to SASL authentication.
https://github.com/apache/kafka/pull/3928/files#diff-fe3c04719e4a85673b22592a8bc286b427b960efc357b7331d77f0fabbed994e
There was a problem hiding this comment.
RIght, I think this distinction was perhaps useful when we had the pre KIP-43 fallback, but not anymore.
| // InvalidRequestException is thrown if the request is not in Kafka format or if the API key is invalid. | ||
| // If it's the initial request, this could be an ancient client (see method documentation for more details), | ||
| // a client configured with the wrong security protocol or a non kafka-client altogether (eg http client). | ||
| throw new InvalidRequestException("Invalid request, potential reasons: kafka client configured with the " + |
There was a problem hiding this comment.
Please take a look at the error message and check if it makes sense. This will only be captured in the broker, but still good to try and make it as informative as possible.
ijuma
force-pushed
the
kafka-18339-remove-raw-unversioned-sasl
branch
from
061cbb0 to
12d120c
Compare
|
@rajinisivaram @omkreddy Any of you has cycles to review this small change? |
ijuma
force-pushed
the
kafka-18339-remove-raw-unversioned-sasl
branch
from
12d120c to
b381c07
Compare
|
@rajinisivaram I'll go ahead and merge this so client developers can test their clients with all KIP-896 changes. Please take a look when you have a chance and I'm happy to submit a follow up to address any feedback. |
…18295) Clients that support SASL but don't implement KIP-43 (eg Kafka producer/consumer 0.9.0.x) will fail to connect after this change. Added unit tests and also manually tested with the console producer 0.9.0. While testing, I noticed that the logged message when a 0.9.0 Java client is used without sasl is slightly misleading - fixed that too. Reviewers: Manikumar Reddy <[email protected]>
| throw new InvalidRequestException(s"Received request api key ${header.apiKey} with version ${header.apiVersion} which is not enabled") | ||
| } else { | ||
| throw new UnsupportedVersionException(s"Received request api key ${header.apiKey} with version ${header.apiVersion} which is not supported") |
There was a problem hiding this comment.
Pardon me, this changes the returned exception from INVALID_REQUEST to UNSUPPORTED_VERSION as isApiVersionDeprecated always returns false for now. Is it expected?
There was a problem hiding this comment.
| @@ -1107,8 +1107,10 @@ private[kafka] class Processor( | |||
| val header = RequestHeader.parse(buffer) | |||
| if (apiVersionManager.isApiEnabled(header.apiKey, header.apiVersion)) { | |||
| header | |||
| } else { | |||
| } else if (header.isApiVersionDeprecated()) { | |||
There was a problem hiding this comment.
Should we add the deprecated versions back to the JSON files? This would enable us to provide more informative error messages to clients, helping them understand why certain APIs are marked as "unsupported" (deprecated, removed, or disabled).
There was a problem hiding this comment.
I don't think we need that, see the response to your other comment. Does that make sense?
There was a problem hiding this comment.
Does that make sense?
yes, that makes sense :)
A minor refactoring just before merging apache#18295 introduced a regression and no test failed. Throw the correct exception and add test to verify it. Also refactor the code slightly to make that possible.
A minor refactoring just before merging #18295 introduced a regression and no test failed. Throw the correct exception and add test to verify it. Also refactor the code slightly to make that possible. Thanks to Chia-Ping for catching the issue. Reviewers: Chia-Ping Tsai <[email protected]>
Clients that support SASL but don't implement KIP-43 (eg Kafka producer/consumer 0.9.0.x) will
fail to connect after this change.
Added unit tests and also manually tested with the console producer 0.9.0.
While testing, I noticed that the logged message when a 0.9.0 Java client is used without sasl is
slightly misleading - fixed that too.
Committer Checklist (excluded from commit message)