Re: [Pce] WG Last Call for draft-ietf-pce-segment-routing-11

["Adrian Farrel" <[e-mail]>]

Hi,

I reviewed this draft as part of the WG last call.

It is important work and needs to be published as an RFC.

However, there are some places where the clarity could be improved and
so increase the chances of interoperable implementations.

I have clustered the nits at the end of this email.

Thanks,
Adrian

=========

Section 3

   In SR networks, an ingress node of an SR path appends all outgoing
   packets with an SR header consisting of a list of SIDs (or MPLS
   labels in the context of this document).

Conventionally, "append" means to add to the end (although it can mean
to arbitrarily add). I think you want "prepend" although the sentence
is still clumsy and might be better as...

   In an SR network, the ingress node of an SR path prepends an SR 
   header to all outgoing packets.  The SR header consists of a list of
   SIDs (or MPLS labels in the context of this document).

---

Section 3 (same paragraph)


I think this is not quite true. Well, it is true that signaling is not
needed, but not true that the header has all the necessary 
information in all cases - the IGP may need to resolve a path to a
Node SID. Perhaps...

   The header has all
   necessary information so that in combination with the information 
   distributed by the IGP, the packets can be guided from the ingress
   node to the egress node of the path, and hence there is no need for
   any signaling protocol.

---

Section 3 para 2 has the same "append" problem, and also overstates what
is carried in the ERO. Suggest...

OLD
   In a PCEP session, LSP information is carried in the Explicit Route
   Object (ERO), which consists of a sequence of subobjects.  Various
   types of ERO subobjects have been specified in [RFC3209], [RFC3473],
   and [RFC3477].  In SR networks, an ingress node of an SR path appends
   all outgoing packets with an SR header consisting of a list of SIDs
   (or MPLS labels in the context of this document).  SR-TE LSPs
   computed by a PCE can be represented in one of the following forms:
NEW
   In PCEP messages, LSP route information is carried in the Explicit 
   Route Object (ERO), which consists of a sequence of subobjects.  
   Various ERO subobjects have been specified in [RFC3209], [RFC3473],
   and [RFC3477].  In SR networks, an ingress node of an SR path 
   prepends an SR header to all outgoing packets.  The SR header 
   consists of a list of SIDs (or MPLS labels in the context of this
   document).  SR-TE paths computed by a PCE can be represented in one
   of the following forms:
END


However, I wonder why you have picked just those three RFCs to reference.
Many other RFCs also define ERO subobjects. The full list is
RFC3209
RFC3473
RFC3477
RFC4874
RFC5520
RFC5553
RFC7570
RFC7898
RFC7897
...but listing them all would be odd.

---

Section 3

   o  An ordered set of IP address(es) representing network nodes/links:
      In this case, the PCC needs to convert the IP address(es) into the
      corresponding MPLS labels by consulting its Traffic Engineering
      Database (TED).

The PCC does not have a TED. It does have an LSDB as distributed by the 
IGP and this will contain the SRGBs and SIDs that allow labels to be
computed.

But I am surprised that this work is offloaded to the PCC since the PCE
has all the information to do this task. Why did the WG want to add this
option?

And then...

   o  An ordered set of SID(s)

s/SID(s)/SIDs/

This list of SIDs would need to be converted to labels by the PCC by 
applying the SRGB offsets. Again, why make the PCC do this?

And finally...

   o  An ordered set of both MPLS label(s) and IP address(es): In this
      case, the PCC needs to convert the IP address(es) into the
      corresponding SID(s) by consulting its TED.

This mixture of the previous two cases seems to compound the level of
complexity. Can't the PCE just know it is making an SR path and supply
a list of MPLS labels corresponding to the SIDs?

---

5.1.1 ---> 9

You should probably set up a registry for other SR PCE Capability sub-
TLV flags.

---
                                                 
5.1.2 ----> 6

   If a PCEP speaker receives a PATH-SETUP-TYPE-CAPABILITY TLV with a
   PST list containing PST=1, but the SR-PCE-CAPABILITY sub-TLV is
   absent, then the PCEP speaker MUST send a PCErr message with Error-
   Type 10 (Reception of an invalid object) and Error-Value TBD1 (to be
   assigned by IANA) (Missing PCE-SR-CAPABILITY sub-TLV) and MUST then
   close the PCEP session.

