Re: [Gen-art] Genart last call review of draft-ietf-spring-mpls-path-segment-08

[Cheng Li <[e-mail]>]

Hi Stewart, 

Thank you so much for your thorough review! It is really helpful!  Mach is quit busy recently, so he assigns me to address the comments. Sorry for a long delay.

Please see inline for the detailed reply. You can review the proposed update in Github as well. [1]

Authors, I converted the XML to markdown file for easier editing. Please review the proposed update from me. 
You can reply the email to cooperate or pull a request in Github as you like.

Respect,
Cheng


[1]. https://github.com/muzixing/SR-MPLS-Path-Segment/commit/0951b5c0fa52c8c63a2dc3ab2975b67c8e5537f4?diff=split




-----Original Message-----
From: Stewart Bryant via Datatracker <[e-mail]>
Sent: Monday, November 21, 2022 9:58 PM
To: [e-mail]
Cc: [e-mail]; [e-mail]; [e-mail]; [e-mail]
Subject: Genart last call review of draft-ietf-spring-mpls-path-segment-08

Reviewer: Stewart Bryant
Review result: Not Ready

I am the assigned Gen-ART reviewer for this draft. The General Area Review Team (Gen-ART) reviews all IETF documents being processed by the IESG for the IETF Chair.  Please treat these comments just like any other last call comments.

For more information, please see the FAQ at

<https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.

Document: draft-ietf-spring-mpls-path-segment-??
Reviewer: Stewart Bryant
Review Date: 2022-11-21
IETF LC End Date: 2022-11-20
IESG Telechat date: Not scheduled for a telechat

Summary: This document is not ready to progress. If it has not yet had one it could usefully be reviewed by the MPLS Review Teams as this is clearly MPLS technology that needs to validated by MPLS specialists. In my opinion the document really ought to go back to the Spring WG to have a number of issues resolved before the other review teams as asked to spend time on it.

Major issues:

   A Path Segment is a single label that is assigned from the Segment
SB> I think that should be a Path Segment Identifier, or a Path Segment 
SB> Label
or a P-SID since a path segment is a subset of the path so the terms path segment without qualification is confusing and aliases with other use of the term, particularly in the transport network space.

[Cheng]Ack, modified to Path Segment Identifier.

=========

   When a Path Segment is used, the Path Segment MUST be inserted at the
   ingress node and MUST immediately follow the last label of the SR
   path, in other words, inserted after the routing segment
   (adjacency/node/prefix segment) pointing to the egress node.
SB> What happens if the payload is not IP - is the PSL before the PW (or 
SB> other
payload) label?
[Cheng]In my understanding, the payload type is irrelevant with a Path Segment.
So if any label is required to indicate the type of payload/processing of payload, then it should be after PSL/PSID.

If a Path Segment Label is assigned to a LSP, then the Path Segment Label is inserted before the PW label. 
If a Path Segment Label is assigned to a PW, then the Path Segment Label is inserted after the PW label. However, do we have this use cases? It may be good to future discussion?

=======

When Path Segment is allocated
   from the SRGB pool, an intermediate node MAY see the Path Segment
   label at the top of the label stack.
SB> Just checking - is it carried as it native value?
SB> If an intermediate node is looking at the PSL how does it know the 
SB> context
of the label to know that it is a PSL and not some other label? SB> If the label is the base label how do you deal with the problem that some hardware is unable to process parts of the 20bit label space? SB> If we are going to provide this as an option the text needs to be specific on the elements of procedure
[Cheng] Same as Adrian's comment, thank you Stewart. The proposed text is below, 

Normally, an intermediate node will not process the Path Segment label in the label stack. But in some use cases, an intermediate node MAY process the Path Segment label in the label stack. In these cases, the Path Segment label MUST be learned before processing. The detailed use cases and processing is out of the scope of this document.

Hope it can address your comment. 

======

   A Path Segment can be used in the case of Penultimate Hop Popping
   (PHP), where some labels are be popped off at the penultimate hop of
   an SR path, but the Path Segment MUST NOT be popped off until it
   reaches at the egress node.

SB> What happens at the penultimate node - is it exposed there as a 
SB> result of
PHP, and is it passed on as its native value or is the base added to it before it is passed across?
[Cheng] Actually, Path Segment Label is like a VPN label, so no new requirement here. 
In this case, it should be the native value. Because the penultimate node should not read the path segment, so the label value should not be modified. 
The path segment will be used at the egress node. We can delete this paragraph if you don't like, nothing new.

=========
   Generic Associated Label (GAL) is used for Operations, Administration
SB: I think that should be "(GAL) MAY be used". A PW running in an MPLS network can use the the CW first nibble 1, ACH mechanism.
[Cheng] no problem. Thanks

