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 unix sockets #139

Closed
dallagi opened this issue May 17, 2021 · 12 comments · Fixed by #153
Closed

Support unix sockets #139

dallagi opened this issue May 17, 2021 · 12 comments · Fixed by #153

Comments

@dallagi
Copy link

dallagi commented May 17, 2021

Hello,

Mint natively supports making requests on unix sockets by setting the address to {:local, "/path/to/socket.sock"} and the port number to 0, for example:

{:ok, conn} = Mint.HTTP.connect(:http, {:local, "/var/run/docker.sock"}, 0, hostname: "localhost")

For more context, tests for this functionality in Mint can be found here.

However, to my knowledge Finch does not currently allow this.
In case this feature may be desirable, a possible approach could be to do as Hackney does, ie. supporting url schemas in the form of eg. "http+unix".

@sneako
Copy link
Owner

sneako commented May 18, 2021

Hi and thanks for the issue! I agree that Finch should support this. I have not looked in to this very much yet, but rather than using a scheme like http+unix, I think we should use the {:local, socket_path} tuple that Mint uses, which is the type from inet https://erlang.org/doc/man/inet.html#type-local_address.

@dallagi
Copy link
Author

dallagi commented May 18, 2021

Hi @sneako , thanks for your quick reply!

Personally I don't have a strong opinion about which approach to use, but I'm afraid that using the local_address() type from inet would not fit well with the current API exposed by Finch, which (if I understand correctly) currently expects a URI or a string that can be parsed to a URI.
Of course my experience with Finch's codebase is limited, so I may very well be missing something here.

Anyway, once the preferred approach is defined I would be happy to try and implement it, if that's OK for you :)

@sneako
Copy link
Owner

sneako commented May 25, 2021

Hi @dallagi, sorry for the delay in getting back to you. I have been thinking about this a bit in the meantime, and have come to the conclusion that my initial proposal of using the local_address type would probably not be sufficient. I have not personally used TLS over a unix socket, however based on my limited research it does seem to be entirely possible. There is no way to specify that you would like to use TLS in the local_address tuple that I had proposed, therefore your proposal of http+unix or https+unix does seem like it could be the best way forward.

@sneako
Copy link
Owner

sneako commented May 25, 2021

This PR was brought to my attention that highlights some potential issues that the current proposal may introduce benoitc/hackney#653

@dallagi
Copy link
Author

dallagi commented May 26, 2021

Hi @sneako,
No worries at all for the delay, I appreciate the effort you’re putting in maintaining finch :)

Thanks for the heads up about the potential issues!
I agree that having to url encode the path of the socket is not very convenient.

However, personally I can’t think of a better alternative.
Passing a custom tuple (similarly as the PR you linked) would work, but I’m afraid it would also feel a little out of place in finch, since until now the URI was always passed as a String or `URI.

@wojtekmach
Copy link
Contributor

out of curiosity, what would be an actual example call to Finch.build/4 that you propose? Please use as real values as possible and if we need to URI encode things, please do that as well. I think showing concrete examples will help move the discussion forward.

@dallagi
Copy link
Author

dallagi commented May 26, 2021

Hi @wojtekmach you are absolutely right, I’ll try to come up with some practical examples.

Let’s say we want to hit the _ping api exposed by a local docker daemon listening on /var/run/docker.sock.

Using an approach similar to hackney’s it would look like this:

Finch.build(:get, "http+unix://%2Fvar%2Frun%2Fdocker.sock/v1.41/_ping")

Otherwise, another option could be to go with a tuple, which for example could look like this:

Finch.build(:get, {"http", "/var/run/docker.sock", "/v1.41/_ping"})

Headers and body would keep working same way as now.

To be clear I don't have a strong preference, as IMHO none of them is inherently better than the other, and of course there may also be some better option I didn't consider.

@dallagi
Copy link
Author

dallagi commented May 28, 2021

I thinked a bit more about this.

If I had to pick one to these two approaches myself I’d probably go with the former, ie. hackney’s approach.
This would allow Finch to keep the API of build/4 consistent (as the url would keep being either a URI or a URI-like String) at the cost of a little more complexity for users due to urlencoding, which IMHO could be an acceptable compromise given that unix sockets are supposedly a rather niche use case.

However, the simplest approach may actually be to skip the build/4 step altogether (for now at least), and let users who want to run request on a unix socket build the Request directly.

This may look something like this:

%Finch.Request{scheme: :"http+unix", host: "/var/run/docker.sock", port: 0, method: "GET", path: "/v1.41/_ping", headers: [], body: nil, query: nil}

Overall, in my opinion this last approach could be a good way to start, as it would allow Finch to add support for unix sockets without changing its user-facing API, while still keeping the possibility to update the build/4 function at a later time if necessary.

@wojtekmach
Copy link
Contributor

In curl, setting the unix socket is just another option instead of messing with the url:

$ curl --unix-socket /var/run/docker.sock http://localhost/v1.41/_ping
OK

(setting the url to http:///v1.41/_ping or even just http://v1.41/_ping also works, more on this below.)

I think this is relevant here. Here's an idea how we could support this in Finch:

{:ok, _} =
  Finch.start_link(
    name: MyFinch,
    pools: %{
      {:local, "/var/run/docker.sock"} => [size: 10]
    }
  )

Finch.build(:get, "http://localhost/v1.41/_ping", unix_socket: "/var/run/docker.sock")
|> Finch.request()

Note, in our pool map we use the path to the socket, not the host. I believe the host is mostly or totally irrelevant. For example, It is ignored by the docker server:

$ curl --unix-socket /var/run/docker.sock http://bad.host/v1.41/_ping
OK

Btw, given at least for Docker, the host is irrelevant, we can also use empty host like this:

iex> URI.parse("http:///v1.41/_ping") |> Map.take([:host, :path, :scheme])
%{host: "", path: "/v1.41/_ping", scheme: "http"}

iex> :uri_string.parse("http:///v1.41/_ping")
%{host: "", path: "/v1.41/_ping", scheme: "http"}

WDYT?

@sneako
Copy link
Owner

sneako commented Aug 10, 2021

I like @wojtekmach's proposal of using a new :unix_socket option rather than some kind of hybrid scheme. We could add a :unix_socket field to the Request struct and from there it should be quite straightforward.

@cgerling
Copy link
Contributor

cgerling commented Oct 9, 2021

I have a similar use case that requires communicating with a UNIX socket.
I would like to do a PR to add support to this, is @wojtekmach's solution the chosen approach?

@sneako
Copy link
Owner

sneako commented Oct 10, 2021

Yes, let's go with the approach Wojtek proposed. Thanks @cgerling!

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 a pull request may close this issue.

4 participants