Subject: [Lsr] Benjamin Kaduk's No Objection on draft-ietf-isis-reverse-metric-16: (with COMMENT)

From: Benjamin Kaduk <[email-masked]>
Date: Fri, 16 Nov 2018 14:08:10 -0800

Benjamin Kaduk has entered the following ballot position for
draft-ietf-isis-reverse-metric-16: No Objection

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-isis-reverse-metric/

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

Section 1

Perhaps expand IIH on first usage?

Section 1.2

(We could perhaps use "virtual network" instead of "VPN", to avoid
ambiguity about whether its traffic is encrypted.  This is quite tangential
to the actual document, so feel free to ignore.)

Section 1.5

Using an offset implies a need to do bounds checking and/or clamping.
Section 2 discusses this to some extent, though it may be worth an
additional mention here or in the security considerations.

Section 2

   The Reverse Metric TLV is a new TLV to be used inside IS-IS Hello
   PDU.  This TLV is used to support the IS-IS Reverse Metric mechanism
   that allows a "reverse metric" to be send to the IS-IS neighbor.

nits: "inside an IS-IS Hello PDU"; "sent"

Do we need to say "network byte order" for the Metric field?

The W bit seems to allow one node to modify the metric for traffic to
adjacent nodes, which is a slightly broader permission than what is
described in the security considerations and could merit mention therein.
(The existing text there on authentication should suffice even for this
permission, though.)

   U bit (0x02): The "Unreachable" bit specifies that the metric
   calculated by addition of the reverse metric value to the "default
   metric" is limited to (2^24-1).

Does this mean that 2^24-1 is the maximum value (as opposed to the maximum
of 2^24-2 when the bit is not set) or that it is the only allowed value?

   [...] This sub-TLV is optional, if it appears
   more than once then the entire Reverse Metric TLV MUST be ignored.

nit: comma splice

Section 3.2

                                                   When an M-ISIS router
   receives a Reverse Metric TLV, it MUST add the received Metric value
   to its Default Metric in all Extended IS Reachability TLVs for all
   topologies. [...]

(I assume this is still scoped to the link in question.  I don't know
whether there's any need to add more text to clarify that, though.)

Section 3.3

         If a DIS is configured to apply Traffic Engineering over a link
   and it receives metric sub-TLV in a Reverse Metric TLV, it should
   update the Traffic Engineering Default Metric sub-TLV value of the
   corresponding Extended IS Reachability TLV or insert a new one if not
   present.

Is "metric sub-TLV" short for "Traffic Engineering Default Metric sub-TLV"?

   In the case of multi-access LANs, the "W" Flags bit is used to signal
   from a non-DIS to the DIS whether to change the metric and,
   optionally Traffic Engineering parameters for all nodes in the
   Pseudonode LSP or solely the node on the LAN originating the Reverse
   Metric TLV.

nit: comma after "optionally"

When the DIS receives the Reverse Metric TLV with the 'W' bit set, does
that mean it ignores any such TLVs it receives without the 'W' bit set, or
would both the global and router-specific offsets be applied?

Section 3.5

Please expand CSPF on first use.  (Also, nit-level, I am inferring that "a
CSPF recalculation" would be more grammatical, but am not entirely sure.)

Also, SHOULD and RECOMMENDED have the same strength, so from an editorial
perspective the current text could be consolidated.  But perhaps the intent
was to have the 2^24-1 case be a stronger requirement, in which case MUST
could be used?

   It is RECOMMENDED that implementations provide a capability to
   disable any IS-IS metric changes by Reverse Metric mechanism through
   neighbor's Hello PDUs.  It can be to a node's individual interface
   Default Metric or Traffic Engineering parameters based upon receiving
   a properly formatted Reverse Metric TLVs.

I'm failing to parse this last sentence.  The first sentence seems to be
saying that implementations should provide a knob to ignore received
Reverse Metric TLVs, so maybe the second sentence is trying to say what the
behavior would be in the case that the received Reverse Metric TLV is
ignored?  I'm not sure.

Section 4

                                 The enhancement in this document makes
   it possible for one IS-IS router to manipulate the IS-IS Default
   Metric and, optionally, Traffic Engineering parameters of adjacent
   IS-IS neighbors. [...]

Just to check my understanding: this manipulation is for parameters either
for links directed at the IS-IS router sending Reverse Metric, or for links
directed to multicast neighbors on a multi-access LAN (or both)?

Section 5

It's a little surprising to me to see a new registry created with a single
value allocated, the value of which matches the sub-TLV of the same name in
the "Sub-TLVs for TLVs 22, 23, 25, 141, 222, and 223 (Extended IS
reachability, IS Neighbor Attribute, L2 Bundle Member Attributes, inter-AS
reachability information, MT-ISN, and MT IS Neighbor Attribute TLVs)"
registry, but I do not object to doing it this way.

Appendix B

   The risks of misidentifying one side of a point-to-point link or one
   or more interfaces attached to a multi-access LAN and subsequently
   increasing its IS-IS metric and potentially increased latency, jitter
   or packet loss. [...]

nit: I'm not sure this is a complete sentence.  I think "The risks of..."
implies there will be another clause following, and also that "subsequently
increasing" and "potentially increased" have mismatched verb tense, but I
am not entirely certain I am working towards the intended meaning.