-
Notifications
You must be signed in to change notification settings - Fork 43
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
Add eio backend for ocaml-dns client #312
base: main
Are you sure you want to change the base?
Conversation
d385733
to
99244fb
Compare
83649fa
to
955b2ad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I experimented with some API changes here: https://github.com/bikallem/ocaml-dns/compare/eio...talex5:ocaml-dns:eio?expand=1
The main ones are:
- Instead of providing a
run
function that just calls mirage-crypto, it's just as easy for the user call it themselves (and better if they're using crypto for other purposes). - There's no need to pass
Client
as a first-class module. We know what it is statically. - I moved
create
tocreate_from_env
for people who want that, and added a more explicitcreate
function so people can pass the inputs separately.
This changes the user code from:
Eio_main.run @@ fun env ->
Eio.Switch.run @@ fun sw ->
Dns_client_eio.run env @@ fun (module Client) ->
let env = (env :> Dns_client_eio.env) in
let c = Client.create (env, sw) in
to
Eio_main.run @@ fun env ->
Mirage_crypto_rng_eio.run (module Mirage_crypto_rng.Fortuna) env @@ fun () ->
Eio.Switch.run @@ fun sw ->
let c = Client.create_from_env ~sw env in
The needs to specify Fortuna
explicitly seems a bit ugly to me, but if we don't want that then it should be fixed in mirage-crypto.
(Was the idea of the first class module to force users to initialise mirage-crypto before using the library? If so, we could probably fix this more easily by having Mirage_crypto_rng_eio.run
return a token and passing that to create_from_env
. Passing first-class modules around tends to get awkward.)
I'm not sure about passing resolv.conf
as an Eio.Path
. Possibly it should just be a function unit -> string
. Also, Eio should provide some convenience types for paths (maybe Path.rw
and Path.ro
or something).
fe4e980
to
080dc84
Compare
I have rebased/updated the PR to address reviewer feedback.
Indeed, the idea of the API was to maintain the invariant that that consumers of the library to be able to use the module only when run under |
8c00d39
to
3c9c976
Compare
Hmmm ... not sure why the ci is failing. Can anyone help? |
It says:
|
I'm not particularly a fan of passing first-class modules around, and don't think there's a good reason here. I wonder what the "EIO" and "RANDOM" story is (if there's any):
So, what is the plan for EIO and RANDOM? How and who should decide where to take random numbers from? Is this all up to the user (and do they have a unified interface)? In DNS, we don't need cryptographically secure random, something that is not predictable is sufficient (it's used mainly for the ID anyways, to avoid spoofing of DNS packets (see https://en.wikipedia.org/wiki/DNS_spoofing if interested in details)). |
Indeed. I removed it in my branch (see #312 (review)).
The PR is just trying to make it harder to misuse the API. mirage-crypto-rng-eio already uses it correctly (I'm not aware of anyone using it incorrectly).
I assume it's just copying the existing lwt code, which uses |
Trying once more. In eio, do you have a user guide / approach to randomness? I can see three different things going on here:
Now, my questions are:
Maybe this PR here is the wrong context for these questions and considerations, but for me it is important to understand how "eio" should be used in order to avoid having to back out such changes. |
I have now removed the use of first class modules, the EDIT: Updating the default nameservers to the same as unix client did the trick. It is working now. |
d159f5f
to
1bbaec3
Compare
Add `dns-client-eio` opam package - an eio backe-end for dns-client. It is compatible with OCaml version 5.0 and above.
16bad51
to
21b58b5
Compare
Needs mirleft/ocaml-tls#458 to operate correctly. |
So it seems we're a bit stuck on this PR:
How to move forward here? I unlikely have time before Q3 2023 to look into "eio", but I suspect you'd like to have "something merged" sooner. But I'm still overwhelmed by the complexity of eio and don't feel confident to grasp the semantics of what is happening where (and why concurrently etc.). |
@hannesm This PR is now ready. It doesn't have the TLS issue anymore.
The issue is separate from dns-client-eio and should be handled in the eio repo. |
I take a long look about this PR and it seems that DNS request by another domain seems not an option with the current proposed design in this PR - at least, the use of "mutable" without protection makes me think there could be problems. The reason I point to this specific usage is that it seems to me that the design of happy-eyeballs (the core package, not the |
In current eio, IIUIC resource/connection/switch created in one domain can't be moved/used in another domain. Therefore, even if happy eyeballs is able to create connections, I am not sure if those connections can be used/shared in an ad-hoc manner by the domains. However, this does not preclude having multiple/parallel dns client requests via OCaml domains - which the current design offers/enables. |
This PR adds support for eio backend for ocaml-dns client. At the moment it only adds
tcp
tansport protocol support. I intend to addtls
or DNS over TLS support in the future once eio support for ocaml-tls is done.Transport
moduleQueue
calls ? (Not sure if it is needed yet)mirage-crypto-rng-eio
(mirage-crypto-rng-eio: Eio backend of mirage-crypto-rng mirage-crypto#155)ohost
executable exercisingdns-client-eio
package (https://github.com/bikallem/ocaml-dns/blob/eio/eio/client/ohost.ml)This PR has a few unreleased packages dependencies:
eio
with the Mutex module (https://github.com/ocaml-multicore/eio/blob/main/lib_eio/eio.mli#L35)mirage-crypto-rng-eio
(mirage-crypto-rng-eio: Eio backend of mirage-crypto-rng mirage-crypto#155)At the moment
eio
doesn't support monotonic clock, so we still usemtime
. It may be preferable to use monotonic clock support fromeio
once support for it lands in eio (ocaml-multicore/eio#229). However, that should be a future PR.