Reviewer: Ladislav Lhotka
Review result: Ready with Issues
This document and YANG module "ietf-sr-topology" contained therein
contribute two new types to the family of network topology data
models: segment routing and segment routing traffic engineering
topologies. The document also provides a companion module
"ietf-sr-topology-state" intended for non-NMDA implementations.
>From the YANG point of view, both modules are in a relatively good
shape. Also, as far as I can tell, they satisfy the requirements
specified in RFC 8345.
The example instance data contained in Appendix B successfully passes
formal validation against the date model, which is far from
commonplace for early versions of documents. See however comment #1
below.
**** Comments
1. I have argued several times that using URIs as identifiers of all
network topology objects in RFC 8345 is an overkill.
Xufeng]: Agree with you on this, but since the type has been published we need to figure out the way to fit into it. It is clear for what we need to do here in this case, so it is appreciated if you can provide a suggestion.
Now, the
present draft demonstrates the consequences: id values used in
many places of the example data (Appendix B) are not valid URIs!
For example, "link-id" leaves have values like
"D1,1-2-1,D2,2-1-1", which is not a URI.
This type violation isn't caught by validating tools because the
"ietf-inet-types:uri" type doesn't specify a regular expession
pattern. However, its description states clearly that the type
represents URI as defined by STD 66.
[Xufeng]: We know that this is not a typical URI, but many of the syntax components are not relevant here. In term of this particular example, how does it violate STD66? Specifically Sec 4.2 Relative Reference? Syntactically can this string be a “path segment” of a “relative-path” (or “path-noscheme”)?
2. The module uses (indirectly) the grouping "srlr" from the
"ietf-segment-routing-common" module. This grouping defines two
leaves, "lower-bound" and "upper-bound". I assume it is expected
that the former must not be greater than the latter. This is
however not enforced by the data model. My suggestion is to add a
"must" statement to the "srlr" grouping corresponding to this
constraint.
[Xufeng]: We have added a few “must” statements to the module ietf-sr-topology, and we will work with the authors of ietf-segment-routing-common to see if the constraint can be added to ietf-segment-routing-common.
3. Typos in the module text:
- description of grouping "sr-topology-type":
s/toplogies/topologies/
[Xufeng]: Fixed.
- description of container "sr-mpls":
s/Indiates/Indicates/
[Xufeng]: Fixed.
