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

[Benjamin Kaduk <[e-mail]>]

Hi Les,

On Mon, Jul 13, 2020 at 11:05:35PM +0000, Les Ginsberg (ginsberg) wrote:
> Ben -
> 
> 
> 
> Thanx for your review.

My pleasure; this is a nice document.  (A shame it's needed, of course, but
that's neither here nor there.)

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

Thanks for the extra explanation.  I think PS makes sense, and the only
text change I might (emphasis: *might*) consider is to emphasize that the
"occurrence of non-conformant behavior seen in real world deployments" is
decidedly not hypothetical.  But I could understand if the current text is
seen to be saying that already, too.

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

I agree that doing anything else would have been a bad idea!  I was just
suggesting that a different word might be preferred (but am not trying to
press the issue).
> 
> 
> 
> 
> > 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!! 😊

Indeed; it was quite helpful to be able to follow along.

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

That makes sense, though now I wonder if I was misreading the quoted
snippet.  I assumed it was meant to refer to how 5304 is not compatible to
ISO10589, and 6233 is not compatible to 5304, and giving specific guidance
for implementing those two RFCs.  But it also makes sense if it's a
forward-looking thing for any future changes that are
backwards-incompatible.  Perhaps a similarly generic:

% When new protocol behaviors are specified that are not backwards
% compatible, it is RECOMMENDED that implementations provide controls for
% their enablement to allow for incremental implementation deployment and a
% smooth transition.

Anyways, up to you.

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

It probably doesn't matter.  I'm just remembering that something similar
got discussed in the past (IIRC, for an NFSv4 document).

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

Ah, I see :)

> 
> 
> >
> 
> > Section 8.1
> 
> >
> 
> > It's not entirely clear that RFC 7356 is referenced in a normative
> 
> > manner.
> 
> >
> 
> >
> 
> 
> 
> [Les:] I will move it to Informational.

Thanks for the updates,

Ben