Last Call Review of draft-ietf-isis-te-app-06
review-ietf-isis-te-app-06-rtgdir-lc-decraene-2019-09-30-00

 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
​http://trac.tools.ietf.org/area/rtg/trac/wiki/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-isis-te-app-06
Reviewer: Bruno Decraene
Review Date: 2019/09/30
IETF LC End Date: not started
Intended Status: Standards Track

Summary:

    I have significant concerns about this document and recommend that the
    Routing ADs discuss these issues further with the authors

Comments:

I found the document a bit difficult to read. May be just improving the
introduction in section 4 could be enough (in addition to the use of consistent
names across the draft).

Major Issues:

The abstract and "Requirements Discussion" sections make it clear that the goal
of this document is to support the following 2 needs: "In cases
   where multiple applications wish to make use of these link attributes
   the current advertisements do not support application specific values
   for a given attribute nor do they support indication of which
   applications are using the advertised value for a given link.
    This draft introduces new link attribute advertisements which address
   both of these shortcomings."

I think the document should make it clear that any application can use the
existing/legacy advertisements (when there is no need for the attributes to be
different depending on the applications). IOW, existing attributes are NOT
restricted to RSVP-TE. This is currently not clear in the document. And some
sentence could even be understood the other way around. e.g. -  in the
Abstract:"Existing traffic engineering related link attribute advertisements
[...] are used in RSVP-TE deployments." While I think that they are also used
in e.g. SR-TE deployments, LFA deployments - in the Introduction "Use of these
extensions has
   been associated with deployments supporting Traffic Engineering over
   Multiprotocol Label Switching (MPLS) in the presence of Resource
   Reservation Protocol (RSVP) - more succinctly referred to as RSVP-TE."
-  "If the topologies are fully congruent this may not be an issue" (shouldn't
this me :s/may not be/is not   ?) (current sentence reads as FUD) - "There are
existing advertisements used in support of RSVP-TE."

If the problem is only editorial, this is easy to clarify. If not, has this
point been fully discussed/reviewed by the WG?
----
§7.4 Deprecating legacy advertisements

My understanding is that the purpose of this document is to define the new code
points, NOT to deprecate the existing code points. Therefore the purpose of
this section is not clear to me. Also it's not clear what you would mean by
"deprecate". According to RFC 8126, "deprecated" means "use is not
recommended". May be what you had in mind is removal from a given
deployment/AS. That would probably belong to the "operational consideration"
section. Note that as an operator, I not sure to see any benefice for such
change. So again, what is the motivation & goal for this section?

============
Minor Issues:

---
§4 I think the introductory text could be improved to better set the overall
context and help the reader get the picture before jumping into the encoding.
e.g.: - Adding one sentence for each code point to introduce its usage/content
- Adding the sections numbers/reference where they are defined - Describing the
3 items in the same order than the table of content. Indeed, current text stats
with the Application Specific Link attribute Advertisements while the sections
starts with the bit mask (which itself is introduced last in the introduction).
One simple option would be to change the order of the sections.
----
§4
"an application bit mask is defined"
Consistently using the same name would help the reader (especially since there
are 2 bits masks (SABM & UDABM) and "one Application Identifier Bit Mask". In
the next section, it's called "Application Identifier Bit Mask"
---
§4.1
OLD: One bit mask is for standard applications
NEW: One bit mask is for Standard Applications (SA)

(first definition of this acronym)
----
OLD:
"SABML+F"
"UDABML+F"

I'm not sure about the value of defining yet another acronym which is quite
long and never reused. I would propose NEW: SABM Length + Flag UDABM Length +
Flag
---
      "L-flag: Applications listed (both Standard and
       User Defined) MUST use the legacy advertisements
       for the corresponding link found in TLVs 22, 23,
       141, 222, and 223 or TLV 138 or TLV 139 as appropriate."

Behavior is not crystal clear to me. Please define the behavior when the L-flag
is set and when the L-flag is cleared. I would assume that currently/by
default, application use the existing/"future legacy" advertisements. Given
that by default applications flag are defined to the zero, that would probably
call that when the application flag is cleared, the application MUST use the
legacy advertisements; no? Also "Applications listed" is not well defined. I
would assume that this means "the application whose Flag is set". If so, please
say so.

---
"Reserved. Transmitted as 0 and ignored on receipt"
Using normative key worlds (SHOULD/MUST) would make the behavior more normative.

----
"F-bit: Loop Free Alternate"
What do you mean precisely by "Loop Free Alternate"? To me, LFA refers to RFC
5286. But presumably you also mean RLFA, TI-LFA...
----
"   NOTE: If both SA-length and UDA-Length are zero, then the
   attributes associated with this attribute identifier bit mask
   MAY be used by any Standard Application and any User Defined
   Application."

At this point in the draft, I find this note extremely unclear.
I would assume :s/attribute identifier bit mask/Application Identifier Bit Mask
But it's not clear what you mean by "attributes associated"

I would assume that this text could be removed. I much better like the
subsequent sentence "Bits that are NOT
   transmitted MUST be treated as if they are set to 0 on receipt."
---
"User Defined Application Bit Mask"
What is meant exactly by "user"? Is this the user of the Autonomous System or
the user of all possible reader of this bit masks? In other world, if a PCE
/central controller, collect such "User Defined Application Bit Mask" from
multiple Autonomous Systemes, should it assume that the bits are comparable or
not comparable? Both options works for me, but I think a choice and explicit
statement would improve interop on the PCE/controller side.
---
§4.1

OLD: (UDA Length * 8)
NEW: (UDA-Length * 8)
----
§4.2 "Application Bit Mask (as defined in Section 3.1)"
:s/Application Bit Mask/Application Identifier Bit Mask
:s/3.1/4.1

