Re: [OPSAWG] AD review of draft-ietf-opsawg-ipfix-tcpo-v6eh

[Mahesh Jethanandani <[e-mail]>]

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, [e-mail] 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 <[e-mail] <mailto:[e-mail]>> 
> Envoyé : dimanche 14 avril 2024 21:42
> À : [e-mail] <mailto:[e-mail]>
> Cc : [e-mail] <mailto:[e-mail]>; [e-mail] <mailto:[e-mail]>
> 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
> [e-mail] <mailto:[e-mail]>
>  
>  
>  
>  
> 
>  
> ____________________________________________________________________________________________________________
> 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
[e-mail]