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

[Alvaro Retana <[e-mail]>]

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


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

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!


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


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


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


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


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


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


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


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


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


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


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?

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

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.


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.


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


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


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.


[major] Same comment as above.


234	   o  Priority : 1 octet value between 0 and 255 inclusive that
235	      specifies the priority of the FAD.

[major] Same comment as above.


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


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.


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


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


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.


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


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


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

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


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.


[?] 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.


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


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


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?


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.


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


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.


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


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


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.


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


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


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.


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.


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


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


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


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

[EoR -08]