Re: [Pce] Benjamin Kaduk's Discuss on draft-ietf-pce-gmpls-pcep-extensions-14: (with DISCUSS and COMMENT)

Thanks for the thorough review, a new version addressing the comments has
been posted,

please see inline

A new version

On Thu, 11 Apr 2019 at 12:46, Benjamin Kaduk via Datatracker <
noreply@ietf.org> wrote:

> Benjamin Kaduk has entered the following ballot position for
> draft-ietf-pce-gmpls-pcep-extensions-14: Discuss
>
> When responding, please keep the subject line intact and reply to all
> email addresses included in the To and CC lines. (Feel free to cut this
> introductory paragraph, however.)
>
>
> Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
> for more information about IESG DISCUSS and COMMENT positions.
>
>
> The document, along with other ballot positions, can be found here:
> https://datatracker.ietf.org/doc/draft-ietf-pce-gmpls-pcep-extensions/
>
>
>
> ----------------------------------------------------------------------
> DISCUSS:
> ----------------------------------------------------------------------
>
> This document makes some well-needed extensions to existing PCEP
> concepts such as bandwidth, but I'm not convinced that the way they
> interact with existing PCEP functionality is sufficiently well specified
> to admit interoperable implementation.  Specifically, we introduce the
> generalized bandwidth structures and reuse that encoding for the
> generalized load balancing structures, which includes a notion of
> "minimum bandwidth specification".  But now that the bandwidth
> specification is a compound data structure instead of a scalar type,
> it's not guaranteed that we have a strict linear ordering with
> well-defined minimum.  If we consider the specific case of Intserv, do I
> insist upon all three of the minimum bucket rate, minimum bucket size,
> and minimum peak data rate?  Or perhaps I only care about the peak data
> rate and not the bucket size/rate.  We need more text in order to
> specify what "minimum" actually means/measures.
>
> Similarly, I'm not sure all the referenced generalized bandwidth
> types/traffic parameters in Section 2.3 clearly indicate which
> structures/fields we are to incorporate by reference (see COMMENT).
>
>
[MC] Thanks Julien for the text that was agreed on; it has been
incorporated in the new version.

OLD
   The semantic of the LOAD-BALANCING object is not changed.  If a PCC
   requests the computation of a set of TE-LSPs so that the total of
   their generalized bandwidth is X, the maximum number of TE-LSPs is N,
   and each TE-LSP must at least have a bandwidth of B, it inserts a
   BANDWIDTH object specifying X as the required bandwidth and a LOAD-
   BALANCING object with the Max-LSP and Min Bandwidth Spec fields set
   to N and B, respectively.

NEW
   The semantic of the LOAD-BALANCING object is not changed.  If a PCC
   requests the computation of a set of TE-LSPs with at most N TE-LSPs
   so that it can carry generalized bandwidth X , each TE-LSP must at
   least transport bandwidth B, it inserts a BANDWIDTH object specifying
   X as the required bandwidth and a LOAD-BALANCING object with the Max-
   LSP and Min Bandwidth Spec fields set to N and B, respectively.  When
   the BANDWIDTH and Min Bandwidth Spec can be summarized as scalars,
   the sum of all TE-LSPs bandwith in the set is greater than X.  The
   mapping of X over N path with (at least) bandwidth B is technology
   and possibly node specific.  Each standard definition of the
   transport technology is defining those mappings and are not repeated
   in this document.  A simplified example for SDH is described in
   Appendix A

   In all other cases, including for technologies based on statistical
   multiplexing (e.g., InterServ, Ethernet), the exact bandwidth
   management (e.g., Ethernet's Excessive Rate) is left to the PCE's
   policies, according to the operator's configuration.  If required,
   further documents may introduce a new mechanism to finely express
   complex load balancing policies within PCEP.

> Section 2.1.2 says:
>
>    GMPLS-CAPABILITY TLV it is RECOMMENDED that the PCC does not make use
>    of the objects and TLVs defined in this document.
>
> Why is this not "the PCC MUST NOT make use of the objects and TLVs
> defined in this document"?  Ignoring the peer's (non-)advertisement and
> plowing ahead seems like a recipe for non-interoperability.
>
>

[MC] The PCC is able to mandate or not the processing of objects on
per-request basic. To make it simpler the capability negotiation has
been made mandatory (both peer must advertize the capability).

Section 2.5.1 notes that:
>
>      <p2mp-endpoints> ::=
>        <endpoint> [<endpoint-restriction-list>]
>        [<endpoint> [<endpoint-restriction-list>]]...
>
>
>    For endpoint type Point-to-Multipoint, several endpoint objects MAY
>    be present in the message and each represents a leave, exact meaning
>    depend on the endpoint type defined of the object.
>
> If all <endpoint>s represent leaves, then how is the head node
> specified?
>
>
[MC] In case of P2P the first endpoint is the ingress and the second is
egress.
In case of P2MP, the first endpoint is root and and anything else is
leaf. the RBNF is changed  mark that at least 2 endpoints must be there.

> I couldn't find a full spcification for some of the fields in the XRO
> Label subobject (Section 2.7) by chasing the indicated references (see
> COMMENT).
>
>
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
>
> Section 1
>
> Please expand OTN and WSON on first use.
>
> [MC] Agree

> Section 1.4
>
> It's very unclear to me what kind of support, from/by what entities/data
> structures, under what conditions, these tables are attempting to
> indicate.
>
> [MC] The tables attempt to identify which existing PCEP objects are able
to meet the requirements described in RFC7025. The Tables are split to
match RFC7025 way of separating the requirements.

> We should probably be consistent whether we talk about just "FOO" or
> "FOO object" as the hanging text for these bulleted lists.
>
>
[MC] I aligned on "Foo"

>    From [RFC8282]:
>
>    o  SWITCH-LAYER: address requirements (1, 2 and 3) for the TE-LSP and
>       indicates which layer(s) should be considered, can be used to
>       represent the RSVP-TE generalized label request.  [...]
>
> nit: this looks like a comma splice.
>

[MC] Agree

>
>    The PCEP extensions defined later in this document to cover the gap
>    are:
>
>       Two new object types are introduced for the BANDWIDTH object
>       (Generalized bandwidth, Generalized bandwidth of existing TE-LSP
>       for which a reoptimization is requested).
>
> I'm confused by this language "new object types are introduced for the
> BANDWIDTH object".  My understanding was that objects did not nest: that
> is, objects have a given structure and can sometimes contain TLVs, but
> do not contain other objects.  So, my current understanding is that new
> objects are introduced that can appear where the BANDWIDTH object would
> previously have appeared, but they are separate object (type)s from the
> RFC 5440 BANDWIDTH objects.  (This language is used in the next couple
> items as well.)  To be clear, this is at most an editorial
> consideration, essentially whether to use "introduced for" or something
> like "introduced akin to".
>
>

[MC] The document means to define two new object types for object class 5,
would the following ( also in subsequent type defintion)
clarify this point?

     Two new object types are defined for the BANDWIDTH object
      (Generalized bandwidth, Generalized bandwidth of existing TE-LSP
      for which a reoptimization is requested).

> Section 2.1.2
>
>                                                   If the PCE does not
>    include the GMPLS-CAPABILITY TLV in the OPEN message and the PCC does
>    include the TLV, it is RECOMMENDED that the PCC indicates a mismatch
>    of capabilities.  Moreover, in case that the PCC does not receive the
>
> Indicate how, to whom?
>
> [MC] The negotiation is now mandatory

NEW

   If the PCEP speaker supports the extensions of this specification but
   did not advertise the GMPLS-CAPABILITY capability, upon receipt of a
   message from the PCE including an extension defined in this document,
   it MUST generate a PCEP Error (PCErr) with Error-Type=10 (Reception
   of an invalid object) and Error-value=TBA-42 (Missing GMPLS-

   CAPABILITY TLV), and it SHOULD terminate the PCEP session.

> Section 2.2
>
> This granularity applies to all links in the path, right?  So I can't
> request label-level granularity for one hop and indicate that I only
> care about node-level granularity for the other hops?
>

[MC] Correct, in the context of GMPLS and transport networks the
granularity generally applies to the path.
In some case several requests to the PCE can be made for different section
of the path, or the PCE can use some policies to reduce the granularity
level (for instance based on the different administrative domains)

> ection 2.3
>
> [similar comments apply here to what I mentioned at the end of Section
> 1.4]
>
>    The Bw Spec Type correspond to the RSVP-TE SENDER_TSPEC (Object Class
>    12) C-Types
>
> Should we ask IANA to update the SENDER_TSPEC registry to note that it
> is used for PCEP as well as RSVP?
>
>

[MC] We can, but that also apply to other registries:
  - EXPLICIT_ROUTE)
  - ROUTE_RECORD
