Path Computation Element Communication Protocol (PCEP) Extensions for Establishing Relationships between Sets of Label Switched Paths (LSPs)
draft-ietf-pce-association-group-10

Thanks to the shepherd for noting and explaining the number of authors, LGTM++

Thank you  for the work put into this document. I have two COMMENTs and one nits: all easy to fix

Regards,

-éric

== COMMENTS ==

-- Section 6.1 --

Please state the obvious: unused "Flags" bits MUST be set to 0.

-- Section 6.1.1 --

Figure 5 includes the type and length of the TLV while figure 1 in previous section does not include the type and length field. Or am I misreading the figures ?

== NITS ==

-- Section 4.1 (and others) --

The caption of figure 1 contains "TLV" but it is actually only about the "V" of the "TLV" ;-) See above COMMENT

Please expand "PCEP" in the document title.

(1) s/before hand/beforehand/g

(2) §5: "Start-Assoc-ID...The values 0 and 0xffff MUST NOT be used."  What should the receiver do if they are?

(3) §5: "Range...it MUST be such that (Start-Assoc-ID + Range) do not cross the association identifier range of 0xffff."  What should the receiver do if it does?

(4) s/is OPTIONAL and MAY/MAY/g   OPTIONAL = MAY

(5) §9.2: "An implementation SHOULD allow...Further implementation SHOULD allow... To serve this purpose, the PCEP YANG module [I-D.ietf-pce-pcep-yang] includes association groups."  If I-D.ietf-pce-pcep-yang is the mechanism that addresses these Normative statements, then it should be a Normative reference.  I think that it is not necessary to point at I-D.ietf-pce-pcep-yang in this document.

(6) RFC8126 should be a Normative reference.

A tiny thing, trivial to fix, and Dhruv has already put it in his working copy:

— Section 2 —

   This document uses the following terms defined in [RFC8051]: Stateful
   PCE, Active Stateful PCE, Passive Stateful PCE, and Delegation.

I think this makes 8051 a normative reference.

I'm always a little reluctant to publish framework documents without any
examples of using that framework (i.e., this document does not define
any association types), but this work seems well thought-out and it
looks like there are a handful of association types in development by
the WG.  Would it be worth listing a few as informational references to
give the reader a broader sense of what associations might be used for?

Thanks for the note in the shepherd writeup about the author count!

Thank you also for the very readable document -- it's clear that the
authors took care to organize the content in a manner accessible to
the reader.

