Subject: [Gen-art] IETF Last Call Gen-Art review of draft-ietf-isis-rfc4971bis-01

From: [email-masked] (Dale R. Worley)
Date: Thu, 04 Aug 2016 16:00:09 -0400

I am the assigned Gen-ART reviewer for this draft. The General Area
Review Team (Gen-ART) reviews all IETF documents being processed
by the IESG for the IETF Chair.  Please treat these comments just
like any other last call comments.

For more information, please see the FAQ at
<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Document: draft-ietf-isis-rfc4971bis-01
Reviewer: Dale R. Worley
Review Date: 4 August 2016
IETF LC End Date: 15 August 2016
IESG Telechat date: 18 August 2016

Summary:

This draft is basically ready for publication, but has nits that
should be fixed before publication.

* Editorial items

How is "capability TLV" to be capitalized?  I count the following
occurrences of the different capitalizations:

    24 CAPABILITY(IES) TLV(s)
     5 Capability(ies) TLV(s)
     4 capability(ies) TLV(s)

The practice in other RFCs seems to be "Capability TLV".

This also affects two occurrences of "TLV named CAPABILITY".

Abstract

   This document defines a new optional Intermediate System to
   Intermediate System (IS-IS) TLV named CAPABILITY, formed of multiple
   sub-TLVs, which allows a router to announce its capabilities within
   an IS-IS level or the entire routing domain.

I think s/formed of/containing/ -- the TLV also contains the Router ID
and Flags fields.

2.  IS-IS Router CAPABILITY TLV

   The IS-IS Router CAPABILITY TLV is composed of 1 octet for the type,
   1 octet that specifies the number of bytes in the value field, and a
   variable length value field that starts with 4 octets of Router ID,
   indicating the source of the TLV, and followed by 1 octet of flags.

Should some note be put here that if Capability is used as an extended
TLV, the type and length are increased to 2 bytes?  That is a general
fact about TLVs, of course, but strictly, the above sentence isn't
always correct.  (Or is the "extended" version available only for TLVs
at some higher level of the hierarchy?)

I think s/and followed by/followed by/.  The subject of "followed" is
"4 octets of Router ID", leaving "and" to join "indicating the source
of the TLV" with "followed by 1 octet of flags", which is not a
parallel construction.  Instead, omit "and" to make the construction
"... value field that starts with 4 octets ... followed by ..."

3.  Elements of Procedure

   Router ID sub-TLV [RFC5316] MUST be present in the TLV.  Router
   CAPABILITY TLVs which have a Router ID of 0.0.0.0 and do NOT have the
   IPv6 TE Router ID sub-TLV present MUST be ignored.

Given that "ignored" is used later to describe processing of unknown
sub-TLVs, which must not be interpreted but which may need to be
leaked, perhaps "ignored" here should be amplified.  Perhaps "ignored,
including not leaked".

      Example: If Level-1 router A generates a Capability TLV and floods
      it to two L1/L2 routers, S and T, they will flood it into the
      Level-2 domain.  Now suppose the Level-1 area partitions, such
      that A and S are in one partition and T is in another.  IP routing
      will still continue to work, but if A now issues a revised version
      of the CAP TLV, or decides to stop advertising it, S will follow
      suit, but T will continue to advertise the old version until the
      LSP times out.

I think you need to expand the last sentence to "... but without the
above prohibition, T would continue ...".  Or otherwise qualify the
entire example with "without the above prohibition".

   Routers in other areas have to choose whether to trust T's copy of
   A's capabilities or S's copy of A's information and, they have no
   reliable way to choose.  By making sure that T stops leaking A's
   information, this removes the possibility that other routers will use
   stale information from A.

This second paragraph is part of the example, and should be indented
the same way as the first paragraph.

Similarly, in the first sentence, s/have to/would have to/.

Remove the comma after "and".

The "this" in the last sentence doesn't have an antecedent.  Change to
"this prohibition".

   How partial support may impact the operation of the
   capabilities advertised within the Router CAPABILITY TLV is outside
   the scope of this document.

Is it worth specifying that the documents that define the sub-TLVs
are responsible for considering the impact of partial support?

   If leaking of the CAPABILITY TLV is required, the entire CAPABILITY
   TLV MUST be leaked into another level even though it may contain some
   of the unsupported sub-TLVs.

I think the final phrase would be better as "... even though it may
contain sub-TLVs that are not supported by the router leaking the
TLV."

6.  IANA Considerations

   IANA assigned a new IS-IS TLV code-point [...]

