Re: [Lsr] AD Review of draft-ietf-isis-reverse-metric-11

["Naiming Shen (naiming)" <[e-mail]>]

Hi Alvaro,

Sorry for the late response.

Thank you for your detailed review on this draft, I’ll try to reply to the comments
inline below, and the proposed document diff is attached at the end. Let me know
if you would like to have other formats.

Many thanks.
- Naiming

On Aug 31, 2018, at 12:27 PM, Alvaro Retana <[e-mail]<mailto:[e-mail]>> wrote:

Dear authors:

I just finished reading this document.  Thanks for the work on it!

I have several concerns (please see detailed comments below), and think the
draft still needs work.  Among other things, my main concerns include:

- the lack of an explicit definition of the reverse metric; examples and
explanations illustrate, but there is no single overview definition that
clearly (and early) talks about the offset...

- the new TLV is underspecified and there are error conditions that should
be addressed


Note that I read -11, but -12 was published in the interim -- so I'm
putting this comment up here.  The only change in -12 is the addition (in
the IANA Considerations section) of "a new registry for sub-TLVs of the
Reverse Metric TLV".  Why do we need this new registry?  The description
(in §2) of the use of this sub-TLV already references rfc5305 (where a TE
Default Metric sub-TLV is already defined), so it seemed somewhat natural
to me to simply reuse that sub-TLV here.  From the discussion on the list,
I understand the intent to "future proof", even if no other applications
come to mind.  If that is the path we want to go, then we'll need a
complete description of the new sub-TLV as well. [Some of my comments
bellow assume the existing sub-TLV from rfc5305.]

<NS> With the email exchange from you and Les, I assume I keep the same
as in the version 12 on this sub-TLV registry.


Thanks!

Alvaro.


[Line numbers came from the idnits output.]

...
10                   IS-IS Routing with Reverse Metric
11                   draft-ietf-isis-reverse-metric-11


...
169 2.  IS-IS Reverse Metric TLV

[minor] This section would read better if you first introduced the TLV
(what is it for...), presented the figure and finally described the
fields.  For example, as written, the Flags field is first mentioned, then
shown in the figure, then the length is mentioned again under it, then a
new figure shows the Flags, and finally (after first talking about the
Metric) the specific flags are defined.  It is disjoint and feels messy.

<NS> Good point, will re-organize that.


171   The Reverse Metric TLV is composed of a 1 octet field of Flags, a 3

[nit] Strictly speaking, the TLV is composed of a Type, Length, etc..

<NS> Ok. Use the “The Value part of the … TLV”.


172   octet field containing an IS-IS Metric Value, and a 1 octet Traffic
173   Engineering (TE) sub-TLV length field representing the length of a

[minor] Even though it is the only one used, the sub-TLV length field is
not the "Traffic Engineering (TE) sub-TLV length field”.

<NS> Ok, remove the TE sub-TLV wording.


174   variable number of Extended Intermediate System (IS) Reachability
175   sub-TLVs.  If the "sub-TLV len" is non-zero, then the Value field
176   MUST also contain one or more Extended IS Reachability sub-TLVs.

[minor] I'm guessing that by "Extended IS Reachability sub-TLVs" you really
mean the sub-TLVs for the Extended IS Reachability TLV, right?  Please at
least put in a reference to rfc5305.

<NS> Will reference to the rfc5305.


178   The Reverse Metric TLV is optional.  The Reverse Metric TLV may be
179   present in any IS-IS Hello PDU.  A sender MUST only transmit a single
180   Reverse Metric TLV in a IS-IS Hello PDU.  If a received IS-IS Hello
181   PDU contains more than one Reverse Metric TLV, an implementation
182   SHOULD ignore all the Reverse Metric TLVs and treat it as an error
183   condition.

[nit] The first two sentences sound redundant to me.

<NS> OK.


[major] The text above is not specific about which PDUs can include the
Reverse Metric TLV.  The text does say that it is optional and that it may
be in an IIH once...but it doesn't say anything about other PDUs.  The IANA
Considerations section contains the attributes to be included in the
registry, but those are not Normative.

<NS> Added other PDU MUST ignore it.


[major] What should happen if this TLV is included in a different PDU?


<NS> MUST be ignored, added in the text.

[major] Under what conditions may the receiver *not* ignore all the Reverse
Metric TLVs?  If not ignoring them, which one should it use?  IOW, why is
it a "SHOULD" and not a "MUST”?

<NS> Changed to MUST from SHOULD.


[major] What should the receiver do in response to the "error condition"?
Should it just ignore the TLVs?  Should it ignore the whole IIH?  Should it
restart the adjacency?  Nothing?  Something else?  Maybe log the error…

