Re: [netconf] WGLC: draft-ietf-netconf-notification-capabilities-11

[Balázs Lengyel <[e-mail]>]

-----Original Message-----
From: Rob Wilton (rwilton) <[e-mail]> 
Sent: 2020. március 9., hétfő 19:34
To: [e-mail]; Balázs Lengyel <[e-mail]>;
[e-mail]
Cc: Kent Watsen <[e-mail]>
Subject: RE: [netconf] WGLC: draft-ietf-netconf-notification-capabilities-11

Hi,

Thank you for this work.  I've reviewed this document as an individual
contributor and also as an early incoming AD review.


Overall comments:

I support the progression of this work, I think that it is both useful and
important, and the document is well written.  I like the split into two
separate YANG modules covering system capabilities vs YANG push
notifications.  I think that in an ideal world this work would probably be
split into two separate documents, one covering system capabilities, and the
second for notification capabilities.  In particular, I could imagine the
system capabilities evolving over time.  However, I also understand the
desire to complete this work, and I don't want to delay it.


Comments on individual sections:


1. Title:
 Perhaps it would be more accurate to name it as 
   "YANG Modules for describing System Capabilities and Yang-Push
Notification Capabilities?"
BALAZS: OK
 
2. Abstract
I propose that you rework the text to.
 
 OLD:
 
    This document proposes two YANG modules.  The module ietf-system-
   capabilities provides a structure that can be used to specify any
   YANG related system capability.

   The module ietf-notification-capabilities allows a publisher to
   specify capabilities related to "Subscription to YANG Datastores"
   (YANG-Push).  It proposes to use YANG Instance Data to document this
   information and make it already available at implementation-time, but
   also allow it to be reported at run-time.
   
 NEW:
 
   This document defines two YANG modules:
   
   The module ietf-system-capabilities provides a structure that can be
   used to specify YANG related system capabilities for servers.  The module
   can be used in conjunction with YANG Instance Data to make this
information
   available at implementation-time.  The module can also be used to report
   capability information from the server at run-time.

   The module ietf-notification-capabilities augments
ietf-system-capabilities
   to specify capabilities related to "Subscription to YANG Datastores"
   (YANG-Push). 
BALAZS: OK


3. Terminology:

- Please add a reference to "instance data" from
draft-lengyel-netmod-yang-instance-data
BALAZS: OK

- Add the sentence: "In addition, this document defines the following
terms:"
BALAZS: OK

- Perhaps change "On-change Notification Capability" -> "On-change
notification capability"?
- Move the definition of on-change notification capability last.
BALAZS: OK

- Implementation-time information and Run-time information.  I think that
these definitions should only be defined in terms of "server" and not
"publisher", because I think that these considerations apply to all system
capabilities, not only notifications.  Perhaps also reference YANG instance
data.
BALAZS: I like the original text more, but I will change it. The term server
is included in both definitions. It can be debated whether sending
notifications is a server activity, as it not immediately connected to a
request; hence I originally  had the "publisher" included too.

E.g.

OLD:

   Implementation-time information: Information about the publisher's or
   server's behavior that is made available during the implementation of
   the publisher/server, available from a source other then a running
   server.

   Run-time information: Information about the publisher's or server's
   behavior that is available from the running server via management
   protocols such as NETCONF [RFC6241] or RESTCONF [RFC8040].

NEW:

   Implementation-time information: Information about the server's
   behavior that is made available during the implementation of
   the server, available from a source other then a running server, such
   as YANG Instance Data [XXXX].

   Run-time information: Information about the server's behavior that is
   available from the running server via management protocols such as
   NETCONF [RFC6241] or RESTCONF [RFC8040].


   
4. Section 2:
Please state that the YANG modules are compliant to NMDA.
BALAZS: What does it mean that a read-only module is NMDA compliant? Please
explain. I would rather not add text that I do not understand.


5. YANG model:

- Does "minimum-dampening-period" only apply of "on-change-supported" is
enabled.  If so, it might be helpful for the description to state this.  I
would also put the "on-change-supported" leaf before the
"minimum-dampening-period" leaf.
BALAZS: The leaf has an if-feature "yp:on-change" substatement, which should
indicate it is relevant to on-change. I would like to avoid replicating too
much information from the Yang-Push RFC.
I will rearrange the order of leaves grouping periodic and on-change related
leaves.

- Does "minimum-dampening-period" only apply if dampening is requested, or
does that mean that dampening is always required?  The text may need to be
clarified.  Otherwise, I'm concerned about the on-change notification for
interface statistics ... that could generate a lot of notifications ;-)
BALAZS: The answer is not trivial. I can see logic in both cases:
a) if there is a minimum-dampening-period declared  here, that means that
dampening MUST always be used. Logical because if the publisher can not
support a smaller than 5 sec dampening period, it is probably incapable to
support on-change without dampening.
b) the publisher MAY support on-change without dampening, but if dampening
is requested the period MUST NOT be smaller than the
minimum-dampening-period

