Re: [netconf] YANG Push drafts comments

["Eric Voit (evoit)" <[e-mail]>]

Hi William,

Thanks for the interest, thoughts in-line...

From: netconf <[e-mail]> On Behalf Of William Lupton
Sent: Tuesday, January 29, 2019 6:41 AM
To: [e-mail]
Subject: [netconf] YANG Push drafts comments

All,

I'm sorry that the comments below are so late. I did ask the chairs what I should do with them, and they said to send them to the list despite the lateness from a process perspective.

The comments are based on the four WG consensus drafts as of late November. I haven't revisited them to see which may have been addressed by subsequent discussion.

Nearly all of the comments relate to the draft-ietf-netconf-subscribed-notifications draft, plus there are some general remarks, so it didn't seem worth splitting them into a message per draft.

Cheers,
William

----
1.      General:
1.      Why is the subscription id optional? Without it, will the receiver always be able to tell how to handle an event message?
I assume you are referring to the pushed notification messages carrying event records.  The RFC-5277 one way notification doesn’t support a subscription-id object in its current event schema, so we can’t embed this object for subscribed-notifications.  This is one reason that draft-ietf-netconf-notification-messages was adopted.  For YANG push, we do have a subscription-id in the notification definition, which is then carried within the RFC-5277 one-way notification.
2.      YANG Push is not referred to consistently across the document set (presence or absence of hyphen, "yang" capitalization, "push" capitalization). There should be a single, consistently-used term.
I think these was caught/covered with other reviews.

2.      draft-ietf-netconf-subscribed-notifications:
1.      I wonder why it was decided that dynamic subscriptions wouldn't allow the receiver to be different from the subscriber. It seems like two different concepts are being mixed: dynamic/configured subscription and embedded/external receiver.
Mostly we were attempting to make attempting a DoS attack with this infrastructure harder.  I.e., by forcing the dynamic subscription to also be the receiver, it is harder to attack a third party receiver with an overwhelming amount of subscribed traffic.   To accomplish a DoS with configured subscriptions, you need to have configuration credentials.  Beyond this, there are other difficulties distributed session management didn’t want to take on.   If someone wants to go down this path, we are happy to advise.

2.      Event occurrence time: This isn't quite clear. What if there was already a timestamp (from a lower-level system) when the event was identified? Should that be used?
Lower-level system times can be used.  There is nothing which says where on the publisher this time might come from, just that the time should be as close to the actual time as possible.

3.      Event record:
1.      So does the "event record" term apply to all the drafts? If so, does "event" apply to all the drafts? (This note is in the context of a later note about the fact that "event stream" appears to be specific to this draft.)
Subscribing to an event stream is different than a subscribing to a datastore.  For event streams, some publisher process has already placed an event record on a time ordered series, which is then filtered per-subscriber.  For datastore subscriptions there is no such time ordered series.  Instead you need a trigger (periodic or on-change, the name of the datastore, the name of the objects, etc.) to define the custom extract which becomes the time ordered series.   Below is a chart from a presentation on this.  I think it was in the material from IETF 100.
[cid:image002.png@01D4B7E7.B0019890]
So you can subscribe to either an event stream or a datastore.
2.      YANG notifications are explicitly defined in YANG models (e.g. the ietf-hardware YANG module defines a hardware-state-change notification). I thought that a major part of YANG Push is to effectively allow notifications to be defined dynamically.
1.      Later: I think perhaps the point is that event streams are indeed restricted to YANG notifications, but that other drafts (e.g. YANG Push) define mechanisms beyond event streams. Section 2.4 (dynamic subscriptions) says "These RPCs have been designed extensibly so that they may be augmented for subscription targets beyond event streams".
2.      Even later: Ah! YANG Push defines generic "push-update" and "push-change-update" notifications. So it's all OK.
4.      Event stream:
1.      Having now read most of the YANG Push draft, I realize that "Event streams" are a legacy thing and are not used by YANG Push. This could be made clear. Otherwise it looks like these are a base concept that will be built on.
I think event streams are useful.  I don’t consider them legacy.
2.      Perhaps the truly cleanest solution would be to move the legacy aspects to a separate document, or clearly indicate they're legacy, or use less "core" names, or move this material to an Annex or Appendix.
This was why we have a fully separate document for subscribed-notifications.  The WG requested that the authors retrofit the infrastructure documents in this way to uplevel what could be done with event streams.
3.      So referring to them as sets of Events (rather than sets of Event streams) seems inaccurate.
I don’t see any text on either sets of events, or sets of event streams.
5.      Notification message:
1.      I don't find "The time the event was generated by the event source" quite clear. Is it the time at which the change occurred or was detected (event occurrence time), or the time at which the message was created? Probably the former?
I can’t find the exact text quote:  "The time the event was generated by the event source" .   Where is this?
6.      Subscription:
1.      The section 2.2 description is very unclear. It appears that filters are on subscriptions, refer to records, and cause "event messages" to be excluded from delivery. But "event message" isn't a defined term.
I will refine “event message” into “event record” in the next document version release.
3.      draft-ietf-netconf-yang-push:
1.      YANG Push defines a new "datastore" subscription target that's independent of the existing "stream" subscription target. (Assuming that this is correct, I'd have liked some explanation of _why_ YANG Push isn't able to extend the base mechanism.)
It was recommended several years ago by members of the WG that subscribing to datastores and streams was sufficiently different, and that different targets were needed.  Some of the reasons are listed above.  Beyond what is listed above, another reason is that filters on a stream target exclude member of an already generated set of event records.  This is quite different from a datastore target, where the filters choose which nodes are of interest (i.e., include).

4.      Editorial (there might be some duplication with earlier comments):
1.      Some YANG descriptions have incorrect indentation on the second and subsequent lines (such lines should begin under the first character of the first line, i..e. one character to the right of the opening double quote). No doubt the YANG Doctors will catch such things!
2.      draft-ietf-netconf-subscribed-notifications-18
1.      Event stream definition: "Event stream: A continuous, chronologically ordered set of events" ... but should this be "...of event records"
It will certainly be implemented that way.  Early in the process, some people said that it is possible to have a pointer to the event records within the event stream.  So we left it with the more abstracted term ‘events’.
2.      s/intantiation/instantiation/ (two occurrences)
Fixed.
3.      The "subscription state change notifications" section name may have been subject to a s/Subscription/subscription/ edit? Should capitalise "subscription" both here and where it's used at the start of sentences (there's at least one case of this)
Fixed
3.      draft-ietf-netconf-yang-push-20
1.      There's a reference to "Custom Subscription to Event Streams". Is this the old title of "Subscription to YANG Event Notifications"? If so, it needs to be updated
2.      Spurious "6 record" in the YANG (should be "record")
3.      In the data model introductory section, the "sn:" prefix isn't explained (I know it's sort of obvious, but the "yp:" prefix (also obvious) _is_ explained)
<Eric> I will note these two Alex, who is hoping to post an update tonight.

Thanks!
Eric