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

[Alvaro Retana <[e-mail]>]

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.


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


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

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

[major] "MUST match the setting of the embedded X-flag"

What if the setting doesn't match?  What should the receiver do?

By "embedded", you are referring to the X bit inside 236/237, right?
To differentiate, please call it "X bit" and add references to
rfc5120/5308.



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


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


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

We need the header to include the Updates tag, and something in the
Abstract and Introduction.

In the Abstract>
   This documents updates rfc7370 by modifying an existing registry.


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

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


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

[End]