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

expose Dns_server.update_data #350

Merged
merged 1 commit into from
May 17, 2024
Merged

Conversation

RyanGibb
Copy link
Contributor

For out-of-band updates with the same semantics as DNS UPDATE. I've experimenting with a capnproto interface that uses this function.

@RyanGibb RyanGibb changed the title expose update_data expose Dns_server.update_data Apr 28, 2024
* Dns.Packet.Update.update list Domain_name.Map.t ->
( Dns_trie.t * (Domain_name.Set.elt * Dns.Soa.t) list,
Dns.Rcode.t )
result
Copy link
Member

Choose a reason for hiding this comment

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

there's need for a docstring describing the semantics -- and what will be done, what won't be (e.g. authentication).

also, the return value should be described in detail -- what the second projection is supposed to contain, etc.

I also wonder what the operation for a DNS server should be -- so once an update_data has been performed, there may be need to notify secondary servers -- how's this supposed to happen? and with your cap'n'proto work, how do you notify the other servers when a DNS update is performed?

@hannesm
Copy link
Member

hannesm commented May 17, 2024

I'm really uncertain about the semantics this should have, in terms of notifying other servers (since the notification mechanism is not exposed via API as far as I know, a potential client could use this proposed update_data and get out-of-sync name servers).

Maybe one path is to refactor the handle_packet function, but I just looked into it, and there's as well the incremental zone transfer support (update_trie_cache), and the notification mechanism.

On the other hand, we can move along and just merge it -- there's already the handle_update exposed, and with_data. Maybe there just needs to be more clear documentation at these functions.

@hannesm hannesm merged commit 9f395ef into mirage:main May 17, 2024
1 check passed
@RyanGibb
Copy link
Contributor Author

I'm really uncertain about the semantics this should have, in terms of notifying other servers
On the other hand, we can move along and just merge it -- there's already the handle_update exposed

Yep, I think handle_update has the same issue.

and with your cap'n'proto work, how do you notify the other servers when a DNS update is performed?

I haven't added this yet, but one option would be to propagate all updates over the RPC interface Though this might be a bit wasteful if the prerequisite conditions aren't met. If I'm reading the code correctly handle_update should return an Error if they aren't met, so we could forward the updates conditional on an Ok result.

hannesm added a commit to hannesm/opam-repository that referenced this pull request May 29, 2024
CHANGES:

* dns-client (lwt, mirage): depend on happy-eyeballs-{lwt,mirage} instead of
  duplicating the code. This requires happy-eyeballs 1.1.0, and now the same
  Happy_eyeballs_{lwt,mirage}.t is used for DNS (connecting to the nameserver)
  and for the application (connecting to a remote host)
  (@dinosaure @hannesm mirage/ocaml-dns#346)
* server: improve API documentation (@hannesm
  1a80bd4080e597687152cf351d035ef5f00c5946
  000ae02dfc477d91c05891e3891a447328ae448a)
* server: add a `packet_callback` to `handle_packet` and `handle_buf`
  (@RyanGibb mirage/ocaml-dns#349)
* server: expose `update_data` (@RyanGibb mirage/ocaml-dns#350)
* resolver: b root name server IP change (@hannesm mirage/ocaml-dns#348)
* secondary server [mirage]: avoid infinite loop in connect (avoids SYN floods)
  (@hannesm @reynir mirage/ocaml-dns#347)
* resolver, dns_zone: use consistently `Log` instead of `Logs` (@palainp mirage/ocaml-dns#342)
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

2 participants