Re: [Lsr] Benjamin Kaduk's Yes on draft-ietf-lsr-isis-invalid-tlv-02: (with COMMENT)

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

Ben -



Thanx for your review.

Responses inline.



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

> From: Benjamin Kaduk via Datatracker <[e-mail]>

> Sent: Monday, July 13, 2020 2:11 PM

> To: The IESG <[e-mail]>

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

> Christian Hopps <[e-mail]>; [e-mail];

> [e-mail]

> Subject: Benjamin Kaduk's Yes on draft-ietf-lsr-isis-invalid-tlv-02: (with

> COMMENT)

>

> Benjamin Kaduk has entered the following ballot position for

> draft-ietf-lsr-isis-invalid-tlv-02: Yes

>

> When responding, please keep the subject line intact and reply to all

> email addresses included in the To and CC lines. (Feel free to cut this

> introductory paragraph, however.)

>

>

> Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html

> for more information about IESG DISCUSS and COMMENT positions.

>

>

> The document, along with other ballot positions, can be found here:

> https://datatracker.ietf.org/doc/draft-ietf-lsr-isis-invalid-tlv/

>

>

>

> ----------------------------------------------------------------------

> COMMENT:

> ----------------------------------------------------------------------

>

> The shepherd writeup is a bit unclear as to why Proposed Standard is the

> right document status (vs., e.g., Informational).  I guess it's desired

> to have the Updates: relationship to the indicated documents, which

> essentially forces it to be standards-track.  On the other hand, perhaps

> the sense that ignoring a TLV that is specifically disallowed (as

> opposed to "merely" unrecognized) is itself a newly specified

> requirement, in which case the status as Proposed Standard makes sense

> for that reason.  It might be worth a little more clarity on which (if

> either) of these scenarios are intended.

>

[Les:] What prompted the writing of this document was a real world interoperability scenario wherein one implementation generated a malformed TLV and a receiving implementation rejected the entire PDU because of it. This resulted in persistent LSPDB inconsistency in the network for a prolonged period with a significant impact on the proper functioning of the network. This failure was considered significant enough that Standards Track seemed appropriate.



As the document developed, the authors were encouraged to address some other issues, one of which was clarifying disallowed TLV handling as well.



I can understand why Informational track may seem appropriate to you. In early discussions with Alvaro I had suggested that there was no need to write the document - that existing specifications were sufficiently clear. But the fact that - despite existing standards - such an interoperability issue did occur was compelling. The WG also embraced this as useful.



> Section 1

>

>    A corollary to ignoring unknown TLVs is having the validation of PDUs

>    be independent from the validation of the TLVs contained in the PDU.

>

> nit: this doesn't exactly seem like a "corollary" specifically, but

> rather a choice that [ISO10589] made (and which keeps some overall sense

> of consistency between PDU and TLV handling).

>

[Les:] Rejecting a PDU because of some issue with a single TLV would mean that the full set of updates contained in that LSP would not be propagated. This has a significant impact on the correct operation of the protocol. So I think this really isn’t an option.





> Section 3.1

>

>    [ISO10589] defines the behavior required when a PDU is received

>    containing a TLV which is "not recognised".  It states (see Sections

>    9.3 - 9.13):

>

> This is Sections 9.5 (not 9.3) to 9.13 in the copy I have.

>

[Les:] Well spotted. I see you have put your newly acquired  copy of ISO 10589 to good use. Bravo!! 😊



> Section 3.2

>

>    Similarly, the extensions defined by [RFC6233] are not compatible

>    with the behavior defined in [RFC5304], therefore can only be safely

>    enabled when all nodes support the extensions.

>

> nit: I'd argue that technically the extensions are *defined* by 6232, even

> though 6233 is what makes their nature (as "allowed in purge") easily

> discoverable.

>

[Les:] Fair enough. I will change this to RFC6232 - which is one of documents updated by this draft.



>    It is RECOMMENDED that implementations provide controls for the

>    enablement of behaviors that are not backward compatible.

>

> We also specifically want the ability to generate but not

> process/require at least some of them, right?  Is that worth calling out

> in addition to just "controls for the enablement"?



[Les:] This sentence is serving as a guideline for new behaviors that have backwards compatibility issues. New information which could be safely sent in the presence of legacy routers does not fall into this category.



>

> Section 3.3

>

>    a given sub-TLV is allowed.  Section 2 of [RFC5305] is updated by the

>    following sentence:

>

>       "As with TLVs, it is required that sub-TLVs which

>        are disallowed MUST be ignored on receipt.".

>

> Do we want to say where this logical insertion occurs?

>

[Les:] As this is not modifying existing text in any way, I am not sure that it is necessary to do so.

I can envision adding this to the end of the first paragraph or creating a new paragraph.



Unless we are actually going to create a BIS draft, I am not sure that it matters.

??



> Section 3.4

>

>    The correct setting for "LSP" is "n".  This document updates

>    [RFC6232] by correcting that error.

>

> It's slightly interesting that there doesn't seem to be a corresponding

> Errata Report (but may not be worth doing anything about given that this

> update is about to be approved).



[Les:] The error was found during the writing of this draft.



>

> Section 8.1

>

> It's not entirely clear that RFC 7356 is referenced in a normative

> manner.

>

>



[Les:] I will move it to Informational.



   Les