[Lsr] AD Review of draft-ietf-lsr-isis-srv6-extensions-11

[Alvaro Retana <[e-mail]>]

Dear authors:

Please find below my review of this document.  I appreciate you and
the WG discussing the details, but there is more work needed to be
done before starting the IETF LC (details inline).

Just one high-level comment: It is not clear to me why all the
behaviors from rfc8986 are not covered in this document.  If some are
not applicable, or are covered elsewhere, please explain in the text.


Thanks!

Alvaro.

[Line numbers from idnits.]


...
16	Abstract

18	   Segment Routing (SR) allows for a flexible definition of end-to-end
19	   paths by encoding paths as sequences of topological sub-paths, called
20	   "segments".  Segment routing architecture can be implemented over an
21	   MPLS data plane as well as an IPv6 data plane.  This draft describes
22	   the IS-IS extensions required to support Segment Routing over an IPv6
23	   data plane.

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


[nit] s/This draft/This document


...
108	1.  Introduction
...
119	   The network programming paradigm
120	   [I-D.ietf-spring-srv6-network-programming] is central to SRv6.  It
121	   describes how any behavior can be bound to a SID and how any network
122	   program can be expressed as a combination of SIDs.

[major] s/[I-D.ietf-spring-srv6-network-programming]/[RFC8986]/g


...
131	   This document defines one new top level IS-IS TLV and several new IS-
132	   IS sub-TLVs.

134	   The SRv6 Capabilities sub-TLV announces the ability to support SRv6.

136	   Several new sub-TLVs are defined to advertise various SRv6 Maximum
137	   SID Depths.

139	   The new SRv6 Locator top level TLV announces SRv6 locators - a form
140	   of summary address for the set of topology/algorithm specific SIDs
141	   instantiated at the node.

143	   The SRv6 End SID sub-TLV, the SRv6 End.X SID sub-TLV, and the SRv6
144	   LAN End.X SID sub-TLV are used to advertise which SIDs are
145	   instantiated at a node and what Endpoint behavior is bound to each
146	   instantiated SID.

[nit] There is some repetition in the last few paragraphs.  You talk
about the new work, the the TLV, sub-TLVs, TLV again, and then the
sub-TLVs.  A little consolidation would make this part read better.


148	2.  SRv6 Capabilities sub-TLV

150	   A node indicates that it supports the SR Segment Endpoint Node
151	   functionality as specified in [RFC8754] by advertising a new SRv6
152	   Capabilities sub-TLV of the router capabilities TLV [RFC7981].

[minor] What about the functionality in rfc8986?  I'm assuming that
you want the nodes advertising this new sub-TLV to support both.  It
may be implicit, but please make it explicit.


[minor] "router capabilities TLV [RFC7981]"  What should be the
flooding scope of the TLV that includes this new sub-TLV?


...
159	      0                   1                   2                   3
160	       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

[nit] Please align the numbering.  There's one other occurrence in this section.


...
180	           O-flag: If set, the router supports use of the O-bit
181	           in the Segment Routing Header(SRH) as defined in
182	           [I-D.ietf-6man-spring-srv6-oam].

[nit] s/Segment Routing Header(SRH)/Segment Routing Header (SRH)


[major] Please ask IANA to set up a registry for the Flags.


184	3.  Advertising Supported Algorithms

186	   SRv6 capable router indicates supported algorithm(s) by advertising
187	   the SR Algorithm TLV as defined in [RFC8667].

[nit] s/SRv6 capable router/An SRv6 capable router


[minor] s/SR Algorithm TLV/SR Algorithm sub-TLV


...
200	4.1.  Maximum Segments Left MSD Type

202	   The Maximum Segments Left MSD Type specifies the maximum value of the
203	   "SL" field [RFC8754] in the SRH of a received packet before applying
204	   the Endpoint behavior associated with a SID.

[minor] s/specifies/signals/g


[minor] Please expand SL.


206	      SRH Max SL Type: 41

208	      If no value is advertised the supported value is assumed to be 0.

[major] What exactly does this MSD type signal?  Is this the
expectation that the SL will be <= the value when received at the
advertiser?  Is there an example of its use?  I'm having a hard time
picturing when (for non PSP behaviors) the SL would be more than 0.


210	4.2.  Maximum End Pop MSD Type

