[netmod] Yangdoctors early review of draft-ietf-ntp-yang-data-model-03

Andy Bierman <andy@yumaworks.com> Tue, 09 October 2018 00:45 UTC

Return-Path: <andy@yumaworks.com>
X-Original-To: netmod@ietf.org
Delivered-To: netmod@ietfa.amsl.com
Received: from ietfa.amsl.com (localhost [IPv6:::1]) by ietfa.amsl.com (Postfix) with ESMTP id 54C6F1310F4; Mon, 8 Oct 2018 17:45:24 -0700 (PDT)
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
From: Andy Bierman <andy@yumaworks.com>
To: yang-doctors@ietf.org
Cc: ntp@ietf.org, netmod@ietf.org, draft-ietf-ntp-yang-data-model.all@ietf.org
X-Test-IDTracker: no
X-IETF-IDTracker: 6.86.0
Auto-Submitted: auto-generated
Precedence: bulk
Message-ID: <153904592430.18394.3630036782970324198@ietfa.amsl.com>
Date: Mon, 08 Oct 2018 17:45:24 -0700
Archived-At: <https://mailarchive.ietf.org/arch/msg/netmod/WhLNiHBLwKdyrbIQMECvi52J1qM>
Subject: [netmod] Yangdoctors early review of draft-ietf-ntp-yang-data-model-03
X-BeenThere: netmod@ietf.org
X-Mailman-Version: 2.1.29
List-Id: NETMOD WG list <netmod.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/netmod>, <mailto:netmod-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/netmod/>
List-Post: <mailto:netmod@ietf.org>
List-Help: <mailto:netmod-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/netmod>, <mailto:netmod-request@ietf.org?subject=subscribe>
X-List-Received-Date: Tue, 09 Oct 2018 00:45:24 -0000

Reviewer: Andy Bierman
Review result: Almost Ready

   - 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
         type uint16 {
           range "123 | 1025..max";
         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
      <transmit-time>10-10-2017 07:33:55.300 Z+05:30
      <transmit-time>10-10-2017 07:33:55.300 Z+05:30\
   - a spell checker should be used; There are many description-stmts
     with spelling errors