[Lsr] AD Review of draft-ietf-isis-segment-routing-extensions-22

Alvaro Retana <aretana.ietf@gmail.com> Wed, 20 March 2019 22:20 UTC

Return-Path: <aretana.ietf@gmail.com>
X-Original-To: lsr@ietfa.amsl.com
Delivered-To: lsr@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 63A2812798C; Wed, 20 Mar 2019 15:20:52 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.996
X-Spam-Level:
X-Spam-Status: No, score=-1.996 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, FREEMAIL_FROM=0.001, HTML_MESSAGE=0.001, NORMAL_HTTP_TO_IP=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_PASS=-0.001, THIS_AD=0.001, UNPARSEABLE_RELAY=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id a4UoZ6mb1a5R; Wed, 20 Mar 2019 15:20:45 -0700 (PDT)
Received: from mail-ot1-x329.google.com (mail-ot1-x329.google.com [IPv6:2607:f8b0:4864:20::329]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 6921A13118F; Wed, 20 Mar 2019 15:20:45 -0700 (PDT)
Received: by mail-ot1-x329.google.com with SMTP id q24so3727781otk.0; Wed, 20 Mar 2019 15:20:45 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:mime-version:date:message-id:subject:to:cc; bh=6lYKVLknMaUhOvWE4/5JgVQS9g7IkvRQoYgU4ijXuoU=; b=Pz7qjpHpxVIZrSC83MsCOO+wVzJ0YefZIr0Fb9vCJqMjK4Y8CA9TnHBsWXqRpUW6pT oPH0aEBGxcta94AB0qYpeBe/VHGQR1jGJ5F9bJTI6TUkd9tDf8v+OSm2e2DMiF9TJT/q 5NMLLNCBy2sacGe4GUo6gwpdmqwRl8iMP7qnaZq5/+gi0nCwHYxP/VK7WTDyHKYJSFQe +K7q1fDWGb/ULgFs/nIyjMM81t29k1LcsmfKrmJDmLGnVKp3RGFx77zVq6sNTDeAfppq 8OY9qezTQe6prWO4qsbuVzW5Trz0LBd5xN3vm0nqifSMvALMhKaCpaw2Zn5L9SjdO0EW KJbQ==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:mime-version:date:message-id:subject:to:cc; bh=6lYKVLknMaUhOvWE4/5JgVQS9g7IkvRQoYgU4ijXuoU=; b=NWctglBc8EZimdpILg/e7SoPDFdRSm/TDvKAXw95UYVp54veLI2S4xBAfz79eS+q+P /7dSGjNXN/OfGSMyTmW5hw5hzxmwMh+zkt0+2mTnMJGxiHYwpI6fEnX5YYZc6lZV0jfh CznaJCRbr+y+WGV8xkMe+Qua1d68A7loUZMgy9cVRcjOMvth8eb2750z2ZNlaGR4gdep vVGrI/eE+7S7V3aeKBHMBtw4TtEK6em0cW0Tf+2crbE93aWIKcj7RgNbgIlztfE6ZgS6 16POTab0eurYPuq+z/FIiPucKzMAdKBWgQ+EntazB6MdjWoTOXXUDYjL7M+CgYLYYT9K n8AQ==
X-Gm-Message-State: APjAAAUkV63l5FVaSHMo2g2gwSfrWjLMB10mHtAPSY75sDU4QiJkOGUG AjvXCi+nDiMuXByxF5w+DM8wCtKDvlYsKHVOo3PxMg==
X-Google-Smtp-Source: APXvYqxC3/iA2GSX0R7dhRopAS1DEkBkXJD7f86tccVYVy4GQXWwSIwVIiYFWrqD2Z4w3uY+E/Bkj6fH8Mei6LemszU=
X-Received: by 2002:a9d:6348:: with SMTP id y8mr299966otk.287.1553120443555; Wed, 20 Mar 2019 15:20:43 -0700 (PDT)
Received: from 1058052472880 named unknown by gmailapi.google.com with HTTPREST; Wed, 20 Mar 2019 15:20:42 -0700
From: Alvaro Retana <aretana.ietf@gmail.com>
MIME-Version: 1.0
Date: Wed, 20 Mar 2019 15:20:42 -0700
Message-ID: <CAMMESsz7nvqpSHY78j_3Gkksbf=SiyMfv8pV83iRN9GUfJJ_YA@mail.gmail.com>
To: draft-ietf-isis-segment-routing-extensions@ietf.org
Cc: Uma Chunduri <uma.chunduri@huawei.com>, lsr-chairs@ietf.org, lsr@ietf.org
Content-Type: multipart/alternative; boundary="00000000000054e5c605848e084d"
Archived-At: <https://mailarchive.ietf.org/arch/msg/lsr/trLsjLG0hE5hBgY2xn9l6zJHPv0>
Subject: [Lsr] AD Review of draft-ietf-isis-segment-routing-extensions-22
X-BeenThere: lsr@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Link State Routing Working Group <lsr.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/lsr>, <mailto:lsr-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/lsr/>
List-Post: <mailto:lsr@ietf.org>
List-Help: <mailto:lsr-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/lsr>, <mailto:lsr-request@ietf.org?subject=subscribe>
X-List-Received-Date: Wed, 20 Mar 2019 22:20:53 -0000

Dear authors:

I just finished reviewing this document.  Please take a look at the in-line
comments below.  In general, I think that some more work is needed.  I will
wait for that before starting the IETF Last Call.

There is a significant issue that I want to highlight here (there are also
related comments in-line):

- §2.1.1.2 talks about interaction with rfc7794 and a transition period
which apparently ends in this specification not being used.  Am I reading
that correctly?

Thanks!

Alvaro.

[Line numbers come from idnits.]

...
107 1.  Introduction

109   Segment Routing (SR) allows for a flexible definition of end-to-end
110   paths within IGP topologies by encoding paths as sequences of
111   topological sub-paths, called "segments".  These segments are
112   advertised by the link-state routing protocols (IS-IS and OSPF).
113   Prefix segments represent an ecmp-aware shortest-path to a prefix (or
114   a node), as per the state of the IGP topology.  Adjacency segments
115   represent a hop over a specific adjacency between two nodes in the
116   IGP.  A prefix segment is typically a multi-hop path while an
117   adjacency segment, in most of the cases, is a one-hop path.  SR's
118   control-plane can be applied to both IPv6 and MPLS data-planes, and
119   do not require any additional signaling (other than the regular IGP).
120   For example, when used in MPLS networks, SR paths do not require any
121   LDP or RSVP-TE signaling.  Still, SR can interoperate in the presence
122   of LSPs established with RSVP or LDP.

[nit] s/ecmp-aware/ECMP-aware


124   There are additional segment types, e.g., Binding SID defined in
125   [RFC8402].  This draft also defines an advertisement for one type of
126   BindingSID: the Mirror Context segment.

[nit] s/BindingSID/Binding SID


128   This draft describes the necessary IS-IS extensions that need to be
129   introduced for Segment Routing operating on an MPLS data-plane.

131   Segment Routing architecture is described in [RFC8402].

[nit] s/Segment Routing architecture/The Segment Routing architecture


133   Segment Routing use cases are described in [RFC7855].

135 2.  Segment Routing Identifiers

137   Segment Routing architecture ([RFC8402]) defines different types of
138   Segment Identifiers (SID).  This document defines the IS-IS encodings
139   for the IGP-Prefix-SID, the IGP-Adjacency-SID, the IGP-LAN-Adjacency-
140   SID and the Binding-SID.

[nit] s/([RFC8402])/[RFC8402]

[nit] s/Segment Routing architecture/The Segment Routing architecture

[major] rfc8402 doesn't use the names above.  Please be consistent.


142 2.1.  Prefix Segment Identifier (Prefix-SID Sub-TLV)

144   A new IS-IS sub-TLV is defined: the Prefix Segment Identifier sub-TLV
145   (Prefix-SID sub-TLV).

147   The Prefix-SID sub-TLV carries the Segment Routing IGP-Prefix-SID as
148   defined in [RFC8402].  The 'Prefix SID' MUST be unique within a given
149   IGP domain (when the L-flag is not set).  The 'Prefix SID' MUST carry
150   an index (when the V-flag is not set) that determines the actual SID/
151   label value inside the set of all advertised SID/label ranges of a
152   given router.  A receiving router uses the index to determine the
153   actual SID/label value in order to construct forwarding state to a
154   particular destination router.

[major] What if the Prefix SID is not unique?  What should the receiver do?

[major] "...MUST carry an index...that determines the actual SID/label
value..."  What are you trying to specify with this "MUST"?  The
description of the V-flag (below) already points at the value being an
index, but the qualification ("that determines...") makes the enforcement
of this "MUST" more complex: the receiver has to verify that the index
actually results in a SID/label inside a range for it to be valid.   What
should the receiver do if that is not the case?

Suggestion: Rearrange the text so that the TLV and its fields are described
first.  I think that then some of the text above won't be needed.


156   In many use-cases a 'stable transport' IP Address is overloaded as an
157   identifier of a given node.  Because the IP Prefixes may be re-
158   advertised into other levels there may be some ambiguity (e.g.
159   Originating router vs. L1L2 router) for which node a particular IP
160   prefix serves as identifier.  The Prefix-SID sub-TLV contains the
161   necessary flags to disambiguate IP Prefix to node mappings.
162   Furthermore if a given node has several 'stable transport' IP
163   addresses there are flags to differentiate those among other IP
164   Prefixes advertised from a given node.

[minor] Again, this text may be clearer if the TLV description was
presented first.  If you decide not to rearrange the text, at least put
forward references so readers can look forward to find out how the
statement above is achieved.


...
180   When the IP Reachability TLV is propagated across level boundaries,
181   the Prefix-SID sub-TLV SHOULD be kept.

[major] When would this sub-TLV not be kept?  IOW, why is that not a "MUST"?

§2.1.2: "The Prefix-SID sub-TLV MUST be preserved when the IP Reachability
TLV
gets propagated across level boundaries."

I'm not saying that one way is correct...either one may be.  Just be
consistent.

[minor] I'm assuming that "IP Reachability TLV" means one of the TLVs
above, true?  It may be worth clarifying that.


183   The Prefix-SID sub-TLV has the following format:

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     |   Algorithm   |
189   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
190   |                        SID/Index/Label (variable)             |
191   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

193   where:

195      Type: TBD, suggested value 3

[major] This value (and all the others in the document) are not "suggested
values", they have been already allocated by IANA.  Please update the
individual descriptions and the IANA Considerations section accordingly.


197      Length: variable.

[minor] Yes, variable, but it can only have two values: 5 or 6.


199      Flags: 1 octet field of following flags:

201    0 1 2 3 4 5 6 7
202   +-+-+-+-+-+-+-+-+
203   |R|N|P|E|V|L|   |
204   +-+-+-+-+-+-+-+-+

206      where:

208         R-Flag: Re-advertisement flag.  If set, then the prefix to
209         which this Prefix-SID is attached, has been propagated by the
210         router either from another level (i.e., from level-1 to level-2
211         or the opposite) or from redistribution (e.g.: from another
212         protocol).

[major] Should it be used to avoid looping as well?  Or is there no risk of
that in this case?


...
236         Other bits: MUST be zero when originated and ignored when
237         received.

[major] Do you expect IANA to handle the assignment of those other bits?


239      Algorithm: the router may use various algorithms when calculating
240      reachability to other nodes or to prefixes attached to these
241      nodes.  Algorithms identifiers are defined in Section 3.2.
242      Examples of these algorithms are metric based Shortest Path First
243      (SPF), various sorts of Constrained SPF, etc.  The algorithm field
244      of the Prefix-SID contains the identifier of the algorithm the
245      router has used in order to compute the reachability of the prefix
246      to which the Prefix-SID is associated.

248      At origination, the Prefix-SID algorithm field MUST be set to 0 or
249      to any value advertised in the SR-Algorithm sub-TLV (Section 3.2).

[major] What should a receiver do if the Algorithm value doesn't match what
has been advertised in the SR-Algorithm Sub-TLV?


...
282 2.1.1.2.  R and N Flags

284   The R-Flag MUST be set for prefixes that are not local to the router
285   and either:

287      advertised because of propagation (Level-1 into Level-2);

289      advertised because of leaking (Level-2 into Level-1);

291      advertised because of redistribution (e.g.: from another
292      protocol).

294   In the case where a Level-1-2 router has local interface addresses
295   configured in one level, it may also propagate these addresses into
296   the other level.  In such case, the Level-1-2 router MUST NOT set the
297   R bit.  The R-bit MUST be set only for prefixes that are not local to
298   the router and advertised by the router because of propagation and/or
299   leaking.

[minor] This last sentence is redundant with the start of this section.
Please specify the behavior just once.


301   The N-Flag is used in order to define a Node-SID.  A router MAY set
302   the N-Flag only if all of the following conditions are met:

304      The prefix to which the Prefix-SID is attached is local to the
305      router (i.e., the prefix is configured on one of the local
306      interfaces, e.g., a 'stable transport' loopback).

308      The prefix to which the Prefix-SID is attached MUST have a Prefix
309      length of either /32 (IPv4) or /128 (IPv6).

[major] "The prefix...MUST have a Prefix length..."  This condition is not
worded as one; perhaps: "The prefix...has a Prefix length..."

[minor] Why is the N-Flag needed?  If the setting is optional ("MAY"), then
not setting it doesn't imply that the conditions are not met...

[major] It is clearly an error if both the R-Flag and N-Flag are set,
right?  What should the receiver do in that case?


311   The router MUST ignore the N-Flag on a received Prefix-SID if the
312   prefix has a Prefix length different than /32 (IPv4) or /128 (IPv6).

314   [RFC7794] also defines the N and R flags and with the same semantics
315   of the equivalent flags defined in this document.  There will be a
316   transition period where both sets of flags will be used and
317   eventually only the flags of the Prefix Attributes will remain.
318   During the transition period implementations supporting the N and R
319   flags defined in this document and the N and R flags defined in
320   [RFC7794] MUST advertise and parse all flags.  In case the received
321   flags have different values, the value of the flags defined in
322   [RFC7794] prevails.

[major] "...transition period where both sets of flags will be used and
eventually only the flags of the Prefix Attributes will remain"  To be
honest, you lost me here?  Please explain what will the transition period
be between?

[major] If "both sets of flags will be used and eventually only the
[already defined] flags of the Prefix Attributes will remain", why do we
need the new ones?

[major] "MUST advertise and parse all flags"  What does this mean?  By "all
flags" I guess you mean the N and R flags...but if both this document and
rfc7794 are implemented, it seems to me that the specification above is
really meaningless.

[major] "In case the received flags have different values..."  For a second
it seemed to me that what you really wanted was for the flags in this
document to behave the same way as the flags defined in rfc7794...the easy
solution to that (and to avoid my questions listed before rfc7794 was
mentioned) would have been to just point at rfc7794 as the place where the
Flags are defined.  However, it looks like the definitions (at least for
the R-Flag) are not the same.  Is that on purpose or is it just an
oversight?

One piece of information that might make this discussion relevant is
understanding the significance of these Flags...but neither document offers
any specific use, beyond simply "it may be useful for other routers to
know" (rfc7794, §2.2).


324 2.1.1.3.  E and P Flags

326   When calculating the outgoing label for the prefix, the router MUST
327   take into account E and P flags advertised by the next-hop router, if
328   next-hop router advertised the SID for the prefix.  This MUST be done
329   regardless of next-hop router contributing to the best path to the
330   prefix or not.

[major] "the router MUST take into account"  What normative action is
standardized with that phrase?  It would be clearer/better if the behavior
is explicitly specified in function of the flags (as sone below).
 s/MUST/must

[major] "This MUST be done regardless of next-hop router contributing to
the best path to the prefix or not."  This what?  I'm assuming the text
refers to "take into account"...  Same comment as before: s/MUST/must


...
366 2.1.2.  Prefix-SID Propagation

368   The Prefix-SID sub-TLV MUST be preserved when the IP Reachability TLV
369   gets propagated across level boundaries.

[major] §2.1 says that it SHOULD.  Please be consistent...and, specify
behavior only once.

371   The level-1-2 router that propagates the Prefix-SID sub-TLV between
372   levels MUST set the R-flag.

374   If the Prefix-SID contains a global index (L and V flags unset) and
375   it is propagated as such (with L and V flags unset), the value of the
376   index MUST be preserved when propagated between levels.

378   The level-1-2 router that propagates the Prefix-SID sub-TLV between
379   levels MAY change the setting of the L and V flags in case a local
380   label value is encoded in the Prefix-SID instead of the received
381   value.

[major]  Up to now, I had been assuming that propagating and preserving
meant not changing the sub-TLV (except for the R-Flag).  But these last
paragraphs indicate that not only can some flags be changed, but the
contents can completely change.  It would be ideal if the valid operations
on the sub-TLV were discussed (or at least referenced) when the preserving
behavior was first introduced (I think in §2.1).


...
409 2.2.1.  Adjacency Segment Identifier (Adj-SID) Sub-TLV

411   The following format is defined for the Adj-SID sub-TLV:

413    0                   1                   2                   3
414    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
415   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
416   |   Type        |     Length    |     Flags     |     Weight    |
417   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
418   |                         SID/Label/Index (variable)            |
419   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

421   where:

423      Type: TBD, suggested value 31

425      Length: variable.

[minor] Yes, variable, but it can only have two values...

427      Flags: 1 octet field of following flags:

429          0 1 2 3 4 5 6 7
430         +-+-+-+-+-+-+-+-+
431         |F|B|V|L|S|P|   |
432         +-+-+-+-+-+-+-+-+

434      where:

436         F-Flag: Address-Family flag.  If unset, then the Adj-SID refers
437         to an adjacency with outgoing IPv4 encapsulation.  If set then
438         the Adj-SID refers to an adjacency with outgoing IPv6
439         encapsulation.

[major] This document describes the operation on the MPLS data plane,
right?  The F-Flag talks about the outgoing encapsulation...but that could
simply be the next label in the MPLS stack, right?  What am I missing?  I
get the impression that there's a relationship between what this Flag
points to and something else...but I just don't know what.


441         B-Flag: Backup flag.  If set, the Adj-SID is eligible for
442         protection (e.g.: using IPFRR or MPLS-FRR) as described in
443         [RFC8355].

[major] Because the specification of the B-Flag depends directly on
rfc8355, it should be a Normative reference.  Looking at rfc8355, I
couldn't find a place where it talks about eligibility for
protection...just text about potential strategies, but (honestly) nothing
that I would call Normative.  Please clarify.  Perhaps rfc8402 might be a
better reference.

[major] I'm assuming that this description (pointing at rfc8355), plus the
text below about allocation of Adj-SIDs should result in some action
related to the protection...what is that?  IOW, presented with the B-Flag
set, what action should a node take?


445         V-Flag: Value flag.  If set, then the Adj-SID carries a value.
446         By default the flag is SET.

[minor] Is the description the same at the V-Flag in §2.1?  If so, then
indicating that, or at least also pointing out here that the value is
carried instead of an index would be helpful.


448         L-Flag: Local Flag.  If set, then the value/index carried by
449         the Adj-SID has local significance.  By default the flag is
450         SET.

[major] Looking back at §2.1.1.1, what are the conditions for which the
combination of the V and L-Flags are valid/invalid?  Do the same conditions
apply?


...
460         Other bits: MUST be zero when originated and ignored when
461         received.

[major] Do you expect IANA to handle the assignment of those other bits?


...
469      An SR capable router MAY allocate an Adj-SID for each of its
470      adjacencies and SHOULD set the B-Flag when the adjacency is
471      eligible for protection (IP or MPLS).

[major] If eligible for protection, when would the router not set the
B-Flag?  IOW, why is that not a MUST?

Beyond that, I think that the use of SHOULD is in conflict with the
definition of the B-Flag (above): "If set, the Adj-SID is eligible for
protection...", which implies that it isn't eligible if it's not set...but
the SHOULD opens the door for not setting the Flag...


...
479      When the P-flag is not set, the Adj-SID MAY be persistent.  When
480      the P-flag is set, the Adj-SID MUST be persistent.

[major] If the adjacency is persistent, why/when would the P-Flag not be
set?


...
488 2.2.2.  Adjacency Segment Identifiers in LANs
...
537      Other bits: MUST be zero when originated and ignored when
538      received.

[major] Do you expect IANA to handle the assignment of those other bits?


...
544      System-ID: 6 octets of IS-IS System-ID of length "ID Length" as
545      defined in [ISO10589].

[major] Which System-ID?

[major] "6 octets of...length "ID Length""  ??


547      SID/Index/Label as defined in Section 2.1.1.1.

549   Multiple LAN-Adj-SID sub-TLVs MAY be encoded.  Note that this sub-TLV
550   MUST NOT appear in TLV 141.

552   When the P-flag is not set, the LAN-Adj-SID MAY be persistent.  When
553   the P-flag is set, the LAN-Adj-SID MUST be persistent.

555   In case one TLV-22/23/222/223 (reporting the adjacency to the DIS)
556   can't contain the whole set of LAN-Adj-SID sub-TLVs, multiple
557   advertisements of the adjacency to the DIS MUST be used and all
558   advertisements MUST have the same metric.

560   Each router within the level, by receiving the DIS PN LSP as well as
561   the non-PN LSP of each router in the LAN, is capable of
562   reconstructing the LAN topology as well as the set of Adj-SID each
563   router uses for each of its neighbors.

565   A label is encoded in 3 octets (in the 20 rightmost bits).

567   An index is encoded in 4 octets.

[minor] Some of the information above (for example, the paragraph about the
P-Flag and these last 2 sentences) seems unnecessary given that the
description of these fields is already done somewhere else.


569 2.3.  SID/Label Sub-TLV

571   The SID/Label sub-TLV may be present in the following TLVs/sub-TLVs
572   defined in this document:

574   SR-Capabilities Sub-TLV (Section 3.1)

576   SR Local Block Sub-TLV (Section 3.3)

578   SID/Label Binding TLV (Section 2.4)

580   Multi-Topology SID/Label Binding TLV (Section 2.5)

[nit] It might have been nice, in line with the OSPF work, to flip Sections
2 and 3.  Just a nit...

582   Note that the code point used in all of the above cases is the SID/
583   Label Sub-TLV code point assigned by IANA in the "sub-TLVs for TLV
584   149 and 150" registry.

[major] Because this document is setting up that new registry, we don't
have to wait for IANA to assign anything: the document can specify it.
IOW, the paragraph above is not needed.


586   The SID/Label sub-TLV contains a SID or a MPLS Label.  The SID/Label
587   sub-TLV has the following format:

589    0                   1                   2                   3
590    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
591   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
592   |   Type        |     Length    |
593   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
594   |                          SID/Label (variable)                 |
595   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

597   where:

599      Type: TBD, suggested value 1

601      Length: variable

[major] Yes, variable...but right now only one value is defined.  There's a
finite number of possibilities...


602      SID/Label: if length is set to 3 then the 20 rightmost bits
603      represent a MPLS label.

[major] Is that it..?  What about the SID?


605 2.4.  SID/Label Binding TLV
...
619   The SID/Label Binding TLV has Type TBD (suggested value 149), and has
620   the following format:

[nit] No need to talk about the Type twice.

622      0                   1                   2                   3
623      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
624     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
625     |      Type     |     Length    |     Flags     |     RESERVED  |
626     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
627     |            Range              | Prefix Length |     Prefix    |
628     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
629     //               Prefix (continued, variable)                  //
630     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
631     |                    SubTLVs (variable)                         |
632     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

634                  Figure 1: SID/Label Binding TLV format

636   o  Type: TBD, suggested value 149


...
657 2.4.1.  Flags
...
671      M-Flag: Mirror Context flag.  Set if the advertised SID
672      corresponds to a mirrored context.  The use of the M flag is
673      described in [RFC8402].

[major] Where?  Note that rfc8402 says that "The Mirror SID is advertised
using the Binding segment defined in SR IGP protocol extensions
[ISIS-SR-EXT].", pointing back at this document.

[minor] Some places use *-Flag and others "* flag", please be consistent.


675      S-Flag: If set, the SID/Label Binding TLV SHOULD be flooded across
676      the entire routing domain.  If the S flag is not set, the SID/
677      Label Binding TLV MUST NOT be leaked between levels.  This bit
678      MUST NOT be altered during the TLV leaking.

[major] If the S-Flag is set, when would the TLV not be flooded across the
entire domain?  IOW, why is the SHOULD not a MUST?


680      D-Flag: when the SID/Label Binding TLV is leaked from level-2 to
681      level-1, the D bit MUST be set.  Otherwise, this bit MUST be
682      clear.  SID/Label Binding TLVs with the D bit set MUST NOT be
683      leaked from level-1 to level-2.  This is to prevent TLV looping
684      across levels.

[minor] What about leaking from L2 to L1 with the Flag set?

[minor] The text uses Flag, but in some cases bit is used as well.  Please
be consistent.


...
695      An implementation MAY decide not to honor the S-flag in order not
696      to leak Binding TLV's between levels (for policy reasons).  In all
697      cases, the D flag MUST always be set by any router leaking the
698      Binding TLV from level-2 into level-1 and MUST be checked when
699      propagating the Binding TLV from level-1 into level-2.  If the D
700      flag is set, the Binding TLV MUST NOT be propagated into level-2.

[major] s/implementation MAY decide/implementation may decide   There's no
Normative action there.

[major] Regardless of whether MAY/may is used, the statement in the first
sentence contradicts the "MUST NOT be leaked" Normative statement.  Given
that there are exceptions, maybe "SHOULD NOT" would be more appropriate.

[major] This last paragraph seems to contain the same information as the
text describing the S-Flag.  Please don't duplicate specifications.

702      Other bits: MUST be zero when originated and ignored when
703      received.

[major] Do you expect IANA to handle the assignment of those other bits?

705 2.4.2.  Range

707   The 'Range' field provides the ability to specify a range of
708   addresses and their associated Prefix SIDs.  This advertisement
709   supports the SRMS functionality.  It is essentially a compression
710   scheme to distribute a continuous Prefix and their continuous,
711   corresponding SID/Label Block.  If a single SID is advertised then
712   the range field MUST be set to one.  For range advertisements > 1,
713   the number of addresses that need to be mapped into a Prefix-SID and
714   the starting value of the Prefix-SID range.

[nit] The last sentence sounds incomplete...

716   Example 1: if the following router addresses (loopback addresses)
717   need to be mapped into the corresponding Prefix SID indexes.

[minor] Suggestion: move the examples to the end of §2.4...after all the
fields have been discussed.

719   Router-A: 192.0.2.1/32, Prefix-SID: Index 1
720   Router-B: 192.0.2.2/32, Prefix-SID: Index 2
721   Router-C: 192.0.2.3/32, Prefix-SID: Index 3
722   Router-D: 192.0.2.4/32, Prefix-SID: Index 4

724      0                   1                   2                   3
725      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
726     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
727     |      Type     |     Length    |0|0|           |     RESERVED  |
728     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
729     |            Range = 4          |       /32     |      192      |
730     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
731     |       .0      |        .2     |       .1      |Prefix-SID Type|
732     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
733     | sub-TLV Length|     Flags     |   Algorithm   |               |
734     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
735     |                                             1 |
736     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

[nit] The Prefix Length should be "32", not "/32".

[nit] The Prefix itself is one field (in the description), but it looks
like 4 separate fields above...and the numbers should not have "." in them.

[minor] It looks like the fields starting with "Prefix-SID Type" belong to
the Prefix-SID Sub-TLV, but that is not explained anywhere (yet).  The
Algorithm field is not set...nor are the Flags.

738   Example-2: If the following prefixes need to be mapped into the
739   corresponding Prefix-SID indexes:

741   10.1.1/24, Prefix-SID: Index 51
742   10.1.2/24, Prefix-SID: Index 52
743   10.1.3/24, Prefix-SID: Index 53
744   10.1.4/24, Prefix-SID: Index 54
745   10.1.5/24, Prefix-SID: Index 55
746   10.1.6/24, Prefix-SID: Index 56
747   10.1.7/24, Prefix-SID: Index 57

[major] Please use addresses in the rfc5737 reserved ranges.

[minor] It would have been nice to see an IPv6 example (see rfc3849).

749      0                   1                   2                   3
750      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
751     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
752     |      Type     |     Length    |0|0|           |     RESERVED  |
753     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
754     |            Range = 7          |       /24     |      10       |
755     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
756     |       .1      |        .1     |Prefix-SID Type| sub-TLV Length|
757     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
758     |    Flags      | Algorithm     |                               |
759     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
760     |                           51  |
761     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

[] Same comments as above.


...
778 2.4.3.  Prefix Length, Prefix
...
784   The 'Prefix Length' field contains the length of the prefix in bits.
785   Only the most significant octets of the Prefix are encoded.  (i.e., 1
786   octet for prefix length 1 up to 8, 2 octets for prefix length 9 to
787   16, 3 octets for prefix length 17 up to 24 and 4 octets for prefix
788   length 25 up to 32, ...., 16 octets for prefix length 113 up to 128).

[nit] s/encoded.  (i.e.,/encoded (i.e.,


790 2.4.4.  Mapping Server Prefix-SID

792   The Prefix-SID sub-TLV (suggested value 3) is defined in Section 2.1
793   and contains the SID/index/label value associated with the prefix and
794   range.  The Prefix-SID SubTLV MUST be present in the SID/Label
795   Binding TLV unless the M-flag is set in the Flags field of the parent
796   TLV.

[major] Does it then mean that the Prefix-SID SubTLV must not be present if
the M-Flag is not set?

[nit] s/SubTLV/Sub-TLV (as it is used almost everywhere else)


798   A node receiving an SRMS entry for a prefix MUST check the existence
799   of such prefix in its link-state database prior to consider and use
800   the associated SID.

[major] What does it mean for the prefix to exist in the link-state
database?  Does it mean that it must have been advertised in a "normal"
routing TLV, or something else?  I ask not only to clarify, but because
§2.4.3 says that the "'Prefix' does not need to correspond to a routable
prefix of the originating node".

[Ah...I think I understand.  Because "the mapping server does not specify
the originator of a prefix" (§2.4.4.2), then the prefix is obviously not
routable in the context of the originating node (the server)...but it
should be routable in general.  Is that it?]


802 2.4.4.1.  Prefix-SID Flags

804   The Prefix-SID flags are defined in Section 2.1.  The Mapping Server
805   MAY advertise a mapping with the N flag set when the prefix being
806   mapped is known in the link-state topology with a mask length of 32
807   (IPv4) or 128 (IPv6) and when the prefix represents a node.  The
808   mechanisms through which the operator defines that a prefix
809   represents a node are outside the scope of this document (typically
810   it will be through configuration).

[major] §2.1.1.2 says that the "router MAY set the N-Flag only if... the
prefix to which the Prefix-SID is attached MUST have a Prefix length of
either /32 (IPv4) or /128 (IPv6)."  That is similar to what the text above
says (but the text talks about "the prefix...known in the link-state
topology"), and it is related to the last question in the previous
section.  Does the prefix being in the link-state database/topology mean
that the match is exact (to the prefix advertised), or is a supernet ok?


812   The other flags defined in Section 2.1 are not used by the Mapping
813   Server and MUST be ignored at reception.

[major] How does the receiver know that the sub-TLV was originated by a
Mapping Server?  Is it the case that it would originate a Binding TLV?
IOW, would the other flags always be ignored when the sub-TLV is included
in the Binding TLV (but not other TLVs)?   Does this apply also to the
Multi-Topology SID/Label Binding TLV (§2.5)?


815 2.4.4.2.  PHP Behavior when using Mapping Server Advertisements
...
823   o  A prefix reachability advertisement for the prefix has been
824      received which includes the Extended Reachability Attribute Flags
825      sub-TLV ([RFC7794]).

[nit] s/([RFC7794])/[RFC7794]

827   o  X and R flags are both set to 0 in the Extended Reachability
828      Attribute Flags sub-TLV.

830   In the absence of an Extended Reachability Attribute Flags sub-TLV
831   ([RFC7794]) the A flag in the binding TLV indicates that the
832   originator of a prefix reachability advertisement is directly
833   connected to the prefix and thus PHP MUST be done by the neighbors of
834   the router originating the prefix reachability advertisement.  Note
835   that A-flag is only valid in the original area in which the Binding
836   TLV is advertised.

[nit] s/([RFC7794])/[RFC7794]

[major] §2.4.1 says that "if the Binding TLV is leaked...the A-flag MUST be
cleared", which is not the same as not considering it valid (ignoring, as
described above).  In any case, please be specific about the functionality
in the description.


838 2.4.4.3.  Prefix-SID Algorithm

840   The algorithm field contains the identifier of the algorithm the
841   router MUST use in order to compute reachability to the range of
842   prefixes.  Use of the algorithm field is described in Section 2.1.

[major nit] At first glance, it looks like this section is not needed
because it repeats what §2.1 already says about the algorithm.  But this
paragraph refers to "the algorithm the router MUST use in order to compute
reachability", in contrast to the definition of the field in §2.1: "The
algorithm field...contains the identifier of the algorithm the router has
used in order to compute the reachability..."


...
901 3.1.  SR-Capabilities Sub-TLV
...
909   The Router Capability TLV specifies flags that control its
910   advertisement.  The SR Capabilities sub-TLV MUST be propagated
911   throughout the level and MUST NOT be advertised across level
912   boundaries.  Therefore Router Capability TLV distribution flags are
913   set accordingly, i.e., the S flag in the Router Capability TLV
914   ([RFC7981]) MUST be unset.

[nit] s/([RFC7981])/[RFC7981]


...
943         I-Flag: MPLS IPv4 flag.  If set, then the router is capable of
944         processing SR MPLS encapsulated IPv4 packets on all interfaces.

946         V-Flag: MPLS IPv6 flag.  If set, then the router is capable of
947         processing SR MPLS encapsulated IPv6 packets on all interfaces.

[minor] Just curious: How are these flags used?  Given that the data plane
we're dealing with is MPLS...


...
981   The originating router MUST ensure the order is same after a graceful
982   restart (using checkpointing, non-volatile storage or any other
983   mechanism) in order to guarantee the same order before and after GR.

[nit] s/order is same/order is the same


...
1015 3.2.  SR-Algorithm Sub-TLV
...
1046   The SR-Algorithm sub-TLV is optional, it MAY only appear a single
1047   time inside the Router Capability TLV.

[major] "MAY only appear a single time" means that it can appear more than
once.  If it does show up more than once, what should the the receiver do?


1049   When the originating router does not advertise the SR-Algorithm sub-
1050   TLV, then all the Prefix-SIDs advertised by the router MUST have
1051   algorithm field set to 0.  Any receiving router MUST assume SPF
1052   algorithm (i.e., Shortest Path First).

[minor] It seems to me that this part of the specification would fit better
in §2.1.

[major] Besides maybe being redundant, what is specified in the last
sentence?  If the indication of the algorithm is explicit, then I don't
understand what "MUST assume" is trying to mandate.


1054   When the originating router does advertise the SR-Algorithm sub-TLV,
1055   then algorithm 0 MUST be present while algorithm 1 MAY be present.

[nit] I think that it is clear that algorithm 1 is optional.  I would take
that last part out just because other algorithms may be defined in the
future...and the important part at this point is the inclusion of algorithm
0.


1057   The SR-Algorithm sub-TLV has following format:

[nit] s/has following/has the following


...
1073      Algorithm: 1 octet of algorithm Section 2.1

[minor] The Algorithm field is described in this section, not in 2.1.


1075 3.3.  SR Local Block Sub-TLV

1077   The SR Local Block (SRLB) Sub-TLV contains the range of labels the
1078   node has reserved for local SIDs.  Local SIDs are used, e.g., for
1079   Adjacency-SIDs, and may also be allocated by other components than
1080   IS-IS protocol.  As an example, an application or a controller may
1081   instruct the router to allocate a specific local SID.  Therefore, in
1082   order for such applications or controllers to know what are the local
1083   SIDs available in the router, it is required that the router
1084   advertises its SRLB.

[nit] s/than IS-IS protocol/than the IS-IS protocol


...
1122   The originating router MUST NOT advertise overlapping ranges.

[major] What should the receiver do if the ranges overlap?  I'm assuming
the same thing as what was specified before...please be specific.


1124   It is important to note that each time a SID from the SRLB is
1125   allocated, it SHOULD also be reported to all components (e.g.:
1126   controller or applications) in order for these components to have an
1127   up-to-date view of the current SRLB allocation and in order to avoid
1128   collision between allocation instructions.

1130   Within the context of IS-IS, the reporting of local SIDs is done
1131   through IS-IS Sub-TLVs such as the Adjacency-SID.  However, the
1132   reporting of allocated local SIDs may also be done through other
1133   means and protocols which mechanisms are outside the scope of this
1134   document.

[major] "SHOULD also be reported to all components"  Because that is
outside the scope, then it shouldn't be a Normative statement.
 s/SHOULD/should


...
1170 4.  Non backward compatible changes with prior versions of this
document

[major] I find this section unnecessary.  If you want to keep it, please
move it to an Appendix and don't use Normative language.


...
1190 5.  IANA Considerations

1192   This documents request allocation for the following TLVs and subTLVs.

[minor] IANA may be ok, but I find the formatting below a little
confusing.  A table may be a better option.


...
1313   This document creates the following sub-TLV Registry:
...
[major] Please indicate what the range is.


1333 6.  Security Considerations

1335   With the use of the extensions defined in this document, IS-IS
1336   carries information which will be used to program the MPLS data plane
1337   [RFC3031].  In general, the same types of attacks that can be carried
1338   out on the IP/IPv6 control plane can be carried out on the MPLS
1339   control plane resulting in traffic being misrouted in the respective
1340   data planes.  However, the latter may be more difficult to detect and
1341   isolate.  Existing security extensions as described in [RFC5304] and
1342   [RFC5310] apply to these segment routing extensions.

[nit] Maybe break up the paragraph into multiple ones with independent
points.

[major] With the last sentence, are you implying that authentication is
required when IS-IS is carrying SR extensions?  If so, please be explicit.
Why would these extensions be different than any other extension?

[minor] The companion OSPF documents added the following paragraph to the
Security Considerations as a result of one of the reviews...perhaps
consider including something similar:

   Implementations MUST assure that malformed TLV and Sub-TLV defined in
   this document are detected and do not provide a vulnerability for
   attackers to crash the OSPFv2 router or routing process.  Reception
   of malformed TLV or Sub-TLV SHOULD be counted and/or logged for
   further analysis.  Logging of malformed TLVs and Sub-TLVs SHOULD be
   rate-limited to prevent a Denial of Service (DoS) attack (distributed
   or otherwise) from overloading the OSPF control plane.


...
1415 9.1.  Normative References
...
[minor] I think that these references can be Informative:

1463   [RFC5305]  Li, T. and H. Smit, "IS-IS Extensions for Traffic
1464              Engineering", RFC 5305, DOI 10.17487/RFC5305, October
1465              2008, <https://www.rfc-editor.org/info/rfc5305>.

1467   [RFC5308]  Hopps, C., "Routing IPv6 with IS-IS", RFC 5308,
1468              DOI 10.17487/RFC5308, October 2008,
1469              <https://www.rfc-editor.org/info/rfc5308>.


...
1527   Les Ginsberg (editor)
1528   Cisco Systems, Inc.
1529   IT

[nit] I didn't know you had moved. ;-)