[yang-doctors] Yangdoctors last call review of draft-ietf-netconf-subscribed-notifications-10

[Andy Bierman <[e-mail]>]

Reviewer: Andy Bierman
Review result: Almost Ready


1.2 Terminology

   Notification message: A set of transport encapsulated information
   intended for a receiver indicating that one or more event(s) have
   occurred.  A notification message may bundle multiple event records.
   This includes the bundling of multiple, independent RFC 7950 YANG
   notifications.

  >> Cannot find any text that supports this claim; find the contrary:
    from 2.6:
       This notification
       message MUST be encoded as one-way notification element
       of [RFC5277]

1.3 Solution Overview

   o  Configured subscriptions can be modified by any configuration
      client with write permission on the configuration of the
      subscription.  Dynamic subscriptions can only be modified via an
      RPC request made by the original subscriber.

  >> what about a filter-ref that changes via another <edit-config>

1.4 Relationship to RFC 5277

   o  the one way operation of [RFC5277] is still used.  However this
      operation will no longer be required with the availability of
      [I.D.draft-ietf-netconf-notification-messages].

   >> But the new message format will be optional, so this message format
      will remain mandatory-to-implement, right?
   >> Is this a normative reference to notification-messages?

  o  a publisher MAY implement both the data model and RPCs defined in
      [RFC5277] and this new document concurrently, in order to support
      old clients.  However the use of both alternatives on a single
      transport session is prohibited.

   >> this constraint is not mentioned in the YANG module; expect text
     in establish-subscription:
       "This operation MUST fail if the session is already being
        used for a RFC 5277 subscription via <create-subscription>".


2.1: Event Streams

   It is out of the scope of this document
   to identify a) how streams are defined, b) how event records are
   defined/generated, and c) how event records are assigned to streams.

   >> Not really true for the NETCONF stream

   YANG notifications
   >> term not defined

   Beyond the NETCONF stream, implementations are free to add additional
   event streams.

   >> please reword so it is clear (a) a server MUST support the NETCONF
      stream and (b) a server MAY add additional streams

   If access control permissions are in use to secure publisher content,
   then for event records to be sent to a receiver, that receiver MUST
   be allowed access to all the event records on the stream.  If
   subscriber permissions change during the lifecycle of a subscription
   and stream access is no longer permitted, then the subscription MUST
   be terminated.

   >> known issues with 5277 compatibility (this text will be changed)

2.2 Event Stream Filters

   This document defines an extensible filtering mechanism.  Two
   optional stream filtering syntaxes supported are [XPATH] and subtree
   [RFC6241].  A filter always removes a complete event record; a subset
   of information is never stripped from an event record.

   >> s/Two/The two/
   >> Suggest mention a filter is a boolean test on the content of an
      event message. A 'false' result causes the event message to
      be excluded from delivery to a receiver.


2.3 QoS

   o  a "dependency" upon another subscription.  Notification messages
      MUST NOT be sent prior to other notification messages containing
      update record(s) for the referenced subscription.

  >> this is unclear, and perhaps belongs in the YANG Push draft
     instead of this draft
  >> suggest moving the normative (MUST NOT) text into sentences instead
     of bullet 3

  A subscription's weighting MUST work identically
  A subscription's dependency MUST work identically...

  >>> this wording is not that clear. Can it be reworded
  to say a server implementation MUST...

2.4 Dynamic Subscription

  Dynamic subscriptions are managed via RPC
  >> should really be protocol operations, not RPC

   These RPCs have been designed
   extensibly so that they may be augmented for subscription targets
   beyond event streams.

   >> not sure what this means.  Can't any YANG module be augmented?
   >> Do not agree text elsewhere about streams allows for extensions
      that ignore streams.  An augment cannot remove the 'identifier'
      leaf.

2.4.1.  Dynamic Subscription State Model

  >> this figure should really be named 'Figure 1' (so other
     figures will need to renumber)

  o  A delete or kill RPC will end the subscription.

  >> write out full names for <delete-subscription> or
     <kill-subscription> operation
  >> what about <edit-config> "delete" operation for configured
     subscriptions?

2.4.2.  Establishing a Subscription

   And it MUST be possible to support the
   interleaving of RPC requests made on independent subscriptions.

   >> should not be separate sentence;
   >> term "independent subscriptions" is not defined or clear

