Ben -

I have prepared V3 of the draft to address your comments.
As the window for draft submissions is temporarily closed, I have attached the 
diffs for your review.

I will post the updated version once the window for submissions reopens.

A few more remarks inline.

> -----Original Message-----
> From: Benjamin Kaduk <ka...@mit.edu>
> Sent: Monday, July 13, 2020 4:24 PM
> To: Les Ginsberg (ginsberg) <ginsb...@cisco.com>
> Cc: The IESG <i...@ietf.org>; draft-ietf-lsr-isis-invalid-...@ietf.org; lsr-
> cha...@ietf.org; lsr@ietf.org; Christian Hopps <cho...@chopps.org>;
> aretana.i...@gmail.com
> Subject: Re: Benjamin Kaduk's Yes on draft-ietf-lsr-isis-invalid-tlv-02: (with
> COMMENT)
> 
> 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 <nore...@ietf.org>
> >
> > > Sent: Monday, July 13, 2020 2:11 PM
> >
> > > To: The IESG <i...@ietf.org>
> >
> > > Cc: draft-ietf-lsr-isis-invalid-...@ietf.org; lsr-cha...@ietf.org; 
> > > lsr@ietf.org;
> >
> > > Christian Hopps <cho...@chopps.org>; aretana.i...@gmail.com;
> >
> > > cho...@chopps.org
> >
> > > 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.
> 
[Les:] There is mention in the Abstract that " deployment experience has
   shown..."

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

[Les:] I have reworded this to indicate this is more than just a "corollary".

> >
> >
> >
> >
> > > 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.
>
[Les:] I have used a modified version of your text. Thanx for the suggestion - 
and let me know if the revised wording is to your liking.

     Les
 

> 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

<<< text/html; name="Diff draft-ietf-lsr-isis-invalid-tlv-02.txt - draft-ietf-lsr-isis-invalid-tlv-03.txt.html": Unrecognized >>>
_______________________________________________
Lsr mailing list
Lsr@ietf.org
https://www.ietf.org/mailman/listinfo/lsr

Reply via email to