Label Switched Path (LSP) Ping/Traceroute for Segment Routing (SR) Egress Peer Engineering Segment Identifiers (SIDs) with MPLS Data Plane
draft-ietf-mpls-sr-epe-oam-19

Section 7:  I think this could stand to be separated into a few paragraphs.  As it stands, it is hard to read and harder to understand.  

Maybe something like this:
The EPE-SIDs are advertised for egress links for Egress Peer Engineering purposes or for inter-AS links between co-operating ASes. When co-operating domains are involved, they can allow the packets arriving on trusted interfaces to reach the control plane and get processed. 

When EPE-SIDs are created for egress TE links where the neighbor AS is an independent entity, it may not allow packets arriving from external world to reach the control plane. In such deployments MPLS OAM packets will be dropped by the neighboring AS that receives the MPLS OAM packet. 

In MPLS traceroute applications, when the AS boundary is crossed with the EPE-SIDs, the FEC stack is changed. [RFC8287] does not mandate that the initiator upon receiving an MPLS Echo Reply message that includes the FEC Stack Change TLV with one or more of the original segments being popped remove a corresponding FEC(s) from the Target FEC Stack TLV in the next (TTL+1) traceroute request. 

If an initiator does not remove the FECs belonging to the previous AS that has traversed, it MAY expose the internal AS information to the following AS being traversed in traceroute.

# Éric Vyncke, INT AD, comments for draft-ietf-mpls-sr-epe-oam-18

Thank you for the work put into this document.

Please find below some non-blocking COMMENT points (but replies would be appreciated even if only for my own education), and some nits.

Special thanks to Tarek Saad for the shepherd's (very) concise write-up including the WG consensus _but it lacks_ the justification of the intended status.

Please note that Wassim Haddad is the Internet directorate reviewer (at my request) and you may want to consider this int-dir review as well when it will be available (no need to wait for it though):
https://datatracker.ietf.org/doc/draft-ietf-mpls-sr-epe-oam/reviewrequest/19810/

I hope that this review helps to improve the document,

Regards,

-éric

# COMMENTS (non-blocking)

## Section 1

It is unclear (to me at least) whether "administration", "operator", and "AS" are equivalent else how they are different. Some clarifications will be welcome.

## Section 4

Unsure whether the lead text helps as it also appears in the IANA considerations.

## Section 4.1

Please specify the fields Type & Length (notably in size) to be complete.

`Length : 16 octets` is somehow confusing as the field length is 2 octets. Let's be detailed here.

## Section 4.2

Are anycast and multicast addresses in scope in this I-D ? Let's be specific and use "unicast" then.

## Section 6

Suggest adding the URL for the registry https://www.iana.org/assignments/mpls-lsp-ping-parameters/mpls-lsp-ping-parameters.xhtml#sub-tlv-1-16-21

# NITS (non-blocking / cosmetic)

Suggestion: use a spell checker for `administriations`, `bandiwdth`, `descrepancies`, `agnored`, ...

s/4 octet unsigned integer/4-octet unsigned integer/

s/Link Local IPv6/Link-local IPv6/

# Internet AD comments for draft-ietf-mpls-sr-epe-oam-17
CC @ekline

* comment syntax:
  - https://github.com/mnot/ietf-comments/blob/main/format.md

* "Handling Ballot Positions":
  - https://ietf.org/about/groups/iesg/statements/handling-ballot-positions/

## Comments

### S4.2

* Do you really need this Adj-Type field?  The Length field suffices to
  differentiate v4 from v6 adjacencies (though not v4 with a v6 next hop);
  if that's all that's really needed...

# Gunter Van de Velde, RTG AD, comments for draft-ietf-mpls-sr-epe-oam-18

Many thanks to Matthew Bocci, Shuping Peng and  Joel Halpern for their Routing Directorate reviews and Tarek Saad for the shepherd writeup.


Please find https://www.ietf.org/blog/handling-iesg-ballot-positions/ documenting the handling of ballots.

##Supporting the Discuss from John Scudder about confederations ASes


#GENERIC COMMENTS
#================
##The draft has been discussed well within the MPLS WG and is easy to read.


#DETAILED COMMENTS
#=================
##classified as [minor] and [major]

244	   Local BGP Router ID :
245
246	   4 octet unsigned integer representing the BGP Identifier of the
247	   advertising node as defined in [RFC4271] and [RFC6286].

[minor]
Maybe add reference to rfc6793 (BGP Support for Four-Octet Autonomous System (AS) Number Space)

295	   Length : variable based on IPv4/IPv6 interface address.  Length
296	   excludes the length of Type and Length fields.For IPv4 interface
297	   addresses length will be 28 octets.  In case of IPv6 address length
298	   will be 52 octets.

[major]
What if the length is different from 28 or 52? how will a node receiving such information process or contain the error situation?

[minor]
Would normative language here be considered useful for implementers?

300	   Adj-Type : Set to 1 when the Adjacency Segment is IPv4 Set to 2 when
301	   the Adjacency Segment is IPv6

[major]
What if for example the length (see comment above) is set to 52 (=IPv6) and the adj-Type is set to IPv4?
What needs to be done? is there a validation that is taking place? does the combination of length and Adj-Type follow certain approved normative combinations?

303	   RESERVED : 3 octets.  MUST be zero when sending, and agnored on
304	   receiving.

[minor]
s/agnored/ignored/
Are the bits RESERVED or UNUSED? (according IANA registries)

383	   Length : variable based on the number of elements in the set.  The
384	   length field does not include the length of Type and Length fields.

[minor]
I suspect that the length is expressed in octets similar as the TBD1 and TBD2. 
This should be clarified in the text.

410	   Number of remote ASes, the set SID load-balances on.

[minor]
Clarification rewrite about the load balancing aspect:
s/Number of remote ASes, the set SID load-balances on/The number of remote ASes over which the set SID performs load balancing/

416	5.  EPE-SID FEC validation

[minor]
This section discusses validation and uses numbers. While it should be already trivial for an implementer to understand that octets are intended, it may be worthwhile to spell it out explicitly so that there is no confusion and by accident bits, nibbles, words or something else obscure would be used instead

I cleared my DISCUSS on the document, but reviewers, make sure you are reviewing -18 version of the draft, even though -17 was submitted for IESG review.

            If an initiator does not remove the FECs belonging to the previous AS that has traversed,
            it MAY expose the internal AS information to the following AS being traversed in traceroute.

I find the use of MAY here strange. I think what is meant that If the FECs are not removed, it will expose ?
Or if it is really "may expose" because it would depend on the content of the FECs, then it would be a lowercase
may, as an uppercase MAY makes it sound like an instruction or action

Thank you to Gyan Mishra for the GENART review.

Thanks for your work!

I'd like to support John's DISCUSS.

Thanks to Tianran Zhou for the OpsDir review (https://datatracker.ietf.org/doc/review-ietf-mpls-sr-epe-oam-15-opsdir-lc-zhou-2024-05-24/) and the authors for addressing the comments. Also thank to Tianran for updating the OpsDir review once the new version was posted.

Thanks for working on this specification. My review didn't surface transport protocol related concerns.

However, it was not easy for me to understand the exact meaning of "receiving node", so I interpreted as a node receiving "SID", that can be completely misinterpretation. Can we define it in this specification or point to where this is defined  to avoid confusion?