Re: [i2rs] Yangdoctors early review of draft-ietf-i2rs-rib-data-model-09

[Amit Dass <[e-mail]>]

Hi Tom,

Thanks for the good feedback. Please see my response inline below along with the updated draft version 11.


Best regards,
Amit

-----Original Message-----
From: t.petch [mailto:[e-mail]] 
Sent: Friday, February 16, 2018 7:01 PM
To: Ebben Aries <[e-mail]>
Cc: [e-mail]; [e-mail]
Subject: Re: [i2rs] Yangdoctors early review of draft-ietf-i2rs-rib-data-model-09

Looking at -10, from the standpoint of one who is not a YANG doctor, I see some issues outstanding.

"   This document defines a YANG [RFC6020][RFC6991] data model"
- RFC6020 is YANG 1.0 not 1.1 - I think there needs to be a reason not to use 1.1
AD: Updated with RFC 7950 reference,
- And what is the status of this vis-a-vis NMDA?  Needs stating

- And RFC6991 is an odd reference for YANG. since it is data types; not off the wall, but seems oddto me

1.2 Guidelines on tree diagrams can be found in Section 3 of
   [I-D.ietf-netmod-yang-tree-diagrams].
-This should be a Informative Reference, not a Normative one

AD: Moved to informative ref.

2.2.  Routing Instance and Rib
- RIB?
Correction done
3.  YANG Modules

 <CODE BEGINS> file "[e-mail]"
- The date will need updating and this calls for  a Note to the RFC Editor
AD: Will follow up to remind the RPC


 "  namespace "urn:ietf:params:xml:ns:yang:ietf-i2rs-rib";
   // replace with iana namespace when assigned  "
I do not understand this
AD: not relevant, removed it.

import ietf-interfaces {
     prefix if;
         reference "RFC 7223";
   }
- RFC7223 has been superseded unless you require the old version in which case you should date the import.
Either way, the referenced I-D/RFC needs to appear in the Reference section of this I-D

AD: Updated with RFC 7277 reference.

"    Copyright (c) <2018> IETF Trust and the persons
      identified as authors of the code.  All rights reserved.";  "
- This looks inadequate to me; see for example draft-ietf-netmod-syslog-model for a more comprehensive one
AD: The copyright statement was updated in version 10. Please check the latest draft.
   revision "2018-02-12" {
- as before, a note to the RFC Editor is needed

         // RFC Ed.: replace XXXX with actual RFC number and remove
         // this note
- I see no XXXX to be replaced here
AD: Added XXXX 😊

"leaf hop-limit {
       type uint8;
       description
         "The hop limit the header.";
"
Is something missing from the description?

AD: Modified the section as below:

leaf hop-limit {
      type uint8;
	    range "1..255";
      description
        "The path hop limit header.";


description   "NvGRE can use eigher IPv4    or IPv6 header for
encapsulation.";
- gher?
AD: Corrected the typo.
- The  module has very few reference clauses and when I see something capitalised, e.g.

leaf ip-rpf-check {
               "Each RIB can be optionally associated with a
            ENABLE_IP_RPF_CHECK
I suspect that a reference is required
AD: I couldn't find it in the latest draft. Could you please recheck the latest draft and confirm?

leaf route-change-reason

- The values that this can take seem rather few
AD: This has been reference in the info model, so although limited this is expected to be quite important. Do you have any suggestions that we can add here on top?


Finally, I like the Security Considerations

Tom Petch

----- Original Message -----
From: "Ebben Aries" <[e-mail]>
To: <[e-mail]>
Cc: <[e-mail]>; <[e-mail]>;
<[e-mail]>
Sent: Thursday, January 18, 2018 8:33 AM
Subject: [i2rs] Yangdoctors early review of
draft-ietf-i2rs-rib-data-model-09


> Reviewer: Ebben Aries
> Review result: On the Right Track
>
> 1 module in this draft:
> - [e-mail]
>
> No YANG validation errors or warnings (from pyang 1.7.3 and yanglint
0.14.59)
>
> 0 examples are provided in this draft (section 3.12 of
> draft-ietf-netmod-rfc6087bis-15)
>
> Module [e-mail]:
> - yang-version statement missing - should be 1.1
> - prefix 'iir' is recommended for this module, would 'rib' suffice
better?
> - import "ietf-inet-types" should reference RFC 6991 per (not as a
comment)
>
https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-15#section-4.7
> - import "ietf-interfaces" should reference RFC 7223 per
>
https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-15#section-4.7
> - import "ietf-yang-types" should reference RFC 6991 per
>
https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-15#section-4.7
> - Since this module imports "ietf-interfaces", a normative references
must be
>   added per
>
https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-15#section-3.9
> - prefix "if" in the import "ietf-interfaces" can remove quotes to
remain
>   consistent with other imports
> - Remove WG Chairs from contact information per
>
https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-15#appendix-C
> - Module description must contain most recent copyright notice per
>
https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-15#appendix-C
> - Module description should contain note to RFC Ed. and placeholder
reference
>   to RFC when assigned
>
https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-15#appendix-C
> - Add placeholder reference and note to RFC Ed. for RFC when assigned
>
https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-15#appendix-C
> - Security Considerations should be updated to reflect new template at
>   https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines
> - Section 1.2 should be replaced with reference to
>   draft-ietf-netmod-yang-tree-diagrams-02 rather (as-is in other i2rs
YANG
>   drafts in progress) per
>
https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-15#section-2.5.
1
> - This module contains '12' features.  While it is understood the
purpose of
>   these features in the module, take precaution as to complexity for
clients
>   if they need to understand >= quantity of features per module in use
on a
>   network-element.
> - A few comments exist that are either unecessary or redundant.
Encode the
>   comment intent rather in description fields if need be.
> - Per NMDA, which datastores are targeted for the module?  Will all
RPC
>   operations be acting upon the dynamic/ephemeral datastore?  It is
not clear
>   to me if the intention is to be persistent or ephemeral
>
> General comments/Nits:
> - references to 'def' could be expanded out to 'definition'
> - references to 'decap' could be expanded out to 'decapsulation' for
>   readability (across definitions and descriptions)
> - Follow consistent capitalization of 'RIB' throughout document text.
Mixed
>   use of 'Rib' and 'rib' exists (Outside of YANG node lowercase
definitions).
> - Is it necessary to prefix all nodes under the nexthop container with
>   "nexthop-"?
> - Section 2.5 - route-add RPC - text mentions it is required that the
nh-add
>   RPC be called as a pre-requisite however if the nh already exists
and the
>   nexthop-id is known, this should not be necessary.  In addition, the
text
>   reads 'or return' which should rather be a result of querying the
>   appropriate node in the data tree.
> - In 'IANA Considerations' - s/This document requests to register/This
>   document registers/
>
> _______________________________________________
> i2rs mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/i2rs