Add support to unix sockets #153

cgerling · 2021-10-13T11:37:54Z

Closes https://github.com/keathley/finch/issues/139

This PR doesn't add support to connect with UNIX sockets through HTTPS, I wasn't sure how to do it without changing a lot of stuff so let me know if there is a "right way" to implement it

cgerling · 2021-10-13T11:45:36Z

lib/finch.ex

@@ -54,7 +54,8 @@ defmodule Finch do
      doc: """
      These options are passed to `Mint.HTTP.connect/4` whenever a new connection is established. \
      `:mode` is not configurable as Finch must control this setting. Typically these options are \
-      used to configure proxying, https settings, or connect timeouts.
+      used to configure proxying, https settings, or connect timeouts. \
+      When connecting to unix sockets a hostname must be provided.


Not sure if it should add "localhost" as the default hostname for this scenario or just leave it up to the client to specify one, any thoughts?

I think it would be a much nicer API if we could add this as the default for socket connections

sneako

Thanks for getting the ball rolling here! This is a great start, and it has helped uncover an issue with the API we were hoping to provide

sneako · 2021-10-14T07:05:18Z

lib/finch.ex

@@ -54,7 +54,8 @@ defmodule Finch do
      doc: """
      These options are passed to `Mint.HTTP.connect/4` whenever a new connection is established. \
      `:mode` is not configurable as Finch must control this setting. Typically these options are \
-      used to configure proxying, https settings, or connect timeouts.
+      used to configure proxying, https settings, or connect timeouts. \
+      When connecting to unix sockets a hostname must be provided.


I think it would be a much nicer API if we could add this as the default for socket connections

sneako · 2021-10-14T07:21:28Z

lib/finch.ex

@@ -133,6 +135,9 @@ defmodule Finch do
      :default ->
        {:ok, destination}

+      {:local, unix_socket} ->
+        {:ok, {:http, {:local, unix_socket}, 0}}


This is making me think that we might have to use a different format for the pool configuration, rather than just {:local, unix_socket}. We can't assume it will be :http so I guess we should revisit the previous suggestion of using a scheme like http+unix:// or https+unix:// but I am definitely open to new ideas as well.

To sum it up, the issue is that pools are started based on the pool configuration. When a pool is started, it registers itself with the Registry, using the {scheme, host, port} as the key. Then when we make a request, based on the request uri, we get the same {scheme, host, port} tuple and use that to query the Registry for the corresponding pool. Therefore, in order to match the request to the correctly configured pool, we need to be able to extract the full {scheme, host, port} tuple from the key that we provide in the pool configuration map.

Yeah, it's a real shame that http+unix:// isn't standardised. See whatwg/url#577.

As was mentioned before, I think a bit of a problem with the http+unix scheme is the socket path vs resource path distinction needs to be made and it means either escaping the socket path which is ugly or figuring out the socket path which seems brittle.

Another idea is to explicitly mention schema in the pool config:

