Subject: [Netconf] mbj's WGLC review of yang-push-17

From: Martin Bjorklund <[email-masked]>
Date: Wed, 15 Aug 2018 13:17:58 +0200 (CEST)

Hi,

Kent Watsen <[email-masked]> wrote:
> This message starts a Last Call on draft-ietf-netconf-yang-push-17:

I have reviewed this document, and I think it is almost ready.  Here
are my comments:

o  3.3

  The text says:

   Putting it all together, following is the conceptual process for
   creating an push-change-update notification:

  Up until this point, the text has just talked about "update
  record".  Here it says "an push-change-update notification".

  Maybe simply s/an push-change-update notification/an update record/?

o  3.4

  The text says:

   the solution that is defined in this document mandates that a
   publisher notifies receivers immediately and reliably whenever it
   encounters a situation in which it is unable to keep the terms of the
   subscription, and provides the publisher with the option to suspend
   the subscription in such a case.

  I think it would help if you could put a forward reference to the
  mechanism that exists to do this immediate notification.

o  3.5.2

  The text describes incorrect usage of the "insert" operation; it is
  only applicable to user ordered lists.  Also, there is no reason for
  special handling of deletion of list entries.  (also use the term
  "list entry" rather than "element").  Hence, I suggest:

  OLD:

   A publisher will indicate a change to the effect that a value of a
   datstore node has been updated by indicating a "replace" operation
   (applied to the datastore node) in the patch.  When a new datastore
   node was created (other than an element in a list), a publisher will
   indicate a "create" operation in the patch.  When a datastore node
   was deleted (other than an element in a list), the publisher
   indicates this by a "delete".  When a new list element was created or
   removed, the publisher indicates it by an "insert" or "remove",
   respectively.

  NEW:

   A publisher will indicate a change to the effect that a value of a
   datstore node has been updated by indicating a "replace" operation
   (applied to the datastore node) in the patch.  When a new datastore
   node was created (other than an entry in a user ordered list), a
   publisher will indicate a "create" operation in the patch.  When a
   datastore node was deleted, the publisher indicates this by a
   "delete".  When a new entry in a user ordered list was created, the
   publisher indicates this by an "insert" operation.

o  3.5.2

  In the last paragraph, s/"merge"/"replace"/
  since the paragraph before just describes "replace", not "merge".

  Shouldn't the text also mention the operation "move" for user
  ordered lists?

o  3.6

  s/Xpath/XPath/

o  3.7

  The examples are not quite correct, and I suggest they are modified
  to not include deprecated nodes:

  OLD:

<notification xmlns="urn:ietf:params:xml:ns:netconf:notification:1.0">
 <eventTime>2017-10-25T08:00:11.22Z</eventTime>
 <push-update xmlns="urn:ietf:params:xml:ns:yang:ietf-yang-push">
   <subscription-id>1011</subscription-id>
   <datastore-contents>
     <interfaces-state xmlns="urn:ietf:params:xml:ns:yang:ietf-interfaces">
       <interface>
         <name>eth0</name>
         <oper-status>up</oper-status>
       </interface>
     </interfaces-state>
   </datastore-contents>
 </push-update>
</notification>

  NEW:

<notification xmlns="urn:ietf:params:xml:ns:netconf:notification:1.0">
 <eventTime>2017-10-25T08:00:11.22Z</eventTime>
 <push-update xmlns="urn:ietf:params:xml:ns:yang:ietf-yang-push">
   <subscription-id>1011</subscription-id>
   <datastore-contents>
     <interfaces xmlns="urn:ietf:params:xml:ns:yang:ietf-interfaces">
       <interface>
         <name>eth0</name>
         <oper-status>up</oper-status>
       </interface>
     </interfaces>
   </datastore-contents>
 </push-update>
</notification>

  OLD:

<notification xmlns="urn:ietf:params:xml:ns:netconf:notification:1.0">
 <eventTime>2017-10-25T08:22:33.44Z</eventTime>
 <push-change-update xmlns="urn:ietf:params:xml:ns:yang:ietf-yang-push">
   <subscription-id>89</subscription-id>
   <datastore-changes>
     <yang-patch xmlns="urn:ietf:params:xml:ns:yang:ietf-yang-patch">
       <patch-id>1</patch-id>
       <edit>
         <edit-id>edit1</edit-id>
         <operation>merge</operation>
         <target>/ietf-interfaces:interfaces-state</target>
         <value>
           <interfaces-state xmlns="http://foo.com/ietf-interfaces">
             <interface>
               <name>eth0</name>
               <oper-status>down</oper-status>
             </interface>
           </interfaces-state>
         </value>
       </edit>
     </yang-patch>
   </datastore-changes>
 </push-change-update>
</notification>

  NEW:

<notification xmlns="urn:ietf:params:xml:ns:netconf:notification:1.0">
 <eventTime>2017-10-25T08:22:33.44Z</eventTime>
 <push-change-update xmlns="urn:ietf:params:xml:ns:yang:ietf-yang-push">
   <subscription-id>89</subscription-id>
   <datastore-changes>
     <yang-patch xmlns="urn:ietf:params:xml:ns:yang:ietf-yang-patch">
       <patch-id>1</patch-id>
       <edit>
         <edit-id>edit1</edit-id>
         <operation>replace</operation>
         <target>/ietf-interfaces:interfaces</target>
         <value>
           <interfaces xmlns="urn:ietf:params:xml:ns:yang:ietf-interfaces">
             <interface>
               <name>eth0</name>
               <oper-status>down</oper-status>
             </interface>
           </interfaces>
         </value>
       </edit>
     </yang-patch>
   </datastore-changes>
 </push-change-update>