212	   The Maximum End Pop MSD Type specifies the maximum number of SIDs in
213	   the SRH to which the router can apply "PSP" or USP" behavior, as
214	   defined in [I-D.ietf-spring-srv6-network-programming] flavors.

[minor] Please expand PSP and USP.


...
221	4.3.  Maximum H.Encaps MSD Type

223	   The Maximum H.Encaps MSD Type specifies the maximum number of SIDs
224	   that can be included as part of the "H.Encaps" behavior as defined in
225	   [I-D.ietf-spring-srv6-network-programming].

[nit] s/included/pushed   That is the terminology used in rfc8986.


...
229	      If the advertised value is zero or no value is advertised
230	      then the router can apply H.Encaps only by encapsulating
231	      the incoming packet in another IPv6 header without SRH
232	      the same way IPinIP encapsulation is performed.

234	      If the advertised value is non-zero then the router supports both
235	      IPinIP and SRH encapsulation subject to the SID limitation
236	      specified by the advertised value.

[major] rfc8986 doesn't talk about IPinIP encapsulation, but is does say this:

   The push of the SRH MAY be omitted when the SRv6 Policy only contains
   one segment and there is no need to use any flag, tag or TLV.

Suggestion (to replace the last two paragraphs)>
    If the advertised value is zero or no value is advertised then the
    headend can apply an SR Policy that only contains one segment, by
    omitting the SRH push.

    A non-zero SRH Max H.encaps MSD indicates that the headend can push
    an SRH up to the advertised value.


238	4.4.  Maximum End D MSD Type

240	   The Maximum End D MSD Type specifies the maximum number of SIDs in an
241	   SRH when performing decapsulation associated with "End.Dx" behaviors
242	   (e.g., "End.DX6" and "End.DT6") as defined in
243	   [I-D.ietf-spring-srv6-network-programming].

[minor] "(e.g., "End.DX6" and "End.DT6")"  This MSD-Type only applies
to *.DX6, DT6 and DT46, right?  If so, please be specific about it.


245	      SRH Max End D Type: 45

247	      If the advertised value is zero or no value is advertised
248	      then it is assumed that the router cannot apply
249	      "End.DX6" or "End.DT6" behaviors if the outer IPv6 header
250	      contains an SRH.

[minor] What about End.DT46?

[major] What about other behaviors?  Are there no limitations to speak
of related to them?  Why?  rfc8986 says this:

   The IGP should also advertise the Maximum SID Depth (MSD) capability of
   the node for each type of SRv6 operation -- in particular, the SR source
   (e.g., H.Encaps), intermediate endpoint (e.g., End and End.X), and final
   endpoint (e.g., End.DX4 and End.DT6) behaviors.


252	5.  SRv6 SIDs and Reachability
...
269	   Locators are routable and MAY also be advertised in Prefix
270	   Reachability TLVs (236 or 237).

272	   Locators associated with Flexible Algorithms [I-D.ietf-lsr-flex-algo]
273	   SHOULD NOT be advertised in Prefix Reachability TLVs (236 or 237).

275	   Locators associated with algorithm 0 and 1 (for all supported
276	   topologies) SHOULD be advertised in a Prefix Reachability TLV (236 or
277	   237) so that legacy routers (i.e., routers which do NOT support SRv6)
278	   will install a forwarding entry for algorithm 0 and 1 SRv6 traffic.

280	   In cases where a locator advertisement is received in both a Prefix
281	   Reachability TLV and an SRv6 Locator TLV, the Prefix Reachability
282	   advertisement MUST be preferred when installing entries in the
283	   forwarding plane.  This is to prevent inconsistent forwarding entries
284	   between SRv6 capable and SRv6 incapable routers.

[] There is a confusing combination of normative behavior for the
locators in the last few paragraphs: "Locators...MAY also be
advertised...SHOULD NOT be advertised...SHOULD be advertised in a
Prefix Reachability TLV".  I realize that there are conditions
applied...perhaps reorder some of them.

