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

Allow to configure rollbax to contact api server via proxy #105

Merged

Conversation

GermanDZ
Copy link
Contributor

@GermanDZ GermanDZ commented Nov 13, 2018

Why?

Because some security hardened production environments require to contact the "outside" internet through a proxy and hackney (the erlang http client used by rollbax) doesn't read the usual env vars used to config a proxy.

How?

Adding an optional entry to Rollbar.config allowing the specification of an URL to be passed to hackney to force the use of a proxy.

Caveats

  • The testing changes seems very complicated but are fast enough
  • hackney will ignore the proxy option in case the value is nil, which simplify a lot passing & using the config.

Kudos to @amuino for guiding me to this solution

hackey supports a really simple way to specify an url to be used as proxy
@GermanDZ
Copy link
Contributor Author

ping @whatyouhide

@whatyouhide
Copy link
Contributor

Hey @GermanDZ, thanks for the contribution! I like it but all the infrastructure for testing it seems like overkill considering that we're basically testing a hackney feature. I would either:

  • not test proxying, again it's a hackney feature so it's supposed to work and I'm fine with not testing it
  • testing it through Mox (by mocking the HTTP client) and asserting that the proxy option is passed

Thoughts?

lib/rollbax/client.ex Outdated Show resolved Hide resolved
@GermanDZ
Copy link
Contributor Author

I cannot use Mox to mock :hackney, being it an Erlang module without a behaviour (ArgumentError) module :hackney is not a behaviour, please pass a behaviour to :for)

My options right now:

  1. Keep the Proxy server for test (current solution)
  2. Add an indirection, extracting :hackney from the Rollbax.Client and using something like http_client/0, then for production http_client/0 will return the :hackney module, but during test we can mock it to return an alternative. This also could lead to allow to use other http client via configuration (but not seems too desirable)
  3. Remove the test for being only a simple config (but I don't code without test)

@whatyouhide
Copy link
Contributor

1 is what I would really prefer to avoid. 2 is what I meant when I said we could use Mox. I personally would go with 3, but either 2 or 3 are okay for me.

@GermanDZ
Copy link
Contributor Author

Working on 2

@GermanDZ
Copy link
Contributor Author

Finally I went for 3, no so much tests. Anyway, I've added a simple test just to ensure nobody removes the config without breaking the feature.

Now only check the request are made to the URL configured as proxy not the api_endpoint.

@whatyouhide whatyouhide merged commit cdff8c4 into ForzaElixir:master Jan 25, 2019
@whatyouhide
Copy link
Contributor

Thanks @GermanDZ 💟

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.

None yet

2 participants