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

[Peter Psenak <[e-mail]>]

Hi Alvaro,

please see inline (##PP2):


On 16/03/2021 21:33, Alvaro Retana wrote:
> On March 11, 2021 at 5:46:51 AM, Peter Psenak wrote:
> 
> 
> Peter:
> 
> Hi!
> 
>> thanks for the review, please see inline (##PP):
> 
> It looks like you didn't get the whole review (Outlook bug).  Take a
> look at it here:
> https://mailarchive.ietf.org/arch/msg/lsr/a4a4I4fP73DyfKsdKnRw_tRuStQ/

##PP2

sigh...

I added the rest of the comments to the end of this email and responded 
inline to them.

> 
> 
> 
> ...
>>> 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.
>>
>> ##PP
>> not all behaviors from rfc8986 are applicable to IGPs. Section 10
>> ("Advertising Endpoint Behaviors") lists the ones that are applicable to
>> ISIS.
> 
> I understand that -- other readers may not.

##PP2
we defined all behaviors that rfc8986 mentions should be advertised by 
IGP, except the END.T. The END.T was originally defined, but during the 
WGLC it was removed based on WG discussion:

Mailing list discussion:
Thread1: 
https://mailarchive.ietf.org/arch/msg/lsr/0Bp5DJrRJPvRyzZMZS0P_OE8Y7Q/
Thread2: 
https://mailarchive.ietf.org/arch/msg/lsr/nKJbY5f6SHEwVCqqfoXSPidGKXQ/




> 
> Also, §8.1/rfc8986 talks about what is expected to be carried in the
> IGP.  End.T is not included in this document.
> 
> 
> ...
>>> 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] "router capabilities TLV [RFC7981]" What should be the
>>> flooding scope of the TLV that includes this new sub-TLV?
>>
>> ##PP
>> It's up to the originator to set the S bit or not. We are not limiting
>> it here.
> 
> But if the originator doesn't set the correct flooding scope then the
> information will not make it to where is has to.  I would like to see
> some guidance on the setting of the bit -- I'm assuming that most of
> the cases require the S bit to be set, right?

##PP2
not really. There is no need for the SRv6 Capabilities sub-TLV to be 
propagated across the area for now. If it will become required later, 
the S-bit is there. We don't really want to specify what is required 
because that may change over time. We are defining it in a way that both 
area and domain wide scope is possible. It's up to the user to pick 
which one does he need.

> 
> 
> ...
>>> 200 4.1. Maximum Segments Left MSD Type
> ...
>>> 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.
>>>
>>
>> ##PP
>>
>> Yes, you are correct. As described in the paragraph right above it, this
>> MSD type specifies what is the maximum value of the "Segments Left"
>> field in the received SRH that this node is capable of processing.
>> A simple example: an ingress PE encapsulates a packet with a new IPv6
>> and SRH. The SRH contains three segments. The Segments Left value of
>> that SRH is set as per RFC8754 4.1, which in this case is equal to 2.
>> The router processing the first segment in the segment list, must
>> support as a minimum an SRH Max SL MSD value of 2 in order to be able to
>> process such packet.
> 
> I see.  It would be very nice if the text said somewhere that the MSD
> applies not just to the final endpoint but also to intermediate nodes.

##PP2
well, I find that a bit misleading. The "router processing the first 
segment" is the one that applies the Endpoint behavior
associated with such SID.

> 
> 
> ...
>>> 252 5. SRv6 SIDs and Reachability
> ...
>>> 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.
> ...
>>> [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?
>>
>> ##PP
>> it's just prefix/mask. I added some text to clarify that.
> 
> So...even if the MT-ID and algorithms are different, the
> advertisements are considered the same?   I don't have a specific case
> right now, but it sounds to me that this could result in some type of
> incongruency.   I'll think more about this later -- no further action
> needed.


##PP2
Prefix Reachability TLV does not have the algo field, so it is 
implicitly associated with Algo 0. Yes, MT-ID needs to be considered, I 
have clarified that in the text.


> 
> 
> ...
>>> [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.
>>
>> #PP
>> The locator TLV advertises the prefix, same way as the Prefix
>> Reachability. The calculation and installation of the prefix
>> reachability is equal to what is done for regular Prefix Reachability.
>> I added the following text to one of the above paragraphs:
>>
>> "The processing of the prefix advertised in the SRv6 Locator TLV,
>> the calculation of its reachability and the installation in the
>> forwarding plane is similar to one used for Prefix Reachability TLV (236
>> or 237)."
> 
> s/is similar to one used for Prefix Reachability TLV (236 or
> 237)./follows the process defined for the Prefix Reachability TLV (236
> or 237) [rfc5308].
> 
> Would that be ok?

##PP2
done

> 
> 
> 
>>> 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;
>>
>> ##PP
>> This section talks about received SRv6 SIDs, not locators. SIDs are not
>> advertised in Prefix Reachability TLV. Remote SIDs are never installed
>> in the forwarding.
>>
>> I updated the text to say "SRv6 SIDs received from other nodes..."
>>
>> for
>>> example, "SIDs MUST be advertised in the sub-TLVs...[or maybe]...MUST
>>> NOT be advertised in another TLV...".
>>
>> ##PP
>> the previous section talks about how SIDs are advertised, which is the
>> only way.
> 
> Just to make sure we're talking about the same thing.  The SIDs are
> covered by the LOC, which is routable, installed in the RIB/FIB, etc.

##PP2
yes

> 
> There's nothing in this document or rfc8986 that prevents a SID to
> have the same value as the IP address on a link (see below).  Then
> that link address could be advertised using normal (non-SR-related)
> mechanisms.

##PP2
I'm not sure how one can use the same 128 bit IPv6 address as a local 
address and as a SID at the same time.


> 
> In this case, the receiver would know the address is also a SID.
> Should that result in "MUST NOT be installed"?   Not even the MUSTs
> that I suggested could prevent this situation.

##PP2
regardless of the validity of the case, this section only talks about 
the remote SID. We are not prohibiting the same address advertised 
otherwise to be installed in forwarding in any way.

> 
> 
>>> [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?
>>
>> ##PP
>> above is the only way how SIDs are advertised.
> 
> This is the text from -11:
> 
>     SRv6 SIDs are advertised as sub-TLVs in the SRv6 Locator TLV except
>     for SRv6 End.X SIDs/LAN End.X SIDs which are associated with a
>     specific Neighbor/Link and are therefore advertised as sub-TLVs in
>     TLVs 22, 23, 222, 223, and 141.
> 
>     SRv6 SIDs are not directly routable and MUST NOT be installed in the
>     forwarding plane.  Reachability to SRv6 SIDs depends upon the
>     existence of a covering locator.
> 
> The text says that there is more than one way to advertise SIDs, *and*
> that they "MUST NOT be installed".

##PP2
there are two ways to advertise SRv6 SIDs in ISIS depending of the SID 
type, but only one way to advertise any particular SID type. So there is 
no duplication possible. I thought the text above is clear on that.

---


...
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?

##PP2
The A-flag is an optional flag. It's hard to mandate the setting of it 
given that anycast has been used for years and we are introducing the 
flag just now.


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

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

##PP2
done



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.

##PP2
done


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


##PP2
there can be action on A-flag. For example, you do not want to use the 
SID associated with the anycast Prefix/Locator for TI-LFA or uloop 
prevention. We do not specify the usage here, because the usage is 
optional, but I would keep the SHOULD here.


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.

##PP2
if I remove the "prefix" and only keep the SRv6 Locator, would you be 
fine with it? We are defining SRv6 Locator in this document.


[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.

##PP2
please see my previous response.


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

##PP2
what about:

    "An SRv6 Locator that is advertised by a single node and without
     an A-Flag is considered as a node specific locator."


...
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.

##PP2
above mentioned paragraph does not say anything about the Prefix 
Attribute Flags Sub-TLV present in the advertisement of the same prefix 
in the Locator TLV and Prefix Reachability TLV. So we need to keep it.



[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

##PP
well, not really. The Prefix Attribute Flags Sub-TLV should be the same 
to guarantee the same treatment of both locator and legacy prefix 
advertisements. The fact that the legacy prefix advertisement is 
preferred when installing reachability of the prefix to forwarding does 
not mean the Prefix Attribute Flags Sub-TLV advertised with Locator TLV 
is useless - it can still be used when using Locator for other things - 
e.g. derive SID for TILFA protection, etc.


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

##PP2
the text has been updated to address Gunter's comment as follows:

"The Prefix Attribute Flags Sub-TLV can be carried in the SRv6
  Locator TLV as well as the Prefix Reachability TLVs. When a router 
originates both the Prefix Reachability TLV and the SRv6 Locator TLV for 
a given prefix, and the router is originating the Prefix Attribute Flags 
Sub-TLV in one of the TLVs, the router SHOULD advertise same flags in 
the Prefix Attribute Flags Sub-TLV in both TLVs. However, unlike TLVs 
236/237 the X-flag in the Prefix Attributes Flags sub-TLV is valid when 
sent in the SRv6 Locator TLV. The state of the X-flag in the Prefix 
Attributes Flags sub-TLV when included in the Locator TLV MUST match the 
setting of the embedded X-flag in any advertisement of the same prefix 
in TLVs 236/237."


[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.

##PP2
s that really required? I have never done it in any of RFCs I was editor 
for.


...
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?

##PP2
I would keep it to, as RFC5120 prevents 0 to be used for TLVs that are 
defined in RFC5120 itself.


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

##PP2
MT-ID is defined as 12 bits, so there is no outside range here.



...
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.

##PP2
I'm following what the WG has been doing so far - no registry for flags 
field. If the WG decides to change its policy on this matter, I will 
update the document with the registries.



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.

##PP2
done


...
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.

##PP2
done


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?


##PP2
Added:

"MUST be from the range (1 - 128). The TLV MUST be ignored if the 
Loc-Size is outside of this 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!

##PP2
fixed


[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.

##PP2:

Added:
Optional sub-TLVs: Sub-TLVs 1, 2, 4, 5, 11, 12 are allowed. Any other 
Sub-TLVs MUST be ignored.


431	     Optional sub-TLVs.

[] This seems to be leftover text.

##PP2
modified as above.


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.

##PP2
This is ISIS specific categorization, not something specified in 
rfc8986. I removed the reference to rfc8986 and added reference to 
section 10.


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

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

##PP2
I'm following what the WG has been doing so far - no registry for flags 
field. If the WG decides to change its policy on this matter, I will 
update the document with the registries.



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

##PP2
I have changed the text to:

"Supported behavior values for this sub-TLV are defined in Section 10
  of this document. Unsupported or unrecognized behavior values are 
ignored by the receiver."

...
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!

##PP2
fixed


481	      Optional sub-sub-TLVs.

[] Leftover text.

##PP2
I don't think it is leftover text, the list described the content of the 
SRv6 End SID sub-TLV, and "Optional sub-sub-TLVs" are part of it.



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.

##PP2
fixed



...
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.


##PP2
correct. Avoided duplication and updated to 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.

##PP2
I have fixed that across the whole document


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?

##PP2
it is informational. It can be used to avoid picking the shared SIDs for 
things like TILFA or uloop avoidance. It has no impact on the way the 
SID behaves.



...
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.

##PP2
I'm following what the WG has been doing so far - no registry for flags 
field. If the WG decides to change its policy on this matter, I will 
update the document with the registries.

...
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?

##PP2
The usage of weight is described in rfc8402. We do not define it here. 
We are only defining the way it is advertised. We did the same in rfc8667.


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

##PP2
I have changed the text to:

"Supported behavior values for this sub-TLV are defined in Section 10
  of this document. Unsupported or unrecognized behavior values are 
ignored by the receiver."


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

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

##PP2
fixed them similarly.


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)

##PP2
fixed



...
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...]

##PP2
The SRv6 SID Structure Sub-Sub-TLV indicates the structure of the SID 
associated with it. It can be used by implementation for validation of 
the SID for consistency (e.g. if there is no ARG but there is something 
in the ARG bits, then it can be ignored). They can be signalled via 
BGP-LS to controller/apps that can verify the consistency in the block 
and SID addressing in the domain. Details are outside the scope of this 
draft.



...
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

##PP2
fixed


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.

##PP2
removed


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

##PP2
section 8.4 of rfc8986 only list subset of behaviors which are to be 
advertised by IGPs. We defined them all except END.T. as I described 
earlier.






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

##PP2
fixed



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.

##PP2
removed END.OP. This section is going to be removed anyway.



...
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).

##PP2
done


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.

##PP2
done

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.).

##PP2
done


...
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).

##PP2
I added following text:

"This document updates 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 defined in [RFC7370] to section 
§12.1.1."

Would that be sufficient?


[minor] Please name and number all tables.

##PP2
is that really required? I have never done it in any of RFCs I was 
editor for.



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

##PP2
done


...
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".

##PP2
done


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

##PP2
done


...
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.

##PP2
done


...
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

#PP2
done

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.

##PP2
done


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)".


##PP2
I find that confusing as the sub-TLVs 5 is a locator Sub-TLV, while 
Sub-TLVs 43 and 44 are IS reachability sub-TLVs.
What about:

"Sub-Sub-TLVs for SID Sub-TLVs (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.

##PP2
done


916	      0: Reserved

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

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

##PP2
done


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

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

##PP2
done

...
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.

##PP2
done


[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.

##PP
added


[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.
##PP2
added

(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.

##PP2
I don't see the security risk there. Traffic may be forwarded to a node 
where the SID does not exist and be dropped there, but that is not a 
security risk.


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

##PP2
yes, but that is not a security concern. Similar to providing 
overlapping static routes pointing to each other. Is that a security 
risk? No, it's a misconfiguration.

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.

##PP2
done


...
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.

##PP2
done


...
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.

##PP2
done


...
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.

##PP2
done


...
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.

##PP2
done

[End of Review]