The usual form is "codepoint".  But perhaps that question is better
left to the RFC Editor.

7.  Acknowledgements

   For the original version of RFC 4971 the authors thanked Jean-Louis
   Le Roux, Paul Mabey, Andrew Partan, and Adrian Farrel for their
   useful comments.

"the original version of RFC 4971" is incorrect.  There are a number
of ways to fix this, but I suggest "the original version of this
document, RFC 4971, ...".

   For this new version the authors would like to thank Kris Michielsen
   for calling the problem associated w an IPv6 only router to our
   attention.

Change "w an IPv6 only" to "with an IPv6-only".

It would be easier to read as "... for calling to our attention the
problem associated ...".

Dale

---

From: "Les Ginsberg (ginsberg)" <[email-masked]>
Date: Mon, 8 Aug 2016 13:34:03 +0000

Dale -

Thanx for your detailed review. I have elected to copy the WG on my reply as you also sent a copy of your review to the WG.

Overall, while I either agree with or have no objection to many of your comments, most of them could be applied to RFC 4971 itself. When generating the bis version we elected NOT to modify the original RFC except where it was necessary to address the issue of an IPv6-only router. It certainly can be argued that your suggestions improve the consistency and readability of the document, but they also make it deviate from RFC 4971 where no intent to define a functional change is intended. It therefore has to be considered whether making many of the changes you suggest might unintentionally suggest a substantive change where none is intended.

Be interested in your thoughts on this.

After we reach agreement on the above point I will spin a new version addressing your comments.

Specific responses inline.

> -----Original Message-----
> From: Dale R. Worley [mailto:[email-masked]]
> Sent: Thursday, August 04, 2016 1:00 PM
> To: [email-masked]; [email-masked]; [email-masked]
> Subject: IETF Last Call Gen-Art review of draft-ietf-isis-rfc4971bis-01
> 
> I am the assigned Gen-ART reviewer for this draft. The General Area Review
> Team (Gen-ART) reviews all IETF documents being processed by the IESG for
> the IETF Chair.  Please treat these comments just like any other last call
> comments.
> 
> This draft is basically ready for publication, but has nits that should be fixed
> before publication.
> 
> * Editorial items
> 
> How is "capability TLV" to be capitalized?  I count the following occurrences of
> the different capitalizations:
> 

[Les:] As "CAPABILITY" was the dominant form in RFC 4971 I would be inclined to use that. Also this is the form used in http://www.iana.org/assignments/isis-tlv-codepoints/isis-tlv-codepoints.xhtml

   " IS-IS Router CAPABILITY TLV"

But I agree consistency is desirable no matter what form we choose.

> I think s/formed of/containing/ -- the TLV also contains the Router ID and
> Flags fields.
> 

[Les:] This text is unchanged from RFC 4971.

> Should some note be put here that if Capability is used as an extended TLV,
> the type and length are increased to 2 bytes?  That is a general fact about
> TLVs, of course, but strictly, the above sentence isn't always correct.  (Or is
> the "extended" version available only for TLVs at some higher level of the
> hierarchy?)
>

[Les:] You refer here to the extended TLVs defined in RFC 7356  (pretty good find for someone who is not supposed to be an IS-IS expert :-) ).
I am reluctant to require IS-IS RFCs in general to have to define both formats. That seems unnecessary. I am even more reluctant to do so in a bis version of an existing RFC.

> "4 octets of Router ID", leaving "and" to join "indicating the source of the
> TLV" with "followed by 1 octet of flags", which is not a parallel construction.
> Instead, omit "and" to make the construction "... value field that starts with 4
> octets ... followed by ..."
> 

[Les:] No objection - though again this is the original text from RFC 4971 which the RFC editor seemed happy with at the time.

> Given that "ignored" is used later to describe processing of unknown sub-
> TLVs, which must not be interpreted but which may need to be leaked,
> perhaps "ignored" here should be amplified.  Perhaps "ignored, including not
> leaked".

[Les:] Later in the same section where the draft discusses not using the TLV the draft says 

" Note that leaking a Capabilities
   TLV is one of the uses that is prohibited under these conditions."

We could repeat similar text above, but I would rather revise the text to indicate whatever the reason for not using the TLV (unusable router-id or stale TLV) that leaking is prohibited so this is stated in one place.
I will come up w revised text.

> I think you need to expand the last sentence to "... but without the above
> prohibition, T would continue ...".  Or otherwise qualify the entire example
> with "without the above prohibition".
> 

[Les:] Agree that something like your suggestion would improve the text, but again this is original RFC 4971 text.

