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 http proxy configuration via env variables #2850

Merged
merged 4 commits into from
Nov 8, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/livebook/application.ex
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ defmodule Livebook.Application do
use Application

def start(_type, _args) do
Livebook.Utils.HTTP.set_proxy_options()

Livebook.ZTA.init()
create_teams_hub = parse_teams_hub()
setup_optional_dependencies()
Expand Down
8 changes: 6 additions & 2 deletions lib/livebook/fly_api.ex
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,9 @@ defmodule Livebook.FlyAPI do
|> Keyword.merge(opts)
|> Keyword.merge(test_options())

case Req.request(opts) do
req = opts |> Req.new() |> Livebook.Utils.req_attach_defaults()

case Req.request(req) do
{:ok, %{status: status, body: body}} when status in 200..299 ->
{:ok, body}

Expand Down Expand Up @@ -281,7 +283,9 @@ defmodule Livebook.FlyAPI do
]
|> Keyword.merge(test_options())

case Req.request(opts) do
req = opts |> Req.new() |> Livebook.Utils.req_attach_defaults()

case Req.request(req) do
{:ok, %{status: 200, body: body}} ->
case body do
%{"errors" => [%{"extensions" => %{"code" => "UNAUTHORIZED"}} | _]} ->
Expand Down
16 changes: 10 additions & 6 deletions lib/livebook/k8s_api.ex
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ defmodule Livebook.K8sAPI do
"""
@spec watch_pod_events(kubeconfig(), String.t(), String.t()) :: {:ok, Enumerable.t()} | error()
def watch_pod_events(kubeconfig, namespace, name) do
req = Req.new() |> Kubereq.attach(kubeconfig: kubeconfig, api_version: "v1", kind: "Event")
req = build_req() |> Kubereq.attach(kubeconfig: kubeconfig, api_version: "v1", kind: "Event")

Kubereq.watch(req, namespace,
field_selectors: [
Expand Down Expand Up @@ -120,7 +120,7 @@ defmodule Livebook.K8sAPI do
when data: list(%{name: String.t()})
def list_namespaces(kubeconfig) do
req =
Req.new() |> Kubereq.attach(kubeconfig: kubeconfig, api_version: "v1", kind: "Namespace")
build_req() |> Kubereq.attach(kubeconfig: kubeconfig, api_version: "v1", kind: "Namespace")

case Kubereq.list(req, nil) do
{:ok, %{status: 200, body: %{"items" => items}}} ->
Expand Down Expand Up @@ -198,7 +198,7 @@ defmodule Livebook.K8sAPI do
when data: list(%{name: String.t()})
def list_storage_classes(kubeconfig) do
req =
Req.new()
build_req()
|> Kubereq.attach(
kubeconfig: kubeconfig,
api_version: "storage.k8s.io/v1",
Expand Down Expand Up @@ -260,7 +260,7 @@ defmodule Livebook.K8sAPI do
}

req =
Req.new()
build_req()
|> Kubereq.attach(
kubeconfig: kubeconfig,
api_version: "authorization.k8s.io/v1",
Expand Down Expand Up @@ -304,11 +304,15 @@ defmodule Livebook.K8sAPI do
end

defp pod_req(kubeconfig) do
Req.new() |> Kubereq.attach(kubeconfig: kubeconfig, api_version: "v1", kind: "Pod")
build_req() |> Kubereq.attach(kubeconfig: kubeconfig, api_version: "v1", kind: "Pod")
end

defp pvc_req(kubeconfig) do
Req.new()
build_req()
|> Kubereq.attach(kubeconfig: kubeconfig, api_version: "v1", kind: "PersistentVolumeClaim")
end

defp build_req() do
Req.new() |> Livebook.Utils.req_attach_defaults()
end
end
10 changes: 1 addition & 9 deletions lib/livebook/runtime/dependencies.ex
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,10 @@ defmodule Livebook.Runtime.Dependencies do
{:ok, String.t()} | {:error, String.t()}
def add_dependencies(code, dependencies) do
deps = Enum.map(dependencies, & &1.dep)
config = Enum.reduce(dependencies, [], &deep_merge(&2, &1.config))
config = Enum.reduce(dependencies, [], &Livebook.Utils.keyword_deep_merge(&2, &1.config))
add_mix_deps(code, deps, config)
end

defp deep_merge(left, right) do
if Keyword.keyword?(left) and Keyword.keyword?(right) do
Keyword.merge(left, right, fn _key, left, right -> deep_merge(left, right) end)
else
right
end
end

@doc """
Finds or adds a `Mix.install/2` call to `code` and modifies it to
include the given Mix deps.
Expand Down
1 change: 1 addition & 0 deletions lib/livebook/teams/requests.ex
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,7 @@ defmodule Livebook.Teams.Requests do
inet6: String.ends_with?(Livebook.Config.teams_url(), ".flycast"),
headers: [{"x-lb-version", Livebook.Config.app_version()}]
)
|> Livebook.Utils.req_attach_defaults()
end