</notification>

  (uses the "replace" operation, fixed xml namespace, don't use
  interfaces-state)

o  3.8

  s/establish-subscription-datasore-error-info/
    establish-subscription-datastore-error-info/

o  3.8

  The text says:

   In the case of a rejected request for an establishment of a datastore
   subscription, the hints MUST be transported within a yang-data
   "establish-subscription-datastore-error-info" container

  Do you mean that *if* there are hints, they MUST be sent within this
  container, or you mean that on failure, this container MUST be sent?

  (ditto for modify-subscription-datastore-error-info)

  (it seems 4.4.1 says that hints SHOULD be included, but Appendix A
  that they MUST be included)

o  3.9

  s/RFC8342/RFC8341/
  s/rfc6536bis/RFC8341/

o  3.9

  It is difficult to relate Figure 5 to the text.   Should it be moved
  to right after the first paragraph?  I think it would be useful to
  add a reference from the text that examplains the "updated access
  control rules" to the figure.

  Also, s/update message/update record/ in the figure.

o  3.9

  The text says:

   A publisher MAY choose reject an establish-subscription request which
   selects non-existent or access-protected data.  In addition, a
   publisher MAY choose to terminate a dynamic subscription or suspend a
   configured receiver when the authorization privileges of a receiver
   change, or the access controls for subscribed objects change.  Such a
   capability enables the publisher to avoid having to support a
   continuous, and total filtering of an entire subscription's content.

   In these cases above, the error identity "unchanging-selection"
   SHOULD be returned.

  "the cases above" refers to (i) terminating a dynamic subscription,
  or (ii) suspend a configured receiver.   What does it mean to
  "return" an error identity when a subscription is terminated, or
  suspended?

  Maybe you meant that the error identity "unchanging-selection"
  SHOULD be sent in an "subscription-terminated" notification or
  "subscription-suspended" notification, respectively.

  If so, the "unchanging-selection" identity should probably also
  derive from "sn:subscription-suspended-reason".

o  3.11.1

  The text says:

   It is not
   required to merge pending update messages.

  This can be read as indicating that a server MAY merge pending
  update messages.  I assume that it should say that pending update
  messages MUST NOT be merged.

  Also, s/update message/update record/

o  4.2

  s/an "excluded-change" flag/an "excluded-change" parameter/

  (it is more than just a flag...)

o  4.3.2

  The second paragraph is a bit confusing.  I suggest to simplify:

  OLD:

   A "subscription-id" MUST be transported along with the subscribed
   contents.  An [RFC5277]  Section 4 one-way notification MAY be used
   for encoding updates.  Where it is, the relevant "subscription-id"
   MUST be encoded as the first element within each "push-update" or
   "push-change-update".  This allows a receiver to differentiate which
   subscription resulted in a particular push.

  NEW:

   A "subscription-id" is transported along with the subscribed
   contents.  This allows a receiver to differentiate which
   subscription resulted in a particular push.

o  4.4.1

  The examples are (still) wrong.

  OLD:

  <establish-subscription
       xmlns="urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications"
       xmlns:yp="urn:ietf:params:xml:ns:yang:ietf-yang-push">
    <yp:datastore>
      <yp:source xmlns:ds="urn:ietf:params:xml:ns:yang:ietf-datastores">
        ds:operational
      </yp:source>
      <xpath-filter
          xmlns:ex="http://example.com/sample-data/1.0"
          select="/ex:foo"/>
    </yp:datastore>
    <yp:periodic>
      <yp:period>500</yp:period>
    </yp:periodic>
  </establish-subscription>

  NEW:

  <establish-subscription
       xmlns="urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications"
       xmlns:yp="urn:ietf:params:xml:ns:yang:ietf-yang-push">
    <yp:datastore xmlns:ds="urn:ietf:params:xml:ns:yang:ietf-datastores">
      ds:operational
    </yp:datastore>
    <yp:datastore-xpath-filter xmlns:ex="http://example.com/sample-data/1.0">
        /ex:foo
    </yp:datastore-xpath-filter>
    <yp:periodic>
      <yp:period>500</yp:period>
    </yp:periodic>
  </establish-subscription>

  OLD:

  <rpc-reply message-id="101"
    xmlns="urn:ietf:params:xml:ns:netconf:base:1.0">
    <subscription-result
      xmlns="urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications">
       ok
    </subscription-result>
    <identifier
      xmlns="urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications">
       52
    </identifier>
  </rpc-reply>

  NEW:

  <rpc-reply message-id="101"
    xmlns="urn:ietf:params:xml:ns:netconf:base:1.0">
    <identifier
      xmlns="urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications">
       52
    </identifier>
  </rpc-reply>

  OLD:

   <netconf:rpc message-id="101"
     xmlns:netconf="urn:ietf:params:xml:ns:netconf:base:1.0">
     <establish-subscription
     xmlns="urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications"
     xmlns:yp="urn:ietf:params:xml:ns:yang:ietf-yang-push">
       <yp:datastore
       xmlns:ds="urn:ietf:params:xml:ns:yang:ietf-datastores">
         ds:operational
       </yp:datastore>
       <yp:datastore-xpath-filter netconf:type="xpath"
       xmlns:ex="http://example.com/sample-data/1.0">
         /ex:foo
       </yp:datastore-xpath-filter>
       <yp:on-change>
         <yp:dampening-period>100</yp:dampening-period>
       </yp:on-change>
     </establish-subscription>
   </netconf:rpc>

  NEW:

   <netconf:rpc message-id="101"
     xmlns:netconf="urn:ietf:params:xml:ns:netconf:base:1.0">
     <establish-subscription
     xmlns="urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications"
     xmlns:yp="urn:ietf:params:xml:ns:yang:ietf-yang-push">
       <yp:datastore
       xmlns:ds="urn:ietf:params:xml:ns:yang:ietf-datastores">
         ds:operational
       </yp:datastore>
       <yp:datastore-xpath-filter
           xmlns:ex="http://example.com/sample-data/1.0">
         /ex:foo
       </yp:datastore-xpath-filter>
       <yp:on-change>
         <yp:dampening-period>100</yp:dampening-period>
       </yp:on-change>
     <establish-subscription>
   </netconf:rpc>

