Last Call Review of draft-ietf-dnssd-update-lease-07
review-ietf-dnssd-update-lease-07-dnsdir-lc-tale-2023-06-14-00
Request | Review of | draft-ietf-dnssd-update-lease |
---|---|---|
Requested revision | No specific revision (document currently at 08) | |
Type | Last Call Review | |
Team | DNS Directorate (dnsdir) | |
Deadline | 2023-06-13 | |
Requested | 2023-05-30 | |
Authors | Stuart Cheshire , Ted Lemon | |
I-D last updated | 2023-06-14 | |
Completed reviews |
Dnsdir Last Call review of -07
by David C Lawrence
(diff)
Secdir Last Call review of -07 by Shivan Kaul Sahib (diff) Genart Last Call review of -07 by Dale R. Worley (diff) Tsvart Last Call review of -07 by Brian Trammell (diff) Dnsdir Telechat review of -08 by David C Lawrence Intdir Telechat review of -08 by Jean-Michel Combes |
|
Assignment | Reviewer | David C Lawrence |
State | Completed | |
Request | Last Call review on draft-ietf-dnssd-update-lease by DNS Directorate Assigned | |
Posted at | https://mailarchive.ietf.org/arch/msg/dnsdir/WSvNuKc2DPCDobwOj8wynnSFVCg | |
Reviewed revision | 07 (document currently at 08) | |
Result | Ready w/issues | |
Completed | 2023-06-14 |
review-ietf-dnssd-update-lease-07-dnsdir-lc-tale-2023-06-14-00
A minor substantive issue that I think should be addressed, then some style nits. For substance, this specification leaves to inference what a server should do if it receives multiple dynamic update 'add' operations in one message that carries this option. I expect that you would say that obviously the lease time should be applied to all data added in the update, but it did take a little extra cognitive overhead for me to come to that conclusion. Along those lines, to be comprehensive you should explicitly specify that the data change caused by a 'delete' operation is not covered by the Update Lease option. (I don't think an implementer would really do that, but still, explicit is good.) The rest of this is nits, mostly stylistic, and it is not expected that you have to do anything about them. I think an implementor could work with the existing document well enough, and your disagreement with any of my style preferences is, of course, fine. While I personally like seeing the rationale for design decisions in an RFC, and realize there is some disagreement about that, the discussion of why you didn't use TTL is a distraction in the introduction. I'd move it toward the end, after the main body and before Security Considerations. I'd also rephrase the final sentence to drop "short enough to minimize stale cached data" because it felt awkward to me and you had just immediately prior already indicated that there were short TTLs to avoid stale data in caches. Section 2.1: s/Mechanism/Mechanisms/. Though I see that RFC 8490 has used the same singular, the referenced RFC 6891 and its predecessor RFC 2671 have been consistent in that the name via the initialism is with the plural. I'd go one step further and also just use EDNS instead of EDNS(0) as the shorthand to clean up the typography, though I see that we've been inconsistent about this across documents. For example, 8490 uses EDNS(0) as you have, I used EDNS0 in RFC 7871, and Mark Andrews used just EDNS in RFC 7314 -- which is how the DNS Terminology RFCs (7719 and 8499) label it with a nod to both EDNS0 and EDNS(0). Call it futureproofing; some day after the universal deployment of DNSSEC and IPv6 we will have an EDNS(1) which will be backward compatible with EDNS(0). Right? Right?? DNS-SD is only used once, in section 4, so could just have its expansion and RFC reference provided there. Section 4: Regarding TSIG, strictly speaking the RR need not appear after the OPT RR; the OPT data need only be included it in the digest but the message ordering is not defined in RFC 8945. This is also more generally about EDNS and TSIG and not specifically about UPDATE-LEASE, and so it shouldn't be necessary to describe that in section 4. 4.2: Because the value type is specified in seconds, I'd make the number of seconds the main part of the "no shorter than" sentence and move 30 minutes into the parenthetical. The second mention of 30 minutes is fine as-is. s/are for example 100/are, for example, 100/ per common style guides. Section 5.1: s/records affected the previous/records affected by the previous/ Is there a reference (RFC 6763 section?) that could be included to how dnssd typically does dynamic update and prerequisite handling? I realized as I was reading section 5.1 about refresh messages that I didn't really see how a refresh message would be different from a registration message, particularly as the section says that a refresh message can act as a registration message and also says that it's formatted the same except maybe with regard to prerequisites. Naively, I'd just do the equivalent of: update delete client.example.com a update add client.example.com a 192.168.1.102 .. but I'm pretty sure that is too simplistic. I was left wondering just how "Refresh Message Format" (5.1) was different from "Registration Message Format" (no section heading). Maybe it shouldn't have a section heading that seems to call it out as its own format rather than maybe having an additional constraint on prerequisites. (I'm not even sure this is the right interpretation.) Also in that section, "the response from the server can be used to determine how to proceed when the Refresh fails" could helpfully describe the basic strategy or maybe use a "For example, ..." Perhaps the reference to how dnssd usually does dynamic update already has discussion of handling failures? I'd like to see a section added with at least one example request/response pair, demonstrating the multiple 'add' situation, presumably of a client staking a name claim on its A and AAAA addresses. Thank you for providing recommended values.