<NS> It says to ignore the TLV. “error condition” is a operational management
terminology. removed the “error condition” wording. it’s up to the
implementation to log or put in debug messages, etc.


185       0                   1                   2                   3
186       0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
187       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
188       |      Type     |     Length    |    Flags      |     Metric
189       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
190             Metric  (Continue)        | sub-TLV Len   |Optional sub-TLV
191       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

193                            Reverse Metric TLV

[nit] Please add a Figure number.

<NS> Ok.


195      TYPE: TBD (be replaced by the value that IANA allocates)
196      LENGTH: variable (5 - 255 octets)
197      VALUE:

199         Flags (1 octet)
200         Metric (3 octets)
201         sub-TLV length (1 octet)
202         sub-TLV data (0 - 250 octets)

204          0 1 2 3 4 5 6 7
205         +-+-+-+-+-+-+-+-+
206         |  Reserved |U|W|
207         +-+-+-+-+-+-+-+-+

[major] Can the other Flag bits be assigned in the future?  If so, then
they should be marked as Unassigned and a registry should be created.

<NS> I saw Chris’s email on this issue, I assume we have done this way often.


209                              Figure 1: Flags

211   The Metric field contains a 24-bit unsigned integer.  This value is a
212   metric offset that a neighbor SHOULD add to the existing, configured
213   "default metric" for the IS-IS link.  Refer to "Elements of
214   Procedure", in Section 3 for details on how an IS-IS router should
215   process the Metric field in a Reverse Metric TLV.

[nit] I think that the term "default metric" may cause confusion to the
casual reader, specially because it sometimes used inside quotation marks.
I know this comes from 10589.  It may be a good idea to either include a
reference to 10589 when it is first mentioned, or to distinguish it somehow
(Default Metric -- with caps), or both.  At minimum, please be consistent
about the use of quotations marks.


<NS> Good point. I agree and will normalize them to be consistent.


...
240   The Reverse Metric TLV can include sub-TLVs when an IS-IS router
241   wishes to signal to its neighbor to raise its Traffic Engineering
242   (TE) Metric over the link.  In this document, only the "Traffic
243   Engineering Default Metric" sub-TLV [RFC5305], sub-TLV Type 18, is
244   defined and MAY be included in the Reverse Metric TLV, because that
245   is a similar 'reverse metric' operation to be used in TE

[nit] The "Traffic Engineering Default Metric" sub-TLV is not defined in
this document.  I think you mean that this document only defines the use of
that sub-TLV with the Reverse Metric TLV. See my suggestion in the next
item.

[minor] The Normative language feels a little clunky to me...I think you
may want to change it's location: (suggestion) s/The Reverse Metric TLV can
include sub-TLVs...sub-TLV Type 18, is defined and MAY be included in the
Reverse Metric TLV.../The Reverse Metric TLV MAY include sub-TLVs...sub-TLV
Type 18, is defined to be included in the Reverse Metric TLV…

<NS> Ok, will change to make this clear.


246   computations.  Upon receiving this TE METRIC sub-TLV in a Reverse
247   Metric TLV, a node SHOULD add the received TE metric offset value to
248   its existing, configured TE default metric within its Extended IS
249   Reachability TLV.  Use of other sub-TLVs is outside the scope of this
250   document.  The "sub-TLV Len" value MUST be set to zero when an IS-IS
251   router does not have TE sub-TLVs that it wishes to send to its IS-IS
252   neighbor.

[minor] Please be consistent with the names used.  For example, the Traffic
Engineering Default Metric sub-TLV (from rfc5305) is mentioned by that
name, but also by "TE METRIC sub-TLV" and "TE Default Metric sub-TLV".
This last one may be close, but given that rfc5305 doesn't even use that
name, at least expanding TE would help.

<NS> Ok, will use the consistent longer version.


254 3.  Elements of Procedure

256 3.1.  Processing Changes to Default Metric

258   The Metric field, in the Reverse Metric TLV, is a "reverse offset
259   metric" that will either be in the range of 0 - 63 when a "narrow"
260   IS-IS metric is used (IS Neighbors TLV, Pseudonode LSP) [RFC1195] or
261   in the range of 0 - (2^24 - 2) when a "wide" Traffic Engineering
262   metric value is used, (Extended IS Reachability TLV) [RFC5305]

[minor] It seems to me like this description might fit better where there
field is being described, in §2.

<NS> Ok, will move over.


