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

From: Peter Psenak <[email-masked]>
Date: Tue, 6 Apr 2021 13:54:24 +0200

Hi Alvaro,

please see inline (##PP3):

On 30/03/2021 19:44, Alvaro Retana wrote:
> On March 25, 2021 at 6:03:53 AM, Peter Psenak wrote:
> 
> 
> Peter:
> 
> Hi!
> 
> I have some comments (see below) -- nothing major.  I look forward to -12.
> 
> Thanks!
> 
> Alvaro.
> 
> 
> 
> 
> ...
>>>>> 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/
> 
> Please just say somewhere that END.T is not included -- no need to
> justify in the document.

##PP3
done
> 
> 
> ...
>> 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.
> 
> No.  The description throughout this section talks about both prefixes
> and locators.  It reads as a general specification for all prefixes
> (including locators).
> 
> Suggestion (taking onto account the responses below)>
> 
>    A prefix/SRv6 Locator that is advertised by a single node and without
>    an A-Flag is considered node specific.

##PP3
done
> 
> 
>> [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.
> 
> Does this mean that having the same sub-TLV is also a consideration
> when comparing the locator and legacy advertisements?   If so, then
> that should also be mentioned in §5.
> 
> 
>> [] 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."
> 
> [nit] s/advertise same flags/advertise the same flags

##PP3
done

> 
> [nit] s/ 236/237 / 236 and 237
>

##PP3
done

> [major] "MUST match the setting of the embedded X-flag"
> 
> What if the setting doesn't match?  What should the receiver do?

##PP3
Added following:

"In case of any inconsistency between the Prefix Attribute
  Flags advertised in the Locator TLV and in the Prefix Reachability TLV,
  the ones advertised in Prefix Reachability TLV MUST be preferred".

> 
> By "embedded", you are referring to the X bit inside 236/237, right?

##PP3
yes

> To differentiate, please call it "X bit" and add references to
> rfc5120/5308.

##PP3
done
> 
> 
> 
> ...
>> [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.
> 
> Required, no...but it is good practice.
> 
> 
> ...
>> 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.
> 
> Can you please add something like that to the draft?  I think that for
> now (unless other people ask as well) simply saying that the use is
> outside the scope should be enough.

##PP3
done
> 
> 
> ...
>> 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.
> 
> That fact doesn't mean that the information in it can be wrong.  If
> inaccurate, I would prefer the section not be there at all.

##PP3
I removed the "Implementation Status" section, it would have to be 
removed later anyway.

> 
> 
> ...
>> 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:
>>
>> "		 "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?
> 
> We need the header to include the Updates tag, 

##PP3
sorry, I'm not sure I get it, what header and what "Updates tag"?

> and something in the
> Abstract and Introduction.
> 
> In the Abstract>
>     This documents updates rfc7370 by modifying an existing registry.

##PP3
done
> 
> 
> The text in Introduction can be the same, with a reference to §12.1.2.

##PP3
done
> 
> This Section should also ask IANA to add this document as a reference
> in the regsitry.

##PP3
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)"
> 
> Most of the other registries include the number of the TLV.  So I
> think it would be good to remain consistent.  Maybe we should ask the
> current DEs: Chris, Hannes and Les.

##PP3
I have updated this based on the discussion with Les.

thanks,
Peter

> [End]

---

From: Alvaro Retana <[email-masked]>
Date: Thu, 8 Apr 2021 18:17:07 -0400

Peter:

Hi!

I looked at -12.

I have a couple of nits/minor comments below.  There's only one
significant one related to the information that must be shared between
the Prefix Reachability TLV and the SRv6 Locator TLV: it is currently
phrased as an example.

We're also waiting of the resolution of the registry thread.  If that
results in not needed to add registries then you can address the
comments below with any other IETF LC comments.  Otherwise I'll wait
for an update.

Thanks!

Alvaro.

[Line numbers from idnits.]

...
16	Abstract
...
25	   This documents updates [RFC7370] by modifying an existing registry.

[minor] s/[RFC7370]/RFC 7370
No references in the Abstract.

...
102	1.  Introduction
...
137	   This documents updates [RFC7370] by modifying an existing registry
138	   Section 11.1.2.

[nit] s/Section 11.1.2/(Section 11.1.2)

...
192	4.1.  Maximum Segments Left MSD Type

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

[minor] s/specifies/signals

...
229	4.4.  Maximum End D MSD Type

231	   The Maximum End D MSD Type specifies the maximum number of SIDs
232	   present in an SRH when performing decapsulation.  These includes, but
233	   not limited to, End.DX6, End.DT4, End.DT46, End with USD, End.X with
234	   USD as defined in [RFC8986]).

[nit] s/[RFC8986])/[RFC8986]

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

