Hi Qiufang, We've updated the security consideration in version -19 to reflect the module changes. Thanks for catching that.
Thanks, Yingzhen On Thu, May 30, 2024 at 7:54 PM maqiufang (A) <[email protected]> wrote: > Thanks Yingzhen for taking care of my comments! > > Just a reminder that please also update nodes/subtrees listed in the > security consideration sec. to reflect your module update. > > > > Best Regards, > > Qiufang > > *From:* Yingzhen Qu [mailto:[email protected]] > *Sent:* Thursday, May 30, 2024 8:52 AM > *To:* maqiufang (A) <[email protected]> > *Cc:* [email protected]; [email protected]; > [email protected]; [email protected] > *Subject:* Re: Yangdoctors last call review of > draft-ietf-lsr-ospf-admin-tags-16 > > > > Hi Qiufang, > > > > Thanks for the review and sorry for the delayed response. I've uploaded > version -18 to address your comments, details below. > > > > Thanks, > > Yingzhen > > > > On Tue, Mar 12, 2024 at 5:29 AM Qiufang Ma via Datatracker < > [email protected]> wrote: > > Reviewer: Qiufang Ma > Review result: Almost Ready > > 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 > below. > > (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 > /rt:control-plane-protocol/ospf:ospf/ospf:areas/ospf:area > /ospf:interfaces/ospf:interface: > +--rw admin-tags > +--rw tags* [tag] > +--rw tag uint32 > +--rw advertise-prefixes* [prefix] > +--rw prefix inet:ip-prefix > > NEW: > augment /rt:routing/rt:control-plane-protocols > /rt:control-plane-protocol/ospf:ospf/ospf:areas/ospf:area > /ospf:interfaces/ospf:interface: > +--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? > > [Yingzhen]: we've modified the interface configuration. Please see whether > it addressed your comments. > > > > (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? > > [Yingzhen]: removed the list. As for the type and length, we're following > the style as in > > the base OSPF model (RFC 9129). When a tlv is known to an implementation, > the type > > and length is used to parse the tlv, so only the content is shown under > the tlv container. > > However for unknown TLVs, type and length are shown. > > > > (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”. > > [Yingzhen]: fixed. > > > > (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 > ( > https://www.ietf.org/archive/id/draft-ietf-netmod-rfc8407bis-09.html#section-4.3.1 > ) > for the following: “List identifiers SHOULD be singular with the > surrounding > container name plural. Similarly, "leaf-list" identifiers SHOULD be > singular.” > > [Yingzhen]: fixed. > > > > (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 > ( > https://www.ietf.org/archive/id/draft-ietf-netmod-rfc8407bis-09.html#section-3.4 > ) > 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.” > > [Yingzhen]: fixed. > > > > (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. > > [Yingzhen]: split into sub sections now. > > > > (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. > > [Yingzhen]: draft-ietf-lsr-ospfv3-extended-lsa-yang is in Auth48 state, > and will be published as RFC 9587. Once it's published, we'll update the > draft accordingly. > > > > (8) I see a couple of “TBD” throughout the draft, did the authors leave > them > intentional? > > [Yingzhen]: yes, those are TLV types to be assigned by IANA. > > > (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 > module? > > > > [Yingzhen]: The configuration is very straightforward. However it's very > difficult to do an example for the database as we don't have an > implementation, so we're not planning to add an example for now. > > >
_______________________________________________ Lsr mailing list -- [email protected] To unsubscribe send an email to [email protected]
