Hi Alvaro,
Thanks for your review.
The review was however received in a truncated manner in my mailbox. The authors should refer to the full version on the mailer archive in case they too are affected by the same issue.
Thanks,
Ketan (as document shepherd)
On Fri, Dec 6, 2024 at 3:42 AM Alvaro Retana <aretana.ietf@xxxxxxxxx> wrote:
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 https://wiki.ietf.org/en/group/rtg/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-lsvr-bgp-spf-39
Reviewer: Alvaro Retana
Review Date: December 5, 2024
IETF LC End Date: December 10, 2024
Intended Status: Standards Track
Summary:
This document is basically ready for publication, but I have some concerns
that I think should be resolved before publication.
Comments:
I was the Responsible AD for this document until -21, and reviewed -27. I
also participated in the WG's discussion of the last RtgDir review (-31).
I based my review mainly on the diffs between -28 and -39.
In general, this is a well-written document. Its revisions have improved
the specification. Any remaining issues (see details below) should be easy
to address.
Major Issues:
While I identified several issues as "major" (below -- mostly because of
their Normative impact), I only want to highlight 2 here:
(1) In §5.2.2.1 (BGP-LS Link NLRI Address Family Link Descriptor TLV), the
case where multiple AFs are supported on the link is not described. See
comments around line 616.
(2) The IANA Considerations subsection 8.2 (BGP-LS-SPF Assignments to
BGP-LS NLRI and Attribute TLV Registry) addresses the fact that some
early-allocated values are not required anymore, but rfc9552 doesn't
specify a process to remove unrequired code points from the registry. The
code points should be marked as "Deprecated". See comments around line
1324.
Minor Issues and Nits: Please see details below.
Thanks!
Alvaro.
[Line numbers from idnits.]
...
182 1.2. BGP Shortest Path First (SPF) Motivation
...
219 With controller and route-reflector peering models, BGP SPF
220 advertisement and distributed computation require a minimal number of
221 sessions and copies of the NLRI since only the latest version of the
222 NLRI from the originator is required (sed Section 4). Given that
[nit] s/sed/see
...
304 3. BGP Link-State (BGP-LS) Relationship
...
315 The "BGP-LS NLRI and Attribute TLVs" registry [RFC9552] is shared
316 between the BGP-LS SAFI and the BGP-LS-SPF SAFI. However, the TLVs
317 defined in this document are not applicable to the BGP-LS SAFI and
318 BGP-LS SAFI consumers [RFC9552] MUST NOT result in the NLRI or the
319 BGP-LS Attribute being considered malformed (section 5.2 of
320 [RFC9552]). The list of BGP-LS TLVs applicable to the BGP-LS-SPF
321 SAFI are described in Section 5.2. By default, the usage of other
322 BGP-LS TLVs or extensions are ignored for the BGP-LS-SPF SAFI.
323 However, this doesn't preclude the usage specification of these TLVs
324 for the BGP-LS-SPF SAFI in future documents.
[major] "...the TLVs defined in this document are not applicable to the
BGP-LS SAFI and BGP-LS SAFI consumers [RFC9552] MUST NOT result in the NLRI
or the BGP-LS Attribute being considered malformed (section 5.2 of
[RFC9552])."
This document cannot require any action from BGP-LS nodes.
The text may have been an attempt to copy the specification in §5.1/rfc9552
(not §5.2): "The presence of unknown or unexpected TLVs MUST NOT result in
the NLRI or the BGP-LS Attribute being considered malformed."
Suggestion>
OLD>
However, the TLVs defined in this document are not applicable to the
BGP-LS SAFI and BGP-LS SAFI consumers [RFC9552] MUST NOT result in the
NLRI or the BGP-LS Attribute being considered malformed (section 5.2
of [RFC9552]).
NEW>
However, the TLVs defined in this document may not be applicable to
the BGP-LS SAFI. As specified in Section 5.1 of RFC9552, the presence
of unknown or unexpected TLVs is required to not result in the NLRI or
the BGP-LS Attribute being considered malformed.
...
337 4.1. BGP Single-Hop Peering on Network Node Connections
...
347 An End-of-RIB (EoR) Marker Section 5.3 for the BGP-LS-SPF SAFI MAY be
348 required from a peer prior to advertising the BGP-LS Link NLRI for
349 the corresponding link to that peer.
[nit] s/Section 5.3/(Section 5.3)/g
[major, but it may just be me] "MAY be required from a peer"
The Normative text sounds confusing even if "required" is in lowercase.
The previous text (-28, for example) said "MAY be expected". I agree that
adding "from a peer" helps with clarity.
s/MAY be required from a peer/MAY be expected from a peer/g
...
498 5.2.1.1. BGP-LS-SPF Node NLRI Attribute SPF Status TLV
...
528 If a BGP SPF speaker received the Node NLRI but the SPF Status TLV is
529 not received, then any previously received SPF status information is
530 considered as implicitly withdrawn and the update is propagated to
531 other BGP SPF speakers. A BGP SPF speaker receiving a BGP Update
532 containing a SPF Status TLV in the BGP-LS attribute [RFC9552] with a
533 non-reserved value (3-254) that is unknown SHOULD be advertised to
534 other BGP SPF speakers. However, a BGP SPF speaker MUST NOT use the
535 Status TLV in its SPF computation. An implementation MAY log this
536 condition for further analysis. If the SPF Status TLV contains a
537 reserved value (0 or 255) the TLV is considered malformed and is
538 handled as described in Section 7.1.
[major] "non-reserved value (3-254) that is unknown"
If we're being strict, values 1 and 2 are also "non-reserved" (because they
are assigned). Instead of attaching a normative behavior to values that
may change in the future (and require future documents to Update this one),
use "unknown".
s/a non-reserved value (3-254) that is unknown/an unknown value/g
540 5.2.2. Link NLRI Usage
...
565 The link descriptors are described in table 4 of [RFC9552].
566 Additionally, an address family link descriptor is defined to
567 determine whether an unnumbered link can be used in the IPv4 SPF, the
568 IPv6, or both (refer to Section 5.2.2.1).
[nit] s/an address family link descriptor/the Address Family Link
Descriptor TLV
570 For a link to be used in SPF computation for a given address family,
571 i.e., IPv4 or IPv6, both routers connecting the link MUST have
572 matching addresses (i.e., interface addresses must match the neighbor
573 addresses).
[] Maybe it's just me, but "matching addresses" doesn't tell me what the
characteristics of the addresses should be. It sounds as if they are
required to be the same.
FWIW, the previous description was more explicit: "both routers connecting
the link MUST have an address in the same subnet for that address family."
...
588 5.2.2.1. BGP-LS Link NLRI Address Family Link Descriptor TLV
590 For unnumbered links, the address family cannot be ascertained from
591 the endpoint link descriptors. Hence, the Address Family (AF) Link
592 Descriptor SHOULD be included with the Link Local/Remote Identifiers
593 TLV for unnumbered links, so that the link can be used in the
594 respective address family SPF. If the Address Family Link Descriptor
595 is not present for an unnumbered link, the link will not be used in
596 the SPF computation for either address family. If the Address Family
597 Link Descriptor is present for a numbered link, the link descriptor
598 will be ignored. If the Address Family Link Descriptor TLV contains
599 an undefined value (3-254), the link descriptor will be ignored. If
600 the Address Family Link Descriptor TLV contains a reserved value (0
601 or 255) the TLV is considered malformed and is handled as described
602 in Section 7.1.
604 0 1 2 3
605 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
606 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
607 | Type (TBD) | Length (1 Octet) |
608 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
609 | Address Family|
610 +-+-+-+-+-+-+-+-+
612 Address Family Values: 0 - Reserved
613 1 - IPv4 SPF Computation
614 2 - IPv6 SPF Computation
615 3-254 - Undefined
616 255 - Reserved
[minor] Why are the AFs called "IPvx SPF Computation" and not simply "IPvx"?
[major] What about the case where multiple AFs are supported on the link?
Should multiple TLVs be advertised, or should the length (which is not
specified above) reflect the inclusion of more than one AF?
...
752 5.3. BGP-LS-SPF End of RIB (EoR) Marker
754 The usage of the End-of-RIB (EoR) Marker [RFC4724] with the BGP-LS-
755 SPF SAFI is somewhat different than the other BGP SAFIs. Optionally,
756 reception of the EoR marker MAY be required prior to advertising an
757 LINK-NLRI for a given peer. This is similar to OSPF's [RFC2328]
758 requirement to not advertise an adjacency until database
759 synchronization has completed.
[major] "Optionally, reception of the EoR marker MAY be required..."
This sentence is confusing because, at first, it sounds redundant
("Optionally" and "MAY" basically mean the same thing), but it is then
mixed with a (possibly) *required* behavior (even if used in lowercase).
In §4.1 I suggested using "MAY be expected from a peer". That wording
should be worked in here as well. Also, the behavior is already specified
in §4.1/§4.2, so there's no need to specify it here again.
Suggestion>
Reception of the EoR marker is optionally expected from a peer
prior to advertising a Link NLRI for that peer.
[major] "This is similar to OSPF's..."
This is the one remaining sentence that compares BGP SPF to other
protocols. Not needed, please remove it.
...
813 6.1. BGP SPF NLRI Selection
...
818 1. NLRI self-originated from directly-connected BGP SPF peers are
819 preferred. This condition can be determined by comparing the BGP
820 Identifiers in the received Local Node Descriptor and the BGP
821 OPEN message for an active BGP session. This rule assures that
822 stale NLRI is updated even if a BGP-LS router loses its sequence
823 number state due to a cold-start. Note that once the BGP session
824 goes down, the NLRI received is no longer considered as being
825 from a directly connected BGP SPF peer.
[minor] s/BGP-LS router/BGP SPF router
...
899 6.3. SPF Calculation based on BGP-LS-SPF NLRI
...
1054 - For unnumbered links to match during the IPv4 or IPv6
1055 SPF computation, Current-Link and Remote-Link's Address
1056 Family link descriptor must match address family of the
1057 IPv4 or IPv6 SPF computation, the Current--Link's Local
1058 Identifier MUST match the Remote-Link's Remote
1059 Identifier, and the Current-Link's Remote Identifier
1060 MUST the Remote-Link's Local Identifier. Since the
1061 Link's Remote Identifier may not be known, a value of 0
1062 is considered a wildcard and will match any Current or
1063 Remote Link's Local Identifier (see TLV 258 [RFC9552])
-- last-call mailing list -- last-call@xxxxxxxx To unsubscribe send an email to last-call-leave@xxxxxxxx