pools = %{ {:local, :http, path} # or {:http, {:local, path}}, {:http, path}, etc } Finch.build(:get, "http://localhost/v1.41/_ping", unix_socket: "/var/run/docker.sock") |> Finch.request()

notice we have the http:// in our url so we can match that with the pool. But yeah, on the other hand it is a bit ugly that we're arbitrarily setting the hostname which seems to be ignored. (On the flip side, if there's a server implementation that cares about the hostname, this approach is beneficial.)

Ah right, good point about distinguishing between the socket path and the resource path :( I also considered adding the protocol to the tuple that we provide in the pool config, but I did not like the fact that it would be so different from the args that are passed to Finch.build/5. Maybe that is not such a big deal though?

Using socket should just work with the lazy :default pool, right? I think it will and so I think that most users would not need to even explicitly configure it. And for those that do, it seems so niche use case that it is perhaps fine that it is a little ugly :)

Good point, in that case {:http, {:local, socket_path}} seems the best to me. ~~Maybe we should also allow {:http, hostname, {:local, socket_path}} in case the server requires a hostname other than localhost (maybe this would be necessary for TLS?)~~ Scratch that, the hostname should be provided via conn_opts if it should be something other than localhost.

lib/finch/pool_manager.ex

cgerling · 2021-10-15T12:15:15Z

This is ready for another review @sneako @wojtekmach

wojtekmach

LGTM!

lib/finch.ex

sneako · 2021-10-15T12:33:39Z

test/finch_test.exs

+
+      start_supervised!({Finch, name: MyFinch})
+
+      Finch.build(:get, "http://localhost/", [], nil, unix_socket: socket_path)


Maybe we should add assert {:ok, %{status: 200}} = to this case and the next one?

I think it's good to ensure that the response was successfully parsed, I've added the asserts

sneako · 2021-10-15T12:34:46Z

Great work @cgerling ! Just a couple minor comments, but overall this looks good!

sneako · 2021-10-15T12:38:09Z

Ah and it looks like there are a few things that credo is complaining about too (don't worry about the failing tests on 1.7.4/19.x, we need to fix that after some recent changes)

Co-authored-by: Wojtek Mach <[email protected]>

sneako · 2021-10-16T07:58:47Z

Thanks @cgerling !

cgerling commented Oct 13, 2021

View reviewed changes

cgerling force-pushed the add-support-to-unix-sockets branch 2 times, most recently from 83cc0f7 to 147fb37 Compare October 13, 2021 11:56

sneako reviewed Oct 14, 2021

View reviewed changes

allow unix sockets to be specified in pool configuration

efec521

cgerling force-pushed the add-support-to-unix-sockets branch 3 times, most recently from 6247f99 to a98bfee Compare October 15, 2021 10:41

cgerling commented Oct 15, 2021

View reviewed changes

lib/finch/pool_manager.ex Show resolved Hide resolved

cgerling added 6 commits October 15, 2021 08:43

create mock to use as an unix socket in tests

e1abf0a

add option to specify a unix socket when building a request

ce7aaed

update function arity in deprecation warning

7575684

require scheme for unix socket in pool configuration

e667297

refactor pool config to a pipeline

f1455cc

add default hostname to unix socket pools

74fc610

cgerling force-pushed the add-support-to-unix-sockets branch from a98bfee to 3a1850b Compare October 15, 2021 11:53

cgerling requested a review from sneako October 15, 2021 12:11

wojtekmach reviewed Oct 15, 2021

View reviewed changes

lib/finch.ex Outdated Show resolved Hide resolved

sneako reviewed Oct 15, 2021

View reviewed changes

cgerling added 3 commits October 15, 2021 12:09

add ssl capability to mock socket server

5235663

ensure that requests can be made to unix sockets with https scheme

6d1c54b

add documentation to configure a pool for an unix socket

6cab10c

cgerling force-pushed the add-support-to-unix-sockets branch from 91500b1 to 6cab10c Compare October 15, 2021 15:42

update pool configuration documentation

976f6f8

Co-authored-by: Wojtek Mach <[email protected]>

cgerling requested a review from sneako October 15, 2021 15:44

sneako merged commit ebbe8f0 into sneako:main Oct 16, 2021

cgerling deleted the add-support-to-unix-sockets branch July 8, 2022 14:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support to unix sockets #153

Add support to unix sockets #153

cgerling commented Oct 13, 2021 •

edited

Loading

cgerling Oct 13, 2021 •

edited

Loading

sneako Oct 14, 2021

sneako left a comment

sneako Oct 14, 2021

sneako Oct 14, 2021

wojtekmach Oct 14, 2021

sneako Oct 14, 2021

wojtekmach Oct 14, 2021

sneako Oct 14, 2021 •

edited

Loading

cgerling commented Oct 15, 2021

wojtekmach left a comment

sneako Oct 15, 2021

cgerling Oct 15, 2021 •

edited

Loading

sneako commented Oct 15, 2021

sneako commented Oct 15, 2021 •

edited

Loading

sneako commented Oct 16, 2021


		start_supervised!({Finch, name: MyFinch})

		Finch.build(:get, "http://localhost/", [], nil, unix_socket: socket_path)

Add support to unix sockets #153

Add support to unix sockets #153

Conversation

cgerling commented Oct 13, 2021 • edited Loading

cgerling Oct 13, 2021 • edited Loading

Choose a reason for hiding this comment

sneako Oct 14, 2021

Choose a reason for hiding this comment

sneako left a comment

Choose a reason for hiding this comment

sneako Oct 14, 2021

Choose a reason for hiding this comment

sneako Oct 14, 2021

Choose a reason for hiding this comment

wojtekmach Oct 14, 2021

Choose a reason for hiding this comment

sneako Oct 14, 2021

Choose a reason for hiding this comment

wojtekmach Oct 14, 2021

Choose a reason for hiding this comment

sneako Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

cgerling commented Oct 15, 2021

wojtekmach left a comment

Choose a reason for hiding this comment

sneako Oct 15, 2021

Choose a reason for hiding this comment

cgerling Oct 15, 2021 • edited Loading

Choose a reason for hiding this comment

sneako commented Oct 15, 2021

sneako commented Oct 15, 2021 • edited Loading

sneako commented Oct 16, 2021

cgerling commented Oct 13, 2021 •

edited

Loading

cgerling Oct 13, 2021 •

edited

Loading

sneako Oct 14, 2021 •

edited

Loading

cgerling Oct 15, 2021 •

edited

Loading

sneako commented Oct 15, 2021 •

edited

Loading