Reviewer: Bruno Decraene Review result: Has Nits Hello, I have been selected as the Routing Directorate reviewer for this draft. The Routing Directorate seeks to review all routing or routing-related drafts as they pass through IETF last call and IESG review, and sometimes on special request. The purpose of the review is to provide assistance to the Routing ADs. For more information about the Routing Directorate, please see http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir Although these comments are primarily for the use of the Routing ADs, it would be helpful if you could consider them along with any other IETF Last Call comments that you receive, and strive to resolve them through discussion or by updating the draft. Document: draft-ietf-idr-bgp-prefix-sid-21 Reviewer: Bruno Decraene Review Date: 2018-06-13 IETF LC End Date: 2018-06-12 Intended Status: Standard Track ========================== Summary: I have some minor concerns about this document that I think should be resolved before publication. ========================== Comments: Document is clear. Altough sometimes it's visible that the document has been significantly edited over its history and could probably be made clearer with a full rewrite from a single editor. ========================== Major Issues: § IANA consideration "This document also requests creation of the "BGP Prefix-SID Label- Index TLV Flags" registry under the "Border Gateway Protocol (BGP) Parameters" registry, Reference: draft-ietf-idr-bgp-prefix-sid. Initially, this 16 bit flags registry will be empty. Flag bits will be allocated First Come First Served (FCFS) consistent with the BGP- SID TLV Types registry." IMHO a registry of only 16 possible entries seems very small for a FCFS policy. Anyone would be able to deplete it in minutes. (cf RFC 8126 "It is also important to understand that First Come First Served really has no filtering."). Is this really the intention of the WG? (Actually I'm wondering what would be the monetary value of such a flag on the black market... If zero, this means that the flag are useless. If non-zero, the benefit may be worth the trouble) Same comment for the "BGP Prefix-SID Originator SRGB TLV Flags" registry. ========================== Minor Issues: I think that the Abstract should mention that this specification is dedicated to SR-MPLS (as opposed to covering both SR-MPLS & SRv6, which was the case at the beginning of its history) ---------------------- §1 "A BGP-Prefix Segment (and its BGP Prefix-SID) is a BGP segment attached to a BGP prefix." I'd propose the following change A BGP-Prefix Segment is a BGP labeled prefix with a Prefix-SID attached. Rationals: 1) I'd rather say that this is the SID which is attached to the prefix, rather than the Segment. 2) Given that this document is dedicated to SR-MPLS, we can probably say that the BGP prefix is labeled. But up to the authors. ----- §1 "within the SR/BGP domain" Can you point to the definition of "BGP domain"? If not, "SR domain" is probably enough. --- The BGP Prefix-SID attribute defined in this document can be attached to prefixes from Multiprotocol BGP labeled IPv4/IPv6 Unicast ([RFC4760], [RFC8277]). Address Family Identifier (AFI)/ Subsequent Address Family Identifier (SAFI) combinations. 1) The full point "." in the middle of the sentence is probably not intended. 2) I don't find the following text crystal clear: "BGP labeled IPv4/IPv6 Unicast ([RFC4760], [RFC8277]). Address Family Identifier (AFI)/ Subsequent Address Family Identifier (SAFI) combinations." IMO adding the numerical values would help (my undertanding is AFI/SAFI 1/4 and 2/4). Alternatively using the names as per the IANA registries. Also using the AFI/SAFI order in "Address Family Identifier (AFI)/ Subsequent Address Family Identifier (SAFI) " while the SAFI/AFI order in "BGP labeled IPv4/IPv6 Unicast" is misleading. Proposed NEW: The BGP Prefix-SID attribute defined in this document may be attached to BGP IPv4/IPv6 labelled routes. (AFI/SAFI 1/4 or 2/4). ----- "o A BGP Prefix-SID MAY be global between domains when the interconnected domains agree on the SID allocation scheme." I think you mean :s/global/domain wide (as opposed to the term "Global Segment" which has a different meaning.) BTW in "A BGP Prefix-SID is always a global SID ([I-D.ietf-spring-segment-routing])" I guess you mean :s/global SID/global segment (for the same reason) BTW2 "An SR domain is defined as a single administrative domain for global SID assignment." Probably :s/global SID/domain wide SID BTW3 "An SR domain is defined as a single administrative domain for global SID assignment." may be :s/global SID assignment/domain wide SID assignement" or :s/global SID/SID ----- OLD o A BGP Prefix-SID MAY be attached to a prefix. In addition, each prefix will likely have a different AS_PATH attribute. This implies that each prefix is advertised individually, reducing the ability to pack BGP advertisements (when sharing common attributes). Proposed NEW: o A BGP Prefix-SID MAY be attached to a prefix. This implies that each prefix is advertised in individually, reducing the ability to pack BGP advertisements (when sharing common attributes). (second sentence is not relevant to this specification and seems to be specific to the MSDC use case) --- "The BGP Prefix-SID advertised for BGP prefix P indicates that the segment routed path should be used" may be :s/segment routed path/SR segment ("segment routed path" is not well defined to me. Usually, the terms used are "segment routing" or "soure routed path")) ---- "2.1. MPLS BGP Prefix SID" "4.1. MPLS Dataplane: Labeled Unicast" There is no need anymore for this title/hierarchy (given that SR-MPLS BGP prefix SID are the only ones defined in this document (i.e. SRv6 removed) ---- "The operator assigns a globally unique label index," The SR architecture and SR-MPLS documents use either a "label" or a "index". Here it's an index so I'd propose to use a consistent terminology, hence :s/label index/index (Multiple times in the document.) ----- "The operator assigns a globally unique label index, L_I, to a locally sourced prefix of a BGP speaker N which is advertised to all other BGP speakers in the SR domain." I thnk that BGP use the term "locally originated" (or simply "originated") rather than "locally sourced prefix" ------ "If the BGP speakers are not all configured with the same SRGB, and if traffic-engineering within the SR domain is required, each node may be required to advertise its local SRGB in addition to the topological information." - What do you mean by "traffic engineering"? I think the issue comes from stacking the BGP SID below other SID(s)/labels rather than related to "traffic-engineering". - I think that "may be required to advertise its local SRGB" is an understatement. (otherwise, please explain how the label is computed) - "If the BGP speakers are not all configured with the same SRGB" how is this information supposed to be known by the source node? Guessing seems dangerous. - If we need the SRGB of the originator, we probably also need the real SID of the originator. In which case, in the following sentence I think we need :s/SHOULD/MUST "If multiple valid paths for the same prefix are received from multiple BGP speakers or, in the case of [RFC7911], from the same BGP speaker, and the BGP Prefix-SID attributes do not contain the same label index, then the label index from the best path BGP Prefix-SID attribute SHOULD be chosen "" --- "As defined in [I-D.ietf-spring-segment-routing], the label index L_I is an offset into the SRGB. Each BGP speaker derives its local MPLS label, L, by adding L_I to the start value of its own SRGB, and programs L in its MPLS dataplane as its incoming/local label for the prefix." Actually the way to compute the MPLS labels from the index and SRGB is defined in https://tools.ietf.org/html/draft-ietf-spring-segment-routing-mpls Plus I'd rather use a pointer to this SR-MPLS specification rather than re-specify the computation. (especially since the text/algo is different, because the text from draft-ietf-idr-bgp-prefix-sid simply assumes that the SRGB is composed of a single block, which is not general enough for a specification. --- " Section 3.1 of this document specifies the Label-Index TLV of the BGP Prefix-SID attribute; this TLV can be used to advertise the label index for a given prefix. In order to advertise the label index of a given prefix P and, optionally, the SRGB, an extension to BGP is needed: the BGP Prefix- SID attribute. This extension is described in subsequent sections." Could probably be rephrased to avoid redundancy. --- " The Label-Index and Originator SRGB TLVs are used only when SR is applied to the MPLS dataplane." Given that the scope of this document is now only MPLS-SR, this sentence may probably be removed. ----- "SRGB: 3 octets of base followed by 3 octets of range." The term "base" is not defined neither in SR-architecture, SR-ISIS, SR-MPLS. Actually SR-ISIS and SR-MPLS use different terms... and both documents are under WGLC... SR-MPLS: "the SRGB is specified by the list of MPLS Label ranges [Ll(1),Lh(1)], [Ll(2),Lh(2)],..., [Ll(k),Lh(k)] where Ll(i) =< Lh(i)." SR-ISIS: One or more SRGB Descriptor entries, each of which have the following format: Range: 3 octets. SID/Label sub-TLV (as defined in Section 2.3). ----- "The Originator SRGB TLV contains the SRGB of the node originating the prefix to which the BGP Prefix-SID is attached. The Originator SRGB TLV MUST NOT be changed during the propagation of the BGP update. The originator SRGB describes the SRGB of the node where the BGP Prefix SID is attached. " Some redundancy (1rst and last sentence) could be removed. ----- " The receiving routers concatenate the ranges and build the Segment Routing Global Block (SRGB) as follows: SRGB = [100, 199] [1000, 1099] [500, 599] The indexes span multiple ranges: index=0 means label 100 ... index 99 means label 199 index 100 means label 1000 index 199 means label 1099 ... index 200 means label 500 ..." I'm not sure that there is a need to respecify the way labels are computed from SRGB and index. A reference to SR-MPLS would probably be better. ------ OLD: Consistent with [RFC7606], only the first occurrence of the BGP Prefix-SID attribute will be considered and subsequent occurrences will be discarded. Similarly, only the first occurrence of a BGP Prefix-SID attribute TLV of a given TLV type will be considered unless the specification of that TLV type allows for multiple occurrences. Proposed NEW (even more consistent with RFC 7606) As per with [RFC7606], if the BGP Prefix-SID attribute appears more than once in an UPDATE message, then all the occurrences of the attribute other than the first one SHALL be discarded and the UPDATE message will continue to be processed. Similarly, if a recognized TLV appears more than once in an BGP Prefix-SID attribute while the specification only allows for a single occurence , then all the occurrences of the TLV other than the first one SHALL be discarded and the Prefix-SID attribute will continue to be processed. That being said, the first sentence does not define any new behavior, so is not really needed IMHO. ----- "4.1. MPLS Dataplane: Labeled Unicast A BGP session supporting the Multiprotocol BGP labeled IPv4 or IPv6 Unicast ([RFC8277]) AFI/SAFI is required. The BGP Prefix-SID attribute MUST contain the Label-Index TLV and MAY contain the Originator SRGB TLV. A BGP Prefix-SID attribute received without a Label-Index TLV MUST be considered as "invalid" by the receiving speaker." Stricto census, the sentence prohibits the use of the BGP Prefix-SID for other usage than carrying Label-Index, including for non labeled AFI/SAFI. I think you rather mean the following proposed NEW: When the BGP Prefix-SID attribute is attached to a BGP labeled IPv4 or IPv6 Unicast ([RFC8277]) AFI/SAFI, it MUST contain the Label-Index TLV and MAY contain the Originator SRGB TLV. A BGP Prefix-SID attribute received without a Label-Index TLV MUST be considered as "invalid" by the receiving speaker. ---- OLD: The label index provides the receiving BGP speaker with guidance as to the incoming label that SHOULD be assigned by that BGP speaker. The sentence could probably be simplified. e.g. NEW The label index provides guidance, to the receiving BGP speaker, for the choice of its incoming label. --- " If multiple valid paths for the same prefix are received from multiple BGP speakers or, in the case of [RFC7911], from the same BGP speaker, and the BGP Prefix-SID attributes do not contain the same label index, then the label index from the best path BGP Prefix-SID attribute SHOULD be chosen with a notable exception being when [RFC5004] is being used to dampen route changes." It's not clear to me why the situation is different with RFC5004, nor what should be done in this case. Could you please elaborate? --- "It should be noted that while SRGBs and SIDs are advertised using 32-bit values, the derived label is advertised in the 20 right-most bits." The derived labeled is not advertised, so I don't understand the goal/meaning of the sentence. ----- § Security considerations "To prevent a Denial-of-Service (DoS) or Distributed-Denial-of-Service (DDoS) attack due to excessive BGP updates with an invalid or conflicting BGP Prefix-SID attribute, message rate-limiting as well as suppression of duplicate messages SHOULD be deployed." What do you mean exactly by "message rate-limiting"? Especially what do you mean by "message" and what is "rate-limited" exactly? The local use of the Prefix SID attribute? It's propagation? In which case, should be propagation delayed or never propagated?