[minor] s/NOT/not
This is not an rfc2119 keyword -- and someone else will ask for the same thing.

268	   In cases where a locator advertisement is received in both a Prefix
269	   Reachability TLV and an SRv6 Locator TLV - (e.g. prefix, prefix-
270	   length, MTID all being equal and Algorithm being 0 in Locator TLV),
271	   the Prefix Reachability advertisement MUST be preferred when
272	   installing entries in the forwarding plane.  This is to prevent
273	   inconsistent forwarding entries between SRv6 capable and SRv6
274	   incapable routers.  Such preference of Prefix Reachability
275	   advertisement does not have any impact on the rest of the data
276	   advertised in the SRv6 Locator TLV.

[major] "e.g. prefix, prefix-length, MTID all being equal and
Algorithm being 0 in Locator TLV"

This text should not be an example because those are the fields that
should match.  Please make it clear: "The locator advertisement is
both TLVs is considered the same when the following fliends match..."
(or something like that with better words).

...
866	11.5.  Sub-Sub-TLVs for SID Sub-TLVs

868	   This document requests a new IANA registry be created under the IS-IS
869	   TLV Codepoints Registry to control the assignment of sub-TLV types
870	   for the SID Sub-TLVs specified in this document - Section 7.2,
871	   Section 8.1, Section 8.2.  The suggested name of the new registry is
872	   "sub-sub-TLVs for SRv6 End SID (5) (sub-TLV of TLVs 27, 135, 235, 236
873	   and 237) and SRv6 End.X SID (43)/SRv6 LAN End.X SID (44) (sub-TLVs of
874	   TLVs 27, 135, 235, 236 and 237)".  The registration procedure is
875	   "Expert Review" as defined in [RFC8126].  Guidance for the Designated
876	   Experts is provided in [RFC7370]The following assignments are made by
877	   this document:

[nit] s/[RFC7370]The/[RFC7370]. The

879	    Type   Description                          Encoding
880	                                                Reference
881	   ---------------------------------------------------------
882	    0      Reserved
883	    1      SRv6 SID Structure Sub-Sub-TLV       Section 9
884	    2-255  Unassigned

[major] The reference should be "[This Document]".

[End]

---

From: Peter Psenak <[email-masked]>
Date: Fri, 9 Apr 2021 10:12:39 +0200

Hi Alvaro,

please see inline (##PP):

##PP
done

##PP
done

##PP
done

##PP
done

##PP
done

what about:

"In case where the same prefix, with the same prefix-length, MTID and 
algorithm is received  in both a Prefix Reachability TLV and an SRv6 
Locator TLV the Prefix Reachability advertisement MUST be preferred.."

##PP
done

##PP
done

thanks,
Peter

---

From: Alvaro Retana <[email-masked]>
Date: Fri, 9 Apr 2021 13:54:42 -0700

Peter:

I’m ok with the text below.

Thanks!

Alvaro.

---

From: Peter Psenak <[email-masked]>
Date: Thu, 22 Apr 2021 13:26:56 +0200

Hi Alvaro,

I have posted an updated version of the draft that has the new 
registries defined for all flags fields defined in it.

thanks,
Peter

---

From: Alvaro Retana <[email-masked]>
Date: Thu, 22 Apr 2021 12:53:10 -0700

> registries defined for all flags fields defined in it.

Thanks Peter!

I've started the IETF LC.

Alvaro.