Re: [Idr] AD Review of draft-ietf-idr-bgp-ls-flex-algo-08

[Ketan Talaulikar <[e-mail]>]

Hi Alvaro,

Thanks for your detailed review and please check inline below for responses.

Since the gates are closed for posting, please find attached the updated
draft and its diff for your review.

On Tue, Jul 12, 2022 at 9:30 PM Alvaro Retana <[e-mail]>
wrote:

> Dear authors:
>
> Here is my review of this document.  I found it clear -- however,
> there are updates needed in line with other BGP-LS documents.
>
> I will wait for a reply to this document and a revised ID before
> starting the IETF LC.
>
> Thanks!
>
> Alvaro.
>
>
> [Line numbers from idnits.]
>
>
> ...
> 13      Abstract
>
> 15         Flexible Algorithm is a solution that allows routing protocols
> (viz.
> 16         OSPF and IS-IS) to compute paths over a network based on
> user-defined
> 17         (and hence, flexible) constraints and metrics.  The computation
> is
> 18         performed by routers participating in the specific network in a
> 19         distribute manner using a Flex Algorithm definition.  This
> definition
> 20         provisioned on one or more routers and propagated (viz.  OSPF
> and IS-
> 21         IS flooding) through the network.
>
> [nit] s/distribute manner/distributed manner
>

KT> Ack


>
>
> [minor] This is the first use of "Flex Algorithm" (in place of
> "Flexible Algorithm").  For the Abstract, please keep the long form.
>

KT> Ack


>
> The perm "flex" is used again in the Introduction but without an
> indication of the (possibly obvious) relationship to "flexible".
> Please indicate it somewhere.
>
> More important -- the document mostly uses "flex algorithm"
> throughout, but also "flexible algorithm", "Flex-Algorithm", and "Flex
> Algo".  Please be consistent!
>

KT> Ack. Fixed.


>
>
> [nit] s/This definition provisioned/This definition is provisioned
>

KT> Ack


>
>
> 23         BGP Link-State (BGP-LS) enables the collection of various
> topology
> 24         information from the network.  This draft defines extensions to
> BGP-
> 25         LS address-family to advertise the Flexible Algorithm
> Definition as a
> 26         part of the topology information from the network.
>
> [minor] s/This draft/This document/g
>

KT> Ack


>
>
> [nit] s/to BGP-LS address-family/to the BGP-LS address-family
>

KT> Ack


>
>
> ...
> 82      1.  Introduction
>
> 84         IGP protocols (OSPF and IS-IS) traditionally compute best paths
> over
> 85         the network based on the IGP metric assigned to the links.  Many
> 86         network deployments use RSVP-TE [RFC3209] based or Segment
> Routing
> 87         (SR) Policy [RFC8402] based solutions to enforce traffic over a
> path
> 88         that is computed using different metrics or constraints than the
> 89         shortest IGP path.  [I-D.ietf-lsr-flex-algo] defines the
> Flexible
> 90         Algorithm solution that allows IGPs themselves to compute
> constraint
> 91         based paths over the network.
>
> [nit] s/use RSVP-TE [RFC3209] based or Segment Routing (SR) Policy
> [RFC8402] based solutions...compute constraint based paths/use
> RSVP-TE-based [RFC3209] or Segment Routing (SR) Policy-based [RFC8402]
> solutions...compute constraint-based paths
>
>
KT> Ack


>
> ...
> 103        The operations of the flexible algorithm solution are described
> in
> 104        detail in [I-D.ietf-lsr-flex-algo] and a high-level summary of
> the
> 105        same is described here for clarity.  The network operator
> enables the
> 106        participation of specific nodes in the network for a specific
> 107        algorithm and then provisions the definition of that flexible
> 108        algorithm on one or more of these nodes.  The nodes where the
> 109        flexible algorithm definition (FAD) is advertised then flood
> these
> 110        definitions via respective IGP (IS-IS and OSPFv2/v3) mechanisms
> to
> 111        all other nodes in the network.  The nodes select the
> definition for
> 112        each algorithm based on the flooded information in a
> deterministic
> 113        manner and thus all nodes participating in a flexible algorithm
> 114        computation arrive at a common understanding of the type of
> 115        calculation that they need to use.
>
> [major] "a high-level summary of the same is described here for clarity"
>
> The pointer to I-D.ietf-lsr-flex-algo is enough.  There's no need to
> explain anything else; BGP-LS just carries the information, it doesn't
> matter how the IGPs (or the consumer) may use it.  Please take the
> extra description out.
>
> Note: I didn't review the extra text.
>

