oleksiyk / kafka

Apache Kafka 0.9 client for Node
MIT License
297 stars 85 forks source link

Do not overwrite user supplied ssl certificates and keys #149

Closed wdullaer closed 7 years ago

wdullaer commented 7 years ago

I changed the way client.js sets default values for the ssl keys. If a user has supplied an ssl object, no default values for certStr, certFile, keyStr and keyFile will be written. This avoids the case where a user tries to set certStr equal to KAFKA_CLIENT_CERT (which is the default on the heroku platform) and no-kafka then still populates certFile and tries to load things from disk.

The edge case with this implementation is that if a user supplies an ssl config object, but no certs or keys no default values will be picked. However I think this is much less likely than the current case. (so better usability and less tickets for you)

I also added two tests for this behaviour.

oleksiyk commented 7 years ago

Looks like this breaks existing behaviour

wdullaer commented 7 years ago

The tests which are not passing are those I wrote myself, all of your tests still pass. Or is there some other behaviour you're worried about.

I'm still working on making the tests less flaky (I don't have a setup which emulates all your options, so I have to rely on the travis)

oleksiyk commented 7 years ago

How about slightly different approach, allow KAFKA_CLIENT_CERT to be either a filename or a certificate string, same for KAFKA_CLIENT_CERT_KEY. Then test for file existence in Client.init if it is a file - read certificate from file, otherwise treat it as certificate string.

wdullaer commented 7 years ago

That's more elegant, but your options name doesn't make too much sense then (certFile), which is why I took this approach.

Let me know what you want, and I'll push the necessary changes to this branch.

oleksiyk commented 7 years ago

How about #150 ?

wdullaer commented 7 years ago

150 looks good to me 👍

oleksiyk commented 7 years ago

published as 3.1.0