Hi Paul, Thank you for the review.
A PR can be seen at: https://github.com/boucadair/ipfix-tcpoptions-and-v6eh/pull/10/files. Please see inline for more context. Cheers, Med De : Aitken, Paul <pait...@ciena.com> Envoyé : jeudi 19 octobre 2023 21:05 À : BOUCADAIR Mohamed INNOV/NET <mohamed.boucad...@orange.com> Cc : ip...@ietf.org; Eric Vyncke (evyncke) <evyncke=40cisco....@dmarc.ietf.org>; opsawg@ietf.org; Benoit Claise <benoit.cla...@huawei.com> Objet : RE: [IPFIX] Full or Truncated EHs RE: Some comments on draft-ietf-opsawg-ipfix-tcpo-v6eh Med, I reviewed the current draft-ietf-opsawg-ipfix-tcpo-v6eh. 1.1 "Specify how to automatically update the IANA IPFIX registry" - is "automatically" correct? I didn't see any mention of this later in the draft. [Med] Please note that this text is preceded with “..does not:”. This is covered by this part: The definition of the ipv6ExtensionHeaders IE is updated in [I-D.ietf-opsawg-ipfix-fixes] to address some of the issues listed in Section 1.1. Where new EHs are automatically mirrored by IANA in the IPFIX registry. 1.2 "Support means to report the observed Experimental Identifiers (ExIDs) that are carried in shared TCP options (kind=253 or 254)" - it took me far too long to parse this correctly. Would "Allow reporting of the observed Experimental Identifiers ..." be clearer? [Med] Changed to « Allow reporting …». 2. Conventions and Definitions - s/Template/Template Record/ - Collector, Data Record, Flow Record, Exporting Process, Collecting Process are not used in the document, so they should be removed. [Med] Fixed. 3. "to address some of the issues listed in Section 1.1." - just some of the issues but not all of them? [Med] Yes, that text is about [I-D.ietf-opsawg-ipfix-fixes]. With this draft, all the issues are fixed as per: Because some of these limitations can't be addressed by simple updates to ipv6ExtensionHeaders, this section specifies a set of new IEs to address all the ipv6ExtensionHeaders IE limitations. 3.1 "while bit 254 corresponds to the most-significant bit of the IE." - 256 bits (2^8) are numbered 0 to 255 or 1 to 256, but never up to 254. [Med] You are right. Fixed. 3.1 The "No Next Header" (59) value is used if there is no upper-layer header in an IPv6 packet. Even if the value is not considered as an extension header as such, the corresponding bit is set in the ipv6ExtensionHeadersFull IE whenever that value is encountered in the Flow. - This goes against the previous paragraph, "In doing so, few octets will be needed to encode common IPv6 extension headers when observed in a Flow." because flows with no EH will require at least 8 octets to set bit 59. [Med] That was also my concern by Éric convinced me that having this allows for better observability. 3.1 "Several extension header chains may be observed in a Flow. These extension headers may be aggregated in one single ipv6ExtensionHeadersFull Information Element or be exported in separate ipv6ExtensionHeadersFull IEs, one for each extension header chain." - say whether or not the order of those IEs is important. eg, an intermediate IPFIX device might not preserve the order. [Med] The order is not important here. This is why the text is silent. If the ordering was important, we will face the IPFIX issue of IPFIX IE ordering anyway. 3.2. ipv6ExtensionHeaderCount Information Element - consider naming this in plural, "ipv6ExtensionHeadersCount", for consistency with the other IEs defined here. [Med] The count is per EH, not for all EHs. That’s why we had this one singular. 3.2 "and number of consecutive occurrences" - remove "consecutive"? [Med] Consecutive is important here because some EHs may appear in several positions in a chain. We don’t want to aggregate the counts. 3.2. "If several extension header chains are observed in a Flow, each header chain MUST be exported in a separate ipv6ExtensionHeaderCount IE." - say whether or not the order of those IEs is important. [Med] The text is silent about ordering because of the SHOULD in IPFIX spec. We can’t do much if order was important. 3.2 "the occurrences that are observed before the Fragment header and the occurrences right after the Fragment header." - singular, "the occurrence". [Med] Many occurrences can be observed. 3.2 (Figure) - please name/number the figure [Med] We are avoiding that here because this text will make it to the registry. and move the bit numbers rightwards one place, consistent with the figures in section 6. [Med] Done. - the count is limited to 8 bits, bu there was no previous mention of this. Say what to do when the count is exceeded. [Med] I’m afraid that we will be over specifying here if we add such text. Please see the typical limit set in draft-ietf-6man-eh-limits. 3.2 Data Type Semantics: - this is not an identifier. It seems be a new type consisting of (type, count) tuples. [Med] Will double check this one. 3.3 e.g., ipv6ExtensionHeaderFull - typo: it's ipv6ExtensionHeadersFull [Med] Fixed. 3.3. ipv6ExtensionHeadersLimit Information Element - Why use negative logic (ie, where "false" indicates a complete set of IPv6 headers). It would make more sense for this to be "true" when matching and "false" when not matching. [Med] OK 3.3 See [RFC8883] for an example of IPv6 packets processing - singular "packet processing" [Med] Fixed. 3.4. However, it was regularly reported - by who? Where? Cite references or it didn't happen! [Med] We already do in the additional info: rfc9098 3.4 "The ipv6ExtensionHeadersChainLength IE is used to report, in octets, the length of an extension header chain observed in a Flow. The length is the sum of the length of all extension headers of the chain." - say whether multiple IEs are to be exported, one per chain. If so, then say whether order is important. [Med] Thought this is covered by “ an extension header chain” part of the text, but OK to add an explicit statement. As for the order, no mention will be included. 4.1 Option number X is mapped to bit position "254 - X". - please, NO! Nobody's going to do that. Please use the same encoding as in 3.1. [Med] It is actually the same encoded. Reworded to have a similar description as in 3.1. - BTW TCP option numbers begin at 0 so it should be "255 - X". 4.2. tcpSharedOptionExID Information Element - From the description here, I did not understand how to encode the IE. [Med] The IE will include list of ExIDs, each encoded in 2-byte. 4.2 Expermients IDs ... (or 4-bute) - typos [Med] Fixed. 6.1 IPv6 Extension Headers - this section is actually about "ipv6ExtensionHeadersFull" 6.1 This section provides few examples to illustrate the use of some IEs defined in the document. - move this text to section 6. [Med] ACK. 6.1 Figure 1 - there should be 256 bits, numbered 0 to 255. - the numbering above the middle and right-most blocks is mis-aligned. Compare with Figure 2. [Med] Fixed. 6.1 Figure 2 - again there should be 256 bits, numbered 0 to 255. [Med] ACK. - draw figures 1 and 2 as figure 3, with only one section of ellipsis. [Med] OK - referring to section 9.1 of I-D.ietf-opsawg-ipfix-fixes] for the IPv6 Hop-by-Hop Options, Routing, and Destination Options headers bits: 1, HOP 0 Hop-by-hop option header 0, DST 60 Destination option header 5, RH 43 Routing header - so please list the headers in order! [Med] The EH are listed in the example as they appear in the flow. What is important then is that the appropriate bit is set. - this shows that the bits 0, 1, and 5 should be set - so the figure should show "1 0 0 0 1 1" rather than "0|1|0|0|1|1|". 6.2. TCP Options - Reword "Given TCP kind allocation practices" - "Concretely, the tcpOptionsFull IE will be set to 15." --> no, "1|1|0|1|" = 8 + 4 + 1 = 13. [Med] Good catch. 15 was in octal :-) 8 TBD3 ipv6ExtensionHeaderLimit - typo, it's "ipv6ExtensionHeadersLimit" [Med] Fixed. P. ____________________________________________________________________________________________________________ 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.
_______________________________________________ OPSAWG mailing list OPSAWG@ietf.org https://www.ietf.org/mailman/listinfo/opsawg
