Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support ssl configuration from files #52

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

reachfh
Copy link
Contributor

@reachfh reachfh commented Dec 10, 2017

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.

@objectuser
Copy link
Contributor

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

@reachfh
Copy link
Contributor Author

reachfh commented Dec 12, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants