Hello I have been selected to do a routing directorate "early" review of this draft. https://datatracker.ietf.org/doc/draft-ietf-lsvr-bgp-spf/ 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. The purpose of the early review depends on the stage that the document has reached. This document appears to have completed WG last call, been passed to the AD for publication, received several Directorate reviews (at around revision -28), and had a review by the AD. It seems to have been returned to the WG for further work. Thus, this review comes before the document is returned to the AD for publication. In that respect, the review is for consideration by the authors and working group rather than for attention of the AD. For more information about the Routing Directorate, please see https://wiki.ietf.org/en/group/rtg/RtgDir Document: draft-ietf-lsvr-bgp-spf-31.txt Reviewer: Adrian Farrel Review Date: date Intended Status: Standards Track Summary: I have concerns about this document. I think it needs more work before being submitted to the IESG. Comments: This draft is well written from a technical perspective. I believe it could be implemented with a high probability of interoperability modulo the issues noted below. As noted in the comments below, some attention to the explanation in the Introduction and the overview of the function of the protocol might be helpful, and a little work on the English usage could be beneficial. The document describes the use of BGP as an interior gateway protocol for SPF calculations in a network environment that is assumed to be highly stable and which might have a large degree of ECMP. The approach builds on th e information elements developed for BGP-LS, but is not a re-use of BGP-LS as a new SAFI is used. I have a number of comments which I believe should be discussed before this document progresses to publication. = Major While BGP-LS was originally invented as an East-West protocol, that idea was discarded because of severe scaling and stability concerns and RFC 7752 was shaped as a Northbound protocol allowing a BGP speaker in the network to report to a controller, management station of route reflector (and subject to filters, aggregation, and policy) the full link-state database of the network. This work obviously decides to change that approach and to use BGP-LS with additional information to distribute the full set of routing information between all nodes in the network. I think that this document needs to more clearly set out this change in philosophy, and (more importantly) discuss the assumptions of network scale and stability that are necessary for this protocol to be used. While a data centre environment may be adequately stable, this definition of a protocol usage would be very unwise in a WAN. All you have in this respect is a statement at the top of section 4 that implies that there are choices based on the network scale, and an observation in the Introduction that the solution would be beneficial if the topology is very stable. The minimum needed for this is an upfront statement along the lines of The protocol described in this document is intended for deployments where the network topology is very stable so that topology update advertisements will be rare. It's use in other networks, and in particular in the Internet, is NOT RECOMMENDED because the protocol might not be stable or scale well under high load caused by rapid or frequent changes in the topology. (Your choice of words may vary.) It might even help to not call this BGP-LS-SPF but BGP-DC-SPF or something similar. Since it is a new SAFI, there is no need to persist with the BGP-LS name and that might help make it clear what the intended or presumed deployment scenarios are. Of course, the wrinkle comes in 5.2.2.1 where you want to define a new TLV that only has use in BGP SPF, but you want to assign it from the BGP-LS registry. Have you thought of looking for a way to keep the SPF information separate? Or would you expect a regular BGP-LS speaker to also share this new TLV if it knows the information? = Medium I find the reference to draft-psarkar-lsvr-bgp-spf-impl unsatisfactory. Not only did that draft expire a good while ago, but this draft has been updated six times since that implementation statement was posted. Have there been no changes to the technical content in all those revisions? The implementation status covers a bit more than 1.5 implementations. It's good information, but is it enough for changes to BGP? --- The text at the top of section 4 says that there are deployment choices based on the network scale, but gives no advice (or pointers to advice) on how to select between deployment options, how to ensure that the same option is used by all nodes in the network, and what to do if two peers discover that they are using different options. --- In the RR mode of section 4.3, it is not clear what information the RR readvertises and to which nodes. --- 8.2 names a registry that does not exist (with the given name). Please don't leave IANA guessing. --- 5.2.1.1, 5.2.2.2, and 5.2.3.1 describe "SPF Status" fields of the various TLVs. It might help if the fields had different names to help not confuse anything (e.g., Node SPF Status). Adding in the Address Family field from 5.2.2.1, it is unclear whether you intend the "undefined" settings to be available for use in future specifications. - If you do, then you probably need to create registries, and while you describe how to handle the receipt of undefined statuses, you are missing a description of how to handle undefined address families. (Note, however, that in describing the handling of undefined status you say they SHOULD be readvertised. This means that there can be holes and discrepencies in the databases that different nodes see if new bits are defined in the future. So you probably need to make this MUST.) - If you don't then it is interesting to ask why you need to specify a difference between "reserved" and "undefined", and why you need more than a single bit flag (in fact, in the case of 5.2.2.2 and 5.2.3.1 it would appear that the very presence of the TLV is enough and no status field is needed). --- 5.2.2.1 has the Address Family (AF) Link Descriptor SHOULD be included with the Link Local/Remote Identifiers TLV so that the link can be used in the respective address family SPF. If the Address Family Link Descriptor is not present for an unnumbered link, the link will not be used in the SPF computation for either address family. The use of SHOULD means that an implementation MAY leave it out. Now, in the case of an unnumbered link, it is clear that that is fine, but in the case of a numbered link, leaving the link descriptor out would seem to make the link advertisement a bit pointless. So why might an implementation make that choice? --- In 5.2.2.2 If a BGP SPF speaker received the Link NLRI but the SPF Status TLV is not received, then any previously received information is considered as implicitly withdrawn and the update is propagated to other BGP SPF speakers. "is not received" in what time scale? Ditto 5.2.3.1 --- 9. Given that this protocol is targetted at stable environments, one might consider that making a part of the network fragile could destabilise the whole. So there is a physical attack of causing a link to flap that could result in many updates being flooded through the network. Is that a realistic attack? How can it be mitigated? = Minor There is a "MAY" in 4.1 that is a bit odd. "MAY be expected..." Does that mean that an implementation is allowed to expect, but that is exceptional behaviour? Or does it mean that the condition might happen? Or what? And what is the alternative to the "MAY"? See also 4.2 --- 4.2 has Hence, this peering model is RECOMMENDED over the single-hop peering model Section 4.1. "RECOMMENDED" has the same weight as "SHOULD". So you need to say why a deployment would ever consider option 1. --- Since the value of 266 is only suggested to IANA for the BGP-LS Address Family Link Descriptor TLV in section 8.2, section 5.2.2.1 should really use a "TBD" so that the RFC editor can spot and fix any issues that might arise if IANA assigns a different value. --- Why are the new registries in 8.4, 8.5, and 8.6 created as IETF Review? If you follow the thrust of BGP-LS, then it would be reasonable to make them Expert Review (and to write some advice for the Designated Experts). I *guess* that since you ask for this to be in a new registry group, you are free to set your own rules, but I do wonder. (By the way, since you call the new registry group "BGP SPF" this seems to agree with my major point at the top of this review: this document defines BGP-SPF not BGP-LS-SPF. --- 6.1 NLRI originated by directly connected BGP SPF peers are preferred. No objection to you setting this rule, but have you considered that NLRI coming from remote peers are likely to convey updates relating to remote resources faster than those that have been forwarded hop-by-hop with each hop performing processing? --- 7.1 has When a BGP SPF speaker receives a BGP Update containing a malformed Node NLRI SPF Status TLV in the BGP-LS Attribute [I-D.ietf-idr-rfc7752bis], the corresponding Node NLRI is considered as malformed and MUST be handled as 'Treat-as-withdraw'. An implementation SHOULD log an error (subject to rate-limiting) for further analysis. But 5.2.4 has If the Sequence-Number TLV is not received, then the corresponding NLRI is considered as malformed and MUST be handled as 'Treat-as- withdraw'. An implementation MAY log an error for further analysis. SHOULD or MAY? --- 7.1 Node attribute TLVs and their error handling rules are either defined in [I-D.ietf-idr-rfc7752bis] or derived from [RFC5305] and [RFC6119]. I suspect that this means that some TLVs are in one source, and some are in another, not that there is a choic! --- 7.2 tells us what checks to perform, but not what to do if a check fails. Compare with 7.3. --- 7.4 While it will be clear to one skilled in the art what it means to reset a BGP session, a pointer to the procedure would be valuable. --- Section 9 appears to use lower case rather than upper case BCP 14 terms. Possibly worth resolving. --- Thanks for including section 10, it's important. 10.1 says that being in a single administrative domain "allows for consistent configuration". This is true, but I don't think it in any way guarantees it. What measures can be taken to ensure consistency? --- 10.2 makes a RECOMMENDATION. Looks like a wise one. Why would a deployment deviate from this, and what are the consequences? --- In 10.2 you say... For non-loopback prefixes, the setting of the metric is a local matter and beyond the scope of this document. ...and that seems good to me. Why do you then go on to discuss a possible setting of the metric? i suggest deleting the paragraph... Algorithms such as setting the metric inversely to the link speed as supported in some IGP implementations MAY be supported. However, the details of how the metric is computed are beyond the scope of this document. --- 10.2 has a paragraph with three cases of "SHOULD" Within a BGP SPF Routing Domain, the IGP metrics for all advertised links SHOULD be configured or defaulted consistently. For example, if a default metric is used for one router's links, then a similar metric should be used for all router's links. Similarly, if the link metric is derived from using the inverse of the link bandwidth on one router, then this SHOULD be done for all routers and the same reference bandwidth SHOULD be used to derive the inversely proportional metric. Failure to do so will result in incorrect routing based on link metric. There is some explanation of the consequences of not following these SHOULDs (although the cascaded SHOULDs make it hard to determine where the choice goes wrong). Since you are allowing variation from this advice, you need to explain the alternative "MAY" : what can be done and why would it be done? --- 10.3 has a reasonable RECOMMENDED. But, again, you need to talk about the reasons to vary this and the consequences. --- 10.4 has two SHOULDs. This allows variation that you need to explain. = Nits The English usage in the document could really do with some work. I know that the RFC Editor is paid to fix this sort of thing, but there is really quite a lot of work needed (more than I felt like dealing with in this review). Perhaps you can get someone from the working group to take a pass on it. --- draft-ietf-idr-rfc7752bis is now RFC 9552