KT> Ack - removed that text and only retained the reference to the LSR
draft.


>
>
> ...
> 128        A flex algorithm specific metric MAY be advertised along with
> the
> 129        prefix as described in [I-D.ietf-lsr-flex-algo] to enable
> end-to-end
> 130        optimal path computation for prefixes across multiple
> areas/domains
> 131        in the flex algorithm computation for the SR-MPLS forwarding
> plane.
>
> [major] "MAY be advertised...as described in [I-D.ietf-lsr-flex-algo]"
>
> This Normative word comes from the IGP draft.  It is not appropriate
> to repeat specifications from other documents.  I'm assuming you're
> taking this part out.
>

KT> Ack. Reworded.


>
>
> ...
> 145        Thus a controller or a Path Computation Engine (PCE) is aware
> of the
> 146        IGP topology across multiple domains which includes the above
> 147        information related to the flexible algorithm.  This draft
> defines
> 148        extensions to BGP-LS for carrying the FAD information so that it
> 149        enables the controller/PCE to learn the mapping of the flex
> algorithm
> 150        number to its definition in each area/domain of the underlying
> IGP.
> 151        The controller/PCE also learns the type of computation used and
> the
> 152        constraints for the same.  This information can then be
> leveraged by
> 153        it for setting up SR Policy paths end to end across domains by
> 154        leveraging the appropriate Flex Algorithm specific SIDs in its
> 155        Segment List [I-D.ietf-spring-segment-routing-policy]. e.g.
> picking
> 156        the Flex Algorithm Prefix SID (in case of SR-MPLS) or End SID
> (in
> 157        case of SRv6) of ABRs/ASBRs corresponding to a definition that
> 158        optimizes on the delay metric enables the PCE/controller to
> build an
> 159        end to end low latency path across IGP domains with minimal
> SIDs in
> 160        the SID list.
>
> [major] This paragraph goes back into what this document defines, but
> it also tries to explain the consumer-specific use of the information,
> which is out of scope for BGP-LS.
>

KT> The text has been reworded. The objective is to briefly inform the
reader of a use case for these extensions. Even RFC7752 has covered the use
cases or applications that may leverage BGP-LS. IMHO, it is good to get
some description of the use case or reason for pushing things from the IGPs
into BGP-LS.


>
>
> ...
> 170     2.  BGP-LS Extensions for Flex Algo
>
> [] The header gives the impression that the extensions are defined in
> this section, but they're not.  Consider making §3 a sub-section of
> this one.
>

KT> Have renamed the header.


>
>
> ...
> 178        The FAD advertised by a node is considered as its node level
> 179        attributes and advertised as such.
>
> [nit] s/as its node level attributes/as a node-level attribute
>

KT> Ack


>
>
> [minor] Consider adding a reference to the section where this is specified.
>

KT> Ack


>
>
> 181        Various link attributes like affinities and SRLGs used during
> the
> 182        Flex-Algorithm path calculations in IS-IS and OSPF are
> advertised in
> 183        those protocols using the Application Specific Link Attribute
> (ASLA)
> 184        advertisements as described in [I-D.ietf-lsr-flex-algo].  The
> BGP-LS
> 185        extensions for ASLA advertisements
> 186        [I-D.ietf-idr-bgp-ls-app-specific-attr] MUST be used for the
> 187        advertisement of these Flex-Algorithm application-specific link
> 188        attributes from the underlying IGP protocols using the Flexible
> 189        Algorithm application specific bit defined in
> 190        [I-D.ietf-lsr-flex-algo].
>
> [major] As far as I can tell, the ASLA encoding is used by
> I-D.ietf-lsr-flex-algo as is.  IOW, a BGP-LS speaker would use
> I-D.ietf-idr-bgp-ls-app-specific-attr regardless of whether flex algo
> is used or not.  Right?
>

KT> Correct.


>
> If so, the specification above requiring
> I-D.ietf-idr-bgp-ls-app-specific-attr is not needed.
>
> As far as the new bit (X-bit) defined in I-D.ietf-lsr-flex-algo, the
> inclusion of that bit by BGP-LS in
> I-D.ietf-idr-bgp-ls-app-specific-attr is already defined there (as it
> takes the SABM definition from rfc8919/8920).
>

KT> Agree.


>
> All this is to say that this whole paragraph is not needed.
>
> If anything, you might want to indicate that there's a relationship to
> I-D.ietf-idr-bgp-ls-app-specific-attr.  For example:
>
>    Application-specific link attributes (ASLA) used during the
>    Flex-Algorithm path calculations are advertised as specified
>    in [RFC8919] and [RFC8920].  The BGP-LS extensions for ASLA
>    advertisements are specified in [I-D.ietf-idr-bgp-ls-app-specific-attr].
>
> All these references should be Informative.
>

KT> Ack. Updated the text.


>
>
> 192        The Flexible Algorithm Prefix Metric (FAPM) are considered as
> prefix
> 193        attributes and advertised as such.
>
> [minor] Consider adding a reference to the section where this is specified.
>

KT> Ack.


>
>
> 195     3.  Flexible Algorithm Definition
> ...
> 217        o  Length: variable.  Minimum of 4 octets.
>
> [major] What if the length is less than 4?  What should the receiver do?
>
> [major] Suggestion>
>
>    Length:  variable length that represents the total length of the
>       value field in octets.  The length value MUST be 4 or larger.
>       If the length is less than 4, the TLV MUST be considered
>       malformed.
>
> I'm mostly interested in capturing the error case.
>
> [This comment applies to all the Length fields.  I'm assuming that one
> bad length would invalidate the whole TLV.]
>

KT> I've fixed the text to use normative language. The convention that we
are trying to follow is to use normative language but not discuss fault
management (i.e. not say malformed). The fault management handling is being
clarified in the BGP-LS base as part of the RFC7752 BIS work which will
apply to this as well as all other BGP-LS extensions.


>
>
> 219        o  Flex-Algorithm : 1 octet value in the range between 128 and
> 255
> 220           inclusive which is the range defined for Flexible Algorithms
> in
> 221           the IANA "IGP Parameters" registries under the "IGP Algorithm
> 222           Types" registry [I-D.ietf-lsr-flex-algo].
>
> [major] Please simply refer to the definition in
> I-D.ietf-lsr-flex-algo.  Note that in this case the definition above
> says more than the one in I-D.ietf-lsr-flex-algo: "Flex-Algorithm:
> Single octet value between 128 and 255 inclusive."
>
> Suggestion>
>    o  Flex-Algorithm :  Single octet value between 128 and 255 inclusive,
>       as defined in [I-D.ietf-lsr-flex-algo].
>

KT> Ack.


>
>
> 224        o  Metric-Type : 1 octet value indicating the type of metric
> used in
> 225           the computation.  Values allowed come from the IANA "IGP
> 226           Parameters" registries under the "Flexible Algorithm
> Definition
> 227           Metric-Type" registry [I-D.ietf-lsr-flex-algo].
>
> [major] Same comment as above.  Also, please don't point at "values
> allowed" because then we're getting close to telling the consumer what
> to do.
>
>
> 229        o  Calculation-Type : 1 octet value in the range between 0 and
> 127
> 230           inclusive which is the range defined for the standard
> algorithms
> 231           in the IANA "IGP Parameters" registries under the "IGP
> Algorithm
> 232           Types" registry [I-D.ietf-lsr-flex-algo].
>
> [major] The field in the Figure and in I-D.ietf-lsr-flex-algo is
> Calc-Type.  Please be consistent.
>

KT> Ack


>
>
> [major] Same comment as above.
>

KT> Ack


>
>
> 234        o  Priority : 1 octet value between 0 and 255 inclusive that
> 235           specifies the priority of the FAD.
>
> [major] Same comment as above.
>

KT> Ack


>
>
> 237        o  sub-TLVs : zero or more sub-TLVs may be included as described
> 238           further in this section.
>
> 240        The FAD TLV can only be added to the BGP-LS Attribute of the
> Node
> 241        NLRI if the corresponding node originates the underlying IGP
> TLV/sub-
> 242        TLV as described below.  This information is derived from the
> 243        protocol specific advertisements as below.
>
> [major] "IGP TLV/sub-TLV as described below"
>
> I didn't find anything called "IGP TLV/sub-TLV".  What do you mean?
>
> Ahh...I think I figured it out!  You mean the FAD sub-TLVs mentioned
> below, right?  Instead, be explicit by saying that the values are
> derived from...
>

KT> Fixed the text.


>
>
> 245        o  IS-IS, as defined by the ISIS Flexible Algorithm Definition
> sub-
> 246           TLV in [I-D.ietf-lsr-flex-algo].
>
> 248        o  OSPFv2/OSPFv3, as defined by the OSPF Flexible Algorithm
> 249           Definition TLV in [I-D.ietf-lsr-flex-algo].
>
> [] In most of the other descriptions you mentioned the origin in a
> more compact way: "derived from the IS-IS and OSPF protocol specific
> xxx sub-TLV as defined in [I-D.ietf-lsr-flex-algo]."  FWIW, I like
> that phrasing better.
>

KT> Ack. Fixed


>
>
> 251        The BGP-LS Attribute associated with a Node NLRI MAY include
> one or
> 252        more FAD TLVs corresponding to the FAD for each algorithm that
> the
> 253        particular node is advertising.
>
> [major] "MAY include"
>
> I understand that it is optional to use flex algo in general, which
> means that the TLVs defined here don't have to be included if flex
> algo is not used by the IGPs.  However, I assume that it is required
> to include FAD TLVs for as many algorithms used by the IGPs, right?
> IOW, it is not up to BGP-LS when to advertise this new TLV, it should
> do it always (when the IGP includes the corresponding information).
>
> TL;DR: Why is this action optional and not required?  OTOH, you may
> simply s/MAY/may
>

KT> Ack. Changed to "may".


>
>
> ...
> 257     3.1.  Flex Algo Exclude Any Affinity
>
> 259        The Flex Algo Exclude Any Affinity sub-TLV is an optional
> sub-TLV
> 260        that is used to carry the affinity constraints [RFC2702]
> associated
> 261        with the FAD and enable the exclusion of links carrying any of
> the
> 262        specified affinities from the computation of the specific
> algorithm
> 263        as described in [I-D.ietf-lsr-flex-algo].  The affinity is
> expressed
> 264        in terms of Extended Admin Group (EAG) as defined in [RFC7308].
>
> [major] "used to carry the affinity constraints [RFC2702]"
>
> This description goes beyond what it specified in
> I-D.ietf-lsr-flex-algo, which doesn't even mention rfc2702.  Note also
> that rfc7308 mentions that the "concept of Administrative Groups comes
> from...[RFC2702]", but then doesn't use it as part of the
> specification.  Please take this reference out and focus on what is
> specified in I-D.ietf-lsr-flex-algo.
>
> [This comment applies to the description in the next 2 sub-sections.]
>

KT> Ack. Removed the reference to RFC2702.


>
>
> 266        The sub-TLV has the following format:
>
> 268         0                   1                   2                   3
> 269         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
> 270
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 271        |               Type            |              Length
> |
> 272
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 273        |             Exclude-Any EAG (variable)
> //
> 274
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>
> [minor] Please add figure numbers to all the packet formats.
>

KT> Ack


>
>
> ...
> 283        o  Exclude-Any EAG : the bitmask used to represent the
> affinities to
> 284           be excluded.
>
> [major] Defining this field as including "the affinities to be
> excluded" goes beyond that I-D.ietf-lsr-flex-algo specifies (and
> assumes an action by the consumer), where the corresponding fields are
> simply described as "Extended Administrative Group as defined in
> [RFC7308]."
>
> Please point this description to the specification in
> I-D.ietf-lsr-flex-algo.  You can include text from the following
> paragraph in it.
>
> Suggestion>
>    o  Exclude-Any EAG : one or more sets of 32-bit bitmasks derived
>       from the IS-IS and OSPF protocol specific Flexible Algorithm
>       Exclude Admin Group sub-TLV as defined in [I-D.ietf-lsr-flex-algo].
>