2.4.2.1.  Replay Subscription

   If the "replay-start-
   time" contains a value that is earlier than content stored within the
   publisher's replay buffer, then the subscription MUST be rejected,
   and the leaf "replay-start-time-hint" MUST be set in the reply.

   >> this is a significant and bad change from RFC 5277 behavior
   >> the start-time says "send all events that you have stored
      since this time" The server sends its oldest event and does
      not reject the request.  This draft incorrectly interprets
      the request as "the server MUST have an event stored at least
      this old"

   If a "stop-time" parameter is included, it MAY also be earlier than
   the current time and MUST be later than the "replay-start-time".  The
   publisher MUST NOT accept a "replay-start-time" for a future time.

   >> MUST be later (if the start-time) if supplied
   >> MAY be before current time?  Inconsistent with start-time
      MUST have events that exist
   >> MUST NOT accept future start-time different than 5277, but OK
      because that was a bad requirement


2.4.3.  Modifying a Subscription

   Dynamic subscriptions established via RPC can only be modified via
   RPC using the same transport session used to establish that
   subscription.  Subscriptions created by configuration operations
   cannot be modified via this RPC.

   >> should say <modify-subscription> only accepted for the
      session that issued <establish-subscription>
   >> <edit-config> by anybody can modify a filter used by filter-ref

   If the publisher accepts the requested modifications on a currently
   suspended subscription, the subscription will immediately be resumed
   (i.e., the modified subscription is returned to an active state.)
   The publisher MAY immediately suspend this newly modified
   subscription through the "subscription-suspended" notification before
   any event records are sent.

   >> Not sure why this is useful to go to active and immediately
      back to suspended.
   >> text in YANG module not the same:
        A successful modify-subscription
       will return a suspended subscription to an active state.


2.4.4.  Deleting a Subscription
2.4.5.  Killing a Subscription

  >> Do not see any need for 2 RPC operations

   The tree structure of "kill-subscription" is almost identical to
   "delete-subscription", with only the name of the RPC and yang-data
   changing.

   >> just include the tree diagram and remove this text

2.4.6.  RPC Failures

  RPC error codes
  >> this term is not used in NETCONF

   RPC error codes returned
   include both existing transport layer RPC error codes, such as those
   seen with NETCONF in [RFC6241] Appendix A, as well as subscription
   specific errors such as those defined within this document's YANG
   model.  As a result of this mixture, how subscription errors are
   encoded within an RPC error response is transport dependent.

   >> Not sure this text is correct...
      NETCONF and RESTCONF define transport-independent error handling.
      Do not see any reason a YANG data model should define any new
      error handling for these protocols. Existing <error-app-tag>
      and <error-info> extensions should be sufficient.

   There are elements of the RPC error mechanism which are transport
   independent.  Specifically, references to specific identities within
   the YANG model MUST be returned as part of the error responses

   >> conflicts with previous text:
      The contents of the resulting RPC error response MAY
      include one or more hints

   >> this section is very unclear wrt to specific standard error fields
      such as <error-tag>, <error-app-tag>, and <error-info>;

2.5.  Configured Subscriptions

   o  Optional parameters to identify an egress interface, a host IP
      address, a VRF (as defined by the network instance name within
      [I-D.draft-ietf-rtgwg-ni-model]), or an IP address plus VRF out of
      which notification messages are to be pushed from the publisher.
      Where any of this info is not explicitly included, or where just
      the VRF is provided, notification messages MUST egress the
      publisher's default interface towards that receiver.

     >> this seems like the wrong place to put normative text about
        VRFs. Is this mandatory-to-implement for all servers?

2.5.1.  Configured Subscription State Model

     State model for a configured subscription.
     >> no Figure number

       Second, the
   publisher itself might itself determine that the subscription in no
   longer supportable.  In either case, a "subscription-terminated"

   >> s/might itself determine that the subscription in no/
      might determine that the subscription is no

   A configured subscription's receiver MUST be moved to a suspended
   state if there is transport connectivity between the publisher and
   receiver, but notification messages are not being generated for that
   receiver.  A configured subscription receiver MUST be returned to an
   active state from the suspended state when notification messages are
   again being generated and a receiver has successfully been sent a
   "subscription-resumed" or a "subscription-modified".

  >> This seems very implementation-specific and unclear why it provides
     any value.  Supposed a subscription is activated for some fault events
     that should rarely be generated.
  >> not sure why terms like CONNECTING are all-caps in the diagram if
     if term is lowercase in the text.

