Re: Yangdoctors last call review of draft-ietf-rtgwg-policy-model-29

[Mahesh Jethanandani <[e-mail]>]

Hi Yingzhen,

Thanks for addressing my comments.

Cheers.

> On Jul 29, 2021, at 10:18 PM, Yingzhen Qu <[e-mail]> wrote:
> 
> Hi Mahesh,
> 
> Thank you for your review. We just submitted version -30 and addressed your comments. Please see detailed answer below.
> 
> Thanks,
> Yingzhen
> 
>> On Jul 18, 2021, at 9:07 PM, Mahesh Jethanandani via Datatracker <[e-mail]> wrote:
>> 
>> Reviewer: Mahesh Jethanandani
>> Review result: Almost Ready
>> 
>> This review is looking at the draft from a YANG perspective. With that said, I
>> have marked it as almost ready, 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 model provides a generic routing policy
>> framework which can be extended for specific routing protocols using the YANG
>> 'augment' mechanism.
>> 
>> The description in the document and in the model is well written and easy to
>> understand.
>> 
>> Nits
>> 
>> - Repeat of parent as a prefix. It is not necessary to repeat the parent name
>> in child attributes, e.g. routing-policy -> policy-definitions ->
>> policy-definition. This can be shortened to routing-policy -> definitions >
>> definition.
> [Yingzhen]: We didn’t change this unless you have a strong opinion about it.
>> 
>> s/domian/domain/
>> s/suspectable/susceptible/
> [Yingzhen]: thanks for catching these. Fixed.
>> 
>> - Consistency in how the reference statements are written. Most of the
>> reference statements start on a new line, except for a few places where they do
>> not.
> [Yingzhen]: fixed.
>> 
>> Comments:
>> 
>> Section 1 - Introduction:
>> 
>> The document does not mention whether the model is YANG a 1.1 model. It
>> includes RFC 7950 which would imply a 1.1 module, and the YANG model has a
>> yang-version is 1.1., but it would be nice to state it explicitly.
> [Yingzhen]: My understanding is RFC 7950 means YANG 1.1. If this has to be explicitly stated, please let us know.
>> 
>> Section 7.2
>> 
>> - Consider moving identity 'metric-type' and 'route-level' and their derived
>> identities into an IANA maintained module, e.g. 'iana-policy-types', so that
>> module can be updated separately from the rest of the module (much more easily).
> [Yingzhen]: I think it’s a bit too late to make this change unless it’s really necessary.
>> 
>> - The leaf 'mode' is defined as an enumeration with enum values of ipv4 and
>> ipv6. The description however says:
>> 
>>             "Indicates the mode of the prefix set, in terms of
>>              which address families (IPv4, IPv6, or both) are
>>              present."
>> 
>> How does a user indicate both?
> [Yingzhen]: I removed “both”.
> 
>> The model uses a lot of groupings, most of them used only once in the model. It
>> does identify that prefix sets, neighbor sets and tag sets as reusable
>> groupings. Is that the case for the rest of the groupings? Unless these
>> groupings are meant for use by other models, they should be folded into the
>> main container.
>> 
> [Yingzhen]: we removed all the groupings not necessary.
> 
>> Please drop <mailto:....> and just keep the e-mail address. That tag works only
>> when embedded within a HTML document. (This is a leftover item from the early
>> review, and if there was a discussion about it already, just ignore it).
> [Yingzhen]: I’ll leave this to RFC editor.
>> 
>> Section 8 - Security Considerations:
>> 
>> The security considerations section lists /routing-policy as one of the nodes
>> as being sensitive from a write operation perspective. That would imply the
>> whole module is sensitive. It however, goes onto identifying specific nodes
>> within the module. Not clear if the whole module was intended to be identified
>> or specific nodes.
>> 
>> Similarly a sub-tree of the module is identified in
>> /routing-policy/policy-definitions.
> [Yingzhen]: removed these two sub-tree.
>> 
>> If the idea of having a node without a description is to identify
>> (sub-)sections of the module where the nodes occur (the path already indicates
>> so), some words to that effect might help. E.g. In the
>> /routing-policy/policy-definitions section of the module the following nodes
>> are considered vulnerable.
>> 
>> The last paragraph is a fairly generic, and seems to repeat what is already
>> identified above. Moreover, it is not clear what is meant by "related model
>> carries potential risk". What is "related model”?
> [Yingzhen]: modified the text. Hope it’s clear now.
>> 
>> General
>> 
>> A pyang compilation of the model with —ietf and —lint option was clean. A
>> validation of the model and the example in Appendix B also succeeded. Thank you
>> for providing an example.
>> 
>> An idnits run of the draft was generally clean.

Mahesh Jethanandani
[e-mail]