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/05/13 17:49:21 UTC
[GitHub] [apisix] bzp2010 opened a new pull request, #7046: feat(pubsub): support kafka tls and sasl/plain auth
bzp2010 opened a new pull request, #7046:
URL: https://github.com/apache/apisix/pull/7046
### Description
Part of #6995 to implement TLS and SASL/PLAIN authentication support for kafka.
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [x] I have updated the documentation to reflect this change
- [x] 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] spacewander merged pull request #7046: feat(pubsub): support kafka tls and sasl/plain auth
Posted by GitBox <gi...@apache.org>.
spacewander merged PR #7046:
URL: https://github.com/apache/apisix/pull/7046
--
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] bzp2010 commented on a diff in pull request #7046: feat(pubsub): support kafka tls and sasl/plain auth
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #7046:
URL: https://github.com/apache/apisix/pull/7046#discussion_r874410084
##########
apisix/upstream.lua:
##########
@@ -435,7 +435,7 @@ local function check_upstream_conf(in_dp, conf)
end
end
- if conf.tls then
+ if conf.tls and conf.tls.client_cert and conf.tls.client_key then
Review Comment:
Yes, it's enough, we ensure `client_cert` and `client_key` both exist by jsonschema's dependencies. Any one of them separate exist is forbidden.
https://github.com/apache/apisix/blob/99bced123eb4d63e66be191b68a6da17b8aac150/apisix/schema_def.lua#L416-L419
--
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] bzp2010 commented on a diff in pull request #7046: feat(pubsub): support kafka tls and sasl/plain auth
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #7046:
URL: https://github.com/apache/apisix/pull/7046#discussion_r874342533
##########
apisix/plugins/kafka-proxy.lua:
##########
@@ -0,0 +1,63 @@
+--
+-- 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.
+--
+local core = require("apisix.core")
+
+
+local schema = {
+ type = "object",
+ properties = {
+ sasl = {
+ type = "object",
+ properties = {
+ username = {
+ type = "string",
+ default = "",
Review Comment:
reference these issue and PR, kafka supports unlimited length of username and password
https://github.com/edenhill/librdkafka/issues/1691
https://github.com/edenhill/librdkafka/commit/ff629e18e2431f1f52fedf5e411278c674774442
And our dependency `lua-resty-kafka` also does not enforce a limit on its length.
--
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 pull request #7046: feat(pubsub): support kafka tls and sasl/plain auth
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on PR #7046:
URL: https://github.com/apache/apisix/pull/7046#issuecomment-1128375401
LGTM
--
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 #7046: feat(pubsub): support kafka tls and sasl/plain auth
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7046:
URL: https://github.com/apache/apisix/pull/7046#discussion_r874397296
##########
apisix/plugins/kafka-proxy.lua:
##########
@@ -0,0 +1,63 @@
+--
+-- 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.
+--
+local core = require("apisix.core")
+
+
+local schema = {
+ type = "object",
+ properties = {
+ sasl = {
+ type = "object",
+ properties = {
+ username = {
+ type = "string",
+ default = "",
+ },
+ password = {
+ type = "string",
+ default = "",
Review Comment:
We can remove the default if these fields are 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] bzp2010 commented on a diff in pull request #7046: feat(pubsub): support kafka tls and sasl/plain auth
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #7046:
URL: https://github.com/apache/apisix/pull/7046#discussion_r874410084
##########
apisix/upstream.lua:
##########
@@ -435,7 +435,7 @@ local function check_upstream_conf(in_dp, conf)
end
end
- if conf.tls then
+ if conf.tls and conf.tls.client_cert and conf.tls.client_key then
Review Comment:
Yes, it's enough, we ensure `client_cert` and `client_key` both exist by jsonschema's dependencies. Any one of them separate existence is forbidden.
https://github.com/apache/apisix/blob/99bced123eb4d63e66be191b68a6da17b8aac150/apisix/schema_def.lua#L416-L419
--
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] bzp2010 commented on a diff in pull request #7046: feat(pubsub): support kafka tls and sasl/plain auth
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #7046:
URL: https://github.com/apache/apisix/pull/7046#discussion_r873888895
##########
.github/workflows/centos7-ci.yml:
##########
@@ -79,6 +79,8 @@ jobs:
- name: Run other docker containers for test
run: |
+ # generating SSL certificates for Kafka
+ keytool -genkeypair -keyalg RSA -dname "CN=127.0.0.1" -alias 127.0.0.1 -keystore ./ci/pod/kafka/kafka-server/selfsigned.jks -validity 365 -keysize 2048 -storepass changeit
Review Comment:
**First make sure that the certificate exists for docker-compose to start kafka.**
##########
ci/linux_openresty_common_runner.sh:
##########
@@ -21,6 +21,9 @@
before_install() {
sudo cpanm --notest Test::Nginx >build.log 2>&1 || (cat build.log && exit 1)
+ # generating SSL certificates for Kafka
+ keytool -genkeypair -keyalg RSA -dname "CN=127.0.0.1" -alias 127.0.0.1 -keystore ./ci/pod/kafka/kafka-server/selfsigned.jks -validity 365 -keysize 2048 -storepass changeit
+
Review Comment:
ditto
--
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] soulbird commented on a diff in pull request #7046: feat(pubsub): support kafka tls and sasl/plain auth
Posted by GitBox <gi...@apache.org>.
soulbird commented on code in PR #7046:
URL: https://github.com/apache/apisix/pull/7046#discussion_r873549504
##########
.github/workflows/centos7-ci.yml:
##########
@@ -79,6 +79,8 @@ jobs:
- name: Run other docker containers for test
run: |
+ # generating SSL certificates for Kafka
+ keytool -genkeypair -keyalg RSA -dname "CN=127.0.0.1" -alias 127.0.0.1 -keystore ./ci/pod/kafka/kafka-server/selfsigned.jks -validity 365 -keysize 2048 -storepass changeit
Review Comment:
Is it more appropriate to put it in the `linux-ci-init-service.sh` script ?
--
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] soulbird commented on a diff in pull request #7046: feat(pubsub): support kafka tls and sasl/plain auth
Posted by GitBox <gi...@apache.org>.
soulbird commented on code in PR #7046:
URL: https://github.com/apache/apisix/pull/7046#discussion_r873557339
##########
ci/linux_openresty_common_runner.sh:
##########
@@ -21,6 +21,9 @@
before_install() {
sudo cpanm --notest Test::Nginx >build.log 2>&1 || (cat build.log && exit 1)
+ # generating SSL certificates for Kafka
+ keytool -genkeypair -keyalg RSA -dname "CN=127.0.0.1" -alias 127.0.0.1 -keystore ./ci/pod/kafka/kafka-server/selfsigned.jks -validity 365 -keysize 2048 -storepass changeit
+
Review Comment:
so, add this to `linux-ci-init-service.sh` script ?
--
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 #7046: feat(pubsub): support kafka tls and sasl/plain auth
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7046:
URL: https://github.com/apache/apisix/pull/7046#discussion_r874393154
##########
apisix/upstream.lua:
##########
@@ -435,7 +435,7 @@ local function check_upstream_conf(in_dp, conf)
end
end
- if conf.tls then
+ if conf.tls and conf.tls.client_cert and conf.tls.client_key then
Review Comment:
```suggestion
if conf.tls and conf.tls.client_cert then
```
is enough?
--
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] bzp2010 commented on pull request #7046: feat(pubsub): support kafka tls and sasl/plain auth
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on PR #7046:
URL: https://github.com/apache/apisix/pull/7046#issuecomment-1126309791
### Update
This is the part after #7032 that is currently in ready state, and when #7032 is merged, it is ready to start the review.
--
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] bzp2010 commented on a diff in pull request #7046: feat(pubsub): support kafka tls and sasl/plain auth
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #7046:
URL: https://github.com/apache/apisix/pull/7046#discussion_r873888895
##########
.github/workflows/centos7-ci.yml:
##########
@@ -79,6 +79,8 @@ jobs:
- name: Run other docker containers for test
run: |
+ # generating SSL certificates for Kafka
+ keytool -genkeypair -keyalg RSA -dname "CN=127.0.0.1" -alias 127.0.0.1 -keystore ./ci/pod/kafka/kafka-server/selfsigned.jks -validity 365 -keysize 2048 -storepass changeit
Review Comment:
**First make sure that the certificate exists for docker-compose to start kafka. If the certificate does not exist then the kafka container will crash.**
--
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 #7046: feat(pubsub): support kafka tls and sasl/plain auth
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #7046:
URL: https://github.com/apache/apisix/pull/7046#discussion_r874332677
##########
apisix/plugins/kafka-proxy.lua:
##########
@@ -0,0 +1,63 @@
+--
+-- 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.
+--
+local core = require("apisix.core")
+
+
+local schema = {
+ type = "object",
+ properties = {
+ sasl = {
+ type = "object",
+ properties = {
+ username = {
+ type = "string",
+ default = "",
Review Comment:
has `minLength`, `maxLength` limit?
--
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] bzp2010 commented on a diff in pull request #7046: feat(pubsub): support kafka tls and sasl/plain auth
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #7046:
URL: https://github.com/apache/apisix/pull/7046#discussion_r874426552
##########
apisix/upstream.lua:
##########
@@ -435,7 +435,7 @@ local function check_upstream_conf(in_dp, conf)
end
end
- if conf.tls then
+ if conf.tls and conf.tls.client_cert and conf.tls.client_key then
Review Comment:
removed
--
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] bzp2010 commented on a diff in pull request #7046: feat(pubsub): support kafka tls and sasl/plain auth
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #7046:
URL: https://github.com/apache/apisix/pull/7046#discussion_r874411087
##########
apisix/plugins/kafka-proxy.lua:
##########
@@ -0,0 +1,63 @@
+--
+-- 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.
+--
+local core = require("apisix.core")
+
+
+local schema = {
+ type = "object",
+ properties = {
+ sasl = {
+ type = "object",
+ properties = {
+ username = {
+ type = "string",
+ default = "",
+ },
+ password = {
+ type = "string",
+ default = "",
Review Comment:
removed
--
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