263   [RFC5817].  It is important to use the same IS-IS metric mode on both
264   ends of the link.  On the receiving side of the 'reverse-metric' TLV,

[minor] Why is that reference to rfc5817 there?  It seems superfluous to me

<NS> I think the RFC5305 defines the operation of 2^^24-1 for “Default Metric”,
but it didn’t say anything on the TE default metric of that value. RFC5817
has the value specified.

[nit] I'm sure you mean the "same metric type" (not just the same metric) on both sides of the link...

[major] What happens if the metric type is not the same on both ends?  Should there be some Normative language there?

<NS> It’s an operational error in the first place, it’s not just reverse metric will be wrong.
I’m adding “, and in the entire IS-IS area or level”.


265       the accumulated value of configured metric and the reverse-metric
266       needs to be limited to 63 in "narrow" metric mode and to (2^24 - 2)
267       in "wide" metric mode.  This applies to both the default metric of
268       Extended IS Reachability TLV and the TE default-metric sub-TLV in LSP

[major] So...when offsetting the Default Metric the local router shouldn't go above 63 when using narrow metics.  Assuming narrow metrics...  Would receiving a reverse metric > 63 be an indication what the sender may not be using narrow metrics, or that at least something went wrong?  Should anything be done about that?

<NS> I think we don’t support this mix type mode, not for IS-IS in general.
But your example is only happens in one case of mixed model. I don’t
think inside reverse metric mechanism, we will try to define the ‘detection’ of that.


269       or Pseudonode LSP for the "wide" metric mode case.  If the "U" bit is
270       present in the flags, the accumulated metric value is to be limited
271       to (2^24 - 1) for both the normal link metric and TE metric in IS-IS
272       "wide" metric mode.


...
297     3.3.  Multi-Access LAN Procedures

299       On a Multi-Access LAN, only the DIS SHOULD act upon information
300       contained in a received Reverse Metric TLV.  All non-DIS nodes MUST
301       silently ignore a received Reverse Metric TLV.  The decision process
302       of the routers on the LAN MUST follow the procedure in section
303       7.2.8.2 of [ISO10589], and use the "Two-way connectivity check"
304       during the topology and route calculation.

[minor] This last sentence (about 10589), while not wrong, seems unnecessary.  There's nothing special about the processing of the reverse metric in LANs that makes calling out the 2-way connectivity check necessary.  Is there?  Is there a reason it is not called out for p2p links?  Should a general statement about only applying the metric after the 2-way check is performed be put somewhere?  Am I missing something?

<NS> Originally, there is no such a mention of that. Due to the mailing list had
a comment a while ago that on the LAN, only a subset of nodes supporting
this ‘reverse-metric’. The comments saying the result of SPF would be
inconsistent. And Les and I pointed out that, the “Two-way connectivity check”
had to be followed, and it would be consistent. I’m happy to remove to that
reference of 10589 here if we think it’s ok.


306       The Reverse Metric TE sub-TLV also applies to the DIS.  If a DIS is
307       configured to apply TE over a link and it receives TE metric sub-TLV
308       in a Reverse Metric TLV, it should update the TE Default Metric sub-
309       TLV value of the corresponding Extended IS Reachability TLV or insert
310       a new one if not present.

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

318       A non-DIS node, e.g., Router B, attached to a multi-access LAN will
319       send the DIS a Reverse Metric TLV with the W bit clear when Router B
320       wishes the DIS to add the Metric value to the default metric
321       contained in the Pseudonode LSP specific to just Router B.  Other
322       non-DIS nodes, e.g., Routers C and D, may simultaneously send a
323       Reverse Metric TLV with the W bit clear to request the DIS to add
324       their own Metric value to their default metric contained in the
325       Pseudonode LSP.  When the DIS receives a properly formatted Reverse
326       Metric TLV with the W bit clear, the DIS MUST only add the default
327       metric contained in its Pseudonode LSP for the specific neighbor that
328       sent the corresponding Reverse Metric TLV.

330       As long as at least one IS-IS node on the LAN sending the signal to
331       DIS with the W bit set, the DIS would add the metric value in the
332       Reverse Metric TLV to all neighbor adjacencies in the Pseudonode LSP,
333       regardless if some of the nodes on the LAN advertise the Reverse
334       Metric TLV without the W bit set.  The DIS MUST use the reverse

[major] This first sentence clearly says that the metric from a node with W set is used in all adjacencies, regardless of what other nodes (not setting the W bit) might say...which contradicts the paragraph right before it.

<NS> if you refer to the line 325 sentence above, then it says with the W bit “clear”.
since the whole paragraph is about no one is setting the W bit. but the last
sentence of that paragraph does not need to be there.


