6744 - Fix Kafka TLS configuration with plaintext authentication #6764

amilbcahat · 2025-02-21T00:15:45Z

Which problem is this PR solving?

Resolves [Bug]: kafka: cannot connect to TLS kafka with TLS + plaintext #6744

Description of the changes

Fixed TLS configuration to work with plaintext authentication when TLS is enabled
Modified TLS configuration logic in Kafka authentication to support SASL-SSL with PLAIN
Restored behavior to enable TLS regardless of authentication method when TLS is configured
Fixed regression introduced in PR [kafka] OTEL helper instead of tlscfg package #6270

How was this change tested?

Verified connectivity with TLS-enabled Kafka cluster using plaintext authentication
Tested both collector and ingester components
Validated SASL-SSL with PLAIN authentication works as expected

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
[] I have added unit tests for the new functionality
I have run lint and test steps successfully
- for jaeger: make lint test

codecov · 2025-02-21T01:00:06Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.09%. Comparing base (148d4ef) to head (6197ff6).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6764      +/-   ##
==========================================
- Coverage   96.10%   96.09%   -0.02%     
==========================================
  Files         358      358              
  Lines       21585    21597      +12     
==========================================
+ Hits        20745    20754       +9     
- Misses        633      635       +2     
- Partials      207      208       +1