Suppose an implementation receives an Open with PST=1 but doesn't 
understand (or doesn't support) that value. Is it still required to
perform this check? Hopefully not.

---

5.3.1

The figure and the following text can be explicit about the value 36 
since it has already been assigned.

   Type  is the type of the SR-ERO subobject.  This document defines the
      SR-ERO subobject type, and requests a new codepoint from IANA.

---

5.3.1 (under Length) has

      As mentioned earlier, an SR-ERO
      subobject MUST have at least SID or NAI.

This is good and does tie in with what is written in earlier sections.

However, 5.3.1 starts with

   An SR-ERO subobject consists of a 32-bit header followed by the SID
   and the NAI associated with the SID.  The SID is a 32-bit number.
   The size of the NAI depends on its respective type, as described in
   the following sections.

That makes it sound like they are both mandatory, and the figure doesn't
help.

A little rewording would go a long way, and you could add "(optional)" 
to the figure.

---

5.3.1

   SID Type (ST)  indicates the type of information associated with the
      SID contained in the object body.  When ST value is 0, SID MUST
      NOT be null and NAI MUST be null.  

Does "null" mean "all zeros" or "absent"?

See also the definition of the S and F flags.

---

5.3.1

      Other ST values are described
      later in this document.

It would be friendly to provide a list somewhere.
Do you need a registry?

---

5.3.1

   Flags  is used to carry any additional information pertaining to SID.

You need to say how to set the unused bits.
Do you need a registry?

---

      *  M: When this bit is set, the SID value represents an MPLS label
         stack entry as specified in [RFC5462] where only the label
         value is specified by the PCE.  Other fields (TC, S, and TTL)
         fields MUST be considered invalid, and PCC MUST set these
         fields according to its local policy and MPLS forwarding rules.

"considered invalid"? You probably mean "ignored".

RFC5462 is not the right reference. It only renamed EXP to TC and does 
not define the other fields. You should reference 3032 and you will pick
up TC through the updates clause. (Ditto other places where the ref is
made).

---

5.3.2 has text like
      Length is 8, 20, or 24 depending on either
      SID or NAI or both are included in the subobject.

But NAI must be present or we wouldn't be talking about NAI types.
So, you need (e.g.)
      Length is 20 or 24 depending on whether
      the SID is absent or also included in the subobject.

---

5.3.2

OLD
   'IPv6 Node ID'  is specified as an IPv6 address.  In this case, ST
      and Length are 2, and Length is 8, 20, or 24 depending on either
      SID or NAI or both are included in the subobject.
NEW
   'IPv6 Node ID'  is specified as an IPv6 address.  In this case, ST
      is 2, and Length is 8, 20, or 24 depending on either SID or NAI
      or both are included in the subobject.

---

5.3.3

   If both M and C bits of an SR-
   ERO subobject are set, and if a PCEP speaker finds erroneous setting
   in one or more of TC, S, and TTL fields, it MUST send a PCErr message
   with Error-Type = 10 ("Reception of an invalid object") and Error-
   Value = 4 ("Bad label format").

But didn't the definition of the C flag say that a not MAY overwrite 
those fields according to local policy? So this is MUST send PCErr or
overwrite the fields.

---

5.3.3

   If a PCC receives a stack of SR-ERO subobjects, and the number of
   stack exceeds the maximum number of SIDs that the PCC can impose on
   the packet, it MAY send a PCErr message with Error-Type = 10
   ("Reception of an invalid object") and Error-Value = 3 ("Unsupported
   number of Segment ERO subobjects").

And if it doesn't send the PCErr, what should it do?

---

5.3.3

   When a PCEP speaker detects that all subobjects of ERO are not
   identical, and if it does not handle such ERO, it MUST send a PCErr
   message with Error-Type = 10 ("Reception of an invalid object") and
   Error-Value = 5 ("Non-identical ERO subobjects").

"Not identical" could have so many meanings here!
- Presumably you don't mean that all SIDs have the same value
- You might be referring to all flags
- You might be referring to just the M and C flags
- You might mean that the ERO must contain all SR-ERO subobjects or
  no SR-ERO subobjects

Please clarify and possibly rename the error value.

(See also 5.4.1).

---

5.3.3

   When a PCEP speaker receives an SR-ERO subobject in which ST is 0,
   SID MUST be present and NAI MUST NOT be present(i.e., S-flag MUST be
   0, F-flag MUST be 1, and the Length MUST be 8).  Otherwise, it MUST
   consider the entire ERO object invalid and send a PCErr message with
   Error-Type = 10 ("Reception of an invalid object") and Error-Value =
   11 ("Malformed object").  The PCEP speaker MAY include the malformed
   SR-ERO object in the PCErr message as well.

This text is good.
But it makes me think: what about the ST values 1 through 5 if the NAI is
absent?

---



========

Nits

Update to the new BCP14 (RFC2119/8174) language and place it as 
Section 2 of the document as required by RFC7322.

---

Need to expand "SR" on first use in the body of the document.

---

Section 1 para 1 has two cases where "SR architecture" should be "the
SR architecture".

---

S1P1

OLD
and is always global within SR/IGP domain
NEW
and is always identified uniquely within the SR/IGP domain
END

---

S1P1
s/e.g./e.g.,/

---

S1P1

OLD
within SR/IGP domain
NEW
an within SR/IGP domain
END

--

draft-ietf-pce-pce-initiated-lsp is now RFC8281

---

5.2 Took some time to parse...
OLD
   In order to setup an SR-TE LSP using SR, RP or SRP object MUST
   include PATH-SETUP-TYPE TLV
NEW
   In order to setup an SR-TE LSP using SR, the RP or SRP object MUST
   include PATH-SETUP-TYPE TLV
END

---

5.3 and 5.4

Please avoid saying "ERO Object" and "RRO Object"

---

Throughout the document, please don't do the optional plural thing.
E.g., "may contain one or more TLV(s)"

English likes you to use the plural even when the singular is possible.
E.g., "may contain one or more TLVs"

> -----Original Message-----
> From: Pce [mailto:[e-mail]] On Behalf Of Julien Meuric
> Sent: 15 January 2018 09:38
> To: [e-mail]
> Subject: [Pce] WG Last Call for draft-ietf-pce-segment-routing-11
> 
> Dear PCE WG,
> 
> Best wishes for this new year, full of interoperable specifications. Let
> us begin by resuming our work in progress.
> 
> This message starts a 2-week WG last call for
> draft-ietf-pce-segment-routing-11. Please send your feedback on the I-D
> to the PCE mailing list by Monday January 29.