Thanks Paul for the detailed review. Please see inline On Mon, 21 Apr 2025 at 04:15, Paul Kyzivat <[email protected]> wrote:
> Reviewer: Paul Kyzivat > Review result: Ready with Nits > > I am the assigned ARTART reviewer for this Internet-Draft. > > Document: draft-ietf-dnsop-structured-dns-error-12 > Reviewer: Paul Kyzivat > Review Date: 2025-04-20 > IETF LC End Date: 2025-04-28 > IESG Telechat date: ? > > Summary: This draft is on the right track but has open issues, described > in the review. > > ISSUES: 7 > NITS: 1 > > Issues: > > 1) NIT: Section 4 - c: (contact) > > This allows sips but not sip URIs. Sips is not widely used. > Please consider allowing sip URLs. > Allowing "sip" URI introduces security issues, "sips" offers encrypted transport for SIP messages. > > 2) ISSUE: Section 4 - s: (suberror) > > This field lacks a specification of its type. > > It appears that "suberror" here is intended to be the same as > "sub-error" in section 7 and "SubError" in section 11.3. Please use a > consistent spelling throughout. And then specify here that the type of > this field is an integer with values defined in the new IANA registry. > Thanks, updated draft to use "sub-error". > > 3) ISSUE: Section 8 - Extended DNS Error Code > > The phrasing here, for both the section title and the content, is odd > and confusing. For clarity and consistency with section 7, I suggest a > title of "New Extended DNS Error Code Definition". > > And then the body could start with: "This document defines the following > new IANA-registered Extended DNS Error Code." The existing text will > then require some tweaking to align with this rephrasing. > > And then to avoid confusion, perhaps change the title of section 11.4 to > "New Extended DNS Error Code Registration". > No, the section title and its body is consistent with the sections in RFC 8914 defining Extended DNS Error Codes, please see https://datatracker.ietf.org/doc/html/rfc8914#section-4 > > 4) ISSUE: Section 9 - Examples > > I fail to see how Figure 2 represents the same content as Figure 1. If > it does, can you please explain? > The script in https://github.com/ietf-wg-dnsop/draft-ietf-dnsop-structured-dns-error/blob/main/examples/minified.json was supposed to update Figure 2, I fixed it. > > 5) ISSUE: Section 11.1 - New Registry for JSON Names > > Some of the fields described in the text are inconsistent with the > fields contained in Table 1: "Short Description" vs. "Description", and > no text description of "Full JSON Name". > Also, is "Full JSON Name" appropriate? IIUC it has no role in JSON. > Rather, it is just a human meaningful long form of the JSON Name, or > perhaps a shorter form of the "Short Description". I suggest rethinking > what you are calling these things. > Good point, I replaced "Full JSON Name" with "Field meaning" and addressed the above comment as well. > > 6) ISSUE: Section 11.2 - New Registry for Contact URI Scheme > > Could you please add some text describing the role and responsibilities > of the Change Controller? What sort of changes are allowed? More than > additions? > IETF review is required to update the registry, see https://datatracker.ietf.org/doc/html/rfc8126#section-4.8, change controller is IETF. > > 7) ISSUE: Section 11.3 - New Registry for DNS SubError Codes > > I don't understand what you mean by "RFC8914 error code applicability". > > First, what do you mean by "RFC8914 error code"? Do you mean the > "Extended DNS Error Codes" defined in RFC8914? > Yes, updated to use "Extended DNS Error Codes". > > Next, what do you mean by "applicability"? Do you mean the "Extended DNS > Error Codes" for which the "SubError Codes" may be used? > Yes. > > Please clarify these. > > Also, again, could you please add some text describing the role and > responsibilities of the Change Controller? What sort of changes are > allowed? More than additions? > IETF review is required to update the registry, see https://datatracker.ietf.org/doc/html/rfc8126#section-4.8, change controller is IETF. > > 8) ISSUE: JSON Name > > Throughout the document you use "JSON Name" to describe a specific field > in a specific JSON document format. This isn't descriptive of the > purpose of the field. I suggest changing this to something more > descriptive - perhaps "EXTRA-TEXT Field Name". > Yes, fixed. Cheers, -Tiru
_______________________________________________ DNSOP mailing list -- [email protected] To unsubscribe send an email to [email protected]