Suggestion (to replace the last 4 paragraphs)>

   Locators associated with algorithm 0 and 1 (for all supported
   topologies) SHOULD be advertised in a Prefix Reachability TLV (236 or
   237) so that legacy routers (i.e., routers which do not support SRv6)
   will install a forwarding entry for algorithm 0 and 1 SRv6 traffic.

   In cases where a locator advertisement is received in both a Prefix
   Reachability TLV and an SRv6 Locator TLV, the Prefix Reachability
   advertisement MUST be preferred when installing entries in the
   forwarding plane.  This is to prevent inconsistent forwarding entries
   between SRv6 capable and SRv6 incapable routers.

   Locators associated with Flexible Algorithms (see Section 4,
   [I-D.ietf-lsr-flex-algo]) SHOULD NOT be advertised in Prefix Reachability
   TLVs (236 or 237).


[major] "locator advertisement is received in both a Prefix
Reachability TLV and an SRv6 Locator TLV"  What information results in
these being an advertisement for the same locator?  Is only the
locator (prefix length, etc.) considered, or should the algorithm,
metric, etc. also match?


[major] "the Prefix Reachability advertisement MUST be preferred when
installing entries in the forwarding plane"   Ok, but what about the
rest of the information in the SRv6 Locator TLV?  How should that be
considered?  For example, I'm assuming that the information in the
sub-TLVs is still needed.


[major] When is it ok to advertise locators in Prefix Reachability
TLVs when associated with Flexible Algorithms?  IOW, why it it only
recommended to do so and not required?  I assume the answer has to do
with the possibility of routers implementing flex-algo and not SRv6 in
the network -- please mention that.


[major] The SRv6 Locator TLV is a new TLV.  If no Prefix Reachability
TLVs are present, how should the new TLV be used for route
calculation/installation?  The text above suggests its use, but there
is no specification.


...
291	   SRv6 SIDs are not directly routable and MUST NOT be installed in the
292	   forwarding plane.  Reachability to SRv6 SIDs depends upon the
293	   existence of a covering locator.

[major] "MUST NOT be installed"  This action depends on how the SIDs
are advertised.  For example, if they are included in a Prefix
Reachability TLV then the receiver will install them.  IOW, this
action should be specified from the point of view of the sender; for
example, "SIDs MUST be advertised in the sub-TLVs...[or maybe]...MUST
NOT be advertised in another TLV...".


[major] If some SIDs are advertised as "sub-TLVs in TLVs 22, 23, 222,
223, and 141", how can the "MUST NOT be installed" be satisfied?


...
302	   In order for forwarding to work correctly, the locator associated
303	   with SRv6 SID advertisements MUST be the longest match prefix
304	   installed in the forwarding plane for those SIDs.  There are a number
305	   of ways in which this requirement could be compromised.  In order to
306	   ensure correct forwarding, network operators should take steps to
307	   make sure that this requirement is not compromised.

309	   o  Another locator associated with a different topology/algorithm is
310	      the longest match

312	   o  A prefix advertisement (i.e., from TLV 236 or 237) is the longest
313	      match

[major] "MUST be the longest match prefix installed"  s/MUST/must
This is not a normative statement.


[minor] The last two sentences in the paragraph sound redundant to me.

Suggestion>
   In order to ensure correct forwarding, network operators should take
   steps to make sure that this requirement is not compromised.  For
   example, the following situations should be avoided:


315	6.  Advertising Anycast Property
...
322	   A new flag in "Bit Values for Prefix Attribute Flags Sub-TLV"
323	   registry [RFC7794] is defined to advertise the anycast property:

[major nit] The flag is defined in the sub-TLV, not the registry.

NEW>
   A new flag in Prefix Attribute Flags Sub-TLV [RFC7794] is defined to
   advertise the anycast property:


...
328	       When the prefix/SRv6 locator is configured as anycast, the A-flag
329	       SHOULD be set. Otherwise, this flag MUST be clear.

[major] When is it ok to not set the flag if advertising an anycast
prefix?  IOW, why is setting the flag recommended and not required?


331	   The A-flag MUST be preserved when leaked between levels.

[minor] s/preserved when leaked/preserved when the advertisement is leaked


333	   The A-flag and the N-flag MUST NOT both be set.

335	   If both N-flag and A-flag are set in the prefix/SRv6 Locator
336	   advertisement, the receiving routers MUST ignore the N-flag.

[nit] Join the last two paragraphs.


338	   The same prefix/SRv6 Locator can be advertised by multiple routers.
339	   If at least one of them sets the A-Flag in its advertisement, the
340	   prefix/SRv6 Locator SHOULD be considered as anycast.

