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

From: Bruno Decraene via Datatracker <[email-masked]>
Date: Fri, 29 May 2020 10:17:35 -0700

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-13
Reviewer: Bruno Decraene
Review Date: 2020-05-29
IETF LC End Date: 2020-05-29
Intended Status: Standards Track

Summary:
    I have some minor concerns about this document that I think should be
    resolved before publication.

Comments:
  Draft is clear.

Minor Issues:

§4.1
*2 (for SABM & UDABM fields)
OLD: The length SHOULD be the minimum required to send all bits which are set.
I'd propose
NEW: The length SHOULD be the minimum required to send all the meaningful bits
which are set.

Motivation; the 'bits which are sent' are the bits in the SABM field. (they do
include non-meaningful and padding bits)

----

OLD: Undefined bits MUST be transmitted as 0
NEW: Undefined transmitted bits MUST be cleared (0)

Motivation: currently the number of undefined bits is 8*8-3. They SHOULD not be
transmitted (beyond the first ones fitting in the first N required octet). The
sentence "Undefined bits MUST be transmitted as 0" could be read as all defined
bits MUST be transmitted (as 0).
---
User Defined Application Identifier Bits have no name. I'd propose to call them
UDABM[0], UDABM[1]... This may avoid that different implementation use
different names and, more problematic, that some implementations starting with
1 (the first, the second) while while some other implementations starts as 0,
creating interop issues (SABM[1] on node A is SABM[0] on node B)
---
§4.2

"In cases where conflicting values for the same application/attribute/link are
advertised all the conflicting values MUST be ignored." I'd propose to add "for
this application" (IOW, those values are still applicable for all other
applications)
---
§6.2
I'd argue that the first part of section 3.2 is a specification of the behavior
and hence should be moved to section 4.1, rather than placed in the section
"deployment consideration" which eventually will not be read by someone
implementing the specification. Especially since the text in section 4.1
implies a different behavior: "Bits that are NOT transmitted MUST be treated as
if they are set to 0 on receipt."
---
§5
"In the case of SRTE, advertisement of application specific link attributes
does NOT indicate enablement of SRTE." What does "enablement of SRTE" means? Do
you have a pointer to a document/text?

I'm not sure I would keep that paragraph on SR-TE enablement.
---
§6.1
"Under the conditions defined above, implementations which support the
   extensions defined in this document have the choice of using legacy
   advertisements or application specific advertisements in support of
   SRTE and/or LFA.  This will require implementations to provide
   controls specifying which type of advertisements are to be sent/
   processed on receive for these applications."

I think that "have the choice" is not prescriptive enough given the deployment
issues described in section 6.3 I'd rather say that implementations MUST
support the use of both advertisements (legacy and application specific
advertisement) and MUST provide controls specifying which type of
advertisements are to be processed on receive for these applications.

---

From: "Les Ginsberg (ginsberg)" <[email-masked]>
Date: Fri, 29 May 2020 22:08:33 +0000

Bruno -

Thanx for your (as always) meticulous review.
Responses inline.
Once we have reached agreement I will send out an updated version.

> -----Original Message-----
> From: Bruno Decraene via Datatracker <[email-masked]>
> Sent: Friday, May 29, 2020 10:18 AM
> To: [email-masked]
> Cc: [email-masked]; [email-masked]; [email-masked]
> Subject: Rtgdir last call review of draft-ietf-isis-te-app-13
> 
> Reviewer: Bruno Decraene

[Les:] The definition of what is "meaningful" and what is "padding"  to me is ambiguous.
Meaningful could be only those bits which are currently defined in the registry (speaking of SABM here). But if there are 10 bits defined in the registry and I only intend to set Bit 5, I do not need to send all 10 bits - I only need to send one octet - because we state:

"Bits that are NOT transmitted MUST be treated as if they
   are set to 0 on receipt.  "

Also, an implementation written when there were only 4 bits defined in the registry might think that "meaningful" is different than an implementation written when more than 8 bits were defined in the registry. Yet they can still interoperate.

I believe the current language is best.

> bits MUST be transmitted (as 0).
[Les:] I do not see how that could be a valid interpretation given that we state:

" The length SHOULD  be the minimum required to send all bits which are set."

And (repeating)

"Bits that are NOT transmitted MUST be treated as if they
   are set to 0 on receipt.  "

And again, you assume that "defined bits" is the same for all implementations - which isn't guaranteed as I discussed above.

> creating interop issues (SABM[1] on node A is SABM[0] on node B)

[Les:] What implementations may name bits they assign from the User space is out of scope of this document.
If I were implementing a non-standard User App I likely would give it a meaningful name both in my code and in any documentation I produce.

As far as interoperability, if you want multiple vendors to interoperate then you need a standard application. User defined applications do not provide any guarantee of interoperability.

We do state that 

"It is recommended that [user defined] bits are used starting with Bit 0..."

but as User Defined Applications are outside the scope of the document they might choose to do otherwise.

> this application" (IOW, those values are still applicable for all other

[Les:] How about adding "for the specified application" ?

> if they are set to 0 on receipt."

[Les:] I think you meant to say the "first part of section 6.2"?? Correct?

If so, I agree - and will move that text - though I would prefer to put it into Section 4.2.
Section 4.1 is describing the encoding of the bit mask. Section 4.2 describes the ASLA sub-TLV and how to interpret it.
For example, that is where L-bit is discussed.
Sound good to you?

> I'm not sure I would keep that paragraph on SR-TE enablement.

[Les:] The paragraph is required because we state 

"the relationship between application specific link attribute
   advertisements and enablement for that application"

is required for all new applications.
In this document we are providing that definition for the three existing applications.

The paragraph does state:

"SRTE is implicitly enabled on all links
   which are part of the Segment Routing enabled topology independent of
   the existence of link attribute advertisements."

I will modify the first sentence to say:

"In the case of SRTE, advertisement of application specific link
   attributes does NOT indicate enablement of SRTE  on that link."

("on that link" is added)

Does this work for you?

> advertisements are to be processed on receive for these applications.

[Les:] We know that existing deployments (pre-this draft) use legacy for SRTE/LFA.
In the future, implementations could choose to migrate to using the new ASLA advertisements for SRTE/LFA. Whether they will do so or not is a business decision.
We do not want to declare implementations as non-conformant if they do not migrate.

   Les