[Pce] AD review of draft-ietf-pce-vn-association-08

[John Scudder <[e-mail]>]

Dear Authors,

Thanks for your patience. Here’s my review.

I’ve supplied my comments in the form of an edited copy of the draft. Minor editorial suggestions I’ve made in place without further comment, more substantive 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. I’d appreciate feedback regarding whether you found this a useful way to receive my comments as compared to a more traditional numbered list of comments with selective quotation from the draft.

Thanks,

—John

--- draft-ietf-pce-vn-association-08.txt	2022-09-20 16:26:37.000000000 -0400
+++ draft-ietf-pce-vn-association-08-jgs-comments.txt	2022-09-26 10:18:24.000000000 -0400
@@ -140,6 +140,34 @@
    making it the base for PCE applicability for ACTN.  In this text
    child PCE would be same as Provisioning Network Controller (PNC), and
    the parent PCE as Multi-domain Service Coordinator (MDSC) [RFC8453].
+   
+---
+jgs: I found the last sentence to be unclear. I see that RFC 6805 §1.4
+defines Child PCE and Parent PCE:
+
+   Parent PCE: A PCE responsible for selecting a path across a parent
+   domain and any number of child domains by coordinating with child
+   PCEs and examining a topology map that shows domain inter-
+   connectivity.
+
+   Child PCE: A PCE responsible for computing the path across one or
+   more specific (child) domains.  A child PCE maintains a relationship
+   with at least one parent PCE.
+
+and I see that RFC 8751 §1.2.1 talks about how in the ACTN context 
+C-PCE maps to PNC and P-PCE maps to MDSC:
+
+   In this case, the P-PCE (as MDSC) interacts with multiple C-PCEs (as
+   PNCs) along the interdomain path of the LSP.
+
+So *maybe* the sentence above is trying to say something like,
+
+   As [RFC8751] explains, in the context of ACTN, the Child PCE is
+   identified with the PNC, and the Parent PCE is identified with 
+   the MDSC.
+   
+If that's not it, let's discuss please?
+---
 
    In this context, there is a need to associate a set of LSPs with a VN
    "construct" to facilitate VN operations in the PCE architecture.
@@ -150,7 +178,7 @@
    policy actions, setting default behavior, etc.
 
    This document specifies a PCEP extension to associate a set of LSPs
-   based on Virtual Network (VN).
+   based on their Virtual Network (VN).
 
 1.1.  Requirement Language
 
@@ -186,7 +214,7 @@
 
    *  Path Computation: When computing a path for an LSP, it is useful
       to analyze the impact of this LSP on the other LSPs belonging to
-      the same VN.  The aim would be optimize all LSPs belonging to the
+      the same VN.  The aim would be to optimize all LSPs belonging to the
       VN rather than a single LSP.  Also, the optimization criteria
       (such as minimizing the load of the most loaded link (MLL)
       [RFC5541]) could be applied for all the LSPs belonging to the VN
@@ -250,19 +278,48 @@
    (MDSC) and a child PCE (PNC).  When computing the path, the child PCE
    (PNC) refers to the VN association in the request from the parent PCE
    (MDSC) and maps the VN to the associated LSPs and network resources.
-   From the perspective of Parent PCE, it receives a virtual network
+   From the perspective of the Parent PCE, it receives a virtual network
    creation request by its customer, with the VN uniquely identified by
-   an Association ID in VNAG as well as the Virtual Network identifier.
+   an Association ID in the VNAG as well as the Virtual Network identifier.
+  
+-- 
+jgs: in the preceding clause, do you mean that the VN is uniquely 
+identified by either the Association ID or the VNI, or do you mean
+it's uniquely identified by the tuple (Association ID, VNI)? I think
+it's probably the latter, based on the defintion of the ASSOCIATION 
+object in RFC 8697:
+
+   Association ID (2 bytes):  The identifier of the association group.
+      When combined with other association parameters, such as an
+      Association Type and Association Source, this value uniquely
+      identifies an association group.
+
+Assuming that is right, perhaps rewrite like
+
+   From the perspective of the Parent PCE, it receives a virtual network
+   creation request by its customer, with the VN uniquely identified by
+   the combination of the Association ID in the VNAG and the Virtual 
+   Network identifier.
+
+If that's wrong, let's discuss please.
+--
+
    This VN may comprise multiple LSPs in the network in a single domain
-   or across multiple domains.  Parent PCE sends a PCInitiate Message
+   or across multiple domains.  The Parent PCE sends a PCInitiate Message
    with this association information in the VNAG Object.  This in effect
    binds an LSP that is to be instantiated at the child PCE with the VN.
    The VN association information could be included as a part of the
    response as well.  Figure 1 shows an example of a typical VN
