kaffe
kaffe copied to clipboard
Support ssl configuration from files
This PR refactors the configuration for endpoints and SSL to support other Erlang SSL options, most importantly getting certs and keys from files.
It's compatible with the current code, except that instead of having the heroku_kafka_env: true
option, it has an optional ssl
option, and the KAFKA_CLIENT_CERT
and KAFKA_CLIENT_CERT_KEY
environment vars are added to the ssl
config if present. It also fully supports the KAFKA_TRUSTED_CERT
environment var.
If the KAFKA_URL
environment var is set, it reads the endpoints from there, overriding anything set in the endpoints
config option.
This PR includes a full set of unit tests. I haven't tested it with Heroku Kafka, as we are running our own Kafka servers.
@reachfh Thank you very much! Kaffe could really use some SSL refactoring.
I haven't had time to look at your PR in any depth, but I did note your mention of replacing heroku_kafka_env
with ssl
. While this might be a more logical name, breaking backward compatibility is something we want to avoid. Can you make both options available?
Also, we're not ready to add a mocking framework dependency in Kaffe. Would you remove the meck
dependency and rewrite your test without that?
The way that that it works in this patch is that if the environment vars exist, then they are used, so the heroku_kafka_env
flag is not needed; the existence of the var is the flag. It's fine if the config has the flag, but it's not necessary. It would be possible to limit checking of the env vars to the case when heroku_kafka_env
is true. The name of the flag is not great, as there are use cases outside of Heroku when reading from the environment might make sense, e.g. setting them in a systemd unit file. So a more generic name like use_os_env
might be better.
The reason for the mocking is that if we call the real System.put_env/2
in one test, then it will be set for other tests. I like to have tests, but in practice the code works, so one option is to remove (or comment out) the tests and the dependency on mock
. In a real project with lots of Erlang libraries, you are probably gong to have mock
as a dependency three times, so it doesn't bother me :-)
Generally speaking, the overall structure of the config process is inflexible. There are a lot of other brod
connection options that might be set (e.g. sasl authentication, which I need), and right now it requires each option to be specially supported in the code. It would be better to have all the brod
options set in one param. Supporting OS env vars would most likely still require code, but it's a special case.