----
§4.2
OLD: When the L-flag is set in the Application Identifiers
NEW: When the L-flag is set in the Application Identifier Bit Mask
---
§4.2

   "When the L-flag is set in the Application Identifiers, all of the
   applications specified in the bit mask MUST use the link attribute
   sub-TLV advertisements listed in Section 3.1 for the corresponding
   link."

I find this specification is much clearer than the one in §4.1 when the L-flag
is defined. I would not specify this behavior twice (both in §4..1 & 4.2)

"applications specified in the bit mask" is still unclear. Do you mean
application with the flag set or cleared?
---
§4.2
"Application specific link attribute sub-sub-TLVs " naming is not consistent
with "Link Attribute sub-sub-TLVs" used a few lines before. (and other with the
title "Application Specific Link Attributes"

----
§4.2
OLD: Multiple sub-TLVs for the same link MAY be advertised.
NEW: Multiple Application Specific Link Attributes sub-TLV MAY be advertised
for the same link.

---
§4.2.1
"Maximum link bandwidth is an application independent attribute of the link."

Well, RFC5305 defines it as
"This sub-TLV contains the maximum bandwidth that can be used on this
   link in this direction (from the system originating the LSP to its
   neighbors).  This is useful for traffic engineering."

https://tools.ietf.org/html/rfc5305#section-3.4

Based on this definition, it's not clear to me why the maximum link bandwidth
used by SR-TE could not be (chosen to be) different than the maximum link
bandwidth used by RSVP-TE. IOW, this is the maximum link bandwidth that can be
used by this application.
---
§4.3

OLD: Application Bit Mask (as defined in Section 3.1)
NEW: Application Identifier Bit Mask (as defined in Section 4.1)
---
§4.3
"The type values are suggested and will be assigned by IANA"
Since this document creates a new registry, it can chose the code points at his
own will.

"but as
       the formats are identical to existing sub-TLVs defined for
       TLVs 22, 23, 141, 222, and 223 the use of the suggested sub-TLV
       types is strongly encouraged."

Is this rule also to the followed for future allocations? If so, if should
probably be indicated, in particulair in the IANA section.
---
§4.3
"   At least one set of link identifiers (IPv4, IPv6, or unnumbered) MUST
   be present.  TLVs which do not meet this requirement MUST be ignored."

What if the IETF latter defines IPv8 and IPv8 only networks are deployed? May
be simply OLD: (IPv4, IPv6, or unnumbered) NEW: (e.g., IPv4, IPv6, or
unnumbered)
---
§5 Deployment consideration
 "If link attributes are advertised associated with zero length
   application bit masks for both standard applications and user defined
   applications, then that set of link attributes MAY be used by any
   application."

- It's not clear if you mean legacy attributes or Link Attribute sub-sub-TLVs
or both. - This Deployment consideration section is not the place to define a
new behavior. - Also section 4.1 specifically defines that "Bits that are NOT
transmitted MUST be treated as if they are set to 0 on receipt.". So it's not
clear if you define a new behavior. (Also "MAY be used by any application"
seems to open different behavior depending on the implementations")
---
§6
"In the case of RSVP-TE, the advertisement of application specific
   link attributes implies that RSVP is enabled on that link."

presumably adding "with the R-bit set"

Also, if you want this new sub-TLV to be used in replacement of the legacy
advertisements, you probably also need to define the negative. e.g. "The
non-adverstisement of both the legacy  advertisements and application specific
link attributes with the R-bit set, implies that RSVP-TE is not enabled in that
link".
---
§6
   "In the case of Flex-Algo, advertisement of application specific link
   attributes does NOT indicate enablement of Flex-Algo.  Rather the
   attributes are used to determine what links are included/excluded in
   the algorithm specific constrained SPF.  This is fully specified in
   [I-D.ietf-lsr-flex-algo]."

The second and third sentences are very close to requiring
[I-D.ietf-lsr-flex-algo] to be a normative reference.

---

§4.2
"IANA is requested to assign the legacy sub-TLV identifer to the corresponding
sub-sub-TLV." Is this rule also to the followed for future allocations? If so,
if should probably be indicated, in particular in the IANA section.
---
§6
"In the presence of an application
   where the advertisement of link attribute advertisements is used to
   infer the enablement of an application on that link (e.g., RSVP-TE),
   the absence of the application identifier leaves ambiguous whether
   that application is enabled on such a link.  This needs to be
   considered when making use of the "any application" encoding."

On my side, this rings the bell that there is an issue with this specification.
I have others comments related to the meaning of the applications flags set and
cleared. I'll see when those points gets clarified.

---
§8 IANA

"   This document defines a new sub-TLV for TLVs 22, 23, 141, 222, and
   223.

    Type  Description             22   23   25  141  222  223
    ----  ---------------------  ---- ---- ---- ---- ---- ----
     16   Application Specific     y    y  y(s)   y    y    y
           Link Attributes
                   "

Sentence does not list TLV 25 while description refers to TLV 25.
Assuming TLV 25 is in scope, the same sentence needs to be updated in sections
4 and 4.2.

I'm not sure what "y(s)" means (compared to "y" )

<ultra pedantic>
Stricto census, the name of the IANA registry is "Sub-TLVs for TLVs 22, 23, 25,
141, 222, and 223 (Extended IS reachability, IS Neighbor Attribute, L2 Bundle
Member Attributes, inter-AS reachability information, MT-ISN, and MT IS
Neighbor Attribute TLVs)" </ultra pedantic>

============
Nits:
OLD:  A sub-sub-TLV is defined for each of the existing sub-TLVs
NEW:  This document defines a sub-sub-TLV for each of the existing sub-TLVs