Hi Med, Thanks for addressing most of the comments. Two follow-up comments. See below with [mj]
> On Apr 15, 2024, at 4:51 AM, mohamed.boucad...@orange.com wrote: > > Hi Mahesh, > > Thank you for the review. > > The changes can be tracked at: Diff: draft-ietf-opsawg-ipfix-tcpo-v6eh.txt - > draft-ietf-opsawg-ipfix-tcpo-v6eh.txt > <https://author-tools.ietf.org/api/iddiff?url_1=https://boucadair.github.io/ipfix-tcpoptions-and-v6eh/draft-ietf-opsawg-ipfix-tcpo-v6eh.txt&url_2=https://boucadair.github.io/ipfix-tcpoptions-and-v6eh/AD-review/draft-ietf-opsawg-ipfix-tcpo-v6eh.txt>. > > Please see more context inline. > > Cheers, > Med > > De : Mahesh Jethanandani <mjethanand...@gmail.com > <mailto:mjethanand...@gmail.com>> > Envoyé : dimanche 14 avril 2024 21:42 > À : draft-ietf-opsawg-ipfix-tcpo-v...@ietf.org > <mailto:draft-ietf-opsawg-ipfix-tcpo-v...@ietf.org> > Cc : opsawg@ietf.org <mailto:opsawg@ietf.org>; pait...@ciena.com > <mailto:pait...@ciena.com> > Objet : AD review of draft-ietf-opsawg-ipfix-tcpo-v6eh > > 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? > > [Med] We used to refer to that I-D till the WG decided to deprecate the IEs. > No need to have that ref back. > > How about reference to RFC 7012 which is only mentioned in the Security > Considerations section? > [Med] That’s OK as the authoritative reference for the IEs/data types is the > IANA registry itself. > > 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. > > [Med] Not sure what you meant by “legacy client receiver”, but I suspect you > mean the collector. If that is what you had in mind, then usual rfc7011 > applies for manipulating templates, etc. > > 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. > > [Med] That’s an issue because we don’t have full visibility on the options > present in flow. For example, TCP-ENO or shared option can’t be reported with > (deprecated) tcpOptions. [mj] În that case, do you want to: s/Describe how any observed TCP options/Describe how some observed TCP options/ > > > 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? > [Med] The current design is consistent with how flags are handled in IPFIX. > As you also rightfully mentioned, support of reduced-encoding is a key > feature to support here given the long type. Please note that RFC7011 is > explicit about target types: > > Reduced-size encoding MAY be applied to the following integer types: > unsigned64, signed64, unsigned32, signed32, unsigned16, and signed16. > The signed versus unsigned property of the reported value MUST be > preserved. The reduction in size can be to any number of octets > smaller than the original type if the data value still fits, i.e., so > that only leading zeroes are dropped. For example, an unsigned64 can > be reduced in size to 7, 6, 5, 4, 3, 2, or 1 octet(s). > > As a side note, we discussed with Benoît whether we need we tag this I-D as > formally updating RFC7011 but we finally preferred to no do so because the > authoritative registry for the data type is the registry. [mj] Regardless, are you not updating RFC 7011 to include unsigned256 in reduced-size 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? > > [Med] There is no “bitfield” data type. The only mention of “bit field” in > 7011 is the following > > > "flags" is an integral value that represents a set of bit fields. > Logical operations are appropriate on such values, but other > mathematical operations are not. Flags MUST always be of an unsigned > data type. > > “bit field” is used for almost all IEs with flags (IP Flow Information Export > (IPFIX) Entities (iana.org) > <https://www.iana.org/assignments/ipfix/ipfix.xhtml>), but with an unsignedXX > abstract data type. > > 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? > [Med] Done. > > 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. > [Med] This IE is about report a limit (hardware or software). We used “limit” > rather “complete” and the like to avoid confusion with > “ipv6ExtensionHeadersFull” > > 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? > > [Med] The text says the following: > > “Bit 0 corresponds to the least-significant bit in the > ipv6ExtensionHeadersFull IE while bit 255 corresponds to the most-significant > bit of the IE.” > > We are not reversing the bit numbering in the figure because of established > conventions in IPFIX specs. > > 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. > [Med] Good point. Done. > > Section 6.2, paragraph 4 > > Figure 3: First Example of TCP Options > > Rather than calling it First, could this be "Example of tcpOptionsFull"? > [Med] OK > > 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. > [Med] The specification is already required to add new (or delete existing) > entries to the (parent) IPv6 registry that is mirrored here. While we don’t > expect to solicit much Experts for review given that mirroring will be the > likely mode, we added a provision for expert review to cover specific cases > where IANA would require some feedback on some actions. > > The next few lines are flagged because the tool is unable to determine the > nature of the reference. You can ignore them. > [Med] All these are false positives. > > 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 > <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. > > [Med] Good catch. Went with “Kind” to be consistent with RFC9293. Thanks. > > 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/ > > [Med] Fixed. Thanks. > > "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. > > [Med] No change is needed here. > > 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".). > > [Med] OK > > 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". > > [Med] I failed to find this one. > > 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". > > [Med] I failed to find this one. > > Section 3.5, paragraph 8 > > rting such information may help identifying root causes of performance degra > > ^^^^^^^^^^^ > The verb "help" is used with an infinitive. > > [Med] changed to “might help” > > 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"? > > > [Med] ACK > > Mahesh Jethanandani > mjethanand...@gmail.com <mailto:mjethanand...@gmail.com> > > > > > > > ____________________________________________________________________________________________________________ > Ce message et ses pieces jointes peuvent contenir des informations > confidentielles ou privilegiees et ne doivent donc > pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu > ce message par erreur, veuillez le signaler > a l'expediteur et le detruire ainsi que les pieces jointes. Les messages > electroniques etant susceptibles d'alteration, > Orange decline toute responsabilite si ce message a ete altere, deforme ou > falsifie. Merci. > > This message and its attachments may contain confidential or privileged > information that may be protected by law; > they should not be distributed, used or copied without authorisation. > If you have received this email in error, please notify the sender and delete > this message and its attachments. > As emails may be altered, Orange is not liable for messages that have been > modified, changed or falsified. > Thank you. Mahesh Jethanandani mjethanand...@gmail.com
_______________________________________________ OPSAWG mailing list OPSAWG@ietf.org https://www.ietf.org/mailman/listinfo/opsawg
