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

[Yingzhen Qu <[e-mail]>]

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