Directly use Happy_eyeballs_lwt instead of a copy of it

[dinosaure]

This commit delete a dependency cycle between happy-eyeballs, happy-eyeballs-lwt, dns and dns-client-lwt. The basic happy-eyeballs-lwt implementation is not able yet to resolve domain-name but the user can inject a getaddrinfo which may come from dns-client-lwt. The idea is: 1) create a happy-eyeballs-lwt instance 2) create a dns-client-lwt instance 3) inject Dns_client_lwt.getaddrinfo into our happy-eyeballs-lwt instance

This patch delete a duplicate code about happy-eyeballs implementations.

Related to robur-coop/happy-eyeballs#38

[dinosaure]

We probably can fully delete the cycle between happy-eyeballs-lwt and dns-client-lwt and replace 'response Dns.Rr_map.key by a polymorphic variant for instance.

[hannesm]

We probably can fully delete the cycle between happy-eyeballs-lwt and dns-client-lwt and replace 'response Dns.Rr_map.key by a polymorphic variant for instance.

Indeed, but that would be rather itchy, no? We'd need to have `A | `AAAA as queries, and then `A of Ipaddr.V4.t set (or list) | `AAAA of Ipaddr.V6.t set as replies... I'm quite fine with having happy-eyeballs-lwt depend on Dns.

[hannesm]

Hmm, I guess I'm still a bit puzzled. What I thought about the API:

• dns-client-lwt now uses happy-eyeballs-lwt well done
• the Dns_client_lwt.create creates a happy_eyeballs_lwt.t with getaddrinfo being defined by dns-client-lwt internally
• there's a way for a client to retrieve this happy-eyeballs instance (i.e. to get one that uses the ocaml dns stack)

So, what is there to note? I believe each application should only have a single happy-eyeballs instance. There's no need for having multiple (there would be if there would be multiple network stacks, but there aren't). A single instance means a shared DNS cache, and a simpler way to debug everything. With this PR, the dns-client-lwt uses the very same happy-eyeballs instance \o/

So, for a client there are then two choices (by default):

• use happy-eyeballs-lwt (-> no dependency on ocaml-dns / dns-client / dns-client-lwt) <- using the standard Lwt_unix.getaddrinfo
• use dns-client-lwt.happy-eyeballs -> no libc getaddrinfo involved

Does this make sense? This also means that we do not need a inject operation, but can just provide the getaddrinfo / getaddrinfo6 when we instantiate the happy-eyeballs.

[hannesm]

issues with the proposal above (and my changes to your happy-eyeballs PR):

• the Dns_client_lwt.create () needs to construct a Happy_eyeballs_lwt.t and feeds itself as resolver into it --> this is tricky, since getaddrinfo is only available once the Dns_client.Make functor has been applied
• timeouts: for DNS resolution and TCP connection setup there may be different timeouts needed. Currently, happy-eyeballs only knows about a single (global) timeout setting

So, another proposal:

• let's leave Dns_client_lwt mostly as-is (but using a Happy_eyeballs_lwt.t internally)
• let's provide a Dns_client_lwt.create_happy_eyeballs : -> Dns_client_lwt.t -> Happy_eyeballs.t -- this way, we may have multiple Happy_eyeballs_lwt.t in one application (thus no shared DNS cache - similar to we may have multiple Dns_client_lwt.t around) -- but if you like to avoid libc's getaddrinfo, you can just get your Happy_eyeballs_lwt.t from Dns_client_lwt, and be fine with it. :)

[hannesm]

see 3fc14f0b0da8aac45547424f0c7ce0182ae137be what I have in mind :)

[hannesm]

I rebased and added some commits, this is now as well adapting dns-client-mirage to use happy-eyeballs-mirage.

A review would be great (API + whether it works), it seems to compile, but I haven't tested it.

[dinosaure]

I finally tried to make a simple unikernel.ml:

let ( let* ) = Lwt.bind