+   
+--
+jgs: Do I correctly infer that it would also be fine for the VN association
+information to *not* be included as part of the response? ("Could" implies
+"could not".)
+--
    operation using PCEP.  It is worth noting that in a multi-domain
    scenario, the different domains are controlled by different child
    PCEs.  In order to set up the cross-domain tunnel, multiple segments
-   need to be stitched, by the border nodes in each domain who receives
+   need to be stitched, by the border nodes in each domain who receive
    the instruction from their child PCE (PNC).
 
 
@@ -315,13 +372,25 @@
           MPI  -> PCEP
 
           Figure 1: Example of VN operations in H-PCE Architecture
+          
+--
+jgs: Figure 1 has a bunch of notations that are never referred to in the
+text. I'm guessing these were carried forward from some other use of the
+figure where they were actually referred to, but now they just create
+visual clutter and potential confusion. Things I flagged as not being 
+referenced include: "with VNAG = 10", "A", "C", "B13", "B43", "B31",
+"B34", "B".
+
+I suggest either tidying the figure up, or expanding the explanation text
+to refer to the labels.
+--
 
    Whenever changes occur with the instantiated LSP in a domain network,
    the domain child PCE reports the changes using a PCRpt Message in
    which the VNAG Object indicates the relationship between the LSP and
    the VN.
 
-   Whenever an update occurs with VNs in the Parent PCE (via the
+   Whenever an update occurs with VNs in the Parent PCE (due to the
    customer's request), the parent PCE sends an PCUpd Message to inform
    each affected child PCE of this change.
 
@@ -369,12 +438,27 @@
    that uniquely identifies the VN.  It SHOULD be a string of printable
    ASCII [RFC0020] characters (i.e., 0x20 to 0x7E), without a NULL
    terminator.  The Virtual Network Identifier is a human-readable
+--
+jgs: Is there an established practice in PCE of using US-ASCII as the
+character set for human-readable strings? This would seem to be a 
+practice that's discouraged by BCP 18, BCP 166. Was this discussed in
+the WG and a decision taken to not use internationalized strings?
+--
    string that identifies a VN and can be specified with the association
    information.  An implementation could use the Virtual Network
    Identifier to maintain a mapping to the VN association group and the
    LSPs associated with the VN.  The Virtual Network Identifier MAY be
    specified by the customer or set via an operator policy or auto-
    generated by the PCEP speaker.
+   
+--
+jgs: At the top of the page you say it's a "mandatory TLV", but then in
+the paragraph right above you say it "can be" specified and an 
+implementation "could" use it. "Can" and "could" don't imply "mandatory"
+in the normal English sense. Does "mandatory" have some special meaning
+in the context of PCE, is there some other explanation for this 
+discrepancy, ... ?
+--
 
    The VIRTUAL-NETWORK-TLV MUST be included in VNAG object.  If a PCEP
    speaker receives the VNAG object without the VIRTUAL-NETWORK-TLV, it
@@ -452,21 +536,29 @@
 
 6.  Security Considerations
 
-   This document defines one new type for association, which do not add
+   This document defines one new type for association, which does not add
    any new security concerns beyond those discussed in [RFC5440],
    [RFC8231] and [RFC8697] in itself.
 
    Some deployments may find the Virtual Network Identifier and the VN
    associations as extra sensitive; and thus should employ suitable PCEP
    security mechanisms like TCP-AO [RFC5925] or TLS [RFC8253].
+--
+jgs: I have a few problems with the paragraph above. First, by "extra 
+sensitive" do you mean "confidential"? If so, use that term or one like 
+it, but in any case "extra sensitive" isn't specific enough.
+
+Second, assuming you do mean "confidential", TCP-AO isn't a suitable
+mitigation: AO doesn't provide confidentiality, only authentication.
+--
 
 7.  IANA Considerations
 
 7.1.  Association Object Type Indicator
 
-   IANA is requested to make the assignment of a new value for the sub-
-   registry "ASSOCIATION Type Field" (request to be created in
-   [RFC8697]) within the "Path Computation Element Protocol (PCEP)
+   IANA is requested to make the assignment of a new value in the sub-
+   registry "ASSOCIATION Type Field" 
+   within the "Path Computation Element Protocol (PCEP)
    Numbers" registry, as follows:
 
          Value     Name                        Reference
@@ -538,8 +630,8 @@
 8.6.  Impact On Network Operations
 
    [RFC8637] describe the network operations when PCE is used for VN
-   operations.  Section 3 further specify the operations when VN
-   associations is used.
+   operations.  Section 3 further specifies the operations when VN
+   associations are used.
 
 9.  References