feat(kafka-logger): support sasl conf by biubiue · Pull Request #7782 · apache/apisix

biubiue · 2022-08-24T12:52:30Z

Description

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 first)

spacewander · 2022-09-07T05:59:13Z

+--- config
+    location /t {
+        content_by_lua_block {
+            local producer = require "resty.kafka.producer"


We should test the kafka-logger plugin directly like other tests, instead of using the producer outside of the plugin.

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 .
OK, I will add test case like others

tzssangglass · 2022-09-07T07:27:37Z

+=== TEST 21: access
+--- request
+GET /hello
+--- response_body
+hello world
+--- wait: 2


we can inject the producer.new like:

apisix/t/plugin/tencent-cloud-cls.t

Lines 220 to 236 in 2aa5c48

=== TEST 7: verify request

--- extra_init_by_lua

local cls = require("apisix.plugins.tencent-cloud-cls.cls-sdk")

cls.send_to_cls = function(self, logs)

if (#logs ~= 1) then

ngx.log(ngx.ERR, "unexpected logs length: ", #logs)

return

end

return true

end

--- request

GET /opentracing

--- response_body

opentracing

--- error_log

Batch Processor[tencent-cloud-cls] successfully processed the entries

--- wait: 0.5

we can inject the producer.new like:

apisix/t/plugin/tencent-cloud-cls.t

Lines 220 to 236 in 2aa5c48

=== TEST 7: verify request

--- extra_init_by_lua

local cls = require("apisix.plugins.tencent-cloud-cls.cls-sdk")

cls.send_to_cls = function(self, logs)

if (#logs ~= 1) then

ngx.log(ngx.ERR, "unexpected logs length: ", #logs)

return

end

return true

end

--- request

GET /opentracing

--- response_body

opentracing

--- error_log

Batch Processor[tencent-cloud-cls] successfully processed the entries

--- wait: 0.5

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

tokers · 2022-09-07T10:55:24Z

+            properties = {
+                mechanism = { type = "string", description = "mechanism" },
+                password =  { type = "string", description = "password" },
+                user = { type = "string", description = "user" },


These fields should be required if the sasl config exists?

These fields should be required if the sasl config exists?

I modify these fields ,and mark required fields

tokers · 2022-09-08T01:54:41Z

 | ---------------------- | ------- | -------- | -------------- | --------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
 | broker_list            | object  | True     |                |                       | List of Kafka brokers (nodes).                                                                                                                                                                                                                                                                                                                   |
+| sasl_config            | object  | False    |                |                       | Kafka sasl conf.                                                                                                                                                                                                                                                                                                                   |
+| sasl_config.mechanism  | string  | False    |                |                       | Kafka mechanism.                                                                                                                                                                                                                                                                                                                   |


Add the default value here.

Add the default value here.

ok

tokers · 2022-09-08T01:56:52Z

+            type = "object",
+            description = "sasl config",
+            properties = {
+                mechanism = { type = "string", description = "mechanism", default = "PLAIN" },


The mechanism should be restricted to only use a few enumerations like PLAIN, SCRAM and so on.

The mechanism should be restricted to only use a few enumerations like PLAIN, SCRAM and so on.

ok, I change it

The mechanism should be restricted to only use a few enumerations like PLAIN, SCRAM and so on.

Just now I review lua-resty-kafka ,
Write this local MECHANISM_SCRAMSHA256 = "SCRAM-SHA-256" -- to do, so SCRAM-SHA-256 maybe not work.

OK, SCRAM-SHA-256 is copied from Confluent docs. No worries at all.

spacewander · 2022-09-09T05:21:07Z

Please make the CI pass, thanks!

biubiue · 2022-09-09T11:08:28Z

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.

tzssangglass · 2022-09-13T11:09:16Z

+            ngx.log(ngx.ERR, "unexpected broker_list length: ", #broker_list)
+            return nil
+        end
+        return producer


The producer should not be returned here, you can see that the original returned is a table.

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 .

feat(kafka-logger): support sasl conf

40ddca3

biubiue mentioned this pull request Aug 24, 2022

help request: add test case #7783

Closed

biubiue added 2 commits August 25, 2022 15:01

feat(kafka-logger): doc add sasl conf

21be757

fix(kafka-logger): doc

fad3df6

spacewander reviewed Aug 26, 2022

View reviewed changes

Comment thread apisix/plugins/kafka-logger.lua

biubiue added 7 commits August 28, 2022 11:22

feat(kafka-logger): test case

de82ec3

feat(kafka-logger): modify test case code style

33cd466

fix(kafka-logger): test case code style

7e1db99

fix(kafka-logger): test case code style

ebeebc8

fix(kafka-logger): test case code style

cd68da2

fix(kafka-logger): test case code style

0104c5a

Update kafka-logger.t

503011c

spacewander reviewed Aug 28, 2022

View reviewed changes

Comment thread t/plugin/kafka-logger.t Outdated

biubiue added 6 commits September 1, 2022 10:20

fix(kafka-logger): test case

18358aa

fix(kafka-logger): test case

3812c9c

Update kafka-logger.t

c8baf35

Update kafka-logger.t

d348987

Update kafka-logger.t

72f0d06

fix(kafka-logger): test case

5b7dd8c

biubiue requested a review from spacewander September 6, 2022 09:54

biubiue added 5 commits September 6, 2022 18:04

fix(kafka-logger): try other actions

9f1817a

Update kafka-logger.t

588bb0f

Update kafka-logger.t

3a90b5d

Update kafka-logger.t

d10ea12

Update kafka-logger.t

70054aa

spacewander reviewed Sep 7, 2022

View reviewed changes

Update kafka-logger.t

35a581b

tzssangglass reviewed Sep 7, 2022

View reviewed changes

tokers reviewed Sep 7, 2022

View reviewed changes

fix(kafka-logger): modify sasl_config properties

9d9f005

tokers reviewed Sep 8, 2022

View reviewed changes

Update kafka-logger.md

8c41473

biubiue added 4 commits September 9, 2022 17:43

Update kafka-logger.t

4e1cc34

Update kafka-logger.lua

ddde433

Update kafka-logger.md

0444c00

Update kafka-logger.lua

cc90a47

fix(kafka-logger): test case

290070c

tzssangglass reviewed Sep 13, 2022

View reviewed changes

biubiue added 4 commits September 26, 2022 16:23

fix(kafka-logger): test case

1481240

test case

6968d48

test case

11d8dc6

test case

8ca60db

biubiue closed this Sep 27, 2022

tzssangglass mentioned this pull request Oct 9, 2022

feat: As a user, I want to support sasl_config in kafka-logger plugin, so that we can send logs to brokers that enable authentication #8046

Closed

starsz mentioned this pull request Oct 10, 2022

feat(kafka-logger): support sasl config in brokers #8050

Merged

5 tasks

	=== TEST 7: verify request
	--- extra_init_by_lua
	local cls = require("apisix.plugins.tencent-cloud-cls.cls-sdk")
	cls.send_to_cls = function(self, logs)
	if (#logs ~= 1) then
	ngx.log(ngx.ERR, "unexpected logs length: ", #logs)
	return
	end
	return true
	end
	--- request
	GET /opentracing
	--- response_body
	opentracing
	--- error_log
	Batch Processor[tencent-cloud-cls] successfully processed the entries
	--- wait: 0.5

Uh oh!

Conversation

biubiue commented Aug 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

biubiue Sep 8, 2022 • 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!

spacewander commented Sep 9, 2022

Uh oh!

biubiue commented Sep 9, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

biubiue commented Aug 24, 2022 •

edited

Loading

biubiue Sep 8, 2022 •

edited

Loading