335       metric of the highest source MAC address Non-DIS advertising the
336       Reverse Metric TLV with the W bit set.  The DIS MUST use the metric
337       value towards the nodes which explicitly advertise the Reverse Metric
338       TLV.

[major] I don't understand what the last sentence is specifying.  All the nodes using the new TLV are explicitly advertising it…

<NS> Will remove this sentence, not sure why it was there.


340       Local provisioning on the DIS to adjust the default metric(s)
341       contained in the Pseudonode LSP MUST take precedence over received
342       Reverse Metric TLVs.  For instance, local policy on the DIS may be
343       provisioned to ignore the W bit signaling on a LAN.

[major] Should an operator be able to configure an adjusted metric to be used only if the Reverse Metric TLV is not received?  In other words, that case wouldn't be able to conform to the MUST.

<NS> Will add the ‘non-dynamic’ wording into that.


[major] The MUST above is in conflict with the text in §3.6 which says that "it is RECOMMENDED that implementations provide a capability to disable any changes".  (1) RECOMMENDED is the same as SHOULD, (2) disabling the change is equivalent to the local configuration taking precedence.

<NS> Ok, for Pnode, it’s a good idea to statically configure a ‘reverse-metric’; but
in general, a node can benefit from configuration of “ignoring” any ‘reverse-metric’
dynamic signaling (not to ignore local configuration of course).

Will add the words in the RECOMMENDED that sentence to highlight the
ignoring the hello sent ‘reverse-metric’ by policy.


345       Multi-Topology IS-IS [RFC5120] specifies there is no change to
346       construction of the Pseudonode LSP, regardless of the Multi-Topology
347       capabilities of a multi-access LAN.  If any MT capable node on the
348       LAN advertises the Reverse Metric TLV to the DIS, the DIS should
349       update, as appropriate, the default metric contained in the
350       Pseudonode LSP.  If the DIS updates the default metric in and floods
351       a new Pseudonode LSP, those default metric values will be applied to
352       all topologies during Multi-Topology SPF calculations.

354     3.4.  Point-To-Point Link Procedures

356       On a point-to-point link, there is already a "configured" IS-IS
357       interface metric to be applied over the link towards the IS-IS
358       neighbor.

[minor] Why is "configured" in quotation marks?  Is that different than configured (without quotations)?  Is this "configured" metric the same as the configured "default metric" mentioned in §2?

<NS> yes. removing quotes.


360       When IS-IS receives the IIH PDU with the "Reverse Metric" on a point-
361       to-point link and if the local policy allows the supporting of
362       "Reverse Metric", it MUST add the metric value in "reverse metric"
363       TLV according to the rules described in Section 3.1 and Section 3.2.

[major] Is there a reason for local policy only being mentioned in the p2p case?  I'm sure an operator may want to also not allow (using local policy) the use of the reverse metric in a LAN.  Should this indication that local policy may override any on-the-wire signaling be moved to a more general place in the document?

<NS> I think the document is talking about “allow/not-allow”.
ok. it’s a good idea to move this wording to a generic place. it is
along the same change of that “RECOMMENDED” comment above.


[nit] Note that except for this (I think general) point about local policy, this section just points back to §3.1 and 3.2..  Do we even need this section at all??

<NS> Right. the p2p section is removed.


365     3.5.  LDP/IGP Synchronization on LANs

367       As described in [RFC6138] when a new IS-IS node joins a broadcast
368       network, it is unnecessary and sometimes even harmful for all IS-IS
369       nodes on the LAN to advertise maximum link metric.  [RFC6138]
370       proposes a solution to have the new node not advertise its adjacency
371       towards the pseudo-node when it is not in a "cut-edge" position.

373       With the introduction of Reverse Metric in this document, a simpler
374       alternative solution to the above mentioned problem can be used.  The
375       Reverse Metric allows the new node on the LAN to advertise its
376       inbound metric value to be the maximum and this puts the link of this
377       new node in the last resort position without impacting the other IS-
378       IS nodes on the same LAN.

380       Specifically, when IS-IS adjacencies are being established by the new
381       node on the LAN, besides setting the maximum link metric value (2^24
382       - 2) on the interface of the LAN for LDP IGP synchronization as
383       described in [RFC5443], it SHOULD advertise the maximum metric offset
384       value in the Reverse Metric TLV in its IIH PDU sent on the LAN.  It
385       SHOULD continue this advertisement until it completes all the LDP
386       label binding exchanges with all the neighbors over this LAN, either
387       by receiving the LDP End-of-LIB [RFC5919] for all the sessions or by
388       exceeding the provisioned timeout value for the node LDP/IGP
389       synchronization.

