StephenSorriaux / ansible-kafka-admin

Manage your topic's configuration (partitions, replication factor, parameters), ACLs, quotas, users and get stats, without any effort with this library. It does not use the Kafka scripts and does not require ssh connection to the remote broker.
Apache License 2.0
150 stars 46 forks source link

Put SCRAM methods under try/except in kafka_protocol.py #160

Closed arenard closed 11 months ago

arenard commented 1 year ago

Looks like an indentation error when SCRAM methods where added to kafka_protocol.py

StephenSorriaux commented 12 months ago

Hello,

Thank you for the PR.

Is this supposed to fix any issue? As far as I can tell, and as the comment is suggesting, the try/except logic is there to implement some Kafka protocol requests/responses that would be available in the kafka-python lib when a release is cut. In the meantime (which has been 3years+, but with the latest changes on the kafka-python project, there is still hope), and to avoid asking our users to pin the kafka-python lib version to a git commit, we decided to get it back in this project. On the other hand, the DescribeUserScramCredentialsResponse_v0 and others implementations are our own, and not part of the kafka-python lib, hence the reason why they are not part of the try/except logic.

arenard commented 12 months ago

Hello,

Yes I see the point and effectively, it seems there is a new hope on kafka-python side. However it seems that the project while revived is still not releasing new versions for now 🤞.

Of course it tries to fix some issues we identified 🙂.

From what I've seen there is curently two problems :

StephenSorriaux commented 12 months ago

Thanks for the context and detailed explanations. I must say, I am intrigued. Using Python 3.11 and 3.9 I can't seem to find a test case that triggers the error.

I would expect the 1. issue to trigger when using a version of kafka-python that does not raise the ImportError exception, hence not triggering the except block and not defining the required function and class for the SCRAM related requests and responses. The 2. issue is weird, I can't see how it can happen.

Would you be willing to share a reproducible test case with me? I would be interested in:

In any case, after re-reading it, we should probably simplify this kafka_protocol module by not trying to use anything from kafka-python that is not yet available and just define everything we need. Even if the classes were to be available in kafka-python other things will break anyway... Can you please change this PR so that we get rid of the try/except block?

StephenSorriaux commented 11 months ago

I opened #163 where I removed the logic around using stuff from kafka-python if it is there. Can you please try it and let met know whether if fixes your issue or not?

StephenSorriaux commented 11 months ago

I merged #163 and will now close this one, please let me know if it does not fix your issue.