Re: [RTG-DIR] Rtgdir early review of draft-ietf-idr-segment-routing-te-policy-18

[Ketan Talaulikar <[e-mail]>]

Hi Med,

The draft update has just been posted:
https://datatracker.ietf.org/doc/html/draft-ietf-idr-segment-routing-te-policy-19

Please let us know if it addresses your comments and if you have any
further feedback.

Thanks,
Ketan


On Wed, Jul 20, 2022 at 1:56 PM <[e-mail]> wrote:

> Hi Ketan,
>
>
>
> Thanks for the follow-up.
>
>
>
> Will monitor when the new version is available and react if I have any
> further comments.
>
>
>
> Cheers,
>
> Med
>
>
>
> *De :* rtg-dir <[e-mail]> *De la part de* Ketan Talaulikar
> *Envoyé :* mardi 19 juillet 2022 19:55
> *À :* BOUCADAIR Mohamed INNOV/NET <[e-mail]>
> *Cc :* [e-mail];
> [e-mail]; idr@ietf. org <
> [e-mail]>
> *Objet :* Re: [RTG-DIR] Rtgdir early review of
> draft-ietf-idr-segment-routing-te-policy-18
>
>
>
> Hi Mohamed,
>
>
>
> Thanks for your very detailed review and helpful suggestions. Please check
> inline below for responses.
>
>
>
> We will post the update once the submission tool reopens.
>
>
>
>
>
> On Fri, Jul 8, 2022 at 6:35 PM Mohamed Boucadair via Datatracker <
> [e-mail]> wrote:
>
> Reviewer: Mohamed Boucadair
> Review result: Has Issues
>
> Document: draft-ietf-idr-segment-routing-te-policy-18
> Reviewer: Mohamed Boucadair
> Review Date: 08/07/2022
> IETF LC End Date: N/A
> Intended Status: Standards Track
>
> I appreciate the effort that was spent to progress this draft since more
> than 6
> years!
>
> Before reviewing the document, I started first by re-reading
> RFC8024/RFC9012
> and reading draft-ietf-spring-segment-routing-policy for establishing the
> context. Overall, the approach documented in
> draft-ietf-idr-segment-routing-te-policy is sound and straightforward.
>
> I didn’t find major concerns from a routing standpoint other than the need
> to
> motivate some few claims (see the detailed review file about RRs, for
> example)
> and the lack of considerations related to the handling of the various
> sub-TLVs
> by intermediate routers (if any).
>
> However, there are a number of generic issues that I would recommend to
> consider (see the detailed review for the full list). All these are
> easy-to-fix
> issues.
>
> # General Comments (in no specific order)
>
> ## Consistency
>
> ### Single or multiple paths
>
> There is an apparent inconsistency in the document about the handling of
> multiple paths. For example, Section 1 says :"Selection of the best
> candidate
> path for an SR Policy" while the same section says also “this will result
> in
> one or more candidate paths being installed into ..”.
>
>
>
> KT> The first is about the selection of the best candidate path for an SR
> Policy by the SRPM - this is what gets installed in the forwarding. The
> second is about the installation of the received candidate paths into the
> BGP table. There is no inconsistency.
>
>
>
>
> If multipath is supported, then please add an explicit statement and make
> sure
> the overall text is consistent.
>
>
>
> KT> Only a single CP is selected for a given SR Policy. This is per the
> draft-ietf-spring-segment-routing-policy and this document does not change
> that.
>
>
>
>
> ### Value 0 is marked as reserved for some registries, while that value is
> associated with a meaning for other registries.
>
> Is there any reason why a consistent approach isn’t followed here? what is
> the
> issues if value 0 is open for assignment?
>
>
>
> KT> It is normal routing protocol practice to not assign the TLV 0 values.
> Can you indicate where the TLV code point 0 is being assigned?
>
>
>
>
> ## Modifications to the format of the Color Extended Community
>
> The text says that you are modifying the format the Color Extended
> Community,
> while this is not true. What this draft does is just associating a meaning
> with
> some bits. I would update the text accordingly.
>
>
>
> KT> We are changing the format of only the Flags field and not of the
> entire EC. Flags are normally independent bits and here we are combining
> two bits to convey 4 values. Clarified this in the Introduction section.
>
>
>
>
> ## Normative language
>
> The use of the normative language should be double-checked. The most
> apparent
> concern is related the statement related to the handling of the reserved
> bits
> (SHOULD) while this RFC9012 uses MUST (which is correct, IMO).
>
>
>
> KT> Ack. I will fix it and change it to MUST.
>
>
>
>
> I tagged many others in the full review, fwiw.
>
> ## Lack of description
>
> Many fields are provided without acceptable description (e.g., “Local IPv4
> Address: a 4-octet IPv4 address.” or “Preference: a 4-octet value” !!).
>
>
>
> KT> These fields are in the context of a sub-TLV. There is text in the
> description of that sub-TLV that provides a reference (e.g., to the
> draft-ietf-spring-segment-routing-policy section or a segment type, etc.)
> There is no need to repeat a detailed description for each field IMO.
>
>
>
>
> Also, some fields are provided with a structure but the text says also that
> these are reserved (e.g., 2.4.2 says “TC, S and TTL (Total of 12 bits) are
> RESERVED”).
>
>
>
> KT> This is the MPLS label field. I am not sure that I follow your concern
> here.
>
>
>
>
> I wonder whether you can add a statement to say that multiple flags can be
> set
> simultaneously unless this is precluded by future flag assignments.
>
>
>
> KT> Not sure that is necessary. In most cases, the bits/flags are
> independent. Where they are not, there is generally text explaining their
> relationship or dependency.
>
>
>
>
> Last, the document does not include the expected behavior of intermediate
> routers (e.g., whether it is allowed or not to alter some fields). I guess,
> they must not touch the content of the attributes but it is better if this
> is
> explicitly mentioned in the text.
>
>
>
> KT> Yes, the contents must not be altered. Will clarify in sec 4.2.4.
>
>
>
>
> ## Reserved vs. Unassigned
>
> Almost all the “reserved” bits in the spec can be assigned in the future. I
> would use “Unassigned” as per RFC8126.
>
>
>
> KT> Ack. Will change in a few places where this has been missed.
>
>
>
>
> FWIW, 8126 says the following:
>
>       Unassigned:  Not currently assigned, and available for assignment
>             via documented procedures.
>
>       Reserved:  Not assigned and not available for assignment.
>             Reserved values are held for special uses, such as to extend
>             the namespace when it becomes exhausted.
>
> ## Deprecated values
>
> The document includes notes about some “deprecated” codepoints. I’m not
> sure
> there is a value in having such notes in the final RFC.
>
>
>
> KT> Yes, there is a need. One is to avoid them being used for any other
> sub-TLV in the future. Two is that there are early implementations out
> there that have some degree of support - even if they are just doing some
> parsing/showing.
>
>
>
>
> ## IANA considerations
>
> ### The document uses a mix of TBD statements (e.g., Section 2.4.3) and
> hard-coded values (early assignments). Not sure what’s was the rationale
> especially that code 20 was assigned but not listed as such.
>
>
>
> KT> Fixed.
>
>
>
>
> ### The IANA actions should be more explicit and ask IANA to update
> existing
> entries. For example, the current registry for code 73 points to
> [draft-previdi-idr-segment-routing-te-policy]. Need to update that entry
> and
> similar ones.
>
>
>
> KT> Have fixed the text. IANA will update "This document" to the RFC
> number before publication. There is no need to keep changing the draft name
> through its lifecycle.
>
>
>
>
> ### The document lists (under IANA section) some values that are
> deprecated.
> The document should be clear whether these codes are available for future
> assignment or not.
>
>
>
> KT> Deprecated means they are not available for assignment by IANA unless
> the IETF changes that via an RFC.
>
>
>
>
> ### Many sub-TLVs have flag bits but not all of them have a registry to
> track
> future flag bit assignments.
>
>
>
> KT> The registries would be added by future documents that start using
> those flags.
>
>
>
>
> ## Manageability considerations
>
> No such considerations are included in the document.
>
>
>
> KT> Will add.
>
>
>
>
> # Detailed review
>
> FWIW, you can find my full review at:
>
> * pdf:
>
> https://github.com/boucadair/IETF-Drafts-Reviews/raw/master/draft-ietf-idr-segment-routing-te-policy-18-rev%20Med.pdf
> * doc:
>
> https://github.com/boucadair/IETF-Drafts-Reviews/raw/master/draft-ietf-idr-segment-routing-te-policy-18-rev%20Med.doc
>
>
>
> KT> This was helpful and have incorporated most of those suggestions.
>
>
>
> Thanks,
>
> Ketan
>
>
>
> _________________________________________________________________________________________________________________________
>
> Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
> pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
> a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
> Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.
>
> This message and its attachments may contain confidential or privileged information that may be protected by law;
> they should not be distributed, used or copied without authorisation.
> If you have received this email in error, please notify the sender and delete this message and its attachments.
> As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
> Thank you.
>
>