Extensible YANG Model for YANG-Push Notifications
draft-ietf-netconf-notif-envelope-02

Disclaimer: This may not be a pure YANG doctor review as I am really
reviewing the entire document and not just the YANG part. I did not
follow the WG discussions that resulted in this ID, so forgive me if I
raise issues already beaten to death by the WG.

* Abstract

  Why is this new extensible notification structure limited to YANG
  push? Why did the WG not fix the notification envelope in general?
  The answer to that question likely does not belong into the abstract
  but I wonder why the NETCONF WG creates a point solution for some
  notifications but not all notifications.

  The introduction has text like "provides an extensible YANG
  notification header allowing implementors and IETF contributors to
  easily add new notification metadata to the notification message"
  which does not seem to be YANG push specific.

* Introduction

  Is a hostname a good choice to scope sequence numbers? Should this
  not be a more flexible identifier that allows multiple notification
  senders on a host without having to coordinate sequence number usage
  if multiple notification senders co-exist? Should there be an
  optional second leaf to provide disambiguation in case hostname is
  not sufficient?

* Relationship

  This text explains that this solution is YANG push specific but it
  does not explain why this solution is restricted to YANG push
  notifications and not a general replacement of the RFC 5277
  format. I assume there has been a long discussion that resulted in
  the current text but would it not be nice to have a plan how to
  converge on a single YANG-defined notification envelope? Or to
  capture the arguments why the WG believes a solution for just YANG
  push notifications is a good idea.

  The forward references to section four caused me to jump to section
  four in the hope fo find additional valuable information but I was
  kind of disappointed by what I found as section four just says "the
  operator should deal with different notification envelopes". I later
  found relevant text in section three indicating that enabling the
  new format replaces the old notifications for all push
  notifications. This is not said in the section four where it would
  have helped me to understand things. As a first time reader, it took
  a while to put the bits and pieces together.

  Has it been considered to define features in order to negotiate
  whether the new notification format should be used during the
  capabilities exchange?

  Again, I wonder why this is a YANG push specific solution for a
  problem that is much bigger. The hard coded notification envelope
  was likely a mistake and the NETCONF WG should in my view aim to fix
  it for all notifications and not just some.

* Solution

  "A notification is created by defining a 'notification' statement in
  the YANG module." I suggest "A notification is defined by a
  'notification' statement in a YANG module." since YANG defines
  notifications, it does not create them.
  
  "When a NETCONF server sends this notification" -> "When a NETCONF
  server sends a notification"

  Be careful with a reference to RFC 3339 without acknowledging the
  update RFC 9557. The IESG is likely picky on this. If you use
  date-and-time, the latest ietf-yang-types update handles things for
  you, sometimes saying less is more.

  I have not verified the example encodings, they appear plausible.

  Is hostname of type inet:host the right choice? What about multiple
  notification sources on the same host? If sequence numbers are
  scoped by the hostname, this requires coordination of notification
  senders at the host level (or different host identities for multiple
  notification senders on the same host). I also wonder why this
  specification refers to 'sysName' inet:host - is the definition of
  the host type in RFC 6991 not sufficient? Why create a dependency on
  RFC 1213?

* YANG Modules

  How does the RFC editor know what RFCXXXX and section Y resolves to?
  They may guess RFCXXXX but section Y may be a challenge for them.

  In the description of event-time, why do you state that this is of
  type dateTime and compliant to [RFC3339]? The definition of the type
  yang:date-and-time and handles RFC 3339 and has text covering RFC
  3339 in its latest update.

  In the description of hostname, why do you refer to RFC 1213? The
  type you use is inet:host and the rules of this definition then
  apply.

  What is this reference? Is RFC XXXX not used for Extensible YANG
  Model for YANG-Push Notifications?

       reference
         "RFC XXXX: Extensible YANG Model for Network Telemetry Notifications";

  Well, before we had:

       reference
         "RFC XXXX: YANG-Push Notification Envelope";

  Later, we also find this:

       reference
         "RFC XXXX: Extensible YANG Model for YANG-Push Notifications";

  The YANG validation page lists some errors but they appear to be
  false positives in this case. Well, this one probably deserves to
  look into:

  libyang err : Augment extension target node "/iypn:envelope" from module "example-foo-extension" was not found. (/example-foo-extension:{extension='sx:augment-structure'}/{augment='/iypn:envelope'})

* Tl;DR

  This looks like a missed opportunity to fix the design of the
  NETCONF notification format. I would have preferred if this
  contribution was split into (i) a new notification envelope format
  for any YANG-defined notifications and an upgrade path towards the
  new notification envelope format and (ii) the YANG push extensions
  making use of it.

  On the technical side, I am concerned that 'hostname' is not
  sufficient to properly identify a sender having authority over
  sequence numbers.