[major] "SHOULD be considered as anycast"  Just to make sure I
understand, the A-flag is informational -- there is no action taken by
the receiving router, right?   If so, then there's no interoperability
requirement reflected here.  s/SHOULD/should


342	   A prefix/SRv6 Locator that is advertised by a single node and without
343	   an A-Flag SHOULD be interpreted as a node specific locator.

[major] "advertised by a single node and without an A-Flag"  This is
equivalent to the current behavior of a prefix being "advertised by a
single node and without an A-Flag".  IOW, you seem to be specifying
behavior that a node that doesn't implement (or even know about) this
document is expected to follow.


[major] Related... What is a "node specific locator"?  The A-flag
functionality could be used in a network that otherwise doesn't
implement SRv6, so calling it a "locator" doesn't seem right.


[major] "SHOULD be interpreted"  Interpreting is not really an
interoperability-requiring action.  Is there anything here resulting
from the interpretation that requires normative language?


...
349	   The Prefix Attribute Flags Sub-TLV can be carried in the SRv6 Locator
350	   TLV as well as the Prefix Reachability TLVs.  When a router
351	   originates both the Prefix Reachability TLV and the SRv6 Locator TLV
352	   for a given prefix, and the router is originating the Prefix
353	   Attribute Flags Sub-TLV in one of the TLVs, the router SHOULD
354	   advertise identical versions of the Prefix Attribute Flags Sub-TLV in
355	   both TLVs.

[minor] This paragraph doesn't seem necessary given this text in §5:

   In cases where a locator advertisement is received in both a Prefix
   Reachability TLV and an SRv6 Locator TLV, the Prefix Reachability
   advertisement MUST be preferred when installing entries in the
   forwarding plane.


[major] If you decide to keep it...  "SHOULD advertise
identical...Prefix Attribute Flags Sub-TLV"   When is it ok to not do
so?  Again, given that the Prefix Reachability TLVs are preferred,
this statement doesn't seem to matter, or carry interoperability
weight.  s/SHOULD/should

[] This point is related to Gunter's recent e-mail [1].

[1] https://mailarchive.ietf.org/arch/msg/lsr/AlglGZ8UsZSaPGwwTeA1qyDWQL0/


...
365	7.1.  SRv6 Locator TLV Format
...
369	    0                   1                   2                   3
370	    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
371	   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
372	   |   Type        |     Length    |R|R|R|R|    MTID               |
373	   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

[minor] Please add Figure numbers/names for all packet formats.


...
382	     MTID: Multitopology Identifier as defined in [RFC5120].
383	     Note that the value 0 is legal.

[major] s/MTID/MT ID/g


[] Is the note necessary?


[major] What should the receiver do if the MT ID is outside the valid range?


...
403	       0
404	       0 1 2 3 4 5 6 7
405	      +-+-+-+-+-+-+-+-+
406	      |D|    Reserved |
407	      +-+-+-+-+-+-+-+-+

[major] How should these reserved flags be managed?  Please create a registry.


409	      where:
410	        D bit: When the Locator is leaked from level-2 to level-1, the D
411	        bit MUST be set.  Otherwise, this bit MUST be clear.  Locators
412	        with the D bit set MUST NOT be leaked from level-1 to level-2.
413	        This is to prevent looping.

[major] Is there any difference between this bit and the up/down bit
defined in rfc5305?   To reuse functionality, generalize (to multiple
levels), and avoid redefining please point to the definition in
rfc5305.


...
418	     Algorithm: 1 octet. Associated algorithm. Algorithm values
419	      are defined in the IGP Algorithm Type registry.

[major] Use this text for all the Algorithm fields.

NEW>
   Algorithm: 1 octet. As defined in rfc8665.

Add a Normative reference.


421	     Loc-Size: 1 octet. Number of bits in the SRv6 Locator field.
422	     (1 - 128)

[major] What should the receiver do if the Loc-Size is not in that range?


...
429	     Sub-TLV-length: 1 octet. Number of octets used by sub-TLVs

[nit] Note that the name of the field in the packet format shown above
is written as "Sub-tlv-len".  Please be consistent!