> This second paragraph is part of the example, and should be indented the
> same way as the first paragraph.
>

[Les:] Agree.

> The "this" in the last sentence doesn't have an antecedent.  Change to "this
> prohibition".
> 

[Les:] Again, editorial changes to original RFC 4971 text to which I have no objection.

> Is it worth specifying that the documents that define the sub-TLVs are
> responsible for considering the impact of partial support?
> 

[Les:] I am fine with the current text. Adding what you suggest might make some existing RFCs which define sub-TLVs non-compliant. I am reluctant to do that. If the review of those RFCs did not feel the need to comment on the lack of discussion of this point I would rather not introduce a compliance issue after the fact. It is not that your suggestion is poor - just that since it was not stated in RFC 4971 it seems problematical to introduce it in the bis version.

> I think the final phrase would be better as "... even though it may contain
> sub-TLVs that are not supported by the router leaking the TLV."
> 

[Les:] No objection.

> The usual form is "codepoint".  But perhaps that question is better left to the
> RFC Editor.
> 

[Les:] This text was added by IANA originally -not by the authors of RFC 4971 - so I will leave it to IANA/RFC editor to deal with this.

> "the original version of RFC 4971" is incorrect.  There are a number of ways to
> fix this, but I suggest "the original version of this document, RFC 4971, ...".
>

[Les:] Agree
 
> It would be easier to read as "... for calling to our attention the problem
> associated ...".
> 

[Les:] Agree

    Les

---

From: [email-masked] (Dale R. Worley)
Date: Mon, 08 Aug 2016 11:25:39 -0400

"Les Ginsberg (ginsberg)" <[email-masked]> writes:
> Thanx for your detailed review. I have elected to copy the WG on my
> reply as you also sent a copy of your review to the WG.

I'm not sure if it is formally specified, but it seems to me that a
Gen-Art review really should be copied to the WG.

> It therefore has to be considered whether making many of the
> changes you suggest might unintentionally suggest a substantive change
> where none is intended.

Of course, my comments are only a review.  Looking over them again, none
seem to technically critical; the ones with technical content are
improving the explanations of features that people (seem to be)
implementing correctly now.  So I don't see any reason to object to
minimizing changes from RFC 4971.

> [Les:] You refer here to the extended TLVs defined in RFC 7356
> (pretty good find for someone who is not supposed to be an IS-IS
> expert :-) ).

I looked at the type codepoint registry, and there were values over 255
(though unassigned), which was inconsistent with the text of
draft-ietf-isis-rfc4971bis.  So it was just a matter of tracking down
what defined the alternative format.

Dale

---

From: "Les Ginsberg (ginsberg)" <[email-masked]>
Date: Mon, 8 Aug 2016 22:15:48 +0000

Dale -

Attached please find an updated version and diffs from previous.
Please let me know if this adequately addresses your comments.

    Les

> Sent: Monday, August 08, 2016 8:26 AM
> To: Les Ginsberg (ginsberg)
> Cc: [email-masked]; [email-masked]; [email-masked]; isis-
> [email-masked]
> Subject: Re: IETF Last Call Gen-Art review of draft-ietf-isis-rfc4971bis-01
> 
> I'm not sure if it is formally specified, but it seems to me that a Gen-Art
> review really should be copied to the WG.
> 
> > It therefore has to be considered whether making many of the changes
> > you suggest might unintentionally suggest a substantive change where
> > none is intended.
> 
> seem to technically critical; the ones with technical content are improving the
> explanations of features that people (seem to be) implementing correctly
> now.  So I don't see any reason to object to minimizing changes from RFC
> 4971.
> 
> > [Les:] You refer here to the extended TLVs defined in RFC 7356 (pretty
> > good find for someone who is not supposed to be an IS-IS expert :-) ).
> 
> (though unassigned), which was inconsistent with the text of draft-ietf-isis-
> rfc4971bis.  So it was just a matter of tracking down what defined the
> alternative format.
> 
> Dale

---

From: [email-masked] (Dale R. Worley)
Date: Wed, 10 Aug 2016 15:46:20 -0400

> Please let me know if this adequately addresses your comments.

It looks good to me.  (nit:  Section 4 ends with two successive
periods.)

Dale

---

From: Jari Arkko <[email-masked]>
Date: Wed, 17 Aug 2016 22:10:52 +0200

Dale, Les, thanks for the reviews and fixes. Much appreciated. I have balloted no-obj for this doc for tomorrow’s IESG telechat.

Jari