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.

Sign up for GitHub

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
Merged

[jaeger]Add kafka authentication option. #76

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7883d61
Add kafka authentication option.
Mar 13, 2020
f00578c
Increase timeout to 10m
Mar 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion charts/jaeger/Chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ apiVersion: v1
appVersion: 1.17.0
description: A Jaeger Helm chart for Kubernetes
name: jaeger
version: 0.23.0
version: 0.23.1
keywords:
- jaeger
- opentracing
Expand Down
3 changes: 2 additions & 1 deletion charts/jaeger/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@ The following table lists the configurable parameters of the Jaeger chart and th
| `esIndexCleaner.enabled` | Enables the ElasticSearch indices cleanup job| `false` |
| `esIndexCleaner.image` | Image for the ElasticSearch indices cleanup job| `jaegertracing/jaeger-es-index-cleaner` |
| `esIndexCleaner.pullPolicy` | Image pull policy of the ES cleanup image | `Always` |
| `esIndexCleaner.numberOfDays` | ElasticSearch indicess older than this number (Number of days) would be deleted by the CronJob | `7`
| `esIndexCleaner.numberOfDays` | ElasticSearch indices older than this number (Number of days) would be deleted by the CronJob | `7`
| `esIndexCleaner.schedule` | Schedule of the cron job | `"55 23 * * *"` |
| `esIndexCleaner.successfulJobsHistoryLimit` | successfulJobsHistoryLimit for ElasticSearch indices cleanup CronJob | `5` |
| `esIndexCleaner.failedJobsHistoryLimit` | failedJobsHistoryLimit for ElasticSearch indices cleanup CronJob | `5` |
Expand Down Expand Up @@ -356,6 +356,7 @@ The following table lists the configurable parameters of the Jaeger chart and th
| `storage.elasticsearch.user` | Provisioned elasticsearch user| `elastic` |
| `storage.elasticsearch.indexPrefix` | Index Prefix for elasticsearch | `nil` |
| `storage.elasticsearch.nodesWanOnly` | Only access specified es host | `false` |
| `storage.kafka.authentication` | Authentication type used to authenticate with kafka cluster. e.g. none, kerberos, tls | `none` |
| `storage.kafka.brokers` | Broker List for Kafka with port | `kafka:9092` |
| `storage.kafka.topic` | Topic name for Kafka | `jaeger_v1_test` |
| `storage.type` | Storage type (ES or Cassandra)| `cassandra` |
Expand Down
2 changes: 2 additions & 0 deletions charts/jaeger/templates/collector-deploy.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ spec:
value: {{ include "helm-toolkit.utils.joinListWithComma" .Values.storage.kafka.brokers }}
- name: KAFKA_PRODUCER_TOPIC
value: {{ .Values.storage.kafka.topic }}
- name: KAFKA_PRODUCER_AUTHENTICATION
value: {{ .Values.storage.kafka.authentication }}
{{ else }}
- name: SPAN_STORAGE_TYPE
value: {{ .Values.storage.type }}
Expand Down
2 changes: 2 additions & 0 deletions charts/jaeger/templates/ingester-deploy.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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?

value: {{ .Values.storage.kafka.authentication }}
ports:
- containerPort: 14270
name: admin
Expand Down
1 change: 1 addition & 0 deletions charts/jaeger/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ storage:
brokers:
- kafka:9092
topic: jaeger_v1_test
authentication: none

# Begin: Override values on the Cassandra subchart to customize for Jaeger
cassandra:
Expand Down
2 changes: 1 addition & 1 deletion ct.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,4 @@ chart-dirs:
chart-repos:
- incubator=https://kubernetes-charts-incubator.storage.googleapis.com
- elastic=https://helm.elastic.co
helm-extra-args: --timeout=500
helm-extra-args: --timeout=600