Skip to content

use dns-client >= 4.0.0 in mirage-conduit#290

Merged
avsm merged 5 commits into
mirage:masterfrom
hannesm:udns
Aug 17, 2019
Merged

use dns-client >= 4.0.0 in mirage-conduit#290
avsm merged 5 commits into
mirage:masterfrom
hannesm:udns

Conversation

@hannesm
Copy link
Copy Markdown
Member

@hannesm hannesm commented Mar 19, 2019

replaces the dns resolver implementation. while at it, I found some code which I could safely remove (I tested end-to-end with canopy). see robur-coop/udns#14 for changes needed in udns

@hannesm
Copy link
Copy Markdown
Member Author

hannesm commented Mar 19, 2019

//cc @dinosaure @cfcs turns out most of resolver_mirage is not used (i tried cohttp-mirage, irmin-mirage and git-mirage as revdeps), and was safe to remove :D

Copy link
Copy Markdown
Member

@dinosaure dinosaure left a comment

Choose a reason for hiding this comment

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

Agree with this PR

@hannesm
Copy link
Copy Markdown
Member Author

hannesm commented Mar 19, 2019

oh, and there's actually a resolver API at https://roburio.github.io/udns/doc/udns-mirage-client/Udns_mirage_client/Make/index.html FWIW

this PR requires a udns release. furthermore, it modifies the semantics. instead of requesting 8.8.8.8 via udp, 91.239.100.100 is requested via tcp. also, udns is slightly more picky about domain names..

@dinosaure
Copy link
Copy Markdown
Member

I noticed these behaviors about tuyau where ocaml-uri is less paranoid but I prefer it (which mostly follow RFC) than before. My only concern is about the default resolver where 92.239.100.100 is a part of udns's internals and, from the patch, it's not clear that we use it now.

I don't know if it's possible to let end-user to specify a default resolver in udns and notice 92.239.100.100 statically here.

@hannesm
Copy link
Copy Markdown
Member Author

hannesm commented Mar 19, 2019

@dinosaure in Mirage_impl_resolver we already support ~ns ~ns_port labeled optional arguments, which are threaded to Udns. since the udns client is the library that needs a nameserver, I'm ok with it having a hardcoded (well, actually better would be multiple) recursive resolver.

@hcarty
Copy link
Copy Markdown
Contributor

hcarty commented Mar 19, 2019

What does 91.239.100.100 map to?

@hannesm
Copy link
Copy Markdown
Member Author

hannesm commented Mar 19, 2019

@hcarty anycast.censurfridns.dk, see https://blog.uncensoreddns.org/

@hcarty
Copy link
Copy Markdown
Contributor

hcarty commented Mar 19, 2019

@hannesm Thanks!

@hannesm
Copy link
Copy Markdown
Member Author

hannesm commented Mar 24, 2019

I especially would like to know whether the module type S (and the Make functor), removed in this PR, is used by someone somewhere (I tried reverse dependencies of mirage-conduit -- namely cohttp-mirage, git-mirage, and irmin-mirage -- they all don't use it)!? (it was introduced 4 years ago in f16721c, but to me it looks like nobody is using the plain Make anymore, and nobody ever used S)!?

@avsm
Copy link
Copy Markdown
Member

avsm commented Apr 2, 2019

The only one i can think of that might be using it is https://github.com/moby/vpnkit, cc @djs55

@djs55
Copy link
Copy Markdown
Member

djs55 commented Apr 2, 2019

I had a quick look -- there's no use of conduit in vpnkit. So feel free to delete module type S and Make with a clear conscience!

@hannesm
Copy link
Copy Markdown
Member Author

hannesm commented Apr 2, 2019

thanks! I still have to release udns, but this is likely to happen very soon now :)

@avsm
Copy link
Copy Markdown
Member

avsm commented Apr 2, 2019

I am massively looking forward to switching away from the venerable ocaml-dns :-)

@hannesm
Copy link
Copy Markdown
Member Author

hannesm commented Apr 2, 2019

