Support http proxy configuration via env variables #2850
Conversation
| @spec mint_connect_options_for_uri(URI.t()) :: keyword() | ||
| def mint_connect_options_for_uri(uri) do | ||
| http_proxy = System.get_env("HTTP_PROXY") || System.get_env("http_proxy") | ||
| https_proxy = System.get_env("HTTPS_PROXY") || System.get_env("https_proxy") || http_proxy |
There was a problem hiding this comment.
curl only uses HTTP_PROXY for http requests and HTTPS_PROXY for https requests, however :httpc uses :proxy as the default for :https_proxy, so I mirrored :httpc here.
|
Uffizzi Preview |
| connect_options: fn request -> | ||
| uri = URI.parse(request.url) | ||
| connect_options = mint_connect_options_for_uri(uri) | ||
| Req.Request.merge_options(request, connect_options: connect_options) |
There was a problem hiding this comment.
This is fine as is but if some other code previously set :connect_options, they'd be overridden. I don't have a solution for this in Req, there's just an issue as a reminder: wojtekmach/req#319.
I think ideally it'd be as easy as:
put_in(req.options[:connect_options][:proxy], proxy)but it of course assumes the nested structure is already there. I guess what I'm trying to say is it would be nice if there's a language feature that makes above easy or at least if there's a Req feature. WDYT?
There was a problem hiding this comment.
I actually did check if Req.merge would handle it recursively, and then considered doing deep merge with req.options[:connect_options], but since we don't specify connect_options anywhere else, I ended up ignoring it.
Instead of making the replace/merge behaviour specified at option registration, we could shift the decision to the caller, as in Req.Request.deep_merge_options (or an option to merge_options).
There was a problem hiding this comment.
having a separate function for deep merge is interesting, thanks!
lib/livebook/utils.ex
Outdated
| cert_opts = | ||
| if uri.scheme == "https" do | ||
| if cacertfile = Livebook.Config.cacertfile() do | ||
| [transport_opts: [cacertfile: cacertfile]] |
There was a problem hiding this comment.
As an aside, in Req I have a few shortcuts like the following, these two calls are equivalent:
Req.new(inet6: true)
Req.new(connect_options: [transport_opts: [inet6: true]])cacerts/cacertfile seems maybe common enough to warrant such shortcut but tbh I don't know where to draw the line, I'm kind of wary of adding a lot of top-level options and are looking into nesting (livebook-dev/req_athena#40 (comment)) but then this is already nested but like too nested? Curious if any of this resonate with you.
There was a problem hiding this comment.
No strong opinion, I don't think we need to rush into more top-level shortcuts. Though with #394 it may be a better argument to be able to specify proxy and certs on the same level.
Adds support for
HTTP_PROXY/HTTPS_PROXYenv vars.