Re: [Pce] AD review of draft-ietf-pce-segment-routing-ipv6-20

[Cheng Li <[e-mail]>]

Hi John,

Thank you so much for your comments, we think they are very helpful.
We have modified the draft to address your comments accordingly, please review it again. 
If they can address your comments, we will update the draft very soon.

Thanks,
Cheng



-----Original Message-----
From: John Scudder <[e-mail]> 
Sent: Thursday, January 25, 2024 12:49 AM
To: [e-mail]
Cc: [e-mail]
Subject: AD review of draft-ietf-pce-segment-routing-ipv6-20

Hi Authors, WG,

Thanks for this document.

I’ve supplied my questions and comments in the form of an edited copy of the draft. Minor editorial suggestions I’ve made in place without further comment, more substantive questions and comments are done in-line and prefixed with “jgs:”. You can use your favorite diff tool to review them; I’ve attached the iddiff output for your convenience if you’d like to use it. I’ve also pasted a traditional diff below in case you want to use it for in-line reply.

One further point regarding “minor editorial suggestions”. While I did make a few, I didn’t do exhaustive copy-editing of this draft, and of course, the RFC Editor will do their usual comprehensive job. However, I did notice a pervasive need for a few particular kinds of changes (for example definite articles, agreement in number) that mar what’s otherwise a high-quality document and are likely to bother some reviewers. It’s optional, but it wouldn’t be a bad idea for you to pass the document through a grammar checker before we send it for IETF Review. Your call.

Thanks,

—John

--- draft-ietf-pce-segment-routing-ipv6-20.txt	2024-01-24 14:39:13.000000000 -0500
+++ draft-ietf-pce-segment-routing-ipv6-20-jgs-comments.txt	2024-01-24 18:40:57.000000000 -0500
@@ -29,12 +29,15 @@
    A Segment Routed Path can be derived from a variety of mechanisms,
    including an IGP Shortest Path Tree (SPT), explicit configuration, or
    a PCE.
+--
+jgs: I think it would be appropriate to expand PCE.
+--
 
    Since SR can be applied to both MPLS and IPv6 forwarding planes, a
-   PCE should be able to compute SR-Path for both MPLS and IPv6
+   PCE should be able to compute an SR path for both MPLS and IPv6
    forwarding planes.  The PCEP extension and mechanisms to support SR-
    MPLS have been defined.  This document describes the extensions
-   required for SR support for IPv6 data plane in the Path Computation
+   required for SR support for the IPv6 data plane in the Path 
+ Computation
    Element communication Protocol (PCEP).
 
 Status of This Memo
@@ -151,7 +154,7 @@
    [RFC8754].
 
    An SR path can be derived from an IGP Shortest Path Tree (SPT), but
-   SR-TE (Segment Routing Traffic Engineering) paths may not follow IGP
+   SR-TE (Segment Routing Traffic Engineering) paths might not follow 
+ IGP
    SPT.  Such paths may be chosen by a suitable network planning tool,
    or a PCE and provisioned on the ingress node.
 
@@ -186,7 +189,7 @@
    stateful PCE for computing one or more SR-TE paths taking into
    account various constraints and objective functions.  Once a path is
    chosen, the stateful PCE can initiate an SR-TE path on a PCC using
-   PCEP extensions specified in [RFC8281] using the SR specific PCEP
+   PCEP extensions specified in [RFC8281] and the SR-specific PCEP
    extensions specified in [RFC8664].  [RFC8664] specifies PCEP
    extensions for supporting a SR-TE LSP for the MPLS data plane.  This
    document extends [RFC8664] to support SR for the IPv6 data plane.
@@ -228,6 +231,16 @@
 
    The message formats in this document are specified using Routing
    Backus-Naur Format (RBNF) encoding as specified in [RFC5511].
+--
+jgs: as far as I can tell, no they are not. Either remove this 
+paragraph, and the normative reference, or help me understand why I am 
+wrong.
+
+Speaking of RBNF, the fact this spec doesn't use it seems to put it out 
+of step with some of the other PCE document set. I'm assuming it's the 
+consensus of the WG that this is fine. (I don't have a problem with the 
+style used here.)
+--
 
    Further, following terms are used in the document,
 
@@ -240,6 +253,11 @@
    SID:  Segment Identifier.
 
    SRv6:  Segment Routing for IPv6 forwarding plane.