module Make
  (S : Tcpip.Stack.V4V6)
  (H : Happy_eyeballs_mirage.S with type flow = S.TCP.flow)
  (D : Dns_client_mirage.S) = struct
  let start _stack he _dns =
    let* flow = H.connect he "google.com" [ 80; 443 ] in
    match flow with
    | Ok ((ipaddr, port), flow) ->
      Fmt.pr ">>> connected to google.com via %a:%d\n%!" Ipaddr.pp ipaddr port;
      S.TCP.close flow
    | Error (`Msg msg) -> failwith msg
end

And this is the config.ml.

open Mirage

let connect_handler =
  main "Unikernel.Make"
    (stackv4v6 @-> happy_eyeballs @-> dns_client @-> job)

let stackv4v6 = generic_stackv4v6 default_network
let happy_eyeballs = generic_happy_eyeballs stackv4v6
let dns = generic_dns_client stackv4v6 happy_eyeballs

let () = register "connect" [ connect_handler $ stackv4v6 $ happy_eyeballs $ dns ]

Of course, we need to upgrade the mirage tool and reverse again the dependency. I did a draft here: mirage/mirage#1543. WDYT?

[hannesm]

your example looks nice. but can we remove the "dns" and generic_dns_client stackv4v6 happy_eyeballs from config.ml?

[dinosaure]

your example looks nice. but can we remove the "dns" and generic_dns_client stackv4v6 happy_eyeballs from config.ml?

So we can ignore it but we must explicitly say that we want the DNS stack at some points to be able to do some DNS resolution into our happy-eyeballs instance. We can make a "super device" in the mirage tool then which aggregates happy_eyeballs's arguments (timeout) and dns's arguments (nameservers) to return an happy_eyeballs instance which is able to resolve domain-names (and just ignore then the dns instance) but such things can be done only into the mirage tool - I don't have any idea to do so at this level (without the mirage tool) nicely.

[hannesm]

And looking into your code, esp. in the mirage changes, I guess:

• providing a dns-client-mirage that is instantiated and provides the Dns_client.S API is nice (which used to be what generic_dns_client did)
• provided a happy_eyeballs device which depends on the dns thing, and then injects getaddrinfo before passing it to the user would be nice.

but I've no idea how we can write that in the mirage tool. maybe we can depend on generic_dns_client in happy_eyeballs device, and then bind a getaddrinfo and call inject from the mirage tool?

[hannesm]

I'd like a user of happy eyeballs device be:

let ( let* ) = Lwt.bind

module Make
  (S : Tcpip.Stack.V4V6)
  (H : Happy_eyeballs_mirage.S with type flow = S.TCP.flow) = struct

  let start _stack he =
    let* flow = H.connect he "google.com" [ 80; 443 ] in
    match flow with
    | Ok ((ipaddr, port), flow) ->
      Fmt.pr ">>> connected to google.com via %a:%d\n%!" Ipaddr.pp ipaddr port;
      S.TCP.close flow
    | Error (`Msg msg) -> failwith msg
end

And this is the config.ml.

open Mirage

let connect_handler =
  main "Unikernel.Make"
    (stackv4v6 @-> happy_eyeballs @-> job)

let stackv4v6 = generic_stackv4v6 default_network
let happy_eyeballs = generic_happy_eyeballs stackv4v6

let () = register "connect" [ connect_handler $ stackv4v6 $ happy_eyeballs ]

Do you think that is possible?

[dinosaure]

To re-centralize my initial point, this PR and the PR on happy-eyeballs re-contextualize happy-eyeballs as a mechanic to handle something like connect(2) with proper timeouts firstly. Then, we can (and usually we do) extend it via a DNS stack (ocaml-dns) to be able to gethostbyname(3) and connect(2) but, for my point of view, it's not necessary the case - we could also want an happy-eyeballs without the DNS stack. This view is concretized by the fact that we must explicitly say that we want a DNS stack via generic_dns_client.

Do you think that is possible?

So it's actually possible to hide everything and say that the given happy-eyeballs allocates a dns-client and do the Happy_eyeballs_mirage.inject in the produced code of the mirage tool - we have two possibilities here, we can just use a first-class module and locally apply the Dns_client_mirage.Make client and create a dns-client instance & inject it and returns the happy_eyeballs instance or we can provide another module like Happy_eyeballs_mirage_with_dns.Make which will do the plumbing for us. But this means that a DNS instance is forgotten for the user perspective.

If we go down this path, we must extend arguments of Mirage.happy_eyeballs to be able to specify dns-clients's arguments like nameservers (and Mirage.happy_eyeballs already has several arguments).

[hannesm]

From our discussion on IRC, we figured:

• Dns_client_mirage.S.create should take the happy_eyeballs (at least optionally) instead of instantiating their own (this is a bit tricky, but likely we can modify the type stack to contain a pair of TCPIP-stack and Happy_eyeballs).
• We need to rethink the timeouts, and eventually allow in happyeyeballs that each `connect` can pass their own timeout.

The reasoning for the latter is that we will use only a single happy-eyeballs instance across the entire unikernel:

• a client of happy_eyeballs wanting to connect to robur.coop on port 80 would like to finish that within 5 seconds or get a failure (so, the happy-eyeballs instance is setup with the connect timeout of 5 seconds)
• this imposes (a) DNS resolution (b) connect to some IP addresses, where (a) again uses happy-eyeballs to connect to the name servers
• going deeper, the connect_timeout is the entire time, but the DNS resolution should take less time (since there are retries (try another name server), we need time for establishing a connection (i.e. the connect TCP 3 way handshake), ...) --> we have a distinct resolve_timeout
• since we only have a single happy-eyeballs, the connect_ip connecting to the name server(s) needs to use a different connect timeout than the overall one --> we need to be able to specify this at this point

We should settle this (as mentioned, it likely requires some minor modification to happy-eyeballs) to get this through the door and cut a release of DNS. I plan to work on this today.

[hannesm]

Timeouts and delays can be specified during establishing a connection in happy-eyeballs with https://github.com/robur-coop/happy-eyeballs/pull/42

[hannesm]

Ok, I updated this PR in respect to the happy-eyeballs 1.1.0 release, and the discussion we had on IRC @dinosaure. Would be great if you could take a look into the recent commits, and whether this is in line with what you think should be done.

Now, there's happy_eyeballs being passed around and extended with getaddrinfo. The timeout for connect_ips is the one passed to Dns_client.create -- which is a good approximation (still not the right value, as remarked in the source comment).

[dinosaure]

I updated the mirage/mirage#1543 according to your recent change. The unikernel itself did not change but it compiles again if you pin this version of dns-client-mirage.

[dinosaure]

For me, the PR is ok to merge now :+1:. And the mirage devices are updated. The example given below works and was tested.