[major] Should this document be marked as Updating rfc6138?  If not, why not?

<NS> I think they are both valid, RFC6138 is also valid although this reverse-metric
mechanism is simpler.


[major] I believe that the Normative specification ("...as described in [RFC5443], it SHOULD...") makes rfc5443 a Normative reference.

<NS> will move over.


391     3.6.  Operational Guidelines

393       A router MUST advertise a Reverse Metric TLV toward a neighbor only
394       for the period during which it wants a neighbor to temporarily update
395       its IS-IS metric or TE parameters towards it.

[major] The TLV is in the IIH.  Should it be in every message?  Would stopping mean just not advertising?  What if the sender doesn't want to wait until the next scheduled IIH?


<NS> This “period” wording is not the IIH interval, but the maintenance window period.
will add words to that.


...
406       When the link TE metric is raised to (2^24 - 1) [RFC5817], either due
407       to the reverse-metric mechanism or by explicit user configuration,
408       this SHOULD immediately trigger the CSPF re-calculation to move the
409       TE traffic away from that link.  It is RECOMMENDED also that the CSPF
410       does the immediate CSPF re-calculation when the TE metric is raised
411       to (2^24 - 2) to be the last resort link.

[major] These Normative statements are ok, but just for the case where the metric is raised because of the reverse metric, and not for the explicit configuration case.  This document is specifying the behavior of the reverse metric, not the general configuration response.  IOW, if someone doesn't read this document, then there's no way for the general case to apply to them -- and the general case (changed config) is not dependent on implementing the reverse metric.

<NS> That’s a good point. We have been strugling on this point. This is added due
to some request from the field, and those networks would like to see this 2^^24-1 and
2^^24-2 to be immediately notified and re-evaluate TE paths, instead of fixed
long interval for re-evaluate. By at least specifying in this document, we document
this behavior. Now if the CSPF module do immediate or not, does not break anything.
But the providers will have a place to point to when they try to convince the
implementation behavior from vendors.

We’ll remove this wording if you insist. Then someone can start a new document
to specifying such a needed behavior.


413       It is RECOMMENDED that implementations provide a capability to
414       disable any changes to a node's individual interface default metric
415       or Traffic Engineering parameters based upon receiving a properly
416       formatted Reverse Metric TLVs.

[major] The text above is in conflict with the text in §3.3 which says that "local provisioning...MUST take precedence".  (1) RECOMMENDED is the same as SHOULD, (2) disabling the change is equivalent to the local configuration taking precedence.

<NS> As mentioned above, changed.


418     4.  Security Considerations

420       The enhancement in this document makes it possible for one IS-IS
421       router to manipulate the IS-IS default metric and, optionally,
422       Traffic Engineering parameters of adjacent IS-IS neighbors.  Although
423       IS-IS routers within a single Autonomous System nearly always are
424       under the control of a single administrative authority, it is highly
425       RECOMMENDED that operators configure authentication of IS-IS PDUs to
426       mitigate use of the Reverse Metric TLV as a potential attack vector,
427       particularly on multi-access LANs.

[major] Authentication doesn't prevent a subverted router from using the reverse metric.

<NS> Right. But authentication is all IS-IS has.


[minor] I would love to see more text on what the threat really is.  I think that includes being able to divert traffic, sent traffic over less preferred paths, etc. -- in short, this extension can have a significant effect on routing decisions.

<NS> Right. But this is no different from any other IS-IS operations, such as changing local interface metric. reverse-metric
is no worse than those. all of them can change the routing behavior in the network.


[major] Is there anything (besides authenticating) that can be done to mitigate the threat?  The text talks about local configuration having the ability to override a reverse metric -- but it seems like the intent is for the default to be "accept the reverse metric"..  Making the default be "don't accept reverse metrics" (i.e. having to explicitly configure the willingness to accept the new TLV) would help by only allowing the reverse metric where the operator knows it wants it.  Was there any discussion about this in the WG?

<NS> I don’t recall there is such a discussion. This document does not specify which is the default
behavior. An implemetation can use either opt-in or opt-out, which does not break the
inter-operability.


429     5.  IANA Considerations

431       This document requests that IANA allocate from the IS-IS TLV
432       Codepoints Registry a new TLV, referred to as the "Reverse Metric"
433       TLV, possibly from the "Unassigned" range of 244-250, with the
434       following attributes: IIH = y, LSP = n, SNP = n, Purge = n.

[nit] IANA completed the early allocation, please update.


<NS> will add.