AlinsRan commented on code in PR #13607:
URL: https://github.com/apache/apisix/pull/13607#discussion_r3568367621
##########
apisix/plugins/kafka-logger.lua:
##########
@@ -89,6 +89,16 @@ local schema = {
},
uniqueItems = true,
},
+ tls = {
+ type = "object",
+ description = "tls config for connecting to kafka brokers",
+ properties = {
+ verify = {
+ type = "boolean",
+ default = false,
+ },
+ },
+ },
Review Comment:
Convention: plugins warn when certificate verification is off, via
`core.utils.check_tls_bool` in `check_schema` (see http-logger.lua:99,
clickhouse-logger.lua:106). Worth adding here:
```lua
local ok, err = core.schema.check(schema, conf)
if not ok then
return nil, err
end
core.utils.check_tls_bool({"tls.verify"}, conf, plugin_name)
```
`get_root_conf` handles dotted paths, and when `tls` is absent it returns
nil so nothing is logged — no noise for the non-TLS case.
##########
apisix/plugins/error-log-logger.lua:
##########
@@ -108,6 +108,16 @@ local metadata_schema = {
},
uniqueItems = true,
},
+ tls = {
+ type = "object",
+ description = "tls config for connecting to kafka brokers",
+ properties = {
+ verify = {
+ type = "boolean",
+ default = false,
+ },
+ },
+ },
Review Comment:
Same `check_tls_bool` suggestion as in kafka-logger, but note it has to go
in the **metadata branch** here:
```lua
if schema_type == core.schema.TYPE_METADATA then
core.utils.check_tls_bool({"kafka.tls.verify"}, conf, plugin_name)
return core.schema.check(metadata_schema, conf)
end
```
Reason: `kafka` only exists in `metadata_schema`, and `check_schema` returns
early for `TYPE_METADATA` before reaching the existing
`check_https`/`check_tls_bool` calls at L191-193. Those calls sit in the
non-metadata branch and operate on the plugin conf, which never contains
`tcp`/`skywalking`/`clickhouse` — so `check_tls_bool({"tcp.tls"})` there never
fires today. That's a pre-existing bug, not yours; just don't copy the
placement.
##########
docs/en/latest/plugins/kafka-logger.md:
##########
@@ -487,3 +489,50 @@ If you have customized the `log_format` in addition to
setting `include_req_body
```
:::
+
+### Log to TLS-Enabled Kafka Brokers
+
+The following example demonstrates how to connect to TLS-enabled Kafka
brokers, such as AWS MSK.
+
+Create a Route with `kafka-logger` and configure the `tls` attribute to
connect to the TLS-enabled Kafka broker:
+
+```shell
+curl "http://127.0.0.1:9180/apisix/admin/routes"; -X PUT \
+ -H "X-API-KEY: ${admin_key}" \
+ -d '{
+ "id": "kafka-logger-tls-route",
+ "uri": "/get",
+ "plugins": {
+ "kafka-logger": {
+ "brokers": [
+ {
+ "host": "kafka.example.com",
+ "port": 9093
+ }
+ ],
+ "kafka_topic": "test2",
+ "key": "key1",
+ "batch_max_size": 1,
+ "tls": {
+ "verify": true
+ }
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "httpbin.org:80": 1
+ },
+ "type": "roundrobin"
+ }
+ }'
+```
+
+When using self-signed certificates, set `tls.verify` to `false` to skip
certificate verification:
Review Comment:
This steers users toward disabling certificate verification. Better to tell
them how to keep it on: point `apisix.ssl.ssl_trusted_certificate` in
`config.yaml` at their CA bundle (it defaults to `system`, so publicly-signed
brokers like AWS MSK work with `verify: true` out of the box — only a private
CA needs this). `verify: false` should be framed as test-environment-only.
Same paragraph at the end of `error-log-logger.md`.
##########
t/plugin/kafka-logger-tls.t:
##########
@@ -0,0 +1,127 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+no_long_string();
+no_root_location();
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+
+ if (!$block->request) {
+ $block->set_value("request", "GET /t");
+ }
+});
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+
+ my $extra_init_by_lua = <<_EOC_;
+ local bp_manager = require("apisix.utils.batch-processor-manager")
+ local core = require("apisix.core")
+ local function log_send_data(entry)
+ local data = type(entry) == "table" and core.json.encode(entry) or
entry
+ core.log.info("send data to kafka: ", data)
+ end
+ local old_add = bp_manager.add_entry
+ bp_manager.add_entry = function(self, conf, entry, max_pending_entries)
+ local ok = old_add(self, conf, entry, max_pending_entries)
+ if ok then
+ log_send_data(entry)
+ end
+ return ok
+ end
+ local old_new = bp_manager.add_entry_to_new_processor
+ bp_manager.add_entry_to_new_processor = function(self, conf, entry, ctx,
func, max_pending_entries)
+ local ok = old_new(self, conf, entry, ctx, func, max_pending_entries)
+ if ok then
+ log_send_data(entry)
+ end
+ return ok
+ end
+_EOC_
+
+ if (!defined $block->extra_init_by_lua) {
+ $block->set_value("extra_init_by_lua", $extra_init_by_lua);
+ }
+});
Review Comment:
Dead code — this preprocessor monkey-patches `bp_manager.add_entry` /
`add_entry_to_new_processor` to log entries, but all three tests in this file
are `check_schema` calls that never reach the batch processor. Leftover from
the integration tests that were removed. Please drop it (and the `GET /t`
default-request preprocessor above can go too if the tests set `--- request`
explicitly).
##########
t/plugin/kafka-logger-tls.t:
##########
@@ -0,0 +1,127 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
Review Comment:
Three pure schema-validation tests don't justify a new test file — it spins
up another nginx instance, and it's inconsistent with the error-log-logger side
of this PR, where the equivalent tests were appended to the existing
`error-log-logger-kafka.t`. Suggest moving these into
`t/plugin/kafka-logger2.t`. If/when real TLS integration tests land, a
dedicated file makes sense.
--
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]
