Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 3 commits into from

Conversation

arenard
Copy link

@arenard arenard commented Nov 26, 2023

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

@StephenSorriaux
Copy link
Owner

StephenSorriaux commented Nov 28, 2023

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
Copy link
Author

arenard commented Nov 29, 2023

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 :

  • First we see errors related to DescribeUserScramCredentials*v0 and AlterUserScramCredentials*v0 in kafka_manager.py when using any kafka-admin modules in ansible 2.13 to 2.16 (not tested with other ansible versions). Schemas inside moved blocks are relying on some custom types definition into try/except block logic like CompactArray, CompactString, CompactBytes, TaggedFields.
    => Maybe we could import them from kafka-python for code outside of try/except block but as we have a custom one into try/except, it may be confusing to have both. I assume that is the reason why they are not currently imported from kafka-python.

  • Secondly, we see headers errors on kafka (at least on kraft enabled kafka 3.6.0 clusters) when using kafka_user(s) module to manage SCRAM users. It is related to FLEXIBLE_VERSION logic allowing to shift from RequestHeader to RequestHeaderV2 which sits into try/except too. Moving DescribeUserScramCredentials*v0 and AlterUserScramCredentials*v0 into try/except block solves it too.

@StephenSorriaux
Copy link
Owner

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:

  • your pip list of the Python virtual env that triggers the issues (so that I can check for the deps)
  • your version of Python (full version number)
  • a "simple" playbook that can trigger the issue
  • your Kafka version
    I can understand if you don't want to share it here publicly, so feel free to reach out by email if that's the case (see my profile/git commits).

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
Copy link
Owner

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
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants