Reviewer: Paul Wouters
Review result: Has Issues
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
