Skip to content
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

[jaeger]Add kafka authentication option. #76

Merged
merged 2 commits into from
Mar 16, 2020

Conversation

naseemkullah
Copy link
Member

Please see title.

Fixes any breakage related to jaegertracing/jaeger#2092 thus fixes #61

cc @pavelnikolov @jpkrohling @arpitjindal97

Signed-off-by: Naseem <naseem@transit.app>
@naseemkullah naseemkullah force-pushed the auth-kafka branch 3 times, most recently from 5b0fa17 to 494cf21 Compare March 13, 2020 12:19
.vscode/settings.json Outdated Show resolved Hide resolved
@@ -64,6 +64,8 @@ spec:
value: {{ include "helm-toolkit.utils.joinListWithComma" .Values.storage.kafka.brokers }}
- name: KAFKA_CONSUMER_TOPIC
value: {{ .Values.storage.kafka.topic }}
- name: KAFKA_CONSUMER_AUTHENTICATION
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how that works in the Helm chart, but note that the Ingester can also be configured with Kafka as backend storage (ie, as producer):

$ podman run -e SPAN_STORAGE_TYPE=kafka jaegertracing/jaeger-ingester:1.17 --help | grep \\.authentication
      --kafka.consumer.authentication string          Authentication type used to authenticate with kafka cluster. e.g. none, kerberos, tls (default "none")
      --kafka.producer.authentication string          Authentication type used to authenticate with kafka cluster. e.g. none, kerberos, tls (default "none")

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh thanks for bringint that up, the chart currently presumes it will only use ES or Cassandra as a backend.

We will have to add the functionality to use kafka as backend (in a separate issue). Any docs covering such use case?

I believe @arpitjindal97 based the implementation off the diagram here: https://www.jaegertracing.io/docs/1.17/architecture/#components

Copy link
Contributor

@jpkrohling jpkrohling Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, the documentation for jaeger-ingester skips a storage of type kafka. I'm now not sure we actually support that: https://www.jaegertracing.io/docs/1.17/cli/#jaeger-ingester

@objectiser, @kevinearls do you remember this is a valid scenario? Have we tested this at all? I don't remember anything in the code that would prevent this from happening, and Kafka Producer is just another storage plugin, but perhaps there's indeed something in the code that prevents the ingester from using a kafka storage mechanism?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is a configuration we would generally support - what is the purpose of ingesting spans from kafka only to then publish them back on kafka (presumably on a different topic)? Surely there are pure kafka related tools that could do that if necessary (e.g. mirrormaker)?

However I am not sure there is anything that explicitly prevents it - although we could add a warning message (or actively prevent it) if we didn't think it was good to allow?

Copy link
Contributor

@arpitjindal97 arpitjindal97 Mar 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kafka is put in between only to reduce the load on Database. It should not be used as Database.

I think for the time being, chart should have authentication at consumable side. When docuemtation for using Kafka as DB will be released, then we can add auth for producer side

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One scenerio i can think of is when load is way too more and putting Kafka in between also isn't working out. We might want to put another Kafka after ingester and then second ingester would put them in DB.

Collector --> Kafka1 --> Ingester1 --> Kafka2 --> Ingester2 --> DB

This works assuming scaling Kafka1 isn't helping much

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The case I had in mind was more like what mirror maker would do, and you are absolutely right: there are better tools to handle that then chaining ingester -> Kafka -> ingester.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks all for explaining!
@jpkrohling can we merge this then?

Deploying Cassandra is time consuming and test does not always complete within 500s

Signed-off-by: Naseem <naseem@transit.app>
Copy link
Contributor

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM then :)

@naseemkullah naseemkullah merged commit 5772a97 into jaegertracing:master Mar 16, 2020
@naseemkullah naseemkullah deleted the auth-kafka branch March 16, 2020 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[jaeger] Ingester: Breaking change in v1.17.0 when using to Kafka + TLS
4 participants