========

SB> Section (3) is out of place WRT the rest of the text. Everything 
SB> else is
data plane specific and control plane agnostic. I would suggest that the control plane is out of scope for the RFC and you should simply say that a method needs to be provided for the PSL imposition node to know what label to use and that this label MUST be unique on the node stripping and processing the PSL. I think information that belongs later in the text.

[Cheng]Agree, the updated text is shown below
"
There are some ways to assign and distribute the Path Segment. The Path Segment can be configured locally or allocated by a centralized controller or by other means, this is out of the scope of this document.  If an egress cannot support the use of the Path Segment Label, it MUST reject the attempt to configure the label.
"

=========

SB> The following text is very hard to read

  BSID and Path SID (PSID) can be combined to achieve both sub-path and
   end-to-end path monitoring.  A reference model for such a combination
   in (Figure 2) shows an end-to-end path (A->D) that spans three
   domains (Access, Aggregation and Core domain) and consists of three
   sub-paths, one in each sub-domain (sub-path (A->B), sub-path (B->C)
   and sub-path (C->D)).  Each sub-path is allocated a BSID.  For
   nesting the sub-paths, each sub-path is allocated a PSID.  Then, the
   SID list of the end-to-end path can be expressed as <BSID1, BSID2,
   ..., BSIDn, e-PSID>, where the e-PSID is the PSID of the end-to-end
   path.  The SID list of a sub-path can be expressed as <SID1, SID2,
   ...SIDn, s-PSID>, where the s-PSID is the PSID of the sub-path.

SB> Doesn't this require that the (single) PSL is unique at all BSID end
points. I think that needs some discussion, such as whether you swap the PSL when you enter the new segment and whether the PSL is its native value of the locally resolved value and if the target locally resolved value how you know what to swap it to.

[Cheng] Please advise, I make some modifications below

  BSID and PSID can be combined to achieve both sub-path and
   end-to-end path monitoring.  A reference model for such a combination
   in (Figure 2) shows an end-to-end path (A->D) that spans three
   domains (Access, Aggregation and Core domain) and consists of three
   sub-paths, one in each sub-domain (sub-path (A->B), sub-path (B->C)
   and sub-path (C->D)).  Each sub-path is associated with a BSID and a s-PSID. 

  The  SID list of the end-to-end path can be expressed as <BSID1, BSID2,
   ..., BSIDn, e-PSID>, where the e-PSID is the PSID of the end-to-end
   path.  The SID list of a sub-path can be expressed as <SID1, SID2,
   ...SIDn, s-PSID>, where the s-PSID is the PSID of the sub-path.

From the aspect of PSL, there is no difference comparing to normal mode without using BSID.  So PSL is also a local label at each egress node of each sub-path or a global label which should be unique among the network. But again, it is the same with normal use case without BSID.
The Path Segment will not be used in forwarding as well, so I do not think the value of Path Segment should be swapped. It is used to identify a path locally at the egress node.

=======

   Figure 2 shows the details of the label stacks when PSID and BSID are
   used to support both sub-path and end-to-end path monitoring in a
   multi-domain scenario.

SB> I think that there needs to be some consolidation of the terminology
between path segment and PSID since I think you are the term interchangeably.
SB> I am also concerned about SR developing a parallel universe with SR
specific approaches that apply to the general MPLS case, but using terms that are not easily back ported into MPLS.
[Cheng]Ack. Please see modifications.

===========

   As defined in [RFC7799], performance measurement can be classified
   into Passive, Active, and Hybrid measurement.  Since Path Segment is
   encoded in the SR-MPLS Label Stack as shown in Figure 1, existing
   implementation on the egress node can be leveraged for measuring
   packet counts using the incoming SID (the Path Segment).  A different
   scheme such as carrying such identifier after the bottom of the label
   stack may require new implementation.
SB> That statement lacks detail and is speculative.
[Cheng]Sorry, about which part? 
This draft only focus on path segment itself. Regarding how to use path segment in measurement will be described in other draft I guess. 
I would suggest to delete the last sentence, because I cannot understand it as well ☹


============

   For Passive performance measurement, path identification at the
   measuring points is the pre-requisite.  Path Segment can be used by
   the measuring points (e.g., the ingress and egress nodes of the SR
   path or a centralized controller) to correlate the packet counts and
   timestamps from the ingress and egress nodes for a specific SR path,
   then packet loss and delay can be calculated for the end-to-end path,
   respectively.
SB> Did you discuss the effect of ECMP if not all packets contain a PSID 
SB> or you
turn the instrumentation on and off?
[Cheng]ECMP? Is it about entropy label? I think this is irrelevant with PSID? PSID is only used for identifying a path. ECMP can be used or not, it is independent.

