[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 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
- [netmod] Yangdoctors early review of draft-ietf-n… Andy Bierman
- Re: [netmod] Yangdoctors early review of draft-ie… Ankit Kumar Sinha