Add basic authentication to Kafka storage #1983

chandresh-pancholi · 2019-12-18T21:18:44Z

Configure Jager Collector to work with Kafka With Basic Authentication

Resolves #1966

Signed-off-by: chandresh-pancholi [email protected]

Which problem is this PR solving?

Configure Jager Collector to work with Kafka With Basic Authentication

Short description of the changes

Adding plaintext Kafka basic authentication

…jaegertracing#1966 Signed-off-by: chandresh-pancholi <[email protected]>

codecov · 2019-12-18T21:39:51Z

Codecov Report

Merging #1983 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1983      +/-   ##
==========================================
+ Coverage   96.94%   96.96%   +0.01%     
==========================================
  Files         202      202              
  Lines       10068    10068              
==========================================
+ Hits         9760     9762       +2     
+ Misses        269      267       -2     
  Partials       39       39

Impacted Files	Coverage Δ
cmd/collector/app/builder/span_handler_builder.go	`100% <100%> (ø)`	⬆️
plugin/storage/badger/spanstore/reader.go	`96.79% <0%> (ø)`	⬆️
cmd/query/app/static_handler.go	`88.59% <0%> (+1.75%)`	⬆️

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 6531292...73732d1. Read the comment docs.

awesomeinsight

Hi Chandresh-Pancholi! I was very happy when I saw your pull request regarding kafka basic auth. I want to connect a Kafka enabled Azure Event Hub to jaeger-ingest and jaeger-collector. Unfortunatly there is currently no support for e.g. these properties:

sasl.mechanism=PLAIN
security.protocol=SASL_SSL
sasl.jaas.config=org.apache.kafka.common.security.plain.PlainLoginModule required
username="$ConnectionString"
password="{MY.EVENTHUBS.CONNECTION.STRING}";

Examples for Azure Event Hubs for Kafka connections: https://github.com/Azure/azure-event-hubs-for-kafka/tree/master/tutorials

It would be nice if you could add that to your PR too cause this would enable a lot of people that are using Azure EH (kafka enabled) to connect jaeger to their solutions. Does it make sense for you?

pavolloffay · 2019-12-19T14:01:14Z

pkg/kafka/auth/options.go

+	flagSet.String(
+		configPrefix+plainTextPrefix+suffixPlainTextUserName,
+		defaultPlainTextUserName,
+		"The plaintext Username for SASL/PLAIN authentication")


shouldn't we call it basic like in other flags?

Kafka and Sarama always mentioned it as Plaintext. Shall I change it to basic?
I can replace "plainText" to "basicAuth"

I'd keep it as plaintext to match Kafka documentation.

chandresh-pancholi · 2019-12-19T19:19:16Z

Hi Chandresh-Pancholi! I was very happy when I saw your pull request regarding kafka basic auth. I want to connect a Kafka enabled Azure Event Hub to jaeger-ingest and jaeger-collector. Unfortunatly there is currently no support for e.g. these properties:

sasl.mechanism=PLAIN
security.protocol=SASL_SSL
sasl.jaas.config=org.apache.kafka.common.security.plain.PlainLoginModule required
username="$ConnectionString"
password="{MY.EVENTHUBS.CONNECTION.STRING}";

Examples for Azure Event Hubs for Kafka connections: https://github.com/Azure/azure-event-hubs-for-kafka/tree/master/tutorials

It would be nice if you could add that to your PR too cause this would enable a lot of people that are using Azure EH (kafka enabled) to connect jaeger to their solutions. Does it make sense for you?

I never used Azure Event bus. Let me spend some time over the weekend. It would be great if you can create another issue specific to Azure Event bus. Let these changes go live for now.

awesomeinsight · 2019-12-20T13:14:27Z

Hi Chandresh-Pancholi! I was very happy when I saw your pull request regarding kafka basic auth. I want to connect a Kafka enabled Azure Event Hub to jaeger-ingest and jaeger-collector. Unfortunatly there is currently no support for e.g. these properties:
sasl.mechanism=PLAIN
security.protocol=SASL_SSL
sasl.jaas.config=org.apache.kafka.common.security.plain.PlainLoginModule required
username="$ConnectionString"
password="{MY.EVENTHUBS.CONNECTION.STRING}";
Examples for Azure Event Hubs for Kafka connections: https://github.com/Azure/azure-event-hubs-for-kafka/tree/master/tutorials
It would be nice if you could add that to your PR too cause this would enable a lot of people that are using Azure EH (kafka enabled) to connect jaeger to their solutions. Does it make sense for you?

I never used Azure Event bus. Let me spend some time over the weekend. It would be great if you can create another issue specific to Azure Event bus. Let these changes go live for now.

Hi chandresh-pancholi! Thanks for your quick response. As requested I created a separate issue for it: #1989. I also referenced this one here. Regards and Merry X-Mas :-)

yurishkuro · 2019-12-22T02:42:35Z

pkg/kafka/auth/options.go

@@ -99,6 +106,17 @@ func addTLSFlags(configPrefix string, flagSet *flag.FlagSet) {
 		"Path to the TLS Key for the Kafka connection")
 }

