Skip to main content

Last Call Review of draft-ietf-lsr-ospf-admin-tags-16

Request Review of draft-ietf-lsr-ospf-admin-tags
Requested revision No specific revision (document currently at 17)
Type Last Call Review
Team YANG Doctors (yangdoctors)
Deadline 2024-04-05
Requested 2024-03-04
Requested by Acee Lindem
Authors Acee Lindem , Peter Psenak , Yingzhen Qu
I-D last updated 2024-03-12
Completed reviews Yangdoctors Last Call review of -16 by Qiufang Ma (diff)
Rtgdir Last Call review of -17 by Russ White
Assignment Reviewer Qiufang Ma
State Completed
Request Last Call review on draft-ietf-lsr-ospf-admin-tags by YANG Doctors Assigned
Posted at
Reviewed revision 16 (document currently at 17)
Result Almost ready
Completed 2024-03-12
Hi, this is my YANG Doctor review of draft-ietf-lsr-ospf-admin-tags-16, I
marked the review as “Almost Ready” with a couple of comments and questions

(1) For the "ietf-ospf-admin-tags" YANG data model, what is the consideration
for the “advertise-prefixes” data node to be defined as a list with only one
child key inside it? Will this list node be populated with new parameters in
the future? Otherwise I think a simpler definition might be the following: OLD:
    augment /rt:routing/rt:control-plane-protocols
      +--rw admin-tags
         +--rw tags* [tag]
            +--rw tag                   uint32
            +--rw advertise-prefixes* [prefix]
               +--rw prefix    inet:ip-prefix

    augment /rt:routing/rt:control-plane-protocols
      +--rw admin-tags
         +--rw tags* [tag]
            +--rw tag                   uint32
            +--rw advertise-prefix* inet:ip-prefix

There is nothing wrong to be defined in the current draft (e.g., PYANG won’t
complain), but I think it can be hierarchically reduced this way, thoughts?

(2) For the grouping "prefix-admin-tag-sub-tlvs", it is defined and used to
augment the OSPF YANG data model and the OSPFv3 Extended LSA YANG data model.
There is a list defined inside the grouping definition, with only one child
declared as leaf-list. I am wondering why the type and length are not defined
inside admin-tag-sub-tlv list? Aren’t they part of the admin tag TLV?

(3) The reference info inside the “revision” statement for this draft is
inconsistent with its real title. It is using “RFC XXXX: YANG Data Model for
OSPF Prefix Administrative Tags.”, while it should be “RFC XXXX: Extensions to
OSPF for Advertising Prefix Administrative Tags”.

(4) There are some list/leaf-list data nodes defined in the YANG data model
with their names defined as plural (e.g., tags, advertise-prefixes,
admin-tags), but the naming convention is to have the list/leaf-list name
singular form. See RFC8407bis
for the following: “List identifiers SHOULD be singular with the surrounding
container name plural. Similarly, "leaf-list" identifiers SHOULD be singular.”

(5) There is a YANG tree diagram included in the draft, an informative
reference to RFC 8340 should be added in the draft. See RFC8407bis
for the following: “If YANG tree diagrams are used, then an informative
reference to the YANG tree diagrams specification MUST be included in the
document. Refer to Section 2.2 of [RFC8349] for an example of such a reference.”

(6) It would be good if the tree structure of the YANG module could be defined
in a separate section/subsection, so that readers are able to navigate to the
overview (i.e., tree structure) of the model quickly if they want. Just a
suggestion for the authors to consider.

(7) Please include a note to the RFC editor requesting RFC XXXX and xxxx(or
even better, use RFC YYYY for draft-ietf-lsr-ospfv3-extended-lsa-yang) is
replaced with the RFC number that is assigned to the document.

(8) I see a couple of “TBD” throughout the draft, did the authors leave them

(9) I think it would be useful if some example instance snippets of this YANG
data model can be added, either throughout the document or in an appendix. Is
this something the authors would consider to help understand the use of YANG