Ballot for draft-ietf-mpls-sr-epe-oam
Discuss
Yes
No Objection
No Record
Summary: Has a DISCUSS. Has enough positions to pass once DISCUSS positions are resolved.
# John Scudder, RTG AD, comments for draft-ietf-mpls-sr-epe-oam-18 CC @jgscudder Thanks for the document. I have a few DISCUSS-level concerns (but I think they will be easy to fix), and a couple of other comments. ## DISCUSS ### Sections 4.1, 4.2, and 4.3, Confederations These three sections have similar or identical text defining the local and remote AS numbers, as follows, Local AS Number : 4 octet unsigned integer representing the Member-AS Number inside the Confederation [RFC5065]. The AS number corresponds to the AS to which PeerNode SID advertising node belongs to. Remote AS Number : 4 octet unsigned integer representing the Member-AS Number inside the Confederation [RFC5065]. The AS number corresponds to the AS of the remote node for which the PeerNode SID is advertised. Although I am naturally a huge fan of confederations, I don't understand why you are referencing them here, in this way. Surely this document is not restricted to use only with confederations (if it were it would need a lot more explanation!). My guess is you're trying to solve the problem where the local and remote nodes are both within the same Confederation, but different Member-ASes. If so, perhaps something like the following, NEW: Local AS Number : The 4-octet AS number of the AS to which the PeerNode SID advertising node belongs. If Confederations [RFC5065] are in use, and if the remote node is a member of a different Member-AS within the local Confederation, this is the Member-AS Number inside the Confederation and not the Confederation Identifier. Remote AS Number : The 4-octet AS number of the AS of the remote node for which the PeerNode SID is advertised. If Confederations [RFC5065] are in use, and if the remote node is a member of a different Member-AS within the local Confederation, this is the Member-AS Number inside the Confederation and not the Confederation Identifier. I don't know how picky you want to get, but the case not covered in my proposed text is if confederations are in use, but the local node and remote node are in different Confederations. In that case, their respective Confederation Identifiers would be used, but it simplifies to the "AS number" case since a Confederation Identifier is just a (globally visible) AS number. As far as I'm concerned that's obvious enough not to need saying, but YMMV. ### Sections 4.1, 4.2, and 4.3, "advertising node" These three sections all refer to the "advertising node". I presume this means, "the node that advertised the information into BGP-LS". But is that really what you mean? Keep in mind that by design, sometimes no other than the local one can advertise information into BGP-LS as a proxy for the other node. I was thinking of proposing different terminology to remedy this problem, but I see RFC 9086 has the same defect (of assuming that the node that owns the SID will be the one that advertises it into BGP-LS), so we may as well stick with it. However, I think it would be helpful to be clear about exactly what "advertising note" means, every time you say it. You've already got a pattern for this in your document, e.g. in Section 4.2 you have "PeerAdj SID advertising node" which is good enough -- I suggest just replicating that pattern and never abbreviating down to "advertising node". ### Sections 4.1, 4.2, and 4.3, "receiving node" These three sections all refer to the "receiving node". The most obvious and usual reading of "receiving node" is, the node that receives the BGP-LS message carrying the sub-TLV in question. I think that can't be what you mean. I think what you mean is "remote node".
## COMMENT ### Section 4.3, order of description text Please sort your field descriptions in the same order the fields appear in Figure 5. The out-of-order description threw me off badly on my first read-through. ### Sections 5 and 5.1, "augments" Section 5 says, The below section augments the section 7.4 point 4a of [RFC8287]. And then Section 5.1 provides the augmentation. Looking at RFC 8287 I feel sad because the referenced section there already "modifies the procedure defined in Section 4.4.1 of [RFC8029]." But at least 8287 is a straight replacement for the 8029 text -- or I think it is, the language of 8287 is not clear on this point. :-( Since RFC 8287 §7.4 (4a) is essentially a big disjunction, it makes sense that you can take your text and paste it onto the end of 4a. And I agree that for the same reason, it doesn't matter whether it's appended to the end, placed at the beginning, or even interleaved. However for clarity, I think it would save some future reader my confusion if you reworded the quoted text as, The below section is appended to the procedure given in Section 7.4 point 4a of [RFC8287]. It would also be nice if you followed the pseudo-code convention established in RFC 8287 since you're augmenting that body of pseudo-code. Yours is close, but for example, you don't use braces "{ }" where 8287 does, you don't use the same bullet conventions, etc. I imagine if there are things it's too much effort for you to fix in your source, you could ask the RFC Editor for help with those. ## Notes This review is in the ["IETF Comments" Markdown format][ICMF], You can use the [`ietf-comments` tool][ICT] to automatically convert this review into individual GitHub issues. [ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md [ICT]: https://github.com/mnot/ietf-comments
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.
# 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.
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?
# É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/