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

add validate_config function for msk module #176

Merged
merged 1 commit into from
Apr 4, 2024

Conversation

Sharu95
Copy link

@Sharu95 Sharu95 commented Apr 4, 2024

What

I had issues connecting to our MSK cluster using the kafka-python-ng library and noticed that there was a call to the function validate_config() when using SASL_SSL, which was not implemented for the MSK module.

Similar to the other oauth modules, it is assumed in the conn.py code that there is a function that does the same check that is found in conn.py and added this.

Why

Connection issues to MSK

References

#170 and potentially also @mattoberle's implementation in https://github.com/mattoberle/kafka-python/tree/feature/2232-AWS_MSK_IAM

kafka/conn.py Show resolved Hide resolved
@Sharu95
Copy link
Author

Sharu95 commented Apr 4, 2024

@wbarnha, let me know what you think. With the current release, authenticating towards MSK using the AWS_MSK_IAM mechanism won't work, but I'm not even sure if it's properly tested for release or just silently released, as I saw no updated docs 😅

@wbarnha
Copy link
Owner

wbarnha commented Apr 4, 2024

@wbarnha, let me know what you think. With the current release, authenticating towards MSK using the AWS_MSK_IAM mechanism won't work, but I'm not even sure if it's properly tested for release or just silently released, as I saw no updated docs 😅

To be honest, I haven't had the chance to appropriately update documentation in my own GitHub Pages environment yet. Thank you for this PR, I took it in good faith that the original implementation was properly tested against an AWS MSK instance and there were no issues.

Copy link
Owner

@wbarnha wbarnha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, sorry for the inconvenience, let's get you a new release to fix this issue. 🙂

kafka/conn.py Show resolved Hide resolved
@wbarnha wbarnha merged commit 6756974 into wbarnha:master Apr 4, 2024
21 checks passed
@Sharu95
Copy link
Author

Sharu95 commented Apr 4, 2024

The original implementation seemed good, it was just this minor thing that was off! No worries, happy to contribute 🙌🏾 Thanks for fast approval and release, appreciate it 👌🏾

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