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 <[email protected]> > Sent: Monday, July 13, 2020 4:24 PM > To: Les Ginsberg (ginsberg) <[email protected]> > Cc: The IESG <[email protected]>; [email protected]; lsr- > [email protected]; [email protected]; Christian Hopps <[email protected]>; > [email protected] > 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 <[email protected]> > > > > > Sent: Monday, July 13, 2020 2:11 PM > > > > > To: The IESG <[email protected]> > > > > > Cc: [email protected]; [email protected]; > > > [email protected]; > > > > > Christian Hopps <[email protected]>; [email protected]; > > > > > [email protected] > > > > > 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 [email protected] https://www.ietf.org/mailman/listinfo/lsr