Those were not changed (since RFC5440)

>    The encoding of the fields Generalized Bandwidth and Reverse
>    Generalized Bandwidth is the same as the Traffic Parameters carried
>    in RSVP-TE, it can be found in the following references.
>
>                       Object Type Name      Reference
>
>                       2           Intserv   [RFC2210]
>                       4           SONET/SDH [RFC4606]
>                       5           G.709     [RFC4328]
>                       6           Ethernet  [RFC6003]
>                       7           OTN-TDM   [RFC7139]
>                       8           SSON      [RFC7792]
>
> It's quite confusing to have the table heading be just "object type"
> when this is the value in the field named "Bw Spec Type" and corresponds
> to class type values in the SENDER_TSPEC registry.
>
> [MC] I changed it to "Bw Spec Type"

> Also, I looked up the Intserv case, and RFC 2210 doesn't really give me
> a clear picture of what I'm supposed to encode as the "transport
> parameters".  I think it's supposed to be the 12-octet assembly
> consisting of the token bucket rate, token bucket size, and peak data
> rate, but I have very low confidence in that assessment.  On the other
> hand, RFC 4606 has a very nice data structure layout in Section 2.1,
> "SONET/SDH Traffic Parameters".  On the gripping hand, there's not a
> clear "bandwidth" number in that structure that I can apply a comparison
> to for load-balancing purposes.  It doesn't look like I'll have time to
> check the other four cases right now, but that will need to be done
> before final publication.
>
>
[MC] In general the BW Spec type would match one another, but its also
possible to ask for 100G Ethernet and split it across 10 ODU2 LSPs, if
the endpoints are supporting that. This is technology, network and
node-architecture specific. A PCE for that domain would know how to
process that. In the case of the PCE not being able to compare those
bandwidth for load-balancing purpose, a NO-PATH should be returned.

> Section 2.4
>
> I'm having trouble parsing:
>
>    The LOAD-BALANCING object [RFC5440] is used to request a set of
>    maximum Max-LSP TE-LSP having in total the bandwidth specified in
>    BANDWIDTH, each TE-LSP having a minimum of bandwidth.
>
> Is it intended to read:
>
>    The LOAD-BALANCING object [RFC5440] is used to request allocation of a
> set of
>    at most Max-LSP TE-LSPs, having in total the bandwidth specified in
>    BANDWIDTH, with each TE-LSP having at least a specified minimum
> bandwidth.
>
> ?
>

[MC] Yes, the text has been changed accordingly

>
> [similar comments apply here to what I mentioned at the end of Section
> 1.4]
>
>    Bandwidth Spec Length (16 bits): the total length of the Min
>    Bandwidth Spec field.  It is to be noted that the RSVP-TE traffic
>    specification MAY also include TLV different from the PCEP TLVs.  The
>    length MUST be strictly greater than 0.
>
> It's not entirely clear to me why the note about different TLVs in
> RSVP-TE and PCEP belongs here.
>
>
[MC] Its indeed better in section 2.3 (added reference to RFC6003)

> Section 2.5.1
>
>               Endpoints label restriction may not be part of the RRO or
>    IRO, they can be included when following [RFC4003] in signaling for
>    egress endpoint, but ingress endpoint properties can be local to the
>    PCC and not signaled.  [...]
>
> nit: the first comma looks like a comma splice.
>
>                       A PCE not supporting a given Endpoint Type SHOULD
>    respond with a PCErr with Error Type 4, Value TBD "Unsupported
>    endpoint type in END-POINTS Generalized Endpoint object type".  [...]
>
> s/TBD/TBA-15/
>
> [MC] Agree

>                                              The TLVs present in the
>    request object body MUST follow the following [RFC5511] grammar:
>
> It feels a bit like a type error to use RBNF to describe the layout
> of TLVs within a TLV block, as RBNF acts on objects.
>
>
[MC] I re-checked RFC5511, the object as defined in RFC5511 is an
indivisible rule, its not bound to a specific encoding.

