Subject: [Netconf] last call on draft-ietf-netconf-subscribed-notifications-16

From: "Tim Jenkins (timjenki)" <[email-masked]>
Date: Thu, 13 Sep 2018 15:15:24 +0000

Greetings,

I have read draft-ietf-netconf-subscribed-notifications-16 and support it going to IESG.

I have a number of comments, mostly extremely minor in nature, and none need affect forward progress of subscribed notifications.

Tim

==>

Comments on https://tools.ietf.org/html/draft-ietf-netconf-subscribed-notifications-16

Section 1.2, Notification message definition: remove the () around event(s)

Section 1.2. No definition for just "stream"; the word stream is used alone (without 'event' in front of it in a few places)

Should this paragraph be in the Terminology section?
   All YANG tree diagrams used in this document follow the notation
   defined in [RFC8340].

Section 1.3, paragraph numbered 1: change "would have enabled" to "may have enabled". Use of would implies use of the suggested hints would succeed, and this cannot be guaranteed. (This would then be more consistent with same text in Section 2.4.2.)

Section 1.3, paragraph number 2: It's sort of a circular definition:
      "Configured subscriptions, which allow the management of
       subscriptions via a configuration so that a publisher can send
       notification messages to a receiver of a configured subscription."
       Suggest rephrasing and removing the "receiver of a configured subscription" part.

Section 1.3, second last paragraph: Partly repeats second bulleted paragraph, which explicitly lists exception to this statement "Similarly, a subscription established via RPC cannot be modified through configuration operations". I would suggest removing duplicate statements from these paragraphs regarding what can be changed.

Section 1.4, replace "the RPC operations in this draft replaces" with "the RPC operations in this draft replace"

Section 1.4, replace "single transport session
      to intermix of notification messages" with "single transport session
      to intermix notification messages"

Section 2.3: Second bulleted paragraph seem to imply that the weighting is per-receiver. Perhaps rephrase or refer instead to description elsewhere.

Section 2.3: Is dscp optional? Not stated here, but is for others. Seems it is.

Section 2.4.1: Does not show the transition described in second last paragraph of Section 2.1, or the transition described in second o paragraph of section 2.4.1. But the second doesn't exist based on section 2.4.3; it explicitly says to change state and them moved immediately back if needed, so perhaps add something to second o paragraph of section 2.4.1":

   o  Failed "modify-subscription" RPCs will leave the subscription in
      its previous state, with no visible change to any streaming
      updates. The transition to the receiver active state and back to
      receiver suspended state might not be not externally visible.