KT> EAG is not actually defined as "one or more sets of 32-bit bitmasks" -
it is a single bitmask of variable size that is in multiples of 32 bits. I
believe there is no issue in referring to RFC7308 to indicate what is it
that is being carried.


>
>
> 286        The information in the Flex Algo Exclude Any Affinity sub-TLV is
> 287        derived from the IS-IS and OSPF protocol specific Flexible
> Algorithm
> 288        Exclude Admin Group sub-TLV as defined in
> [I-D.ietf-lsr-flex-algo].
>
> [The comment above applies to the description in the next 2 sub-sections.]
>

KT> I've reworded the text in the field description but retained the text
after the field description.


>
>
> ...
> 358     3.4.  Flex Algo Definition Flags
> ...
> 381        o  Flags : the bitmask used to represent the flags for the FAD
> as
> 382           introduced by [I-D.ietf-lsr-flex-algo] and listed in the
> "Flex-
> 383           Algorithm Definition Flags" registry under the "Interior
> Gateway
> 384           Protocol (IGP) Parameters" IANA registry.
>
> 386        The information in the Flex Algo Definition Flags sub-TLV is
> derived
> 387        from the IS-IS and OSPF protocol specific Flexible Algorithm
> 388        Definition Flags sub-TLV as defined in [I-D.ietf-lsr-flex-algo]


< adding below the rest of the review from the mailer that got snipped out
for me >

[major] Sugestion>
   o  Flags : the bitmask used to represent the flags for the FAD
      derived from the IS-IS and OSPF protocol specific Flexible
      Algorithm Definition Flags sub-TLV as defined in
      [I-D.ietf-lsr-flex-algo].

KT> Ack - reworded as per the fields of the FAD TLV.


390	3.5.  Flex Algo Exclude SRLG

392	   The Flex Algo Exclude SRLG sub-TLV is an optional sub-TLV that is
393	   used to carry the shared risk link group (SRLG) [RFC4202] information
394	   associated with the FAD and enable the exclusion of links that are
395	   associated with any of the specified SRLG in the computation of the
396	   specific algorithm as described in [I-D.ietf-lsr-flex-algo].  The
397	   SRLGs associated with a link are carried in the BGP-LS Shared Link
398	   Risk Group (TLV 1096) [RFC7752].

[major] "used to carry the shared risk link group (SRLG) [RFC4202]"

This description goes beyond what it specified in
I-D.ietf-lsr-flex-algo, which doesn't even mention rfc4202.  Please
take this reference out and focus on what is specified in
I-D.ietf-lsr-flex-algo.

KT> Updated the text to remove reference to RFC4202.


[?] Just curious.  Why was TLV 1096 not reused?  It seems to me that
the context (using it as a sub-TLV of the FAD TLV) would have been
enough to understand the function.

KT> The semantics of TLV 1096 is that it indicates SRLGs associated
with a link. Here, it is about SRLG exclusion as a constraint and
hence we thought it better to use a new TLV.


...
417	   o  SRLG Values : One or more SRLG values, each of 4 octet size, as
418	      defined in [RFC4202].

[major] The field is called "Shared Risk Link Group Values".  Please
be consistent.

KT> Ack


420	   The information in the Flex Algo SRLG Exclude sub-TLV is derived from
421	   the IS-IS and OSPF protocol specific Flexible Algorithm Exclude SRLG
422	   sub-TLV as defined in [I-D.ietf-lsr-flex-algo].

[major] "SRLG Values : ...as defined in [RFC4202]."

This isn't even the reference in I-D.ietf-lsr-flex-algo.  The values
come from the TLVs in I-D.ietf-lsr-flex-algo.

Suggestion>
   o  Shared Risk Link Group Values : one or more 4-octet SRLG values
      derived from the IS-IS and OSPF protocol specific Flexible
      Algorithm Exclude SRLG sub-TLV as defined in
      [I-D.ietf-lsr-flex-algo].

KT> Fixed the text in the same way as for previous TLVs.



424	3.6.  Flex Algo Unknown
...
456	   o  Protocol-ID: Indicates the BGP-LS Protocol-ID of the protocol from
457	      which the FAD is being advertised via BGP-LS.  The values are from
458	      the "BGP-LS Protocol-IDs" registry under the IANA BGP-LS
459	      Parameters registry.

