-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add sasl plain auth kafka plugin #1855
Conversation
…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>
There was a problem hiding this 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
func addSASLPlainFlags(configPrefix string, flagSet *flag.FlagSet) { | ||
flagSet.String( | ||
configPrefix+saslPlainPrefix+suffixSASLUsername, | ||
defaultSASLUsername, |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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>
pkg/kafka/auth/config.go
Outdated
@@ -39,6 +39,7 @@ var authTypes = []string{ | |||
// AuthenticationConfig describes the configuration properties needed authenticate with kafka cluster | |||
type AuthenticationConfig struct { | |||
Authentication string | |||
TlsEnabled bool |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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>
@somnathnitb do you have cycles to finish this PR? |
Closing thanks for the PR. Jaeger already supports plain auth for Kafka. |
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.
New SASL Plain configuration options