Re: [Netconf] Yangdoctors last call review of draft-ietf-netconf-restconf-notif-11

"Reshad Rahman (rrahman)" <rrahman@cisco.com> Mon, 07 January 2019 19:00 UTC

Return-Path: <rrahman@cisco.com>
X-Original-To: netconf@ietfa.amsl.com
Delivered-To: netconf@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id E3513131002; Mon, 7 Jan 2019 11:00:54 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -14.502
X-Spam-Level:
X-Spam-Status: No, score=-14.502 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIMWL_WL_MED=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, RCVD_IN_DNSWL_HI=-5, SPF_PASS=-0.001, USER_IN_DEF_DKIM_WL=-7.5] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (1024-bit key) header.d=cisco.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 gVOv5mhEJOQl; Mon, 7 Jan 2019 11:00:53 -0800 (PST)
Received: from rcdn-iport-8.cisco.com (rcdn-iport-8.cisco.com [173.37.86.79]) (using TLSv1.2 with cipher DHE-RSA-SEED-SHA (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id A1AB31294D0; Mon, 7 Jan 2019 11:00:52 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=cisco.com; i=@cisco.com; l=6988; q=dns/txt; s=iport; t=1546887652; x=1548097252; h=from:to:cc:subject:date:message-id:references: in-reply-to:content-id:content-transfer-encoding: mime-version; bh=ZuyBnjQJQA94Zn/44Sx4CBYDr3moKSpuxGcTXg9C7nQ=; b=SbeZ+z4DGRlsSqYTbYdVDwyPIQZg4e0xhK0uPY+dZRi7PuCCMeyIRxvj YFkHHNoXanW9rVInfitv3eB7m3wNWr3d8IoDNMkj1MEKmOPKM0QgCfnQE fezLUTQ5c7xziYjTbKSTM3LfMiwiPrNBkCvsmhuapmjOOpNOig+aVZdVr w=;
X-IronPort-Anti-Spam-Filtered: true
X-IronPort-Anti-Spam-Result: =?us-ascii?q?A0ADAABboTNc/4sNJK1aCRkBAQEBAQE?= =?us-ascii?q?BAQEBAQEHAQEBAQEBgVEEAQEBAQELAYFaKWaBAicKg3WIGotrmXuBewsBASy?= =?us-ascii?q?EQAIXgW0iNAkNAQMBAQIBAQJtKIVLBiMEDUUQAgEIGgIJHQICAjAVEAIEAQ0?= =?us-ascii?q?FgyIBggGnIXwzii2BC4s0F4FAP4ERJx+CTIRXQYJxMYIEIgKhXAkCkW8YkW+?= =?us-ascii?q?JYpA0AhEUgScfOIFWcBVlAYJBgiIFF4EAAQeNFnKBKIg7AYEeAQE?=
X-IronPort-AV: E=Sophos;i="5.56,451,1539648000"; d="scan'208";a="500877102"
Received: from alln-core-6.cisco.com ([173.36.13.139]) by rcdn-iport-8.cisco.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 07 Jan 2019 19:00:51 +0000
Received: from XCH-RCD-006.cisco.com (xch-rcd-006.cisco.com [173.37.102.16]) by alln-core-6.cisco.com (8.15.2/8.15.2) with ESMTPS id x07J0pmR025425 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=FAIL); Mon, 7 Jan 2019 19:00:51 GMT
Received: from xch-rcd-005.cisco.com (173.37.102.15) by XCH-RCD-006.cisco.com (173.37.102.16) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Mon, 7 Jan 2019 13:00:50 -0600
Received: from xch-rcd-005.cisco.com ([173.37.102.15]) by XCH-RCD-005.cisco.com ([173.37.102.15]) with mapi id 15.00.1395.000; Mon, 7 Jan 2019 13:00:50 -0600
From: "Reshad Rahman (rrahman)" <rrahman@cisco.com>
To: "Robert Wilton -X (rwilton - ENSOFT LIMITED at Cisco)" <rwilton@cisco.com>, "yang-doctors@ietf.org" <yang-doctors@ietf.org>
CC: "netconf@ietf.org" <netconf@ietf.org>, "draft-ietf-netconf-restconf-notif.all@ietf.org" <draft-ietf-netconf-restconf-notif.all@ietf.org>
Thread-Topic: Yangdoctors last call review of draft-ietf-netconf-restconf-notif-11
Thread-Index: AQHUprCFOch/e2rpE06mX+DnhAHT46WkOf4A
Date: Mon, 7 Jan 2019 19:00:50 +0000
Message-ID: <AE624D93-94E7-4A39-8BAF-DDC0FC54FD33@cisco.com>
References: <154688301471.23184.11003837983933531435@ietfa.amsl.com>
In-Reply-To: <154688301471.23184.11003837983933531435@ietfa.amsl.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
user-agent: Microsoft-MacOutlook/10.10.5.181209
x-ms-exchange-messagesentrepresentingtype: 1
x-ms-exchange-transport-fromentityheader: Hosted
x-originating-ip: [161.44.212.43]
Content-Type: text/plain; charset="utf-8"
Content-ID: <F711D2B7EEF93543AE834E55908BE23B@emea.cisco.com>
Content-Transfer-Encoding: base64
MIME-Version: 1.0
X-Outbound-SMTP-Client: 173.37.102.16, xch-rcd-006.cisco.com
X-Outbound-Node: alln-core-6.cisco.com
Archived-At: <https://mailarchive.ietf.org/arch/msg/netconf/OkonY68Mojekt2wObpxbR57jjys>
Subject: Re: [Netconf] Yangdoctors last call review of draft-ietf-netconf-restconf-notif-11
X-BeenThere: netconf@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Network Configuration WG mailing list <netconf.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/netconf>, <mailto:netconf-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/netconf/>
List-Post: <mailto:netconf@ietf.org>
List-Help: <mailto:netconf-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/netconf>, <mailto:netconf-request@ietf.org?subject=subscribe>
X-List-Received-Date: Mon, 07 Jan 2019 19:00:55 -0000

Thanks Rob, will take a look and respond soon.

Regards,
Reshad.


On 2019-01-07, 12:43 PM, "Robert Wilton" <rwilton@cisco.com> wrote:

    Reviewer: Robert Wilton
    Review result: Ready with Issues
    
    I have reviewed this document as part of the YANG doctors directorate's
    ongoing effort to review all IETF documents being processed by the IESG.  These
    comments were written with the intent of improving the operational aspects of
    the IETF drafts. Comments that are not addressed in last call may be included
    in AD reviews during the IESG review.  Document editors and WG chairs should
    treat these comments just like any other last call comments.
    
    This document and the associated YANG module looks like it is in good shape to
    me.
    
    The two minor issues that I had that I think need to be resolved are:
    - The section 3.4 handling subscriptions by admin users wasn't intuitive to me
    (relative to what is stated in draft-ietf-netconf-subscribed-notifications). 
    Specifically what should an admin user be allowed to do vs the client that
    initiated the subscription. - Section 4 on HTTP2 QoS seemed unclear to me.
    
    I think that all my other comments are really just nits:
    
    1) Section 3.2 Discovery, last paragraph.
    
    Would it be better for this to reference draft-ietf-netconf-nmda-restconf,
    section 2 and '/yang-libary/datastore' draft-ietf-netconf-rfc7895bis, section 3?
    
    2) RPC table in section 3.3.  The identity for "kill-subscription" should
    probably be "delete-subscription-error" since "kill-subscription-error" isn't
    defined.
    
    3) Diagram in 3.4.  It wasn't immediately obvious to me that the vertical lines
    in the lined up with connections (a) and (b).  I'm not sure whether it is worth
    adding a comment to make this more explicit.  I figured it out whilst reading
    the text below, perhaps most other readers would also do so.
    
    4) Section 3.4, bottom of page 7, "must be sent within (b)" -> "MUST be sent
    within (b)"
    
    5) Section 3.4:
       o  RPCs modify-subscription, resync-subscription and delete-
          subscription can only be done by the same RESTCONF username
          [RFC8040] who did the establish-subscription, or by a RESTCONF
          username with the required administrative permissions.  The latter
          also has access to the kill-subscription RPC.
    
    Just to check, is it true that any RESTCONF username with the required
    permissions is allowed to invoke the "delete-subscription" RPC?  Or should this
    be restricted to same RESTCONF username?  In fact, I was wondering whether an
    administrator should be allowed to invoke modify-subscription or
    resync-subscription, or whether they should be restricted to kill-subscription
    only?
    
    6) Section 4 is unclear to me:
    
     - The paragraph starting "take any existing subscription "priority" ..."
     states that the "dscp" field is copied into the HTTP2 stream priority, but I
     couldn't find such a field in section 5.3 of RFC7540.  Hence, I was wondering
     whether this paragraph shouldn't instead be stating the the "weighting" field
     is copied into the HTTP2 stream priority header?
    
    E.g.
    OLD:
       o  take any existing subscription "priority", as specified by the
          "dscp" leaf node in
          [I-D.draft-ietf-netconf-subscribed-notifications], and copy it
          into the HTTP2 stream priority, [RFC7540] section 5.3, and
    
    NEW:
       o  take any existing subscription "priority", as specified by the
          "weighting" leaf node in
          [I-D.draft-ietf-netconf-subscribed-notifications], and copy it
          into the HTTP2 stream weight, [RFC7540] section 5.3, and
    
    There is also no mention of how the "E" (Exclusive) bit should be set in the
    HTTP2 stream headers.  Is this an omission?
    
    7) Section 6 doesn't contain any YANG Tree output.  Probably time to put that
    in, but I would suggest not putting in the entire tree output, just the
    relevant snippets.  If you want to include the full tree output then I would
    suggest putting in an appendix instead.  The text in this section describing
    the YANG model needs to be fixed since the YANG module doesn't actually define
    any identities.
    
    8) Section 7, YANG module comments
    
    Just nits:
    
    8.1) Check line length of the file, it looks like it goes over 69 characters.
     - In particular, the namespace statement might need to be split.
     - The module description text indentation looks off on the first line, and
     probably needs to be re-flowed. - Some of the other descriptions look wide.
    
    8.2) The description for "subscription-modified" doesn't quite scan (as it goes
    over the line break).
    
    9) One of the filter examples appendix A.3 is clearly over 72 characters,
    suggest spliting.
    
    Thanks,
    Rob