Breaking change in v1.17.0 when using to Kafka + TLS

[jpkrohling]

From: #1838 (comment), decided to create a separate issue to track this and decide what to do.

The PR #1838 introduced a breaking change: previously, there was no --kafka.consumer.tls option (or producer), and TLS config would be used on whether the getTLS() function returned an error or not. Now, even if all the parameters are set, the collector/ingester won't be using TLS unless a new option is added to the command line, --kafka.consumer.tls.enabled (or the one marked as deprecated but that never existed: --kafka.consumer.tls).

For example, we had this in the jaeger-operator before, when auto-provisioning a Kafka cluster:

      --kafka.producer.authentication=tls
      --kafka.producer.brokers=auto-provision-kafka-kafka-bootstrap.default.svc.cluster.local:9093
      --kafka.producer.tls.ca=/var/run/secrets/auto-provision-kafka-cluster-ca/ca.crt
      --kafka.producer.tls.cert=/var/run/secrets/auto-provision-kafka/user.crt
      --kafka.producer.tls.key=/var/run/secrets/auto-provision-kafka/user.key

It now requires the new property to be added, otherwise, the collector/ingester will crash with the following error:

{"level":"fatal","ts":1582796332.9417691,"caller":"collector/main.go:91","msg":"Failed to init storage factory","error":"kafka: client has run out of available brokers to talk to (Is your cluster reachable?)","stacktrace":"main.main.func1\n\t/home/travis/gopath/src/github.com/jaegertracing/jaeger/cmd/collector/main.go:91\ngithub.com/jaegertracing/jaeger/vendor/github.com/spf13/cobra.(*Command).execute\n\t/home/travis/gopath/src/github.com/jaegertracing/jaeger/vendor/github.com/spf13/cobra/command.go:698\ngithub.com/jaegertracing/jaeger/vendor/github.com/spf13/cobra.(*Command).ExecuteC\n\t/home/travis/gopath/src/github.com/jaegertracing/jaeger/vendor/github.com/spf13/cobra/command.go:783\ngithub.com/jaegertracing/jaeger/vendor/github.com/spf13/cobra.(*Command).Execute\n\t/home/travis/gopath/src/github.com/jaegertracing/jaeger/vendor/github.com/spf13/cobra/command.go:736\nmain.main\n\t/home/travis/gopath/src/github.com/jaegertracing/jaeger/cmd/collector/main.go:183\nruntime.main\n\t/home/travis/.gimme/versions/go1.13.5.linux.amd64/src/runtime/proc.go:203"}

For completeness, here's the set of options for v1.17.0 that works in the same scenario:

      --kafka.producer.authentication=tls
      --kafka.producer.brokers=auto-provision-kafka-kafka-bootstrap.default.svc.cluster.local:9093
      --kafka.producer.tls.enabled=true
      --kafka.producer.tls.ca=/var/run/secrets/auto-provision-kafka-cluster-ca/ca.crt
      --kafka.producer.tls.cert=/var/run/secrets/auto-provision-kafka/user.crt
      --kafka.producer.tls.key=/var/run/secrets/auto-provision-kafka/user.key

[yurishkuro]

Hm, I do recall commenting on the pr about that. Because we already have a selector for auth scheme, specifying tls.enabled is redundant.

[pavolloffay]

I am working on the fix

[pavolloffay]

Yes @yurishkuro asked about this, but it wasn't applied to the PR https://github.com/jaegertracing/jaeger/pull/1838/files#r338868216.

I think we cannot revert (hide TLS enabled) as some consumers might already adopted the change - like jaeger operator.