Hi Gunter, Thanks for a detailed review.
On Tue, Apr 2, 2024 at 10:49 PM Gunter Van de Velde via Datatracker < nore...@ietf.org> wrote: > Gunter Van de Velde has entered the following ballot position for > draft-ietf-pce-segment-routing-ipv6-22: No Objection > > 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/about/groups/iesg/statements/handling-ballot-positions/ > for more information about how to handle DISCUSS and COMMENT positions. > > > The document, along with other ballot positions, can be found here: > https://datatracker.ietf.org/doc/draft-ietf-pce-segment-routing-ipv6/ > > > > ---------------------------------------------------------------------- > COMMENT: > ---------------------------------------------------------------------- > > Please find this review using a fresh pair of eyes upon the draft. feel > free to > use or ignore these comments. Comments are ordered with some first set of > idnit > typo's, followed with observations when reading the document. > > **Idnits:** > > 349 Endpoint node as well as the tailend node also need to be > considered > > I believe that the grammatically correct form is "tail-end," which refers > to > the final part of something, such as a process, activity, or physical > object. > Using a hyphen clarifies that the two words function together as a single > concept. Not sure if there is earlier art that uses the term with the > proposed > spelling in the document? > > Dhruv: I checked; authors - please change to tail-end! > 659 PCE. As such,the flags MUST be set to zero and a (MSD-Type,MSD- > > s/such,the/such, the/ > > 635 mechanisms, e.g routing protocols [RFC9352], then it ignores the > > s/e.g/e.g./ > > 653 SRv6 MSD capabilties, the PCC MUST send a PCErr message with > Error- > > s/capabilties/capabilities/ > > Dhruv: Ack for above! > **Observations when reading through the document:** > > 20 Segment Routing (SR) can be used to steer packets through an > IPv6 or > 21 MPLS network using the source routing paradigm. SR enables any > head- > 22 end node to select any path without relying on a hop-by-hop > signaling > 23 technique (e.g., LDP or RSVP-TE). > > Proposed rewrite > Segment Routing (SR) can be used to steer packets through a network using > the > IPv6 or MPLS data plane, employing the source routing paradigm. SR enables > any > head-end node to select any path without relying on hop-by-hop signaling > techniques (e.g., LDP or RSVP-TE). > > Dhruv: Thanks for the rewrite, it reads better! > 29 Since SR can be applied to both MPLS and IPv6 forwarding > planes, a > 30 PCE should be able to compute an SR Path for both MPLS and IPv6 > 31 forwarding planes. > > I suspect we are talking dataplane instead of forwarding plane? I see the > terms > "forwarding plane" and "data plane" often used interchangeably, but they do > seem to have nuanced differences. The forwarding plane deals with the > logical > decision-making process of packet forwarding, the data plane deals with the > physical implementation of forwarding those packets based on those > decisions. > In addition the term dataplane is used later in this same abstract. Maybe > best > to use single terminology (maybe dataplane) through the document? > > Dhruv: Looking at spring RFCs I see a mix of terms but when talking about SR instantiation as SR-MPLS and SRv6, the term data-plane is used and thus we should also use the same. > 31 forwarding planes. The PCEP extension and mechanisms to > support SR- > 32 MPLS have been defined. > > What about adding the reference to RFC5440? > Dhruv: I would avoid adding references in the abstract. > > 32 MPLS have been defined. This document describes the extensions > 33 required for SR support for the IPv6 data plane in the Path > 34 Computation Element communication Protocol (PCEP). > > This text reads a bit odd. What about a readability rewrite: > “This document outlines the necessary extensions to support Segment Routing > (SR) for the IPv6 data plane within the Path Computation Element > Communication > Protocol (PCEP).” > > Dhruv: Ack, with slight modification as the whole para can be read as - "Since SR can be applied to both MPLS and IPv6 data-planes, a PCE should be able to compute an SR Path for both MPLS and IPv6 data-planes. The Path Computation Element communication Protocol (PCEP) extension and mechanisms to support SR-MPLS have been defined. This document outlines the necessary extensions to support SR for the IPv6 data-plane within PCEP." > 126 When the SR architecture is applied to the MPLS forwarding > plane, it > 127 is called SR-MPLS. When the SR architecture is applied to the > IPv6 > 128 data plane, is is called SRv6 (Segment Routing over IPv6 data > plane) > 129 [RFC8754]. > > See earlier comments. Data plane vs forwarding plane. > > 133 IGP SPT. Such paths may be chosen by a suitable network > planning > 134 tool, or a PCE and provisioned on the ingress node. > > The correlation between PCE and suitable network planning tool is unclear. > Can > the following text be used to close down on the ambiguity: “Such paths can > be > selected either by an appropriate network planning tool or by a Path > Computation Element (PCE) and then provisioned on the ingress node.” > > Dhruv: The text is borrowed for RFC8664 but I agree that using Network Controller is a better term. > 143 [RFC8231] specifies extensions to PCEP that allow a stateful > PCE to > 144 compute and recommend network paths in compliance with > [RFC4657] and > 145 defines objects and TLVs for MPLS-TE LSPs. Stateful PCEP > extensions > > I am unclear what 'recommend' means in this context? Can this be better > explained and clarified? In RFC8231 there is no mentioning of recommended > paths. > Dhruv: In RFC 8231 you will note "LSP Update Request" i.e. PCE is requesting the PCC to update and PCC can report that the update is rejected. Also RFC 8664 uses similar framing. This could be updated to "compute and update" but it then loses the point that the PCC is free to reject. I will let this be... > > 157 account various constraints and objective functions. Once a > path is > 158 chosen, the stateful PCE can initiate an SR-TE path on a PCC > using > 159 PCEP extensions specified in [RFC8281] and the SR-specific PCEP > > “Once a path is chosen” seems to imply that there are multiple paths > calculated > and the best one is selected or chosen. Is this what is implied with this? > > Dhruv: Yes, 'computed' is better than 'chosen'! > 161 extensions for supporting a SR-TE LSP for the MPLS data plane. > This > 162 document extends [RFC8664] to support SR for the IPv6 data > plane. > 163 Additionally, using procedures described in this document, a > PCC can > 164 request an SRv6 path from either a stateful or stateless PCE. > This > 165 specification relies on the PATH-SETUP-TYPE TLV and procedures > 166 specified in [RFC8408]. > > This section is explaining what this draft is standardizing. It is a bit > hidden > and tucked all the way in the back of the introduction, a bit less trivial > for > the reader to discover. > > Dhruv: This can be put in its own paragraph. > 168 This specification provides a mechanism for a network controller > 169 (acting as a PCE) to instantiate candidate paths for an SR > Policy > 170 onto a head-end node (acting as a PCC) using PCEP. For more > > Before there was mentioning of a “network planning tool”. Maybe instead the > term network controller can be used? > > Dhruv: agree > 212 Basic operations for PCEP speakers are as per [RFC8664]. SRv6 > Paths > 213 computed by a PCE can be represented as an ordered list of SRv6 > 214 segments > > Reading this gives wrong indication that RFC8664 computes SRv6 paths. In > the > RFC8664 is explicitly written that “This document is relevant to the MPLS > forwarding plane only.” > > Dhruv: maybe "built on" instead of "as per". > 250 In SR networks, an SR source node encodes all packets being > steered > 251 onto an SR path with a list of segments. > > “SR source node”. I am unsure what this refers towards. Would this be the > segment routing ingress node? In Segment Routing (SR), the ingress node is > known by the fact that it is the node where the packet enters the Segment > Routing domain. When a packet enters a network that employs Segment > Routing, it > is typically tagged with a Segment List at the ingress node. > > Dhruv: RFC 8754 and RFC 8986 use the term. Maybe we can reframe this as - "In SR networks, an SR source node [RFC8754] steers a packet into an SR Policy resulting in a segment list." > 363 order to indicate that the path is for SRv6, any RP or SRP > object > > These acronyms are not specified in the terminology section: Request > Parameters > (RP) [RFC5440] and the Stateful PCE Request Parameters (SRP) > > Dhruv: They are expanded on first use in section 3.2 > 398 The 'L' Flag: Indicates whether the subobject represents a > loose-hop > 399 (see [RFC3209]). If this flag is set to zero, a PCC MUST NOT > 400 overwrite the SID value present in the SRv6-ERO subobject. > 401 Otherwise, a PCC MAY expand or replace one or more SID values > in the > 402 received SRv6-ERO based on its local policy. > > The exact meaning of L-flag is confusing for SRv6. When looking at RFC3209 > it > reflects upon nodes, however with SRv6 this may be an adj-SID or some other > instruction. Maybe the L-flag can be enhanced to described what this means > in > the context of SRv6 SID. > > Dhruv: The text is the same as RFC 8664 (SR-MPLS) and within the context of PCEP it means that PCC should use the SID as it is v/s PCC being able to apply suitable changes. If the intention is that the path as computed by PCE should be used as it is, it is expected that the L flag would be set accordingly. > From RFC3209: > The path between a strict node and its preceding node MUST include > only network nodes from the strict node and its preceding abstract > node. > > 438 Flags: Used to carry additional information pertaining to the > 439 SRv6-SID. This document defines the following flag bits. The > other > 440 bits MUST be set to zero by the sender and MUST be ignored by > the > 441 receiver. > > There is mentioning of S/F/T/V. is there a reason they are called like > that? I > suspect I am missing the history of naming of these flags and it just looks > mostly random at this stage > > Dhruv: The trend that started from RFC 8664 :) > 475 SRv6 SID: SRv6 Identifier is an 128-bit value representing the > SRv6 > 476 segment > > Any special considerations for csid? > Dhruv: As stated in the compression draft, no change is needed in PCEP. > 481 At least one SRv6-SID or the NAI MUST be included in the > SRv6-ERO > 482 subobject, and both MAY be included. > > Is there any checking or processing to check if the NAI and SRV6-SID > belong to > the same node? Can they belong to different nodes? > > Dhruv: Note that such a such is not done in SR-MPLS either, I would assume that the SID value trumps NAI. > 731 If a PCC receives an SRv6 path that exceeds the SRv6 MSD > 732 capabilities, it MUST send a PCErr message with Error-Type = 10 > 733 ("Reception of an invalid object") and Error-Value = 43 > ("Unsupported > 734 number of SRv6-ERO subobjects") as per [RFC8664]. > > I assume this is about exceeding the local PCC capabilities? A local PCC > router > may have enough intelligence to understand the capability of all nodes > through > which the datapacket will be steered. In theory the encoded payload may > traverse a node that is not capable to process the SRH pushed by the SR PCC > ingress router. > > Dhruv: Yes, this is about local. > 738 The SRv6-ERO contains a sequence of subobjects. According to > 739 [RFC9256], each SRv6-ERO subobject in the sequence identifies a > 740 segment that the traffic will be directed to, in the order > given. > 741 That is, the first subobject identifies the first segment the > traffic > 742 will be directed to, the second SRv6-ERO subobject represents > the > 743 second segment, and so on > > Is there expectation that the node of a NAI corresponds with the node > owning a > SRv6-SID > Dhruv: Yes > > 771 Note that this specification enables a network controller to > 772 instantiate an SRv6 path in the network. This creates an > additional > > Would it be more correct to indicate that it enables both to initiate and > to > monitor an SRv6 path? > > > Dhruv: The security vulnerability is more about instantiation. Authors, please feel free to add your comments especially if you disagree with my responses here. Thanks! Dhruv
_______________________________________________ Pce mailing list Pce@ietf.org https://www.ietf.org/mailman/listinfo/pce