[?] The Protocol-ID is already present in the Node NLRI header.  Why
is a second indication needed?

KT> I agree that it is the same information as in the Node NLRI
header. It was mainly for parsing efficiency in an implementation. The
Protocol Field is carried in the NLRI while these TLVs are in the
BGP-LS attribute.


461	   o  Sub-TLV Types : Zero or more sub-TLV types that are unknown or
462	      unsupported by the node originating the BGP-LS advertisement.  The
463	      size of each sub-TLV type depends on the protocol indicated by the
464	      Protocol-ID field e.g., for ISIS each sub-TLV type would be of
465	      size 1 byte while for OSPF each sub-TLV type would be of size 2
466	      bytes.

[major] The field is called "sub-TLV types" (lower case), please be consistent.

KT> Ack


...
476	   The node originating the advertisement SHOULD include the Flex Algo
477	   Unknown sub-TLV when it comes across an unsupported or unknown sub-
478	   TLV in the corresponding FAD in the IS-IS and OSPF advertisement.

[major] "SHOULD include...unsupported or unknown sub-TLV"

When is it ok to not include them?  As explained in the next
paragraph, the values need to be included.  IOW, why is this action
recommended and not required?

KT> Ack - MUST is better for the inclusion of the Unknown sub-TLV
while including the sub-TLVs themselves can remain a SHOULD.

480	   This serves as an indication that the FAD information in BGP-LS is
481	   incomplete and is not usable for computation purposes.  When
482	   advertising the Flex Algo Unknown sub-TLV, the protocol specific sub-
483	   TLV types that are unsupported or unknown SHOULD be included.  This
484	   information serves as a diagnostic aid.

[minor] The first and last sentences are a good description of the
purpose of the sub-TLV.  Please move them to be right after the
description of the sub-TLV types field.

KT> Ack


[major] The second sentence is a restatement of the recommendation
above.  Please remove it to avoid specifying actions more then once.

KT> Ack


486	4.  Flex Algorithm Prefix Metric
...
508	   o  Flex-Algorithm : 1 octet value in the range between 128 and 255
509	      inclusive which is the range defined for Flexible Algorithms in
510	      the IANA "IGP Parameters" registries under the "IGP Algorithm
511	      Types" registry [I-D.ietf-lsr-flex-algo].

[major] Please simply refer to the definition in
I-D.ietf-lsr-flex-algo.  Note that in this case the definition above
says more than the one in I-D.ietf-lsr-flex-algo: "Flex-Algorithm:
Single octet value between 128 and 255 inclusive."

Suggestion>
   o  Flex-Algorithm :  Single octet value between 128 and 255 inclusive,
      as defined in [I-D.ietf-lsr-flex-algo].

KT> Ack


513	   o  Flags: single octet value and only applicable for OSPF as defined
514	      in [I-D.ietf-lsr-flex-algo].  The value MUST be set to 0 for ISIS
515	      and ignored by the receiver.

[major] No instructions to the consumer: s/MUST be set to 0 for ISIS
and ignored by the receiver./MUST be set to 0 for ISIS.

KT> Ack


...
522	   The FAPM TLV can be added to the BGP-LS Attribute of the Prefix NLRI
523	   originated by a node, only if the corresponding node originates the
524	   Prefix in along with the underlying IGP TLV/sub-TLV as described
525	   below.  This information is derived from the protocol specific
526	   advertisements as below.

528	   o  IS-IS, as defined by the ISIS Flexible Algorithm Prefix Metric
529	      sub-TLV in [I-D.ietf-lsr-flex-algo].

531	   o  OSPFv2/OSPFv3, as defined by the OSPF Flexible Algorithm Prefix
532	      Metric sub-TLV in [I-D.ietf-lsr-flex-algo].

[] See the related comments in §3.

KT> Ack. Fixed.


534	   The BGP-LS Attribute associated with a Prefix NLRI MAY include one or
535	   more FAPM TLVs corresponding to the Flexible Algorithm Prefix Metric
536	   for each algorithm associated with that particular prefix.

[major] "MAY include"

I understand that it is optional to use flex algo in general, which
means that the TLVs defined here don't have to be included if flex
algo is not used by the IGPs.  However, I assume that it is required
to include FAPM TLVs for as many algorithms used by the IGPs, right?
IOW, it is not up to BGP-LS when to advertise this new TLV, it should
do it always (when the IGP includes the corresponding information).