- For both "minimum-dampening-period" and on-change-supported", can probably
remove "data store or" from the description.
BALAZS: OK

- Suggest moving "supported-excluded-change-type" above
periodic-notifications-supported, to keep all the on-change options
together.  I'm also not particularly keen on the negative enumeration
values, having them as positive values would seem more normal to me.
BALAZS: I will rearrange the order of leaves grouping periodic and on-change
related leaves.
I used negative numbers as the type's union also includes type
yp:change-type which is an enumeration with implicit value assignment. Using
negative numbers is a way to avoid clashing with these implicitly assigned
numbers.

6. Section 6 (Security):

Is there a risk that the capabilities information, if available offline and
also from a device, could be used to fingerprint the device and gain
information about what version of software/hardware is being run?
BALAZS: I t could be used as part of the fingerprinting, as any readable
data that changes infrequently. IMHO this is a generic problem, to be solved
by proper access control rules. Adding it as a kind of standard boiler text
to every YANG module would not help. Maybe we would need an OAM security
RFC. 


7. Could you please add a brief section to thank folks who have contributed
and reviewed this work.
- I think that it easier to get reviews, if the time/effort spent in the
review is acknowledged.
BALAZS: I usually do it. It seems I forgot it now.


8. Appendix A:
- Your examples look like they probably have some introduced line wrapping
(e.g. for the description, node-selector, and others).  Perhaps we should be
using the artwork draft for this, or alternatively add a comment that
linespaces have been introduced in various fields to improve readability.
BALAZS: I would prefer to wait till  draft-ietf-netmod-artwork-folding
becomes a full RFC. Even creating/using the folding header without the RFC
number is problematic.

- Perhaps:

OLD:

 The following example is instance-data describing the notification
   capabilities of a hypothetical "acme-switch" ...
   
NEW:

 The following instance-data example describes the notification
   capabilities of a hypothetical "acme-switch" ...
BALAZS: OK

- First example:
  "        </datastore-capabilities>" should be outdented at two spaces
after first block.
BALAZS: OK

- I would normally expect candidate/running to be listed before operational,
although there is obviously no harm in having it the other way.

 
9. Appendix B:
 - Please add a note to ask the RFC Editor to remove this section on
publication.
BALAZS: OK
 
Nits:
 - Please check for full stops (e.g. terminology)
BALAZS: OK, I corrected what I found. If you see more, please notify me.

Thanks,
Rob


> -----Original Message-----
> From: netconf <[e-mail]> On Behalf Of Kent Watsen
> Sent: 04 March 2020 21:54
> To: [e-mail]
> Subject: Re: [netconf] WGLC: 
> draft-ietf-netconf-notification-capabilities-
> 11
> 
> Dear All,
> 
> This WGLC has received zero responses, which is insufficient to 
> progress the document at this time.  The chairs have therefore decided 
> to extend the WGLC for another two weeks, now ending March 18th.
> 
> Again, positive comments, e.g., "I've reviewed this document and 
> believe it is ready for publication", are welcomed.  This is useful 
> and important, even from authors.  Objections, concerns, and 
> suggestions are also welcomed at this time.
> 
> The NETCONF Chairs
> 
> 
> 
> 
> > On Feb 17, 2020, at 8:27 PM, Kent Watsen <[e-mail]> wrote:
> >
> > FWIW, this is WGLC #2, after the major updates resulting from WGLC 
> > #1
> (in October).
> >
> > K.
> >
> >
> >> On Feb 17, 2020, at 7:47 PM, Kent Watsen <[e-mail]> wrote:
> >>
> >> This message begins a two-week WGLC for draft-ietf-netconf-
> notification-capabilities-11 ending on March 2nd (a week before the 
> IETF
> 107 submission cutoff deadline).  Here is a direct link to the HTML 
> version of the draft:
> >>
> >> 	https://tools.ietf.org/html/draft-ietf-netconf-notification-
> capabilities-11
> >>
> >> Positive comments, e.g., "I've reviewed this document and believe 
> >> it is
> ready for publication", are welcome!  This is useful and important, 
> even from authors.  Objections, concerns, and suggestions are also 
> welcomed at this time.
> >>
> >> Thank you,
> >> NETCONF Chairs
> >> _______________________________________________
> >> netconf mailing list
> >> [e-mail]
> >> https://www.ietf.org/mailman/listinfo/netconf
> >
> > _______________________________________________
> > netconf mailing list
> > [e-mail]
> > https://www.ietf.org/mailman/listinfo/netconf
> 
> _______________________________________________
> netconf mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/netconf