swenzel commented 5 years ago

This pull request adds support for modular SASL authentication mechanisms with two mechanisms (PLAIN, SCRAM) already implemented.

closes #651

codecov-io commented 5 years ago

Codecov Report

Merging #972 into master will decrease coverage by 1.54%. The diff coverage is 86.27%.

@@            Coverage Diff             @@
##           master     #972      +/-   ##
==========================================
- Coverage   83.53%   81.98%   -1.55%     
==========================================
  Files          36       37       +1     
  Lines        3807     4468     +661     
  Branches      562      682     +120     
==========================================
+ Hits         3180     3663     +483     
- Misses        483      565      +82     
- Partials      144      240      +96

Impacted Files	Coverage Δ
pykafka/client.py	`88.46% <ø> (ø)`	:arrow_up:
pykafka/exceptions.py	`100% <100%> (ø)`	:arrow_up:
pykafka/cluster.py	`70.9% <100%> (+1.2%)`	:arrow_up:
pykafka/rdkafka/simple_consumer.py	`87.82% <100%> (-1.56%)`	:arrow_down:
pykafka/connection.py	`89.18% <100%> (+6.02%)`	:arrow_up:
pykafka/rdkafka/producer.py	`91.22% <100%> (-3.32%)`	:arrow_down:
pykafka/protocol/__init__.py	`100% <100%> (ø)`	:arrow_up:
pykafka/broker.py	`92.22% <71.42%> (-1.42%)`	:arrow_down:
pykafka/protocol/sasl.py	`84.93% <84.93%> (ø)`
pykafka/sasl_authenticators.py	`85.03% <85.03%> (ø)`
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e7665bf...73f8e8e. Read the comment docs.

swenzel commented 5 years ago

Ditched kafka 0.8 because there were too many issues getting it to run. It neither supports SSL nor SASL, the configs differ significantly and there seems to be an issue with its zk client. I thought it's more useful to test kafka 2 instead.

Switched to librdkafka 0.11.5 because 0.9.5 doesn't support the SCRAM mechanisms. In the end I didn't choose a 1.x version since it introduces another configuration issue that I didn't want to solve.

I went far out of scope fixing several issues with CI and the tests which took me a lot longer than actually implementing the feature.
I've really put a lot of work into it but now is the point where I say it's enough. I'm open to refactoring certain things or to revert a few changes if you don't want them. But I won't continue fixing CI or other tests that do not belong to the code that I changed. Especially if it's for keeping python 2 or kafka 0 compatibility.

hfjn commented 4 years ago

Could someone please just take a look at this PR? I think @swenzel has done some great work here which really pushes pykafka forward at should be a high priority to work on.

garrettthomaskth commented 4 years ago

Hi, I would also like to see this merged! :D

Neustradamus commented 4 years ago

Any news?

DoZX commented 4 years ago

Look forward to this feature.

Parsely / pykafka

Feature SASL SCRAM support #972

Codecov Report