+func addPlainTextFlag(configPrefix string, flagSet *flag.FlagSet) {


Suggested change

func addPlainTextFlag(configPrefix string, flagSet *flag.FlagSet) {

func addPlainTextFlags(configPrefix string, flagSet *flag.FlagSet) {

yurishkuro · 2019-12-22T02:44:17Z

pkg/kafka/auth/plaintext.go

+func setPlainTextConfiguration(config *PlainTextConfig, saramaConfig *sarama.Config) {
+	saramaConfig.Net.SASL.Enable = true
+	saramaConfig.Net.SASL.User = config.UserName
+	saramaConfig.Net.SASL.Password = config.Password


this doesn't track. Kafka docs https://docs.confluent.io/current/kafka/authentication_sasl/authentication_sasl_plain.html mention "plaintext" as an alternative to SASL "plain", but here we seem to make them the same.

Please correct me if I understood it, You are trying to say rather than using "PlainText" we shall use "Plain".

no, I am saying Kafka docs make clear distinction between those two methods, but in this code we seem to treat them as the same thing: calling it plaintext but using SASL structure to pass it. So which one is it?

saramaconfig only has SASL struct. below is the code snippet.

// SASL based authentication with broker. While there are multiple SASL authentication methods // the current implementation is limited to plaintext (SASL/PLAIN) authentication SASL struct { // Whether or not to use SASL authentication when connecting to the broker // (defaults to false). Enable bool // SASLMechanism is the name of the enabled SASL mechanism. // Possible values: OAUTHBEARER, PLAIN (defaults to PLAIN). Mechanism SASLMechanism // Whether or not to send the Kafka SASL handshake first if enabled // (defaults to true). You should only set this to false if you're using // a non-Kafka SASL proxy. Handshake bool //username and password for SASL/PLAIN or SASL/SCRAM authentication User string Password string

well, then this is "plain", not "plaintext", because credentials are still sent over encrypted channel. From the docs:

PLAIN versus PLAINTEXT Do not confuse the SASL mechanism PLAIN with no SSL encryption being called PLAINTEXT.

From Confluent slack channel:
None of the SASL mechanisms encrypt wire data not even GSSAPI. You have to enable TLS for that.

https://docs.confluent.io/current/kafka/authentication_ssl.html

By default, Apache Kafka® communicates in PLAINTEXT, which means that all data is sent in the clear. To encrypt communication, it is recommended to configure all the Confluent Platform components in your deployment to use SSL encryption.

…rtracing#1966 Signed-off-by: chandresh-pancholi <[email protected]>

Configure Jager Collector to work with Kafka With Basic Authentication …

43e8f5d

…jaegertracing#1966 Signed-off-by: chandresh-pancholi <[email protected]>

chandresh-pancholi requested review from black-adder, jpkrohling, objectiser, pavolloffay, tiffon, vprithvi and yurishkuro as code owners December 18, 2019 21:18

awesomeinsight reviewed Dec 19, 2019

View reviewed changes

pavolloffay reviewed Dec 19, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into jaeger-1966

9abe026

yurishkuro reviewed Dec 22, 2019

View reviewed changes

chandresh-pancholi added 2 commits December 23, 2019 13:52

Changing method name from addPlainTextFlag to addPlainTextFlags jaege…

6531292

…rtracing#1966 Signed-off-by: chandresh-pancholi <[email protected]>

Merge remote-tracking branch 'upstream/master' into jaeger-1966

73732d1

yurishkuro merged commit 775b1d1 into jaegertracing:master Dec 31, 2019

pavolloffay changed the title ~~Configure Jager Collector to work with Kafka With Basic Authenticatio…~~ Add basic authentication to Kafka storage Jan 3, 2020

pavolloffay added this to the Release 1.17 milestone Jan 3, 2020

pavolloffay added the storage/kafka label Jan 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic authentication to Kafka storage #1983

Add basic authentication to Kafka storage #1983

chandresh-pancholi commented Dec 18, 2019 •

edited by pavolloffay

Loading

codecov bot commented Dec 18, 2019 •

edited

Loading

awesomeinsight left a comment

pavolloffay Dec 19, 2019

chandresh-pancholi Dec 19, 2019

yurishkuro Dec 22, 2019

chandresh-pancholi commented Dec 19, 2019

awesomeinsight commented Dec 20, 2019

yurishkuro Dec 22, 2019

chandresh-pancholi Dec 23, 2019

yurishkuro Dec 22, 2019

chandresh-pancholi Dec 22, 2019

yurishkuro Dec 22, 2019

chandresh-pancholi Dec 23, 2019

yurishkuro Dec 24, 2019

chandresh-pancholi Dec 30, 2019

chandresh-pancholi Dec 30, 2019

	func addPlainTextFlag(configPrefix string, flagSet *flag.FlagSet) {
	func addPlainTextFlags(configPrefix string, flagSet *flag.FlagSet) {

Add basic authentication to Kafka storage #1983

Add basic authentication to Kafka storage #1983

Conversation

chandresh-pancholi commented Dec 18, 2019 • edited by pavolloffay Loading

Which problem is this PR solving?

Short description of the changes

codecov bot commented Dec 18, 2019 • edited Loading

Codecov Report

awesomeinsight left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chandresh-pancholi commented Dec 19, 2019

awesomeinsight commented Dec 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chandresh-pancholi commented Dec 18, 2019 •

edited by pavolloffay

Loading

codecov bot commented Dec 18, 2019 •

edited

Loading