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 kafka auth using SASL with PLAIN mechanism #412

Merged
merged 2 commits into from
Oct 26, 2020

Conversation

jaysonsantos
Copy link
Contributor

@jaysonsantos jaysonsantos commented Oct 20, 2020

Description

This adds variables for authentication and enabling simple ssl without specifying certificates and others

Motivation and Context

This should enable to use kafka with providers like confluent which uses sasl_plain over ssl.

How Has This Been Tested?

I've tested it against our cluster which requires authentication with and ssl

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation. (I am not sure here, does the docs only point to config/env.go?)
  • I have updated the documentation accordingly. (Same as aboce)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@jaysonsantos jaysonsantos force-pushed the kafka-sasl-plain-support branch 2 times, most recently from 8457c2d to 73529b7 Compare October 20, 2020 19:12
@codecov-io
Copy link

codecov-io commented Oct 20, 2020

Codecov Report

Merging #412 into master will decrease coverage by 0.15%.
The diff coverage is 55.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #412      +/-   ##
==========================================
- Coverage   81.90%   81.75%   -0.16%     
==========================================
  Files          27       27              
  Lines        1603     1611       +8     
==========================================
+ Hits         1313     1317       +4     
- Misses        211      214       +3     
- Partials       79       80       +1     
Impacted Files Coverage Δ
pkg/handler/data_recorder_kafka.go 73.49% <50.00%> (-2.51%) ⬇️
pkg/handler/export.go 69.81% <100.00%> (ø)

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 3b50881...88b13a2. Read the comment docs.

Comment on lines 138 to 139
RecorderKafkaUsername string `env:"FLAGR_RECORDER_KAFKA_USERNAME" envDefault:""`
RecorderKafkaPassword string `env:"FLAGR_RECORDER_KAFKA_PASSWORD" envDefault:""`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can add a comment that FLAGR_RECORDER_KAFKA_USERNAME and FLAGR_RECORDER_KAFKA_PASSWORD will only work if FLAGR_RECORDER_KAFKA_SIMPLE_SSL=true

Copy link
Member

Choose a reason for hiding this comment

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

Should we add something to the docs or the comment is enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey there, according to confluent docs you can also use SASL with PLAINTEXT which does not require SSL.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add something to the docs or the comment is enough?

The doc directly shows the content of this env.go file, so the comment here is good enough.

Also the naming of the env, what do you think of FLAGR_RECORDER_KAFKA_SASL_USERNAME and FLAGR_RECORDER_KAFKA_SASL_PASSWORD? Just to make it clear that it's using SASL

@jaysonsantos
Copy link
Contributor Author

I've just rebased the code to force the tests to run as one of them got cancelled.

pkg/handler/data_recorder_kafka.go Outdated Show resolved Hide resolved
Comment on lines 138 to 139
RecorderKafkaUsername string `env:"FLAGR_RECORDER_KAFKA_USERNAME" envDefault:""`
RecorderKafkaPassword string `env:"FLAGR_RECORDER_KAFKA_PASSWORD" envDefault:""`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add something to the docs or the comment is enough?

The doc directly shows the content of this env.go file, so the comment here is good enough.

Also the naming of the env, what do you think of FLAGR_RECORDER_KAFKA_SASL_USERNAME and FLAGR_RECORDER_KAFKA_SASL_PASSWORD? Just to make it clear that it's using SASL

@jaysonsantos
Copy link
Contributor Author

@zhouzhuojie ready for review again.

@zhouzhuojie zhouzhuojie merged commit 98831a8 into openflagr:master Oct 26, 2020
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.

4 participants