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