==========

   Path Segment can also be used for Active performance measurement for
   an SR path in SR-MPLS networks for collecting packet counters and
   timestamps from the egress node using probe messages
   [I-D.ietf-mpls-rfc6374-sr] and [I-D.ietf-spring-stamp-srpm].

SB> Again out old friend ECMP needs a discussion
[Cheng] Same as above. But we deleted the drafts because they will introduce dependencies.

============

   Path Segment can also be used for In-band PM for SR-MPLS to identify
   the SR Path associated with the collected performance metrics
   [I-D.ietf-mpls-inband-pm-encapsulation].
SB> In a standards track document you cannot make a claim without 
SB> providing
supporting text showing how this is done and what the issues might be.
[Cheng] The logic is the same as Passive PM, the only thing this part would like to state is that the Path segment can be used for identifying a path, and this is required in PM.
How about let’s modify them as below.

---OLD---
Path Segment can also be used for Active performance measurement for
an SR path in SR-MPLS networks for collecting packet counters and
timestamps from the egress node using probe messages.

Path Segment can also be used for In-situ OAM for SR-MPLS to identify
the SR Path associated with the in-situ data fields in the data packets
on the egress node.

Path Segment can also be used for In-band PM for SR-MPLS to identify
the SR Path associated with the collected performance metrics.
---OLD---

---NEW---
Path Segment can also be used for identifying path in active performance measurement, hybrid performance measurement, etc.
The details of using Path Segment in these measurements are out of the scope of this document, and will be described in other drafts.
---NEW---

==========

   In the current SR architecture, an SR path is a unidirectional path
   [RFC8402].  In order to support bidirectional SR paths, a
   straightforward way is to bind two unidirectional SR paths to a
   single bidirectional SR path.  Path Segments can then be used to
   identify and correlate the traffic for the two unidirectional SR
   paths at both ends of the bidirectional path.
SB> SR uses loose source routing and ECMP to minimise segments. Even 
SB> between a
pair of adjacent nodes you cannot be sure of path reciprocity unless you you AdjSIDS. Is that the proposal?
[Cheng]Yes. From the aspect of path segment, we do not care about it is really a true bidirectional path using Adj-SIDs or not. This is ensure by the operators. 
Even if a loose TE path is used in bidirectional path, this is not an error of Path Segment. It may be an error or a choice of operator. This is out of the scope of this document.

=========

   An attacker could exploit path segment to manipulate the accounting
   of SR traffic at the egress.  Path segment could also be used to
   monitor traffic patterns for the E2E paths.  The control protocols
   used to allocate path segments could also be exploited to disseminate
   incorrect path segment information.  Note that, the path segment is
   imposed at the ingress and removed at the egress boundary and is not
   leaked out of the administered domain.
SB> What are the mitigations for the increase in attack surface?
[Cheng] It is the same with other labels/SIDs. Keeping it within the domain can help. Nothing new here I think.  I will propose the delete this paragraph.


============

Minor issues:

 For SR-MPLS, this can be the Path
   Segment label allocated by the egress node.
SB> Can be is not strict enough for standards track
[Cheng] This only says that Path Segment is one of the options. There may be other options. So we are using 'can'

=========

 It
   is obvious that this equivalence group can be instantiated in the
   network by an SDN controller using the Path Segments of the SR paths.
SB> Nothing is obvious until it is proven.
[Cheng]Ack

---NEW---
 This equivalence group can be instantiated in the  network by an SDN controller using the Path Segments of the SR paths.
---NEW---


========

   Path Segment in SR-MPLS does not introduce any new behavior or any
   change in the way the MPLS data plane works.
SB> If it does not change anything we do not need an ST RFC.
[Cheng]Ack. I make some modifications as below. Please advise.

---NEW---
Path Segment in SR-MPLS is used within the SR domain, and no new security threats are introduced comparing to current SR-MPLS. The security consideration of SR-MPLS is described in {{Section 8.1 of RFC8402}} applies to this document.
---NEW---


========
 Path
   segment is additional metadata that is added to the packet consisting
   of the SR path.
SB> I think you mean that the change is to add a new label.
[Cheng]Deleted.

========

Nits/editorial comments:

6.  Path Segment for Bidirectional SR Path

   In some scenarios, for example, mobile backhaul transport networks,
   there are requirements to support bidirectional paths, and the path
   is normally treated as a single entity.  The both directions of the
SB> "The both directions" is not good grammar
[Cheng]Ack.  

s/The both directions/Forward and reverse directions

Many thanks for your thorough review, it is really helpful!
Respect!