[RTG-DIR]Re: Rtgdir last call review of draft-ietf-lsvr-bgp-spf-31

[Acee Lindem <[e-mail]>]

Hi Ketan, Adrian, 

> On Jul 11, 2024, at 01:03, Ketan Talaulikar <[e-mail]> wrote:
> 
> Hi Adrian,
> 
> Thanks for your review. Request the authors to respond and for the WG members to also chime in.
> 
> Thanks,
> Ketan
> 
> 
> On Sat, Jul 6, 2024 at 2:18 AM Adrian Farrel via Datatracker <[e-mail] <mailto:[e-mail]>> wrote:
>> Reviewer: Adrian Farrel
>> Review result: Not Ready
>> 
>> 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?


We can discuss this in today’s meeting. Perhaps, we should remove the reference to “very stable environments”. I’d expect BGP SPF to perform better than RFC 7938.



>> 
>> = 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?

I’m not sure why you’d consider this a medium comment. The reference is to be removed before publication. I’ll go ahead and remove it now. 



>> 
>> ---
>> 
>> 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.

Possibly we’ll remove this deployment model for now. 



>> 
>> ---
>> 
>> 8.2 names a registry that does not exist (with the given name).
>> Please don't leave IANA guessing.


Fixed. 


>> 
>> ---
>> 
>> 5.2.1.1, 5.2.2.2, and 5.2.3.1 describe "SPF Status" fields of
>> the various TLVs.

SPF Status is a TLV. 


>> 
>> It might help if the fields had different names to help not confuse
>> anything (e.g., Node SPF Status).


There is a single SPF Status TLV used for different NLRI. 


>> 
>> 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).


Fixed. 


>> 
>> ---
>> 
>> 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,

Actually, it is the opposite. It is needed for unnumbered links. 

>> 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?

This applies to SPF status information only - so it is any pending SPF status information. 




>> 
>> Ditto 5.2.3.1

Same as above.


>> 
>> ---
>> 
>> 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?

BGP SPF is no more fragile than BGP. 



>> 
>> = 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


Waiting for End-of-RIB is an optional feature. 


>> 
>> ---
>> 
>> 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.

BGP itself allow single hop or loopback peering. 



>> 
>> ---
>> 
>> 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.


Fixed. 




>> 
>> ---
>> 
>> 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.

Fixed. 

>> 
>> (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.

This registry falls under the category of BGP-LS. 

>> 
>> ---
>> 
>> 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?


You are misinterpreting this rule. In other words, NLRI corresponding to the peer itself will be preferred. I changed this to “self-originated”. 



>> 
>> ---
>> 
>> 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?

SHOULD


>> 
>> ---
>> 
>> 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!

Please suggest text. 



>> 
>> ---
>> 
>> 7.2 tells us what checks to perform, but not what to do if a check
>> fails. Compare with 7.3.


These checks are defined in RFC 9552 and need not be repeated in this document. 


>> 
>> ---
>> 
>> 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.


TBD.

>> 
>> ---
>> 
>> Section 9 appears to use lower case rather than upper case BCP 14 terms.
>> Possibly worth resolving.


Fixed. 

>> 
>> ---
>> 
>> 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?

This is beyond the scope of the document. 




>> 
>> ---
>> 
>> 10.2 makes a RECOMMENDATION. Looks like a wise one. Why would a
>> deployment deviate from this, and what are the consequences?

There may be use cases that go beyond the scope of the document. 


>> 
>> ---
>> 
>> 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.

Why? This is useful. 



>> 
>> ---
>> 
>> 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.


I don’t think this is reasonable - please suggest text. 



>> 
>> ---
>> 
>> 10.4 has two SHOULDs. This allows variation that you need to explain.


These are both SHOULDs. I don’t see that any more explanation necessary. 


Thanks,
Acee



>> 
>> = 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
>> 
>> 
>>