Subject: [yang-doctors] Yangdoctors early review of draft-ietf-rtgwg-policy-model-09

From: Mahesh Jethanandani via Datatracker <[email-masked]>
Date: Tue, 19 May 2020 22:21:47 -0700

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.
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. 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. 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? 5. Remove unnecessary
empty lines in the model.

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.

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.

Section 8 - Security Considerations:

The security considerations section needs to follow the guidelines outlined in
Section 3.7 of RFC 8407.

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.

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.

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.";
}

Please drop <mailto:....> and just keep the e-mail address. That tag works only
when embedded within a HTML document.

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.

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.

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. */

s/compatibiity/compatibility/

I believe masklength is two words. s/masklength-lower/mask-length-lower/ and
the same for masklength-upper. Also update the description.

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.

A idnits run of the draft reveals a few issues. Please address them.

   Checking nits according to https://www.ietf.org/id-info/checklist :
  ----------------------------------------------------------------------------

  == 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.

  == 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'

     Summary: 0 errors (**), 0 flaws (~~), 9 warnings (==), 5 comments (--).

---

From: Yingzhen Qu <[email-masked]>
Date: Sat, 23 May 2020 20:21:19 +0000

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-masked]> wrote:

    
    Summary:

    
    Nits
    
[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
[YQ]: Done.
    
    Comments:

    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.

    The security considerations section needs to follow the guidelines outlined in
    Section 3.7 of RFC 8407.
[YQ]: this section has been rewritten.

 
[YQ]: this section has been rewritten.

    
    
           leaf name {
             type string;
             description

[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.

    import ietf-inet-types {
      prefix "inet";
      reference
[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? 
  
  [YQ]: fixed.
 
 [YQ]:  OSPF and ISIS will use these identities as "metric-type". 

[YQ]: fixed.
    
 [YQ]: fixed.
   
[YQ]: fixed
 
[YQ]: I can't find this example in OpenConfig Github anymore, so removed this section.

       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 :
      ----------------------------------------------------------------------------
    
  [YQ]: fixed

    

    

  
[YQ]: there is dependency on both 'I-D.ietf-netmod-intf-ext-yang' and 'I-D.ietf-netmod-sub-intf-vlan-model'.

---

From: Mahesh Jethanandani <[email-masked]>
Date: Sun, 24 May 2020 11:39:17 -0700

[Trimming the list to open issues]

Hi Yingzhen,

> On May 23, 2020, at 1:21 PM, Yingzhen Qu <[email-masked]> wrote:
> 
> Hi Mahesh,
> 
> I submitted -10 version to address your comments: https://datatracker.ietf.org/doc/draft-ietf-rtgwg-policy-model/ <https://datatracker.ietf.org/doc/draft-ietf-rtgwg-policy-model/> 
> 

ietf-if-extensions is part of the draft-ietf-netmod-inft-ext-yang draft, which is still a draft. It should ideally be in the {draftlib} path variable. I would suggest a open an issue with the tools team to have them track it.

I am assuming of course that you are able to validate the model locally on your machine.
…

> research on this and see whether we should add it to the terminology.

Looking at how the draft describes it, how about something like this?

“A routing policy defines whether a routing protocol imports a route into the routing table, modifies it, or exports a route from the routing table."

…

> Since this become an IETF draft. I removed "policy-definitions" and "policy-statements" 

The general rule is that if the grouping is used only once in the model, and not meant for use outside the module, that it should not be a grouping.

For example, ‘grouping neighbor-set’ is used only once in the model, and I doubt it is meant for use outside of this module. That grouping should be collapsed into the model where it gets used. Same for ‘grouping tag-set’. These are just two examples. There are many more.

>    missing the cross-reference in the table for if-l3-vlan),

A nit. if-l3-vlan is still missing the cross-reference (and the hyperlink) to the draft. Also why idnits is giving you an error at the bottom.

> but does not provide

I meant to say you should drop the word “mailto:" from the list of e-mail addresses of the editors. That tag works inside a HTML document, not inside a YANG model.

> [YQ]: I can't find this example in OpenConfig Github anymore, so removed this section.

Hmm. That leaves the model without any examples, and examples are important.

I was able to find the example in an earlier version of your own draft (01). I would suggest that you take that example and modify it to make it compatible with the latest version of the model. If you need help, let me know.

>      Checking references for intended status: Proposed Standard

The reason you are getting this error is because you are lacking a normative reference to I-D.ietf-netmod-sub-intf-vlan-model in the document. Goes back to the nit I point up earlier.

I would suggest that once you have fixed all the issues in the draft, that you re-run idnits locally on the machine.

---

From: Yingzhen Qu <[email-masked]>
Date: Wed, 27 May 2020 20:36:51 +0000

Hi Mahesh,

Thanks for your nice and patient review. I’ve uploaded version -11 to address your comments.

Major changes:

  *   Removed groupings only used in the model and not meant to be used later by augmentations
  *   Added an xml example in section 11.
  *   Fixed the cross-reference issue.
  *   Changed the “mailto” in the model to “Email”

Looking at the terms we have so far in section 2, I don’t think it’s necessary to add a definition for “routing policy”. But this is open for discussion.

Thanks,

Yingzhen

From: Mahesh Jethanandani <[email-masked]>
Date: Sunday, May 24, 2020 at 11:39 AM
To: Yingzhen Qu <[email-masked]>
Cc: "[email-masked]" <[email-masked]>, "[email-masked]" <[email-masked]>, "[email-masked]" <[email-masked]>
Subject: Re: Yangdoctors early review of draft-ietf-rtgwg-policy-model-09

Hi Yingzhen,

On May 23, 2020, at 1:21 PM, Yingzhen Qu <[email-masked]<mailto:[email-masked]>> wrote:

Hi Mahesh,

I submitted -10 version to address your comments: https://datatracker.ietf.org/doc/draft-ietf-rtgwg-policy-model/<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdatatracker.ietf.org%2Fdoc%2Fdraft-ietf-rtgwg-policy-model%2F&data=02%7C01%7Cyingzhen.qu%40futurewei.com%7Cc70b253b70a247ec74aa08d80011ce8c%7C0fee8ff2a3b240189c753a1d5591fedc%7C1%7C0%7C637259423777478171&sdata=ZN3wyJ8RziT2DBS7WCNgiffg8yDO25jMSrPcMF4TDlM%3D&reserved=0>

…

          leaf name {
            type string;
            description

but does not provide

   import ietf-inet-types {
     prefix "inet";
     reference

---

From: Mahesh Jethanandani <[email-masked]>
Date: Wed, 27 May 2020 15:16:42 -0700

Hi Yingzhen,

Thank you for addressing my comments. I think we are down to the last few.

Thanks for including the example. It really helps in implementation. However, I believe we have a bug. On the command line below, all the YANG models published by IETF reside in my ~/git/iana/yang-parameters directory and the example you included in the draft is in the file called example-routing-policy.xml. On running the command, I get the following error:

% yanglint -s -i -t auto -p ~/git/iana/yang-parameters/ [email-masked] example-routing-policy.xml 
err : Failed to resolve identityref "bgp". (/ietf-routing-policy:routing-policy/policy-definitions/policy-definition[name='export-tagged-BGP']/statements/statement[name='term-0']/conditions/source-protocol)

That is because you do not have “bgp” defined anywhere in your module. The only definition for that exists in ietf-bgp module. But you cannot include that module, because it creates a circular dependency (ietf-bgp depends on ietf-routing-policy).

Luckily for you, ‘source-protocol’ is not a mandatory field, so if you remove the following line in the example, the example is validated.

              <source-protocol>bgp</source-protocol>

For the remaining point see inline.

> On May 27, 2020, at 1:36 PM, Yingzhen Qu <[email-masked]> wrote:
> 
> Hi Mahesh,
>  
> Removed groupings only used in the model and not meant to be used later by augmentations
> Added an xml example in section 11.
> Fixed the cross-reference issue.
> Changed the “mailto” in the model to “Email”
>  

It might be obvious to folks who know and understand routing what "routing policy" means. But it is not obvious to anyone who knows YANG modeling what it means. I am not going to hold up the review because of this, but I would highly encourage the authors to come up with a definition and include it in the draft.

Cheers.

>> On May 23, 2020, at 1:21 PM, Yingzhen Qu <[email-masked] <mailto:[email-masked]>> wrote:
>>  
>> Hi Mahesh,
>> 
>> I submitted -10 version to address your comments: https://datatracker.ietf.org/doc/draft-ietf-rtgwg-policy-model/ <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdatatracker.ietf.org%2Fdoc%2Fdraft-ietf-rtgwg-policy-model%2F&data=02%7C01%7Cyingzhen.qu%40futurewei.com%7Cc70b253b70a247ec74aa08d80011ce8c%7C0fee8ff2a3b240189c753a1d5591fedc%7C1%7C0%7C637259423777478171&sdata=ZN3wyJ8RziT2DBS7WCNgiffg8yDO25jMSrPcMF4TDlM%3D&reserved=0> 
>>

---

From: Yingzhen Qu <[email-masked]>
Date: Fri, 29 May 2020 22:16:23 +0000

Hi Mahesh,

Thank you for your thorough review. I’ve uploaded version -12: removed “source-protocol” from the example and added “Routing Policy” definition.

Thanks,
Yingzhen

Date: Wednesday, May 27, 2020 at 3:17 PM

Hi Yingzhen,

% yanglint -s -i -t auto -p ~/git/iana/yang-parameters/ [email-masked]<mailto:[email-masked]> example-routing-policy.xml
err : Failed to resolve identityref "bgp". (/ietf-routing-policy:routing-policy/policy-definitions/policy-definition[name='export-tagged-BGP']/statements/statement[name='term-0']/conditions/source-protocol)

On May 27, 2020, at 1:36 PM, Yingzhen Qu <[email-masked]<mailto:[email-masked]>> wrote:

Hi Mahesh,

Major changes:

Cheers.

Thanks,
Yingzhen

From: Mahesh Jethanandani <[email-masked]<mailto:[email-masked]>>
Date: Sunday, May 24, 2020 at 11:39 AM
To: Yingzhen Qu <[email-masked]<mailto:[email-masked]>>
Cc: "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>, "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>, "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>
Subject: Re: Yangdoctors early review of draft-ietf-rtgwg-policy-model-09

Hi Yingzhen,

Hi Mahesh,

I submitted -10 version to address your comments: https://datatracker.ietf.org/doc/draft-ietf-rtgwg-policy-model/<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdatatracker.ietf.org%2Fdoc%2Fdraft-ietf-rtgwg-policy-model%2F&data=02%7C01%7Cyingzhen.qu%40futurewei.com%7C6ec8621912f64640bd2208d8028baff0%7C0fee8ff2a3b240189c753a1d5591fedc%7C1%7C0%7C637262146270083465&sdata=g07tQ8sUtiRs7rQqf3tCDBTfp6Ya0q6j681QIxZ3duc%3D&reserved=0>

…

          leaf name {
            type string;
            description

but does not provide

   import ietf-inet-types {
     prefix "inet";
     reference