[major] Please indicate which sub-TLVs are applicable/can be included
with this new TLV.  I know that §12.1.2 updates the registry, but that
is not normative with respect to the operation of the protocol.  This
seems like a simple (one-sentence) addition since all the sub-TLVs
seem applicable.


431	     Optional sub-TLVs.

[] This seems to be leftover text.


433	7.2.  SRv6 End SID sub-TLV

435	   The SRv6 End SID sub-TLV is introduced to advertise SRv6 Segment
436	   Identifiers (SID) with Endpoint behaviors which do not require a
437	   particular neighbor in order to be correctly applied
438	   [I-D.ietf-spring-srv6-network-programming].  SRv6 SIDs associated
439	   with a neighbor are advertised using the sub-TLVs defined in
440	   Section 8.

[minor] "which do not require a particular neighbor in order to be
correctly applied"  Which behaviors are those?   I didn't find in
rfc8986 any similar phrasing.  You may want to put a reference to
Section 10 instead.


...
470	      Flags: 1 octet.  No flags are currently defined.

[minor] How will these flags be managed?  Do you want to define a new registry?


472	      Endpoint Behavior: 2 octets, as defined in [I-D.ietf-spring-srv6-
473	      network-programming].  Legal behavior values for this sub-TLV are
474	      defined in Section 10 of this document.

[major] What should the receiver do if an illegal value is received?


[nit] s/Legal/Allowed


...
478	      Sub-sub-TLV-length: 1 octet.  Number of octets used by sub-sub-
479	      TLVs.

[nit] Note that the name of the field in the packet format shown above
is written as "Sub-sub-tlv-len".  Please be consistent!


481	      Optional sub-sub-TLVs.

[] Leftover text.


483	   The SRv6 End SID MUST be a subnet of the associated Locator.  SRv6
484	   End SIDs which are NOT a subnet of the associated locator MUST be
485	   ignored.

[minor] s/NOT/not/g   This is not an rfc2119 keyword.


...
494	8.  Advertising SRv6 Adjacency SIDs
...
508	   All End.X SIDs MUST be a subnet of a Locator with matching topology
509	   and algorithm which is advertised by the same node in an SRv6 Locator
510	   TLV.  End.X SIDs which do not meet this requirement MUST be ignored.

512	   All End.X and LAN End.X SIDs MUST be subsumed by the subnet of a
513	   Locator with the matching algorithm which is advertised by the same
514	   node in an SRv6 Locator TLV.  End.X SIDs which do not meet this
515	   requirement MUST be ignored.  This ensures that the node advertising
516	   the End.X or LAN End.X SID is also advertising its corresponding
517	   Locator with the algorithm that will be used for computing paths
518	   destined to the SID.

[minor] There is repetition in the last two paragraphs.


[minor] "All End.X SIDs..."  It looks like you're talking about all
SIDs that can be included in the End.X or LAN End.X sub-TLVs, and not
just about End.X (the behavior) SIDs, is that correct?   Please be
specific.


520	8.1.  SRv6 End.X SID sub-TLV
...
561	         B-Flag: Backup flag.  If set, the End.X SID is eligible for
562	         protection (e.g., using IPFRR) as described in [RFC8355].

[minor] "End.X SID"   As mentioned above, the use of this terminology
creates confusion because you should be talking about the SID (in the
End.X SID sub-TLV) and not the End.X SID.


564	         S-Flag.  Set flag.  When set, the S-Flag indicates that the
565	         End.X SID refers to a set of adjacencies (and therefore MAY be
566	         assigned to other adjacencies as well).

[major] What should a receiver do if a SID is advertised in multiple
sub-TLVs (associated to different adjacencies) without the S-Flag set?
  What about the case where the SID was first advertised without the
S-Flag set and a second sub-TLV (for a different adjacency) has it
set?  Is this Flag only informational?


...
573	         Reserved bits: MUST be zero when originated and ignored when
574	         received.

[major] How should these bits be managed?  Please set up a new registry.


...
579	      Weight: 1 octet.  The value represents the weight of the End.X SID
580	      for the purpose of load balancing.  The use of the weight is
581	      defined in [RFC8402].

[major] What is the relationship between the Weight and the S-Flag?
If the S-Flag is set (in multiple sub-TLVs for different adjacencies),
should a Weight value also be present?   What should the receiver do
if only one of the sub-TLVs includes a weight?


