Closed alieander closed 8 years ago
Hey @alieander! This looks awesome and totally down to have this added. The reason it's not here now is because in our topology we don't have a need for it, but certainly not fundamentally against it.
Code looks great. The only thing that would be nice would be to have some integration style tests that spin up an nsqd, connect to it with TLS, and read and write messages. Do you think that's worth adding and easy to do? How did you test what you have here?
@bschwartz Thanks for taking a look.
I absolutely think it is worth testing. I tested what I have here using our local key
, certificate
, and ca_certificate
information in a simple script that wrote several messages to the queue and read them back off again.
That being said I have not yet made myself familiar with nsq-cluster
. From looking at the tests now, I believe that nsq-cluster
would need to understand how to spin up nsqd
and nsqlookupd
with the correct TLS flags. Additionally I will have to generate some dummy certificates to support those tests to drop in both projects.
Does that sound about right to you?
@alieander Sweet. Yeah that sounds 100% right. I'm not sure you'll need to make any changes to the nsq-cluster
code itself since it should be able to handle extra args just fine (see: https://github.com/wistia/nsq-cluster/blob/master/lib/nsq-cluster/nsqd.rb#L27). But yeah, you'll need to add the right TLS flags in the test code here.
@bschwartz I have added a spec that uses the new TLS functionality. It uses a local set of keys and certs to produce and consume a single message using TLS.
While attempting to write this, I discovered that there are some parts of nsq-cluster that will need slight adjustment to support TLS fully. (eg. checking for nsqd health over https port with certs...)
Hopefully, this spec is covering enough for now. Let me know what you think!
@alieander This looks great! Going to merge, bump the version, and release to ruby gems. Thanks so much for contributing this!
Hope you don't mind me adding you to the list of authors! :)
Reason for change
We would really like the ability to encrypt data that goes onto the queue and be able to connect to queues on our network that are already using SSL/TLS.
This change add's a simple way to pass the needed
ssl_context
to aConsumer
orProducer
and have it create connections usingOpenSSL
.Please let me know what you think! :D
Changes