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

Enable Transport configuration for http client #129

Merged
merged 1 commit into from Jul 19, 2023
Merged

Enable Transport configuration for http client #129

merged 1 commit into from Jul 19, 2023

Conversation

leandro-deveikis
Copy link
Contributor

Context:

This change enables passing a RoundTripper as a configuration parameter (via the WithTransport() func) to use, for instance, a Socks5 proxy.
If the transport is not configured, it remains with the original behavior.

Changes made:

  • Changed Transport.Base to be of interface http.RoundTripper (is returned as that at PooledTransport() )
  • Added 'WithTransport()' func to set the transport configuration.
  • Added validation to existing test case.

@rcypher-databricks
Copy link
Contributor

Quick question: is this primarily to allow using a proxy? If so, have you tried setting the HTTP_PROXY and HTTPS_PROXY environment variables, or is that mechanism insufficient?

@leandro-deveikis
Copy link
Contributor Author

leandro-deveikis commented Jun 2, 2023

Hi @rcypher-databricks thanks for the quick response!
That is insufficient as this will be used to run a SOCKS5 proxy (wich works on a different layer), a different protocol that can work with many other protocols (HTTP among them).

@rcypher-databricks
Copy link
Contributor

You should be able to use socks5, based on the documentation. But of course this still could not be sufficient for your needs. Just want to make sure we're not reinventing the wheel here.
The environment values may be either a complete URL or a "host[:port]", in which case the "http" scheme is assumed. The schemes "http", "https", and "socks5" are supported. An error is returned if the value is a different form.
Fun Fact: http.DefaultTransport natively supports SOCKS5 proxies from environment variables

@leandro-deveikis
Copy link
Contributor Author

You are right on that :)

However, we are also needing to set the TLS configuration (certificates), which we do use the http.Transport struct. It looks like there is no way to configure TLS, it’s using a default Direct dialer.

On the other hand, we have multiple connectors in the same system, and not all of them are connected to the proxy or use different credentials, which invalidates also the env variables approach (it would be shared across all the system).

@leandro-deveikis
Copy link
Contributor Author

@rcypher-databricks Thanks for the approval!
I signed off both commits, so I think it would be ready to merge when the workflows are run again.

@leandro-deveikis
Copy link
Contributor Author

hi @rcypher-databricks did you had a chance to review the workflows to check if it is ok to merge??

@leandro-deveikis
Copy link
Contributor Author

Hi @rcypher-databricks sorry to bother you again, is there anything else I can do to move this along?

cc: @susodapop @andrefurlan-db also sorry to ping you as well, it shows me that you are also owners of some files in this PR, but I can't ask for review

@rcypher-databricks
Copy link
Contributor

We want to add this to the next point release. Would id be possible for you to sign the commit with a DCO

…d test

Signed-off-by: Leandro Deveikis <leandro.deveikis@gmail.com>
@leandro-deveikis
Copy link
Contributor Author

Thanks @rcypher-databricks. I just squashed both commits and signed them off. I don't usually do that, so let me know if there is something wrong or missing. Thanks!

@rcypher-databricks rcypher-databricks merged commit 0c28a5a into databricks:main Jul 19, 2023
3 checks passed
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