583	      Endpoint Behavior: 2 octets.  As defined in [I-D.ietf-spring-srv6-
584	      network-programming] Legal behavior values for this sub-TLV are
585	      defined in Section 10.

[major] What should the receiver do if an illegal value is received?


[nit] s/Legal/Allowed


[nit] s/] Legal/]. Legal


...
596	8.2.  SRv6 LAN End.X SID sub-TLV

[] Some of the comments form the previous section apply here as well.


598	   This sub-TLV is used to advertise an SRv6 SID associated with a LAN
599	   adjacency.  Since the parent TLV is advertising an adjacency to the
600	   Designated Intermediate System(DIS) for the LAN, it is necessary to
601	   include the System ID of the physical neighbor on the LAN with which
602	   the SRv6 SID is associated.  Given that a large number of neighbors
603	   may exist on a given LAN a large number of SRv6 LAN END.X SID sub-
604	   TLVs may be associated with the same LAN.  Note that multiple TLVs
605	   for the same DIS neighbor may be required in order to advertise all
606	   of the SRv6 End.X SIDs associated with that neighbor.

[nit] s/System(DIS)/System (DIS)


...
666	9.  SRv6 SID Structure Sub-Sub-TLV

[] I don't understand what this sub-sub-TV is used for.  Can you
please explain?  Is there a relationship between it and the SID that
is advertised in the sub-TLVs?  For example, I would assume that the
SID would have the bits that correspond to the argument set to 0 --
what if they're not?  What is the purpose of this information?   [Of
course, none of the supported behaviors take an ARG...]


...
707	   The sum of all four sizes advertised in ISIS SRv6 SID Structure Sub-
708	   Sub-TLV must be lower or equal to 128 bits.  If the sum of all four
709	   sizes advertised in the ISIS SRv6 SID Structure Sub-Sub-TLV is larger
710	   than 128 bits, the parent Sub-TLV MUST be ignored by the receiver.

[major] s/must be lower/MUST be lower


712	10.  Advertising Endpoint Behaviors

714	   Endpoint behaviors are defined in
715	   [I-D.ietf-spring-srv6-network-programming].  The codepoints for the
716	   Endpoint behaviors are defined in the "SRv6 Endpoint Behaviors"
717	   registry defined in [I-D.ietf-spring-srv6-network-programming].  This
718	   section lists the Endpoint behaviors which MAY be advertised by ISIS,
719	   together with their codepoints.  If this behavior is advertised it
720	   MUST only be advertised in the TLV[s] as indicated by "Y" in the
721	   table below, and MUST NOT be advertised in the TLV[s] as indicated by
722	   "N" in the table below.

[minor] The second sentence is redundant (and inaccurate), please take it out.


[major] What about other behaviors from rfc8986?  If they are not
applicable, please explain why.


724	     Endpoint             |Endpoint          | End | End.X | Lan End.X |
725	     Behavior             |Behavior Codepoint| SID | SID   |   SID     |
726	    ----------------------|------------------|-----|-------|-----------|
727	     End   (PSP, USP, USD)| 1-4, 28-31       |  Y  |   N   |    N      |
728	    ----------------------|------------------|-----|-------|-----------|
729	     End.X (PSP, USP, USD)| 5-8, 32-35       |  N  |   Y   |    Y      |
730	    ----------------------|------------------|-----|-------|-----------|
731	     End.DX6              | 16               |  N  |   Y   |    Y      |
732	    ----------------------|------------------|-----|-------|-----------|
733	     End.DX4              | 17               |  N  |   Y   |    Y      |
734	    ----------------------|------------------|-----|-------|-----------|
735	     End.DT6              | 18               |  Y  |   N   |    N      |
736	    ----------------------|------------------|-----|-------|-----------|
737	     End.DT4              | 19               |  Y  |   N   |    N      |
738	    ----------------------|------------------|-----|-------|-----------|
739	     End.DT64             | 20               |  Y  |   N   |    N      |

[minor] s/End.DT64/End.DT46



741	11.  Implementation Status
...
752	      Types of SID supported: End, End.X, LAN End.X, END.OP

[] "END.OP" is not defined.  Also, the others are not types of SIDs,
but sub-TLVs.