@avsm yes, i hope https://github.com/roburio/udns/compare/next is my last rewrite before an initial release

@hannesm
Copy link
Copy Markdown
Member Author

hannesm commented Aug 14, 2019

this PR has been revised to the revised API of dns-mirage-client (taking the random device as functor argument) -- also the ns and ns_port are now (mirage/mirage#996) parameters (configuration time and runtime) -- thus I think all the issues have been addressed, and this is good to go and being released once dns is released (tomorrow!)

since I've not much clue about conduit releases, is it viable to merge, tag, and release only mirage-conduit (with version 4.0.0)? or how is the tooling to release conduit* as y.z and mirage-conduit as a different version (since they started off differently)!?

@hannesm hannesm changed the title use Udns in mirage-conduit use dns-client >= 4.0.0 in mirage-conduit Aug 15, 2019
@hannesm
Copy link
Copy Markdown
Member Author

hannesm commented Aug 15, 2019

ok, this was a bit more involved than anticipated. I took the opportunity (since there's anyways a split universe between those who use dns-1.y and dns-4.y) to rename mirage-conduit to conduit-mirage here -- this allows us to reset the version numbers for conduit-mirage as well (and start e.g. at 1.5.1, where other conduit packages are at -- no more manual messing around for a release).

this in addition adds a travis job which compiles conduit-mirage. if this is acceptable, please say so (or merge and release directly -- conduit-mirage is not yet used by anyone). once this is in opam, I'm happy to adjust other packages (most of which, since the API / module have not changed, may even just need a depends: ("mirage-conduit" | "conduit-mirage"), i.e. work with either old or new package names! :)

as a side-effect this fixes #178 then!

@hannesm
Copy link
Copy Markdown
Member Author

hannesm commented Aug 15, 2019

the main argument from #231 is not valid anymore, since the dominant naming is now YY-mirage (as can be seen on the dependencies onto mirage-conduit http://opam.ocaml.org/packages/mirage-conduit/) for YY in {git,cohttp,irmin,resp,h2,dns}.

@hannesm
Copy link
Copy Markdown
Member Author

hannesm commented Aug 15, 2019

turns out, it is a bit more work than adjusting opam files, since the references in dune need to be adjusted as well -- I'm fine PRing these changes to the projects, pls let me know what you think about the rename. I can as well revert these commits from this PR, and we can rename another time.

@hannesm
Copy link
Copy Markdown
Member Author

hannesm commented Aug 15, 2019

force-pushed the mirage-conduit changes only, the rename is over in https://github.com/hannesm/ocaml-conduit/tree/rename -- feel free to PR that branch here and merge if you feel like...

@avsm avsm merged commit 700d50b into mirage:master Aug 17, 2019
avsm added a commit to avsm/opam-repository that referenced this pull request Aug 17, 2019
… conduit-lwt-unix (2.0.0)

CHANGES:

* lwt-unix: obtain client IP correctly when using TLS connections (mirage/ocaml-conduit#277 @victorgomes)
* lwt-unix: replace the dune/ocaml file with a `(select)` build form.
  This avoids invoking `ocamlfind` from the build, and fits in with the
  rest of dune builds much more naturally (@avsm).
* lwt-unix: force callers to give a custom callback `on_exn` in case of exceptions
  to avoid random crashes (mirage/ocaml-conduit#261 @kit-ty-kate)
* mirage: use `dns-client>=4.0.0` which is the `udns` implementation (mirage/ocaml-conduit#290 @hannesm)
* mirage: rename `mirage-conduit` to `conduit-mirage` to fit the naming structure
  of this library suite more.  All new users of Mirage should use `conduit-mirage`,
  and migrating should involve simply swapping the name in your `dune` and `opam`
  files (mirage/ocaml-conduit#302 @hannesm @avsm)
* async: expose `verify_mode` correctly in `Conduit_async` (mirage/ocaml-conduit#298 @brendanlong)
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 this pull request may close these issues.

5 participants