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

[Dhruv Dhody <[e-mail]>]

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