Last Call Review of draft-ietf-netconf-netconf-event-notifications-17
review-ietf-netconf-netconf-event-notifications-17-rtgdir-lc-dhody-2019-04-29-00

Hello,

I have been selected as the Routing Directorate reviewer for this draft. The
Routing Directorate seeks to review all routing or routing-related drafts as
they pass through IETF last call and IESG review, and sometimes on special
request. The purpose of the review is to provide assistance to the Routing
ADs. For more information about the Routing Directorate, please see
http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir

Although these comments are primarily for the use of the Routing ADs, it would
be helpful if you could consider them along with any other IETF Last Call
comments that you receive, and strive to resolve them through discussion or by
updating the draft.

Document: draft-ietf-netconf-netconf-event-notifications-17
Reviewer: Dhruv Dhody
Review Date: 2019-04-29
IETF LC End Date: 2019-04-12
Intended Status: Standards Track

Summary:
--------
I have some minor concerns about this document that I think should be resolved
before publication.

Comments:
---------
This document provides a binding for events streamed over the NETCONF for
dynamic subscriptions. This is a companion document to draft-ietf-netconf-
subscribed-notifications and this capability for RESTCONF is defined in
draft-ietf-netconf-restconf-notif.

The document is overall well written, it makes an assumption that the reader
is well versed in this area and thus sparse in providing details in the
Introduction section. The appendix provides good examples.

I don't see any Routing Yang model specific issue.

Major Issues:
-------------
Note - An IETF process issue, but worth handling right away.

Section 11 says -

11.  Notes to the RFC Editor

   This section can be removed by the RFC editor after the requests have
   been performed.

It further says -

   RFC 6241 needs to be updated based on the needs of this draft.
   RFC-6241 section 1.2 bullet "(2)" targets RFC-5277 (actually it
   identifies RFC 5717, but that was an error fixed after RFC
   publication).  Anyway the current phrasing in RFC-5277 says that a
   notification message can only be sent after a successful "create-
   subscription".  Therefore the reference text must be modified to also
   allow notification messages be sent after a successful "establish-
   subscription".  Proposed text for bullet (2) of RFC-6241 would be:

     (2)  The Messages layer provides a simple, transport-independent
          framing mechanism for encoding RPCs and notifications.
          Section 4 documents the RPC messages, [RFC5277] documents
          Notifications sent as a result of a <create-subscription> RPC,
          and [RFC xxxx] documents Notifications sent as a result of
          an <establish-subscription> RPC.

      (where xxxx is replaced with this RFC number)

I am not sure if this is correct. I don't think RFC editor can do the action
you are asking them to do on their own. They would need an errata (which is
not correct here) or another document that updates RFC 6241. In my view this
document should just update RFC 6241 (and mark that in this document's
header) and do necessary text changes to reflect that.

Minor Issues:
-------------
(1) Abstract & Introduction, It is not clear what does the 'binding' mean and
who are the parties to this binding? If this is the document that mentions
'binding' first, so please add some more clarifying text.

(2) Section 3, since you use MUST in the error handling, isn't it better to
use normative in below sentence as well -
OLD:
                                                       However a single
   NETCONF transport session cannot support both this specification and
   a subscription established by [RFC5277]'s "create-subscription" RPC.
NEW:
                                                       However a single
   NETCONF transport session MUST NOT support both this specification and
   a subscription established by [RFC5277]'s "create-subscription" RPC.


(3) Section 6, You have -

   And per [RFC5277]'s "eventTime" object definition, the
   "eventTime" MUST be populated with the event occurrence time.

Is this a new requirement, or just re-stating RFC5277? RFC5277 says -

      eventTime

         The time the event was generated by the event source.  This
         parameter is of type dateTime and compliant to [RFC3339].
         Implementations must support time zones.

      Also contains notification-specific tagged content, if any.  With
      the exception of <eventTime>, the content of the notification is
      beyond the scope of this document.

Maybe remove MUST? If you are trying to refine the text from RFC5277, then
please re-word.


Nits:
-----
(1) Abstract

   RFC Editor note: please replace the four references to pre-RFC
   normative drafts with the actual assigned RFC numbers.

I see two drafts in the reference section. Why four?
Also, since those two are normative references, these would be published as a
cluster as a part of normal RFC editor processing right?

(2) Regarding NETCONF, the RFC editor says [1] -

NETCONF    - Network Configuration Protocol (NETCONF)
               [Not typically expanded in titles, but expand in abstract]

Please expand.

(3) s/[I-D.draft-ietf-netconf-subscribed-notifications]
     /[I-D.ietf-netconf-subscribed-notifications]

Just so that you have the same style of draft reference in the document. I get
that it would be replaced with a RFC number anyways :)

[1] https://www.rfc-editor.org/materials/abbrev.expansion.txt

Thanks!
Dhruv