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

[Peter Psenak <[e-mail]>]

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