Support SASL authentication in AIOKafkaAdminClient

[selevit]

Changes

Fixes #889

Added the following keyword arguments into AioKafkaAdminClient:

1. sasl_mechanism.
2. sasl_plain_username
3. sasl_plain_password
4. sasl_kerberos_service_name
5. sasl_kerberos_domain_name
6. sasl_oauth_token_provider

[ods]

It would be nice to cover them with tests.

[codecov]

Codecov Report

Merging #890 (6e0a0af) into master (34b2d19) will decrease coverage by 8.90%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #890      +/-   ##
==========================================
- Coverage   97.61%   88.71%   -8.90%     
==========================================
  Files          30       30              
  Lines        5451     5451              
==========================================
- Hits         5321     4836     -485     
- Misses        130      615     +485     

Flag	Coverage Δ
cext	79.65% <ø> (-8.76%)	⬇️
integration	79.61% <ø> (-17.97%)	⬇️
purepy	37.64% <ø> (-59.52%)	⬇️
unit	38.10% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiokafka/admin.py	18.69% <ø> (-69.63%)	⬇️

... and 13 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[selevit]

It would be nice to cover them with tests.

@ods, TBH it's not quite clear, why the coverage is decreased so much. Especially taking into account the there shouldn't be any coverage loss because of this change - parameters are just passed to AioKafkaClient as is.

I agree that would be nice to cover the SASL auth by tests in general. But it implies configuring a separate kafka instance with those settings, and IMO this should have been done when adding this SASL functionality into the main client.

Regarding this particular change, I just proxy the same parameters into AioKafkaClient and that's it. This code should be executed it tests, because there are tests that create AioKafkaAdminClient class, and parameters should be passed with their default values. Yes, the SASL functionality itself is not covered, but is it actually covered at all for the generic client?

Let me know, what kind of tests would you like to see here, and I'll try to implement them, if it's not something very time consuming.

UPD: I see why coverage is decreased, because of a different bug that seems to be fixed already in master, right? Some existing tests just didn't run because of an environment error related to docker.

[ods]

Sure, we can introduce such simple changes with just manual testing. But producer/consumer have the tests for some of SASL mechanisms in tests/test_sasl.py. Doesn't the same approach work for admin client?

[selevit]

Oh, thanks for pointing them out.

I have added a first testcase into test_sasl.py: test_admin_client_sasl_plaintext_basic. But when running this, I get an error like this:

kafka.errors.IncompatibleBrokerVersion: IncompatibleBrokerVersion: Kafka broker does not support the 'MetadataRequest_v0' Kafka protocol.

How to reproduce:

pytest --log-cli-level=INFO -s --docker-image aiolibs/kafka:2.13_2.8.1 tests/test_sasl.py -k test_admin_client_sasl_plaintext_basic

I noticed that there was a similar bug in python-kafka, but it's fixed in 2.0.2 already.

Any quick idea about the reason? Exactly the same calls work in test_admin.py but with non-sasl brokers.