Re: Yangdoctors early review of draft-ietf-rtgwg-policy-model-09

[Yingzhen Qu <[e-mail]>]

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" <[e-mail]> 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&amp;data=02%7C01%7Cyingzhen.qu%40futurewei.com%7C5fbfd5e232374ca2774d08d7fc7db255%7C0fee8ff2a3b240189c753a1d5591fedc%7C1%7C0%7C637255489110446326&amp;sdata=as3LAkcLhaEt51NkRKsiuYSxa5HxXV0zOQ0FAlL6uuc%3D&amp;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 (--).