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

[Ketan Talaulikar <[e-mail]>]

Hi Adrian,

Could you please check and update your RTGDIR last-call review of this
document based on the latest updates following the discussions during
IETF 121?

Thanks,
Ketan (as shepherding co-chair)


On Fri, Jul 26, 2024 at 3:11 AM Acee Lindem <[e-mail]> wrote:
>
> 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]> 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
>>
>>
>>
>