[Pce] Document Shepherd Review of draft-ietf-pce-pcep-extension-native-ip-25

[Dhruv Dhody <[e-mail]>]

# Document Shepherd Review of draft-ietf-pce-pcep-extension-native-ip-25

I have done a shepherd review of this I-D. I have some concerns that should
be resolved before we send this to our AD.

I continue to believe that this I-D is better suited as experimental;
authors seem to disagree.

## Major

- The use of the PCErr message to report an established BGP session as
'broken' is not right. The PCErr message is always sent in response to a
message from a peer. We should use a PCRpt message with the status as
'down' in this case. Section 5.2 should also include the use of PCRpt
messages during synchronization.

- The I-D is silent on Native-IP path update procedures. I think this
should be highlighted -- The EPR update is done as per the
make-before-break procedures, i.e., the PCECC first updates new native-ip
instructions based on the updated path and then informs the source node to
switch traffic before cleaning up the former instructions as described in
[RFC9050].

- Section 7.4, it is difficult to decode this object. All you have is the
length of the full object from the object header and it is difficult to
decode how many prefixes exist with additional TLV of variable length
allowed. It is also unclear on the advantage of using RFC 3209 subobjects
here since the mix of subobject types is anyway not allowed. I suggest
changing this to -

~~~~
    0                   1                   2                   3
    0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |                  Peer IPv4 Address                            |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   | No. of Prefix |                  Reserved                     |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |                  IPv4 Prefix #1                               |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |Prefix #1 Len  |                  Reserved                     |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |                               :                               |
   |                               :                               |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |                  IPv4 Prefix #n                               |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |Prefix #n Len  |                  Reserved                     |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   //                  Additional TLVs                            //
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
~~~~


- Section 9, I am worried about this -
~~~~
   When the PCE sends out the PCInitiate message with the BPI object
   embedded to establish the BGP session between the PCC peers, it
   should wait enough time to get the BGP session successful
   establishment report from the underlay PCCs. If the PCE can't get
   such report after the duration, then it should declare the failure of
   BGP session setup and try the establishment procedure again, or
   report the failure to the operator.
~~~~
I think you are doing this because you are expecting a PCRpt message to
indicate BGP session establishment only in case of success and what error
to use in other cases. This goes against the PCInitiate mechanism of RFC
8281. I suggest you add BGP session status in the BPI Object and allow a
PCRpt message with status as in-progress, established, down, etc (or re-use
LSP object status). Also, the BGP session must be explicitly cleared by the
PCE. It is better if all CCIs (BPI/EPR/PPA) are explicitly cleared by the
PCE as that would be aligned to the PCInitiate/PCECC way.


## Minor

- I suggest enhancing the Introduction by summarizing
https://www.rfc-editor.org/rfc/rfc8821.html#section-3

- The role of the symbolic path name should be more clearly stated
(currently it is mentioned in passing in 5.1). It looks like you expect the
same symbolic path name (and PLSP-ID(?)) to be used in all related messages
from the BGP session to the explicit route to prefix advertisement. This
requirement needs to be made normative.

- In section 6.3, please state clearly that the prefixes encoded are
advertised on the corresponding BGP session with reference to the relevant
BGP RFC.

- Section 7.2;
    - Please change the below text to MUST. Since you already have error
codes associated with this. It makes complete sense for this to be a MUST.
It would also be wise to move this to Section 6.1

~~~~
   OLD:
   By default, the Local/Peer IP address SHOULD be dedicated to the
   usage of native IP TE solution, and SHOULD NOT be used by other BGP
   sessions that established by manual or non PCE initiated
   configuration.
   NEW:
   The Local/Peer IP address MUST be dedicated to the usage of native
   IP TE solution, and MUST NOT be used by other BGP sessions that
   established by manual or other ways.
   END
~~~~

- Section 7.2;
    - Please expand ETTL
    - Suggest to change Reserved to a Flag field and corresponding new IANA
registry to allow easy extensibility
    - When the T bit is cleared, what happens to the Tunnel source and
destination IP? If they are not needed make them optional and let them be
included only if T bit is set.  Also, you should explain where IP in IP is
used.

- Section 7.3; Should route priority be called route preference instead?
Please check.

- Section 7.4; Please add an error code when the prefix received is already
being advertised on some BGP session.

- Section 10; I think we can be explicit in saying that by using unique
peer IP addresses that are not used for any other purpose we avoid making
an impact on the other routes and routing system.

- I am not a fan of the message table (1-4) approach taken by this I-D. The
better and more consistent way would be to use the same approach of event
diagrams as RFC 9050 and past RFCs from this WG. It would show the timing
relationship between these messages which is currently missing. But it is
up to the authors to decide. If you keep the table, consider ordering
tables 2 and 3 in the correct order.

- I suggest following RFC 6123 and including a crisp manageability
consideration highlighting the key distinctions of Native IP operations.
Currently, the I-D says "Manageability considerations that are described in
[RFC9050] should be followed."

## Nits

- I have made edits directly in the XML, please review and accept if you
agree with the changes
    - Expanded PCEP in the title
    - Various grammar errors (suggest using tools like Grammarly)
    - Replace head with source and end with destination.
    - Various others...

Regards,
Dhruv

XML:
https://raw.githubusercontent.com/dhruvdhody/ietf/master/draft-ietf-pce-pcep-extension-native-ip-26.xml
TXT:
https://raw.githubusercontent.com/dhruvdhody/ietf/master/draft-ietf-pce-pcep-extension-native-ip-26.txt
DIFF:
https://author-tools.ietf.org/diff?doc_1=draft-ietf-pce-pcep-extension-native-ip-25&url_2=https://raw.githubusercontent.com/dhruvdhody/ietf/master/draft-ietf-pce-pcep-extension-native-ip-26.txt

PS. In case of formatting issues, you can find this review at
https://notes.ietf.org/draft-ietf-pce-pcep-extension-native-ip-25?view