doujiang24 / lua-resty-kafka

Lua kafka client driver for the Openresty based on the cosocket API
BSD 3-Clause "New" or "Revised" License
801 stars 274 forks source link

Add TLS support to kafka broker connections #91

Closed jeremyjpj0916 closed 4 years ago

jeremyjpj0916 commented 4 years ago

Add optional support for TLS and TLS verification for Kafka connections.

References: https://github.com/openresty/lua-nginx-module#tcpsocksslhandshake https://github.com/Kong/kong/blob/master/kong/plugins/http-log/handler.lua#L78 https://github.com/ledgetech/lua-resty-http/blob/master/lib/resty/http.lua#L174

@doujiang24 What do you think? Can you push out the new version too to luarocks if you agree this is good to go?

jeremyjpj0916 commented 4 years ago

Also worth noting your build scripting is a bit messed up for travis CI:

install.1
0.67s$ git clone https://github.com/openresty/test-nginx.git ../test-nginx
install.2
9.87s$ if [ ! -f download-cache/kafka_$SCALA_VER-$KAFKA_VER.tgz ]; then wget -P download-cache http://mirrors.tuna.tsinghua.edu.cn/apache/kafka/$KAFKA_VER/kafka_$SCALA_VER-$KAFKA_VER.tgz;fi
1.15s$ if [ ! -f download-cache/apache-zookeeper-$ZK_VER-bin.tar.gz ]; then wget -P download-cache https://mirrors.tuna.tsinghua.edu.cn/apache/zookeeper/zookeeper-ZK_VER/apache-zookeeper-$ZK_VER-bin.tar.gz;fi
--2020-04-16 21:29:00--  https://mirrors.tuna.tsinghua.edu.cn/apache/zookeeper/zookeeper-ZK_VER/apache-zookeeper-3.5.7-bin.tar.gz
Resolving mirrors.tuna.tsinghua.edu.cn (mirrors.tuna.tsinghua.edu.cn)... 101.6.8.193, 2402:f000:1:408:8100::1
Connecting to mirrors.tuna.tsinghua.edu.cn (mirrors.tuna.tsinghua.edu.cn)|101.6.8.193|:443... connected.
HTTP request sent, awaiting response... 404 Not Found
2020-04-16 21:29:01 ERROR 404: Not Found.
The command "if [ ! -f download-cache/apache-zookeeper-$ZK_VER-bin.tar.gz ]; then wget -P download-cache https://mirrors.tuna.tsinghua.edu.cn/apache/zookeeper/zookeeper-ZK_VER/apache-zookeeper-$ZK_VER-bin.tar.gz;fi" failed and exited with 8 during .
Your build has been stopped.
doujiang24 commented 4 years ago

Also worth noting your build scripting is a bit messed up for travis CI:

install.1
0.67s$ git clone https://github.com/openresty/test-nginx.git ../test-nginx
install.2
9.87s$ if [ ! -f download-cache/kafka_$SCALA_VER-$KAFKA_VER.tgz ]; then wget -P download-cache http://mirrors.tuna.tsinghua.edu.cn/apache/kafka/$KAFKA_VER/kafka_$SCALA_VER-$KAFKA_VER.tgz;fi
1.15s$ if [ ! -f download-cache/apache-zookeeper-$ZK_VER-bin.tar.gz ]; then wget -P download-cache https://mirrors.tuna.tsinghua.edu.cn/apache/zookeeper/zookeeper-ZK_VER/apache-zookeeper-$ZK_VER-bin.tar.gz;fi
--2020-04-16 21:29:00--  https://mirrors.tuna.tsinghua.edu.cn/apache/zookeeper/zookeeper-ZK_VER/apache-zookeeper-3.5.7-bin.tar.gz
Resolving mirrors.tuna.tsinghua.edu.cn (mirrors.tuna.tsinghua.edu.cn)... 101.6.8.193, 2402:f000:1:408:8100::1
Connecting to mirrors.tuna.tsinghua.edu.cn (mirrors.tuna.tsinghua.edu.cn)|101.6.8.193|:443... connected.
HTTP request sent, awaiting response... 404 Not Found
2020-04-16 21:29:01 ERROR 404: Not Found.
The command "if [ ! -f download-cache/apache-zookeeper-$ZK_VER-bin.tar.gz ]; then wget -P download-cache https://mirrors.tuna.tsinghua.edu.cn/apache/zookeeper/zookeeper-ZK_VER/apache-zookeeper-$ZK_VER-bin.tar.gz;fi" failed and exited with 8 during .
Your build has been stopped.

