Reviewer: Bruno Decraene Review result: Has Issues 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