o  4.4.1

  REMOVE:

   o  "error-app-tag" with the value being a string that corresponds to
      an identity with a base of "establish-subscription-error".

  (this app-tag thing was removed from subscribed-notifications)

  And modify the example accordingly:

  OLD:

<rpc-reply message-id="101"
  xmlns="urn:ietf:params:xml:ns:netconf:base:1.0">
  <rpc-error>
    <error-type>application</error-type>
    <error-tag>operation-failed</error-tag>
    <error-severity>error</error-severity>
    <error-app-tag>
        on-change-unsupported
    </error-message>
    <error-path
   xmlns:yp="urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications">
      /yp:periodic/yp:period
    </error-path>
  </rpc-error>
</rpc-reply>

  NEW:

<rpc-reply message-id="101"
    xmlns="urn:ietf:params:xml:ns:netconf:base:1.0">
    xmlns:yp="urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications">
  <rpc-error>
    <error-type>application</error-type>
    <error-tag>operation-failed</error-tag>
    <error-severity>error</error-severity>
    <error-path>/yp:periodic/yp:period</error-path>
    <error-info>
    <yp:establish-subscription-error-datastore>
      <yp:reason>yp:on-change-unsupported</yp:reason>
    </yp:establish-subscription-error-datastore>
  </rpc-error>
</rpc-reply>

o  4.4.2

  The example is wrong:

  OLD:

 <netconf:rpc message-id="102"
    xmlns:netconf="urn:ietf:params:xml:ns:netconf:base:1.0">
    <modify-subscription
    xmlns="urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications"
    xmlns:yp="urn:ietf:params:xml:ns:yang:ietf-yang-push">
     <identifier>1011</identifier>
     <yp:datastore
     xmlns:ds="urn:ietf:params:xml:ns:yang:ietf-datastores">
       ds:operational
     </yp:datastore>
     <yp:datastore-xpath-filter
     netconf:type="xpath" xmlns:ex="http://example.com/sample-data/1.0">
       /ex:bar
     </yp:datastore-xpath-filter>
     <yp:periodic>
       <yp:period>250</yp:period>
     </yp:periodic>
    </modify-subscription>
 </netconf:rpc>

  NEW:

 <netconf:rpc message-id="102"
    xmlns:netconf="urn:ietf:params:xml:ns:netconf:base:1.0">
    <modify-subscription
    xmlns="urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications"
    xmlns:yp="urn:ietf:params:xml:ns:yang:ietf-yang-push">
     <identifier>1011</identifier>
     <yp:datastore
     xmlns:ds="urn:ietf:params:xml:ns:yang:ietf-datastores">
       ds:operational
     </yp:datastore>
     <yp:datastore-xpath-filter
         xmlns:ex="http://example.com/sample-data/1.0">
       /ex:bar
     </yp:datastore-xpath-filter>
     <yp:periodic>
       <yp:period>250</yp:period>
     </yp:periodic>
    </modify-subscription>
 </netconf:rpc>

o  4.4.2

  REMOVE:

   o  "error-app-tag" with the value being a string that corresponds to
      an identity with a base of "modify-subscription-error".

o  4.4.5

  s/YANG 1.0/YANG 1/

  (the version is "1", not "1.0")

o  4.4.5

  I suggest you remove the sentence:

  The
   "/modules-state/module-set-id" leaf in the "ietf-yang-library" module
   can be used to cache the YANG library information.

  This is chnaged with yang-library-bis, and the sentence is not
  really needed in this draft.

o  4.4.5

  I don't understand what the third paragraph is supposed to tell me.
  Can it be removed?

o  5

  In subscribed-notifications, the subscription identifier leaf is
  called "identifier", in this model it is called "subscription-id"
  and "identifier".

  I think the two models should use the same term.  Either change this
  model, or subscribed notifications.  Remember to update the
  examples.