Section 2.4.2, third o paragraph: Remove "Note: " since statement is definitive about behaviour (it's not a note).

Section 2.4.5: Should there be some mention about permissions?

Section 2.4.6: Should there be a permissions failure error for kill-subscription? Or is that more generic? Meaning, is command execution permission enforcement not part of this specification?

Section 2.5: Should this
      "an ability to send notification messages to more than one receiver
      (note that receivers are unaware of the existence of any other
      receivers.)"
be changed to something like this
      "an ability to send notification messages to more than one receiver.
      Receivers MUST not be aware of the existence of other receivers.
      "?

2.5.1, receiver state: I think I prefer the state "receiver disconnected" over "receiver timeout" since there are more reasons a receiver would be disconnected. While the text says the states are only meaningful when the subscription state is valid, in practise receivers of invalid subscriptions will have state as well, and "timeout" isn't really true. Further, an implementation may choose to not even create connections under overload conditions, so the receiver won't be just "suspended" nor will it be "timeout".

Additionally, should it not be stated that the "connecting" state is only for when the publisher is actively trying to connect to the receiver? This sentence seems to imply it's more than that "In addition, a configured
   subscription's receiver MUST be moved to connecting if transport
   connectivity cannot be achieved, or ..." Or maybe the "cannot" confuses me: cannot before timeout is really what is meant?

Section 2.5.2

Last paragraph says that replay is supported on configured subscriptions. So why this statement in section 2.5? "On a receiver of a
   configured subscription, support for dynamic subscriptions is
   optional except where replaying missed event records is required.
"

Section 2.5.3, second paragraph: Receivers would only be put into the connecting state if the protocol specific callhome mechanism was invoked. Could they not also go instead to the active state if the connection needed already existed?

Section 2.5.3, maybe change
"Also note that if multiple modifications have occurred during the
   suspension, only the latest one need be sent to the receiver."
to
"Also note that if multiple modifications have occurred during the
   suspension, only the "subscription-modified" notification describing the latest one need be sent to the receiver.
"

Section 2.5.5: maybe change "re-initialization" to "action" - it's not really a re-initialization

Section 2.5.6, first paragraph, last sentence: event -> events

Section 2.7.2, paragraph numbered 2: this case would apply to both dynamic and configured subscriptions; suggest rephrasing to describe the situation and then saying that it applies to both

Section 2.8, maybe change
"This leaf also
   indicates if current state of that subscription is valid, invalid,
   and concluded."
to
"This leaf also
   indicates if the current state of that subscription is valid, invalid,
   or concluded."

Section 2.8, paragraph 3: remove "configured and dynamic" from first sentence

Section 2.8, paragraph 3: Is it not a potential security leak to have event notifications suppressed by permissions change the difference between the two counts?

Section 2.9: Change
"In addition support for optional features "encode-xml",
   "encode-json", "configured" "supports-vrf", "qos", "xpath",
   "subtree", "interface-designation", "dscp", and "replay" MUST be
   indicated if supported."
to
"If supported, the optional features "encode-xml",
   "encode-json", "configured" "supports-vrf", "qos", "xpath",
   "subtree", "interface-designation", "dscp", and "replay" MUST be
   indicated.
"

Section 3, third sentence: remove first word "Or"

Section 4, features for encoding: I don't see the point of global features for showing support for encodings. The encodings that are supported are tied to the protocols that are supported, and it's unlikely any encoding will be supported over all protocols.

Section 4, feature "replay": Given that the streams table shows which streams support reply, is there a need for this feature?

Section 4, DATA NODES: leaf "replay-log-creation-time" has
        when "../replay-support";
        if-feature "replay";
but leaf "replay-log-aged-time" has only
        if-feature "replay";

Should they be the same?

Also, is the
        if-feature "replay";
needed if
        when "../replay-support";
is used when it is using
        if-feature "replay";
already?

Section 4, "leaf configured-subscription-state": The description for "enum valid" looks wrong.

Section 4, "leaf count-sent": Should this be reset to zero on receiver state transition to active as opposed to subscription transition to valid? Just because the subscription is valid doesn't mean the receiver's being sent anything if the connection isn't up.

Section 5.4: "discovered,take" to "discovered, take"

--
Cisco Systems Canada Co.
2000 Innovation Drive
Kanata, ON, Canada, K2K 3E8
Preferences <http://www.cisco.com/offer/subscribe/?sid=000478326>
Unsubscribe <http://www.cisco.com/offer/unsubscribe/?sid=000478327>
Privacy <http://www.cisco.com/web/siteassets/legal/privacy.html>

---

From: "Eric Voit (evoit)" <[email-masked]>
Date: Thu, 13 Sep 2018 20:33:16 +0000

Hi Tim,

In-line...

> From: Tim Jenkins, September 13, 2018 11:15 AM
> 
> Greetings,
> 
> I have a number of comments, mostly extremely minor in nature, and none
> need affect forward progress of subscribed notifications.
> 
> Tim
> 
> ⇒
> 

Done

> Section 1.2. No definition for just "stream"; the word stream is used alone
> (without 'event' in front of it in a few places)

Updated to "event stream" everywhere in the document.

> Section 1.3, paragraph numbered 1: change "would have enabled" to "may
> have enabled". Use of would implies use of the suggested hints would succeed,
> and this cannot be guaranteed. (This would then be more consistent with same
> text in Section 2.4.2.)

Done
 
>        Suggest rephrasing and removing the "receiver of a configured

Removed the "receiver of a configured subscription".   Other reviews drove the earlier wording (previously was configuration operations), so I am hoping not to change further.
 
> Section 1.3, second last paragraph: Partly repeats second bulleted paragraph,
> which explicitly lists exception to this statement "Similarly, a subscription
> established via RPC cannot be modified through configuration operations". I
> would suggest removing duplicate statements from these paragraphs regarding
> what can be changed.

I removed the "Similarly..." sentence.  You are correct, it was fully covered previously.

> operations in this draft replace"

Updated

>       to intermix notification messages"

Updated
 
> Section 2.3: Second bulleted paragraph seem to imply that the weighting is
> per-receiver. Perhaps rephrase or refer instead to description elsewhere.

Deleted the per receiver part.  That is adequately covered a few paragraphs below.

> Section 2.3: Is dscp optional? Not stated here, but is for others. Seems it is.

It does say "if the publisher supports the "dscp" feature".  I think that makes it optional.
 
> Section 2.4.1: Does not show the transition described in second last paragraph
> of Section 2.1, or the transition described in second o paragraph of section
> 2.4.1. But the second doesn't exist based on section 2.4.3; it explicitly says to
> change state and them moved immediately back if needed, so perhaps add
> something to second o paragraph of section 2.4.1":
> 

As there is no transition out of receiver suspended state, it is not shown.   The reason the loop is shown with the modify-subscription for the "active" state is that the subscription terms are actually changed there.

> about behaviour (it's not a note).

Done

> Section 2.4.5: Should there be some mention about permissions?

With a comment from Martin, NACM has been added to the YANG model explicitly.  So I think we are covered.

> Section 2.4.6: Should there be a permissions failure error for kill-subscription?
> Or is that more generic? Meaning, is command execution permission
> enforcement not part of this specification?

It is a more generic RPC permissions failure.
 
>       Receivers MUST not be aware of the existence of other receivers.

I don't think so.   Outside of this specification, people might want to coordinate receivers.  But they won't get protocol help here.
 
> 2.5.1, receiver state: I think I prefer the state "receiver disconnected" over
> "receiver timeout" since there are more reasons a receiver would be
> disconnected. While the text says the states are only meaningful when the
> subscription state is valid, in practise receivers of invalid subscriptions will have
> state as well, and "timeout" isn't really true. Further, an implementation may
> choose to not even create connections under overload conditions, so the
> receiver won't be just "suspended" nor will it be "timeout".

Changed to "disconnected".

> Additionally, should it not be stated that the "connecting" state is only for
> when the publisher is actively trying to connect to the receiver? This sentence
> seems to imply it's more than that "In addition, a configured
>    connectivity cannot be achieved, or ..." Or maybe the "cannot" confuses me:
> cannot before timeout is really what is meant?

Now says: If transport connectivity cannot be achieved while in the connecting state, the receiver MAY be moved to the disconnected state.
 
> Section 2.5.2
> 
> Last paragraph says that replay is supported on configured subscriptions. So
> why this statement in section 2.5? "On a receiver of a

Deleted the paragraph as it is repeated in section 2.5.6.

> Section 2.5.3, second paragraph: Receivers would only be put into the
> connecting state if the protocol specific callhome mechanism was invoked.
> Could they not also go instead to the active state if the connection needed
> already existed?

The current text is correct.  A subscription does not exist connecting until the successful passing of a "subscription-started"

> latest one need be sent to the receiver.

updated
 
> Section 2.5.5: maybe change "re-initialization" to "action" - it's not really a re-

Done

> Section 2.5.6, first paragraph, last sentence: event -> events

Made event records.

> Section 2.7.2, paragraph numbered 2: this case would apply to both dynamic
> and configured subscriptions; suggest rephrasing to describe the situation and
> then saying that it applies to both

I reordered the text to do this.

>    indicates if the current state of that subscription is valid, invalid,

updated
 
> Section 2.8, paragraph 3: remove "configured and dynamic" from first sentence

Done

> Section 2.8, paragraph 3: Is it not a potential security leak to have event
> notifications suppressed by permissions change the difference between the
> two counts?

As the counts are operational data, this need not be generally available.  This risk is also discussed in the Security Considerations section.

>    "subtree", "interface-designation", "dscp", and "replay" MUST be

Updated

> Section 3, third sentence: remove first word "Or"

Done
 
> Section 4, features for encoding: I don't see the point of global features for
> showing support for encodings. The encodings that are supported are tied to
> the protocols that are supported, and it's unlikely any encoding will be
> supported over all protocols.

I hope we don't revisit this one :-)

> Section 4, feature "replay": Given that the streams table shows which streams
> support reply, is there a need for this feature?

Yes.  Certain objects are only available when the replay feature is supported.

> but leaf "replay-log-aged-time" has only

I think yes.   Updated.

>         when "../replay-support";

Perhaps not.   But it doesn't hurt. and the YANG tree displays optional features.

> Section 4, "leaf configured-subscription-state": The description for "enum

Good catch.  Fixed.

> Section 4, "leaf count-sent": Should this be reset to zero on receiver state
> transition to active as opposed to subscription transition to valid? Just because
> the subscription is valid doesn't mean the receiver's being sent anything if the
> connection isn't up.

Agree.   Made "receiver".

> Section 5.4: "discovered,take" to "discovered, take"

Thanks Tim!   Changes have been embedded.

Eric