2.5.2.  Creating a Configured Subscription

       In this case, when there is
   something to transport for an active subscription, transport specific
   call-home operations will be used to establish the connection.  When

   >> is this normative or is callhome optional-to-implement?

   With active configured subscriptions, it is allowable to buffer event
   records even after a "subscription-started" has been sent.  However
   if events are lost (rather than just delayed) due to replay buffer
   overflow, a new "subscription-started" must be sent.  This new
   "subscription-started" indicates an event record discontinuity.

  >> this is confusing to send multiple "subscription-started" events.

   To see an example at subscription creation using configuration
   operations over NETCONF, see Appendix A of
   [I-D.draft-ietf-netconf-netconf-event-notifications].

   >> IMO the examples should be moved to this draft

2.5.3.  Modifying a Configured Subscription

       If a receiver is removed, the
   state change notification "subscription-terminated" is sent to that
   receiver if that receiver is "active" or "suspended" .

   >> how are events sent to suspended receivers? They have to go through
     the state machine from CONNECTING -> ACTIVE (or TIMEOUT) first?
     Seems there should be no requirement to ever send an event
     to a suspended receiver. Should be OK to terminate receiver and drop
     the transport session

2.5.4.  Deleting a Configured Subscription

   Immediately after a subscription is successfully deleted, the
   publisher sends to all receivers of that subscription a state change
   notification stating the subscription has ended (i.e., "subscription-
   terminated").

   >> should only be for active receivers. For suspended receivers,
      just drop the transport session

2.5.5.  Resetting a Configured Receiver

   It is possible that a configured subscription to a receiver needs to
   be reset.  This re-initialization may be useful in cases where a
   publisher has timed out trying to reach a receiver.  When such a
   reset occurs, a transport session will be initiated if necessary, and
   a new "subscription-started" notification will be sent.

   >> this section does not define how a configuration is "reset".
      There is no such protocol operation in NETCONF or RESTCONF.
      Should mention the "reset" action in the YANG module here


2.6.  Event Record Delivery

  s/RPC operations/protocol operations/

      In all
   cases, a single transport session MUST be able to support the
   interleaving of event records, RPCs, and state change notifications
   from independent subscriptions.

   >> this is a significant change from RFC 5277, and should be noted
      in sec 1.4
   >> this text is confusing. The receiver would never get <rpc>,
      just <notification> (for configured subscriptions). The
      <rpc-reply> message (dynamic subscriptions only) is only
      sent to 1 receiver (the client that sent <rpc>)
  >> why distinguish between event records and state change notifications?
     seems the receiver has to process every <notification> sent to it

   A notification message is sent to a receiver when an event record is
   able to traverse the specified filter criteria.  This notification

   >> traverse is odd terminology; expect "when the event record
      is selected by...

      A subscription's events MUST NOT be sent to a receiver
   until after a corresponding RPC response or state-change notification
   has been passed receiver indicating that events should be expected.

   >> sentence is mangled; even after fixing that, not clear
     this text is needed because already stated elsewhere.
   >> Think you mean that <subscription-modified> is delivered
     to receivers immediately after the last event before modification
     (ie. event placed in FILO queue) and before any events
     after modification

2.7.  Subscription State Notifications

   >> it should be clear if state change events are saved in the replay
     buffer; IMO they should be included

2.7.1.  subscription-started
2.7.2.  subscription-modified

  >> what is the value of returning all the input or configuration
    parameters in these notifications?  For a dynamic subscription,
    the only receiver just sent that info and does not need it.
    For a configured subscription, that data can be read from
    the configuration datastore.
  >> the subscription-modified event can be sent if the underlying
     filter of a filter-ref changes; there will not appear to
     be any parameter change at all;
  >> Suggest that filter-ref-change be an explicit flag within
     the subscription-modified event

2.7.3.  subscription-terminated

      Identities within the YANG model
   corresponding to such a loss include: "filter-unavailable", "no-such-
   subscription", and "stream-unavailable".

   >> text is confusing: do you mean these things were valid
     when the subscription was activated, but became invalid later?
   >> for configured subscriptions, seems more intuitive to reject
     <edit-config> that contains filter-ref to unknown filter
     (for example)

   Note: subscribers themselves can terminate existing subscriptions
   established via a "delete-subscription" RPC.  In such cases, no
   "subscription-terminated" state change notifications are sent.
   However if a "kill-subscription" RPC is sent, or some other event
   other than reaching the subscription's stop time results in the end
   of a subscription, then this state change notification MUST be sent.

   >> this text is confusing; seems to contradict text in sec. 2.5.1
   about sending "subscription-terminated"

2.7.4.  subscription-suspended

   The tree structure of "subscription-suspended" is almost identical to
   "subscription-terminated", with only the name of the notification
   changing.
   >> just include the tree diagram and remove this text

2.7.7.  replay-completed

   This notification indicates that all of the event records prior to
   the current time have been sent.  This includes new event records
   generated since the start of the subscription.  This notification
   MUST NOT be sent for any other reason.

   >> this text (prior to the current time) is confusing
     The RFC 5277 definition was more clear and different behavior.
     The replay-completed is sent before an event with a timestamp
     later than (1) stop-time or (2) subscription start time.
     So this event is before the "new event records", not after.

   The tree structure of "replay-completed" is almost identical to
   "subscription-resumed", with only the name of the notification
   changing.
   >> include tree diagram and remove this text

2.9.  Advertisement

  >> this section mentions some YANG features but not all

3.3.  Subscriptions Container

  +--ro subscriptions
     +--ro subscription* [identifier]
        +--ro identifier                       subscription-id

  >> tree diagram shows read-only but data nodes are config=true

4.  Data Model

4A) message encoding

  feature encode-json {
    description
      "This feature indicates that JSON encoding of notification
       messages is supported.";
  }

  feature encode-xml {
    description
      "This feature indicates that XML encoding of notification
       messages is supported.";
  }

  identity encodings {
    description
      "Base identity to represent data encodings";
  }

  identity encode-xml {
    base encodings;
    if-feature "encode-xml";
    description
      "Encode data using XML";
  }

  identity encode-json {
    base encodings;
    if-feature "encode-json";
    description
      "Encode data using JSON";
  }

  typedef encoding {
    type identityref {
      base encodings;
    }
    description
      "Specifies a data encoding, e.g. for a data subscription.";
  }

    leaf encoding {
      type encoding;
      mandatory true;
      description
        "The type of encoding for the subscribed data.";
    }

  >> IMO all YANG definitions related to message encoding should
     be removed because they are in conflict with existing protocols.
     NETCONF defines XML encoding. HTTP already defines
     media type handling for message encoding (Accept, Content-Type)
     There is no definition how to use JSON with NETCONF.

 4B) extension

  extension subscription-state-notification {
    description
      "This statement applies only to notifications. It indicates that
       the notification is a subscription state notification. Therefore
       it does not participate in a regular event stream and does not
       need to be specifically subscribed to in order to be received.
       This statement can only occur as a substatement to the YANG
       'notification' statement.";
  }

  >> not really clear what it means for other modules to use this
     extension; not clear how a client knows about other event types
     sent as subscription state, but not defined in this document

  >> All tool implementations MAY ignore any extension, which
     includes a notification receiver; implies a receiver MUST
     accept (and discard) event types that it does not expect.