seems the bin file of zookeeper is deleted by the maintainer, can you help to replace it by other providers, like the following one? thanks https://gist.github.com/estliberitas/b2c004f7447ed6fa3248

also, we should add some tests for the new feature.

jeremyjpj0916 commented 4 years ago

Added these changes for where we pull kafka + zookeeper down from, went straight to the source(apache). So we should be good to iterate on versions from their official repo's:

  - if [ ! -f download-cache/kafka_$SCALA_VER-$KAFKA_VER.tgz ]; then wget -P download-cache https://archive.apache.org/dist/kafka/$KAFKA_VER/kafka_$SCALA_VER-$KAFKA_VER.tgz;fi
  - if [ ! -f download-cache/apache-zookeeper-$ZK_VER-bin.tar.gz ]; then wget -P download-cache https://downloads.apache.org/zookeeper/zookeeper-$ZK_VER/apache-zookeeper-$ZK_VER-bin.tar.gz;fi
jeremyjpj0916 commented 4 years ago

Hmm added testcases @doujiang24 as you suggested but I see connection refused:

#   Failed test 'TEST 2: simple ssl send - pattern "[error]" should not match any line in error.log but matches line "2020/04/17 05:29:25 [error] 16073\#0: *1 connect() failed (111: Connection refused), client: 127.0.0.1, server: localhost, request: \"GET /t HTTP/1.1\", host: \"localhost\"" (req 0)
#   Failed test 'TEST 2: simple ssl fetch - pattern "[error]" should not match any line in error.log but matches line "2020/04/17 05:29:24 [error] 16052\#0: *1 connect() failed (111: Connection refused), client: 127.0.0.1, server: localhost, request: \"GET /t HTTP/1.1\", host: \"localhost\"" (req 0)

Do we need to explicitly do anything to enable TLS + port 9093 for Kafka? I left ssl_verify off for now with respect for these tests, figured it would not be necessary as long as we could establish an ssl connection with the hosts on port 9093.

I guess we need to setup a keystore for kafka at the minimum with ssl.client.auth none. Seems to be outlined here: https://docs.cloudera.com/HDPDocuments/HDP3/HDP-3.1.5/configuring-wire-encryption/content/configuring_the_kafka_broker.html

So edit server.properties for both plaintext and ssl listening port(i wonder if its already defaulted to that) + keystore and password and that should do it.

doujiang24 commented 4 years ago

@jeremyjpj0916 I haven't used Kafka TLS, but I think you are right, you can follow the guide you have listed.

jeremyjpj0916 commented 4 years ago

@doujiang24 what is this line doing here:

https://github.com/doujiang24/lua-resty-kafka/blob/master/.travis.yml#L44

- sudo sed -i '$ahost\.name=127.0.0.1' /usr/local/kafka/config/server.properties

https://github.com/apache/kafka/blob/2.4.0/config/server.properties ?

I will sed overwrite the listeners to support tls but I don't know what the above command is really doing. is $ahost defined anywhere?

Edit - ohhh, maybe its replacing host.name occurrences with 127.0.0.1 ? But it never comments anything out, so I honestly don't think its doing anything at the moment.

wanghuizzz commented 4 years ago
- sudo sed -i '$ahost\.name=127.0.0.1' /usr/local/kafka/config/server.properties

This command will append host.name=127.0.0.1 to the end of file.

jeremyjpj0916 commented 4 years ago

This command will append host.name=127.0.0.1 to the end of file.

@wanghuizzz

Ahh thx for dropping in and letting me know, will add it back to the travis build incase its needed.

