| Hi Yingzhen,
Thanks for addressing my comments.
Cheers.
Hi Mahesh,Thank you for your review. We just submitted version -30 and addressed your comments. Please see detailed answer below.Thanks,YingzhenOn Jul 18, 2021, at 9:07 PM, Mahesh Jethanandani via Datatracker <noreply@xxxxxxxx> 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.
|
