Hi Mahesh, Thanks for the review, very much appreciated.
I submitted -10 version to address your comments: https://datatracker.ietf.org/doc/draft-ietf-rtgwg-policy-model/ Note: There are YANG validation errors because yanglint can't find model "ietf-if-extensions", not sure why. yanglint SO 1.6.7: yanglint --verbose -p {rfclib} -p {draftlib} -p {tmplib} {model} -i: err : Data model "ietf-if-extensions" not found. err : Importing "ietf-if-extensions" module into "ietf-routing-policy" failed. err : Module "ietf-routing-policy" parsing failed. Please see detailed feedbacks below inline starting with [YQ]. Thanks, Yingzhen On 5/19/20, 10:21 PM, "Mahesh Jethanandani via Datatracker" <[email protected]> wrote: Reviewer: Mahesh Jethanandani Review result: On the Right Track This review is looking at the draft from a YANG perspective. With that said, I have marked it as on the right track, because of some of the points discussed below. Summary: This document defines a YANG data model for configuring and managing routing policies in a vendor-neutral way. The description in the document and in the model is well written and easy to understand. Nits 1. Please run the spell checker on the document. [YQ]: fixed 2. Indentations in the YANG model are not consistent. There is at least one place in the definition of 'grouping neighbor-set' that needs fixing. [YQ]: fixed 3. Consistency in how the description statements are written. Most of the descriptions start on a new line, except for a few places where they do not. [YQ]: fixed 4. The match-set-options-type defines enums as 'all', 'any' and 'invert'. Then why use ALL, ANY (all caps) when describing them? Could you use single quotes (as above) around them when referring to the definition? [YQ]: fixed. 5. Remove unnecessary empty lines in the model. [YQ]: Done. Comments: Section 1 - Introduction: The document does not mention whether the model is YANG 1.0 or a 1.1 model. It includes both RFC 6020 and RFC 7950 even though the actual YANG model clearly states that the yang-version is 1.1. Please remove reference to RFC 6020. [YQ]: removed reference to RFC 6020 The document includes a section on Terminology and Notation. However it does not define what is a "routing policy" or provide a reference to it. [YQ]: I'm not sure there is a standard definition of "routing policy". I'll do some research on this and see whether we should add it to the terminology. Section 8 - Security Considerations: The security considerations section needs to follow the guidelines outlined in Section 3.7 of RFC 8407. [YQ]: this section has been rewritten. Section 9 - IANA Considerations The IANA considerations section needs a complete rework. Please see Section 3.8 of RFC 8407 or see any of the many drafts/RFCs that have samples of what needs to go into that section. [YQ]: this section has been rewritten. Section 10.1 - Routing policy model The model uses a lot of groupings, most of them used only once in the model. That makes the model difficult to understand. Unless these groupings are meant for use by other models, they should be folded into the main container. Take this grouping for example: grouping policy-definitions { description "This grouping provides policy definitions"; leaf name { type string; description "Name of the top-level policy definition -- this name is used in references to the current policy"; } } For one the description of the grouping is not correct. Secondly, it is used only once in the model and that too to define a key of a list. [YQ]: This model was initiated by OpenConfig, we've removed some groupings Since this become an IETF draft. I removed "policy-definitions" and "policy-statements" groups. The YANG model imports multiple models from other drafts. It provides normative references to the imported models in Section 2.2. (A nit here is that it missing the cross-reference in the table for if-l3-vlan), but does not provide references to them in the model. See Section 3.9 of RFC 8407. Please add something like this in the model. import ietf-inet-types { prefix "inet"; reference "RFC 6991: Common YANG Data Types."; } [YQ]: fixed. Please drop <mailto:....> and just keep the e-mail address. That tag works only when embedded within a HTML document. [YQ]: do you mean the "editor" in the model? The description section splits the description of the model by inserting the Copyright statement in the middle. Can that statement be moved so that the actual description of the model is together. [YQ]: fixed. The model defines a lot of identities some of which are not used in the model or used at all. Would it not be better to have those identities defined in the individual routing models that augment this model? The BGP YANG model does not use any of the identities defined in this model. [YQ]: OSPF and ISIS will use these identities as "metric-type". Can the following comment be made part of the description statement of the typedef? /* This typedef retained for name compatibiity with default import and export policy. */ [YQ]: fixed. s/compatibiity/compatibility/ [YQ]: fixed. I believe masklength is two words. s/masklength-lower/mask-length-lower/ and the same for masklength-upper. Also update the description. [YQ]: fixed A pyang compilation of the model with —ietf and —lint option was clean. However, there seems to be an issue with the example in Section 11. Instead of actually including an example, it is showing the path to what should have been the example. I was therefore not able to validate the example against the model. [YQ]: I can't find this example in OpenConfig Github anymore, so removed this section. A idnits run of the draft reveals a few issues. Please address them. Checking nits according to https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.ietf.org%2Fid-info%2Fchecklist&data=02%7C01%7Cyingzhen.qu%40futurewei.com%7C5fbfd5e232374ca2774d08d7fc7db255%7C0fee8ff2a3b240189c753a1d5591fedc%7C1%7C0%7C637255489110446326&sdata=as3LAkcLhaEt51NkRKsiuYSxa5HxXV0zOQ0FAlL6uuc%3D&reserved=0 : ---------------------------------------------------------------------------- == There are 4 instances of lines with private range IPv4 addresses in the document. If these are generic example addresses, they should be changed to use any of the ranges defined in RFC 6890 (or successor): 192.0.2.x, 198.51.100.x or 203.0.113.x. [YQ]: fixed == The document doesn't use any RFC 2119 keywords, yet seems to have RFC 2119 boilerplate text. Checking references for intended status: Proposed Standard ---------------------------------------------------------------------------- (See RFCs 3967 and 4897 for information about using normative references to lower-maturity documents in RFCs) == Unused Reference: 'I-D.ietf-netmod-sub-intf-vlan-model' is defined on line 1620, but no explicit reference was found in the text -- No information found for draft-ietf-netmod-intf-ext-yang - is the name correct? -- Possible downref: Normative reference to a draft: ref. 'I-D.ietf-netmod-intf-ext-yang' -- No information found for draft-ietf-netmod-sub-intf-vlan-model - is the name correct? -- Possible downref: Normative reference to a draft: ref. 'I-D.ietf-netmod-sub-intf-vlan-model' [YQ]: there is dependency on both 'I-D.ietf-netmod-intf-ext-yang' and 'I-D.ietf-netmod-sub-intf-vlan-model'. Summary: 0 errors (**), 0 flaws (~~), 9 warnings (==), 5 comments (--). _______________________________________________ rtgwg mailing list [email protected] https://www.ietf.org/mailman/listinfo/rtgwg
