I have been selected to do a routing directorate "early" review of this draft. https://datatracker.ietf.org/doc/draft-ietf-idr-bgpls-srv6-ext The routing directorate will, on request from the working group chair, perform an "early" review of a draft before it is submitted for publication to the IESG. The early review can be performed at any time during the draft's lifetime as a working group document. In this case, it appears that the review request was triggered by the request to the AD for publication. The purpose of this early review is to ensure as wide of a review as possible. For more information about the Routing Directorate, please see http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir Document: draft-ietf-idr-bgpls-srv6-ext-07.txt Reviewer: Adrian Farrel Review Date: 29-May-2021 Intended Status: Standards Track == Summary == Some minor issues and lot of nits. == Comments == I am one of the Designated Experts for the BGP-LS registries and I had previously considered the draft when an Early Allocation request was made. That request was approved. The document is clear enough and I believe I could implement it if I had to. The approach described in this document is consistent with the body of BGP-LS work, but I think it is important to note that the mechanism set out here differs slightly from the mechanism defined to do similar function with SR-MPLS. This distinction is clearly made in Appendix A. However, the WG chairs and AD will want to be sure that the WG recognise this difference and are OK with having two slightly different mechanisms running side by side. There are some Minor Issues that I think should be addressed before IETF Last Call in case they create any points that community review might object to. There are also quite a lot of nits in the document. I have tried to capture them below, but the large number means I may have missed some. I recommend fixing these before IETF Last Call so that other reviewers has a cleaner document to look at. == Minor Issues == You will need to reduce the number of front-page authors to five or fewer, or you will need to provide the document shepherd with a credible reason to diverge from this rule. --- Section 1 On the similar lines, introducing the SRv6 related information in BGP-LS allows consumer applications that require topological visibility to also receive the SRv6 SIDs from nodes across a domain or even across Autonomous Systems (AS), as required. I caution you to be *very* careful with the word "domain". The IESG has recently been concerned by the definition contained in 8402. In that document, the "SR domain" (also just "domain") is the collection of all interconnected SR-capable nodes. Here (and in other parts of the document) I think you are using "domain" in a more restricted sense. I don't know how you fix that, but I believe you should do something. Perhaps you can call out the terminology issues here by noting the 8402 definition and specifically defining what you mean by your local definition of "domain". --- Section 2 The SRv6 SIDs associated with the node are advertised using the BGP- LS SRv6 SID NLRI introduced in this document. This enables the BGP- LS encoding to scale to cover a potentially large set of SRv6 SIDs instantiated on a node with the granularity of individual SIDs and without affecting the size and scalability of the BGP-LS updates. The claims of scalability are not expanded here or anywhere else in the document. Scalability of BGP-LS is important, so I'd prefer some explanation. But if that isn't available, it might be better to leave out these mentions. --- 4.1, 4.2, and 7.2 You should consider asking IANA to create a registry for the flags fields. This would make it easier to keep track of future assignments. You should also state clearly that the three flags fields are aligned so that an addition to one is always also made to the other (if this is the case). --- 4.1 and 4.2 Sub-TLVs : Used to advertise sub-TLVs that provide additional attributes for the specific SRv6 SID. You need to say where these sub-TLVs are defined. If there are none defined yet, you should say so and explain why some might be defined in the future. You should also say where the sub-TLV type values are tracked. Compare with 5.1 --- 5.1 You should say where the sub-TLVs are tracked, and you should consider a registry for the flags. --- 7.1 The SRv6 Endpoint Behavior TLV is a mandatory TLV that MUST be included in the BGP-LS Attribute associated with the BGP-LS SRv6 SID NLRI. What if it is missing? --- 8. The total of the locator block, locator node, function and argument lengths MUST be less than or equal to 128. What if they add up to more than 128? --- Well done for including Appendix A. I assume that the WG discussed the relative merits of having a slightly different approach for SR-MPLS and SRv6, and contrasted that with the benefits of a consistent approach. I personally think that making a difference is unfortunate, but if the WG was happy with this decision then I guess it's OK. Without launching into a sales pitch, it might be nice if Appendix A was to very briefly explain why the difference. == Nits == You have: - "SR for [the] MPLS data-plane" (Abstract) - SR/MPLS (Section 4.1) - "SR with [the] MPLS data-plane" (Section 7.2) - "SR-MPLS" (Sections 10 and 11, Appendix A) 8402 has "SR-MPLS" --- Abstract s/Segment Routing (SR) over IPv6 (SRv6)/Segment Routing over IPv6 (SRv6)/ s/by the various/by various/ OLD BGP Link-state (BGP-LS) address-family solution for SRv6 is similar to BGP-LS for SR for MPLS data-plane. This draft defines extensions to the BGP-LS to advertise SRv6 Segments along with their behaviors and other attributes via BGP. NEW This document defines extensions to BGP Link-state (BGP-LS) to advertise SRv6 segments along with their behaviors and other attributes via BGP. The BGP-LS address-family solution for SRv6 described in this document is similar to BGP-LS for SR for the MPLS data-plane defined in a separate document. END --- Section 1 s/A SRv6 Segment/An SRv6 segment/ OLD The IS-IS [I-D.ietf-lsr-isis-srv6-extensions] and OSPFv3 [I-D.ietf-lsr-ospfv3-srv6-extensions] link-state routing protocols have been extended to advertise some of these SRv6 SIDs and SRv6-related information. NEW The IS-IS and OSPFv3 link-state routing protocols have been extended to advertise some of these SRv6 SIDs and SRv6-related information [I-D.ietf-lsr-isis-srv6-extensions], [I-D.ietf-lsr-ospfv3-srv6-extensions]. END s/Certain other/Other/ s/On the similar lines/On similar lines/ s/(e.g./(e.g.,/ --- Section 2 o SRv6 SID of the IGP Adjacency SID or the BGP EPE Peer Adjacency SID [RFC8402] is advertised via SRv6 End.X SID TLV introduced in this document (Section 4.1) o SRv6 SID of the IGP Adjacency SID to a non-Designated Router (DR) or non-Designated Intermediate-System (DIS) [RFC8402] is advertised via SRv6 LAN End.X SID TLV introduced in this document (Section 4.2) 1. s/SRv6 SID/The SRv6 SID/ twice 2. It is confusing that the first bullet talks about advertising the IGP Adjacency with no qualification, but the second bullet talks about the IGP Adjacnecy to a non-DR or non-DIS. That appears to make the first bullet a superset of the second bullet. Sections 4.1 and 4.2 make this a lot clearer: perhaps you could bring some of that clarity to these bullets. --- Section 2 o SRv6 Locator is advertised via SRv6 Locator TLV introduced in this document (Section 5.1) s/SRv6/The SRv6/ s/SRv6/the SRv6/ --- 2. The SRv6 SIDs associated with the node are advertised using the BGP- LS SRv6 SID NLRI introduced in this document. Please include a forward pointer to Section 6. --- 2. o The endpoint behavior of the SRv6 SID is advertised via SRv6 Endpoint Behavior TLV (Section 7.1) s/via SRv6/via the SRv6/ --- 2. o The BGP EPE Peer Node and Peer Set context for a Peer Node and Peer Set SID [RFC8402] respectively is advertised via SRv6 BGP EPE Peer Node SID TLV (Section 7.2) s/via SRv6/via the SRv6/ The text is hard to read. Maybe... o The BGP EPE Peer Node context for a Peer Node, and the Peer Set context for a Peer Set SID [RFC8402] are advertised via the SRv6 BGP EPE Peer Node SID TLV (Section 7.2). --- 2. s/(e.g./(e.g.,/ --- 2. When the BGP-LS router is advertising topology information that it sources from the underlying link-state routing protocol (as described in [RFC7752]), then it maps the corresponding SRv6 information from the SRv6 extensions for IS-IS [I-D.ietf-lsr-isis-srv6-extensions] and OSPFv3 [I-D.ietf-lsr-ospfv3-srv6-extensions] protocols to their BGP- LS TLVs/sub-TLVs for all SRv6 capable nodes in that routing protocol domain. When the BGP-LS router is advertising topology information from the BGP routing protocol (e.g. for EPE as described in [I-D.ietf-idr-bgpls-segment-routing-epe]), then it advertises the SRv6 information from the local node alone. I'd suggest moving this paragraph to be the second in the section as it explains what is going on. --- 3.1 and This TLV maps to the SRv6 Capabilities sub-TLV and the SRv6 Capabilities TLV of the IS-IS and OSPFv3 protocol SRv6 extensions respectively. : : o Flags: 2 octet field. The following flags are defined: 0 1 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | |O| Reserved | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ Figure 2: SRv6 Capability TLV Flags Format * O-flag: If set, then router is capable of supporting SRH O-bit Flags, as specified in [I-D.ietf-6man-spring-srv6-oam]. * The rest of the bits are reserved for future use and MUST be set to 0 and ignored on receipt. Would be good to include references for the two capabilities TLVs. It would also help to not include the bit definitions here. I think I am correct that if a new bit is defined for inclusion in the ISIS SRv6 Capabilities sub-TLV flags field, you expect to pick it up here automatically. It looks like the mapping is in the wrong direction. I.e., this TLV maps from, not to, the IGP TLVs. I think you would say... This TLV maps from the SRv6 Capabilities sub-TLV of the IS-IS SRv6 extensions [ref] and the SRv6 Capabilities TLV of the OSPFv3 SRv6 extensions [ref]. : : o Flags: 2 octet field. The contents of this field are copied from the IS-IS or OSPF SRv6 Capabilities TLVs and have the same meaning. For what it's worth, it's a pitty that there is no registry for tracking the flags, but that's a problem with draft-ietf-lsr-isis-srv6-extensions and not with this document. --- 4.1 s/End.X with USP and/End.X with USP, and/ (twice) s/This TLV is also used by BGP/This TLV is also used by BGP-LS/ --- 4.1 Figures 3 and 5 Convetion is that if you show all of the bytes, you close the start and end of the lines with a vertical pipe "|". Or you can leave out bytes and close the line ends with a tilda "~" or double slash "//" (see Figure 9). --- 4.1, 4.2, 5.1, and 6.1 Can you say what the Length field is. "The length of the value part of the TLV in bytes including any sub-TLVs that may be present." ?? --- 4.1, 4.2, 5.1, and 7.1 Algorithm: 1 octet field. Algorithm associated with the SID. Algorithm values are defined in the IANA IGP Algorithm Type registry. It would be good to add a reference to RFC 8665 or to https://www.iana.org/assignments/igp-parameters/igp-parameters.xhtml. --- 4.2 s/normally an IGP node only/an IGP node normally only/ s/to announce SRv6 SID/to announce the SRv6 SID/ s/(i.e./(i.e.,/ s/End.X with USP and/End.X with USP, and/ (twice) --- 4.2 The IS-IS and OSPFv3 SRv6 LAN End.X SID TLVs have the following format: Do you mean the BGP-LS TLVs? --- 5.1 OLD As specified in [RFC8986], an SRv6 SID comprises of Locator, Function and Argument parts. NEW As specified in [RFC8986], an SRv6 SID comprises Locator, Function, and Argument parts. END --- 6. Figure 9 There is a broken format line in the middle of the figure. --- 6. The figure has | Identifier | | (64 bits) | and the text has o Identifier: 8 octet value as defined in [RFC7752]. Be consistent? --- 6. o Protocol-ID: 1 octet field that specifies the protocol component through which BGP-LS learns the SRv6 SIDs of the node. The Protocol-ID registry was created by [RFC7752] and then extended by other BGP-LS extensions. - s/Protocol-ID registry/BGP-LS Protocol-ID registry/ - Probably, "...and then additional assignments were made for other BGP-LS extensions." --- 7.1 s/are bound with a SID/are bound to a SID,/ --- 7.2 s/The similar/Similar/ --- 7.2 is an optional TLV for use in the BGP-LS Attribute for an SRv6 SID Is that "OPIONAL"? But then... This TLV MUST be included along with SRv6 SIDs that are associated with the BGP Peer Node or Peer Set functionality. ... is confusing. I suspect that the second sentence should be... If this TLV is present, it MUST be accompanied by the SRv6 SIDs that are associated with the BGP Peer Node or Peer Set functionality. --- 7.2 s/For a SRv6/For an SRv6/ --- 8. s/SRv6 SID Structure TLV is used/The SRv6 SID Structure TLV is used/ --- 8. optional TLV Is that "OPTIONAL"? --- 9. OLD This document requests assigning code-points from the IANA "Border Gateway Protocol - Link State (BGP-LS) Parameters" registry as described in the sub-sections below. NEW IANA has made early allocations of code points from the "Border Gateway Protocol - Link State (BGP-LS) Parameters" registry as described in the sub-sections below. IANA is requested to updated the references to indicate this document when it is published as an RFC. END --- 9.2 It would make sense to list the codepoints in numeric order. I.e., lift 518 to the top. --- 10. s/(e.g./(e.g.,/ --- 11. s/(e.g./(e.g.,/ --- Appendix A s/session(s)/session/ s/advertised as attribute/advertised as attributes/