Re: [Pce] Shepherd Review of draft-ietf-pce-segment-routing-policy-cp-14

[Mike Koldychev <[e-mail]>]

Hi Dhruv,

I've incorporated your changes and all the other comments that I have received so far. Hopefully I didn't miss anything. Version 15 is uploaded. Thanks a lot for your comments and updates!

Thanks,
Mike.

On Saturday, March 9th, 2024 at 8:23 AM, Dhruv Dhody <[e-mail]> wrote:

> Hi Authors,
>
> I have finished the shepherd review of draft-ietf-pce-segment-routing-policy-cp-14. Please handle these comments before we ship this I-D to IESG.
>
> ## Major- Section 5.6, you need to add update: RFC 8231 in the draft metadata. This should also be captured in the abstract. The prefered way is to clearly identify the text in RFC8231 that is changing with "OLD:" and "NEW:" format!
> - Section 8, Security considerations need to also cover the non-SRPA TLVs which are not considered in the current text.
>
> ## Query
> - Section 4.1,
> ````
> If the PCC receives a
> PCInit message with the Association Source set not to the headend IP
> but to some globally unique IP address that the headend owns, then
> the PCC SHOULD accept the PCInit message and create the SRPA with the
> Association Source that was sent in the PCInit message.
> ````
> What is the purpose of this text? PCC should use the source as set by the PCE - isn't it given? Am I missing something? Boris also pointed this out in his review.
>
> ## Minor
> - Abstract is not very useful for a non-expert. Maybe change something like -
> ````
> OLD:
> A Segment Routing (SR) Policy is a non-empty set of SR Candidate
> Paths, which share the same <headend, color, endpoint> tuple. SR
> Policy is modeled in PCEP as an Association of one or more SR
> Candidate Paths. PCEP extensions are defined to signal additional
> attributes of an SR Policy. The mechanism is applicable to all SR
> forwarding planes (MPLS, SRv6, etc.).
> NEW:
> Segment Routing (SR) allows a node to steer a packet flow along any
> path. SR Policy is an ordered list of segments (i.e.,
> instructions) that represent a source-routed policy. Packet flows
> are steered into an SR Policy on a node where it is instantiated
> called a headend node. An SR Policy is made of one or more candidate
> paths.
>
> This document specifies Path Computation Element Communication
> Protocol (PCEP) extension to associate candidate paths of the SR
> Policy. It applies equally to the SR-MPLS and Segment Routing over
> IPv6 (SRv6) instantiations of segment routing.
> END
> ````
> - Similarly I find Introduction to be very light on details. Consider adding text by looking through recently published RFCs for instance.
> - Terminology:
> ```
> OLD:
> SRPA: SR Policy Association. PCEP ASSOCATION that describes the SR
> Policy. Depending on discussion context, it refers to a PCEP
> object or to a group of LSPs that belong to the Association.
> NEW:
> SRPA: SR Policy Association. A new association type 'SR Policy
> Association' is used to group candidate paths belonging to the SR
> Policy. Depending on discussion context, it can refer to the PCEP
> ASSOCIATION object of SR Policy type or to a group of LSPs that
> belong to the association.
> END
> ```
> - Section 4, please add this text at the start -
> ````
> As per [RFC8697], LSPs are associated with other LSPs with which they
> interact by adding them to a common association group. As described
> in [RFC8697], the association group is uniquely identified by the
> combination of the following fields in the ASSOCIATION object:
> Association Type, Association ID, Association Source, and (if
> present) Global Association Source or Extended Association ID,
> referred to as Association Parameters.
> ````
> - Section 4.2, since none of the TLV are multi-instance. Can we simplify this text -
> ````
> OLD:
> Unless specifically stated otherwise, the TLVs listed in the
> following sub-sections are assumed to be single instance. Meaning,
> only one instance of the TLV SHOULD be present in the object and only
> the first instance of the TLV SHOULD be interpreted and subsequent
> instances SHOULD be ignored.
> NEW:
> This document specifies four new TLVs to be carried in the SRPA object.
> Only one TLV instance of each type can be carried, and only the first
> occurrence is processed. Any others MUST be ignored.
> ````
> Also applicable to section 5!
> - Section 4.2.2, consider changing the SHOULD to MUST in this section. I could not think of a justification for SHOULD here!
> - Section 5.1,
> - please also state what happens if the TLVs are used without the exchange of SRPOLICY-CAPABILITY TLV or the corresponding bit is unset. Without it, what is the use of adding this TLV?
> - Consider updating the description such as "P-flag: If set to '1' by a PCEP speaker, the P flag indicates that the PCEP speaker supports the handling of COMPUTATION-PRIORITY TLV for the SR Policy."
> - please add "Unassigned bits MUST be set to '0' on transmission and MUST be ignored on receipt."
> - Section 5.2, I am unsure about the interaction between the unsetting of P-flag (PCEP speaker does not support the TLV) and the default value (128 when the TLV is not present). Isn't it a bit weird?
> - Section 5.3, should the use of this TLV be limited to SR-MPLS? Also can ENLP value be converted into a registery maintained at https://www.iana.org/assignments/segment-routing/segment-routing.xhtml which can be referred by both PCE and BGP?
> - Section 5.4, please add "The unassigned bits in the Flag octet MUST be set to zero upon transmission and MUST be ignored upon receipt." Should "Invalidation Reasons Flags" get a registry for ease of adding new flags in future? In general, can the text in this section be tightened a little bit? Examples - be explicit on who is sending and who is receiving for instance. Also, consider adding a more detailed example to show the usage of the flags better alongside PCEP message exchange.
> - Section 5.5, please add normative reference to draft-ietf-pce-binding-label-sid
> - Section 6.5, are you refereing to the registry at https://www.iana.org/assignments/segment-routing/segment-routing.xhtml, in which case it is called "Segment Routing" and not "Segment Routing Parameters". Also better to call the new registry being added as subregistry.
> - Section 10.2, please make RFC 8253 and RFC 7525 as normative references.
>
> ## Nits
> - Expand PCEP and SR in the title
> - Expand PCEP, SRv6 in the abstract
> - Expand MBZ on first use. It is also better to state that the field is ignored on receipt
> - Section 4.2.2, add reference to RFC 9256 for Discriminator, as you have done for other fields
> - Section 4.2.4, add reference to RFC 9356 for Preference
> - s/there needs to be a separate capability negotiation/a separate capability negotiation is useful/
> - Expand on first use OAM/PM/BFD
> - Section 6, please update the text in subsections where the number of assignments in tables do not match the introductory text.
>
> I am also attaching the updated xml that could be a starting point for you to work on -15 version.
>
> Thanks!
> Dhruv