[Lsvr] Review of draft-ietf-lsvr-bgp-spf-26

Alvaro Retana <aretana.ietf@gmail.com> Thu, 13 July 2023 14:38 UTC

Return-Path: <aretana.ietf@gmail.com>
X-Original-To: lsvr@ietfa.amsl.com
Delivered-To: lsvr@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 2186FC151B09; Thu, 13 Jul 2023 07:38:17 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: 2.902
X-Spam-Level: **
X-Spam-Status: No, score=2.902 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, GB_SUMOF=5, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_ZEN_BLOCKED_OPENDNS=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001] autolearn=no autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com
Received: from mail.ietf.org ([50.223.129.194]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id cBhUsInhIH-2; Thu, 13 Jul 2023 07:38:12 -0700 (PDT)
Received: from mail-pf1-x42b.google.com (mail-pf1-x42b.google.com [IPv6:2607:f8b0:4864:20::42b]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 8FFA2C13738E; Thu, 13 Jul 2023 07:38:12 -0700 (PDT)
Received: by mail-pf1-x42b.google.com with SMTP id d2e1a72fcca58-6686c74183cso827103b3a.1; Thu, 13 Jul 2023 07:38:12 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1689259092; x=1691851092; h=content-transfer-encoding:cc:to:subject:message-id:date :mime-version:from:from:to:cc:subject:date:message-id:reply-to; bh=kj8cOfuzqBk2E58XKExF7yeDuvwkuxewNHjUVqK1xuI=; b=Vqb4KLPRMasuOfd5zB/qHA17lZrw6alpb7Zyp+Zdboyy7FUezOiRyVkiC37m1RmJuF zNqWMxpEjXEQZWIVt6kVW8stZtIXd0PSDPzj6Zi5S7D2TjblpvNUMUNu2BY4JhPAWfMf GmBBWMB/P7Cx7OfxGrhg0H6elW05KIVWihE56+6dwdL9HauB9Xi/m/sKXf1DocOVvFwK DmdXq9twOahiuSzePtJYC3A2ZvleMBqRQ9OSsDl3OyxTg4SNnNMD3SRmjfbHUKEXLE2a Togcrmn2YNC0V6I4o73Zt5D2cW4bqu82Ya0UgF1H3PmLRFStojmjvAThbcba6EJVJNMy qw3g==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689259092; x=1691851092; h=content-transfer-encoding:cc:to:subject:message-id:date :mime-version:from:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=kj8cOfuzqBk2E58XKExF7yeDuvwkuxewNHjUVqK1xuI=; b=gLn4iu9M3NJanieD395STirT9RPuQ0fwYWx6xvkomF2sAzarr5gOezlZMhsRea2mLn HXjw6m/87UORUF7XoDDB4ZLStl4pMbGI3BQuDay7JIASHLNkdFplBtHE8kva8fgyE27x 4n1c2dEBSraHpk98g7pHhUYw3U84rMydwqzsKWSJTCBPzh0uOhSHfQ/r5+FKDUVFzxxl lm3VTjCwM/doSe904fbVIgzW7kghgNp8NJyE8w7WG+dP+n6+n2iaRa7EfRX3n6CIh27x WP4A3F4i3Up7OAPe1S/BTQ4FOkCingL+FIbI9iy5kuYhD7n/hIs9AxoXRk/vwPnoG3mj V2cA==
X-Gm-Message-State: ABy/qLbhxMQauikuHhhpUln/Fy+f8o9d9UMKvYtrvo6aLu0Sba+8WyHD rmXJ11/MspoTEzeRaw3JODHEnCjK9MWdQjIRD7NMU5eaSmg=
X-Google-Smtp-Source: APBJJlHIYOoV6Jv6anJFKUesCTJsGSsdYQVhhzag2p48LtD0a6WfAs0c470xbv4EdlxC1I9/2Hhu/8u4WVAZudcv0cQ=
X-Received: by 2002:a17:903:2285:b0:1b8:5a49:a290 with SMTP id b5-20020a170903228500b001b85a49a290mr2215996plh.43.1689259091378; Thu, 13 Jul 2023 07:38:11 -0700 (PDT)
Received: from 1058052472880 named unknown by gmailapi.google.com with HTTPREST; Thu, 13 Jul 2023 07:38:10 -0700
From: Alvaro Retana <aretana.ietf@gmail.com>
MIME-Version: 1.0
Date: Thu, 13 Jul 2023 07:38:10 -0700
Message-ID: <CAMMESsxTCrsdd9sP6pWO_1mNugTsGUC2sPz3tSK_631zQYcR9g@mail.gmail.com>
To: Acee Lindem <acee.ietf@gmail.com>
Cc: draft-ietf-lsvr-bgp-spf@ietf.org, James Guichard <james.n.guichard@futurewei.com>, Victor Kuarsingh <victor@jvknet.com>, "lsvr@ietf.org" <lsvr@ietf.org>, lsvr-chairs@ietf.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://mailarchive.ietf.org/arch/msg/lsvr/53qx9ERNrHek1U4OphEHQJrhT_o>
Subject: [Lsvr] Review of draft-ietf-lsvr-bgp-spf-26
X-BeenThere: lsvr@ietf.org
X-Mailman-Version: 2.1.39
Precedence: list
List-Id: Link State Vector Routing <lsvr.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/lsvr>, <mailto:lsvr-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/lsvr/>
List-Post: <mailto:lsvr@ietf.org>
List-Help: <mailto:lsvr-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/lsvr>, <mailto:lsvr-request@ietf.org?subject=subscribe>
X-List-Received-Date: Thu, 13 Jul 2023 14:38:17 -0000

Acee:

Hi!

Jim asked me to look at the latest version because I had done the
original AD review for this document.  I looked at the diffs between
-19 and -26.

I have some minor comments and nits -- see them inline below (based on
-26).  I do want to highlight a couple of points:

(1) Synchronization

   There are several errors that result in loosing the synchronization, and not
   being able to recover it.  Most of them have to do with discarding the
   BGP-LS Attribute and using treat-as-withdraw (which I think are the correct
   actions).  The problem can't be avoided, but it should be mentioned as a
   potential issue. I put a couple of suggestions in §5.1.2 (line 458).

(2) Next Hop

   The next-hop-related information from BGP is not used because BGP SPF nodes
   "locally compute the Local-RIB Next-Hop as a result of the SPF process"
   (§5.3).  However, text in §3 and §5.3 try to (confusingly) specify the
   behavior.  In short, §5.3 is not needed, and §3 can simply point at
   rfc7752bis.  See the comments in §5.3 (line 765).


(3) Semantic Validation

   I think the "hack" to point at the original IGP documents works! ;-)  There
   are some missing references (look for the comments around lines 1256 and
   1265) as well as making sure that the non-IGP-originated TLVs (like the ones
   defined in rfc9086) are covered.


I will leave it to Jim to decide if the document is ready for IETF LC.

Thanks!

Alvaro.


[Line numbers from idnits.]

...
260	2.  Base BGP Protocol Relationship

262	   With the exception of the decision process, the BGP SPF extensions
263	   leverage the BGP protocol [RFC4271] without change.  This includes
264	   the BGP protocol Finite State Machine, BGP messages and their
265	   encodings, processing of BGP messages, BGP attributes and path
266	   attributes, BGP NLRI encodings, and any error handling defined in the
267	   [RFC4271] and [RFC7606].

[nit] s/defined in the/defined in

269	   Due to the changes to the decision process, there are mechanisms and
270	   encodings that are no longer applicable.  While not necessarily
271	   required for computation, the ORIGIN, AS_PATH, LOCAL_PREF (IBGP), and
272	   NEXT_HOP path attributes are mandatory [RFC4271] and are validated.
273	   Unless explicitly specified in the context of BGP SPF, all other
274	   attributes SHOULD NOT be advertised.  However, if they are
275	   advertised, they will be accepted, validated, and propagated
276	   consistent with the BGP protocol.

[minor] Suggestion>
OLD>
   While not necessarily required for computation, the ORIGIN, AS_PATH,
   LOCAL_PREF (IBGP), and NEXT_HOP path attributes are mandatory [RFC4271] and
   are validated.  Unless explicitly specified in the context of BGP SPF, all
   other attributes SHOULD NOT be advertised.  However, if they are advertised,
   they will be accepted, validated, and propagated consistent with the BGP
   protocol.

NEW>
   Unless explicitly specified in the context of BGP SPF, all optional path
   attributes SHOULD NOT be advertised.  If received, all path attributes MUST
   be accepted, validated, and propagated consistent with the BGP protocol
   [rfc4271], even if not needed by BGP SPF.



...
302	3.  BGP Link-State (BGP-LS) Relationship
...
320	   The rules for setting the NLRI next-hop path attribute for the BGP-
321	   LS-SPF SAFI follow the BGP-LS SAFI as specified in section 3.4 of
322	   [I-D.ietf-idr-rfc7752bis].

[major] §5.3 in this document also talks about the next hop, and the
specification there is not the same as what it is here.  Please
specify things only once -- see my comments there (search for line
765).



...
421	5.1.1.  BGP-LS-SPF NLRI TLVs
...
427	   The NLRI and conprising TLVs MUST be processed as specified in
428	   section 5.1 [I-D.ietf-idr-rfc7752bis].  TLVs specified as mandatory
429	   in [I-D.ietf-idr-rfc7752bis] are considered mandatory for the BGP-LS-
430	   SPF SAFI as well.  If a mandatory TLV is not specified, the NLRI is
431	   not used in the BGP SPF route calculation.  All the other TLVs are
432	   considered as an optional TLVs.

[minor] "If a mandatory TLV is not specified, the NLRI is not used in
the BGP SPF route calculation."

Did you mean "not present" (instead of "not specified")?

Can we make this behavior normative: MUST NOT be used in the BGP SPF
route calculation ?



434	5.1.2.  BGP-LS Attribute

436	   The BGP-LS attribute of the BGP-LS-SPF SAFI uses exactly same format
437	   of the BGP-LS AFI [I-D.ietf-idr-rfc7752bis].  In other words, all the
438	   TLVs used in BGP-LS attribute of the BGP-LS AFI are applicable and
439	   used for the BGP-LS attribute of the BGP-LS-SPF SAFI.  This attribute
440	   is an optional, non-transitive BGP attribute that is used to carry
441	   link, node, and prefix properties and attributes.  The BGP-LS
442	   attribute is a set of TLVs.

[minor] To be consistent with the previous section:

NEW>
   All the TLVs defined for the BGP-LS Attribute [rfc7752bis] are applicable
   and can be used with the BGP-LS-SPF SAFI to carry link, node, and prefix
   properties and attributes.



444	   The BGP-LS attribute may potentially grow large in size depending on
445	   the amount of link-state information associated with a single Link-
446	   State NLRI.  The BGP specification [RFC4271] mandates a maximum BGP
447	   message size of 4096 octets.  It is RECOMMENDED that an
448	   implementation support [RFC8654] in order to accommodate larger size
449	   of information within the BGP-LS Attribute.  BGP SPF speakers MUST
450	   ensure that they limit the TLVs included in the BGP-LS Attribute to
451	   ensure that a BGP update message for a single Link-State NLRI does
452	   not cross the maximum limit for a BGP message.  The determination of
453	   the types of TLVs to be included by the BGP SPF speaker originating
454	   the attribute is outside the scope of this document.  When a BGP SPF
455	   speaker finds that it is exceeding the maximum BGP message size due
456	   to addition or update of some other BGP Attribute (e.g., AS_PATH), it
457	   MUST consider the BGP-LS Attribute to be malformed and the attribute
458	   discard handling of [RFC7606] applies.

[major] I made these comments before:

-----
[major] As above, most of this text is a copy from rfc7752bis.


[major] "MUST consider the BGP-LS Attribute to be malformed and the
attribute discard handling of [RFC7606] applies."

In BGP SPF this is a serious problem because it results in loss of
synchronization: after a router discards the attribute all the other
routers won't have the same information as the ones before the
discard.

§7.3 has some text (also copied from rfc7752bis) that tries to say
that it is ok to discard the BGP-LS Attribute because the remains can
be used for troubleshooting. Even if that was true (see my comments
there), the lack of synchronization remains.

Can this issue be mitigated?
-----

The reply was: "If the BGP-LS attribute is malformed, there really
isn't anything that can be done."


I agree. :-(  Some comments:

(1) The text above is still a copy from rfc7752bis.  It's not needed
unless it is specifying something different.

(2) Is there an opportunity to consider "some other BGP Attribute" as
malformed and drop that one instead?  Obviously the AS_PATH is not
it...  Just thinking out loud...

(3) Following the specification above/rfc7752bis results in the UPDATE
being propagated, but without the BGP-LS Attribute.  Among other
things, the Sequence-Number TLV won't be received and §5.2.4 says:

   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.

This text addresses the issue of the downstream nodes trying to use
the incomplete information.  Can the optional logging be changed to a
recommendation or a requirement (SHOULD/MUST)?

But the loss of synchronization is still an issue as the view of the
topology will be incomplete (in the downstream nodes).  Please add
some text related to the potential effect in the network -- maybe in
the Management Considerations section.  Note that this issues comes up
whenever a node uses "treat-as-withdraw" (as in §7.1).

(4) Support for rfc8654 is RECOMMENDED in rfc7752bis.  Consider making
it REQUIRED for BGP SPF.



460	5.2.  Extensions to BGP-LS
...
473	   The protocol identifier specified in the Protocol-ID field
474	   [I-D.ietf-idr-rfc7752bis] represents the origin of the advertised
475	   NLRI.  For Node NLRI and Link NLRI, this MUST be the direct protocol
476	   (4).  Node or Link NLRI with a Protocol-ID other than the direct
477	   protocol is considered malformed.  For Prefix NLRI, the specified
478	   Protocol-ID MUST be the origin of the prefix.  The local and remote
479	   node descriptors for all NLRI MUST include the BGP Identifier (TLV
480	   516) and the AS Number (TLV 512) [I-D.ietf-idr-rfc7752bis].  The BGP
481	   Confederation Member (TLV 517) [I-D.ietf-idr-rfc7752bis] is currently
482	   not applicable.

[minor] s/(TLV 516)/(TLV 516) [RFC9086]

The reference should be normative.


[minor] s/(TLV 517) [I-D.ietf-idr-rfc7752bis]/(TLV 517) [RFC9086]



...
520	5.2.1.2.  BGP-LS-SPF Node NLRI Attribute SPF Status TLV
...
551	   If a BGP SPF speaker received the Node NLRI but the SPF Status TLV is
552	   not received, then any previously received information is considered
553	   as implicitly withdrawn and the update is propagated to other BGP SPF
554	   speakers.  A BGP SPF speaker receiving a BGP Update containing a SPF
555	   Status TLV in the BGP-LS attribute [I-D.ietf-idr-rfc7752bis] with a
556	   value that is undefined values SHOULD be advertised to other BGP SPF
557	   speakers.  However, a BGP SPF speaker MUST NOT use the Status TLV in
558	   its SPF computation.  An implementation MAY log this condition for
559	   further analysis.

[nit] s/value that is undefined values/value that is undefined


561	   If the BGP-LS-SPF Status TLV is advertised and the advertised value
562	   is not defined, then the SPF Status TLV is ignored and not used in
563	   SPF computation but is still advertised to other BGP SPF speakers.
564	   An implementation MAY log an error for further analysis.

[] This (new) paragraph (which is also in §5.2.2.2) repeats the
normative statements in the previous one.  It is not needed.



566	5.2.2.  Link NLRI Usage
...
571	   Link NLRI is advertised with unique local and remote node descriptors
572	   dependent on the IP addressing.  For IPv4 links, the link's local
573	   IPv4 (TLV 259) and remote IPv4 (TLV 260) addresses are used.  For
574	   IPv6 links, the local IPv6 (TLV 261) and remote IPv6 (TLV 262)
575	   addresses are used.  For unnumbered links, the link local/remote
576	   identifiers (TLV 258) are used.  For links supporting having both
577	   IPv4 and IPv6 addresses, both sets of descriptors MAY be included in
578	   the same Link NLRI.  The link descriptors are described in table 4 of
579	   [I-D.ietf-idr-rfc7752bis].

[major]  "For unnumbered links, the link local/remote identifiers (TLV
258) are used."

§1.5/rfc5307 says this about the link identifiers in this TLV:

   Link Identifiers are exchanged in the Extended Local Circuit ID field
   of the "Point-to-Point Three-Way Adjacency" IS-IS Option type [ISIS-3way].

IS-IS uses Hellos to figure out the IDs.  But there is no 3-way handshake,
or Hellos in BGP SPF.  How are these values learned?



...
601	5.2.2.1.  BGP-LS-SPF Link NLRI Attribute Prefix-Length TLVs
...
626	   The maximum prefix-length is 32 bits for an IPv4 Prefix-Length TLV
627	   and 128 bits for an IPv6 Prefix-Length TLV.  A prefix-length field
628	   indicating a larger value is in error and the the corresponding Link
629	   NLRI is considered as malformed and MUST be handled as 'Treat-as-
630	   withdraw' [RFC7606].  An implementation MAY log.  An implementation
631	   MAY log an error for further analysis.

[nit] s/An implementation MAY log.  An/An



...
765	5.3.  NEXT_HOP Attribute Manipulation

[major] I still don't feel comfortable with this section, including
this text from §3:

    The rules for setting the NLRI next-hop path attribute for the BGP-
    LS-SPF SAFI follow the BGP-LS SAFI as specified in section 3.4 of
    [I-D.ietf-idr-rfc7752bis].


Let's start here: the next hop information carried in BGP SPF exists
in the "Network Address of Next Hop" field (aka "next hop information"
[rfc4760], but "next hop address" is ok too [rfc7752bis]) of the
MP_REACH_NLRI attribute [rfc4760], not in the NEXT_HOP attribute.
IOW, the text in §3 is wrong because the NEXT_HOP attribute is not
used...and ignored when using MP_REACH_NLRI [rfc4760].

§5.5 (not §3.4) of rfc7752bis ("BGP Next-Hop Information") does talk
about how BGP-LS expects the value of the next hop information to be
set.

Suggestion>
   The rules for setting the next hop information for the BGP-LS-SPF SAFI
   follow the specification in section 5.5 of [I-D.ietf-idr-rfc7752bis].



767	   All BGP peers that support SPF extensions will locally compute the
768	   Local-RIB Next-Hop as a result of the SPF process.  Consequently, the
769	   Next Hop Attribute is always ignored on receipt.  The Next Hop
770	   address MUST be encoded as described in [RFC4760].  BGP SPF speakers
771	   MUST interpret the Next Hop address of MP_REACH_NLRI attribute as an
772	   IPv4 address whenever the length of the Next Hop address is 4 octets,
773	   and as a IPv6 address whenever the length of the Next Hop address is
774	   16 octets.

The first sentence is ok.

The second one is not needed because the NEXT_HOP attribute is already
ignored according to rfc4760.  No need to respecify it here.

The third and fourth are already covered in rfc7752bis.


776	   [RFC4760] modifies the rules of NEXT_HOP attribute whenever the
777	   multiprotocol extensions for BGP-4 are enabled.  BGP SPF speakers
778	   MUST set the NEXT_HOP attribute according to the rules specified in
779	   [RFC4760] as the BGP-LS-SPF routing information is carried within the
780	   multiprotocol extensions for BGP-4.

First sentence: rfc4760 doesn't modify the rules of the NEXT_HOP
attribute, it in fact uses them to set the next hop information in the
MP_REACH_NLRI attribute.

While the second sentence is true, it is the same thing that
rfc7752bis says and is covered by the proposed statement in §3
(above).

In summary, only the proposed sentence in §3 is needed (+ "All BGP
peers that support SPF extensions will locally compute the Local-RIB
Next-Hop as a result of the SPF process.", which can also go in §3).
This section can be removed.



...
910	6.3.  SPF Calculation based on BGP-LS-SPF NLRI
...
1006	       *  If the Current-Prefix's corresponding prefix is in the Local-
1007	          RIB and the cost is the same as the Local-RIB route's metric,
1008	          the Current-Node's next-hops are merged with Local-RIB route's
1009	          next-hops.  The algorithm below supports Equal Cost Multi-Path
1010	          (ECMP) routes.  Some platforms or implementations may have
1011	          limits on the number of ECMP routes that can be supported.
1012	          The setting or identification of any limitations is outside
1013	          the scope if this document.  Nonetheless, step 4 (below)
1014	          includes a set of recommendations in case such as limit is
1015	          encountered.  Weighted Unequal Cost Multi-Path routes are out
1016	          of scope as well.

[nit] s/such as limit/such a limit



...
1200	7.1.  Processing of BGP-LS-SPF TLVs
...
1244	   When a BGP SPF speaker receives a BGP Update containing any malformed
1245	   BGP-LS Attribute TE and IGP Metric TLV, then the spreaker MUST drop
1246	   an entire BGP-LS Attribute.  In such cases, a BGP SPF speaker MUST
1247	   NOT pass the dropped BGP-LS Attribute to other BGP peers as specified
1248	   in [RFC7606].  The corresponding NLRI is considered as a malformed
1249	   and MUST be handled as 'Treat-as-withdraw'.  An implementation SHOULD
1250	   log an error (subject to rate-limiting) for further analysis.

[major] "MUST drop an entire BGP-LS Attribute"

The text also says that the "Link NLRI is considered as malformed and
MUST be handled as 'Treat-as-withdraw'". This means that it doesn't
matter what happens to the attribute. Is there a reason to keep that
text?



1252	   The BGP-LS Attribute consists of Node attribute TLVs, Link attribute
1253	   TLVs, and the Prefix attribute TLVs.  Node attribute TLVs and their
1254	   error handling rules are either defined in [I-D.ietf-idr-rfc7752bis]
1255	   or derived from xref target="RFC5305" format="default"/> and xref
1256	   target="RFC6119" format="default"/>.

[nit] You missed the opening "<" in "xref".


[major] I think that what is missing is an explicit indication of the
action to be taken.  Let's say that a TLV is malformed according to
rfc5305, what should BGP SPF do?  I'm assuming the answer is the same
as the other actions (treat-as-withdraw), but that should be explicit.


[] The same comments apply to the next two paragraphs.


1258	   Link Attribute TLVs and their error handling rules are either defined
1259	   in [I-D.ietf-idr-rfc7752bis] or derived from xref target="RFC5305"
1260	   format="default"/> and xref target="RFC6119" format="default"/>.

1262	   Prefix Attribute TLVs and their error handling rules are either
1263	   defined in [I-D.ietf-idr-rfc7752bis] or derived from xref
1264	   target="RFC5130" format="default"/> and xref target="RFC2328"
1265	   format="default"/>.


[major] What about the validation of the Node/Link/Prefix Descriptor
TLVs in the NLRIs?  Please include that too.



...
1273	7.2.  Processing of BGP-LS-SPF NLRIs

1275	   A Link-State NLRI MUST NOT be considered as malformed or invalid
1276	   based on the inclusion/exclusion of TLVs or contents of the TLV
1277	   fields (i.e., semantic errors), as described in Section 5.1 and
1278	   Section 5.1.1.

[major] This first paragraph doesn't apply anymore: as specified in
the last section, the TLVs are considered malformed...  Also, there
are requirements earlier about the inclusion of some TLVs.



1280	   A BGP-LS-SPF Speaker MUST perform the following syntactic validation
1281	   of the BGP-LS-SPF NLRI to determine if it is malformed.

1283	   1.  Does the sum of all TLVs found in the BGP MP_REACH_NLRI attribute
1284	       correspond to the BGP MP_REACH_NLRI length?

1286	   2.  Does the sum of all TLVs found in the BGP MP_UNREACH_NLRI
1287	       attribute correspond to the BGP MP_UNREACH_NLRI length?

1289	   3.  Does the sum of all TLVs found in a BGP-LS-SPF NLRI correspond to
1290	       the Total NLRI Length field of all its Descriptors?

1292	   4.  When an NLRI TLV is recognized, is the length of the TLV and its
1293	       sub-TLVs valid?

1295	   5.  Has the syntactic correctness of the NLRI fields been verified as
1296	       per [RFC7606]?

1298	   6.  Has the rule regarding ordering of TLVs been followed as
1299	       described in Section 5.1.1?

[major] These checks were copied from (an old version of) rfc7752bis.
The updated text in §8.2.2 is not phrased in the form of a question.
Please reference that.

Suggestion>
   A BGP-LS-SPF Speaker MUST perform the syntactic validation checks of the
   BGP-LS-SPF NLRI listed in Section 8.2.2 of [rfc7752bis] to determine if
   it is malformed.



...
1309	7.3.  Processing of BGP-LS Attribute

1311	   A BGP-LS Attribute MUST NOT be considered as malformed or invalid
1312	   based on the inclusion/exclusion of TLVs or contents of the TLV
1313	   fields (i.e., semantic errors), as described in Section 5.1 and
1314	   Section 5.1.1.

[major] Same comment as above.



1316	   A BGP-LS-SPF Speaker MUST perform the following syntactic validation
1317	   of the BGP-LS Attribute to determine if it is malformed.

1319	   1.  Does the sum of all TLVs found in the BGP-LS-SPF Attribute
1320	       correspond to the BGP-LS Attribute length?

1322	   2.  Has the syntactic correctness of the Attributes (including BGP-LS
1323	       Attribute) been verified as per [RFC7606]?

1325	   3.  Is the length of each TLV and, when the TLV is recognized then,
1326	       its sub-TLVs in the BGP-LS Attribute valid?

[] Same observation...

Suggestion>
   A BGP-LS-SPF Speaker MUST perform the syntactic validation checks of the
   BGP-LS Attribute listed in Section 8.2.2 of [rfc7752bis] to determine if
   it is malformed.



...
1331	8.  IANA Considerations
...
1340	8.2.  BGP-LS-SPF Assignments to BGP-LS NLRI and Attribute TLV Registry

1342	   IANA has assigned five TLVs for BGP-LS-SPF NLRI in the "BGP-LS NLRI
1343	   and Attribute TLV" registry.  These TLV types include the SPF
1344	   capability TLV, Sequence Number TLV, IPv4 Link Prefix-Length TLV,
1345	   IPv6 Link Prefix-Length TLV, and SPF Status TLV.

1347	     +==========+===============+===================================+
1348	     | TLV Code | Description   | Reference                         |
1349	     | Point    |               |                                   |
1350	     +==========+===============+===================================+
1351	     | 1180     | SPF           | Section 5.2.1.1                   |
1352	     |          | Capability    |                                   |
1353	     +----------+---------------+-----------------------------------+
1354	     | 1184     | SPF Status    | Section 5.2.1.2, Section 5.2.2.2, |
1355	     |          |               | and Section 5.2.3.1               |
1356	     +----------+---------------+-----------------------------------+
1357	     | 1182     | IPv4 Link     | Section 5.2.2.1                   |
1358	     |          | Prefix Length |                                   |
1359	     +----------+---------------+-----------------------------------+
1360	     | 1183     | IPv6 Link     | Section 5.2.2.1                   |
1361	     |          | Prefix Length |                                   |
1362	     +----------+---------------+-----------------------------------+
1363	     | 1181     | Sequence      | Section 5.2.4                     |
1364	     |          | Number        |                                   |
1365	     +----------+---------------+-----------------------------------+

1367	                       Table 1: NLRI Attribute TLVs

[major] The references should be to RFCXXX ([this document]), not the
sections.  The registry contains a mixture of references to RFCs and
RFC+Section -- I'll leave the decision of which format to use to you.



1369	8.3.  BGP-LS-SPF Node NLRI Attribute SPF Status TLV Status Registry

1371	   IANA is requested to create the "BGP-LS-SPF Node NLRI Attribute SPF
1372	   Status TLV Status" Registry for status values.  The allocation of the
1373	   unsigned 8-bit status are defined in the table below:

[major] Where should this registry (and the ones created in §8.4 and
§8.5) be located?  IMO, a new group should be created for BGP SPF --
the alternative of locating them in the BGP-LS or BGP group doesn't
seem right to me.

Suggestion>
   IANA is requested to create the "BGP-LS-SPF Node NLRI Attribute SPF
   Status TLV Status" Registry for status values in a new BGP SPF group.
   Initial values for this registry are provided below.  Future assignments
   are to be made using the IETF Review registration policy [rfc8126].

1375	           +=======+==========================================+
1376	           | Range | Assignment Policy                        |
1377	           +=======+==========================================+
1378	           | 0     | Reserved (Not to be assigned)            |
1379	           +-------+------------------------------------------+
1380	           | 1     | Node unreachable with respect to BGP SPF |
1381	           +-------+------------------------------------------+
1382	           | 2     | Node does not supprot transit traffic    |
1383	           |       | with respect to BGP SPF                  |
1384	           +-------+------------------------------------------+
1385	           | 3-254 | Unassigned (IETF Review)                 |
1386	           +-------+------------------------------------------+
1387	           | 255   | Reserved (Not to be assigned)            |
1388	           +-------+------------------------------------------+

1390	               Table 2: BGP-LS-SPF Node NLRI Attribute SPF
1391	                  Status TLV Status Registry Assignments

[major] This table mixes the registration policy with the assigned
values.  Given that only IETF Review is used for future assignments,
there's no need to indicate the policy for unassigned values -- only
the assignments made in this document are needed:

   Value          Description

     0       Reserved
     1	     Node unreachable with respect to BGP SPF
     2	     Node does not supprot transit traffic with respect to BGP SPF
     3-255   Unassigned
     255     Reserved


[] Please apply the same suggestions to the other two sections.



...
1435	9.  Security Considerations
...
1445	   As the modifications described in this document for BGP SPF apply to
1446	   IPv4 Unicast and IPv6 Unicast as underlay SAFIs in a single BGP SPF
1447	   Routing Domain, the BGP security solutions described in [RFC6811] and
1448	   [RFC8205] are somewhat out of scope as they are meant to apply for
1449	   inter-domain BGP where multiple BGP Routing Domains are typically
1450	   involved.  The BGP-LS-SPF SAFI NLRI described in this document are
1451	   typically advertised between EBGP or IBGP speakers under a single
1452	   administrative domain.

[nit] s/somewhat out of scope/out of scope



...
1486	10.2.  SPF Algorithm Consistency

1488	   Within a BGP SPF Routing Domain, all routers MUST use the same SPF
1489	   algorithm (refer to Section 5.2.1.1).  This is the responsibility of
1490	   the administration for the routing domain.

[minor] Please add something about the effect of not having the same
SPF Algorithm.  §5.2.1.1 already says that "only BGP nodes advertising
the SPF capability TLV with same SPF algorithm are included in the SPF
computation"; that is the effect from a management/operational point
of view?  I'm thinking about partitioned networks, incomplete
topologies, etc..


[EoR -26]