Re: [Lsr] Rtgdir last call review of draft-ietf-isis-te-app-06

["Les Ginsberg (ginsberg)" <[e-mail]>]

Bruno -



Thanx for your detailed review.

V7 of the draft has been posted in response to your comments.



Responses inline.





> -----Original Message-----

> From: Lsr <[e-mail]> On Behalf Of Bruno Decraene via

> Datatracker

> Sent: Monday, September 30, 2019 10:19 AM

> To: [e-mail]

> Cc: [e-mail]; [e-mail]; [e-mail]

> Subject: [Lsr] Rtgdir last call review of draft-ietf-isis-te-app-06

>

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

>

[Les:] You touch on a very fundamental point and motivation for the draft.
It is essential that we have a common understanding on this point.

Existing (legacy) link attribute advertisements are directly tied to the
enablement of RSVP-TE on a link. Every implementation I am aware of
infers RSVP-TE enablement from the legacy advertisements. This is
a significant issue when a new application is introduced as if
RSVP-TE is also enabled in the network then the use of legacy advertisements
will be ambiguous both for RSVP-TE and for the new application.

This is one of the major issues which the draft needs to address. We have
deployments today where operators wish to run both RSVP-TE and SRTE - but
the set of links on which those applications are to be enabled are not
identical. To address this issue REQUIRES a different advertisement/application.
You are correct that today RSVP-TE and SRTE (for example) share the same
legacy advertisements - but only because prior to this draft there was
no alternative. And the consequences of this are that deployments cannot
support incongruence in the set of links used by each application. This
is a problem today and is the primary motivation for the draft.

We have therefore defined an encoding that both addresses the need for
incongruence in the set of links/application, but also supports
application specific values even on links used by multiple applications.
This combination provides a framework which addresses the problems we have
today and guarantees that the same problem will never occur again no
matter what new applications may be introduced in the future.

So we are definitely not OK with saying that new applications can/should
use the legacy advertisements.

As a matter of encoding efficiency, we allow new applications to reference
legacy advertisements when RSVP-TE is enabled on the same link and
the attribute values are the same for both applications. But the key here
is that new applications reference the legacy advertisements by using
the new sub-TLVs with a flag that says "look at legacy".

This point has been presented many times in the WG meetings. Look
at proceedings for IETF 98, 99, 100, 101.


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

>
[Les:] You are correct that deprecation of legacy code points is not necessary.
I have rewritten this section to simply describe an alternative supported
by the draft which allows RSVP-TE to utilize the new advertisements.
Implementations may find this beneficial in that they may eventually be able
to simplify their implementations by eliminating support for the legacy
advertisements. However this is not required and is something that will be
determined by the marketplace. Whether it happens or not does not have
any impact on the normative portion of the draft.


> ============

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

>
[Les:] I have revised the document to use consistent naming for bit mask throughout.
The bit mask definition is presented in Section 4.1 (before the sub-TLVs/TLVs which use it) because it makes sense to me to define something before you use it.
Section 4 however is introducing what is contained in the following sub-sections. It makes sense to mention the common data structure used by the new sub-TLVs/TLVs after mentioning the containers that use it as we have then established a reason for defining a data structure which is shared.

I therefore did not change the order of the presentation (as you suggested). Please reread and let me if it now makes more sense to you.



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



[Les:] SABML+F/UDABML+F  exist because the expanded name does not fit in the ASCII artwork above.
There is no intent to introduce yet another acronym for general use in the document – which is why it does not appear elsewhere in the document.




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

>

[Les:] I added “When set” to the description.



> ---

> "Reserved. Transmitted as 0 and ignored on receipt"

> Using normative key worlds (SHOULD/MUST) would make the behavior

> more normative.

>

[Les:] Done.

> ----

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



[Les:] Yes – any flavor of LFA is included.



"   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



[Les:]  User Defined Attributes was added as a result of WG input. By definition they are NOT subject to standardization. So who defines them is outside the scope of the document.
Section 4.1 states:

“User Defined Application bits have no relationship to Standard
   Application bits and are NOT managed by IANA or any other standards
   body.”




>

> OLD: (UDA Length * 8)

> NEW: (UDA-Length * 8)



[Les:] Done



> ----

> §4.2 "Application Bit Mask (as defined in Section 3.1)"

> :s/Application Bit Mask/Application Identifier Bit Mask

> :s/3.1/4.1

>

[Les:] Done



> ----

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

>

[Les:]  4.1 defines the generic behavior of L-flag.
4.2/4.3 define the behavior specific to the particular sub-TLV/TLV.
I do not see these as duplicates – but rather as complementary.




> "applications specified in the bit mask" is still unclear. Do you mean

> application with the flag set or cleared?



[Les:] I have added “Set to specify” in the bit descriptions.



--- §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"

>



[Les:] Corrected.



> ----

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

>



[Les:] Done



> ---

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



[Les:] AFAIK, implementations advertise  the bandwidth of the L3 link in this sub-TLV. I do not know of any implementation that supports advertising a value other than the actual link bandwidth - which would require a configuration knob to define "RSVP-TE bandwidth".
The intent here is not to change the value associated with  maximum link bandwidth as it is used today. If you believe we have incorrectly interpreted what
is currently advertised and can point to implementations that support what you propose then we would be happy to change this to an application
specific attribute.



--- §4.3

>

> OLD: Application Bit Mask (as defined in Section 3.1)

> NEW: Application Identifier Bit Mask (as defined in Section 4.1)



[Les:] Done



> ---

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



[Les:] :] I would hope that if new attributes are defined that they are only defined for the new style advertisement(s). Therefore I don't think we should make the suggestion you mention.



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



[Les:] Unlike existing SRLG advertisements which (unfortunately) required separate TLVs for IPv4 and IPv6, we are using a single new TLV to cover both address families – which is why we mention the requirement for one of the existing set of defined link identifiers.
I have no idea what addressing schemes might be invented in the future and do not know if under such a new addressing scheme following the existing link identifier model would make sense.
I would like to leave such definitions to the inventors of  IPv8. 😊



--- §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")



[Les:] We aren’t defining behavior in Section 5. The behavior when the bit masks are 0 is defined in Section 4.1.
This section is trying to explain some of the consequences of using zero length bit masks.



---

> §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"

>


[Les:] This section is defining the “enablement” behavior of each defined application. It isn’t defining the encoding.
I don’t see the need to say “bit set” here.




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



[Les:] Again, we are discussing the “enablement behavior” – not the encoding of the advertisements.



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

>

[Les:] This is a good point. We have decided to remove the Flex Algo
related content and have the related application flag and enablement behavior
defined in the flex algo draft. Therefore the reference has been completely
removed. This will result in a one way dependency (flex algo draft on this draft) which we think is more appropriate.


> ---

>

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



[Les:] As stated above, I hope future allocations only need to be done for new style advertisements. If we do need to define new attributes for both legacy and new I would hope we use consistent identifier values – but I think we can leave that to the documents which define the new extensions to address.



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



[Les:] Done.



>

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

[Les:] “y(s)” is defined here:  https://www.iana.org/assignments/isis-tlv-codepoints/isis-tlv-codepoints.xhtml#isis-tlv-codepoints-22-23-25-141-222-223

<snip>
Note

    The column for TLV 25 may be marked as follows:

    y - sub-TLV may appear in TLV 25 but MUST NOT be shared by multiple
        L2 Bundle Members
    y(s) - sub-TLV may appear in TLV 25 and MAY be shared by multiple
        L2 Bundle Members
    n - sub-TLV MUST NOT appear in TLV 25
<end snip>


>

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

[Les:] I hope we can live with the shorter version – but if not I can certainly use the long one. 😊


>

> ============

> 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



[Les:] Done



    Les

>

>

> _______________________________________________

> Lsr mailing list

> [e-mail]<mailto:[e-mail]>

> https://www.ietf.org/mailman/listinfo/lsr