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 support for SASL_PLAINTEXT authentication with Kafka broker #3056

Merged
merged 3 commits into from
Feb 18, 2019
Merged

Add support for SASL_PLAINTEXT authentication with Kafka broker #3056

merged 3 commits into from
Feb 18, 2019

Conversation

PHameete
Copy link
Contributor

@PHameete PHameete commented Jan 30, 2019

What does this PR do?

Add support for SASL Plaintext authentication with the Kafka broker to the kafka_consumer integration. This uses properties already available on the used KafkaClient.

Motivation

It was currently only possible to use PLAINTEXT or SSL. This makes it possible to also connect to a Kafka broker using SASL_PLAINTEXT. The KafkaClient does not support SASL_SSL yet unfortunately.

Review checklist

  • [] PR has a meaningful title or PR has the no-changelog label attached
  • Feature or bugfix has tests
  • Git history is clean
  • If PR impacts documentation, docs team has been notified or an issue has been opened on the documentation repo
  • If PR adds a configuration option, it has been added to the configuration file.

@PHameete PHameete requested a review from a team as a code owner January 30, 2019 15:31
@dabcoder
Copy link
Contributor

dabcoder commented Feb 1, 2019

@PHameete Thanks for the PR. Tests are failing though, this is flake8 update related - recent updates (https://pypi.org/project/flake8/#history). Can you merge the master branch into yours, which should allow those tests to pass?

@codecov-io
Copy link

Codecov Report

Merging #3056 into master will decrease coverage by 5.37%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3056      +/-   ##
==========================================
- Coverage   84.03%   78.65%   -5.38%     
==========================================
  Files         648        8     -640     
  Lines       36033      520   -35513     
  Branches     4352      110    -4242     
==========================================
- Hits        30279      409   -29870     
+ Misses       4482       79    -4403     
+ Partials     1272       32    -1240

@PHameete
Copy link
Contributor Author

PHameete commented Feb 4, 2019

@dabcoder ty for getting back to me. I did the merge as requested.

@gzussa
Copy link
Contributor

gzussa commented Feb 4, 2019

@PHameete Thanks a lot for this contribution. 💯
This is a good catch!

After looking at the client doc, it seems that we are missing quite a lot of config in there.
Would you be able to add them all?
Also, are sasl_* params global to all *_connect_str or do you think they should be specific?
ssl_* config seems to be global so my guess is that sasl params should be the same but you may know better.

Also, would you mind to format the config file based on this convention. @l0k0ms should be able to assist you there.

@PHameete
Copy link
Contributor Author

PHameete commented Feb 5, 2019

@gzussa I've added all authentication parameters (SSL, SASL, Kerberos) that were missing on the datadog integration side, and example config according to the example you provided. It seems a bit overkill to add all the timeouts and buffer sizes.

Regarding the sasl: I dont think they are global.

@ofek ofek changed the title [kafka_consumer] Add support for SASL_PLAINTEXT authentication with Kafka broker Add support for SASL_PLAINTEXT authentication with Kafka broker Feb 5, 2019
@dabcoder
Copy link
Contributor

@PHameete Thanks, and apologies for the delay, we are still looking into the codecov/project failure there, we'll get back to you as soon as possible.

@PHameete
Copy link
Contributor Author

@dabcoder not a problem, let me know if you need anything!

Copy link
Contributor

@l0k0ms l0k0ms left a comment

Choose a reason for hiding this comment

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

Plop, many thanks for the PR. 🙇
Note for later @DataDog/baklava will need to do a refactoring of the configuration file once this PR is merged, there a lots of nits to update. Creating a card on our internal board to keep this under our radar.

Copy link
Contributor

@gzussa gzussa left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for your contribution 💯

@gzussa gzussa merged commit 084be36 into DataDog:master Feb 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants