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

Vendor the OCaml DNS code #542

Merged
merged 22 commits into from
May 20, 2021
Merged

Vendor the OCaml DNS code #542

merged 22 commits into from
May 20, 2021

Conversation

djs55
Copy link
Collaborator

@djs55 djs55 commented May 14, 2021

Previously we used 2 libraries:

  • ocaml-dns-forward: this was originally split out from this tree
  • ocaml-dns

However:

  • the upstream ocaml-dns is completely different from version 2.0.0 onwards. It's now based on a DNS library with different goals, prioritising standards compliance; while we prioritise backwards compatibility with old resolvers. If we updated to the new implementation we might expose bugs in old clients which we wouldn't be able to work around easily. It would be interesting to try upgrading later, after we have updated all the other dependencies.
  • the ocaml-dns-forward library was never used by anyone else. It's really an implementation of vpnkit-specific policy so should live in this repo anyway.

Furthermore

  • ocaml-dns contains lots of code we don't need and dependencies we don't want (e.g. hashcons). By vendoring it we can avoid the dependencies from the unused code.
  • probably a lot of ocaml-dns-forward is now unused, since we don't try to use it to talk to split-tunnel DNS servers in VPNs. It will be easier to trim if all the code lives in one place.

This is a required step on the plan to vendor all the OCaml dependencies, to make building easier (and on Windows: to make building possible again)

djs55 added 16 commits May 13, 2021 11:31
```
File "src/hostnet/vmnet.ml", line 327, characters 30-42:
327 |   let client_negotiate ~uuid ?preferred_ip ~fd =
                                    ^^^^^^^^^^^^
Error (warning 16 [unerasable-optional-argument]): this optional argument cannot be erased.
```

Signed-off-by: David Scott <dave@recoil.org>
This was originally split into a library djs55/ocaml-dns-forward
which became mirage/ocaml-dns-forward but it's only useful inside
this project so we should inline it.

Signed-off-by: David Scott <dave@recoil.org>
Signed-off-by: David Scott <dave@recoil.org>
This vendors the old version of ocaml-dns that we still use.

Updating to the new APIs could be done, but much of this code is unused
so we should prioritise code elimination first.

Signed-off-by: David Scott <dave@recoil.org>
Signed-off-by: David Scott <dave@recoil.org>
Signed-off-by: David Scott <dave@recoil.org>
Signed-off-by: David Scott <dave@recoil.org>
Signed-off-by: David Scott <dave@recoil.org>
Signed-off-by: David Scott <dave@recoil.org>
Signed-off-by: David Scott <dave@recoil.org>
Signed-off-by: David Scott <dave@recoil.org>
Signed-off-by: David Scott <dave@recoil.org>
Signed-off-by: David Scott <dave@recoil.org>
Signed-off-by: David Scott <dave@recoil.org>
Signed-off-by: David Scott <dave@recoil.org>
Signed-off-by: David Scott <dave@recoil.org>
djs55 added 6 commits May 14, 2021 14:23
Signed-off-by: David Scott <dave@recoil.org>
Signed-off-by: David Scott <dave@recoil.org>
Signed-off-by: David Scott <dave@recoil.org>
Signed-off-by: David Scott <dave@recoil.org>
Signed-off-by: David Scott <dave@recoil.org>
Signed-off-by: David Scott <dave@recoil.org>
@samoht
Copy link
Member

samoht commented May 20, 2021

LGTM

@djs55 djs55 merged commit 0b63102 into moby:master May 20, 2021
@djs55 djs55 deleted the dns-forward branch May 20, 2021 12:58
@avsm
Copy link
Collaborator

avsm commented May 20, 2021

Looks fine. You may want to add a (vendored_dirs) field to cover the dns directory. https://dune.readthedocs.io/en/stable/dune-files.html#vendored-dirs-since-1-11

@djs55
Copy link
Collaborator Author

djs55 commented May 20, 2021

@avsm thanks, useful tip!

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.

None yet

3 participants