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 sasl plain auth kafka plugin #1855

Conversation

somnathnitb
Copy link

Which problem is this PR solving?

Issue #1845 As of today Jaeger Kafka plugin only supports SASL GSSPI (Kerberos) and certificate-based authentication (mutual TLS). However, not all Kafka deployments support this authentication mechanism. we have a Kafka cluster deployed with TLS transport for communication however our authentication is set up to use SASL PLAIN. https://docs.confluent.io/current/kafka/authentication_sasl/authentication_sasl_plain.html#kafka-sasl-auth-plain

Short description of the changes

Added new authentication configuration for SASL Plain.

kafka.producer.authentication = "plain"

New SASL Plain configuration options

kafka.producer.sasl.plain.username = "username"
kafka.producer.sasl.plain.password = "password"

…kend (jaegertracing#1845)

* Add SASL Plain configuration options and SASL Plain configuration set up

Signed-off-by: somnath <somnath.ghosh@honeywell.com>
…kend (jaegertracing#1845)

* Update config paramaters for sasl plain input to have sasl plain prefix

Signed-off-by: somnath <somnath.ghosh@honeywell.com>
…kend (jaegertracing#1845)

* refactor SASL Plain configuration to check for valid username/password before enabling configuration

Signed-off-by: somnath <somnath.ghosh@honeywell.com>
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

  • run make fmt
  • there are no tests

pkg/kafka/auth/config.go Show resolved Hide resolved
func addSASLPlainFlags(configPrefix string, flagSet *flag.FlagSet) {
flagSet.String(
configPrefix+saslPlainPrefix+suffixSASLUsername,
defaultSASLUsername,
Copy link
Member

Choose a reason for hiding this comment

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

""

"Username for SASL Plain authentication")
flagSet.String(
configPrefix+saslPlainPrefix+suffixSASLPassword,
defaultSASLPassword,
Copy link
Member

Choose a reason for hiding this comment

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

""

…kend (jaegertracing#1845)

* add option to enable/disable TLS for transport. TLS can be enabled/disabled irrespective of authentication mechanism.

Signed-off-by: somnath <somnath.ghosh@honeywell.com>
@@ -39,6 +39,7 @@ var authTypes = []string{
// AuthenticationConfig describes the configuration properties needed authenticate with kafka cluster
type AuthenticationConfig struct {
Authentication string
TlsEnabled bool
Copy link
Member

Choose a reason for hiding this comment

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

we should not need this once #1838 is finished

Copy link
Author

Choose a reason for hiding this comment

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

makes sense. Reverted the change. I looked at the PR #1838 looks good. However, will take a closer look over the weekend.

…kend (jaegertracing#1845)

* Revert Support for TLS as it will handled with issue jaegertracing#1838

Signed-off-by: somnath <somnath.ghosh@honeywell.com>
@yurishkuro
Copy link
Member

@somnathnitb do you have cycles to finish this PR?

@pavolloffay
Copy link
Member

Closing thanks for the PR. Jaeger already supports plain auth for Kafka.

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