Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 45 commits into from
Oct 11, 2022
Merged

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

merged 45 commits into from
Oct 11, 2022

Conversation

starsz
Copy link
Contributor

@starsz starsz commented Oct 9, 2022 •
edited
Loading

Description

Fixes #8046

Thanks to @biubiue and the original PR #7782.

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)

@biubiue
feat(kafka-logger): support sasl conf
40ddca3
@biubiue
feat(kafka-logger): doc add sasl conf
21be757
@biubiue
fix(kafka-logger): doc
fad3df6
@biubiue
feat(kafka-logger): test case
de82ec3
@biubiue
feat(kafka-logger): modify test case code style
33cd466
@biubiue
fix(kafka-logger): test case code style
7e1db99
@biubiue
fix(kafka-logger): test case code style
ebeebc8
@biubiue
fix(kafka-logger): test case code style
cd68da2
@biubiue
fix(kafka-logger): test case code style
0104c5a
@biubiue
Update kafka-logger.t
503011c
@biubiue
fix(kafka-logger): test case
18358aa
@biubiue
fix(kafka-logger): test case
3812c9c
@biubiue
Update kafka-logger.t
c8baf35
@biubiue
Update kafka-logger.t
d348987
@biubiue
Update kafka-logger.t
72f0d06
@biubiue
fix(kafka-logger): test case
5b7dd8c
@biubiue
fix(kafka-logger): try other actions
9f1817a
@biubiue
Update kafka-logger.t
588bb0f
@biubiue
Update kafka-logger.t
3a90b5d
@biubiue
Update kafka-logger.t
d10ea12
@biubiue
Update kafka-logger.t
70054aa
@biubiue
Update kafka-logger.t
35a581b
@biubiue
fix(kafka-logger): modify sasl_config properties
9d9f005
@biubiue
Update kafka-logger.md
8c41473
@biubiue
Update kafka-logger.t
4e1cc34
@biubiue
Update kafka-logger.lua
ddde433
@biubiue
Update kafka-logger.md
0444c00
@biubiue
Update kafka-logger.lua
cc90a47
@biubiue
fix(kafka-logger): test case
290070c
@biubiue
fix(kafka-logger): test case
1481240
@biubiue
Copy link
Contributor

biubiue commented Oct 9, 2022

I think it is not good to put sasl_config under borkers node, if more than one borker node, you need to configure duplicate sasl_config

@biubiue
Copy link
Contributor

biubiue commented Oct 9, 2022

I'm glad you continue this feat PR

@starsz
Copy link
Contributor Author

starsz commented Oct 10, 2022

I think it is not good to put sasl_config under borkers node, if more than one borker node, you need to configure duplicate sasl_config

Hi, @biubiue .I think we can put the sasl_config under brokers first so that we can support configuring different sasl_config under different brokers.

Then we can add sasl_config besides sasl_config as global sasl_config, if the broker doesn't configure sasl_config then it will use global sasl_config instead.

@starsz starsz marked this pull request as ready for review October 10, 2022 05:55
spacewander
spacewander reviewed Oct 10, 2022
View reviewed changes
docs/en/latest/plugins/kafka-logger.md Outdated Show resolved Hide resolved
docs/en/latest/plugins/kafka-logger.md Outdated Show resolved Hide resolved
docs/zh/latest/plugins/kafka-logger.md Outdated Show resolved Hide resolved
docs/zh/latest/plugins/kafka-logger.md Outdated Show resolved Hide resolved
apisix/plugins/kafka-logger.lua Outdated
properties = {
mechanism = {
type = "string",
default = "PLAIN"
Copy link
Member

Choose a reason for hiding this comment

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

We can use enum to make sure the passing mechanism is legal? Although only PLAIN is currently supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion. Let me add the enum and add a test case to cover it.

@tzssangglass
Copy link
Member

LGTM

starsz and others added 5 commits October 10, 2022 14:33
@starsz @spacewander
Update docs/en/latest/plugins/kafka-logger.md
23dc1b1 
Co-authored-by: 罗泽轩 <spacewanderlzx@gmail.com>
@starsz @spacewander
Update docs/en/latest/plugins/kafka-logger.md
1825dc8 
Co-authored-by: 罗泽轩 <spacewanderlzx@gmail.com>
@starsz @spacewander
Update docs/zh/latest/plugins/kafka-logger.md
d0e53fd 
Co-authored-by: 罗泽轩 <spacewanderlzx@gmail.com>
@starsz @spacewander
Update docs/zh/latest/plugins/kafka-logger.md
7cec61a 
Co-authored-by: 罗泽轩 <spacewanderlzx@gmail.com>
@starsz
chore: add enum for mechanism
8f08476
tzssangglass
tzssangglass previously approved these changes Oct 10, 2022
View reviewed changes
soulbird
soulbird reviewed Oct 11, 2022
View reviewed changes
ci/pod/docker-compose.plugin.yml Outdated
volumes:
- ./ci/pod/kafka/kafka-server/kafka_jaas.conf:/opt/bitnami/kafka/config/kafka_jaas.conf:ro
- ./ci/pod/kafka/kafka-server/selfsigned.jks:/opt/bitnami/kafka/config/certs/kafka.keystore.jks:ro
- ./ci/pod/kafka/kafka-server/selfsigned.jks:/opt/bitnami/kafka/config/certs/kafka.truststore.jks:ro
Copy link
Contributor

Choose a reason for hiding this comment

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

The selfsigned.jks file is only generated when the last test is executed, look: https://github.com/apache/apisix/blob/master/.github/workflows/build.yml#L117-L120, and should not exist when the plugin test is executed. Why can CI still pass, very puzzled.....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry.My fault.

Why can CI still pass, very puzzled....

The file or directory does not need to exist on the Docker host already. It is created on demand if it does not yet exist.
So it wouldn't influence the CI.

refer: https://docs.docker.com/storage/bind-mounts/

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it ~~~

@spacewander spacewander merged commit c201d72 into apache:master Oct 11, 2022
@starsz starsz deleted the kafka_sasl branch October 11, 2022 05:45
Liu-Junlin pushed a commit to Liu-Junlin/apisix that referenced this pull request Nov 4, 2022
@starsz @spacewander
@biubiue @Liu-Junlin
feat(kafka-logger): support sasl config in brokers (apache#8050)
017c311 
Co-authored-by: 罗泽轩 <spacewanderlzx@gmail.com>
Co-authored-by: biubiue <hichuanbiao@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soulbird soulbird soulbird approved these changes

@spacewander spacewander spacewander approved these changes

@tzssangglass tzssangglass tzssangglass left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@starsz @biubiue @tzssangglass @spacewander @soulbird