Reviewer: Ladislav Lhotka
Review result: On the Right Track
*** General comments
This document contains two YANG modules:
ietf-segment-routing-common and ietf-segment-routing. The latter
augments ietf-routing with data necessary for configuring and
operating segment routing, but also provides groupings that define
data to be used in segment routing extensions of routing protocols
and interfaces. This design makes it relatively easy to define
such IGP extensions.
**** Naming
- My suggestion is to review the identifiers defined in the
modules so as to adhere more closely to the identifier naming
conventions in sec. 4.3.1 of RFC 8407. It is subjective to
decide which acronyms are well-known but, in my view, "srgb",
"srlb" and "msd" do not fall into this category. For example,
"msd" can be changed to to "max-sid-depth" (both as a data node
and a feature).
- On the other hand, the "-cfg" suffix in the names of several
groupings should be removed - according to RFC 8407,
identifiers should not carry semantics. (And perhaps the
same groupings can also be used for state data?)
**** Revisions
The YANG modules contain revision dates of all development
versions. Although RFC 8407 doesn't require to do so, I think it
can be useful for the developmnent and testing of the
modules. However, these development revisions should be removed
before publishing the RFC (maybe the authors intend to do so).
**** References
Normative References should include the RFCs defining YANG
modules that are imported by the segment-routing modules: 6991,
7223, 8294 and 8349.
*** Specific comments
- Containers "connected-prefix-sid-map" and "local-prefix-sid"
have subcontainers "ipv4" and "ipv6" that only differ in the
type of the "prefix" leaf. An alternative solution can be to
change the outer containers into lists with "address-family" as
the key. Did the authors discuss this option?
- The text uses the terms "states" and "operational states" in
several places. The plural form looks weird to me.
