Hi Benoit, authors, Thanks for the updates - they look good to me. I've requested IETF last call on -08.
Regards, Rob -----Original Message----- From: Benoit Claise <benoit.cla...@huawei.com> Sent: 27 March 2023 01:45 To: Rob Wilton (rwilton) <rwil...@cisco.com>; draft-ietf-opsawg-ipfix-srv6-srh....@ietf.org; opsawg@ietf.org Subject: Re: AD review for draft-ietf-opsawg-ipfix-srv6-srh-07 Hi Rob, Thanks for your review. We posted a new draft version. Htmlized:https://datatracker.ietf.org/doc/html/draft-ietf-opsawg-ipfix-srv6-srh Diff:https://author-tools.ietf.org/iddiff?url2=draft-ietf-opsawg-ipfix-srv6-srh-08 See inline for some replies. Regards, Benoit on behalf of the authors. On 3/22/2023 6:55 PM, Rob Wilton (rwilton) wrote: > Hi, > > Thanks for this document. Here are my AD review comments for > draft-ietf-opsawg-ipfix-srv6-srh-07. There is an obvious disclaimer here > that I'm not an IPFIX expert, and I appreciate that one of the authors is ;-). > > > Moderate level comments: > > > > > (1) p 4, sec 3. IPFIX Information Elements > > > > > srhIPv6Section > Exposes the SRH and its TLVs as specified in Section 2 of > [RFC8754] as series of n octets in IPFIX. > > > > > It isn't clear to me exactly what this is? Does this cover the whole SRH, or > just the "SRH TLVs" section? From looking at the examples, I think that this > exposes the whole SRH, in which case, I think that the description could be > clearer. Right, we added a reference RFC 8754 section 2.1, next to 2. > Please can you also update the description in the IANA section. > > > > > > > > > (2) p 12, sec 5.11. srhSegmentIPv6EndpointBehavior > > ElementID: TBD11 > Description: The 16-bit SRv6 Endpoint behavior. Assigned values and > their meanings are provided in the "SRV6 Endpoint Behavior" IANA > registry [IANA-IPFIX]. > > Is this linking to the right reference? It looks like this should be the > SRv6 IANA registry rather than the IPFIX registry. Good catch. You are right. To be consistent with https://datatracker.ietf.org/doc/draft-boucla-opsawg-ipfix-fixes/, we changed it to. srhSegmentIPv6EndpointBehavior ElementID: TBD11 Description: The 16-bit SRv6 Endpoint behavior. Assigned values and their meanings are provided in the "SRV6 Endpoint Behavior" registry. Abstract Data Type: unsigned16 Data Type Semantics: identifier Additional Information: See the "SRV6 Endpoint Behavior" registry at https://www.iana.org/assignments/segment-routing/segment-routing.xhtml#srv6-endpoint-behaviors. Section 4 of RFC8986. > Minor level comments: > > (3) p 4, sec 3. IPFIX Information Elements > > srhSegmentIPv6BasicList > Ordered basicList [RFC6313] of zero or more 128-bit IPv6 addresses > in the SRH that represents the SRv6 segment list. As specified in > Section 2 of [RFC8754], the Segment List is encoded starting from > the last segment of the SR Policy. That is, the first element of > the Segment List (Segment List[0]) contains the last segment of > the SR Policy, the second element contains the penultimate segment > of the SR Policy, and so on. > > In terms of naming, I note that the "IPv6" seems to turn up in difference > places in the IEs defined in this draft. It might be awkward to change, but > wouldn't the names be more consistent (e.g., with RFC8754), if they put IPv6 > all in the same place? I was going to suggest putting them at the start > (e.g., ipv6srh...), but looking > athttps://www.iana.org/assignments/ipfix/ipfix.xhtml, it looks like the > "IPv6" is often put at the end of the name. > > E.g., srhSegmentIPv6BasicList -> srhSegmentBasicListIPv6, etc. > > Note, I'm happy to defer to Benoit's judgement as an IPFIX expert here ... We discussed that point with Paul Aitken from the IPFIX IE doctors, (ie-doctor mailing list). To be consistent with the IPFIX registry, we used IPv6 as an adjective. As an example, IPv6 refers to the srhSegment, hence srhSegmentIPv6 > (4) p 4, sec 3. IPFIX Information Elements > > srhSegmentIPv6LocatorLength > The number of significant bits. Together with srhSegmentIPv6 it > enables the calculation of the SRv6 Locator. > > I suggest "The number of significant bits" is reworded to something like: > "The IPv6 location length specified as the number of significant bits." > > If you update this, then please can you make a similar clarification for the > IANA entry description. The SRH segment IPv6 locator length specified as the number of significant bits. Together with srhSegmentIPv6 it enables the calculation of the SRv6 Locator. > (5) p 9, sec 5.6. srhSegmentIPv6ListSection > > Description: The SRH Segment List as defined in section 2 of > [RFC8754] as series of n octets. > > Am I right to assume that the length of the array must be a multiple of 16? Right. > Is that worth specifying at all? Isn't it sufficient/better to point to the specification RFC 8754? > (6) p 26, sec Appendix A. IPFIX Encoding Examples > > Figure 8: Data Set Encoding Format for SRH Section > > Note, I don't really know IPFIX well enough to validate the examples, hence > I'm trusting the authors that they are correct. Andrew Feren, as IPFIX doctor reviewed the examples (and admittedly found a mistake in A1.1, which was corrected in a previous version. Thanks Andrew. Other examples were implemented, so we should be good. > Nit level comments: > > (7) p 2, sec 1. Introduction > > These IEs are used to export SRv6 active segment and its control > plane protocol, the SRv6 segment list, the next SRv6 node and its > type, and then numbers of SRv6 segments left. > > "then numbers" -> "the numbers"? > > > (8) p 4, sec 3. IPFIX Information Elements > > srhSegmentIPv6ListSection > Exposes the SRH Segment List as defined in Section 2 of [RFC8754] > as series of n octets in IPFIX. > > I suggest "series of n octets" -> "sesies of octets", here and below. > > > (9) p 12, sec 6.1. SRv6 Segment List > > The srhSegmentIPv6BasicList encodes the SRv6 segment list with a > basicList, specified in the IPFIX Structured Data [RFC6313]. This > encoding offers the advantage to the data collection that the > different IPv6 addresses are already structured as a list, without > the need of post processing. However, this method requires some > extra processing on the exporter, to realize the BasicList data > mapping. > > Should it be "to the data collector" or is "to the data collection" correct? Actually, the data collection seems more appropriate to the authors, as it encompasses the data collector itself, but also the data streaming/ETL, up to the data analytics. So the data collector is only one piece of the data collection. > > (10) p 12, sec 6.1. SRv6 Segment List > > The srhSegmentIPv6ListSection, on the other hand, encodes the list of > IPv6 addresses as an octetArray. This doesn't impose any data flow > manipulation on the exporter, facilitating the immediate export. > However, the data collection MUST be able to decode the IPv6 > addresses according the SR specifications. Compared to the > srhSegmentIPv6BasicList, the srhSegmentIPv6ListSection flow records > length is slightly reduced. > > 'according the' -> 'according to the' > > Regards, > Rob > _______________________________________________ OPSAWG mailing list OPSAWG@ietf.org https://www.ietf.org/mailman/listinfo/opsawg