4C) identities for error handling

  >> need to specify how these identities are used
     with <rpc-error> fields.

  >> it needs to be clear that protocol and YANG error handling will
     be processed before any application-level error handling.
     Any YANG validation errors will cause the initial <edit-config>
     to fail. In this case, error responses will conform to the
     protocol or YANG error handling procedures.


4D) references

        "RFC-7540, section 5.3.1";

     >> the NETMOD WG decided all references need to have the document
        title included

4E) dependency leaf

    leaf dependency {
      if-feature "qos";
      type subscription-id;
      description
        "Provides the Subscription ID of a parent subscription which
         has absolute priority should that parent have push updates
         ready to egress the publisher. In other words, there should be
         no streaming of objects from the current subscription if
         the parent has something ready to push.";

   >> it is unclear how message delivery on individual subscriptions
      is supposed to be implemented to meet the requirements in the
      description-stmt.  The purpose of this leaf is not clear either.
      A tree of subscriptions is not described anywhere.
      No examples are provided which show how parent and dependent
      subscriptions are used together

4F) groupings

   >> these grouping do not look reusable outside this module;
      they make the module extra difficult to read, especially with
      extensive use of 'refine' and 'augment' statements. An example
      is "notification-origin-info" and "receiver-info".

4G) stop-time

    leaf stop-time {
      type yang:date-and-time;
      description
        "Identifies a time after which notification messages for a
        subscription should not be sent.  If stop-time is not present,
        the notification messages will continue until the subscription
        is terminated.  If replay-start-time exists, stop-time must be
        for a subsequent time. If replay-start-time doesn't exist,
        stop-time must be for a future time.";
    }

    >> sec 2.4.2.1 says:
       If a "stop-time" parameter is included, it MAY also be earlier than
       the current time and MUST be later than the "replay-start-time".
    >> this conflicts with:
       If replay-start-time doesn't exist, stop-time must be for a future
       time.
    >> what does "future time" mean for configured subscriptions?
       Seems really broken to configure a stop-time, which is an
       absolute data/time, not relative to any other point in time

