Re: [Ntp] [Last-Call] Last Call: <draft-ietf-ntp-yang-data-model-10.txt> (A YANG Data Model for NTP) to Proposed Standard

tom petch <daedulus@btconnect.com> Tue, 23 February 2021 11:26 UTC

Return-Path: <daedulus@btconnect.com>
X-Original-To: ntp@ietfa.amsl.com
Delivered-To: ntp@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 02E9C3A146A; Tue, 23 Feb 2021 03:26:57 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.9
X-Spam-Level:
X-Spam-Status: No, score=-1.9 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, MSGID_FROM_MTA_HEADER=0.001, NICE_REPLY_A=-0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (1024-bit key) header.d=btconnect.onmicrosoft.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id INoPxzGrCcJw; Tue, 23 Feb 2021 03:26:52 -0800 (PST)
Received: from EUR04-HE1-obe.outbound.protection.outlook.com (mail-he1eur04on0706.outbound.protection.outlook.com [IPv6:2a01:111:f400:fe0d::706]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id D01523A1460; Tue, 23 Feb 2021 03:26:51 -0800 (PST)
ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=HGkksdxCOR/weFTFmS7V5152RuBxlXLqBLQfPf9IoYB6HynKEfhVrl699F5vxUuyWtxuNzKhWndKqymaUWT5mHnlMeiqlMgfRVu0lyW2cJL/SZLwPCqzZP7gCCEkFYwncfobI03mb4mxC9PHBmiCihQCoXt/1Ez1heqoXM6x66a/dKxnB8hjj66otVqciGGBl0Vs0q30OMlcbSxg1M4nz+74v6jihyobiHUW680Jq7ZyysRTUgm1IqPsyWPUaAQEB+W5atmPV6pvLMGVuilIWFIpoy9zzqB8L7w3h30y301/Lf6fNQjPFlc92qxzLsC2hUtSUZ9ojjRRl1+2UHQWxQ==
ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=N5I59iOWIfTcxQi9/VXk/AEXzf4QjUkiKxKv/l/DvMw=; b=EjJOJfbWh5DYgJYbUXy2c4zYL5UOKz8eGDAEbNV5ZK3/EkVgIwP6uDsWwA6Z6zbLpH0ys1NBxK+NVr933xbwsfEKAW/LYX+kFoTrcxkE1qb+nuhs6lMyABFFjz4mKF4X8Efdg/qL4x9VujT47UqIaQCvhVniGiz/NcYtAMlAB6BoJ+UkTeWngXM2MkuC2nM1/fsjuX2PwFkBqqkwnNWPMs1wGMPlHNqPTGYDzT96K2v8yK8Vh8zDkJ2GtaE7JL65xcchtvBc1hHKVz0w2tUwnM2lY9MByzAwB/WwbUfkSFhIHKXxBEPSBq0HGllvEm81j5+tHV9DnA7dEpgHdR746g==
ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=btconnect.com; dmarc=pass action=none header.from=btconnect.com; dkim=pass header.d=btconnect.com; arc=none
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=btconnect.onmicrosoft.com; s=selector2-btconnect-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=N5I59iOWIfTcxQi9/VXk/AEXzf4QjUkiKxKv/l/DvMw=; b=IYp83a/OWTNgtSwz94kppVXMIGjfNFdtp1uZgWoMSWTz8mvPKm3rji362hzj5xCCrAIAd8kCBC/e8DLC+i3LOrSJNJQK4YheOi8VvxqN2bEWEUUGqY2WiHxlmhVruzc8nIEW07cbmdpEe2VtJs/EA8NRM2P+xG2frQFkIj57anw=
Authentication-Results: gmail.com; dkim=none (message not signed) header.d=none;gmail.com; dmarc=none action=none header.from=btconnect.com;
Received: from VI1PR07MB6704.eurprd07.prod.outlook.com (2603:10a6:800:18b::8) by VI1PR07MB5502.eurprd07.prod.outlook.com (2603:10a6:803:b7::29) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3890.16; Tue, 23 Feb 2021 11:26:34 +0000
Received: from VI1PR07MB6704.eurprd07.prod.outlook.com ([fe80::58e4:6cf1:a739:545d]) by VI1PR07MB6704.eurprd07.prod.outlook.com ([fe80::58e4:6cf1:a739:545d%5]) with mapi id 15.20.3890.018; Tue, 23 Feb 2021 11:26:34 +0000
To: Dhruv Dhody <dhruv.ietf@gmail.com>
References: <161195994417.2651.6499166797756243533@ietfa.amsl.com> <CAB75xn5CQr2yg7wWZHj-sJM7WaaTJK5NF0pzzLhqmx5hHf8GiQ@mail.gmail.com> <60266E12.6070207@btconnect.com> <602A611E.4020306@btconnect.com> <CAB75xn7QVL+F_5bQ8roZYakbADgQ06pChb0ei7Oaf0=eqLu7Mg@mail.gmail.com>
Cc: NTP WG <ntp@ietf.org>, last-call@ietf.org, Ankit Kumar Sinha <ankit.ietf@gmail.com>, ntp-chairs@ietf.org, draft-ietf-ntp-yang-data-model@ietf.org, ek.ietf@gmail.com, Dieter Sibold <dsibold.ietf@gmail.com>
From: tom petch <daedulus@btconnect.com>
Message-ID: <6034E663.8060907@btconnect.com>
Date: Tue, 23 Feb 2021 11:26:27 +0000
User-Agent: Mozilla/5.0 (Windows NT 5.1; rv:38.0) Gecko/20100101 Thunderbird/38.5.0
In-Reply-To: <CAB75xn7QVL+F_5bQ8roZYakbADgQ06pChb0ei7Oaf0=eqLu7Mg@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"; format="flowed"
Content-Transfer-Encoding: 7bit
X-Originating-IP: [86.146.121.140]
X-ClientProxiedBy: LO2P265CA0386.GBRP265.PROD.OUTLOOK.COM (2603:10a6:600:f::14) To VI1PR07MB6704.eurprd07.prod.outlook.com (2603:10a6:800:18b::8)
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
Received: from [192.168.1.65] (86.146.121.140) by LO2P265CA0386.GBRP265.PROD.OUTLOOK.COM (2603:10a6:600:f::14) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.20.3868.31 via Frontend Transport; Tue, 23 Feb 2021 11:26:33 +0000
X-MS-PublicTrafficType: Email
X-MS-Office365-Filtering-Correlation-Id: ab2b1dac-afe1-49be-a447-08d8d7ede02b
X-MS-TrafficTypeDiagnostic: VI1PR07MB5502:
X-MS-Exchange-MinimumUrlDomainAge: ietf.org#9481
X-Microsoft-Antispam-PRVS: <VI1PR07MB5502CCDBF783EFE1618FE506C6809@VI1PR07MB5502.eurprd07.prod.outlook.com>
X-MS-Oob-TLC-OOBClassifiers: OLM:4941;
X-MS-Exchange-SenderADCheck: 1
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: GN/s7gtEp5pawrih5Mz3F/tU9BlgDBQ3RNZyFyScWOMGUygX2QX/WUC2PthieHcbx4+Jv7tfVRobHpmfed0pD+VFg4dlAzqYe7R1pHY+MQTLIUW8cjXHqZejFxoApBqcXkjjk4X22hAWV93mdzGW3KJwIm4PSv4KLsewHYpGRGPFCNDKUYUAfxDLGAsx7NT2jAANz9Yo3ea3mYi6RZxz3PPgn0R5PkMiLJLe4xaXIhN2CNG1a0x8mlEmE64fmgsNueYBcH+XV4OkFtgdo33qS9bTjjcTu43SIOjunNrsyUNgyOoDI3juNKd7g5AuWxI93PUsyHHzBMWQ9oLnhhT3LHNpqxD6eJMsrUqrnNy8UzJcg5gLPn2CEACD+l8E8zCrcC2kA26b/N+p5NhuHOzr8flZ18YQOXo886oEV+9bKJR/EJPAcze8CY4Zolatxs/C1UZy1Wo3G/+WZvOapeEoy5mYn5rmyGuicKC80tR2qbDihYyUVDCTxZTDrhrnT/iZHmLoLnSy2fTLpc1xZ+JFXAjfA8Phf39f8fJNn+oS98blJfz5SKBW443JqbPQireJTDzk05CTt+9vE2eeYqJzXcdpE9K3zjEGiU4ZeBSOMex+Y+vFvbS+HOFc3YjzejZ6FBHTxllZAvF6k8PYPGw+jQ==
X-Forefront-Antispam-Report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:VI1PR07MB6704.eurprd07.prod.outlook.com; PTR:; CAT:NONE; SFS:(136003)(396003)(346002)(376002)(39860400002)(366004)(6916009)(30864003)(2906002)(83380400001)(5660300002)(66556008)(86362001)(36756003)(478600001)(966005)(66476007)(66946007)(45080400002)(4326008)(54906003)(2616005)(8936002)(53546011)(6666004)(52116002)(6486002)(186003)(316002)(956004)(87266011)(26005)(16526019)(16576012)(33656002)(8676002)(21314003); DIR:OUT; SFP:1102;
X-MS-Exchange-AntiSpam-MessageData: kRJTkHugfvSxGhoXaykvX+wEYEUmJZ2b1UJVvyj0gJ4WhMBWbGdQ/VgMXlNQJJ/DB8p7gj3yIycSoXVRsTvPhhF73zSM5xVPtlJTvEi2+tZPyMUbEcBD5Qp+LziZpseHrTAPmpPHCaiywAzlAhu2l+N6SPC7aP8hncdqUsyvYurgOxQCTMzt//8Hn25Syprg6c3P5XXbeFi2SpNAesfeCnJw282+nJGSK+/jYy+bFn97mifPY4nsVRd4FXKTHSU6qh2lkG6JVG9e4aprqXSgI9qgqd2xSDva5KSW10jZjkuwRdBxCBpl3BGOB6hX0X7g9SPD/n8U7ttp2D0a5TBsgrR3Vn72orWaKntSGUsbVFghNHTFDWuDUZ/we17nWGRDZKTTAV1XY5v2UheNLT3voCcmSuV8mmtTVNOqdUU0RCifP0F9LfeuQoNwef88+9/zHIhqIJkYHGWP3ozmLd7ibIykYfPdckj3EzcrfX8ojyaWSkWURIwKW6n7sk2jilyeiUXK0dGwFPQGcuWfZ9alRX8RroVoPxCarYaN2SiUcnKnmflDaeEOfvptTltEpsL7fK7om73BJ/GSzvQbo9q4yCTmSfCSZC9Yu21lXi330wrTYBdYkBs15fdkWG4hjmuoFKcDQSKG7VH7EXZwwGwqt3hAGSOYoVv2h/+yE8RbVRYXVUNaUZMiAc+blq673IP3N2Q6jiHfCNawdIQVlJuKj8WmoruK4BbqztaB/mKuPmzf0OWlNzGB0m4PWgZfJT/31d/VWLgAbrzGrK9zLYdSxbYlSo3vHOhbPb1po++slwIrbouKANv3P3vyX2/h5ueTGGMj9WpfcBP7ANlrAR39tWZkGBNjA6ud6g96n+9t0HCOzUgLNv6FOaFOAZGGnpJjNxyhl/QKndgofDdZ9wAAxlYg9TUL6GASNxBSMvgue3V2W+5H9VPTTSF0e2ywLisJ+/zxtr4vRSmuuKugPlzuBV+9LT8uNHlExAL5xSK0pFHWbiNSOt1vjq15z4HfWina6C3vMULuCPk/n/8H6vKmNt0fabriWNMtxHVGZAtM3WBRh32ZUmt+S+Vya0NKT83S68Blr5mxJOJ72qf7Rq4nskUxYoDGwS76Sopfvo2qDovqMtZ5KWDPeh5B95iLhWYsMKrwz9bExOsmSGuEYO8lvqibX3NqTYMBIRrJIGNBcOOzeF/dSy0W71kImJQWbfwYH3ydD+3eHU20hCGCpxdYpahO92EJ0Gaj+e7PnY1rUep6u6DpxtG9rIWiOF2rZAm8Ht2IPwzp4zUg5Qg2vHvQ5wOhn1+Dl+MQfxomqWhaeTLbC2gUPc/U8j9+GL3LCIlZ
X-OriginatorOrg: btconnect.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ab2b1dac-afe1-49be-a447-08d8d7ede02b
X-MS-Exchange-CrossTenant-AuthSource: VI1PR07MB6704.eurprd07.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Feb 2021 11:26:34.5363 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: cf8853ed-96e5-465b-9185-806bfe185e30
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: LzhRbyWZ7EOQ6WS2VcHzXLNqft5M1uu7eEl/HKEHePIh3JHyyGDSQ1XsvER1las4xkyNTvDkqvy6Bk+yEc+2LQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR07MB5502
Archived-At: <https://mailarchive.ietf.org/arch/msg/ntp/JLgtpYQdHASHkEwWFiIfXV5lpvA>
Subject: Re: [Ntp] [Last-Call] Last Call: <draft-ietf-ntp-yang-data-model-10.txt> (A YANG Data Model for NTP) to Proposed Standard
X-BeenThere: ntp@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: <ntp.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/ntp>, <mailto:ntp-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/ntp/>
List-Post: <mailto:ntp@ietf.org>
List-Help: <mailto:ntp-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/ntp>, <mailto:ntp-request@ietf.org?subject=subscribe>
X-List-Received-Date: Tue, 23 Feb 2021 11:26:57 -0000

On 17/02/2021 11:34, Dhruv Dhody wrote:
> Hi Tom,
>
> We have posted another update.

And yet another update (along with 5000 others, or so it seems, as the 
submission window closes:-)

I have been through it and it looks good; I think that I now understand 
access.

I note a 'synchornize' - perhaps 'synchronize'.  That apart, I think 
that I will not now come up with anything new; I got to the examples 
before I realised that the section numbers had changed and that I had 
not noticed that a new section had been inserted early on.  My eyes are 
seeing what they want to see and not what is there:-(

Tom Petch


>
> Updated: https://datatracker.ietf.org/doc/draft-ietf-ntp-yang-data-model/
> Diff: https://www.ietf.org/rfcdiff?url2=draft-ietf-ntp-yang-data-model-13
>
> As suggested by Ben, added a YANG feature for MD5.
>
> Further details -
>
>
>> s.6
>> /For this reason, this YANG module allow/
>> For this reason, this YANG module allows/
>>
>
> Updated
>
>> s.7
>> This has lost its two character margin, needs insetting
>>
>
> Not sure where. I did run "pyang -f yang --keep-comments --yang-line-length 69"
>
>> typedef refid I like, much clearer, but perhaps could do with a
>> reference for MD5, RFC1321
>>
>
> I added the reference around the new MD5 feature (as suggested by Ben)
> and not at refid.
>
>> /Discountinuities in the value/Discontinuities in the value/
>> in three places
>>
>
> Updated.
>
>> when 'false() = boolean(/sys:system/sys:ntp)' {
>> I am sure that this is valid but is not something I am used to seeing.
>> An aspect of YANG that I have never understood is when you have to spell
>> things out and when they are implicit and I think that all you need here is
>> when not <presence container>
>> but would want to verify that with a YANG doctor
>>
>
> I have used when presence before but not sure what is the best way for
> "when not presence" and didn't see it before :)
>
>> The definition of each possible values:/
>> The definition of each possible value:/
>>
>
> Updated.
>
>>         leaf poll {
>>           type uint8;
>> This seems wrong to me.  Looking at s.7.3
>>      Poll: 8-bit signed integer representing the maximum interval between
>>      successive messages, in log2 seconds.  Suggested default limits for
>>      minimum and maximum poll intervals are 6 and 10, respectively.
>> ie signed integer and not uint
>>
>
> int8 is better, updated.
>
>> TTL good to have the reference - I was barking up the wrong tree
>>
>> I note
>> #define TTLMAX          8       /* max ttl manycast */
>> I assume that this is a matter of choice - I see 255 in examples which
>> is at the other end of the spectrum
>>
>
> Updated example.
>
>>             leaf beacon {
>>               type uint8;
>> I cannot find a definition of this in the RFC (which I think a defect in
>> the RFC).  Reverse engineering the code I see beacon compared to
>> unreach, which is defined, as
>>      unreach: integer representing the number of seconds the server has
>>      been unreachable
>> which suggests that beacon is a number of seconds, and not a log base 2
>> value or a counter; is this right? (The RFC should say IMHO)
>>
>
> Yes, it is not log2. I have added more text in the description.
>
>> s.8
>> /for illustration purporses./for illustration purposes./
>>
>
> Updated.
>
>>
>> I have run out of time but have made a second pass.  Some aspects still
>> confuse me, where I cannot be sure of the correlation between YANG and
>> RFC, notable time and mode; I want the words to be identical and they
>> are not!
>>
>>     identity access-mode {
>> I do not understand. The RFC has protocol, association and packet modes
>> but not access mode.  This is perhaps section 3, but I am uncertain, the
>> terminology is not quite the same, which is perhaps ok for an NTP
>> expert, which I am not, but not in a technical specification IMHO.
>>
>
> The access-mode is related to the ACL. It is not related to
> association and packet modes. The RFC does not go into detail but
> mentions its use in Section 9.2.
>
>> Thus you derive
>>     identity query-access-mode {
>> but searching the RFC only yields a match in Security.  I want the terms
>> to be identical!
>>
>
> This is not mentioned, but the granularity comes from the existing ACL
> configurations with NTP.
>
>> In contrast, association mode is in the RFC and so I can see
>>     identity broadcast-client {
>> which lacks a value for mode - perhaps 6.  But I would still like a
>> reference to section 3 for these.
>>
>
> I have made an update and aligned it as per the RFC
>
>>
>>     identity ntp-sync-state {
>> is again troublesome.  The RFC has
>> #define NSET     0       /* clock never set */
>> #define FSET     1       /* frequency set from file */
>> #define SPIK     2       /* spike detected */
>> #define FREQ     3       /* frequency mode */
>> #define SYNC     4       /* clock synchronized */
>> whereas the I-D has
>>     identity clock-not-set {
>>     identity freq-set-by-cfg {
>>     identity clock-set {
>>     identity freq-not-determined {
>>     identity clock-synchronized {
>>     identity spike {
>> which is close but I want them to be identical or a note saying how they
>> are different.
>>
>
> Updated
>
>>
>> Then what is the difference between this and
>>     identity clock-state {
>> which has
>>     identity synchronized {
>>     identity unsynchronized {
>> Why have both?
>>
>
> I have added the description to say one is a higher-level state and
> another a granular one.
>
>> Other thoughts
>>
>> I wonder about the role of
>>     identity aes-cmac {
>>       base key-chain:crypto-algorithm;
>> What does it add that a direct reference to AES-CMAC does not have? Why
>> import key-chain.  I am not saying it has no value but am unclear of
>> that value.  And
>>       leaf algorithm {
>> could do with a reference for AES-CMAC.
>>
>
> We are no longer using key-chain to make sure we could use the YANG
> feature for MD5.
>
>> Some authors include the prefix on the module in the list of prefix;
>> seems logical.
>>
>>      typedef refid
>> MD5 could do with a reference.
>>
>>           leaf clock-stratum {
>> What is syspeer? I do not see it in the RFC:-)
>>
>
> Updated.
>
>>           leaf clock-precision {
>> should be signed not unsigned I think
>>
>
> Updated.
>
>> leaf poll
>> is indeed poll in section 7.3 but I wonder why  13.1 uses hpoll?
>> Probably a comment on the RFC and not on the I-D.
>>
>
> Hal provided the details.
>
>> With ones from my previous post, below, that is it!
>>
>> Tom Petch
>>
>
> Thanks!
> Dhruv & Ankit
>
> On Mon, Feb 15, 2021 at 5:25 PM tom petch <daedulus@btconnect.com> wrote:
>>
>> On 12/02/2021 12:01, tom petch wrote:
>>> On 11/02/2021 09:39, Dhruv Dhody wrote:
>>>> Hi Tom,
>>>>
>>>> Thanks for your review. I have posted an updated version of the YANG.
>>>>
>>>> https://datatracker.ietf.org/doc/draft-ietf-ntp-yang-data-model/
>>>> https://www.ietf.org/rfcdiff?url1=draft-ietf-ntp-yang-data-model-10&url2=draft-ietf-ntp-yang-data-model-12
>>>>
>>>
>>> That was quick!  I have done a first pass but have yet to chase down the
>>> references and one at least looks odd to me. I expect to complete the
>>> process on Monday am GMT.
>>
>> Ian
>>
>> I have run out of time but have made a second pass.  Some aspects still
>> confuse me, where I cannot be sure of the correlation between YANG and
>> RFC, notable time and mode; I want the words to be identical and they
>> are not!
>>
>>     identity access-mode {
>> I do not understand. The RFC has protocol, association and packet modes
>> but not access mode.  This is perhaps section 3, but I am uncertain, the
>> terminology is not quite the same, which is perhaps ok for an NTP
>> expert, which I am not, but not in a technical specification IMHO.
>>
>> Thus you derive
>>     identity query-access-mode {
>> but searching the RFC only yields a match in Security.  I want the terms
>> to be identical!
>>
>> In contrast, association mode is in the RFC and so I can see
>>     identity broadcast-client {
>> which lacks a value for mode - perhaps 6.  But I would still like a
>> reference to section 3 for these.
>>
>>
>>     identity ntp-sync-state {
>> is again troublesome.  The RFC has
>> #define NSET     0       /* clock never set */
>> #define FSET     1       /* frequency set from file */
>> #define SPIK     2       /* spike detected */
>> #define FREQ     3       /* frequency mode */
>> #define SYNC     4       /* clock synchronized */
>> whereas the I-D has
>>     identity clock-not-set {
>>     identity freq-set-by-cfg {
>>     identity clock-set {
>>     identity freq-not-determined {
>>     identity clock-synchronized {
>>     identity spike {
>> which is close but I want them to be identical or a note saying how they
>> are different.
>>
>>
>> Then what is the difference between this and
>>     identity clock-state {
>> which has
>>     identity synchronized {
>>     identity unsynchronized {
>> Why have both?
>>
>> Other thoughts
>>
>> I wonder about the role of
>>     identity aes-cmac {
>>       base key-chain:crypto-algorithm;
>> What does it add that a direct reference to AES-CMAC does not have? Why
>> import key-chain.  I am not saying it has no value but am unclear of
>> that value.  And
>>       leaf algorithm {
>> could do with a reference for AES-CMAC.
>>
>> Some authors include the prefix on the module in the list of prefix;
>> seems logical.
>>
>>      typedef refid
>> MD5 could do with a reference.
>>
>>           leaf clock-stratum {
>> What is syspeer? I do not see it in the RFC:-)
>>
>>           leaf clock-precision {
>> should be signed not unsigned I think
>>
>> leaf poll
>> is indeed poll in section 7.3 but I wonder why  13.1 uses hpoll?
>> Probably a comment on the RFC and not on the I-D.
>>
>> With ones from my previous post, below, that is it!
>>
>> Tom Petch
>>
>>> Meanwhile,
>>>
>>> s.6
>>> /For this reason, this YANG module allow/
>>> For this reason, this YANG module allows/
>>>
>>> s.7
>>> This has lost its two character margin, needs insetting
>>>
>>> typedef refid I like, much clearer, but perhaps could do with a
>>> reference for MD5, RFC1321
>>>
>>> /Discountinuities in the value/Discontinuities in the value/
>>> in three places
>>>
>>> when 'false() = boolean(/sys:system/sys:ntp)' {
>>> I am sure that this is valid but is not something I am used to seeing.
>>> An aspect of YANG that I have never understood is when you have to spell
>>> things out and when they are implicit and I think that all you need here is
>>> when not <presence container>
>>> but would want to verify that with a YANG doctor
>>>
>>> The definition of each possible values:/
>>> The definition of each possible value:/
>>>
>>>         leaf poll {
>>>           type uint8;
>>> This seems wrong to me.  Looking at s.7.3
>>>      Poll: 8-bit signed integer representing the maximum interval between
>>>      successive messages, in log2 seconds.  Suggested default limits for
>>>      minimum and maximum poll intervals are 6 and 10, respectively.
>>> ie signed integer and not uint
>>>
>>> TTL good to have the reference - I was barking up the wrong tree
>>>
>>> I note
>>> #define TTLMAX          8       /* max ttl manycast */
>>> I assume that this is a matter of choice - I see 255 in examples which
>>> is at the other end of the spectrum
>>>
>>>             leaf beacon {
>>>               type uint8;
>>> I cannot find a definition of this in the RFC (which I think a defect in
>>> the RFC).  Reverse engineering the code I see beacon compared to
>>> unreach, which is defined, as
>>>      unreach: integer representing the number of seconds the server has
>>>      been unreachable
>>> which suggests that beacon is a number of seconds, and not a log base 2
>>> value or a counter; is this right? (The RFC should say IMHO)
>>>
>>> s.8
>>> /for illustration purporses./for illustration purposes./
>>>
>>>
>>> Back on Monday, all being well
>>>
>>> I got a bounce message last time from Yi saying on holiday until 22nd
>>> February.
>>>
>>> Tom Petch.
>>>
>>>
>>>> Please find my consolidated response below -
>>>>
>>>> Email 1:
>>>> ========
>>>>
>>>> On Mon, Feb 8, 2021 at 5:52 PM tom petch <daedulus@btconnect.com> wrote:
>>>>>
>>>>> I have two main problems and a lot of lesser ones with this I-D; given
>>>>> the number, about 50, I am not optimistic that a single cycle of
>>>>> revision will see them addressed.
>>>>>
>>>>> I see a potential loophole in the security which I will post separately
>>>>> since the audience is likely to be different.
>>>>>
>>>>> References are missing or not specific enough so when I try to compare
>>>>> values in the I-D with those of the protocol, either I cannot find them
>>>>> or they seem to be different. Giving values to enumerations is unusual
>>>>> in YANG, since NETCONF does not transmit them, and their presence
>>>>> suggests that they are protocol values, in which case I want to see what
>>>>> the protocol says.  A reference to a 110 page I-D, with two updating
>>>>> I-D, is inadequate IMO - section numbers are needed in every case.
>>>>>
>>>>
>>>> I have added section numbers in most of the cases.
>>>> I have changed enums to identity.
>>>>
>>>>> Introduction
>>>>> should mention support, or lack thereof, for NMDA
>>>>>
>>>>
>>>> Its present in Section 1.1 already, I have added keyword NMDA as well
>>>> now.
>>>>
>>>>> 1.4.  Prefixes in Data Node Names
>>>>>               | ianach    | iana-crypt-hash          | [RFC7317] |
>>>>> the reference is wrong; this is an IANA- maintained module so the
>>>>> reference must be to the IANA website
>>>>>
>>>>
>>>> No longer using ianach in this update.
>>>>
>>>>> 1.5.  Refrences in the Model
>>>>> /Refrences/References/
>>>>> /refrenced in /referenced in /
>>>>>
>>>>
>>>> Updated
>>>>
>>>>> 2.  NTP data model
>>>>>
>>>>> I do not see the value of a condensed model followed immediately by a
>>>>> full model. Perhaps the full model should be an Appendix although at
>>>>> less than three pages, this is quite small and would be ok on its own
>>>>> IMHO.
>>>>>
>>>>
>>>> Updated
>>>>
>>>>> 4.  Relationship with RFC 7317
>>>>> /supports per-interface configurations /
>>>>> support per-interface configuration/
>>>>>
>>>>
>>>> Updated
>>>>
>>>>> 5.  Access Rules
>>>>> /refer access-mode) and attach different acl-rule/
>>>>> see access-mode) and attach a different acl-rule/
>>>>>
>>>>
>>>> Updated
>>>>
>>>>> 6.  Key Management
>>>>> /32-bits unsigned /32-bit unsigned/
>>>>>
>>>>> /this YANG modules/this YANG module/
>>>>>
>>>>
>>>> Updated
>>>>
>>>>> NTP association (for example unicast-server),
>>>>> /specefied/specified/
>>>>>
>>>>
>>>> Updated
>>>>
>>>>> 7.  NTP YANG Module
>>>>>
>>>>>        import iana-crypt-hash {
>>>>>         reference         "RFC 7317: A YANG Data Model for System
>>>>> Management";
>>>>> wrong reference - this module is IANA-maintained so the reference must
>>>>> be to the IANA website
>>>>>
>>>>
>>>> Not using iana-crypt-hash anymore!
>>>>
>>>>>        contact
>>>>>           WG List:  <mailto: ntpwg@lists.ntp.org
>>>>> this is not the address I see on the datatracker
>>>>>
>>>>> the I-D has five editors but there are only two here
>>>>>
>>>>
>>>> There are 2 co-authors marked as editors. RFC 8407 says the document
>>>> author or editor contact information needs to be present.
>>>>
>>>>>        typedef access-mode {
>>>>> I cannot find this in RFC5905
>>>>>
>>>>
>>>> Section 9.2 and Appendix A.5.4. I have added section number in reference.
>>>>
>>>>>        typedef association-mode {
>>>>> this I can find but it ranges from 0 to 7 whereas the I-D has 0 to 4
>>>>> - is
>>>>> this intended?
>>>>>
>>>>
>>>> Changed to identity.
>>>>
>>>>>        typedef ntp-sync-state {
>>>>> this I cannot find; a search for 'spike' yields a value of 2 in the
>>>>> RFC, 5 here - is this intended?
>>>>>
>>>>
>>>> Instead of enum and values, changed to identity.
>>>>
>>>>>                 effect in XXX seconds.";
>>>>> for what value of XXX?
>>>>>
>>>>
>>>> Removed.
>>>>
>>>>>          leaf packet-sent {
>>>>>          leaf packet-received {
>>>>>          leaf packet-dropped {
>>>>>               discontinuities in the value of sysUpTime.";
>>>>> those who have been involved with network management for ten years or
>>>>> less will likely not recognise this object.  You could add a reference
>>>>> but I suggest you replace it with a YANG-based approach; see for example
>>>>> how draft-ietf-ospf-yang handles discontinuities
>>>>>
>>>>
>>>> Updated as suggested.
>>>>
>>>>
>>>>>              leaf access-mode {
>>>>> /defination/definition/
>>>>>
>>>>
>>>> Updated
>>>>
>>>>>              leaf clock-refid {
>>>>> ...                         reference clock of the peer to
>>>>>                   which clock is synchronized.";
>>>>>
>>>>> I do not understand this.  Presumably this corresponds to
>>>>>                  type string {
>>>>>                    length "4";
>>>>> from the three type union but what object is this?
>>>>>
>>>>
>>>> I created a typedef and cleaned it up. This is as described in Section
>>>> 7.3 of RFC5905 under Reference ID.
>>>>
>>>>>              leaf clock-offset {
>>>>> examples could do with units to make it clear that it is '1.232mS' and
>>>>> not '1.232s'
>>>>>
>>>>
>>>> Updated
>>>>
>>>>>            leaf address {
>>>>>              type inet:host;
>>>>> this includes the domain name, which I see no mention of in the RFC
>>>>>
>>>>
>>>> Changed to inet:ip-address
>>>>
>>>>>          list associations {
>>>>>               /and isconfigured is required/and isconfigured are
>>>>> required/
>>>>>
>>>>>            leaf address {
>>>>>              type inet:host;
>>>>> as above, the description seems to ignore the option of the domain name
>>>>>
>>>>>            leaf refid {
>>>>> same union as for leaf clock-refid, but a completely different
>>>>> description, neither of which I understand.
>>>>>
>>>>
>>>> Updated (all of the above).
>>>>
>>>>>                   '20.1.1.1'
>>>>> this address would appear to be assigned to Microsoft, not an
>>>>> affiliation I see among the authors.  Is the company ok with this?
>>>>>
>>>>
>>>> Updated to 203.0.113.1
>>>>
>>>>>            leaf reach {
>>>>>              type uint8;
>>>>> is this the 8-bit p.reach shift register? reference needed (again:-)
>>>>>
>>>>>            leaf unreach {
>>>>> ditto
>>>>>
>>>>
>>>> Added.
>>>>
>>>>>            leaf poll {
>>>>>              type uint8;
>>>>>              units "seconds";
>>>>>              description
>>>>>                "The polling interval for current association";
>>>>> is there a useful default?  2s appears in the RFC in places
>>>>>
>>>>
>>>> No default in this case.
>>>>
>>>>>            leaf offset {
>>>>> as above, the example values would be clearer with units
>>>>>
>>>>
>>>> Updated
>>>>
>>>>>            leaf transmit-time {
>>>>>              type yang:date-and-time;
>>>>>              description
>>>>>                "This is the local time, in timestamp format,
>>>>>                 at which the NTP packet departed the peer(T3).
>>>>>                 If the peer becomes unreachable the value is set to
>>>>> zero.";
>>>>> I think, but am not sure, that a yang:date-and-time can never be set to
>>>>> zero, the syntax does not allow it; the usual approach with YANG is a
>>>>> union with another type which can indicate a special condition - int,
>>>>> boolean, etc
>>>>>
>>>>>            leaf input-time {
>>>>>              type yang:date-and-time;
>>>>> ditto
>>>>>
>>>>
>>>> Thanks, updated as per your suggestion.
>>>>
>>>>>                leaf ttl {
>>>>>                  type uint8;
>>>>>                  description
>>>>>                    "Specifies the time to live (TTL)
>>>>> TTL does not exist in IPv6
>>>>>
>>>>
>>>> This TTL is not related to IP. See section 3.1 for manycast and
>>>> updated with reference.
>>>>
>>>>>                uses common-attributes {
>>>>>                  description
>>>>> /attribute like/attributes such as/
>>>>>
>>>>
>>>> Updated
>>>>
>>>>>                leaf ttl {
>>>>>                  type uint8;
>>>>>                  description
>>>>>                    "Specifies the maximum time to live (TTL) for
>>>>> TTL does not exist in IPv6
>>>>>
>>>>
>>>> As above.
>>>>
>>>>>                uses common-attributes {
>>>>> /attributes like/attributes such as/
>>>>>
>>>>
>>>> Updated
>>>>
>>>>>                leaf beacon {
>>>>> what are the units and is there a default? Is there a maximum of 15?  As
>>>>> ever, a reference could tell me.
>>>>>
>>>>
>>>> Added reference
>>>>
>>>>> 8.  Usage Example
>>>>> lots of examples but none for IPv6 or JSON
>>>>>
>>>>
>>>> Added one IPv6. I dont think we need add one for JSON as well.
>>>>
>>>>> 8.1
>>>>>         <sys:ntp xmlns:sys="urn:ietf:params:xml:ns:yang:ietf-ntp">
>>>>> sys: is a defined prefix and must not be re-used
>>>>>
>>>>
>>>> Oops, updated here and all other instance!
>>>>
>>>>>             <refid>20.1.1.1</refid>
>>>>> as above, is Microsoft ok with this?
>>>>>
>>>>
>>>> Updated to 203.0.113.1
>>>>
>>>>> 8.2
>>>>>           <sys:ntp xmlns:sys="urn:ietf:params:xml:ns:yang:ietf-ntp">
>>>>> sys: is a defined prefix and must not be re-used
>>>>>
>>>>> 8.3
>>>>>           <sys:ntp xmlns:sys="urn:ietf:params:xml:ns:yang:ietf-ntp">
>>>>> sys: is a defined prefix and must not be re-used
>>>>>
>>>>> 8.4
>>>>>           <sys:ntp xmlns:sys="urn:ietf:params:xml:ns:yang:ietf-ntp">
>>>>> sys: is a defined prefix and must not be re-used
>>>>>
>>>>> 8.5
>>>>>     "224.1.1.1"
>>>>> would appear to be a reserved address.  Other RFC used 224.0.1.1
>>>>>
>>>>
>>>> Updated
>>>>
>>>>>           <sys:ntp xmlns:sys="urn:ietf:params:xml:ns:yang:ietf-ntp">
>>>>> and again, twice
>>>>>
>>>>>                 <address>224.1.1.1</address>
>>>>> as above
>>>>>
>>>>> 8.6
>>>>>     "224.1.1.1"
>>>>> as above
>>>>>
>>>>>            <sys:ntp xmlns:sys="urn:ietf:params:xml:ns:yang:ietf-ntp">
>>>>> as above, twice
>>>>>
>>>>> 8.7
>>>>>         <ntp xmlns="urn:ietf:params:xml:ns:yang:ietf-ntp">
>>>>> as above
>>>>>
>>>>> 8.8
>>>>>         <ntp xmlns="urn:ietf:params:xml:ns:yang:ietf-ntp">
>>>>> as above
>>>>>
>>>>>             <refid>20.1.1.1</refid>
>>>>> as above
>>>>>
>>>>> 8.9
>>>>>         <ntp xmlns="urn:ietf:params:xml:ns:yang:ietf-ntp">
>>>>> as above
>>>>>
>>>>
>>>> Updated in all cases.
>>>>
>>>>> 12.2
>>>>>       [RFC7317]  Bierman, A. and M. Bjorklund, "A YANG Data Model
>>>>> wrong reference in the wrong place
>>>>> this is an IANA-maintained module and so the reference must be to the
>>>>> IANA website; and since the module is imported, the reference must be
>>>>> Normative.
>>>>>
>>>>
>>>> No longer using iana-crypt-hash.
>>>>
>>>>> Tom Petch
>>>>>
>>>>>
>>>>> ----- Original Message -----
>>>>> From: "The IESG" <iesg-secretary@ietf.org>
>>>>> To: "IETF-Announce" <ietf-announce@ietf.org>
>>>>> Cc: <ek.ietf@gmail.com>; <ntp-chairs@ietf.org>; <ntp@ietf.org>;
>>>>> <dsibold.ietf@gmail.com>; <draft-ietf-ntp-yang-data-model@ietf.org>
>>>>> Sent: Friday, January 29, 2021 10:39 PM
>>>>> Subject: Last Call: <draft-ietf-ntp-yang-data-model-10.txt> (A YANG Data
>>>>> Model for NTP) to Proposed Standard
>>>>>
>>>>
>>>>
>>>> Email 2:
>>>> ========
>>>>
>>>> On Mon, Feb 8, 2021 at 6:07 PM tom petch <daedulus@btconnect.com> wrote:
>>>>>
>>>>> This is my second response to this Last Call, about a possible security
>>>>> issue.
>>>>>
>>>>> RFC8573 seems clear that MD5 must not be used to effect security for NTP
>>>>> but this I-D imports iana-crypt-hash which allows MD5 without any
>>>>> restriction, so is MD5 allowed or not?
>>>>>
>>>>> There are features defined which allow the hash in iana-crypt-hash to be
>>>>> restricted but this I-D does not use them.
>>>>>
>>>>> Probably iana-crypt-hash should be updated - I will raise that on the
>>>>> NETMOD WG list.
>>>>>
>>>>
>>>> No longer using iana-crypt-hash, instead made this change to allow all
>>>> sort of key formats -
>>>>
>>>> OLD:
>>>> |     +--rw key?         ianach:crypt-hash
>>>> NEW:
>>>> |     +--rw key
>>>> |     |  +--rw (key-string-style)?
>>>> |     |     +--:(keystring)
>>>> |     |     |  +--rw keystring?            string
>>>> |     |     +--:(hexadecimal) {hex-key-string}?
>>>> |     |        +--rw hexadecimal-string?   yang:hex-string
>>>> END
>>>>
>>>> The comment related to use of MD5 still applies. I have not yet added
>>>> a check for the algorithm. I have updated the description and security
>>>> consideration to highligh that MD5 is depricated as per RFC 8573. Lets
>>>> see what the IESG suggests...
>>>>
>>>>> The I-D also uses MD5 in a way that would appear not to be security
>>>>> related, to hash an IPv6 address.
>>>>>
>>>>
>>>> This is as per RFC 5905 - "If using the IPv6 address family, it is the
>>>> first four octets of the MD5 hash of the IPv6 address."
>>>>
>>>>> In passing, this I-D has three references to RFC7317.  This is wrong -
>>>>> the module is IANA-maintained and so the references should be to the
>>>>> IANA website.
>>>>>
>>>>
>>>> No longer using iana-crypt-hash.
>>>>
>>>>> The secdir reviewer might be interested in my thoughts.
>>>>>
>>>>> Tom Petch
>>>>
>>>>
>>>> Email 3:
>>>> ========
>>>>
>>>> On Tue, Feb 9, 2021 at 4:40 PM tom petch <daedulus@btconnect.com> wrote:
>>>>>
>>>>> A separate thought to my previous two.
>>>>>
>>>>> Section 4 is very keen that this I-D and the system module which
>>>>> configures ntp SHOULD NOT coexist but this is not enforced by the YANG.
>>>>>
>>>>> It could be.  Import the system module and make the presence container
>>>>> in this I-D dependent on the absence of the presence container in
>>>>> /system/ntp.
>>>>>
>>>>
>>>> Updated as per your suggestion!
>>>>
>>>> ====
>>>>
>>>> Thanks again for your detailed review. It has improved the YANG model
>>>> significantly!
>>>>
>>>> Thanks!
>>>> Dhruv & Ankit
>>>>
>>>> On Sat, Jan 30, 2021 at 4:09 AM The IESG <iesg-secretary@ietf.org> wrote:
>>>>>
>>>>
> .
>