o  5

  Is it ok to do:

   <establish-subscription>
     <datastore>operational</datastore>
   </establish-subscription>

  Probably not, so I suggest making this illegal in the model:

  augment "/sn:establish-subscription/sn:input" {
    when "sn:target/yp:datastore";  // NEW statement

    description
      "This augmentation adds additional subscription parameters that
      apply specifically to datastore updates to RPC input.";
    uses update-policy;
  }

  and ditto for all of:

    augment "/sn:modify-subscription/sn:input" {
    augment "/sn:subscription-started" {
    augment "/sn:subscription-modified" {
    augment "/sn:subscriptions/sn:subscription" {

  and then modify update-policy-modifiable to make the update-trigger
  choice mandatory:

  grouping update-policy-modifiable {
    description
      "This grouping describes the datastore specific subscription
       conditions that can be changed during the lifetime of the
       subscription.";
    choice update-trigger {
      mandatory true;  // NEW

o  5

  I have made this comment before.  The anydata node
  "datastore-changes" should be a container that uses the grouping
  "yang-patch".  It is more precise than using anydata and in text
  explain that the opaque anydata must be yang patch.

o  5

   identity result-too-big {

   identity synchronization-size {

  Why do we need both these errors?  Can't we just have a single one,
  maybe "update-too-big"?

  (I think that *result-too-big* is a misnomer.  If the result of an
  rpc is too big, the standard error-tag "too-big" should be used.)

o  5

  Since 3.5.2 specifies that a subset of all operations from YANG
  patch to be used in push update records, shouldn't the typedef
  change-type only include this subset?

  Otherwise, why should a client be able to exclude "merge", when
  "merge" can never be included?

o  9.2

  The reference [bergstra2014] is not used and can be removed.

o  Comment from an earlier review:

  (the document also uses the term "data object" and "datastore
  object", these should be fixed)

  These should both be changed to "datastore node" or "object".

/martin

---

From: Alexander Clemm <[email-masked]>
Date: Tue, 28 Aug 2018 01:16:48 +0000

Hi Martin,

thank you for your comments.  

Please see my responses inline, <ALEX>

(Apologies for the sluggish response; I have been travelling)

--- Alex

> -----Original Message-----
> From: Netconf [mailto:[email-masked]] On Behalf Of Martin
> Bjorklund
> Sent: Wednesday, August 15, 2018 4:18 AM
> To: [email-masked]
> Subject: [Netconf] mbj's WGLC review of yang-push-17
> 
> Hi,
> 
> I have reviewed this document, and I think it is almost ready.  Here are my
> comments:
> 
> 
> o  3.3
> 
>   The text says:
> 

<ALEX> The process itself talk also about sending the record to the receiver.  How about refining it to say " following is the conceptual process for
   creating a update record as part of an on-change subscription:"?  
</ALEX>

>   mechanism that exists to do this immediate notification.

<ALEX> Adding the following forward reference: 
"This is described further in Section 3.11.1."  

>    publisher indicates this by an "insert" operation.

<ALEX> OK, changed. </ALEX>

>   since the paragraph before just describes "replace", not "merge".

<ALEX> done </ALEX>

>   Shouldn't the text also mention the operation "move" for user
<ALEX> Thank you.  Added the following text: " When an entry in a user-ordered list was moved, the publisher indicates this by a "move" operation.  "

>   s/Xpath/XPath/

<ALEX> done </ALEX>

>   (uses the "replace" operation, fixed xml namespace, don't use

<ALEX> Thank you.  Updated.  </ALEX>

>     establish-subscription-datastore-error-info/

<ALEX> Fixed, thank you </ALEX>

>   that they MUST be included)

<ALEX> Changing "MUST" to "SHOULD".  And yes, *if* there are hints.   I don't think a change to Appendix A, is needed; it does not  actually state that they must be included.  (It says the yang-data with the hint MUST be sent if hints are included.)  
</ALEX>

>   s/rfc6536bis/RFC8341/

<ALEX> Thank you, changed </ALEX>

>   control rules" to the figure.

<ALEX> I moved the paragraph preceding Figure 5, as well as Figure 5 itself, further to the top (behind the first paragraph).  </ALEX>

>   Also, s/update message/update record/ in the figure.

<ALEX> Done </ALEX>

>   derive from "sn:subscription-suspended-reason".

<ALEX> Changed this section as follows:
"A publisher MAY choose to reject an establish-subscription request which selects non-existent or access-protected data. In addition, a publisher MAY choose to terminate a dynamic subscription or suspend a configured receiver when the authorization privileges of a receiver change, or the access controls for subscribed objects change.  As reason, the error identity "unchanging-selection" SHOULD be returned. Such a capability enables the publisher to avoid having to support continuous and total filtering of a subscription's content for every update record.  It also reduces the possibility of leakage of access-controlled objects."
</ALEX>

<ALEX> On a separate note, the next paragraph states: "If read access into previously accessible nodes has been lost due to a receiver permissions change, this SHOULD be reported as a patch "delete" operation for on-change subscriptions. If not capable of handling such receiver permission changes with such a "delete", publisher implementations MUST force dynamic subscription re-establishment or configured subscription re-initialization so that appropriate filtering is installed."  

I am wondering if one should actually report the "delete"-operation here.   This is somewhat inaccurate, as the object may not have been actually deleted, only its access has been revoked.  I am leaving this as is for now, but am wondering if we should change this instead as follows:  
" If read access into previously accessible nodes has been lost due to a receiver permissions change, publisher implementations MUST force dynamic subscription re-establishment or configured subscription re-initialization so that appropriate filtering is installed."

Thoughts?
</ALEX>

>   messages MUST NOT be merged.

<ALEX> Hmm.  I am not sure I agree.  The server is not required to merge pending update messages - i.e. can send multiple messages each with a separate update record.  However, there is no reason to preclude that they could be combined.  So, I don't think an update is needed here.  
</ALEX>

>   Also, s/update message/update record/

<ALEX> Sure </ALEX>

>   (it is more than just a flag...)

<ALEX> Updated </ALEX>

>    subscription resulted in a particular push.

<ALEX> Updated </ALEX>

>          <yp:dampening-period>100</yp:dampening-period>

<ALEX> Updated </ALEX>

>     </yp:establish-subscription-error-datastore>

<ALEX> I think there are some mismatches in the NEW as indicated above (no error-info closing bracket, xmln:yp without container) - updated as follows (NEW2):

  xmlns="urn:ietf:params:xml:ns:netconf:base:1.0"
	</error-info>
  </rpc-error>
</rpc-reply>

</ALEX> 

>        <yp:period>250</yp:period>

<ALEX> Done </ALEX>
>       an identity with a base of "modify-subscription-error".

<ALEX> Done </ALEX>

>   (the version is "1", not "1.0")

<ALEX> Done </ALEX>

>   really needed in this draft.

<ALEX> Agreed, removed </ALEX>

>   I don't understand what the third paragraph is supposed to tell me.

<ALEX> Greatly simplified and rephrased it as follows:
" The set of modules, revisions, features, and deviations can change at run-time (if supported by the publisher implementation). For this purpose, the YANG library provides a simple "yang-library-change" notification that informs the subscriber that the library has changed.  In this case, a subscription may need to be updated to take the updates into account.  The receiver may also need to be informed of module changes in order to process updates regarding datastore nodes from changed modules correctly."
</ALEX>

>   model, or subscribed notifications.  Remember to update the

<ALEX>  I think what you refer to is the way that subscriptions are identified in push update notifications.  In all other places, we consistently use subscription-id as the type, and "identifier" to identify a subscription (YANG-Push does not introduce a new leaf, but imports yp:identifier).   

Personally I find the term "identifier" a bit generic.  That said, if you want to make it consistent, we should stick with what we have in subscribed notifications (to keep the impact low).  I am not really convinced it is a change for the better, but I am updating the model to use "identifier" in the push update notifications (push-update and push-change-update) to identify subscriptions.   Examples have been updated accordingly.  

</ALEX>

>        conditions that can be changed during the lifetime of the

<ALEX> I am not sure I understand this comment fully.  Can you please clarify?  You should be able to subscribe to updates to <operational>.   I don't think this should be precluded.  
I guess you are referring to the fact that you would like to make update-trigger mandatory?   Arguably, the server should always be able to reject a request it cannot fulfill, which may include underspecification.  

Let me add your suggested augmentation to the inputs for establish-subscription and modify-subscription.  I don't think this would be needed for a notification, which is generated by the server anyway.    Let me also add the mandatory statement to the grouping; however, I am not sure this is actually legal, as the grouping will be used in an augmentation.  (Can an augmentation contain a "mandatory" item?)  
</ALEX>

>   explain that the opaque anydata must be yang patch.

<ALEX> ok </ALEX>
>   rpc is too big, the standard error-tag "too-big" should be used.)

<ALEX> Renamed "result-too-big" to "update-too-big".  However, leaving the synchronization-size identity.  That one would be used for resynch only.  While strictly speaking it may be possible to eliminate, it probably doesn't hurt either.
</ALEX>

>   "merge" can never be included?

<ALEX> Not sure what you would like me to change?  remove "merge" from the enum?  
</ALEX>

>   The reference [bergstra2014] is not used and can be removed.

<ALEX> Removed
</ALEX>

>   These should both be changed to "datastore node" or "object".

<ALEX> Changed the last remaining instances
</ALEX>

> 
> 
> /martin
> 
> _______________________________________________
> Netconf mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/netconf

---

From: Martin Bjorklund <[email-masked]>
Date: Tue, 28 Aug 2018 10:09:17 +0200 (CEST)

Hi,

Thanks for addressing my comments.  Some replies inline.

Alexander Clemm <[email-masked]> wrote:
> > are my
> <ALEX> The process itself talk also about sending the record to the
> receiver.  How about refining it to say " following is the conceptual
> process for

Ok.  (s/a update/an update/)

> "This is described further in Section 3.11.1."  

Ok.

> <ALEX> Thank you.  Added the following text: " When an entry in a
> user-ordered list was moved, the publisher indicates this by a "move"
> operation.  "

Ok.

> <ALEX> Changing "MUST" to "SHOULD".  And yes, *if* there are hints.  I
> don't think a change to Appendix A, is needed; it does not actually
> state that they must be included.  (It says the yang-data with the
> hint MUST be sent if hints are included.)

So then it seems that this text (3.8) says that if there are hints,
they SHOULD be transported witin the
"establish-subscription-datastore-error-info" container, but the
appendix says that if there are hints they MUST be sent within the
"establish-subscription-datastore-error-info" container.

I would prefer to have the normative language in just one place
(probably not in the appendix).  But I also think that the current
text in the appendix is more clear, since it says that *if* there are
hints, they MUST be transported...

> <ALEX> I moved the paragraph preceding Figure 5, as well as Figure 5
> itself, further to the top (behind the first paragraph).  </ALEX>
> 
> "A publisher MAY choose to reject an establish-subscription request
> which selects non-existent or access-protected data. In addition, a
> change, or the access controls for subscribed objects change.  As
> reason, the error identity "unchanging-selection" SHOULD be
> returned. Such a capability enables the publisher to avoid having to
> support continuous and total filtering of a subscription's content for
> every update record.  It also reduces the possibility of leakage of
> access-controlled objects."
> </ALEX>

This new text doesn't address my concern, which is the usage of the
term "return".  How can a server "return" anything when a subscription
is terminated?

> <ALEX> On a separate note, the next paragraph states: "If read access
> into previously accessible nodes has been lost due to a receiver
> permissions change, this SHOULD be reported as a patch "delete"
> operation for on-change subscriptions. If not capable of handling such
> receiver permission changes with such a "delete", publisher
> implementations MUST force dynamic subscription re-establishment or
> configured subscription re-initialization so that appropriate
> filtering is installed."
> 
> I am wondering if one should actually report the "delete"-operation
> here.  This is somewhat inaccurate, as the object may not have been
> actually deleted, only its access has been revoked.  I am leaving this
> as is for now, but am wondering if we should change this instead as
> follows:
> " If read access into previously accessible nodes has been lost due to
> a receiver permissions change, publisher implementations MUST force
> dynamic subscription re-establishment or configured subscription
> re-initialization so that appropriate filtering is installed."
> 
> Thoughts?

Ok with me.

> <ALEX> Hmm.  I am not sure I agree.  The server is not required to
> merge pending update messages - i.e. can send multiple messages each
> with a separate update record.  However, there is no reason to
> preclude that they could be combined.  So, I don't think an update is
> needed here.
> </ALEX>

So you say that the text means that the server MAY merge pending
update messages in this case?  If so, I think you should update the
text so that this is clear.

> <ALEX> I think there are some mismatches in the NEW as indicated above
> (no error-info closing bracket, xmln:yp without container) - updated
> as follows (NEW2):
> 
> " The set of modules, revisions, features, and deviations can change
> at run-time (if supported by the publisher implementation). For this
> purpose, the YANG library provides a simple "yang-library-change"
> notification that informs the subscriber that the library has changed.
> In this case, a subscription may need to be updated to take the
> updates into account.  The receiver may also need to be informed of
> module changes in order to process updates regarding datastore nodes
> from changed modules correctly."
> </ALEX>

Ok.

> <ALEX> I think what you refer to is the way that subscriptions are
> identified in push update notifications.  In all other places, we
> consistently use subscription-id as the type, and "identifier" to
> identify a subscription (YANG-Push does not introduce a new leaf, but
> imports yp:identifier).
> 
> Personally I find the term "identifier" a bit generic.  That said, if
> you want to make it consistent, we should stick with what we have in
> subscribed notifications (to keep the impact low).  I am not really
> convinced it is a change for the better, but I am updating the model
> to use "identifier" in the push update notifications (push-update and
> push-change-update) to identify subscriptions.  Examples have been
> updated accordingly.
> 
> </ALEX>

Ok.  (I agree that "subscription-id" is more descriptive...)

> <ALEX> I am not sure I understand this comment fully.  Can you please
> clarify?  You should be able to subscribe to updates to <operational>.
> I don't think this should be precluded.
> I guess you are referring to the fact that you would like to make
> update-trigger mandatory?

Yes.

> Arguably, the server should always be able
> to reject a request it cannot fulfill, which may include
> underspecification.

Sure, but if a parameter is mandatory it is better to mark it as such
instead of letting implementors figuring this out on their own.

> Let me add your suggested augmentation to the inputs for
> establish-subscription and modify-subscription.  I don't think this
> would be needed for a notification, which is generated by the server
> anyway.  Let me also add the mandatory statement to the grouping;
> however, I am not sure this is actually legal, as the grouping will be
> used in an augmentation.  (Can an augmentation contain a "mandatory"
> item?)

Yes, in YANG 1.1, and it was added specifically for use cases like
this, where the mandatory node are "protected" by a "when"
expression.

> <ALEX> Renamed "result-too-big" to "update-too-big".  However, leaving
> the synchronization-size identity.  That one would be used for resynch
> only.  While strictly speaking it may be possible to eliminate, it
> probably doesn't hurt either.
> </ALEX>
> 
> > 
> > o  5
> > 

Yes, my suggestion is to remove "merge" and "remove".

/martin

---

From: Alexander Clemm <[email-masked]>
Date: Wed, 29 Aug 2018 00:33:38 +0000

Hi Martin,

thank you for your replies!  One inline reply, <ALEX2>

Thanks
--- Alex

> From: Martin Bjorklund [mailto:[email-masked]]
> Sent: Tuesday, August 28, 2018 1:09 AM
> To: Alexander Clemm <[email-masked]>
> Cc: [email-masked]; [email-masked]
> Subject: Re: [Netconf] mbj's WGLC review of yang-push-17
> 
> Hi,
> 
> > > <notification
> > > xmlns="urn:ietf:params:xml:ns:netconf:notification:1.0">
> > > <notification
> > > xmlns="urn:ietf:params:xml:ns:netconf:notification:1.0">
> > > <notification
> > > xmlns="urn:ietf:params:xml:ns:netconf:notification:1.0">
> > > <notification
> > > xmlns="urn:ietf:params:xml:ns:netconf:notification:1.0">
> So then it seems that this text (3.8) says that if there are hints, they SHOULD
> be transported witin the "establish-subscription-datastore-error-info"
> container, but the appendix says that if there are hints they MUST be sent
> within the "establish-subscription-datastore-error-info" container.
> 
> I would prefer to have the normative language in just one place (probably
> not in the appendix).  But I also think that the current text in the appendix is
> more clear, since it says that *if* there are hints, they MUST be
> transported...
> 
> 
> > </ALEX>
> >
> >
> > >
> > > o  3.9
> > >
> > >   s/RFC8342/RFC8341/
> > >   s/rfc6536bis/RFC8341/
> > >
> >
> > reason, the error identity "unchanging-selection" SHOULD be returned.
> > Such a capability enables the publisher to avoid having to support
> > continuous and total filtering of a subscription's content for every
> > update record.  It also reduces the possibility of leakage of
> This new text doesn't address my concern, which is the usage of the term
> "return".  How can a server "return" anything when a subscription is
> terminated?
> 
> 
> So you say that the text means that the server MAY merge pending update
> messages in this case?  If so, I think you should update the text so that this is
> clear.
> 
> 
> > Arguably, the server should always be able to reject a request it
> > cannot fulfill, which may include underspecification.
> 
> Sure, but if a parameter is mandatory it is better to mark it as such instead of
> letting implementors figuring this out on their own.
> 
> Yes, in YANG 1.1, and it was added specifically for use cases like this, where
> the mandatory node are "protected" by a "when"
> expression.
> 
> 
> > </ALEX>
> >
> > >
> > > o  5
> > >
> > <ALEX> Not sure what you would like me to change?  remove "merge"
> from
> > the enum?
> > </ALEX>
> 

<ALEX2> ok </ALEX2>

> > <ALEX> Changed the last remaining instances </ALEX>
> 
> 
> 
> /martin

---

From: Martin Bjorklund <[email-masked]>
Date: Wed, 29 Aug 2018 15:07:40 +0200 (CEST)

> thank you for your replies!  One inline reply, <ALEX2>

Ok.  What about my other comments?  I see that some of them still
applies to -18 (the ones you didn't reply to below).

Also, looking at the diff for 17-18, I found a new issue:

o  Section 3

   Subscriptions specify when notification messages (also referred to as
   "push updates") should be sent and what data to include in update
   records.

  I accept that you use "update record" and "update" for the same
  thing (namely "A representation of one or more datastore node
  updates.").

  But what is a "push update"?  From the quoted text, it seems to be
  the *notifican message* rather than the "update record".  But when
  the term "push update" is used, it seems to be a third name for
  "update record".  (For example in 3.9:  "A publisher MUST ensure
  that no non-authorized data is included in push updates.")

  There is also the specific "push-update" notification (one form of
  "update record", the other being "push-change-update", I assume).

  In order to avoid confusion, I suggest that you change the
  occurances of "push update" to simply "update" or "update record"
  (or "push-update" notification, when/if that is referred to).

/martin

---

From: Alexander Clemm <[email-masked]>
Date: Thu, 30 Aug 2018 01:09:42 +0000

Hi Martin,

thank you for your new suggestion  - I will go through the occurrences of "push update" to clean up / clarify which ones refer to update record (respectively update) versus the notification.  

Regarding your previous comments (or replies to my responses to your comments), indeed I did not add responses to all replies of yours and in the course missed two things; my apologies; please see <ALEX3>

Thanks
--- Alex

> Sent: Wednesday, August 29, 2018 6:08 AM
> Ok.  What about my other comments?  I see that some of them still applies to
> -18 (the ones you didn't reply to below).
> 
> > > > > From: Netconf [mailto:[email-masked]] On Behalf Of
> > > > > Martin Bjorklund
> > > > <ALEX> The process itself talk also about sending the record to
> > > > the receiver.  How about refining it to say " following is the
> > > > conceptual process for
<ALEX3> OK, closed </ALEX3>

> > > > "This is described further in Section 3.11.1."
<ALEX3> OK, closed </ALEX3>

> > > > user-ordered list was moved, the publisher indicates this by a "move"

<ALEX3> OK, closed </ALEX3>
> > > > >  <push-change-update
> > > > > xmlns="urn:ietf:params:xml:ns:yang:ietf-yang-
> > > > > push">
> > > > >  <push-change-update
> > > > > xmlns="urn:ietf:params:xml:ns:yang:ietf-yang-
> > > > > push">
> > > > <ALEX> Changing "MUST" to "SHOULD".  And yes, *if* there are
> > > > hints.  I don't think a change to Appendix A, is needed; it does
> > > > not actually state that they must be included.  (It says the
> > > > yang-data with the hint MUST be sent if hints are included.)
> > >
> > > they SHOULD be transported witin the "establish-subscription-datastore-
> error-info"
> > > container, but the appendix says that if there are hints they MUST
> > > be sent within the "establish-subscription-datastore-error-info"
> container.
> > >
> > > text in the appendix is more clear, since it says that *if* there
> > > are hints, they MUST be transported...
> > >
> > >
> > > > </ALEX>
> > > >
> > > >
> > > > >
> > > > > o  3.9
> > > > >
> > > > >   s/RFC8342/RFC8341/
> > > > >   s/rfc6536bis/RFC8341/
> > > > >
> > > >
> > > > <ALEX> I moved the paragraph preceding Figure 5, as well as Figure
> > > > 5 itself, further to the top (behind the first paragraph).
> > > > </ALEX>
> > > >
> > > > "A publisher MAY choose to reject an establish-subscription
> > > > request which selects non-existent or access-protected data. In
> > > > addition, a publisher MAY choose to terminate a dynamic
> > > > subscription or suspend a configured receiver when the
> > > > authorization privileges of a receiver change, or the access
> > > > controls for subscribed objects change.  As reason, the error identity
> "unchanging-selection" SHOULD be returned.
> > > > continuous and total filtering of a subscription's content for
> > > > every update record.  It also reduces the possibility of leakage
> > > > of access-controlled objects."
> > > > </ALEX>
> > >
> > > term "return".  How can a server "return" anything when a
> > > subscription is terminated?
> > >

<ALEX3> Ah, ok, I overlooked this one.   So, when rejecting, we can return the error identity.  When the subscription is terminated, a notification "subscription-terminated" is sent (per the Subscribed Notifications draft) that can include ungaging-selection as its reason. I will make this explicit in -19.  
  </ALEX3>

> > >
> > > > <ALEX> On a separate note, the next paragraph states: "If read
> > > > access into previously accessible nodes has been lost due to a
> > > > receiver permissions change, this SHOULD be reported as a patch
> "delete"
> > > > operation for on-change subscriptions. If not capable of handling
> > > > such receiver permission changes with such a "delete", publisher
> > > > implementations MUST force dynamic subscription re-establishment
> > > > or configured subscription re-initialization so that appropriate
> > > > filtering is installed."
> > > >
> > > > I am wondering if one should actually report the
> > > > "delete"-operation here.  This is somewhat inaccurate, as the
> > > > object may not have been actually deleted, only its access has
> > > > been revoked.  I am leaving this as is for now, but am wondering
> > > > if we should change this instead as
> > > > follows:
> > > > " If read access into previously accessible nodes has been lost
> > > > due to a receiver permissions change, publisher implementations
> > > > MUST force dynamic subscription re-establishment or configured
> > > > subscription re-initialization so that appropriate filtering is installed."
> > > >
> > > > Thoughts?
> > >
> > > Ok with me.
> > >
<ALEX3> OK, closed </ALEX3>

> > > > merge pending update messages - i.e. can send multiple messages
> > > > each with a separate update record.  However, there is no reason
> > > > to preclude that they could be combined.  So, I don't think an
> > > > update is needed here.
> > > > </ALEX>
> > >

<ALEX3> OK, how about saying "The server MAY (but does not have to) merge pending update messages". </ALEX3>

> > > > <ALEX> I think there are some mismatches in the NEW as indicated
> > > > above (no error-info closing bracket, xmln:yp without container) -
> > > > updated as follows (NEW2):
> > > >
> > > > " The set of modules, revisions, features, and deviations can
> > > > change at run-time (if supported by the publisher implementation).
> > > > For this purpose, the YANG library provides a simple "yang-library-
> change"
> > > > updates into account.  The receiver may also need to be informed
> > > > of module changes in order to process updates regarding datastore
> > > > nodes from changed modules correctly."
> > > > </ALEX>
> > >
> > > Ok.
> > >
<ALEX3> OK, closed </ALEX3>
> > > > identify a subscription (YANG-Push does not introduce a new leaf,
> > > > but imports yp:identifier).
> > > >
> > > > Personally I find the term "identifier" a bit generic.  That said,
> > > > if you want to make it consistent, we should stick with what we
> > > > have in subscribed notifications (to keep the impact low).  I am
> > > > not really convinced it is a change for the better, but I am
> > > > updating the model to use "identifier" in the push update
> > > > notifications (push-update and
<ALEX3> OK, closed </ALEX3>
> > > > <ALEX> I am not sure I understand this comment fully.  Can you
> > > > please clarify?  You should be able to subscribe to updates to
> <operational>.
> > > Sure, but if a parameter is mandatory it is better to mark it as
> > > such instead of letting implementors figuring this out on their own.
> > >
> > > > establish-subscription and modify-subscription.  I don't think
> > > > this would be needed for a notification, which is generated by the
> > > > server anyway.  Let me also add the mandatory statement to the
> > > > grouping; however, I am not sure this is actually legal, as the
> > > > grouping will be used in an augmentation.  (Can an augmentation
> contain a "mandatory"
> > > > item?)
> > >

<ALEX3> OK, incorporated your suggestions, closed </ALEX3>
> > > > <ALEX> Renamed "result-too-big" to "update-too-big".  However,
> > > > leaving the synchronization-size identity.  That one would be used
> > > > for resynch only.  While strictly speaking it may be possible to
> > > > eliminate, it probably doesn't hurt either.
> > > > </ALEX>
> > > >
> > > > >
> > > > > o  5
> > > > >

---

From: Martin Bjorklund <[email-masked]>
Date: Thu, 30 Aug 2018 08:10:21 +0200 (CEST)

Hi,

Alexander Clemm <[email-masked]> wrote:
> Hi Martin,
> 
> thank you for your new suggestion - I will go through the occurrences
> of "push update" to clean up / clarify which ones refer to update
> record (respectively update) versus the notification.

Ok.

> Regarding your previous comments (or replies to my responses to your
> comments), indeed I did not add responses to all replies of yours and
> in the course missed two things; my apologies; please see <ALEX3>

I have trimmed the discussion to just the open issues:

> <ALEX3> Ah, ok, I overlooked this one.  So, when rejecting, we can
> return the error identity.  When the subscription is terminated, a
> notification "subscription-terminated" is sent (per the Subscribed
> Notifications draft) that can include ungaging-selection as its
> reason. I will make this explicit in -19.
>   </ALEX3>

Good.

> merge pending update messages". </ALEX3>

Well, the term "update message" is not defined, so it shouldn't be
used.  Presumably you mean "update record".

But I still don't really understand how this is supposed to work.  The
push-code in the publisher creates an update record A and hands it
over to the transport layer, where it is queued:

   It is perfectly acceptable to have a series of "push-change-
   update" notifications (and even "push update" notifications) serially
   queued at the transport layer awaiting transmission.

Then the push-code in the publisher creates another update record B
and hands it over to the transport layer.

Are you saying that the transport layer code now can merge these two
update records into a single record?  So if record A had "create foo,
delete bar" and record B had "delete foo", there would be a single
record with "delete bar, delete foo"?

If two records are merged, what happens to the idea of using the
'patch-id' as an indication of lost records?

/martin