Re: [Teas] Yangdoctors early review of draft-ietf-teas-yang-rsvp-10

Hi Ebben,

Thanks much for your review and comments.
We have uploaded a new revision ver-11 that addresses your comments.
The diff is at https://tools.ietf.org/rfcdiff?url2=draft-ietf-teas-yang-rsvp-11.txt
Please look inline for on resolution details and do let us know if you have further comments.

On 5/3/19, 8:11 PM, "Teas on behalf of Ebben Aries via Datatracker" <teas-bounces@ietf.org on behalf of noreply@ietf.org> wrote:

    Reviewer: Ebben Aries
    Review result: On the Right Track

    2 modules in this draft:
    - ietf-rsvp@2019-02-18.yang
    - ietf-rsvp-extended@2019-02-18.yang

    No YANG compiler errors or warnings (pyang 2.0, yanglint 1.1.16, confdc 6.6.1)

    Module ietf-rsvp@2019-02-18.yang:
    - Remove WG Chairs from contact information per
      https://tools.ietf.org/html/rfc8407#appendix-B
[TS]: done.

    - Use of 'state' containers.  It is stated in Section 2.3 that 'Derived state
      data is contained under a "state" container...'.  My only comments here are:
      a) Should use caution when using 'state' containers in NMDA compliant
      modules.  Rob put together a nice doc here that I won't reiterate:
      https://github.com/netmod-wg/FAQ/wiki/NMDA-Modelling-FAQ
      Using such nomenclature locks you into r/o nodes only.
[TS]: removed reference to "state" container for consistency.

      b) In some cases, the hierarchy is a bit redundant (statistics/state).
      Other NMDA compliant modules will not introduce another 'state' hierarchy
      for instance (see ietf-interfaces)
[TS]: removed 'state' container.

    - leaf 'packet-len' is abbreviated while most other leafs are not
[TS]: fixed.

    - authentication-key is of type string.  Is this leaf mean to be clear-text?
      There is nothing associated with this type that would imply special
      treatment in handling.
[TS]: Yes, this is entered in clear text. This is analogous with the 'keystring' defined in rfc8177. We discussed this amongst authors, and we think there may be a gap in YANG/NMDA on retrieving applied secret string configuration (e.g. passwords). I'm not sure if this was discussed before for other protocols or if there are any extensions in NMDA or YANG to allow retrieving/displaying it e.g. "****" instead of the clear text that was entered. 

    - crypto-algorithm: Are all identities from ietf-key-chain supported?
[TS]: We are not adding any restriction on the support of any of the algorithms defined in ietf-key-chain standard model. However, a vendor model can apply a strict check using a deviation.

    - Pluralization on counters:  e.g. 'in-error' vs. 'in-errors'. Maintain
      consistency with other modules (see ietf-interfaces)
[TS]: done.

    - Normative references are missing for some of the modules imported.  These
      must be added per https://tools.ietf.org/html/rfc8407#section-3.9
[TS]: done.

    Module ietf-rsvp-extended@2019-02-18.yang:
    - Remove WG Chairs from contact information per
      https://tools.ietf.org/html/rfc8407#appendix-B
[TS]: done.

    - Use of 'state' containers.  It is stated in Section 2.3 that 'Derived state
      data is contained under a "state" container...'.  My only comments here are:
      a) Should use caution when using 'state' containers in NMDA compliant
      modules.  Rob put together a nice doc here that I won't reiterate:
      https://github.com/netmod-wg/FAQ/wiki/NMDA-Modelling-FAQ
      Using such nomenclature locks you into r/o nodes only.
      b) In some cases, the hierarchy is a bit redundant (statistics/state).
      Other NMDA compliant modules will not introduce another 'state' hierarchy
      for instance (see ietf-interfaces)
    - Pluralization on counters:  e.g. 'in-error' vs. 'in-errors'. Maintain
      consistency with other modules (see ietf-interfaces)
    - 9 features are defined with no 'if-feature' statements.  Where are these
      feature conditions meant to be used?
[TS]: The defined features are for extended RSVP feature support that other modules (outside the scope of this document) may choose to use to control access to specific data nodes.

    - Normative references are missing for some of the modules imported.  These
      must be added per https://tools.ietf.org/html/rfc8407#section-3.9
[TS]: Added.

    General comments on the draft/modules:
    - The state container and list key designs appear very familiar to that of
      OpenConfig modules however not consistent with IETF module design.
      Consistency is key from each producing entity (e.g. IETF in this case)
[TS]: addressed and removed instances of 'state' container.

    - The draft mentions RPCs and Notifications however none are defined within
      the modules
[TS]: We added two new RPCs to the draft:
- one to clear rsvp session (single or all)
- one to clear rsvp neighbor (single or all)

    - Section 2.3: Has examples of RPCs and Notifications that are non-existant in
      the modules
[TS]: We added a new section for notifications and suggested leveraging I-D.ietf-netconf-subscribed-notifications] and I-D.ietf-netconf-yang-push in general to register on specific data nodes.

    - Abstract: s/RVSP/RSVP/
    - Abstract: s/remote procedural/remote procedure/
    - Section 2: s/extensiion/extension/
    - Section 4: Update the security considerations section to adhere to
      https://tools.ietf.org/html/rfc8407#section-3.7 and
      https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines
[TS]: Fixed above.

    - Various (missing) wording/pluralization cleanup throughout that I'll defer
      to the RFC Editor.  A once over proofread should solve this.
[TS]: in progress.

Regards,
Tarek    
    _______________________________________________
    Teas mailing list
    Teas@ietf.org
    https://www.ietf.org/mailman/listinfo/teas