Conveying Path Setup Type in PCE Communication Protocol (PCEP) Messages
draft-ietf-pce-lsp-setup-type-10

People already nitpicked on definition of "reserved", so I will not :-)

(1) The Length field in S3 has no units.  I'm sure people can guess it is in bytes, from the rest of the description, but it should be explicit.

(2) The Reserved fields "MUST be set to zero".  What happens if they're not?  Typically they are also ignored by the receiver...

(3) S3: "Each sub-TLV MUST obey the rules for TLV formatting defined in ([RFC5440]).  That is, each sub-TLV MUST be padded to a four byte alignment, and the length field of each sub-TLV MUST NOT include the padding bytes."  The first sentence is ok.  The second one tries to paraphrase what rfc5440 says -- but rfc5440 doesn't say that, it doesn't even use Normative language!  This is the text from rfc5440:

   The Length field defines the length of the value portion in bytes.
   The TLV is padded to 4-bytes alignment; padding is not included in
   the Length field (so a 3-byte value would have a length of 3, but the
   total size of the TLV would be 8 bytes).

(3a) The text in this document shouldn't use Normative language to describe what rfc5440 says and specifies.

(3b) Note that the text from rfc5440 (specifically the part about "padding is not included in the Length field") is not aligned with the description of the Length field in this document: "The TLV Length field MUST be equal to the size of the appended sub-TLVs plus the size of the PST list (rounded up to the nearest multiple of four) plus four bytes."  Rounding up includes the padding.

(4) S6: "Each document that introduces a new path setup type to PCEP must include a manageability section."  Why is a Normative "MUST" not used?

(5) rfc6123 is a Historic document.  Maybe a reference to rfc5706 is more appropriate (even in addition to rfc6123).

Substantive Comments:

§1.1: There are at least a few instances of lower case versions of 2119 keywords. Please consider using the boilerplate from RFC 8174.

§7:
Doesn't this need to say something about the possible security considerations when adding new path setup types ?

Editorial Comments and Nits:

§5: "... it MUST consider that the peer suports only ...: I think perhaps "consider" should have been "assume"? Also, s/suports/supports.

I'm concerned about defining the space for optional sub-TLVs in PATH-SETUP-TYPE-CAPABILITY but
not giving much description of how future sub-TLVs would work (since none are currently defined).
Is there expected to be a one-to-one mapping from PST to sub-TLV, or one-to-many, or something else?
If a given sub-TLV can be associated with more than one PST, some rules would need to be specified for
how that mapping works, what dependency there is on the order in which sub-TLVs appear in the message,
etc.  I am not balloting DISCUSS because I suspect the intent is for each sub-TLV to correspond to exactly one
PST, in which case the behavior is pretty easy.  But I would like to see more description of how this is expected
to work.

Both new TLVs have 'Reserved' fields that MUST be set to zero.  Should they be ignored on receipt (to allow
for potential future use as an extension) or can the receiver validate that they are zero?

The Security Considerations defer to RFCs 5440 and 8281, which (as the secdir review notes) is mostly okay.
RFC 5440 does have a long discussion of the value of TCP authentication, but IIUC it does not mandate that
TCP authentication be used.  That would leave open the possibility that an attacker (e.g., TCP MITM) could
generate error messages when a particular PST is used, potentially forcing the use of a different PST, and this
attacker capability seems to be new in this document.  As such, it would merit a mention in the Security Considerations.
(This attack only becomes relevant in the face of some additional weakness or flaw in a PST that makes forcing
its use advantageous to the attacker for other reasons.)

Please expand RP and SRP on first use.

What is the backward compatibility story here? I.e., say I only support method X and the peer doesn't know about this TLV at all? How will it behave?

This is a major comment, I am not balloting DISCUSS at this time as author's intentions may have been to do the right thing, and it has simply slipped or not yet done. 

Manageability section lists the right questions, but there are no answers to those questions. Could authors please answer the questions in section 9? 

Would capabilities be augmenting the PCEP YANG model, under entity or peer branches? It does not seem that there is a need for a separate model just for this extension?

Document says:
   If a PCEP speaker does not recognize the PATH-SETUP-TYPE-CAPABILITY
   TLV, it MUST ignore the TLV in accordance with ([RFC5440]).
and:
   If a PCEP speaker does not recognize the PATH-SETUP-TYPE TLV, it MUST
   ignore the TLV in accordance with ([RFC5440]).

I am fine with this but it does not say anything, explicitly, on what shall be the selected signalling protocol. I guess it will be RSVP-TE because I guess we treat this situation in the same way as if there had been no TLV. But since I am guessing, I feel it wouldn't hurt to be explicit.
On the assumption that my guesses are correct, what about adding, for example, the following sentence (or anything else you'd judge more appropriate):
   From a signalling protocol selection point of view, this situation is treated as if the ... TLV was absent.

Mostly editorial (and I guess already flagged by Alvaro and Martin):

This sentence (in sec 3 as well as the similar sentence in sec 4) should not use normative language because that basically non-sensical:
"If a PCEP speaker does not recognize the PATH-SETUP-TYPE-CAPABILITY
   TLV, it MUST ignore the TLV in accordance with ([RFC5440])."
Should be instead:
"If a PCEP speaker does not recognize the PATH-SETUP-TYPE-CAPABILITY
   TLV, it will ignore the TLV in accordance with ([RFC5440])."

I'll let you folks work with Benjamin on this, but I echo his concern about the level of specification covering sub-TLVs (Spencer's summary: "not much specification").  As a related comment, I note that not defining any sub-TLVs in this document prevents the authors from giving any examples of what sub-TLVs might be appropriate, which would have been helpful for me in both the Abstract and Introduction. 

(I usually prefer clues about whether the reader should be reading a specification or not. It would be easier for me to know whether this document is relevant to me, if I knew what kinds of sub-TLVs were envisioned, even if only a couple of examples were provided. But do the right thing, of course)