4H) protocol + receivers

   >> receiver list contains an address and port
      assume the "protocol" field applies to all receivers
      type transport = (netconf, http2, http1,1)
      *** Do not see how this is interoperable
      *** No detailed procedures defined to establish the transport
          session for any of these protocols
      *** Not clear how callhome is used, eg. which one (SSH or TLS)
      *** Not clear if NETCONF 1.0 or NETCONF 1.1 is used
   >> there does not seem to be any mandatory-to-implement transport
      so nothing a client can rely on

4I) lack of mandatory filtering

      anydata stream-subtree-filter {
        if-feature "subtree";

   >> The subtree filtering was mandatory-to-implement in RFC 5277
       This should be mandatory in this module.
       The feature "subtree" should be removed
   >> If not done, then list this difference in sec. 1.4

4J) source-interface

  >> this object is mandatory-to-implement for configured subscriptions
     Not clear that all servers have capability to select a
     source-interface from the application level.  Suggest a feature
     for this data node so it is optional

4K) modify-subscription/subscription-id

      leaf identifier {
        type subscription-id;
        description
          "Identifier to use for this subscription.";
      }
   >> should have "mandatory true"
   >> description should say "subscription to modify"
      looks like establish-subscription cut-and-paste

4L) <edit-config> delete on /subscriptions or /subscriptions/subscription

  >> no mention of any requirements to send a <subscription-terminated>
  if a configured subscription is deleted by a client

4M) dependency leaf

  >> what happens if the subscription-id is invalid at configure time?
     Is the edit-config rejected?
  >> what happens to this subscription if the referenced
     subscription is deleted?
  >> what happens if there are loops configured
     (e.g, dependency.1 = 2; dependency.2 = 3; dependency.3 = 1)

4N) reset action + state=connecting

              enum connecting {
                value 3;
                if-feature "configured";
                description
                  "A subscription has been configured, but a
                  subscription-started state change notification needs
                  to be successfully received before notification
                  messages are sent.";
              }

  >> The text for connecting state does not account for the <reset>
     action.
  >> what happens to any notifications pending for the receiver if
     the reset action is invoked?
  >> the subscription-started is not a receiver-specific event.
      A new event called <receiver-reset> should be sent instead
      of <subscription-started> when this action is invoked
  >> what happens if the state=suspended?  The server is forced
     to activate again? And then go immediately from "connecting"
     to "suspended" state?

4O) reset action / output

            output {
              leaf time {
                type yang:date-and-time;
                mandatory true;
                description
                  "Time a publisher returned the receiver to a
                  connecting state.";
              }

  >> I do not see what value this output parameter has to the client
     The client will not get this info until the response is received,
     and then it knows the receiver has already been set to connecting
     state.  What problem does this output leaf solve?

4P) configured-subscription-state


          enum invalid {
            value 2;
            description
              "The subscription as a whole is unsupportable with its
              current parameters.";
          }

  >> it is not clear which specific conditions will cause the edit
     to be accepted, but immediately put a configured subscription
     into "invalid" state

  >> this draft should explain in detail which conditions can occur
     after the subscription was initially configured that will
     cause this state variable to be set to "invalid"