> Section 2.5.2.4
>
>    The LABEL-REQUEST TLV indicates the switching capability and encoding
>    type of the following label restriction list for the endpoint.  Its
>    format and encoding is the same as described in [RFC3471] Section 3.1
>    Generalized label request.  [...]
>
> Presumably the "Its" refers to just the value portion of the TLV?
> That should probably be stated explicitly.
>
> [MC] Agree

> Section 2.5.2.5
>
> Is there any reason for the section title to not be "LABEL-SET TLV" for
> consistency with the other sections?
>
>    A LABEL-SET TLV represents a set of possible labels that can be used
>    on an interface.  If the L bit is cleared, the label allocated on the
>    first endpoint MUST be within the label set range.  [...]
>
> Is this MUST binding on the PCC that generates a request, or on the
> computed LSP returned by the PCE?
>
> [MC] The MUST is on the speaker doing the allocation. With explicit
label control, the PCE is the one doing it, but the PCE MAY send a
PCRep and add the LABEL-SET for the PCC to allocate it.

>    A LABEL-SET TLV with the O and L bit set MUST trigger a PCErr message
>    with error type="Reception of an invalid object" error value="Wrong
>    LABEL-SET TLV present with O and L bit set".
>
>    A LABEL-SET TLV with the O bit set and an Action Field not set to 0
>    (Inclusive list) or containing more than one subchannel MUST trigger
>    a PCErr message with error type="Reception of an invalid object"
>    error value="Wrong LABEL-SET TLV present with O bit and wrong
>    format".
>
>    If a LABEL-SET TLV is present with O bit set, the R bit of the RP
>    object MUST be set, otherwise a PCErr message MUST be sent with error
>    type="Reception of an invalid object" error value="LABEL-SET TLV
>    present with O bit set but without R bit set in RP".
>
> nit: I don't know if it makes more sense to use the TBA-25, TBA-26, and
> TBA-24 values in these descriptions.
>
> [MC] I added the TBA-xx in addition to the text

> Section 2.6
>
>    The IRO as defined in [RFC5440] is used to include specific objects
>    in the path.  RSVP-TE allows to include label definition, in order to
>    fulfill requirement 13 of [RFC7025] the IRO needs to support the new
>    subobject type as defined in [RFC3473]:
>
> nit: this looks like a comma splice.  (A similar construction appears in
> Section 2.7 as well.)
>
> [MC] Agree

> Section 2.7
>
>       U (1 bit): see [RFC3471].
>
>       C-Type (8 bits): the C-Type of the included Label Object as
>       defined in [RFC3471].
>
>       Label: see [RFC3471].
>
> Sorry, where exactly in RFC 3471?  I do not see discussion of a U bit
> or C-Type therein.  (Perhaps RFC 3473 was intended?  Though, RFC 3473
> seems to refer back to 3471 for the U parameter, again without section
> reference.)
>
> [MC] The reference should be RFC3473 for th C-Tye, the U bit is
described in RFC3471 section 6.1
The Label field is technology-dependent and defined in RFC3471, the
per-technology labels are defined in the technology-specific RFCs.

> Section 6
>
> It seems that a malicious PCC might be able to effect a denial of
> service attack on the PCE by attempting to make many requests that
> consume lots of resources (whether on the PCE itself or in the managed
> network elements).
>
> [MC] Correct, this is addressed bu RFC5440 Section 10.7.2.

>                  In addition Technology specific data plane mechanism
>    can be used (following [RFC5920] Section 5.8) to verify the data
>    plane connectivity and deviation from constraints.
>
> nit: "In addition, technology-specific"
>
> [MC] Agree

> Appendix A
>
> It's not entirely clear to me why this specific group of examples was
> chosen and no others.  (The appendix does not seem to be referenced from
> elsewhere in the document, so it appears fairly random to a reader
> making it that far.)
>
>
> [MC] This was moved from the LOAD-BALANCING section, added a reference
to it in LOAD-BALANCING.

> _______________________________________________
> Pce mailing list
> Pce@ietf.org
> https://www.ietf.org/mailman/listinfo/pce
>