Kafka kerberos authentication support for collector/ingester #1589

rubenvp8510 · 2019-06-05T23:05:49Z

Signed-off-by: Ruben Vargas ruben.vp8510@gmail.com

Which problem is this PR solving?

Add support for Kerberos authentication to ingester/collector
Fixes Using a kerberos secured Kafka as buffer between Jaeger and Elasticsearch #1188

Short description of the changes

Now that sarama library has support for Kerberos authentication, this PR add support for it on the jaeger side using flags.

codecov · 2019-06-06T02:49:40Z

Codecov Report

Merging #1589 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1589      +/-   ##
==========================================
+ Coverage   98.72%   98.74%   +0.02%     
==========================================
  Files         191      191              
  Lines        9182     9190       +8     
==========================================
+ Hits         9065     9075      +10     
+ Misses         91       89       -2     
  Partials       26       26

Impacted Files	Coverage Δ
cmd/ingester/app/flags.go	`100% <100%> (ø)`	⬆️
plugin/storage/kafka/options.go	`100% <100%> (ø)`	⬆️
...lugin/sampling/strategystore/adaptive/processor.go	`100% <0%> (+0.78%)`	⬆️

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 5b52726...fa0e5ad. Read the comment docs.

rubenvp8510 · 2019-06-06T05:30:56Z

I just need to test this.

objectiser

Just left some initial comments, as will be on PTO next week.

objectiser · 2019-06-06T08:31:45Z

cmd/ingester/app/flags.go

+	// KerberosPrefix for Kerberos configuration options
+	KerberosPrefix = ".kerberos"
+	// SuffixKerberosServiceName is the suffix for Kerberos service name
+	SuffixKerberosServiceName = ".serviceName"


Instead of camel case, I think most flags use hyphens (e.g. service-name). Same applies to useKeyTab and kerberosConfig.

objectiser · 2019-06-06T08:33:02Z

cmd/ingester/app/flags.go

+	// SuffixKerberosPassword is Kerberos password
+	SuffixKerberosPassword = ".password"
+	// SuffixKerberosConfig is path to the kerberos configuration file.
+	SuffixKerberosConfig = ".kerberosConfig"


This option will already have a .kerberos prefix - so the option itself could just be named .config

objectiser · 2019-06-06T08:33:43Z

pkg/kafka/consumer/config.go

+	UseKeyTab          bool
+	Username           string
+	Password           string
+	KerberosConfigPath string


nit: This could be just ConfigPath

objectiser · 2019-06-06T08:34:19Z

plugin/storage/kafka/options.go

+	suffixAuthentication = ".authentication"
+	// Kerberos configuration options
+	kerberosPrefix             = ".kerberos"
+	suffixKerberosServiceName  = ".serviceName"


Same comments as for the ingester, related to the option names.

pavolloffay · 2019-06-06T08:44:48Z

Gopkg.toml