+--
+jgs: in the introduction you expanded this as "Segment Routing over 
+IPv6 data plane", which seems like a better expansion to me. But either 
+way, please pick one and stick with it.
+--
 
    SRH:  IPv6 Segment Routing Header.
 
@@ -255,6 +273,14 @@
    Basic operations for PCEP speakers are as per [RFC8664].  SRv6 Paths
    computed by a PCE can be represented as an ordered list of SRv6
    segments of 128-bit value.
+--
+jgs: probably just "128 bit SRv6 segments", or if you feel that's not 
+explicit enough, "SRv6 segments, each of which is 128 bits in length."
+(The problem with the current version is that first, 128 bits is the 
+length of the segment, not the value of it, and second, there's 
+possible ambiguity as to whether 128 bits is the length of the list, or 
+the length of each individual segment.)
+--
 
    [RFC8664] defined a new Explicit Route Object (ERO) subobject denoted
    by "SR-ERO subobject" capable of carrying a SID as well as the @@ -271,6 +297,16 @@
 
    This document defines new subobjects "SRv6-ERO" and "SRv6-RRO" in the
    ERO and the RRO respectively to carry the SRv6 SID (IPv6 Address).
+--
+jgs: it's not at all clear to me that it's formally correct to say an
+SRv6 SID is the same as an IPv6 address; for example, the abstract of
+draft-ietf-6man-sids-05 tells me that "Segment Identifiers (SIDs) used 
+by SRv6 *can resemble* IPv6 addresses and behave like them while 
+exhibiting *slightly different* behaviors in some situations" (my 
+emphasis).
+
+Probably the best fix is just to remove the text inside the parentheses.
+--
    SRv6-capable PCEP speakers MUST be able to generate and/or process
    these subobjects.
 
@@ -304,6 +340,15 @@
    necessary information to guide the packets from the ingress node to
    the egress node of the path, and hence there is no need for any
    signaling protocol.
+--
+jgs: that last clause can't possibly be right as written. SR networks 
+aren't completely devoid of any and all signaling... consider that PCEP 
+itself is a signaling protocol of sorts. While it would be possible to 
+expand and rewrite the clause more precisely to be formally correct, I 
+think the easiest fix is just to delete it, since it's not directly 
+relevant to the subject at hand. (I see this language may have been 
+inherited from RFC 8664; I think it's wrong there too.)
+--
 
    For the use of an IPv6 control plane but an MPLS data plane,
    mechanism remains the same as specified in [RFC8664].
@@ -412,6 +457,10 @@
          SRv6-SID.
 
       -  Unassigned bits MUST be set to 0 and ignored on receipt.
+--
+jgs: I think this should be "set to 0 on transmission and ignored on 
+receipt", right?
+--
 
       A pair of (MSD-Type, MSD-Value): Where MSD-Type (1 octet) is as
       per the IGP MSD Type registry created by [RFC8491] and populated @@ -426,12 +475,29 @@
    to ensure those midpoints are able to correctly process their
    segments and for the tailend to dispose of the SRv6 encapsulation.
    The SRv6 MSD capabilities of these other nodes MAY be learned as part
+--
+jgs: The above RFC 2119 style "MAY" should probably be "may", or better 
+still "might", I don't think you're stating a requirement here (or in 
+this case, since "MAY", a non-requirement). I assume you'd take the 
+position that it's beyond the scope of this specification to say how 
+the capabilities of the other nodes have to be learned.
+--
    of the topology information via IGP/BGP-LS or via PCEP if the PCE
    also happens to have PCEP sessions to those nodes.
+--
+jgs: you need a reference for BGP-LS.
+--
 
    It is RECOMMENDED that the SRv6 MSD information be not included in
    the SRv6-PCE-Capability sub-TLV in deployments where the PCE is able
    to obtain this via IGP/BGP-LS as part of the topology information.
+--
+jgs: By using the RFC 2119 style "RECOMMENDED" you're making this an 
+almost-MUST. Do you intend that? If so, please consider providing 
+guidance as to when the requirement can be disregarded.
+
+But I think probably you mean "recommended".
+--
 
 4.2.  The RP/SRP Object
 
@@ -478,6 +544,12 @@
       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 
                     Figure 2: SRv6-ERO Subobject Format
+--
+jgs: the fields marked "optional" aren't really completely optional, 
+are they? I'm not aware of a totally standard pattern for this kind of 
+field, but I have seen the term "conditional" used in such a context, 
+or even "conditional, see below".
+--
 
    The fields in the SRv6-ERO subobject are as follows:
 
@@ -511,6 +583,13 @@
    below) then the NT field has no meaning and MUST be ignored by the
    receiver.  This document reuses NT types defined in [RFC8664], but
    assigns them new meanings appropriate to SRv6.
