kaffe icon indicating copy to clipboard operation
kaffe copied to clipboard

Support ssl configuration from files

Open reachfh opened this issue 7 years ago • 2 comments

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 avatar Dec 10 '17 08:12 reachfh

@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?

objectuser avatar Dec 11 '17 21:12 objectuser

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.

reachfh avatar Dec 12 '17 01:12 reachfh