Reviewer: Paul Wouters Review result: Has Issues I have reviewed this document as part of the security directorate's ongoing effort to review all IETF documents being processed by the IESG. These comments were written primarily for the benefit of the security area directors. Document editors and WG chairs should treat these comments just like any other last call comments. Note: I'm not a segment routing or BGP expert, so I might have misunderstood some things. As far as I can understand the Security Considerations, it seems that it is adequately pointing to other documents and mentions, with solutions, new concerns raised by implememting this document. I do have a number of comments and suggested improvements for the tables and figures layout, and some questions about IANA registries. See below for details. Paul Section 2.1 Can the IANA registry be referenced here so the reader can easilly go to see the entire list of Node Attribute TLVs ? Section 2.1.1 I personally find Figures to have more readable with less -+-+-+-+ symbols eg to change Figure 2 to: 0 1 2 3 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 +-------------------------------+-------------------------------+ | Type | Length | +-------------------------------+-------------------------------+ ~ SID/Label (variable) ~ +---------------------------------------------------------------+ Note that since the SID/Label is a variable length, I used the "~" symbol as in common to donate that. And important in this case too, as I was thinking it was a fixed size but reading the description found out it was either 3 or 4 bytes. I see the document uses // for this elsewhere, which is also okay. Type: 1161 I would write: Type: set to 1161 denoting a SID/Label Node Attribute Length: Either 3 or 4 depending whether [...] I find this language confusing. It suggests the Length field can be of size 3 or 4, instead of saying the Length field value can be 3 or 4. then the 20 rightmost bits represent a label Should it be specified what to do with the 4 leftmost bits? Are they reserved? used for something else ? Section 2.1.2 I find the split Range Size vs / SID/Label confusing. The table is supposed to give me an idea of the byte stream, but by being split it two it doesn't do that well. How about this: 0 1 2 3 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 +-------------------------------+-------------------------------+ | Type | Length | +---------------+---------------+-------------------------------+ | Flags | Reserved | Range Size, or | +---------------+---------------+ | ~ SID/Label sub-TLV ~ +---------------------------------------------------------------+ I find this bit a little misleading in our way of writing it: +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | Range Size | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ // SID/Label sub-TLV (variable) // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ Because I don't think Range Size is really 20 bits and the next 4 bits can be used for another payload. I assume those unspecified 4 bits are reserved and set to 0? Type: 1034 I would write: Type: set to 1034 denoting an SR Capabilities Node Attribute Length: Variable. Minimum length is 12. Again, it find this confusing. The length field itself is not variable length and has no minimum length of 12. I would write: Length: two octects in network order, specifying the length of the Range Size or SID/Label sub-TLV Reserved: 1 octet that SHOULD be set to 0 and MUST be ignored on receipt. Why is that SHOULD not a MUST? One or more entries, each of which have the following format: Is this really "one or more entries"? The table above does not show that at all. Can we only have more entries of the same capabilities or can they be mixed (eg one range size and two SID/Labels ??) If there is more than one entry, how would one know whether these are RAnge Sizes or SID/Labels? If there is only one, then the Length denotes that implicitely. Since the SID/Label sub-TLV is used to indicate the first label of the SRGB range, only label encoding is valid under the SR Capabilities TLV. Does this mean the above "one or more entries" is actually restricted and means "one or more rang sizes, followed by 0 or 1 SID/Labels" ? The ambiguity perceived means I can not fully deducate the field contents as it is currently specified. Section 2.1.3 Similar remarks to the sections above here regarding the field descriptions. I would enclose Algorithm 1, since 1 is the minimum and close the table, since our content description does end (based on the Length field) 0 1 2 3 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 +-------------------------------+-------------------------------+ | Type | Length | +---------------+---------------+---------------+---------------+ | Algorithm 1 | Algorithm... ~ Algorithm N ~ | +---------------+---- | ~ ~ +---------------------------------------------------------------+ Section 2.1.4 See my similar comments above. Additionally, one Range Size and SID label should be added to the diagram unconditionally, as the minimum is one, and then the block follows of optional additional blocks of range size plus SID/Label. Section 2.1.5 See similar comments above. Section 2.2 These TLVs should only be added to [...] Should that "should" be a SHOULD (or MUST?) Section 2.2.1 See similar comments above that apply to this table and values. Here it is even more important because Length appears even more variable because Flags is described as a static "one octet". And then "weight" isn't given an octet size but the following Reserved field is. Should the SHOULD in the reserved field description be a MUST ? Especially for reserved fields, I see no reason why it is not a MUST. If this is left unchecked by an implementation and possibly filled with bogus data, while that will not break anything NOW, as the document also states "MUST ignore", but once any of these fields get defined in the future, it would break. So it is better to dictate now to not leave it with potential garbage values. [The Following sections all have the same characteristics as the above ones, so I won't mention these further in the review, but I think these should be fixed similarly.] Section 2.2.3 This table seems to extend an existing table in what I hope is some kind of IANA registry? Can that be referenced here? If there is no IANA registry of these, and this seems to extend a list of entries from RFC 7752 and RFC 8571, I would suggest this document creates that new IANA registry and populates it with all those values. Section 2.3 Similar here, should this be a new IANA Registry? Section 2.3.4 If the Prefix NLRI is not considered a "normal routing prefix", what is it considered as? What implications does that have? need to be interpreted accordingly Is that "need to" not a MUST ? or perhaps rephrase this as: The Flags field of this TLV are interpreted accordingly to the respective underlying IS-IS, OSPFv2 or OSPFv3 protocol. Section 3 We normally don't say early code points were assigned. We just say what we request(ed) from IANA. Also, can the registies named be linked to IANA? "should be left empty" -> "is left empty" The table in Section 3.1 seems to extend an existing table/registry. Can it be named and linked so the reader can jump the the IANA registry ? NITS: Abstract: This draft -> This document Introduction: This sentence seems malformed: A segment can represent any instruction, topological or service-based. or global within a domain. -> or a global semantic within a domain. Within IGP topologies an SR path -> Within IGP topologies, an SR path Use of "segement" vs "Segment" in the Introduction is inconsistent Figure 1 describes -> Figure 1 denotes then can -> can Section 2.1.1 as a label or an index/SID. -> as a label or as an index/SID Section 2.2.1 The Protocol-ID of the BGP-LS Link NLRI should be used to determine the underlying protocol specification for parsing these fields. I would change the "should be" to an "is", as there is no chocie here. That is how it is. Similar in other sections, such as 2.3.1 Section 2.3.4 is considered as a normal routing -> is considered a normal routing