Hi Authors, Thanks for working on this document.
Here is my AD review of draft-ietf-opsawg-ipfix-tcpo-v6eh-10 version of the draft. They are divided between COMMENT and NIT. I expect that the COMMENTs will be resolved before the document is sent to IETF Last Call. ------------------------------------------------------------------------------- COMMENT ------------------------------------------------------------------------------- Section 1, paragraph 1 Should the draft-ietf-opsawg-ipfix-fixes be referenced also? How about reference to RFC 7012 which is only mentioned in the Security Considerations section? Section 1.1, paragraph 12 > Section 3 addresses these issues. Also, ipv6ExtensionHeaders IPFIX > IE is deprecated in favor of the new IEs defined in this document. On the question of how a legacy client receiver receiving this new ipv6ExtensionHeader definition would react, I was wondering if a forward reference to the Operational Considerations be made. Otherwise, till one reads that section, it is not clear what a legacy client should do. Section 1.2, paragraph 3 > * Describe how any observed TCP option in a Flow can be exported > using IPFIX. Only TCP options having a kind <= 63 can be exported > in a tcpOptions IE. Is the problem that no TCP options can be exported using IPFIX, or is that TCP options having a Kind value >= 64 cannot be exported? Note, the first sentence starts by saying “how any observed TCP option in a Flow can(not) be exported”, the not added from the sentence above. Section 1.2, paragraph 4 > Section 4 addresses these issues. Also, tcpOptions IE is deprecated > in favor of the new IEs defined in this document. Same comment as above on providing a forward reference. Section 3.3, paragraph 7 > Abstract Data Type: unsigned256 I wonder why the opinion of a Expert Reviewer was overridden on the choice of defining this as unsigned256 instead of a bitfield. I understand that there is precedence of using unsigned values for "flags", but as Paul noted, the value of a unsigned number is meaningless in the case where the individual bits hold values, not the whole unsigned number. Was it because of reduced-encoding? If so, that should be stated as the reason. BTW, can a bitfield not have similar semantics of reduced-encoding, if all the (MSB) bits are not being used? Section 3.4, paragraph 7 > The same extension header type may appear several times in an > ipv6ExtensionHeaderTypeCountList Information Element. For > example, if an IPv6 packet of a Flow includes a Hop-by-Hop Options > header, a Destination Options header, a Fragment header, and > Destination Options header, the ipv6ExtensionHeaderTypeCountList > Information Element will report two counts of the Destination > Options header: the occurrences that are observed before the > Fragment header and the occurrences right after the Fragment > header. Could this example be made complete by mentioning what the IE will report as count for Fragment header, and Hop-by-Hop Options header? Section 3.5, paragraph 1 > Name: ipv6ExtensionHeadersLimit How are these names decided? Is the use of Limit the right way to express this? Could the name be ipv6ExtensionHeadersComplete or something that indicates the complete set of headers has been accounted for? Something similar was reported in the Transport Area review. Section 6.1, paragraph 1 > Figure 1 provides an example of reported values in an > ipv6ExtensionHeadersFull IE for an IPv6 Flow in which only the IPv6 > Destination Options header is observed. One octet is sufficient to > report these observed options. Concretely, the > ipv6ExtensionHeadersFull IE will be set to 0x01. Per Section 8.4.1, Initial Values for the [NEW_IPFIX_IPV6EH_SUBREGISTRY], the bit value for the Destination Options is bit 0. However, in the diagram bit 255 is shown set to 1. That can be confusing. Would it help to reverse the bit numbering keeping everything else the same? Section 6.1, paragraph 3 > Figure 2 provides another example of reported values in an > ipv6ExtensionHeadersFull IE for an IPv6 Flow in which the IPv6 Hop- > by-Hop Options, Routing, and Destination Options headers are > observed. One octet is sufficient to report these observed options. > Concretely, the ipv6ExtensionHeadersFull IE will be set to 0x23. Please refer to Section 8.4.1, Initial Values of the [NEW_IPFIX_IPV6EH_SUBREGISTRY] for folks to understand the bit assignments in this example. Section 6.2, paragraph 4 > Figure 3: First Example of TCP Options Rather than calling it First, could this be "Example of tcpOptionsFull"? Section 8.4.2, Guidelines for Designated Experts Want to make sure that Expert Review is an appropriate registration policy here. Or would Specification Required [RFC8126] be more appropriate? This policy is the same as Expert Review, with the additional requirement of a formal public specification. The next few lines are flagged because the tool is unable to determine the nature of the reference. You can ignore them. No reference entries found for these items, which were mentioned in the text: [NEW_IPFIX_IPv6EH_SUBREGISTRY]. Possible DOWNREF from this Standards Track doc to [IANA-IPFIX]. If so, the IESG needs to approve it. Possible DOWNREF from this Standards Track doc to [IANA-TCP]. If so, the IESG needs to approve it. Possible DOWNREF from this Standards Track doc to [IANA-EH]. If so, the IESG needs to approve it. Possible DOWNREF from this Standards Track doc to [IANA-Protocols]. If so, the IESG needs to approve it. Possible DOWNREF from this Standards Track doc to [IANA-TCP-EXIDs]. If so, the IESG needs to approve it. ------------------------------------------------------------------------------- NIT ------------------------------------------------------------------------------- All comments below are about very minor potential issues that you may choose to address in some way - or ignore - as you see fit. Some were flagged by automated tools (via https://github.com/larseggert/ietf-reviewtool), so there will likely be some false positives. There is no need to let me know what you did with these suggestions. Section 1.2, paragraph 2 Inconsistent use of Kind. Sometimes it is with a capital K, but other times it is with a small k. Section 7, paragraph 3 > This document does not add new security considerations for exporting > other IEs other than those already discussed in Section 8 of > [RFC7012]. s/exporting other IEs other/exporting IEs other/ "Abstract", paragraph 3 > ions and Management Area Working Group Working Group mailing list (opsawg@iet > ^^^^^^^^^^^^^^^^^^^^^^^^^^^ This phrase is duplicated. You should probably use "Working Group" only once. Section 2, paragraph 2 > ementID: TBD1 Description: The type of an IPv6 extension header observed in > ^^^^^^^^^^ If "type" is a classification term, "an" is not necessary. Use "type of". (The phrases "kind of" and "sort of" are informal if they mean "to some extent".). Section 3.3, paragraph 4 > igned256 I wonder why the opinion of a Expert Reviewer was overridden on the > ^ Use "an" instead of "a" if the following word starts with a vowel sound, e.g. "an article", "an hour". Section 3.3, paragraph 5 > ags", but as Paul noted, the value of a unsigned number is meaningless in the > ^ Use "an" instead of "a" if the following word starts with a vowel sound, e.g. "an article", "an hour". Section 3.5, paragraph 8 > rting such information may help identifying root causes of performance degra > ^^^^^^^^^^^ The verb "help" is used with an infinitive. Section 4.2, paragraph 6 > [RFC8883] for a behavior of an intermediate nodes that encounters an unknown > ^^^^^^^^^^^^^^^^^^^^^ The plural noun "nodes" cannot be used with the article "an". Did you mean "an intermediate node" or "intermediate nodes"? Mahesh Jethanandani mjethanand...@gmail.com
_______________________________________________ OPSAWG mailing list OPSAWG@ietf.org https://www.ietf.org/mailman/listinfo/opsawg