Flag	Coverage Δ
badger_v1	`9.90% <0.00%> (-0.02%)`	⬇️
badger_v2	`2.05% <0.00%> (-0.01%)`	⬇️
cassandra-4.x-v1-manual	`14.89% <0.00%> (-0.02%)`	⬇️
cassandra-4.x-v2-auto	`2.04% <0.00%> (-0.01%)`	⬇️
cassandra-4.x-v2-manual	`2.04% <0.00%> (-0.01%)`	⬇️
cassandra-5.x-v1-manual	`14.89% <0.00%> (-0.02%)`	⬇️
cassandra-5.x-v2-auto	`2.04% <0.00%> (-0.01%)`	⬇️
cassandra-5.x-v2-manual	`2.04% <0.00%> (-0.01%)`	⬇️
elasticsearch-6.x-v1	`20.14% <0.00%> (-0.03%)`	⬇️
elasticsearch-7.x-v1	`20.22% <0.00%> (-0.03%)`	⬇️
elasticsearch-8.x-v1	`20.40% <0.00%> (-0.03%)`	⬇️
elasticsearch-8.x-v2	`2.05% <0.00%> (-0.01%)`	⬇️
grpc_v1	`11.44% <0.00%> (-0.02%)`	⬇️
grpc_v2	`2.05% <0.00%> (-0.01%)`	⬇️
kafka-3.x-v1	`10.84% <100.00%> (+0.65%)`	⬆️
kafka-3.x-v2	`2.05% <0.00%> (-0.01%)`	⬇️
memory_v2	`2.05% <0.00%> (-0.01%)`	⬇️
opensearch-1.x-v1	`20.27% <0.00%> (-0.03%)`	⬇️
opensearch-2.x-v1	`20.27% <0.00%> (-0.03%)`	⬇️
opensearch-2.x-v2	`2.05% <0.00%> (-0.01%)`	⬇️
query	`2.05% <0.00%> (-0.01%)`	⬇️
tailsampling-processor	`0.55% <0.00%> (-0.01%)`	⬇️
unittests	`94.90% <100.00%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

yurishkuro · 2025-02-21T22:20:05Z

what are you trying to achieve by merging main? It erases the CI checks which clearly show that your PR does not pass the linter.

amilbcahat · 2025-02-21T23:43:47Z

what are you trying to achieve by merging main? It erases the CI checks which clearly show that your PR does not pass the linter.

I was updating the branch to latest, just that. I have committed for Lint checks , now. Can you check again ?

amilbcahat · 2025-02-23T14:02:27Z

I have made the corrections for Unit Tests, can you update the PR label please ? @yurishkuro and run it again, I dont have necessary permissions to add the label I think.

yurishkuro · 2025-02-23T21:31:25Z

pkg/kafka/auth/config.go

 		tlsClientConfig := tlscfg.ClientFlagsConfig{
 			Prefix: configPrefix,
 		}
 		tlsCfg, err := tlsClientConfig.InitFromViper(v)
 		if err != nil {
 			return fmt.Errorf("failed to process Kafka TLS options: %w", err)
 		}
+		tlsCfg.IncludeSystemCACertsPool = (config.Authentication == tls)


This is needed to maintain the security model difference between TLS authentication and TLS encryption:

When using TLS authentication (auth="tls"), we need system CA certs to validate client certificates

When using TLS encryption with SASL PLAIN auth, we don't need system CA certs
The unit tests specifically verify this distinction.

When using TLS encryption with SASL PLAIN auth, we don't need system CA certs

why? The only time you don't need system certs is if you are providing your own. Am I wrong about that?

Yes, you are right. Both modes need CA certs for TLS validation, but they use different sources:

TLS auth (authentication="tls"):

IncludeSystemCACertsPool=true to validate client/server certificates using system CA pool

Uses mutual TLS where both authenticate each other

SASL PLAIN with TLS:

IncludeSystemCACertsPool=false because it validates the server certificate using explicitly provided CA cert

Client authentication happens via username/password

The tests expect this specific behavior to enforce these different trust models, without this distinction tests fails. Both approaches provide certificate validation, just from different trust sources.

pkg/kafka/auth/config.go

yurishkuro · 2025-02-23T21:32:48Z

what is the testing procedure for this change? How do we know it does what's needed?

amilbcahat · 2025-02-24T13:42:06Z

what is the testing procedure for this change? How do we know it does what's needed?

To verify this fix works, I've set up a test environment with:

Kafka container configured for SASL_SSL:
- Uses TLS encryption with SASL PLAIN authentication

Docker Image configuration used -

version: "3"
services:
  zookeeper:
    image: bitnami/zookeeper:latest
    ports:
      - "2181:2181"
    environment:
      - ALLOW_ANONYMOUS_LOGIN=yes
  kafka:
    image: bitnami/kafka:3.7.0
    ports:
      - "9092:9092"
      - "29093:29093"
    environment:
      - KAFKA_BROKER_ID=1
      - KAFKA_CFG_ZOOKEEPER_CONNECT=zookeeper:2181
      - KAFKA_OFFSETS_TOPIC_REPLICATION_FACTOR=1
      - ALLOW_PLAINTEXT_LISTENER=yes
      - KAFKA_CFG_LISTENER_SECURITY_PROTOCOL_MAP=INTERNAL:PLAINTEXT,EXTERNAL:SASL_SSL
      - KAFKA_CFG_LISTENERS=internal://0.0.0.0:9092,external://0.0.0.0:29093
      - KAFKA_CFG_ADVERTISED_LISTENERS=internal://kafka:9092,external://localhost:29093
      - KAFKA_INTER_BROKER_LISTENER_NAME=INTERNAL
      - KAFKA_TLS_TYPE=JKS
      - KAFKA_CFG_SSL_KEYSTORE_LOCATION=/opt/bitnami/kafka/config/certs/kafka.keystore.jks
      - KAFKA_CFG_SSL_KEYSTORE_PASSWORD=kafkapass123
      - KAFKA_CFG_SSL_KEY_PASSWORD=kafkapass123
      - KAFKA_CFG_SSL_TRUSTSTORE_LOCATION=/opt/bitnami/kafka/config/certs/kafka.truststore.jks
      - KAFKA_CFG_SSL_TRUSTSTORE_PASSWORD=kafkapass123
      - KAFKA_CFG_SSL_ENDPOINT_IDENTIFICATION_ALGORITHM=
      - KAFKA_CFG_SASL_ENABLED_MECHANISMS=PLAIN
      - KAFKA_CFG_SASL_MECHANISM_INTER_BROKER_PROTOCOL=PLAIN
      - KAFKA_CLIENT_USERS=admin
      - KAFKA_CLIENT_PASSWORDS=admin-secret
    volumes:
      - ./certs:/opt/bitnami/kafka/config/certs
    depends_on:
      - zookeeper

Exposes port 29093 for SASL_SSL connections
Configured with admin/admin-secret credentials locally

Test with ingester accessing Kafka:

   ./ingester \
     --kafka.consumer.brokers=localhost:29093 \
     --kafka.consumer.topic=jaeger-spans \
     --kafka.consumer.authentication=plaintext \
     --kafka.consumer.plaintext.username=admin \
     --kafka.consumer.plaintext.password=admin-secret \
     --kafka.consumer.plaintext.mechanism=PLAIN \
     --kafka.consumer.group-id=jaeger-ingester \
     --kafka.consumer.tls.enabled=true \
     --kafka.consumer.tls.ca=./certs/ca.crt \
     --kafka.consumer.tls.skip-host-verify=true

Verified with standard Kafka clients (producer/consumer) that the connection works with the same settings

Without the fix in PR #6764, the ingester command would fail because TLS settings weren't properly applied when using SASL PLAIN authentication. With the fix, the connection succeeds. Do you need any more configuration related information used for testing here ?

yurishkuro · 2025-02-24T13:45:48Z

To verify this fix works, I've set up a test environment with:

Is this something we can add to internal/storage/integration/kafka_test.go?

amilbcahat · 2025-02-24T21:25:35Z

To verify this fix works, I've set up a test environment with:

Is this something we can add to internal/storage/integration/kafka_test.go?

Yes, we can add an integration test in internal/storage/integration/kafka_test.go to verify this scenario works properly. The test would:

Set up a Kafka client with:
- SASL PLAIN authentication
- TLS enabled for encryption
- Connected to a Kafka broker with SASL_SSL listener
Verify the client can successfully connect and produce/consume messages

This would formalize the manual test case I've been using to verify the fix. Would you like me to implement this as part of the PR?

yurishkuro · 2025-02-24T22:04:32Z

Yes, I prefer the tests to be part of the PR. However, is it possible to configure a single instance of Kafka to work with different auth-n methods, or do we need to spin different Kafka container for each auth flavor? The latter is much more expensive to run in the CI.

amilbcahat · 2025-02-24T22:38:39Z

Yes, I prefer the tests to be part of the PR. However, is it possible to configure a single instance of Kafka to work with different auth-n methods, or do we need to spin different Kafka container for each auth flavor? The latter is much more expensive to run in the CI.

Yes, this is possible. We can open different listeners in the same Kafka instance to work with different types of Authentication/security protocols. Should I go ahead with implementation ?

ref: https://developer.confluent.io/courses/security/authentication-basics/#:~:text=Configuring%20Authentication%3A%20Listeners%20and%20Security%20Protocols&text=Essentially%2C%20when%20configuring%20the%20broker,authenticate%2C%20whether%20SSL%20or%20SASL_SSL.

yurishkuro · 2025-02-24T22:51:11Z

yes, sounds good. One broker/container, multiple listeners, multiple tests using different ports.

I would recommend not running a full test suite against each listener, only some basic write/read tests.

amilbcahat · 2025-02-25T05:23:09Z

yes, sounds good. One broker/container, multiple listeners, multiple tests using different ports.

I would recommend not running a full test suite against each listener, only some basic write/read tests.

Sure, I will keep in mind

amilbcahat · 2025-03-01T21:21:24Z

Hi @yurishkuro, As we discussed, I have added -

Two Integration tests, for SASL Plaintext + TLS and SASL Plaintext + no TLS
Added the changes to include the flow in CI execution

To test these changes -

Go to - ./scripts/e2e/kafka.sh
Change jaeger_version to v1
Run kafka.sh file

Alternatively you can run specific integration test as well -

go test -v github.com/jaegertracing/jaeger/internal/storage/integration -run TestKafkaStorageWithSASLSSLPlaintext (need to setup the docker container first though and required certs as well)

amilbcahat · 2025-03-07T21:49:32Z

@yurishkuro Can you review this , please.

yurishkuro · 2025-03-07T21:57:50Z

docker-compose/kafka/v3/docker-compose.yml

 services:
+  zookeeper:


why do we need a separate zookeeper, especially for a single-node cluster? it just makes the CI longer to start

Yes, you are right, I have removed zookeeper in my latest commit and used Kraft with Kafka instead

yurishkuro · 2025-03-07T21:58:41Z

docker-compose/kafka/v3/docker-compose.yml

-      - "9092:9092"
+      - "9092:9092" #Internal PLAINTEXT
+      - "9094:9094" #External PLAINTEXT
+      - "29093:29093" #SASL_PLAINTEXT + SSL


why not 9093?

Changed to 9093

yurishkuro · 2025-03-07T22:16:20Z

scripts/e2e/kafka.sh

+  CERTS_DIR="${SCRIPT_DIR}/../../docker-compose/kafka/certs"
+  TEST_CERTS_DIR="${SCRIPT_DIR}/../../certs"
+
+  mkdir -p "$CERTS_DIR"


how critical is it to generate cert for the test? We already have working certs used in other parts of unit tests

./pkg/config/tlscfg/testdata/example-server-cert.pem ./pkg/config/tlscfg/testdata/example-CA-cert.pem ./pkg/config/tlscfg/testdata/example-server-key.pem ./pkg/config/tlscfg/testdata/example-client-key.pem ./pkg/config/tlscfg/testdata/wrong-CA-cert.pem ./pkg/config/tlscfg/testdata/example-client-cert.pem

I did try with these certs , but SASL + SSL would require certs generated with IP SANS information specified, these certs dont have it. But you are right, generating certs again and again is unnecessary... So I have generated the certs with IP Sans info and kept all the certs for this integration test at path -
pkg/config/tlscfg/testdata/kafka-certs , and removed the code for generating/cleaning up certs, in kafka.sh

certs generated with IP SANS information specified

Why do we need this, isn't there skipVerify option that ignores the host address? And how can you pre-generate IP SANS if you don't know which IP the server will be running in the CI?

Yes you right, I missed that ... added the flag now. Now it will just use the previous testdata. We need JKS format , so I have generated those through the testdata provided at -
internal/config/tlscfg/testdata, and used the same in kafka's docker compose file.

amilbcahat · 2025-05-04T11:36:59Z

@yurishkuro Please review the changes made

yurishkuro · 2025-05-04T13:39:59Z

your change has merge conflicts that need to be resolved

## Which problem is this PR solving? - The test in jaegertracing#6753 was still failing, even with the submodule update ## Description of the changes - Try to fetch tags ## How was this change tested? - CI --------- Signed-off-by: Yuri Shkuro <github@ysh.us> Signed-off-by: amol-verma-allen <amol.verma@allen.in>

This change fixes the Kafka TLS configuration to work correctly when tls.enabled flag is not provided but authentication=tls is set. Previously, TLS would not be enabled in this case. Changes: - TLS is now properly configured when authentication=tls, regardless of tls.enabled - Maintains backward compatibility with existing tls.enabled flag - Sets explicit insecure mode only when TLS is intentionally disabled Testing: - Added unit tests for TLS configuration scenarios - Verified with local Kafka cluster using TLS authentication - Tested with HotROD example application Resolves jaegertracing#6744 Signed-off-by: Amol Verma <amolverma@LT-BEN-90852.local> Signed-off-by: amol-verma-allen <amol.verma@allen.in>

This change fixes the Kafka TLS configuration to work correctly when using plaintext authentication with TLS enabled. Previously, TLS would only be configured when authentication=tls, breaking SASL-SSL with PLAIN authentication. Changes: - Modified TLS configuration logic to support TLS with other authentication methods - Fixed SASL-SSL with PLAIN authentication scenario - Maintained backward compatibility with existing authentication methods - Restored pre-PR-6270 behavior for TLS configuration Resolves jaegertracing#6744 Signed-off-by: Amol Verma <amolverma@LT-BEN-90852.local> Signed-off-by: amol-verma-allen <amol.verma@allen.in>

## Which problem is this PR solving? - This is solution to issue jaegertracing#6752 ## Description of the changes - Done minor code change of deprecated symbols after bot provided upgrade ## How was this change tested? - ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [ ] I have added unit tests for the new functionality - [ ] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `npm run lint` and `npm run test` --------- Signed-off-by: AnmolxSingh <anmol7344@gmail.com> Signed-off-by: Yuri Shkuro <github@ysh.us> Co-authored-by: Yuri Shkuro <github@ysh.us> Signed-off-by: amol-verma-allen <amol.verma@allen.in>

…ng#7072) ## Which problem is this PR solving? Currently, Jaeger sends its traces to ElasticSearch as uncompressed text. Since text is can be compressed quite well, enabling Gzip compression can significantly reduce Jaeger's network traffic. ElasticSearch has accepted compressed requests since version 5.0 and since the same version it has already sent compressed responses by default (cf. elastic/elasticsearch@0a6f40c). ## Description of the changes * 🛑 (breaking change) **Enable by default** the compression for requests to ElasticSearch * Add a new flag `--es.http-compression=true|false` that can be used to opt-out of compression . The setting is already supported by both client libraries that are used. ## How was this change tested? I tested the change running a local ElasticSearch instance and `SPAN_STORAGE_TYPE=elasticsearch ./cmd/collector/collector-linux-amd64 --es.http-compression=true`. I sent traces to Jaeger using `tracepusher` and observed the network traffic between Jaeger and ElasticSearch using `tcpdump` to verify that the traffic is indeed compressed. ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `npm run lint` and `npm run test` --------- Signed-off-by: Timon Engelke <timon.engelke@inovex.de> Signed-off-by: amol-verma-allen <amol.verma@allen.in>

…ertracing#7079) ## Which problem is this PR solving? - Fixes a part of: jaegertracing#7034 ## Description of the changes - The shared code need to be displaced completely to v2. This is a pre-requisite PR for implementing the dependency store in v2 ## How was this change tested? - Unit Tests ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `npm run lint` and `npm run test` Signed-off-by: Manik2708 <mehtamanik96@gmail.com> Signed-off-by: amol-verma-allen <amol.verma@allen.in>

…` level (jaegertracing#6946) ## Which problem is this PR solving? - Fixes a part of: jaegertracing#6458 ## Description of the changes - `TagDotReplacement` which is responsible for nested and field tags distinction was earlier at writer level (of model and OTLP) which doesn't make sense as DB layer should manipulate whatever is optimal for the database, therfore now it is a part of `CoreSpanWriter` ## How was this change tested? - Unit Tests ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `npm run lint` and `npm run test` --------- Signed-off-by: Manik2708 <mehtamanik96@gmail.com> Signed-off-by: amol-verma-allen <amol.verma@allen.in>

…der` level (jaegertracing#7067) ## Which problem is this PR solving? - Fixes a part of: jaegertracing#7034 ## Description of the changes - Make `NestedTags` and `ElevatedTags` distinction at `CoreSpanReader` level and a follow-up PR for jaegertracing#6946 ## How was this change tested? - Unit And Integration Tests ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `npm run lint` and `npm run test` --------- Signed-off-by: Manik2708 <mehtamanik96@gmail.com> Signed-off-by: amol-verma-allen <amol.verma@allen.in>

…g#6935) ## Which problem is this PR solving? - Part of jaegertracing#5058 ## Description of the changes - Based on the `ch-go` wire protocol, convert the OTel traces model to the ClickHouse native format for batch insertion. ## How was this change tested? - unit tests ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `npm run lint` and `npm run test` --------- Signed-off-by: zhengkezhou1 <madzhou1@gmail.com> Signed-off-by: amol-verma-allen <amol.verma@allen.in>

…acing#7085) ## Which problem is this PR solving? - Fixes a part of: jaegertracing#7034 ## Description of the changes - Upgrade DependencyStore for ES ## How was this change tested? - Unit Tests ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `npm run lint` and `npm run test` Signed-off-by: Manik2708 <mehtamanik96@gmail.com> Signed-off-by: amol-verma-allen <amol.verma@allen.in>

…ng#7091) This PR contains the following updates: | Package | Update | Change | |---|---|---| | [prom/prometheus](https://redirect.github.com/prometheus/prometheus) | patch | `v3.3.0` -> `v3.3.1` | --- ### Release Notes <details> <summary>prometheus/prometheus (prom/prometheus)</summary> ### [`v3.3.1`](https://redirect.github.com/prometheus/prometheus/compare/v3.3.0...v3.3.1) [Compare Source](https://redirect.github.com/prometheus/prometheus/compare/v3.3.0...v3.3.1) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR was generated by [Mend Renovate](https://mend.io/renovate/). View the [repository job log](https://developer.mend.io/github/jaegertracing/jaeger).  --------- Signed-off-by: Mend Renovate <bot@renovateapp.com> Signed-off-by: Yuri Shkuro <github@ysh.us> Co-authored-by: Yuri Shkuro <github@ysh.us> Signed-off-by: amol-verma-allen <amol.verma@allen.in>

…racing#7092) ## Which problem is this PR solving? - Resolves jaegertracing#7090 ## Description of the changes - Add validation to the build step that the version number is correctly embedded in the binary ## How was this change tested? - When reverting the fix done in jaegertracing#6990 the build fails: ``` $ make build-jaeger echo "building binary for $(go env GOOS)-$(go env GOARCH)"; CGO_ENABLED=0 installsuffix=cgo go build -trimpath -o ./cmd/jaeger/jaeger-darwin-arm64 -ldflags "-X github.com/jaegertracing/jaeger/pkg/version.commitSHA=687207be86edb688436f8ef6c4383968a0ec1855 -X github.com/jaegertracing/jaeger/pkg/version.latestVersion=v2.5.0 -X github.com/jaegertracing/jaeger/pkg/version.date=2025-05-02T18:47:48Z" ./cmd/jaeger building binary for darwin-arm64 ❌ ERROR: version mismatch: want=v2.5.0, have= make: *** [build-jaeger] Error 1 ``` --------- Signed-off-by: Yuri Shkuro <github@ysh.us> Signed-off-by: amol-verma-allen <amol.verma@allen.in>

## Which problem is this PR solving? - Resolves jaegertracing#6930 ## Description of the changes - Import `filterprocessor` as a counterpart to `attributeprocessor` - Increases the binary size by 2.18%, more than our "accidental" threshold, but acceptable ``` Previous binary size: 100223599 bytes New binary size: 102409625 bytes ❌ binary size increased by more than 2% (2.18%) ``` --------- Signed-off-by: Yuri Shkuro <github@ysh.us> Signed-off-by: amol-verma-allen <amol.verma@allen.in>

## Which problem is this PR solving? - When previous PR jaegertracing#7094 was force-merged with binary size check failing (there's no way to override that check to succeed), the same check [failed](https://github.com/jaegertracing/jaeger/actions/runs/14816241094/job/41597074760) on `main` AND the steps to update the binary size cache did not run ## Description of the changes - Change the check for size to not run when on the main branch ## Testing - The check is expected to still fail for this PR and will have to be force-merged - But then the same check on main should succeed and update the cached value Signed-off-by: Yuri Shkuro <github@ysh.us> Signed-off-by: amol-verma-allen <amol.verma@allen.in>

…ead of Zookeeper, Added Testdata for Kafka in testdata Signed-off-by: amol-verma-allen <amol.verma@allen.in>

This change fixes the Kafka TLS configuration to work correctly when using plaintext authentication with TLS enabled. Previously, TLS would only be configured when authentication=tls, breaking SASL-SSL with PLAIN authentication. Changes: - Modified TLS configuration logic to support TLS with other authentication methods - Fixed SASL-SSL with PLAIN authentication scenario - Maintained backward compatibility with existing authentication methods - Restored pre-PR-6270 behavior for TLS configuration Resolves jaegertracing#6744 Signed-off-by: Amol Verma <amolverma@LT-BEN-90852.local> Signed-off-by: amol-verma-allen <amol.verma@allen.in>

Signed-off-by: Amol Verma <amolverma@LT-BEN-90852.local> Signed-off-by: amol-verma-allen <amol.verma@allen.in>

- Fix TLS configuration initialization for Kafka auth - Add proper handling of system CA certs pool - Set secure defaults for TLS configuration - Remove redundant code comments Signed-off-by: Amol Verma <amilbcahat@gmail.com> Signed-off-by: Amol Verma <amolverma@LT-BEN-90852.local> Signed-off-by: amol-verma-allen <amol.verma@allen.in>

…m in the CI execution Signed-off-by: Amol Verma <amolverma@LT-BEN-90852.local> Signed-off-by: amol-verma-allen <amol.verma@allen.in>

…ead of Zookeeper, Added Testdata for Kafka in testdata Signed-off-by: amol-verma-allen <amol.verma@allen.in>

Signed-off-by: Amol Verma ( Dingus ) <68186313+amilbcahat@users.noreply.github.com>

Signed-off-by: amol-verma-allen <amol.verma@allen.in>

amilbcahat · 2025-05-04T22:28:16Z

@yurishkuro , resolved the conflicts now. Please review...

yurishkuro · 2025-05-05T01:46:26Z

docker-compose/kafka/v3/docker-compose.yml

 services:
  kafka:
-    image: bitnami/kafka:3.9.0@sha256:55df55bfc7ed5980447387620afa3498eab3985a4d8c731013d82b3fa8b43bff
+    image: bitnami/kafka:3.9.0


why does the image need to change? We intentionally pin images by SHA

yurishkuro

please do not commit binary files. If there are not available in the source form then it's better to regenerate on each run

yurishkuro · 2025-05-05T01:48:55Z

scripts/e2e/kafka.sh

@@ -129,4 +129,4 @@ main() {
  success="true"
 }

-main "$@"
+main "$@"


these last two files have no changes. please git checkout main $file to restore them

yurishkuro · 2025-05-05T01:50:28Z

internal/storage/kafka/auth/config.go

-			v.Set(configPrefix+".tls.enabled", "true")
-		}
+	// Initialize TLS config with default values
+	var tlsCfg configtls.ClientConfig


not necessary, you can assign config.TLS = tlsCfg inside if()

yurishkuro · 2025-05-05T01:51:12Z

internal/storage/kafka/auth/config.go

@@ -75,20 +84,26 @@ func (config *AuthenticationConfig) InitFromViper(configPrefix string, v *viper.
 	config.Kerberos.KeyTabPath = v.GetString(configPrefix + kerberosPrefix + suffixKerberosKeyTab)
 	config.Kerberos.DisablePAFXFast = v.GetBool(configPrefix + kerberosPrefix + suffixKerberosDisablePAFXFAST)

-	if config.Authentication == tls {
-		if !v.IsSet(configPrefix + ".tls.enabled") {
-			v.Set(configPrefix+".tls.enabled", "true")


don't you need this?

amilbcahat requested a review from a team as a code owner February 21, 2025 00:15

amilbcahat requested a review from joe-elliott February 21, 2025 00:15

dosubot bot added the storage/kafka label Feb 21, 2025

amilbcahat force-pushed the fix-kafka-tls-plaintext-6744 branch from c2267d2 to 2b355d0 Compare February 21, 2025 23:42

yurishkuro reviewed Feb 23, 2025

View reviewed changes

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

amilbcahat requested a review from yurishkuro February 24, 2025 13:43

yurishkuro reviewed Mar 7, 2025

View reviewed changes

yurishkuro and others added 4 commits May 5, 2025 03:37

timonegk and others added 20 commits May 5, 2025 03:37

Addressed comments, Changed SASL SSL Port, Used Kraft with Kafka inst…

601ec4a

…ead of Zookeeper, Added Testdata for Kafka in testdata Signed-off-by: amol-verma-allen <amol.verma@allen.in>

Fixed Lint Checks

708f405

Signed-off-by: Amol Verma <amolverma@LT-BEN-90852.local> Signed-off-by: amol-verma-allen <amol.verma@allen.in>

Added Integration Tests for Plaintext, Plaintext + SSL, and added the…

8496d29

…m in the CI execution Signed-off-by: Amol Verma <amolverma@LT-BEN-90852.local> Signed-off-by: amol-verma-allen <amol.verma@allen.in>

Addressed comments, Changed SASL SSL Port, Used Kraft with Kafka inst…

dd232eb

…ead of Zookeeper, Added Testdata for Kafka in testdata Signed-off-by: amol-verma-allen <amol.verma@allen.in>

amilbcahat force-pushed the fix-kafka-tls-plaintext-6744 branch from 307aa7b to dd232eb Compare May 4, 2025 22:07

amilbcahat and others added 3 commits May 5, 2025 03:41

Merge branch 'main' into fix-kafka-tls-plaintext-6744

df25180

Signed-off-by: Amol Verma ( Dingus ) <68186313+amilbcahat@users.noreply.github.com>

Removed unnecessary files

0a35eea

Signed-off-by: amol-verma-allen <amol.verma@allen.in>

Moved back to default version

6197ff6

Signed-off-by: amol-verma-allen <amol.verma@allen.in>

yurishkuro reviewed May 5, 2025

View reviewed changes

yurishkuro requested changes May 5, 2025

View reviewed changes

yurishkuro reviewed May 5, 2025

View reviewed changes

6744 - Fix Kafka TLS configuration with plaintext authentication #6764

Are you sure you want to change the base?

6744 - Fix Kafka TLS configuration with plaintext authentication #6764

Uh oh!

Conversation

amilbcahat commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

Uh oh!

codecov bot commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yurishkuro commented Feb 21, 2025

Uh oh!

amilbcahat commented Feb 21, 2025

Uh oh!

amilbcahat commented Feb 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amilbcahat Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yurishkuro commented Feb 23, 2025

Uh oh!

amilbcahat commented Feb 24, 2025

Uh oh!

yurishkuro commented Feb 24, 2025

Uh oh!

amilbcahat commented Feb 24, 2025

Uh oh!

yurishkuro commented Feb 24, 2025

Uh oh!

amilbcahat commented Feb 24, 2025

Uh oh!

yurishkuro commented Feb 24, 2025

Uh oh!

amilbcahat commented Feb 25, 2025

Uh oh!

amilbcahat commented Mar 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amilbcahat commented Mar 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amilbcahat May 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amilbcahat commented May 4, 2025

Uh oh!

yurishkuro commented May 4, 2025

Uh oh!

amilbcahat commented May 4, 2025

Uh oh!

Choose a reason for hiding this comment

amilbcahat commented Feb 21, 2025 •

edited

Loading

codecov bot commented Feb 21, 2025 •

edited

Loading

amilbcahat commented Feb 23, 2025 •

edited

Loading

amilbcahat Feb 24, 2025 •

edited

Loading

amilbcahat commented Mar 1, 2025 •

edited

Loading

amilbcahat May 4, 2025 •

edited

Loading