Do not enable TopicAccessValidator if Kafka has unsupported authorizedOperations#2888
Conversation
NULL means the Kafka broker does not support authorizedOperations
| return new AuthorizationTopicAccessValidator(); | ||
| } | ||
|
|
||
| LOG.info("The Kafka broker has an authorization service enabled, but the Kafka " |
There was a problem hiding this comment.
I think we should log this at warning level. The resulting behavior is likely not what the user intended. Might be good to get Michael Drogalis (@MichaelDrogalis) take on what the behavior should be - maybe it would be better to just hard-fail and require the user to either upgrade Kafka or not configure the validator.
There was a problem hiding this comment.
The topic access validator is used mostly to provide better permission error messages when the KSQL user won't be able to execute a persistent query on KSQL. I don't think a warning message is a correct level for that case (I am not against about it, though). However, failing KSQL because is not using the correct version of Kafka might be out of scope of our current release. Do we have a compatibility list of Kafka versions that KSQL should support?
There was a problem hiding this comment.
Do we have a compatibility list of Kafka versions that KSQL should support?
Not that I'm aware of, no.
I agree with Sergio Peña (@spena) that it's probably a bit late to introduce a code-path to hard-fail here -- though switching the log level up to warn does feel like the right play to me. Sergio Peña (@spena) Can you expand a bit about why you think info is right here? Maybe I'm missing some nuance.
There was a problem hiding this comment.
I feel it's not a warn because if permission checks are not available, KSQL will still fail further in the code without providing good error messages. However, it is harmless to switch the level to warn now that I am thinking about it. I will do the switch and update this PR.
Victoria Xia (vcrfxia)
left a comment
Victoria Xia (vcrfxia)
left a comment
There was a problem hiding this comment.
Thanks Sergio Peña (@spena) -- LGTM.
| ); | ||
|
|
||
| return authorizedOperations.authorizedOperations().get() != null; | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
do we need to check for ClusterAuthorizationException like we do above?
There was a problem hiding this comment.
I don't think so. By the time this method is called, the TopicAccessValidatorFactory already checked if the user is authorized to access the cluster.
5 participants
Description
This PR avoids enabling the
TopicAccessValidatorif the Kafka broker version does not support theauthorizedOperationsAPI.The
authorizedOperationsAPI is a new method added in Kafka 2.3. If KSQL runs on Kafka 2.2 or lower, this API will return NULL causing a NPE in KSQL and denying any operation executed on a Kafka 2.2 environment.To avoid the NPE issue, KSQL avoids enabling the topic validator instead. Look at the
TopicAccessValidatorFactorywhich checks if theauthorizedOperationsfrom the Cluster is Null.See apache/kafka#6812
Testing done
Added unit tests.
Run local tests with Kafka 2.2. Warning message is printed:
[2019-05-29 14:14:32,512] WARN The Kafka broker has an authorization service enabled, but the Kafka version does not support authorizedOperations(). KSQL topic authorization checks will not be enabled. (io.confluent.ksql.engine.TopicAccessValidatorFactory:48)Reviewer checklist