-
Notifications
You must be signed in to change notification settings - Fork 9
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
Support MSK IAM Authentication #74
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have one question, LGTM otherwise.
@mhaseebmlk @sofyat are there any special considerations for deploying a new version with these changes? Can I just merge to |
@danielpops I think you might want to update the version here (https://github.com/Yelp/kafka-python/blob/master/kafka/version.py) to |
@mhaseebmlk is correct, we need to push the tag and then manually push the package. However, one of our longer term plans is to not use kafka-python for producing / consuming, but rather use confluent-kafka (a librdkafka wrapper) directly. Only use yelp-kafka (or a similar tool if we want to completely replace it) for discovery of kafka brokers as a utilities library but do the kafka-related stuff from confluent-kafka. Reason is that MSK uses Kafka versions 2.8+ which is not explicitly supported by kafka-python (+ it is no longer maintained). Having said that, it seems that there's a pushback to not release MSK IAM authentication for librdkafka (see comment). We might need to use a fork that supports it |
@gkousouris thanks!
FYI AWS has recently released support for the Having said that, since afaict a large majority of Yelp use cases are using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good!
Feature or Description
Roughly adapted from this upstream PR that was never merged.
Changes from that PR:
AWS_DEFAULT_REGION
which may not always be setboto_session.get_credentials()
to ensure that role credentials that can be automatically refreshed once they expire (from e.g. instance profile, container pod identity, boto profile that do role assumption, etc) will be automatically refreshedSimpleClient
so fields likesecurity_protocol
andsasl_mechanism
can be passed through to theBrokerConnection
Caveats
kafka.errors.NoBrokersAvailable: NoBrokersAvailable
error is raised. Basically the failures to authenticate don't bubble up correctly and it just gets treated as if all brokers timed out or otherwise couldn't be connected. I think this can probably be fixed, but my initial attempt to fix it with minimal changes to the overall control flow was unsuccessfulTesting Done
KafkaProducer and KafkaConsumer
Producer
Consumer
SimpleClient Producer and Consumer
Producer
Consumer
Admin Client with KafkaClient
Create Topic