...
808	12.1.  SRv6 Locator TLV

810	   This document makes the following registrations in the the IS-IS TLV
811	   Codepoints registry:

813	      Type: 27

815	      Description: SRv6 Locator TLV.

817	      Reference: This document (Section 7.1).

[major] Include all the information related to the message types (IIH,
LSP, SNP, Purge).


819	   A Locator TLV shares sub-TLV space with existing "Sub-TLVs for TLVs
820	   135, 235, 236 and 237 registry".  The name of this registry needs to
821	   be changed to "Sub-TLVs for TLVs 27, 135, 235, 236 and 237 registry".

[major] Use the complete name of the registry.

NEW>
   The SRv6 Locator TLV shares sub-TLV space with TLVs 135, 235, 236 and 237.
   IANA is requested to update the name of the "Sub-TLVs for TLVs 135, 235,
   236, and 237 (Extended IP reachability, MT IP. Reach, IPv6 IP. Reach, and
   MT IPv6 IP. Reach TLVs)" registry to "Sub-TLVs for TLVs 27, 135, 235, 236,
   and 237 (SRv6 Locator, Extended IP reachability, MT IP. Reach, IPv6 IP.
   Reach, and MT IPv6 IP. Reach TLVs)".

Also, this action (renaming) should be moved to a common section with
the new SRv6 End SID sub-TLV (§12.1.1) and the updated table
(§12.1.2).  The action of allocating the SRv6 Locator TLV is
independent (different registry, etc.).


...
834	12.1.2.  Revised sub-TLV table
...
839	      Type  27 135 235 236 237

841	      1     y   y   y   y   y
842	      2     y   y   y   y   y
843	      3     n   y   y   y   y
844	      4     y   y   y   y   y
845	      5     y   n   n   n   n
846	      6     n   y   y   y   y
847	      11    y   y   y   y   y
848	      12    y   y   y   y   y
849	      32    n   y   y   y   y

[major] Because the structure of the registry is changed, this
document should formally Update rfc7370 (where the current registry
was defined).


[minor] Please name and number all tables.


851	12.2.  SRv6 Capabilities sub-TLV

853	   This document makes the following registrations in the "Sub- TLVs for
854	   TLV 242 registry":

[major] s/"Sub- TLVs for TLV 242 registry"/"Sub-TLVs for TLV 242
(IS-IS Router CAPABILITY TLV)" registry


...
862	   This document requests the creation of a new IANA managed registry
863	   for sub-sub-TLVs of the SRv6 Capability sub-TLV.  The registration
864	   procedure is "Expert Review" as defined in [RFC8126].  Suggested
865	   registry name is "sub-sub-TLVs for SRv6 Capability sub-TLV".  No sub-
866	   sub-TLVs are defined by this document except for the reserved value.

[major] Please indicate tat it should be part of "IS-IS TLV Codepoints".


[major] ""Expert Review" as defined in [RFC8126]"  Please add that the
guidance for the Designated Experts is provided in rfc7310.


...
872	12.3.  SRv6 End.X SID and SRv6 LAN End.X SID sub-TLVs

874	   This document makes the following registrations in the "sub- TLVs for
875	   TLV 22, 23, 25, 141, 222 and 223 registry":

[major] s/"sub- TLVs for TLV 22, 23, 25, 141, 222 and 223
registry"/"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)"

Yes, I know that's a long name, but that is the name.


...
894	12.4.  MSD Types

896	   This document makes the following registrations in the IGP MSD Types
897	   registry:

[minor] s/MSD Types/MSD-Types


899	   Type  Description
900	   ------------------
901	    41    SRH Max SL
902	    42    SRH Max End Pop
903	    44    SRH Max H.encaps
904	    45    SRH Max End D

[minor] This table should have 3 columns: Value, Name and Reference.


906	12.5.  Sub-Sub-TLVs for SID Sub-TLVs

908	   This document requests a new IANA registry be created under the IS-IS
909	   TLV Codepoints Registry to control the assignment of sub-TLV types
910	   for the SID Sub-TLVs specified in this document - Section 7.2,
911	   Section 8.1, Section 8.2.  The suggested name of the new registry is
912	   "Sub-Sub-TLVs for SID Sub-TLVs".  The registration procedure is
913	   "Expert Review" as defined in [RFC8126].  The following assignments
914	   are made by this document:

[minor] In line with the name of other registries; suggestion:
"Sub-sub-TLVs for sub-TLVs 5, 43 and 44 (SRv6 End SID , SRv6 End.X SID
and SRv6 LAN End.X SID)".


[major] ""Expert Review" as defined in [RFC8126]"  Please add that the
guidance for the Designated Experts is provided in rfc7310.


916	      0: Reserved

918	      1: SRv6 SID Structure Sub-Sub-TLV (Section 9).

[minor] Please create a table and include the unassigned range.


920	12.6.  Prefix Attribute Flags Sub-TLV
...
927	      Description: A bit

[major] Can we please have the registry show "Anycast Flag (A-flag)"?


...
931	13.  Security Considerations

933	   Security concerns for IS-IS are addressed in [ISO10589], [RFC5304],
934	   and [RFC5310].

<sigh> Well, at least it is not explicitly claimed that no new
security considerations exist. <sigh>


[minor] Please copy the first paragraph of the Security Consideration
from rfc8919.


[major] Let's add some more meat.

Suggestion>
   This document describes the IS-IS extensions required to support Segment
   Routing over an IPv6 data plane.  The security considerations for Segment
   Routing are discussed in [RFC8402].  [RFC8986] defines the SRv6 Network
   Programming concept and specifies the main Segment Routing behaviors to
   enable the creation of interoperable overlays; the security considerations
   from that document apply too.


[major] Are there new security issues/risks created by this document?
Sure!  Note that even if the risk existed before, adding new ways to
exploit it is a new attack vector.

(1) Suggestion>
   The advertisement of an incorrect MSD value may have negative
   consequences, see rfc8491 for additional considerations.


(2) The interaction in §5 between the Prefix Reachability TLV and SRv6
Locator TLV is not completely clear.  Depending on the answers to my
questions, there may be cases where inconsistent information can be
present resulting in unexpected forwarding.


(3) Not following the operational considerations at the end of §5
could also result in unexpected forwarding.

What else?


...
982	15.1.  Normative References
...
997	   [ISO10589]
998	              Standardization", I. ". O. F., "Intermediate system to
999	              Intermediate system intra-domain routeing information
1000	              exchange protocol for use in conjunction with the protocol
1001	              for providing the connectionless-mode Network Service (ISO
1002	              8473), ISO/IEC 10589:2002, Second Edition.", Nov 2002.

[minor] Please use this as the reference:

[ISO10589]

    ISO, "Information technology -- Telecommunications and information
    exchange between systems -- Intermediate System to Intermediate System
    intra-domain routeing information exchange protocol for use in
    conjunction with the protocol for providing the connectionless-mode
    network service (ISO 8473)", ISO/IEC 10589:2002, Second Edition,
    November 2002.


...
1015	   [RFC5304]  Li, T. and R. Atkinson, "IS-IS Cryptographic
1016	              Authentication", RFC 5304, DOI 10.17487/RFC5304, October
1017	              2008, <https://www.rfc-editor.org/info/rfc5304>.

[minor] This reference can be Informative.


...
1023	   [RFC5310]  Bhatia, M., Manral, V., Li, T., Atkinson, R., White, R.,
1024	              and M. Fanto, "IS-IS Generic Cryptographic
1025	              Authentication", RFC 5310, DOI 10.17487/RFC5310, February
1026	              2009, <https://www.rfc-editor.org/info/rfc5310>.

[minor] This reference can be Informative.


...
1067	15.2.  Informative References

1069	   [I-D.ietf-lsr-flex-algo]
1070	              Psenak, P., Hegde, S., Filsfils, C., Talaulikar, K., and
1071	              A. Gulko, "IGP Flexible Algorithm", draft-ietf-lsr-flex-
1072	              algo-12 (work in progress), October 2020.

[major] This reference should be Normative.

...
1080	   [RFC8402]  Filsfils, C., Ed., Previdi, S., Ed., Ginsberg, L.,
1081	              Decraene, B., Litkowski, S., and R. Shakir, "Segment
1082	              Routing Architecture", RFC 8402, DOI 10.17487/RFC8402,
1083	              July 2018, <https://www.rfc-editor.org/info/rfc8402>.

[major] This reference should be Normative.

[End of Review]