[fix] Enable Kafka TLS when TLS auth is specified #2107

pavolloffay · 2020-03-02T14:22:48Z

Resolves #2092

Signed-off-by: Pavol Loffay [email protected]

Signed-off-by: Pavol Loffay <[email protected]>

jpkrohling · 2020-03-02T14:27:57Z

cmd/ingester/app/flags_test.go

+			expected: auth.AuthenticationConfig{Authentication: "tls", Kerberos: auth.KerberosConfig{ServiceName: "kafka", ConfigPath: "/etc/krb5.conf", KeyTabPath: "/etc/security/kafka.keytab"}, TLS: tlscfg.Options{Enabled: true}},
+		},
+		{
+			flags:    []string{"--kafka.consumer.authentication=tls", "--kafka.consumer.tls.enabled=false"},


This means that if someone explicitly set tls.enabled to false, this will be silently changed to true`. I have nothing against that, but perhaps there's a way to detect whether this value was explicitly provided and keep whatever the user set?

it can be done by changing the enabled property to a pointer. I am not sure if there is much value in it. Some signatures will have to be also changed to accept the logger.

More controversial is this setting

// --kafka.consumer.authentication=kerberos", --kafka.consumer.tls.enabled=true"

Somebody omitting the auth type and specifying tls.enabled=true.

@yurishkuro any thoughts on this? I don't have a strong opinion whether kafka.consumer.tls.enabled=true should set kafka.consumer.authentication=tls.

From Gitter, we just saw a case where a user wants TLS encryption between the agent and collector without the auth parts. kafka.producer.tls.enabled=true + kafka.producer.authentication=none is a valid combination.

You are right, it's confusing for people to have both kafka.producer.tls.enabled and kafka.producer.authentication they will probably tend to forget the second one.

I have updated the PR to set tls auth when .tls.enabled is true.

I have updated the PR to set tls auth when .tls.enabled is true.

It's the opposite. tls.enabled does not imply authentication=tls. It's perfectly possible to have authentication=none but the traffic be encrypted using TLS.

@jpkrohling I am not sure if I understand you...

It's perfectly possible to have authentication=none but the traffic be encrypted using TLS.

This is exactly what the PR does.

Sorry, I misunderstood you, you are absolutely correct :-)

codecov · 2020-03-02T15:59:49Z

Codecov Report

Merging #2107 into master will decrease coverage by 0.16%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2107      +/-   ##
==========================================
- Coverage   96.27%   96.11%   -0.17%     
==========================================
  Files         214      217       +3     
  Lines       10535    10541       +6     
==========================================
- Hits        10143    10131      -12     
- Misses        333      354      +21     
+ Partials       59       56       -3

Impacted Files	Coverage Δ
cmd/collector/app/collector.go	`69.11% <0%> (-3.04%)`	⬇️
cmd/agent/app/builder.go	`100% <0%> (ø)`	⬆️
cmd/collector/app/builder_flags.go	`100% <0%> (ø)`	⬆️
cmd/collector/app/handler/tchannel_handler.go
cmd/agent/app/reporter/tchannel/flags.go
cmd/agent/app/reporter/tchannel/builder.go
cmd/agent/app/reporter/tchannel/collector_proxy.go
cmd/collector/app/server/thrift.go
cmd/agent/app/configmanager/tchannel/manager.go
cmd/agent/app/reporter/tchannel/reporter.go
... and 12 more

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 b0ad5c7...1f1a6f6. Read the comment docs.

Signed-off-by: Pavol Loffay <[email protected]>

yurishkuro · 2020-03-02T17:55:01Z

pkg/kafka/auth/config.go

+	if config.Authentication == tls {
+		config.TLS.Enabled = true
+	}
+	if config.TLS.Enabled == true {


in L83, can we change ShowEnabled to false?

It will break jaeger-operator and clients which adopted 1.17

Fair enough, but do we really want to keep this duality? We can extend the config to render tls.enabled as deprecated, and add warnings when used.

I can try to do that, I don't like the current design either.

I thought @jpkrohling had mentioned a required use case where TLS could be enabled, but authentication set to none or something other than tls?

I have created an issue to remove them in the next release #2111

@objectiser yes, the current flags are misleading for people because there is overlap betwen:

--kafka.consumer.authentication --kafka.consumer.tls.enabled

The goal is to remove the second one.

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay · 2020-03-03T10:35:34Z

pkg/kafka/auth/config.go

@@ -89,3 +90,14 @@ func (config *AuthenticationConfig) InitFromViper(configPrefix string, v *viper.
 	config.PlainText.UserName = v.GetString(configPrefix + plainTextPrefix + suffixPlainTextUserName)
 	config.PlainText.Password = v.GetString(configPrefix + plainTextPrefix + suffixPlainTextPassword)
 }
+
+// Normalize normalizes kafka options
+func (config *AuthenticationConfig) Normalize(logger *zap.Logger) {


I had to add to split InitFromViper and Normalize. InitFromViper is called via storage factory and it does not have access to the logger when doing the call.

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay · 2020-03-04T12:00:31Z

@jaegertracing/jaeger-maintainers can anybody review this?

cmd/ingester/app/flags_test.go

yurishkuro · 2020-03-04T13:02:07Z

cmd/ingester/app/flags_test.go

+		},
+		{
+			flags:    []string{"--kafka.consumer.authentication=kerberos", "--kafka.consumer.tls.enabled=true"},
+			expected: auth.AuthenticationConfig{Authentication: "tls", Kerberos: auth.KerberosConfig{ServiceName: "kafka", ConfigPath: "/etc/krb5.conf", KeyTabPath: "/etc/security/kafka.keytab"}, TLS: tlscfg.Options{Enabled: true}},


why is Authentication: "tls" here?

yurishkuro · 2020-03-04T13:03:29Z

pkg/kafka/auth/config.go

@@ -85,6 +85,12 @@ func (config *AuthenticationConfig) InitFromViper(configPrefix string, v *viper.
 	}

 	config.TLS = tlsClientConfig.InitFromViper(v)
+	if config.TLS.Enabled {
+		config.Authentication = tls


I don't think this is correct. Other modes can be compatible with TLS according to @jpkrohling, so just having tls.enabled should not change the auth scheme.

I have removed this and made change to pkg/kafka/auth/config.go to load TLS when tls.enabled=true

yurishkuro · 2020-03-04T13:05:34Z

plugin/storage/kafka/options_test.go

@@ -164,3 +167,43 @@ func TestRequiredAcksFailures(t *testing.T) {
 	_, err := getRequiredAcks("test")
 	assert.Error(t, err)
 }
+
+func TestTLSFlags(t *testing.T) {


These tests are duplicated. Don't we have Kafka flags parsed by a packaged shared between ingester and storage?

Both tests are in different packages, there is no shared test class.

Signed-off-by: Pavol Loffay <[email protected]>

pkg/kafka/auth/config.go

Signed-off-by: Pavol Loffay <[email protected]>

* [fix] Enable Kafka TLS when TLS auth is specified Signed-off-by: Pavol Loffay <[email protected]> * Use TLS when tls.enabled=true Signed-off-by: Pavol Loffay <[email protected]> * Log deprecation message Signed-off-by: Pavol Loffay <[email protected]> * Fix fmt and lint Signed-off-by: Pavol Loffay <[email protected]> * Remove deprecated TLS enabled flag Signed-off-by: Pavol Loffay <[email protected]> * Review comments Signed-off-by: Pavol Loffay <[email protected]> * Add tls case back Signed-off-by: Pavol Loffay <[email protected]>

[fix] Enable Kafka TLS when TLS auth is specified

0042cd3

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay requested a review from a team as a code owner March 2, 2020 14:22

pavolloffay requested a review from vprithvi March 2, 2020 14:22

jpkrohling reviewed Mar 2, 2020

View reviewed changes

Use TLS when tls.enabled=true

5b48422

Signed-off-by: Pavol Loffay <[email protected]>

yurishkuro reviewed Mar 2, 2020

View reviewed changes

Log deprecation message

4e170e9

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay commented Mar 3, 2020

View reviewed changes

pavolloffay added 2 commits March 3, 2020 16:37

Fix fmt and lint

5dc88cb

Signed-off-by: Pavol Loffay <[email protected]>

Remove deprecated TLS enabled flag

4c760ad

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay mentioned this pull request Mar 3, 2020

Remove deprecated kafka tls.enabled flags #2111

Closed

yurishkuro reviewed Mar 4, 2020

View reviewed changes

Review comments

7e8c615

Signed-off-by: Pavol Loffay <[email protected]>

yurishkuro reviewed Mar 4, 2020

View reviewed changes

pkg/kafka/auth/config.go Show resolved Hide resolved

Add tls case back

1f1a6f6

Signed-off-by: Pavol Loffay <[email protected]>

yurishkuro approved these changes Mar 5, 2020

View reviewed changes

yurishkuro merged commit c328471 into jaegertracing:master Mar 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix] Enable Kafka TLS when TLS auth is specified #2107

[fix] Enable Kafka TLS when TLS auth is specified #2107

pavolloffay commented Mar 2, 2020

jpkrohling Mar 2, 2020

pavolloffay Mar 2, 2020

pavolloffay Mar 2, 2020

jpkrohling Mar 2, 2020 •

edited

Loading

pavolloffay Mar 2, 2020

jpkrohling Mar 3, 2020

pavolloffay Mar 3, 2020

jpkrohling Mar 3, 2020

codecov bot commented Mar 2, 2020 •

edited

Loading

yurishkuro Mar 2, 2020

pavolloffay Mar 2, 2020

yurishkuro Mar 2, 2020

pavolloffay Mar 3, 2020

objectiser Mar 3, 2020

pavolloffay Mar 3, 2020

pavolloffay Mar 3, 2020 •

edited

Loading

pavolloffay Mar 3, 2020

pavolloffay commented Mar 4, 2020

yurishkuro Mar 4, 2020

yurishkuro Mar 4, 2020

pavolloffay Mar 4, 2020

yurishkuro Mar 4, 2020

pavolloffay Mar 4, 2020

[fix] Enable Kafka TLS when TLS auth is specified #2107

[fix] Enable Kafka TLS when TLS auth is specified #2107

Conversation

pavolloffay commented Mar 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling Mar 2, 2020 • edited Loading

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

codecov bot commented Mar 2, 2020 • edited Loading

Codecov Report

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 Mar 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Mar 4, 2020

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

jpkrohling Mar 2, 2020 •

edited

Loading

codecov bot commented Mar 2, 2020 •

edited

Loading

pavolloffay Mar 3, 2020 •

edited

Loading