Subject: [yang-doctors] Yangdoctors early review of draft-ietf-ntp-yang-data-model-03

From: Andy Bierman <[email-masked]>
Date: Mon, 08 Oct 2018 17:45:24 -0700

Reviewer: Andy Bierman
Review result: Almost Ready

Overall:
   - no compiler warnings; passes --ietf check as well

Normative Sections:

sec 1:
   - YANG version cited should be RFC 7950, not 6020.
     YANG 1.1 is used in this document.\

sec 4: Relationship with RFC 7317
  - this section should say how overlapping configuration
    objects interact. Does setting 1 field (e.g., /system/ntp/enabled)
    change the other field at the same time  (e.g., /ntp/enabled)
    It seems the intent is to ignore and deprecate /system/ntp if
    /ntp is implemented.  This section should explain.

sec 5:
  - this section is supposed to have citations for every imported
    YANG module used in the ietf-ntp YANG module

YANG Module Issues:
  1 - Indexing used in /ntp/associations list
     key "address local-mode isConfigured";

     a) will there really be multiple instances per address for
        different association-modes?  The list description-stmt
        should explain.

     b) There can be configured values (isConfigured key) and learned
        entries for the same address and association-modes?  Why isn't
        NMDA used instead? (i.e. intended and operational datastores
        used instead of the isConfigured list key?)

  2 - Purpose of /ntp/access-rules
      There is no explanation for what it means to configure an entry
      here that points at an ACL entry in /acls/acl; The description
      statement needs to specify the details.  Doesn't the ACL module
      just work? How does the /ntp/access-rules/access-rule list
      change behavior?

  3 - Reinvent Key Management
      It does not seem like a good idea for every protocol to
      duplicate key management functionality. The draft should
      explain why other YANG modules related to this work are not
      relevant here

  4 - Reference statements
      There are no reference statements used in any objects or typedes
      Definitions which are intended to match a definition in NTP
      should include a reference-stmt

Minor Issues:
   - typedef names should be singular
      - access-mode not access-modes
      - association-mode not association-modes
   - grouping comman-attributes should be common-attributes
   - mixed-case naming should not be used (isConfigured)
   - indentation used in module needs a lot of corrections
   - some descriptions have incorrect tense (e.g., "NTP is enable")
   - port definition should be based on inet:port-number, not uint16
     OLD:
         type uint16 {
           range "123 | 1025..max";
         }
     NEW:
         type inet:port-number {
           range "123 | 1025..max";
         }

   - /ntp/authentication/trusted-keys
     Why is this list needed? Seems a leaf (e.g., trusted-key (true/false)
     added to the authentication-keys list is sufficient
   - /ntp/clock-state : some leafs should have a units-stmt instead of
     specifying the units in the description-stmt (could also apply elsewhere)
   - Examples should use line continuation convention
     e.g.:
     OLD:
      <transmit-time>10-10-2017 07:33:55.300 Z+05:30
         </transmit-time>
     NEW:
      <transmit-time>10-10-2017 07:33:55.300 Z+05:30\
         </transmit-time>
   - a spell checker should be used; There are many description-stmts
     with spelling errors

---

From: Ankit Kumar Sinha <[email-masked]>
Date: Mon, 15 Oct 2018 21:05:37 +0530

Hi Andy,

Please find our comment in line below

Thanks,
Ankit

>      YANG 1.1 is used in this document.\

[Authors]: Updated

>     /ntp is implemented.  This section should explain.

[Authors]: Updated

>     YANG module used in the ietf-ntp YANG module

[Authors]: In section 1.4, we have a table of all the improted yang modules
with references. We also use reference statement in the yang model in
section 5. All imported modules are mentioned as normative reference. Is
there any other change you would like to see?

>         different association-modes?  The list description-stmt

[Authors]: It is possible to have different association-modes for same
address. Example is updated in the the document

>         used instead of the isConfigured list key?)

[Authors]: Here /ntp/associations/ is read only, which can have learned
entries and configured entries. Configured entries depends on "unicast,
broadcast, multicast and manycast" configurations. Lets take following
examples

1) If RT1 acting as broadcast server,
   and RT2 acting as broadcast client, then RT2
   will form dynamic association with address as RT1,
   local-mode as client and isconfigured as false.

2) When RT2 is configured
   with unicast-server RT1, then RT2 will form
   association with address as RT1, local-mode as client
   and isconfigured as true.

>       just work? How does the /ntp/access-rules/access-rule list

[Authors]: We have updated draft with "section 5 Access Rules" with
explanation

>       explain why other YANG modules related to this work are not

[Authors]: We have updated draft with "section 6 Key Management" with
explanation

>       should include a reference-stmt

[Authors]: Updated

>    - port definition should be based on inet:port-number, not uint16

[Authors]: All the above comments are updated

>      added to the authentication-keys list is sufficient
[Authors]: trusted-key is removed and leaf istrusted is added to
authentication-key

>    - a spell checker should be used; There are many description-stmts

[Authors]: All the above comments are updated