defp add_team_auth(req, nil), do: req
Expand Down
19 changes: 8 additions & 11 deletions lib/livebook/teams/web_socket.ex
Original file line number Diff line number Diff line change
Expand Up @@ -22,19 +22,16 @@ defmodule Livebook.Teams.WebSocket do
{http_scheme, ws_scheme} = parse_scheme(uri)
state = %{status: nil, headers: [], body: []}

transport_opts =
if http_scheme == :https do
[cacerts: :public_key.cacerts_get()]
else
[]
end
opts = Livebook.Utils.mint_connect_options_for_uri(uri)

transport_opts =
if String.ends_with?(Livebook.Config.teams_url(), ".flycast"),
do: Keyword.put(transport_opts, :inet6, true),
else: transport_opts
opts = Keyword.merge(opts, protocols: [:http1])

opts = [protocols: [:http1], transport_opts: transport_opts]
opts =
if String.ends_with?(Livebook.Config.teams_url(), ".flycast") do
Livebook.Utils.keyword_deep_merge(opts, transport_opts: [inet6: true])
else
opts
end

with {:ok, conn} <- Mint.HTTP.connect(http_scheme, uri.host, uri.port, opts),
{:ok, conn, ref} <- Mint.WebSocket.upgrade(ws_scheme, conn, @ws_path, headers) do
Expand Down
76 changes: 76 additions & 0 deletions lib/livebook/utils.ex
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,18 @@ defmodule Livebook.Utils do
end
end

@doc """
Recursively merges keyword lists.
"""
@spec keyword_deep_merge(keyword(), keyword()) :: keyword()
def keyword_deep_merge(left, right) do
if Keyword.keyword?(left) and Keyword.keyword?(right) do
Keyword.merge(left, right, fn _key, left, right -> keyword_deep_merge(left, right) end)
else
right
end
end

@doc """
Validates if the given URL is syntactically valid.

Expand Down Expand Up @@ -745,4 +757,68 @@ defmodule Livebook.Utils do
def ip_to_host({0, 0, 0, 0}), do: "localhost"
def ip_to_host({127, 0, 0, 1}), do: "localhost"
def ip_to_host(ip), do: ip |> :inet.ntoa() |> List.to_string()

@doc """
Req plugin adding global Livebook-specific plugs.

The plugin covers cacerts and HTTP proxy configuration.
"""
@spec req_attach_defaults(Req.Request.t()) :: Req.Request.t()
def req_attach_defaults(req) do
Req.Request.append_request_steps(req,
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

having a separate function for deep merge is interesting, thanks!

end
)
end

@doc """
Returns options for `Mint.HTTP.connect/4` that should be used for
the given target URI.
"""
@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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


no_proxy =
if no_proxy = System.get_env("NO_PROXY") || System.get_env("no_proxy") do
String.split(no_proxy, ",")
else
[]
end

proxy_opts =
cond do
uri.host in no_proxy -> []
uri.scheme == "http" && http_proxy -> proxy_options(http_proxy)
uri.scheme == "https" && https_proxy -> proxy_options(https_proxy)
true -> []
end

cert_opts =
if uri.scheme == "https" do
if cacertfile = Livebook.Config.cacertfile() do
[transport_opts: [cacertfile: cacertfile]]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@jonatanklosko jonatanklosko Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

else
[transport_opts: [cacerts: :public_key.cacerts_get()]]
end
else
[]
end

proxy_opts ++ cert_opts
end

defp proxy_options(proxy) do
uri = URI.parse(proxy || "")

if uri.host && uri.port do
[proxy: {:http, uri.host, uri.port, []}]
else
[]
end
end
end
27 changes: 27 additions & 0 deletions lib/livebook/utils/http.ex
Original file line number Diff line number Diff line change
Expand Up @@ -212,4 +212,31 @@ defmodule Livebook.Utils.HTTP do
]
]
end

@doc false
def set_proxy_options() 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")

no_proxy =
if no_proxy = System.get_env("NO_PROXY") || System.get_env("no_proxy") do
no_proxy
|> String.split(",")
|> Enum.map(&String.to_charlist/1)
else
[]
end

set_proxy_option(:proxy, http_proxy, no_proxy)
set_proxy_option(:https_proxy, https_proxy, no_proxy)
end

defp set_proxy_option(proxy_scheme, proxy, no_proxy) do
uri = URI.parse(proxy || "")

if uri.host && uri.port do
host = String.to_charlist(uri.host)
:httpc.set_options([{proxy_scheme, {{host, uri.port}, no_proxy}}])
end
end
end
Loading