adapt to domain-name phantom type API #31
Conversation
There was a problem hiding this comment.
Scanned over, most of the DNS logic I lack background knowledge / context to review properly.
What does the ?mac argument in the tsig_verify code represent / do? It feels a bit scary reading this with so many optional arguments.
EDIT: Re: SOA + odig: Agreed, perhaps the validation function could return the rejected value in a polymorphic error variant so that odig can pick it up?
| Log.err (fun m -> m "error %a while establishing tcp connection to %a:%d" | ||
| T.pp_error e Ipaddr.V4.pp ip dport) ; | ||
| Lwt.async (fun () -> | ||
| TIME.sleep_ns (Duration.of_sec 5) >>= fun () -> |
There was a problem hiding this comment.
Why are we waiting 5 seconds here before closing?
| (* outstanding notifications, with timestamp and retry count | ||
| (at most one per zone per ip) *) | ||
| type outstanding = | ||
| (int64 * int * Packet.t * Domain_name.t option) Domain_name.Map.t IPM.t | ||
| (int64 * int * Cstruct.t option * Packet.t * Domain_name.t option) Domain_name.Map.t IPM.t |
There was a problem hiding this comment.
I assume int64 is timestamp, int is retry count, maybe document what the Cstruct.option is?
| let packet = | ||
| let question = Packet.Question.create zone Soa | ||
| and header = | ||
| (* TODO: all are getting the same ID *) | ||
| let id = Randomconv.int ~bound:(1 lsl 16 - 1) server.rng in |
There was a problem hiding this comment.
How about Randomconv.int16 server.rng, or does this need to be 0 .. 65534 specifically?
65534 (1 lsl 16 = 0x10000, 1 lsl 16 - 1 = 0xffff, bound being exclusive in Randomconv.int ~bound meaning you get 0 ... 0xfffe inclusively) seems like a weird number to me; usually you'd want 0x0 ... 0xffff?
well, the request does not contain a mac, but the reply the mac of the request, as defined in https://tools.ietf.org/html/rfc2845
that's my home-grown reconnection logic ;) wait 5 seconds till next attempt ;p (yes, the connection logic in mirage requires some work)
yes, good idea. its the mac of the request (in this case a notify)
I guess #9 should include this.. I opened #32 which superseeds this (but does not address your points, will attempt to work on a separate PR with your concerns above) and #29 |
esp last commit is of interest, will clean up other commits. I'm sure there's more
[ `host ] Domain_name.tto be introduced (i.e. all zone boundaries should be on hostnames, ...) -- but it's also a bit unclear where to require hostnames, and where domain names are sufficient (i.e. zone parser -- should origin be a hostname? can we verify that each zone we add to dns_trie is a hostname?).in respect to regression2/3 -- I'm not sure whether SOA nameserver (/MX) should really be hostname -- from what our server should accept, this is very reasonable, for what
odigshould accept & print, I think this is a bit too limited.