Skip to content

feat(kafka-logger): add TLS support for Kafka brokers#13607

Merged
AlinsRan merged 13 commits into
apache:masterfrom
ecsimsw:feat/kafka-logger-tls-support
Jul 16, 2026
Merged

feat(kafka-logger): add TLS support for Kafka brokers#13607
AlinsRan merged 13 commits into
apache:masterfrom
ecsimsw:feat/kafka-logger-tls-support

Conversation

@ecsimsw

@ecsimsw ecsimsw commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Description

  • Add TLS option to kafka-logger and error-log-logger plugins for connecting to TLS-enabled Kafka brokers such as AWS MSK.
  • APISIX uses lua-resty-kafka for Kafka produce, and lua-resty-kafka already supports ssl/ssl_verify in broker_config.
  • However, APISIX plugins were not exposing these options to users.
  • This PR adds a tls object schema so users can enable TLS connections.
  • The tls object design allows future extension for mTLS support.

Changes

  • kafka-logger.lua : add tls schema + pass ssl/ssl_verify to broker_config
  • error-log-logger.lua : same change
  • kafka-logger-tls.t : schema validation + integration tests via SSL port (9093)
  • error-log-logger-kafka.t : schema validation + integration tests for TLS
  • kafka-logger.md, error-log-logger.md : add tls attributes and usage examples

Which issue(s) this PR fixes:

Verification

  • Tested against AWS MSK (Real world, TLS-only) : TLS handshake and metadata fetch confirmed working.
  • luacheck: passed locally (0 warnings / 0 errors)
  • check-test-code-style.sh: passed locally
  • lj-releng: requires LuaJIT 2.1 (not available locally), to be verified by CI

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. enhancement New feature or request labels Jun 25, 2026
@juzhiyuan

Copy link
Copy Markdown
Member

Hi, thank you for your contribution.

I looked through the historical APISIX issues related to this topic and agree that this is a reasonable fix. A previous issue, #9353, was closed by the bot as stale.

Also, services such as AWS MSK require TLS, which means kafka-logger cannot work properly in those environments before this PR is accepted. Also I notice that https://github.com/ecsimsw/apisix/blob/841dbea7a167684e6b9834c33e841ea61693a100/apisix/pubsub/kafka.lua#L83 already supports TLS, while kafka-logger does not.

So I think this is a valid request.

@juzhiyuan

Copy link
Copy Markdown
Member

@ecsimsw some CI fails - do you have time to fix it before further reviewing?

@ecsimsw

ecsimsw commented Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

@juzhiyuan
Hi, thank you for reviewing.
It's exciting to contribute to a project that I've been actively using at work.

I've checked the CI failures.
The plugin test environment does not have a Kafka SSL listener, so the TLS integration tests could not connect.
The schema validation tests pass fine.

The last test environment already has Kafka SSL configured.
I can add the SSL config following the last environment setup,
but since it's shared infrastructure, I'd like to get your opinion before making changes.

  1. Add Kafka SSL to the plugin test environment, similar to the last environment.
  2. Keep only schema validation tests — consistent with how SASL tests are structured.

Which approach would you prefer?

**
For now, I've removed the TLS integration tests and kept only the schema validation tests.
If you or other maintainers would like to add the SSL Kafka setup to the plugin CI environment, I'm happy to follow up on that.

@ecsimsw

ecsimsw commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Hi @juzhiyuan, just a friendly follow-up — CI is green now.
Let me know if there's anything else needed for review.
Thanks!

@juzhiyuan

Copy link
Copy Markdown
Member

Hi @ecsimsw, thanks for your following up! I will request reviews from team next monday.

Comment thread apisix/plugins/kafka-logger.lua
Comment thread apisix/plugins/error-log-logger.lua
Comment thread t/plugin/kafka-logger-tls.t Outdated
Comment thread t/plugin/kafka-logger-tls.t Outdated
@AlinsRan

AlinsRan commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

The current tests only cover check_schema — nothing exercises ssl = true. Please add an e2e test for the TLS path (CI has no TLS broker today; kafka-server1 in ci/pod/docker-compose.plugin.yml needs an SSL listener on 9093).

ecsimsw added 5 commits July 13, 2026 20:51
Follow the check_tls_bool convention used by http-logger and
clickhouse-logger so users are warned when tls.verify is set to false.
…abled

Call check_tls_bool inside the metadata branch, since the kafka config
only exists in metadata_schema and check_schema returns early for
TYPE_METADATA before reaching the non-metadata checks.
Drop the dedicated kafka-logger-tls.t file and its leftover
batch-processor monkey-patch preprocessor, which was dead code since the
integration tests were removed. The three schema-validation tests now
live in kafka-logger2.t, consistent with the error-log-logger side of
this PR.
Enable a TLS listener on 9093 for kafka-server1 so the TLS integration
tests can connect. Generate a self-signed keystore via keytool and mount
it as the broker keystore/truststore, mirroring the last test env.
- kafka-logger: produce over TLS (verify=false) and reject self-signed
  cert (verify=true)
- error-log-logger: produce over TLS via plugin metadata
- security-warning: assert check_tls_bool warns when tls.verify /
  kafka.tls.verify is disabled
@ecsimsw

ecsimsw commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

@AlinsRan
Thanks for the careful review.

Following the last env setup, I added a 9093 SSL listener to kafka-server1 in the plugin CI environment.

  • Generate a self-signed certificate via keytool in init-plugin-test-service.sh
  • kafka-logger: TLS produce succeeds with verify=false, and rejects the self-signed cert with verify=true, which shows the flag is actually wired through
  • error-log-logger: TLS produce via metadata succeeds
  • security-warning.t: a warning is logged when tls.verify / kafka.tls.verify is disabled

I also considered reusing the fixture (t/certs/apisix.crt) to test verify: true as a success:

  • TLS tests pass an SNI to sslhandshake that is independent of the connect address, so it can match the cert (test.com)
  • but lua-resty-kafka pins the SNI to the broker host (self.host), so the connect address 127.0.0.1 becomes the verified name
  • the cert is CN=test.com, so it mismatches → fails with certificate host mismatch
  • making verify: true succeed would need advertising the broker as test.com plus name resolution → touches more of the shared CI
  • so I kept the self-signed cert and cover verify: true as a rejection
  • happy to add the trusted-cert approach if you'd prefer it

@ecsimsw ecsimsw requested a review from AlinsRan July 14, 2026 00:43
Comment thread apisix/plugins/error-log-logger.lua
check_tls_bool ran before core.schema.check filled schema defaults, so
a metadata config with "tls": {} (verify omitted, defaults to false)
left kafka.tls.verify as nil and the security warning was skipped. Move
the call after the schema check, matching kafka-logger, and add
security-warning tests for the verify-omitted case.
@dosubot dosubot Bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Jul 15, 2026
@ecsimsw ecsimsw requested a review from AlinsRan July 15, 2026 12:06

@membphis membphis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@AlinsRan AlinsRan merged commit b86a6c8 into apache:master Jul 16, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request size:XL This PR changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

kafka security_protocol Does kafka support SASL_SSL?

5 participants