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.

Sign up for GitHub

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

cohttp-eio: add Client.make_generic and HTTPS support #1002

Merged
merged 3 commits into from
Oct 27, 2023
Merged

cohttp-eio: add Client.make_generic and HTTPS support #1002

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c1f0433
cohttp-eio: factor out URI resolution
talex5 Sep 18, 2023
09d8f14
cohttp-eio: add Client.make_generic and HTTPS support
talex5 Sep 18, 2023
822285a
Make CI happy
talex5 Oct 25, 2023
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
1 change: 1 addition & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
## Unreleased
- cohttp-eio: Complete rewrite to follow common interfaces and behaviors. (mefyl #984)
- cohttp-eio: Add Client.make_generic and HTTPS support. (talex5 #1002)

## v6.0.0~alpha2 (2023-08-08)
- cohttp-lwt: Do not leak exceptions to `Lwt.async_exception_hook`. (mefyl #992, #995)
Expand Down
12 changes: 9 additions & 3 deletions cohttp-eio/examples/client_tls.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,24 @@ let () =
Logs_threaded.enable ();
Logs.Src.set_level Cohttp_eio.src (Some Debug)

let null_auth ?ip:_ ~host:_ _ = Ok None (* Warning: use a real authenticator in your code! *)
let null_auth ?ip:_ ~host:_ _ =
Ok None (* Warning: use a real authenticator in your code! *)

let https ~authenticator =
let tls_config = Tls.Config.client ~authenticator () in
fun uri raw ->
let host = Uri.host uri |> Option.map (fun x -> Domain_name.(host_exn (of_string_exn x))) in
let host =
Uri.host uri
|> Option.map (fun x -> Domain_name.(host_exn (of_string_exn x)))
in
Tls_eio.client_of_flow ?host tls_config raw

let () =
Eio_main.run @@ fun env ->
Mirage_crypto_rng_eio.run (module Mirage_crypto_rng.Fortuna) env @@ fun () ->
let client = Client.make ~https:(Some (https ~authenticator:null_auth)) env#net in
let client =
Client.make ~https:(Some (https ~authenticator:null_auth)) env#net
in
Eio.Switch.run @@ fun sw ->
let resp, body =
Client.get ~sw client (Uri.of_string "https://example.com")
Copy link
Member

Choose a reason for hiding this comment

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

I am a bit sad with the amount of boilerplate we have to come up to do a simple https call. Could we somehow ship a default authenticator as well? The current API doesn't look very simple to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't ship a default authenticator without adding extra dependencies to cohttp-eio. The plan is to have another package for that later (either cohttp-eio-tls, or maybe something conduit-like); this PR is just making that possible.

Copy link
Collaborator

@mseri mseri Oct 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think we could merge this and release it as beta, so that people can test it out in the meantime and inform possible api refinements. But I agree with @samoht, and I think it would be nice to have cohttp-eio-tls before we release the new stable 6.0.0

Copy link
Member

Choose a reason for hiding this comment

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

That sounds like a good plan. I'm already using that PR in a project of mine and it's working great 👍 Thanks for this!

Expand Down
11 changes: 10 additions & 1 deletion cohttp-eio/examples/dune
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,15 @@
(executables
(names server1 client1 docker_client client_timeout client_tls)
(libraries cohttp-eio eio_main eio.unix fmt unix logs.fmt logs.threaded tls-eio mirage-crypto-rng-eio))
(libraries
cohttp-eio
eio_main
eio.unix
fmt
unix
logs.fmt
logs.threaded
tls-eio
mirage-crypto-rng-eio))

(alias
(name runtest)
Expand Down
28 changes: 17 additions & 11 deletions cohttp-eio/src/client.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ include Client_intf
open Utils

type connection = Eio.Flow.two_way_ty r

type t = sw:Switch.t -> Uri.t -> connection

include
Expand All @@ -15,7 +14,7 @@ include

let map_context v f t ~sw = f (v t ~sw)

let call (t:t) ~sw ?headers ?body ?(chunked = false) meth uri =
let call (t : t) ~sw ?headers ?body ?(chunked = false) meth uri =
let socket = t ~sw uri in
let body_length =
if chunked then None
Expand Down Expand Up @@ -84,16 +83,23 @@ let tcp_address ~net uri =

let make ~https net : t =
let net = (net :> [ `Generic ] Eio.Net.ty r) in
let https = (https :> (Uri.t -> [ `Generic ] Eio.Net.stream_socket_ty r -> connection) option) in
let https =
(https
:> (Uri.t -> [ `Generic ] Eio.Net.stream_socket_ty r -> connection) option)
in
fun ~sw uri ->
match Uri.scheme uri with
| Some "httpunix" ->
(* FIXME: while there is no standard, http+unix seems more widespread *)
(Eio.Net.connect ~sw net (unix_address uri) :> connection)
| Some "http" -> (Eio.Net.connect ~sw net (tcp_address ~net uri) :> connection)
| Some "https" ->
(match https with
| Some wrap -> wrap uri @@ Eio.Net.connect ~sw net (tcp_address ~net uri)
| None -> Fmt.failwith "HTTPS not enabled (for %a)" Uri.pp uri)
(* FIXME: while there is no standard, http+unix seems more widespread *)
(Eio.Net.connect ~sw net (unix_address uri) :> connection)
| Some "http" ->
(Eio.Net.connect ~sw net (tcp_address ~net uri) :> connection)
| Some "https" -> (
match https with
| Some wrap ->
wrap uri @@ Eio.Net.connect ~sw net (tcp_address ~net uri)
| None -> Fmt.failwith "HTTPS not enabled (for %a)" Uri.pp uri)
| x ->
Fmt.failwith "Unknown scheme %a" Fmt.(option ~none:(any "None") Dump.string) x
Fmt.failwith "Unknown scheme %a"
Fmt.(option ~none:(any "None") Dump.string)
x
22 changes: 13 additions & 9 deletions cohttp-eio/src/client.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,20 @@ include
and type body = Body.t

val make :
https:(Uri.t -> [`Generic] Eio.Net.stream_socket_ty r -> _ Eio.Flow.two_way) option ->
_ Eio.Net.t -> t
(** [make ~https net] is a convenience wrapper around {!make_generic} that
uses [net] to make connections.
https:
(Uri.t -> [ `Generic ] Eio.Net.stream_socket_ty r -> _ Eio.Flow.two_way)
option ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
option ->
->

Since it is no longer optional, why not dropping the option from the signature?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https support is optional (you don't have to use it); see #984 (comment):

I don’t have too strong of an opinion, I think the optional is cleaner but perhaps https is so pervasive nowadays that it is good to keep it explicit instead.

Copy link
Member

@samoht samoht Oct 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

if the plan is to have a separate cohttp-eio-tls package I suggest making the parameter optional - otherwise it's a bit clumsy for HTTP calls to have to thread that ~https:None parameter everwhere.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I agree. I was recalling what we had discussed incorrectly

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this signature ok as is, or do we need to change https into ?https?

Copy link
Member

Choose a reason for hiding this comment

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

I think I would prefer ?https but I also understand the rationale to keep it explicit as the error mode (the call will just hang) is not great right now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's leave it as is. We can make it optional in due course once there is the tls package if we can think of a better way to deal with it

_ Eio.Net.t ->
t
(** [make ~https net] is a convenience wrapper around {!make_generic} that uses
[net] to make connections.
- URIs of the form "http://host:port/..." connect to the given TCP host and port.
- URIs of the form "https://host:port/..." connect to the given TCP host and port,
and are then wrapped by [https] (or rejected if that is [None]).
- URIs of the form "httpunix://unix-path/http-path" connect to the given Unix path.
*)
- URIs of the form "http://host:port/..." connect to the given TCP host and
port.
- URIs of the form "https://host:port/..." connect to the given TCP host and
port, and are then wrapped by [https] (or rejected if that is [None]).
- URIs of the form "httpunix://unix-path/http-path" connect to the given
Unix path. *)

val make_generic : (sw:Switch.t -> Uri.t -> _ Eio.Net.stream_socket) -> t
(** [make_generic connect] is an HTTP client that uses [connect] to get the
Expand Down
Loading