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

dns-client: if /etc/resolv.conf modifies, update the internal list of resolvers #291

Merged
merged 5 commits into from
Feb 4, 2022

Conversation

hannesm
Copy link
Member

@hannesm hannesm commented Feb 3, 2022

Only affects lwt and unix.
Previously, when you started an application, /etc/resolv.conf was read once at
startup. When you switch to a different network, and /etc/resolv.conf is updated
by DHCP, this is not reflected in the dns-client.

Now, the digest of /etc/resolv.conf is stored internally, and if it changes, it
is parsed again. The lwt implementation needed to be modified slightly since
connect_to_ns_via_tcp carried a nameserver list, and a different task may have
been woken up. Now, there are two functions and it should play out nicely.

//cc @reynir addresses #243

… resolvers

Only affects lwt and unix.
Previously, when you started an application, /etc/resolv.conf was read once at
startup. When you switch to a different network, and /etc/resolv.conf is updated
by DHCP, this is not reflected in the dns-client.

Now, the digest of /etc/resolv.conf is stored internally, and if it changes, it
is parsed again. The lwt implementation needed to be modified slightly since
connect_to_ns_via_tcp carried a nameserver list, and a different task may have
been woken up. Now, there are two functions and it should play out nicely.
Copy link
Member

@reynir reynir left a comment

Choose a reason for hiding this comment

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

This looks good to me except I think we should not update the list of nameservers if the caller of Dns_client_{lwt,unix}.create explicitly passed a list of nameservers.

match nameservers with
| Some (`Udp, _) -> invalid_arg "UDP is not supported"
| Some (`Tcp, ns) -> ns
| Some (`Tcp, ns) -> ns, None
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should preserve whether the nameservers were passed explicitly and then not try to read resolv.conf.

In Dns_client_lwt and Dns_client_unix.
@haesbaert
Copy link
Member

Wouldn't it make more sense to just look at mtime on /etc/resolv.conf instead of constantly calculating digests ?
Or did I miss something ?

@reynir
Copy link
Member

reynir commented Feb 3, 2022

Wouldn't it make more sense to just look at mtime on /etc/resolv.conf instead of constantly calculating digests ? Or did I miss something ?

@haesbaert we discussed this on IRC and most of it is summarized in #296. Note that the lwt implementation will reuse the connection if possible and "only" re-read resolv.conf when reconnecting.

@haesbaert
Copy link
Member

Wouldn't it make more sense to just look at mtime on /etc/resolv.conf instead of constantly calculating digests ? Or did I miss something ?

@haesbaert we discussed this on IRC and most of it is summarized in #296. Note that the lwt implementation will reuse the connection if possible and "only" re-read resolv.conf when reconnecting.

Ack, sorry for the noise :D

@reynir
Copy link
Member

reynir commented Feb 3, 2022

I made a pull request to this pull request hannesm#1 where I implement the behavior I mentioned earlier.

@hannesm hannesm merged commit 7763da2 into mirage:main Feb 4, 2022
@hannesm hannesm deleted the client-resolv-conf branch February 4, 2022 11:44
hannesm added a commit to hannesm/opam-repository that referenced this pull request Mar 9, 2022
…er, dns-mirage, dns-client, dns-cli and dns-certify (6.2.0)

CHANGES:

* New opam package "dnssec" implementing dnssec validation (@reynir @hannesm)
* Use custom log sources, not the default one from Logs (@reynir @hannesm)
* BUGFIX dns-resolver: unlisten on the listen port, not the packet src_port
  (mirage/ocaml-dns#290 @hannesm)
* dns-resolver: add IPv6 addresses of root servers (fixes mirage/ocaml-dns#262, @hannesm)
* dns-resolver: preliminary support for DNSSec (mirage/ocaml-dns#262 @reynir @hannesm)
* dns-client: when /etc/resolv.conf modifies, update the list of nameservers
  (mirage/ocaml-dns#291 @hannesm @reynir)
* dns-cli: update to cmdliner 1.1.0 (mirage/ocaml-dns#300 @hannesm)
* dns-client-mirage: add module type and nameserver_of_string and connect to
  allow creation of a MirageOS device (mirage/ocaml-dns#297 @dinosaure)
* dns-cache: add size, capacity, and weight to metrics (mirage/ocaml-dns#301, fixes mirage/ocaml-dns#299,
  @hannesm)
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.

3 participants