You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2022/08/24 12:52:32 UTC
[GitHub] [apisix] biubiue opened a new pull request, #7782: feat(kafka-logger): support sasl conf
biubiue opened a new pull request, #7782:
URL: https://github.com/apache/apisix/pull/7782
### Description
<!-- Please include a summary of the change and which issue is fixed. -->
<!-- Please also include relevant motivation and context. -->
Fixes # (issue)
### 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](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a diff in pull request #7782: feat(kafka-logger): support sasl conf
Posted by GitBox <gi...@apache.org>.
tokers commented on code in PR #7782:
URL: https://github.com/apache/apisix/pull/7782#discussion_r964694916
##########
apisix/plugins/kafka-logger.lua:
##########
@@ -51,6 +51,16 @@ local schema = {
},
},
},
+ sasl_config = {
+ type = "object",
+ description = "sasl config",
+ properties = {
+ mechanism = { type = "string", description = "mechanism" },
+ password = { type = "string", description = "password" },
+ user = { type = "string", description = "user" },
Review Comment:
These fields should be required if the sasl config exists?
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] biubiue commented on a diff in pull request #7782: feat(kafka-logger): support sasl conf
Posted by GitBox <gi...@apache.org>.
biubiue commented on code in PR #7782:
URL: https://github.com/apache/apisix/pull/7782#discussion_r965430180
##########
apisix/plugins/kafka-logger.lua:
##########
@@ -51,6 +51,16 @@ local schema = {
},
},
},
+ sasl_config = {
+ type = "object",
+ description = "sasl config",
+ properties = {
+ mechanism = { type = "string", description = "mechanism" },
+ password = { type = "string", description = "password" },
+ user = { type = "string", description = "user" },
Review Comment:
> These fields should be required if the sasl config exists?
I modify these fields ,and mark required fields
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] biubiue commented on a diff in pull request #7782: feat(kafka-logger): support sasl conf
Posted by GitBox <gi...@apache.org>.
biubiue commented on code in PR #7782:
URL: https://github.com/apache/apisix/pull/7782#discussion_r965430033
##########
apisix/plugins/kafka-logger.lua:
##########
@@ -51,6 +51,16 @@ local schema = {
},
},
},
+ sasl_config = {
+ type = "object",
+ description = "sasl config",
+ properties = {
+ mechanism = { type = "string", description = "mechanism" },
+ password = { type = "string", description = "password" },
+ user = { type = "string", description = "user" },
Review Comment:
I modify these fields ,and add required
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] biubiue commented on a diff in pull request #7782: feat(kafka-logger): support sasl conf
Posted by GitBox <gi...@apache.org>.
biubiue commented on code in PR #7782:
URL: https://github.com/apache/apisix/pull/7782#discussion_r965899870
##########
t/plugin/kafka-logger.t:
##########
@@ -592,3 +592,111 @@ qr/partition_id: 2/]
[qr/partition_id: 1/,
qr/partition_id: 0/,
qr/partition_id: 2/]
+
+
+
+=== TEST 20: set route(id: 1)
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "kafka-logger": {
+ "broker_list" :
+ {
+ "127.0.0.1":9092
+ },
+ "kafka_topic" : "test2",
+ "key" : "key1",
+ "timeout" : 1,
+ "batch_max_size": 1,
+ "sasl_config": {
+ "mechanism": "PLAIN",
+ "strategy": "sasl",
+ "user": "admin",
+ "password": "admin-secret"
+ }
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 21: access
+--- request
+GET /hello
+--- response_body
+hello world
+--- wait: 2
Review Comment:
> we can inject the producer.new like:
>
> https://github.com/apache/apisix/blob/2aa5c48d3acc9bea9b61958a09b51ae99c8a528b/t/plugin/tencent-cloud-cls.t#L220-L236
Em, like this?
`=== TEST 20: create producer with sasl_config`
`--- extra_init_by_lua`
    `local producer = require("resty.kafka.producer")`
    `local klogger = require("apisix.plugins.kafka-logger")`
    `producer.new = function(klogger)`
        `if (!klogger.sasl_config) then`
            `ngx.say("create producer without sasl_config")`
            `return producer`
        `end`
        `producer.sasl_config = klogger.sasl_config`
        `ngx.say("create producer with sasl_config")`
        `return producer`
    `end`
`--- request`
`GET /hello`
`--- response_body`
`hello world`
`--- wait: 2`
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #7782: feat(kafka-logger): support sasl conf
Posted by GitBox <gi...@apache.org>.
spacewander commented on PR #7782:
URL: https://github.com/apache/apisix/pull/7782#issuecomment-1241510577
Please make the CI pass, thanks!
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #7782: feat(kafka-logger): support sasl conf
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7782:
URL: https://github.com/apache/apisix/pull/7782#discussion_r955698695
##########
apisix/plugins/kafka-logger.lua:
##########
@@ -205,7 +215,8 @@ function _M.log(conf, ctx)
for host, port in pairs(conf.broker_list) do
local broker = {
host = host,
- port = port
+ port = port,
+ sasl_config = conf.sasl_config or nil
Review Comment:
Would you please add test to cover it?
To make things simple, you can validate the behavior by injecting the producer.new and checking if sasl_config is passed through.
You can learn about the test framework with:
https://github.com/apache/apisix/blob/master/docs/en/latest/internal/testing-framework.md
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] biubiue commented on a diff in pull request #7782: feat(kafka-logger): support sasl conf
Posted by GitBox <gi...@apache.org>.
biubiue commented on code in PR #7782:
URL: https://github.com/apache/apisix/pull/7782#discussion_r955758809
##########
apisix/plugins/kafka-logger.lua:
##########
@@ -205,7 +215,8 @@ function _M.log(conf, ctx)
for host, port in pairs(conf.broker_list) do
local broker = {
host = host,
- port = port
+ port = port,
+ sasl_config = conf.sasl_config or nil
Review Comment:
Ok
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] biubiue commented on a diff in pull request #7782: feat(kafka-logger): support sasl conf
Posted by GitBox <gi...@apache.org>.
biubiue commented on code in PR #7782:
URL: https://github.com/apache/apisix/pull/7782#discussion_r964418667
##########
t/plugin/kafka-logger.t:
##########
@@ -592,3 +592,44 @@ qr/partition_id: 2/]
[qr/partition_id: 1/,
qr/partition_id: 0/,
qr/partition_id: 2/]
+
+
+
+=== TEST 20: user sasl create producer
+--- extra_init_by_lua
+local producer = require("resty.kafka.producer")
+producer.send = function()
+ return true
+end
+--- config
+ location /t {
+ content_by_lua_block {
+ local producer = require "resty.kafka.producer"
Review Comment:
> We should test the kafka-logger plugin directly like other tests, instead of using the producer outside of the plugin.
Em... I maybe not understand [https://github.com/apache/apisix/pull/7782#discussion_r956718372](url) .
OK, I will add test case like others
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a diff in pull request #7782: feat(kafka-logger): support sasl conf
Posted by GitBox <gi...@apache.org>.
tokers commented on code in PR #7782:
URL: https://github.com/apache/apisix/pull/7782#discussion_r965714584
##########
apisix/plugins/kafka-logger.lua:
##########
@@ -51,6 +51,16 @@ local schema = {
},
},
},
+ sasl_config = {
+ type = "object",
+ description = "sasl config",
+ properties = {
+ mechanism = { type = "string", description = "mechanism", default = "PLAIN" },
Review Comment:
OK, `SCRAM-SHA-256` is copied from Confluent docs. No worries at all.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a diff in pull request #7782: feat(kafka-logger): support sasl conf
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #7782:
URL: https://github.com/apache/apisix/pull/7782#discussion_r969483865
##########
t/plugin/kafka-logger.t:
##########
@@ -592,3 +592,118 @@ qr/partition_id: 2/]
[qr/partition_id: 1/,
qr/partition_id: 0/,
qr/partition_id: 2/]
+
+
+
+=== TEST 20: set route(id: 1)
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "kafka-logger": {
+ "broker_list" :
+ {
+ "127.0.0.1":9092
+ },
+ "kafka_topic" : "test2",
+ "key" : "key1",
+ "timeout" : 1,
+ "batch_max_size": 1,
+ "sasl_config": {
+ "mechanism": "PLAIN",
+ "user": "admin",
+ "password": "admin-secret"
+ }
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 21: inject create producer
+--- extra_init_by_lua
+ local producer = require("resty.kafka.producer")
+ producer.new = function(self, broker_list, producer_config, cluster_name)
+ if (#broker_list) then
+ ngx.log(ngx.ERR, "unexpected broker_list length: ", #broker_list)
+ return nil
+ end
+ return producer
Review Comment:
The `producer` should not be returned here, you can see that the original returned is a table.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a diff in pull request #7782: feat(kafka-logger): support sasl conf
Posted by GitBox <gi...@apache.org>.
tokers commented on code in PR #7782:
URL: https://github.com/apache/apisix/pull/7782#discussion_r965433945
##########
apisix/plugins/kafka-logger.lua:
##########
@@ -51,6 +51,16 @@ local schema = {
},
},
},
+ sasl_config = {
+ type = "object",
+ description = "sasl config",
+ properties = {
+ mechanism = { type = "string", description = "mechanism", default = "PLAIN" },
Review Comment:
The `mechanism` should be restricted to only use a few enumerations like `PLAIN`, `SCRAM` and so on.
##########
docs/en/latest/plugins/kafka-logger.md:
##########
@@ -38,6 +38,11 @@ It might take some time to receive the log data. It will be automatically sent a
| Name | Type | Required | Default | Valid values | Description |
| ---------------------- | ------- | -------- | -------------- | --------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
| broker_list | object | True | | | List of Kafka brokers (nodes). |
+| sasl_config | object | False | | | Kafka sasl conf. |
+| sasl_config.mechanism | string | False | | | Kafka mechanism. |
Review Comment:
Add the default value here.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] biubiue commented on a diff in pull request #7782: feat(kafka-logger): support sasl conf
Posted by GitBox <gi...@apache.org>.
biubiue commented on code in PR #7782:
URL: https://github.com/apache/apisix/pull/7782#discussion_r965447660
##########
apisix/plugins/kafka-logger.lua:
##########
@@ -51,6 +51,16 @@ local schema = {
},
},
},
+ sasl_config = {
+ type = "object",
+ description = "sasl config",
+ properties = {
+ mechanism = { type = "string", description = "mechanism", default = "PLAIN" },
Review Comment:
> The `mechanism` should be restricted to only use a few enumerations like `PLAIN`, `SCRAM` and so on.
Just now I review [lua-resty-kafka](https://github.com/doujiang24/lua-resty-kafka) ,
Write this `local MECHANISM_SCRAMSHA256 = "SCRAM-SHA-256" -- to do`, so `SCRAM-SHA-256` maybe not work.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] biubiue commented on a diff in pull request #7782: feat(kafka-logger): support sasl conf
Posted by GitBox <gi...@apache.org>.
biubiue commented on code in PR #7782:
URL: https://github.com/apache/apisix/pull/7782#discussion_r965447497
##########
apisix/plugins/kafka-logger.lua:
##########
@@ -51,6 +51,16 @@ local schema = {
},
},
},
+ sasl_config = {
+ type = "object",
+ description = "sasl config",
+ properties = {
+ mechanism = { type = "string", description = "mechanism", default = "PLAIN" },
Review Comment:
> `SCRAM`
Just now I review [lua-resty-kafka](https://github.com/doujiang24/lua-resty-kafka) ,
Write this `local MECHANISM_SCRAMSHA256 = "SCRAM-SHA-256" -- to do`, so `SCRAM-SHA-256` maybe not work.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] biubiue commented on a diff in pull request #7782: feat(kafka-logger): support sasl conf
Posted by GitBox <gi...@apache.org>.
biubiue commented on code in PR #7782:
URL: https://github.com/apache/apisix/pull/7782#discussion_r979686015
##########
t/plugin/kafka-logger.t:
##########
@@ -592,3 +592,118 @@ qr/partition_id: 2/]
[qr/partition_id: 1/,
qr/partition_id: 0/,
qr/partition_id: 2/]
+
+
+
+=== TEST 20: set route(id: 1)
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "kafka-logger": {
+ "broker_list" :
+ {
+ "127.0.0.1":9092
+ },
+ "kafka_topic" : "test2",
+ "key" : "key1",
+ "timeout" : 1,
+ "batch_max_size": 1,
+ "sasl_config": {
+ "mechanism": "PLAIN",
+ "user": "admin",
+ "password": "admin-secret"
+ }
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 21: inject create producer
+--- extra_init_by_lua
+ local producer = require("resty.kafka.producer")
+ producer.new = function(self, broker_list, producer_config, cluster_name)
+ if (#broker_list) then
+ ngx.log(ngx.ERR, "unexpected broker_list length: ", #broker_list)
+ return nil
+ end
+ return producer
Review Comment:
> The `producer` should not be returned here, you can see that the original returned is a table.
Hi, when inject `producer.new`，it also dependency `client.new`. It means I will need inject `client.new` .
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] biubiue commented on a diff in pull request #7782: feat(kafka-logger): support sasl conf
Posted by GitBox <gi...@apache.org>.
biubiue commented on code in PR #7782:
URL: https://github.com/apache/apisix/pull/7782#discussion_r965447660
##########
apisix/plugins/kafka-logger.lua:
##########
@@ -51,6 +51,16 @@ local schema = {
},
},
},
+ sasl_config = {
+ type = "object",
+ description = "sasl config",
+ properties = {
+ mechanism = { type = "string", description = "mechanism", default = "PLAIN" },
Review Comment:
> The `mechanism` should be restricted to only use a few enumerations like `PLAIN`, `SCRAM` and so on.
Just now I review [lua-resty-kafka](https://github.com/doujiang24/lua-resty-kafka) ,
Write this local MECHANISM_SCRAMSHA256 = "SCRAM-SHA-256" -- to do, so SCRAM-SHA-256 maybe not work.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] biubiue commented on a diff in pull request #7782: feat(kafka-logger): support sasl conf
Posted by GitBox <gi...@apache.org>.
biubiue commented on code in PR #7782:
URL: https://github.com/apache/apisix/pull/7782#discussion_r965436131
##########
docs/en/latest/plugins/kafka-logger.md:
##########
@@ -38,6 +38,11 @@ It might take some time to receive the log data. It will be automatically sent a
| Name | Type | Required | Default | Valid values | Description |
| ---------------------- | ------- | -------- | -------------- | --------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
| broker_list | object | True | | | List of Kafka brokers (nodes). |
+| sasl_config | object | False | | | Kafka sasl conf. |
+| sasl_config.mechanism | string | False | | | Kafka mechanism. |
Review Comment:
> Add the default value here.
ok
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] biubiue commented on a diff in pull request #7782: feat(kafka-logger): support sasl conf
Posted by GitBox <gi...@apache.org>.
biubiue commented on code in PR #7782:
URL: https://github.com/apache/apisix/pull/7782#discussion_r955759131
##########
apisix/plugins/kafka-logger.lua:
##########
@@ -205,7 +215,8 @@ function _M.log(conf, ctx)
for host, port in pairs(conf.broker_list) do
local broker = {
host = host,
- port = port
+ port = port,
+ sasl_config = conf.sasl_config or nil
Review Comment:
> Would you please add test to cover it? To make things simple, you can validate the behavior by injecting the producer.new and checking if sasl_config is passed through.
>
> You can learn about the test framework with: https://github.com/apache/apisix/blob/master/docs/en/latest/internal/testing-framework.md
Ok
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a diff in pull request #7782: feat(kafka-logger): support sasl conf
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #7782:
URL: https://github.com/apache/apisix/pull/7782#discussion_r964481607
##########
t/plugin/kafka-logger.t:
##########
@@ -592,3 +592,111 @@ qr/partition_id: 2/]
[qr/partition_id: 1/,
qr/partition_id: 0/,
qr/partition_id: 2/]
+
+
+
+=== TEST 20: set route(id: 1)
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "kafka-logger": {
+ "broker_list" :
+ {
+ "127.0.0.1":9092
+ },
+ "kafka_topic" : "test2",
+ "key" : "key1",
+ "timeout" : 1,
+ "batch_max_size": 1,
+ "sasl_config": {
+ "mechanism": "PLAIN",
+ "strategy": "sasl",
+ "user": "admin",
+ "password": "admin-secret"
+ }
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 21: access
+--- request
+GET /hello
+--- response_body
+hello world
+--- wait: 2
Review Comment:
we can inject the producer.new like: https://github.com/apache/apisix/blob/2aa5c48d3acc9bea9b61958a09b51ae99c8a528b/t/plugin/tencent-cloud-cls.t#L220-L236
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #7782: feat(kafka-logger): support sasl conf
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7782:
URL: https://github.com/apache/apisix/pull/7782#discussion_r956718372
##########
t/plugin/kafka-logger.t:
##########
@@ -592,3 +592,40 @@ qr/partition_id: 2/]
[qr/partition_id: 1/,
qr/partition_id: 0/,
qr/partition_id: 2/]
+
+
+
+=== TEST 20: sasl simple send
+--- http_config eval: $::HttpConfig
+--- config
+ location /t {
+ content_by_lua_block {
+ local producer = require "resty.kafka.producer"
+ local broker_list = {
+ {
+ host = "127.0.0.1",
+ port = 9092,
+ sasl_config = {
+ mechanism = "PLAIN",
+ strategy = "sasl",
+ user = "admin",
+ password = "admin-secret"
+ }
+ }
+ }
+ local message = "halo world"
+ local p = producer:new(broker_list)
Review Comment:
The test doesn't pass through the code you add.
We can inject a hook or mock into the code. Like:
https://github.com/apache/apisix/blob/b3d4b464045b9bdfed3170cbd7a343ed5c40b24a/t/plugin/zipkin2.t#L35
https://github.com/apache/apisix/blob/b3d4b464045b9bdfed3170cbd7a343ed5c40b24a/t/plugin/opentelemetry.t#L553
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] biubiue commented on pull request #7782: feat(kafka-logger): support sasl conf
Posted by GitBox <gi...@apache.org>.
biubiue commented on PR #7782:
URL: https://github.com/apache/apisix/pull/7782#issuecomment-1241835116
> Please make the CI pass, thanks!
Yeah,I also want to make CI pass, but write test case ,it’s not easy for me. I will do it soon.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #7782: feat(kafka-logger): support sasl conf
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7782:
URL: https://github.com/apache/apisix/pull/7782#discussion_r964411531
##########
t/plugin/kafka-logger.t:
##########
@@ -592,3 +592,44 @@ qr/partition_id: 2/]
[qr/partition_id: 1/,
qr/partition_id: 0/,
qr/partition_id: 2/]
+
+
+
+=== TEST 20: user sasl create producer
+--- extra_init_by_lua
+local producer = require("resty.kafka.producer")
+producer.send = function()
+ return true
+end
+--- config
+ location /t {
+ content_by_lua_block {
+ local producer = require "resty.kafka.producer"
Review Comment:
We should test the kafka-logger plugin directly like other tests, instead of using the producer outside of the plugin.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] biubiue commented on a diff in pull request #7782: feat(kafka-logger): support sasl conf
Posted by GitBox <gi...@apache.org>.
biubiue commented on code in PR #7782:
URL: https://github.com/apache/apisix/pull/7782#discussion_r965436068
##########
apisix/plugins/kafka-logger.lua:
##########
@@ -51,6 +51,16 @@ local schema = {
},
},
},
+ sasl_config = {
+ type = "object",
+ description = "sasl config",
+ properties = {
+ mechanism = { type = "string", description = "mechanism", default = "PLAIN" },
Review Comment:
> The `mechanism` should be restricted to only use a few enumerations like `PLAIN`, `SCRAM` and so on.
ok, I change it
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] biubiue closed pull request #7782: feat(kafka-logger): support sasl conf
Posted by GitBox <gi...@apache.org>.
biubiue closed pull request #7782: feat(kafka-logger): support sasl conf
URL: https://github.com/apache/apisix/pull/7782
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org