As a general note, do we need to say anything about the multi-byte
integer values being encoded in network byte order?  (Perhaps following
RFC 5440's implicit convention is the right thing to do.)

Section 1

   [RFC6780] defines the RSVP ASSOCIATION object, which was defined in
   the context of GMPLS-controlled Label Switched Paths (LSPs) to be

nit: is this supposed to be  RFC 4872?

Section 3.3

   The dynamically created association can be reported to the PCEP peer
   via the PCEP messages as per the stateful extensions.  While the
   operator-configured association is known to the PCEP peer before
   hand, a PCEP peer could ask for a LSP to join the operator-configured
   association via the stateful PCEP messages.

nit: I suggest s/While/When/, if I understand correctly.

   Multiple types of associations can exist, each with their own
   association identifier space.  The definition of the different
   association types and their behaviours is outside the scope of this
   document.  The establishment and removal of the association
   relationship can be done on a per LSP basis.  An LSP may join
   multiple association groups, of different or of the same association
   type.

Is it possible for an LSP to join multiple association groups of the
same type and then for configuration to be assigned to two groups in a
manner that conflicts?  What procedure is used for conflict resolution
in such a case?

Section 3.4

   Association identifier range for sources other than the PCEP speaker
   (for example an NMS system) is not communicated in PCEP and the
   procedure for operator-configured association range setting is
   outside the scope of this document.

Given the discussion in the rest of the document, it seems that
reserving a specific range for operator configuration (across all
association types) is too rigid to meet the various anticipated use
cases.  Is that a correct assessment?

Section 5.1

   If the Assoc-type is not recognized or supported by the PCEP speaker,
   it MUST ignore that respective Start-Assoc-ID and Range.  If the
   Start-Assoc-ID or Range are set incorrectly, the PCEP session MUST be
   rejected with error type 1 and error value 1 (PCEP session
   establishment failure / Reception of an invalid Open message).

I could maybe see competent engineers disagreeing about which of these
MUSTs would take precedence in a case where both apply.

   The Assoc-type MAY appear more than once in the OP-CONF-ASSOC-RANGE
   TLV in the case of a non-contiguous Operator-configured Association
   Range.  The PCEP speaker originating this TLV MUST NOT carry
   overlapping ranges for an association type.  If a PCEP peer receives
   overlapping ranges for an association type, it MUST consider the Open
   message malformed and MUST reject the PCEP session with error type 1
   and error value 1 (PCEP session establishment failure / Reception of
   an invalid Open message).

This might be a good place to specify the  handling if a
received range would cross the 0xffff boundary.

Section 6.1.1

   The Global Association Source TLV is an optional TLV for use in the
   Association Object.  The meaning and the usage of Global Association
   Source is as per [RFC6780].

Do we want to say Section 4 specifically of 6780?
(Similarly for Extended Association ID.)

Section 6.1.4

   the association group.  In this document, all these fields are called
   'association parameters'.  Note that the ASSOCIATION object MAY

I would humbly suggest s/all these fields are called 'association
parameters'/these fields are collectively called 'association
parameters'/.

Section 6.3.1

   The ASSOCIATION Object is OPTIONAL and MAY be carried in the Path
   Computation Update (PCUpd), Path Computation Report (PCRpt) and Path
   Computation Initiate (PCInitiate) messages.

   When carried in PCRpt message, it is used to report the association
   group membership pertaining to a LSP to a stateful PCE.  The PCRpt
   message are used for both initial state synchronization operations
   (Section 5.6 of [RFC8231]) as well as whenever the state of the LSP
   changes.  The associations MUST be included during the state
   synchronization operations.

I suspect this is just my hazy memory of OPTIONAL, but how does "MUST be
included" match up with "OPTIONAL".

Section 6.4

Do I understand correctly that for dynamically created association
groups, the creation is effected by just using the relevant parameters
in a request(/update/etc.) and there is no need to separately create or
allocate the association?

   If a PCE peer is unwilling or unable to process the ASSOCIATION
   object, it MUST return a PCErr message with the Error-Type "Not
   supported object" and follow the relevant procedures described in
   [RFC5440].  [...]

Does this imply that the P flag in the common header should always be
set for ASSOCIATION objects?

   In case the LSP is delegated to another PCE on session failure, the
   associations (and association information) set by the PCE remains
   intact, unless updated by the new PCE that takes over.

This includes the association source address?

Section 8

   attack vector.  An attacker could report too many associations in an
   attempt to load the PCEP peer.  The PCEP peer responds with PCErr as

"report" in the sense of causing the peer to create state to track them?

Section 12.2

Since the RFC 7525 procedures are RECOMMENDED, I think that reference
needs to be normative.

Hi,

thanks for this document, here are a couple of comments/questions.

   The PCEP ASSOC-Type-List TLV is optional.  It MAY be carried within
   an OPEN object sent by a PCEP speaker in an Open message to a PCEP
   peer so as to indicate the list of supported Association types.
This is said twice. (First paragraph of section 4.1) and then in 4.1.1:
   A PCEP speaker MAY include an ASSOC-Type-List TLV within an OPEN
   object in an Open message sent to a PCEP peer in order to advertise a
   set of one or more supported association types. 
   The use of ASSOC-Type-List TLV is OPTIONAL.

It doesn't hurt, but you might want to consider saying this only once. Also I note OPTIONAL vs optional

Sending ASSOC-Type-List TLV is optional but it might be mandatory to send some to-be-defined Association types. Isn't that somehow conflicting?

The PCEP OP-CONF-ASSOC-RANGE TLV is optional.
OPTIONAL?

Could you clarify the difference between
a PCEP speaker does not recognize the ASSOCIATION object
and
a PCE peer is ... unable to process the ASSOCIATION
I see that the errors thrown are different.


Nits:
s/protections LSPs/protection LSPs/
s/The Assoc-type MAY appear more than once/A given Assoc-type MAY appear more than once/
s/to uniquely identifying/to uniquely identify/

nit: s/The association type are defined/The association types are defined/

* Section 6.1.

   Association Source: 4 or 16 bytes - A valid IPv4 or IPv6 address that
   provides scoping for the Association ID.  See Section 6.1.3 for
   details.

If I understand correctly, the length of this field is not really 4 or 16 bytes but rather fully dependent on the ASSOCIATION Object-Type. i.e. you cannot have a 16 byte address here if the Object_Type is 1. If so, it would be good to state this dependence explicitly. Suggest something like

  Association Source: Contains a valid IPv4 address (4 bytes) if  the ASSOCIATION 
  Object-Type is 1 or a valid IPv6 address (16 bytes) if  the ASSOCIATION 
  Object-Type is 2. It provides scoping for the Association ID.  See Section 6.1.3 for details.