Robert, thank you for your review and thank you all for the following discussion. I have entered a Discuss ballot for this document based on my own review.
Lars On 2022-1-22, at 1:01, Robert Sparks via Datatracker <nore...@ietf.org> wrote: > > Reviewer: Robert Sparks > Review result: Ready with Issues > > I am the assigned Gen-ART reviewer for this draft. The General Area > Review Team (Gen-ART) reviews all IETF documents being processed > by the IESG for the IETF Chair. Please treat these comments just > like any other last call comments. > > For more information, please see the FAQ at > > <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>. > > Document: draft-ietf-dots-telemetry-19 > Reviewer: Robert Sparks > Review Date: 2022-01-21 > IETF LC End Date: 2022-01-24 > IESG Telechat date: Not scheduled for a telechat > > Summary: Ready for publication as a Proposed Standard RFC, but with issues and > nits to consider before publication > > Issues: > ------- > > This document relies heavily on the requirements in the DOTS signal and data > channel specifications (RFC9132 and RFC8783) saying authenticated encryption > MUST be used. Explicitly calling that out in this document's security section > would be worthwhile. > > I am not a YANG Doctor. This document seems to make a reasonable but unusual > use of YANG. I would really like to see YANG Doctor review of -19. I wonder if > it would be possible to factor some of the more generic enums out into > something that could be reused elsewhere. Why do you stop at discouraging > rather than forbidding server deviations? The point of a specification like > this is interoperability. > > I'm a little uncomfortable with the passively stated requirement to use > ietf-dots-rehoming (at "clients are assumed to follow") while that document is > an Informative reference. > > In 6.1.2, I'm confused by the requirement that "'tsid' values MUST increase > monotonically when a new PUT is generated" combined with "If the dot server > finds the 'tsid' parameter value conveyed in the PUT request in its > configuration data and (it) has accepted the updated configuration parameters, > a 2.04 (Changed) MUST be returned". What am I missing that would allow a PUT > with a tsid that's already in the server's configuration data? Is there > perhaps > leftover tension here from earlier designs that were changed? > > At the end of 7.1, there is a MUST NOT level requirement against repeating > attack-descriptions that have been previously sent to a peer. Is it always the > case that when these descriptions are sent, the message they are contained in > is acknowledged? If not, consider adding discussion of how/whether a peer can > ask for a description when it gets an attack-id it doesn't have a description > for. > > In 7.2, you require a freshly rebooted client to send a GET request to > retrieve > active 'tmid' values. Why? Why not let it just send a DELETE with no 'tmid' > right away if that's what it would do after receiving the response to the GET > anyhow? > > In 7.3 at "The PUT request MUST be maintained in an active state", you mean > the > configuration that the PUT established must be kept. The PUT request itself > has > no state beyond its response. Consider saying something like "The > pre-or-ongoing mitigation requests MUST be considered active" instead. > > In the yang module, there's a reference at page 79 that points to "Section > 4.4.2 of RFC UUUU." This is the only occurrence of UUUU in the document, and > if > it's meant to be replaced with 'this RFC', this document has no section 4.4.2. > > Nits: > --------------- > > Last paragraph of introduction at "Nevertheless, when..". That sentence is > particularly hard to parse, please simplify it. > > Curious that you called out TCP port 80 in the discussion in 3.3. Why not 443? > > At 4.1, where you say "that truly require reliable delivery", I think you mean > something more like "that are important to deliver". Nothing here is trying to > guarantee reliable delivery and the statement misleads the reader to look for > reliable delivery mechanisms. > > In 4.5 at "The reason for not including these keys...". That sentence does not > parse. Please simplify. > > In 4.6, the second paragraph is about validating messages, not examples. It's > not clear the paragraph is useful as a roadmap through the document. Consider > deleting it. > > The outline structure of section 4 is odd. There are several Considerations > subsections including "Generic Considerations" (which is jarring), and several > subsections that are not considerations sections. Having them under a section > titled "Design Overview" doesn't help pull them together. Perhaps a mild > restructure/reorder would bring more clarity? > > At 6.1, consider replacing "(with suitable default)." with ". Default values > are defined later in this section." > > In 6.1 you claim clients can negotiate configuration parameters with server. I > don't find a negotiation protocol specified here. I think you mean clients can > configure parameters at a servers, and servers can configure parameters at a > client. There's not really a mechanism for a peer to say "no, I don't the > percentile config you're trying to set and I won't do that". > > The introduction of "a third-party DOTS server" in the narrative after Figure > 13 is surprising. For the paragraph to make sense, the messages in the > discussion before Figure 13 would also have to have gone to that third-part > DOTS server. Consider calling out that the DOTS server may not be hosted at > any > of the ISPs at the beginning of 6.2.1 instead. > > In 6.2.1 where you require servers to reject PUT requests with 0 capacity for > all included links, consider saying "Use DELETE instead." Maybe say _why_ you > require that PUT to be rejected. > > In the sentence after Figure 19 (at least in the text version), you mean to > point to Figure 20. > > At 7.1 "DOTS telemetry attributes are optionally signaled and therefore MUST > NOT be treated as mandatory fields in the DOTS signal channel protocol" is an > odd thing to call out this way. I think this was mentioned in the earlier yang > doctors review as well. Why is it here? > > In 7.1.1 consider replacing "the same attributes" with simply "the > attributes". > > In 7.1.6, please replace "reiterates" with "repeats". > > In 7.1.6, please replace the phrase "Concretely, " with "A". > > > > -- > last-call mailing list > last-c...@ietf.org > https://www.ietf.org/mailman/listinfo/last-call
signature.asc
Description: Message signed with OpenPGP
_______________________________________________ Gen-art mailing list Gen-art@ietf.org https://www.ietf.org/mailman/listinfo/gen-art