TL;DR: Why is this action optional and not required?  OTOH, you may
simply s/MAY/may

KT> Ack.


538	5.  IANA Considerations

540	   This document requests assigning code-points from the registry "BGP-
541	   LS Node Descriptor, Link Descriptor, Prefix Descriptor, and Attribute
542	   TLVs" <https://www.iana.org/assignments/bgp-ls-parameters/bgp-ls-
543	   parameters.xhtml#node-descriptor-link-descriptor-prefix-descriptor-
544	   attribute-tlv> based on the table below which reflects the values
545	   assigned via the early allocation process.  The column "IS-IS TLV/
546	   Sub-TLV" defined in the registry does not require any value and
547	   should be left empty.

[major] The request should be to make the assignments permanent.  And
to assign the remaining value.

KT> The allocations are actually permanent now per RFC9029. Will fix the text.


549	    +------------+----------------------------------------+----------+
550	    | Code Point |         Description                    | Length   |
551	    +------------+----------------------------------------+----------+
552	    |   1039     | Flex Algorithm Definition TLV          | variable |
553	    |   1040     | Flex Algo Exclude Any Affinity sub-TLV | variable |
554	    |   1041     | Flex Algo Include Any Affinity sub-TLV | variable |
555	    |   1042     | Flex Algo Include All Affinity sub-TLV | variable |
556	    |   1043     | Flex Algo Definition Flags sub-TLV     | variable |
557	    |   1044     | Flex Algorithm Prefix Metric TLV       | variable |
558	    |   1045     | Flex Algorithm Exclude SRLG sub-TLV    | variable |
559	    |   TBD      | Flex Algorithm Unknown sub-TLV         | variable |
560	    +------------+----------------------------------------+----------+

[minor] Please add a Table number and description.

KT> Ack


562	6.  Manageability Considerations

564	   The new protocol extensions introduced in this document augment the
565	   existing IGP topology information that was distributed via [RFC7752].
566	   Procedures and protocol extensions defined in this document do not
567	   affect the BGP protocol operations and management other than as
568	   discussed in the Manageability Considerations section of [RFC7752].
569	   Specifically, the malformed NLRIs attribute tests in the Fault
570	   Management section of [RFC7752] now encompass the new TLVs for the
571	   BGP-LS NLRI in this document.

[nit] s/that was distributed/that can be distributed

KT> Ack


...
578	7.  Security Considerations

580	   The procedures and protocol extensions defined in this document do
581	   not affect the BGP security model.  See the "Security Considerations"
582	   section of [RFC4271] for a discussion of BGP security.  Also, refer
583	   to [RFC4272] and [RFC6952] for analyses of security issues for BGP.
584	   Security considerations for acquiring and distributing BGP-LS
585	   information are discussed in [RFC7752].  The TLVs introduced in this
586	   document are used to propagate the IGP Flexible Algorithm extensions
587	   defined in [I-D.ietf-lsr-flex-algo].  It is assumed that the IGP
588	   instances originating these TLVs will support all the required
589	   security (as described in [I-D.ietf-lsr-flex-algo]) in order to
590	   prevent any security issues when propagating the TLVs into BGP-LS.
591	   The advertisement of the node and prefix attribute information
592	   defined in this document presents no significant additional risk
593	   beyond that associated with the existing node and prefix attribute
594	   information already supported in [RFC7752].

[minor] There's no need to refer to BGP security, as that is covered in rfc7752.

KT> Ack


[nit] Divide this paragraph into a couple of smaller ones.

KT> Ack


[major] I would like to see something about the new functionality
intriduced here -- for example, borrowing heavily from
draft-ietf-idr-bgp-ls-app-specific-attr:

   This document defines a new way to advertise topology-specfic
   descriptions and attributes.  Tampering with the information
   defined in this document may affect applications using it,
   including impacting route calculation and programming, which
   may use characteristics adverted here.  As the advertisements
   defined in this document limit the scope to a specific topology,
   the impact of tampering is similarly limited in scope.

KT> Ack - reworded and updated the text.

Thanks,

Ketan



[EoR -08]