+--
+jgs: I feel very uncomfortable about being told that you're using an 
+existing registry, but assigning different semantics to 
+already-allocated code points. I think the most straightforward fix to 
+this would be to create a new "PCEP SRv6-ERO NAI Types" registry, 
+populated with values 0, 2, 4 and 6, defined however you like.
+--
 
       If NT value is 0, the NAI MUST NOT be included.
 
@@ -531,6 +610,11 @@
       identify the IPv6 Adjacency and used with the SRv6 Adj-SID.
 
       SR-MPLS specific NT types are not valid in SRv6-ERO.
+--
+jgs: if you take my suggestion of creating a new registry, the above 
+line becomes unnecessary. There would be no SR-MPLS specific NT types 
+in your new registry.
+--
 
    Flags: Used to carry additional information pertaining to the
    SRv6-SID.  This document defines the following flag bits.  The other @@ -575,9 +659,23 @@
    is recommended to signal it always if possible.  It could be used for
    maintainability and diagnostic purpose.  If behavior is not known,
    the opaque value '0xFFFF' is used [RFC8986].
+--
+jgs: please be explicit that the behavior values are taken from the 
+registry "SRv6 Endpoint Behaviors".
+
+It sure seems strange to me to choose to use a reserved value called 
+"opaque" to signal "behavior not known" instead of having a registered 
+value called "behavior not known". But, it's probably too late to 
+change this now.
+--
 
    SRv6 SID: SRv6 Identifier is an 128-bit IPv6 address representing the
    SRv6 segment.
+--
+jgs: but is it in fact an address? See my comment with reference to
+draft-ietf-6man-sids-05 earlier on. An easy fix would be to replace
+"IPv6 address" with "value".
+--
 
    NAI: The NAI associated with the SRv6-SID.  The NAI's format depends
    on the value in the NT field, and is described in [RFC8664].
@@ -650,7 +748,7 @@
    validation of SRv6 SIDs being instantiated in the network and checked
    for conformance to the SRv6 SID allocation scheme chosen by the
    operator as described in Section 3.2 of [RFC8986].  In the future,
-   PCE can also be utilized to verify and automate the security of the
+   PCE might also be utilized to verify and automate the security of 
+ the
    SRv6 domain by provisioning filtering rules at the domain boundaries
    as described in Section 5 of [RFC8754].  The details of these
    potential applications are outside the scope of this document.
@@ -811,7 +909,7 @@
 5.2.1.  SRv6 ERO Validation
 
    If a PCC does not support the SRv6 PCE Capability and thus cannot
-   recognize the SRv6-ERO or SRv6-RRO subobjects.  It should respond
+   recognize the SRv6-ERO or SRv6-RRO subobjects, it should respond
    according to the rules for a malformed object as described in
    [RFC5440].
 
@@ -832,6 +930,11 @@
       MUST be 48, otherwise the Length MUST be 64.
 
    *  NT types (1,3, and 5) are not valid for SRv6.
+--
+jgs: If you take my suggestion to set up a separate registry, you won't 
+need the above bullet, because NT types 1, 3, and 5 won't exist in that 
+registry.
+--
 
    *  If T bit is 1, then S bit MUST be zero.
 
@@ -963,10 +1066,15 @@
 7.2.  Information and Data Models
 
    The PCEP YANG module is out of the scope of this document and defined
-   in other drafts.  An augmented YANG module for SRv6 is also specified
-   in another draft that allows for SRv6 capability and MSD
+   in other documents.  An augmented YANG module for SRv6 is also specified
+   in another document that allows for SRv6 capability and MSD
    configurations as well as to monitor the SRv6 paths set in the
    network.
+--
+jgs: please provide informative references to the "other documents" and 
+"another document" (which I've edited from "other drafts" and "another 
+draft").
+--
 
 7.3.  Liveness Detection and Monitoring