AlinsRan commented on PR #13607:
URL: https://github.com/apache/apisix/pull/13607#issuecomment-4954954802
Expanding on the coverage point from my review, since it's the one thing I'd
want resolved before this merges.
**Right now no test in this PR executes `ssl = true`.** The three tests in
`kafka-logger-tls.t` and the two in `error-log-logger-kafka.t` are all
`check_schema` calls. They assert that jsonschema accepts `{verify = false}`
and rejects `{verify = "abc"}` — they would pass identically if the
`broker_config["ssl"] = true` lines were deleted from both plugins. So the
actual feature has zero coverage, and "tested against AWS MSK" in the PR
description is not something CI can keep true.
I know you dropped the integration tests because the CI broker is
plaintext-only. That's real: `ci/pod/docker-compose.plugin.yml` `kafka-server1`
exposes only `PLAINTEXT://:9092`. But adding a TLS listener turns out to be
cheap, and there's already more groundwork than it looks.
### Option 1 — add an SSL listener to the CI broker (preferred)
`ci/pod/kafka/kafka-server/env/common.env`:
```env
KAFKA_CFG_LISTENERS=PLAINTEXT://0.0.0.0:9092,SSL://0.0.0.0:9093
KAFKA_CFG_ADVERTISED_LISTENERS=PLAINTEXT://127.0.0.1:9092,SSL://127.0.0.1:9093
KAFKA_TLS_TYPE=PEM
KAFKA_CFG_SSL_ENDPOINT_IDENTIFICATION_ALGORITHM=
```
and mount the existing fixtures as the broker's keystore in
`docker-compose.plugin.yml` (`kafka-server1`), plus publish `9093:9093`:
```yaml
volumes:
-
./t/certs/apisix.crt:/opt/bitnami/kafka/config/certs/kafka.keystore.pem:ro
-
./t/certs/apisix.key:/opt/bitnami/kafka/config/certs/kafka.keystore.key:ro
```
Two things make this land nicely:
- `t/APISIX.pm:538` already emits `lua_ssl_trusted_certificate
cert/apisix.crt;` in the test nginx conf. lua-resty-kafka's `ssl_verify` path
goes through `sock:sslhandshake(false, host, ssl_verify)`
(`resty/kafka/broker.lua:142-145`), which uses exactly that trust store — so if
the broker serves `t/certs/apisix.crt`, you can test **`verify: true`** end to
end with no extra config, not just `verify: false`.
- `common2.env` already carries `KAFKA_CFG_SSL_KEYSTORE_*` leftovers, so the
SSL knobs aren't new territory for this compose file.
That gives a real produce-over-TLS test, and it's the same infra mTLS will
need when the `tls` object grows `client_cert`/`client_key`.
### Option 2 — if that's out of scope for this PR, at least a negative test
Point the plugin at the plaintext 9092 broker with TLS on and assert the
failure surfaces:
```
=== TEST: tls enabled against a plaintext broker fails the handshake
--- config
location /t {
content_by_lua_block {
local plugin = require("apisix.plugins.kafka-logger")
-- brokers = {{host = "127.0.0.1", port = 9092}}, tls = {verify
= false},
-- batch_max_size = 1 -> flush immediately
}
}
--- error_log
failed to send data to Kafka topic
```
It needs no new CI infra, and unlike the schema tests it fails if
`broker_config["ssl"]` is ever dropped — which is the whole point of a
regression test here.
Option 1 is worth the extra effort though. Happy to help with the compose
changes if that's the blocker.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