jeremyjpj0916 commented 4 years ago

Hmm now the build pauses asking keytool questions, my local does not behave like that for the same keytool command...

$ sudo keytool -genkeypair -keyalg RSA -dname "CN=127.0.0.1" -alias 127.0.0.1 -keystore selfsigned.jks -validity 365 -keysize 2048 -storepass changeit
Enter key password for <127.0.0.1>
    (RETURN if same as keystore password):  

vs

☁  kafkatests  keytool -genkeypair -keyalg RSA -dname "CN=127.0.0.1" -alias 127.0.0.1 -keystore selfsigned.jks -validity 365 -keysize 2048 -storepass changeit
☁  kafkatests  ls
selfsigned.jks     server.properties1

lets see if I can get around it with the yes package.

jeremyjpj0916 commented 4 years ago

Welp added logic I thought would do the trick, seems it still fails tests:

https://travis-ci.com/github/doujiang24/lua-resty-kafka/builds/160686856

I do not know why. If anyone has ideas let me know.

3.89s$ /usr/local/kafka/bin/kafka-topics.sh --create --zookeeper localhost:2181  --replication-factor 1 --partitions 2 --topic test
Error while executing topic command : Replication factor: 1 larger than available brokers: 0.
[2020-04-17 07:45:57,442] ERROR org.apache.kafka.common.errors.InvalidReplicationFactorException: Replication factor: 1 larger than available brokers: 0.
 (kafka.admin.TopicCommand$)
The command "/usr/local/kafka/bin/kafka-topics.sh --create --zookeeper localhost:2181  --replication-factor 1 --partitions 2 --topic test" exited with 1.

Makes me think the Kafka was unable to Start but the lines above:

The command "sudo /usr/local/zookeeper/bin/zkServer.sh start" exited with 0.
0.40s$ sudo /usr/local/kafka/bin/kafka-server-start.sh  -daemon /usr/local/kafka/config/server.properties
The command "sudo /usr/local/kafka/bin/kafka-server-start.sh  -daemon /usr/local/kafka/config/server.properties" exited with 0.
1.03s$ sleep 1

Seem like they worked, so my server /usr/local/kafka/config/server.properties file should be fine I would think?

doujiang24 commented 4 years ago

@jeremyjpj0916 maybe it need more sleep time here: https://github.com/doujiang24/lua-resty-kafka/pull/91/files#diff-354f30a63fb0907d4ad57269548329e3R53

jeremyjpj0916 commented 4 years ago

@doujiang24 you were right, extending the sleep to 5 gave it enough time it seems to start with the extra TLS configurations:

https://travis-ci.com/github/doujiang24/lua-resty-kafka/builds/160742674 , now errors for other odd reasons I am unsure about. I have never liked unit tests haha, hard to get them right 😆 .

doujiang24 commented 4 years ago

@wanghuizzz do you have time to take a look at the travis failure thing?

wanghuizzz commented 4 years ago

@jeremyjpj0916

        local broker_list = {
           { host = "$TEST_NGINX_KAFKA_HOST", port = $TEST_NGINX_KAFKA_SSL_PORT, ssl = true },
       }

ssl option should be placed here, not in broker list.

local p = producer:new(broker_list, { ssl = true })
jeremyjpj0916 commented 4 years ago

@wanghuizzz good catch! totally didn't realize my misplace in new variable there haha. Lets see what travisCI says next.

jeremyjpj0916 commented 4 years ago

@doujiang24 all righty! Tests passed and I squashed all commits into 1 to clean things up for yah. Hope to merge soon and have you get luarocks up to date. Thanks to both you and @wanghuizzz for the assistance, hope you are both doing okay during these crazy days.

doujiang24 commented 4 years ago

@jeremyjpj0916 @wanghuizzz Thanks, already uploaded to luarocks and opm.

jeremyjpj0916 commented 4 years ago

@doujiang24 Thanks for handling this PR quickly and getting package managers updated. Helps unblock me on a kafka logging task that needed to be done over ssl. Take care!