@@ -150,7 +150,7 @@ required = [

 [[constraint]]
  name = "github.com/Shopify/sarama"
-  version = "1.20.1"


Is there a way to depend on a version?

Hopefully a new release should be coming soon: IBM/sarama#1366 (comment)

This is just temporal, waiting for a new release of sarama.

@pavolloffay if we don't get a sarama release soon, are we ok to release with the SHA?

pavolloffay · 2019-06-06T08:57:49Z

plugin/storage/kafka/options.go

+	suffixBrokers        = ".brokers"
+	suffixTopic          = ".topic"
+	suffixEncoding       = ".encoding"
+	suffixAuthentication = ".authentication"


Are there more authentication mechanisms? Is so maybe we should rather have .kerberos=true

Or .authentication=kerberos

for now, there is only one authentication mechanism which is in this case Kerberos, but Kafka (and Sarama) support others, I would say that @objectiser suggestion is the way to go, just in case that in the future we wanted more mechanisms.

rubenvp8510 · 2019-06-07T18:22:42Z

I've already tested this with a kafka cluster configured with Kerberos. this PR is still using a commit hash, waiting for a new release of Sarama.

objectiser · 2019-06-19T15:23:53Z

Gopkg.toml

@@ -150,7 +150,7 @@ required = [

 [[constraint]]
  name = "github.com/Shopify/sarama"
-  version = "1.20.1"


@pavolloffay if we don't get a sarama release soon, are we ok to release with the SHA?

objectiser · 2019-06-19T15:24:32Z

cmd/ingester/app/flags.go

+	flagSet.String(
+		KafkaConsumerConfigPrefix+SuffixAuthentication,
+		DefaultAuthentication,
+		"Authentication type used to authenticate with kafka cluster")


Could you include the valid values (e.g. none, kerberos) and which is default.

I changed this to indicate valid values, the default value is displayed by the library that generates the help output.

This is the printed message:

--kafka.consumer.authentication string Authentication type used to authenticate with kafka cluster. e.g. none, kerberos (default "none")

pavolloffay

It looks good. Just some massaging comments.

The flags names should be changed for consistency and the kerberos flags could be defined in one place.

pavolloffay · 2019-06-26T09:32:28Z

cmd/ingester/app/flags.go

+	// SuffixKerberosPassword is Kerberos password
+	SuffixKerberosPassword = ".password"
+	// SuffixKerberosConfig is path to the kerberos configuration file.
+	SuffixKerberosConfig = ".config-path"


For the consistency with other flags this should be changed .config-file.

PAN_STORAGE_TYPE=elasticsearch go run -tags ui ./cmd/all-in-one/main.go -h | grep file  11:30 2019/06/26 11:31:02 maxprocs: Leaving GOMAXPROCS=8: CPU quota undefined --collector.grpc.tls.cert string Path to TLS certificate file --collector.grpc.tls.key string Path to TLS key file --config-file string Configuration file in JSON, TOML, YAML, HCL, or Java properties formats (default none). See spf13/viper for precedence. --es-archive.tags-as-fields.all (experimental) Store all span and process tags as object fields. If true .tags-as-fields.config-file is ignored. Binary tags are always stored as nested objects. --es-archive.tags-as-fields.config-file string (experimental) Optional path to a file containing tag keys which will be stored as object fields. Each key should be on a separate line. --es-archive.tls.ca string Path to TLS CA file --es-archive.tls.cert string Path to TLS certificate file --es-archive.tls.key string Path to TLS key file --es-archive.token-file string Path to a file containing bearer token. This flag also loads CA if it is specified. --es.tags-as-fields.all (experimental) Store all span and process tags as object fields. If true .tags-as-fields.config-file is ignored. Binary tags are always stored as nested objects. --es.tags-as-fields.config-file string (experimental) Optional path to a file containing tag keys which will be stored as object fields. Each key should be on a separate line. --es.tls.ca string Path to TLS CA file --es.tls.cert string Path to TLS certificate file --es.tls.key string Path to TLS key file --es.token-file string Path to a file containing bearer token. This flag also loads CA if it is specified. --query.static-files string The directory path override for the static assets for the UI --query.ui-config string The path to the UI configuration file in JSON format --reporter.grpc.tls.ca string Path to a TLS CA file. (default use the systems truststore) --sampling.strategies-file string The path for the sampling strategies file in JSON format. See sampling documentation to see format of the file

pavolloffay · 2019-06-26T09:33:28Z

cmd/ingester/app/flags.go

+	// SuffixKerberosConfig is path to the kerberos configuration file.
+	SuffixKerberosConfig = ".config-path"
+	// SuffixKerberosKeyTab is path keytab file used instead of password when SuffixKerberosUseKeyTab = true
+	SuffixKerberosKeyTab = ".keytab-path"


See the previous comment - change to keytab-file

pavolloffay · 2019-06-26T09:37:31Z

cmd/ingester/app/flags.go

+	flagSet.String(
+		KafkaConsumerConfigPrefix+KerberosPrefix+SuffixKerberosConfig,
+		DefaultKerberosConfig,
+		"Path to Kerberos configuration. i.e /etc/krb5.conf")


use the DefaultKerberosConfig

pavolloffay · 2019-06-26T09:38:03Z

cmd/ingester/app/flags.go

+	flagSet.String(
+		KafkaConsumerConfigPrefix+KerberosPrefix+SuffixKerberosKeyTab,
+		DefaultKerberosKeyTab,
+		"Path to keytab file. i.e /etc/security/kafka.keytab")


use DefaultKerberosKeyTab

pavolloffay · 2019-06-26T09:40:27Z

plugin/storage/kafka/options.go

+	defaultTopic               = "jaeger-spans"
+	defaultEncoding            = EncodingProto
+	defaultAuthentication      = "none"
+	defaultKerberosConfig      = "/etc/krb5.conf"


nit: reuse constant DefaultKerberosConfig?

pavolloffay · 2019-06-26T09:41:05Z

plugin/storage/kafka/options.go

+	defaultKerberosRealm       = ""
+	defaultKerberosPassword    = ""
+	defaultKerberosUsername    = ""
+	defaultKerberosKeyTab      = "/etc/security/kafka.keytab"


nit: reuse constant DefaultKerberosKeyTab?

pavolloffay · 2019-06-26T09:52:35Z

plugin/storage/kafka/options.go

@@ -69,12 +87,57 @@ func (opt *Options) AddFlags(flagSet *flag.FlagSet) {
 		defaultEncoding,
 		fmt.Sprintf(`(experimental) Encoding of spans ("%s" or "%s") sent to kafka.`, EncodingJSON, EncodingProto),
 	)
+	flagSet.String(


Instead of repeating this over, could we define it only in one place e.g. pkg/kafka/auth?

pavolloffay

LGTM just two nits

pavolloffay · 2019-06-26T15:42:40Z

pkg/kafka/auth/config.go

+)
+
+const none = "none"
+const kerberos = "kerberos"


nit: I would define this as array and pass to flag description:

to avoid duplication in constants

this way we don't forget to update help message if new auth is added

pavolloffay · 2019-06-26T15:43:49Z

pkg/kafka/auth/options.go

+	suffixKerberosConfig      = ".config-file"
+	suffixKerberosKeyTab      = ".keytab-file"
+
+	defaultAuthentication      = "none"


nit: related to my previous comment, remove this and just pass authTypes[0] to flag definition

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

gauravgill95 · 2020-11-26T10:33:51Z

Hi I am getting error saying Failed to init storage factory. Kafka client has run out of available brokers to talk to(Is your cluster reachable) ?
I error comes in SASL PlAINTEXT. My producer works fine in Plaintext.
Any help would be great

rubenvp8510 requested review from black-adder, jpkrohling, objectiser, pavolloffay, tiffon, vprithvi and yurishkuro as code owners June 5, 2019 23:05

rubenvp8510 force-pushed the kafka-kerbero-support branch 4 times, most recently from 3610065 to 53e2731 Compare June 6, 2019 02:15

rubenvp8510 changed the title ~~WIP - Add kafka kerberos authentication support for collector/ingester~~ Kafka kerberos authentication support for collector/ingester Jun 6, 2019

rubenvp8510 force-pushed the kafka-kerbero-support branch from 53e2731 to e554c19 Compare June 6, 2019 02:49

rubenvp8510 changed the title ~~Kafka kerberos authentication support for collector/ingester~~ WIP - Kafka kerberos authentication support for collector/ingester Jun 6, 2019

objectiser reviewed Jun 6, 2019

View reviewed changes

pavolloffay reviewed Jun 6, 2019

View reviewed changes

rubenvp8510 force-pushed the kafka-kerbero-support branch 12 times, most recently from 4ac2890 to b68c652 Compare June 7, 2019 13:38

rubenvp8510 force-pushed the kafka-kerbero-support branch 2 times, most recently from 4424d7e to be8726a Compare June 7, 2019 17:53

rubenvp8510 changed the title ~~WIP - Kafka kerberos authentication support for collector/ingester~~ Kafka kerberos authentication support for collector/ingester Jun 7, 2019

objectiser mentioned this pull request Jun 7, 2019

Release 1.13.0 #1592

Closed

objectiser approved these changes Jun 19, 2019

View reviewed changes

rubenvp8510 force-pushed the kafka-kerbero-support branch 2 times, most recently from 519f845 to 772e6a1 Compare June 19, 2019 17:40

pavolloffay reviewed Jun 26, 2019

View reviewed changes

pavolloffay mentioned this pull request Jun 26, 2019

Preparing release 1.13.0 #1632

Merged

rubenvp8510 force-pushed the kafka-kerbero-support branch 3 times, most recently from 2f5f210 to dc68b14 Compare June 26, 2019 15:08

pavolloffay approved these changes Jun 26, 2019

View reviewed changes

rubenvp8510 force-pushed the kafka-kerbero-support branch 2 times, most recently from 95738f8 to 6eac579 Compare June 26, 2019 19:16

Add kafka kerberos authentication support for collector/ingester

fa0e5ad

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

rubenvp8510 force-pushed the kafka-kerbero-support branch from 6eac579 to fa0e5ad Compare June 26, 2019 19:26

pavolloffay approved these changes Jun 27, 2019

View reviewed changes

pavolloffay merged commit 5b58ebc into jaegertracing:master Jun 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kafka kerberos authentication support for collector/ingester #1589

Kafka kerberos authentication support for collector/ingester #1589

rubenvp8510 commented Jun 5, 2019 •

edited

Loading

codecov bot commented Jun 6, 2019 •

edited

Loading

rubenvp8510 commented Jun 6, 2019

objectiser left a comment

objectiser Jun 6, 2019

objectiser Jun 6, 2019

objectiser Jun 6, 2019

objectiser Jun 6, 2019

pavolloffay Jun 6, 2019

objectiser Jun 6, 2019

rubenvp8510 Jun 6, 2019

objectiser Jun 19, 2019

pavolloffay Jun 6, 2019

objectiser Jun 6, 2019

rubenvp8510 Jun 6, 2019

rubenvp8510 commented Jun 7, 2019

objectiser Jun 19, 2019

objectiser Jun 19, 2019

rubenvp8510 Jun 19, 2019

pavolloffay left a comment

pavolloffay Jun 26, 2019

pavolloffay Jun 26, 2019

pavolloffay Jun 26, 2019

pavolloffay Jun 26, 2019

pavolloffay Jun 26, 2019

pavolloffay Jun 26, 2019

pavolloffay Jun 26, 2019

pavolloffay left a comment

pavolloffay Jun 26, 2019

pavolloffay Jun 26, 2019

gauravgill95 commented Nov 26, 2020

Kafka kerberos authentication support for collector/ingester #1589

Kafka kerberos authentication support for collector/ingester #1589

Conversation

rubenvp8510 commented Jun 5, 2019 • edited Loading

Which problem is this PR solving?

Short description of the changes

codecov bot commented Jun 6, 2019 • edited Loading

Codecov Report

rubenvp8510 commented Jun 6, 2019

objectiser 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

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

rubenvp8510 commented Jun 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay 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

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

pavolloffay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gauravgill95 commented Nov 26, 2020

rubenvp8510 commented Jun 5, 2019 •

edited

Loading

codecov bot commented Jun 6, 2019 •

edited

Loading