Subject: Re: [Netconf] mbj's WGLC comments on netconf-event-notifications-08

From: Martin Bjorklund <[email-masked]>
Date: Fri, 08 Jun 2018 16:39:24 +0200 (CEST)

Hi,

I haven't seen any reply to this WGLC review.

/martin

Martin Bjorklund <[email-masked]> wrote:
> Hi,
> 
> Here are my WGLC comments on
> draft-ietf-netconf-netconf-event-notifications-08
> 
> 
> o  On p. 3, theres a missing " (which messes up the colors in the
>    emacs mode I use)
> 
>    OLD:
> 
>      SHOULD NOT"
> 
>    NEW:
> 
>      "SHOULD NOT"
> 
>    There's another one on p. 5:
> 
>    OLD:
> 
>       responses to an establish-subscription request) or "modify-
>       subscription-error-datastore (for error responses to a modify-
> 
>    NEW:
> 
>       responses to an establish-subscription request) or "modify-
>       subscription-error-datastore" (for error responses to a modify-
> 
> 
> o  Section 3
> 
>   As I have noted before, you mustn't require the :interleave
>   capability to be supported.  That capability is for 5277 only.  This
>   new mechanism *requires* that rpc's can be sent while there are
>   active subscriptions, so there is no need for a capability.
> 
>   Remove this section.
> 
> 
> o  Section 4
> 
>   What is the reason for not allowing 5277 subscriptions on the same
>   session as these new subscriptions?
> 
>   AFAICT this should just work fine, and no special rule is needed.
> 
> 
> o  Section 5
> 
>   You write:
> 
>    A NETCONF publisher MUST support XML encoding of RPCs and
>    Notifications.
> 
>   This is already specificed in RFC 6241.  You are not changing this,
>   so this sentence should be removed.
> 
> 
> o  Section 5
> 
>   You write:
> 
>    A NETCONF publisher supporting
>    [I-D.draft-ietf-netconf-subscribed-notifications] MUST support the
>    "NETCONF" event stream identified in that draft.
> 
>   This is already specificed in that draft. You are not changing this,
>   so this sentence should be removed.
> 
> 
> o  Section 6.2
> 
>   (editorial, and clarified)
> 
>   OLD:
> 
>    For a configured subscription, there is no guarantee a transport
>    session is currently in place with each associated receiver.  In
>    cases where a configured subscription has a receiver in the
>    connecting state and the protocol configured as NETCONF, but no
>    NETCONF transport session exists to that receiver, the publisher MUST
>    initiate a transport session via NETCONF call home [RFC8071], section
>    4.1 to that receiver.  Until NETCONF connectivity is established and
>    a subscription-started state change notification is successfully
>    sent, that receiver MUST remain in a status of either "connecting" or
>    "timeout".
> 
>   NEW:
> 
>    For a configured subscription, there is no guarantee a transport
>    session is currently in place with each associated receiver.  In
>    cases where a configured subscription has a receiver in the
>    "connecting" state (see section 2.5.1 of [RFCXXXX] and the protocol
>    is configured as NETCONF, but no
>    NETCONF transport session exists to that receiver, the publisher MUST
>    initiate a transport session via NETCONF call home [RFC8071], section
>    4.1 to that receiver.  Until NETCONF connectivity is established and
>    a "subscription-started" state change notification is successfully
>    sent, that receiver MUST remain in either the "connecting" or the
>    "timeout" state.
> 
>   OLD:
> 
>    If the call home fails because the publisher receives receiver
>    credentials which are subsequently declined per [RFC8071],
>    Section 4.1, step S5 authentication, then that receiver MUST be
>    assigned a "timeout" status.
> 
>   NEW:
> 
>    If the call home fails because the publisher receives receiver
>    credentials which are subsequently declined per [RFC8071],
>    Section 4.1, step S5 authentication, then that receiver MUST be
>    placed in the "timeout" state.
> 
>   OLD:
> 
>    If the call home fails to establish for any other reason, the
>    publisher MUST NOT progress the receiver to the "active" state.
>    Additionally, the publisher SHOULD place the receiver into a
>    "timeout" status after a predetermined number of either failed call
>    home attempts or NETCONF sessions remotely terminated by the
>    receiver.
> 
>   NEW:
> 
>    If the call home fails to establish for any other reason, the
>    publisher MUST NOT progress the receiver to the "active" state.
>    Additionally, the publisher SHOULD place the receiver into the
>    "timeout" state after a predetermined number of either failed call
>    home attempts or NETCONF sessions remotely terminated by the
>    receiver.
> 
>   OLD:
> 
>    NETCONF Transport session connectivity SHOULD be verified via
>    Section 4.1, step S7.
> 
>   NEW:
> 
>    NETCONF Transport session connectivity SHOULD be verified as
>    described in [RFC8071], Section 4.1, step S7.
> 
> 
> 
> o  Section 7
> 
>   You write:
> 
>    Notification messages transported over NETCONF will be identical in
>    format and content to those encoded using one-way operations defined
>    within [RFC5277], section 4.
> 
>   "identical in content"?  What does this section tell me?
> 
> 
> o  Section 8
> 
>    o  "error-app-tag" with the value being a string that corresponds to
>       an identity associated with the error, as defined in
>       [I-D.draft-ietf-netconf-subscribed-notifications] section 2.4.6
> 
>   This needs to explained better.  See also my WGLC comments on the
>   other drafts.
> 
> 
>   Also, I don't think the 5th bullet is complete; it doesn't mention
>   "establish-subscription-error-stream" for example.
> 
> 
>   What is this section trying to tell me that isn't already said, e.g.
>   in section 3.8 of the push draft.  Maybe the other drafts should be
>   less specific and all such text moved here.  As it is now it is not
>   quite clear.
> 
> 
> o  Section 8
> 
>   You write:
> 
>    Note that "error-path" does not need to be included with the "rpc-
>    error" element, as subscription errors are generally not associated
>    with nodes in the datastore but with the choice of RPC input
>    parameters.
> 
>   This is a misconception how error-path works.  Please remove this
>   sentence.  For info, check RFC 6241.
> 
> 
> o  Appendix A.2.1
> 
>   I think it is useful to show an example of something that is easily
>   missed; that notifications can be sent at any time:
> 
>   I suggest:
> 
>   OLD:
> 
>             |    establish-subscription    |
>             |----------------------------->|
>             | RPC Reply: OK, id = 23       |
>             |<-----------------------------|
>             |                              |
>             |                              |
>             | notification message (for 22)|
>             |<-----------------------------|
> 
>   NEW:
> 
>             |    establish-subscription    |
>             |----------------------------->|
>             | notification message (for 22)|
>             |<-----------------------------|
>             | RPC Reply: OK, id = 23       |
>             |<-----------------------------|
>             |                              |
>             |                              |
>             | notification message (for 22)|
>             |<-----------------------------|
> 
> 
> o   Appendix A.2.1
> 
>   The example in Figure 3 is not correct.
> 
>   The example in Fixgure 5 is not correct wrt namespace.
> 
>   Hmm, it seems many examples are wrong.  I strongly suggest that you
>   set up automatic testing of all your examples.  If you for some
>   reason don't do that, please let me (and the WG) know so that we can
>   validate all examples in detail manually.  Meanwhile, I will not
>   check all examples.
> 
> 
> 
> 
> /martin
> 
> _______________________________________________
> Netconf mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/netconf
>

---

From: "Eric Voit (evoit)" <[email-masked]>
Date: Fri, 8 Jun 2018 16:30:25 +0000

> From: Martin Bjorklund, June 8, 2018 10:39 AM
> 
> Hi,
> 

Hi Martin,

Below.  And with changes reflected in:
https://github.com/netconf-wg/notif-netconf/blob/master/draft-ietf-netconf-netconf-event-notifications-10.txt 

> > o  On p. 3, theres a missing " (which messes up the colors in the

Your text is reflected in git version

> >       subscription-error-datastore" (for error responses to a modify-

Based on comments with Kent, this has been turned into a more descriptive table format.

> >   active subscriptions, so there is no need for a capability.

Done.

> >   session as these new subscriptions?

The biggest reason is that existing 5277 implementations can safely assume that only one subscription can be established on a NETCONF session.  And therefore all returned <notification> elements will belong to that subscription.    Even if the subscriber doesn't support subscribed-notifications or subsequently send an <establish-subscription>, without this constraint a configured subscription to the same receiver could inject notifications on the RFC-5277's NETCONF transport session.  (E.g., a <subscription-started> state change notification.)

> >   AFAICT this should just work fine, and no special rule is needed.

It likely could on a well behaved RFC-5277 implementation.  But caution is needed as RFC5277 implementations hadn't previously needed to worry about such co-existence.

> >   so this sentence should be removed.

Previously changed this text to indicate the mandatory support of the "encode-xml" feature.

> >   so this sentence should be removed.

In subscribed-notifications, the "NETCONF" stream is defined, but it is not mandatory support.  Instead, just that the stream name is reserved.    There will be IoT clients out there which don't need the NETCONF stream. 

> >    sent, that receiver MUST remain in either the "connecting" or the

The git version is now...

For a configured subscription, there is no guarantee a transport session is currently in place with each associated receiver. In cases where a configured subscription has a receiver in the "connecting" state as described in [I-D.draft-ietf-netconf-subscribed-notifications], section 2.5.1, and the "transport" for that subscription is "NETCONF", but no NETCONF transport session exists to that receiver (or all existing NETCONF transport sessions are currently supporting [RFC5277] subscriptions), then the publisher MUST initiate a transport session via NETCONF call home [RFC8071], section 4.1 to that receiver.  Until NETCONF connectivity is established and a "subscription-started" state change notification is successfully sent, that receiver MUST remain in either the "connecting" or the "timeout" state.

> >    placed in the "timeout" state.

Your text is reflected in git version

> >    home attempts or NETCONF sessions remotely terminated by the

Your text is reflected in git version

> >    described in [RFC8071], Section 4.1, step S7.

Update made

> >   "identical in content"?  What does this section tell me?

Tweaked the words to:

Notification messages transported over the NETCONF protocol will use the one-way operations defined within [RFC5277], section 4.

> >   This needs to explained better.  See also my WGLC comments on the

Current git version includes your requested updates, such as which base identity to use for each RPC, and the JSON encoding format for the identities.

> >   "establish-subscription-error-stream" for example.

There is a whole table on this is the current git version.

> >   less specific and all such text moved here.  As it is now it is not

Alex is removing the text from the yang-push draft.

> >   sentence.  For info, check RFC 6241.

Removed

> >             |    establish-subscription    |

Added

> >   validate all examples in detail manually.  Meanwhile, I will not

Einar has since built an automated testbed for the examples.  Results are included in independent directories in the git repository:

https://github.com/netconf-wg/notif-netconf

Eric

---

From: Martin Bjorklund <[email-masked]>
Date: Mon, 11 Jun 2018 09:12:48 +0200 (CEST)

Hi,

Thanks for addressing my comments.  Some follow-ups inline.

> https://github.com/netconf-wg/notif-netconf/blob/master/draft-ietf-netconf-netconf-event-notifications-10.txt

Thanks.  I have some additional comments on this version, but I don't
know if I should wait until you have published a new version or not.

> The biggest reason is that existing 5277 implementations can safely
> assume that only one subscription can be established on a NETCONF
> session.  And therefore all returned <notification> elements will
> belong to that subscription.  Even if the subscriber doesn't support
> subscribed-notifications or subsequently send an
> <establish-subscription>, without this constraint a configured
> subscription to the same receiver could inject notifications on the
> RFC-5277's NETCONF transport session.  (E.g., a <subscription-started>
> state change notification.)
> 
> It likely could on a well behaved RFC-5277 implementation.  But
> caution is needed as RFC5277 implementations hadn't previously needed
> to worry about such co-existence.

I am ok with this.

> Previously changed this text to indicate the mandatory support of the

IMO this shows that the decoupling of transport and encoding is not
ideal.  It seems to imply that if I implement this draft I have to
support XML also for RESTCONF?    If the answer is "no" then the
encoding should not be decoupled from the transport.

> In subscribed-notifications, the "NETCONF" stream is defined, but it
> is not mandatory support.  Instead, just that the stream name is
> reserved.  There will be IoT clients out there which don't need the
> NETCONF stream.

Ok.

> session is currently in place with each associated receiver. In cases
> where a configured subscription has a receiver in the "connecting"
> state as described in
> [I-D.draft-ietf-netconf-subscribed-notifications], section 2.5.1, and
> the "transport" for that subscription is "NETCONF", but no NETCONF
                                           ^^^^^^^^^

This should be "nsn:netconf" (an identity defined in this draft).

> transport session exists to that receiver (or all existing NETCONF
> transport sessions are currently supporting [RFC5277] subscriptions),
> then the publisher MUST initiate a transport session via NETCONF call
> home [RFC8071], section 4.1 to that receiver.  Until NETCONF
> connectivity is established and a "subscription-started" state change
> notification is successfully sent, that receiver MUST remain in either
> the "connecting" or the "timeout" state.
> 
> 
> > >   OLD:
> > >
> Notification messages transported over the NETCONF protocol will use
> the one-way operations defined within [RFC5277], section 4.

I would prefer to be more explicit:

  Notification messages transported over the NETCONF protocol will use
  the "notification" message defined in [RFC5277], section 4.

> Current git version includes your requested updates, such as which
> base identity to use for each RPC, and the JSON encoding format for
> the identities.

Hmm, I think I probably have to see updated versions of all three
drafts to see that it is consistently explained.

But wasn't the idea to have a "reason" leaf in each error-info
structure that contains this identity?  If so, shouldn't we remove
this "error-app-tag" handling?

> Einar has since built an automated testbed for the examples.  Results
> are included in independent directories in the git repository:
> 

/martin

---

From: "Eric Voit (evoit)" <[email-masked]>
Date: Mon, 11 Jun 2018 18:59:54 +0000

Hi Martin,

> From: Martin Bjorklund, June 11, 2018 3:13 AM
> 
> Hi,
> 
> Thanks.  I have some additional comments on this version, but I don't know if I
> should wait until you have published a new version or not.

Current version is at:
https://github.com/netconf-wg/notif-netconf/blob/master/draft-ietf-netconf-netconf-event-notifications-10.txt
I can post to IETF if it makes a difference to you.

> IMO this shows that the decoupling of transport and encoding is not ideal.  It
> seems to imply that if I implement this draft I have to

For dynamic subscriptions, if a platform doesn't support a transport+encoding, an RPC error is returnable.  

For configured subscriptions, where the encoding is not configurable (like with NETCONF) there is no issue.  

The hard one is for configured subscriptions where configurable encodings are supported. if people really wanted to try to enforce, we could add validations.   But the complexity adds up really fast (as you will see below)

Specifically, adding model based validations to the base YANG model would look something like:
https://github.com/netconf-wg/rfc5277bis/blob/master/ietf-subscribed-notifications%402018-06-11-complex-encoding-validation.yang

(Line 1069)
  container configurable-encodings {
    if-feature "sn:configured";
    config false;
    description
      "This container contains a list of configurable encodings
       that can be applied to transports which support more than one.";
    list transport {
      key "transport";
      leaf transport {
        type transport;
        description
          "A transport which supports more than one encoding.";
      }
      leaf-list configurable-encodings {
        type encoding;
        description
          "A list of http configurable encodings for a configured 
          subscription";
      }
    }
  }  

And a refinement to the data node definition of "encoding" in the YANG data tree which would add MUST constraints that refer to the configurable-encodings container:

(line 1124)
        refine "encoding" {
          must 'derived-from(../transport,"sn:configurable-encoding")' + 
            'or derived-from(/configurable-encodings[../transport]' +
            '/configurable-encodings,../encoding)' {
            error-message "publisher doesn't support this encoding" + 
              " for the selected transport";
          }
        }

I believe such a proposal is valid, but very much overkill.   (Also note the NETCONF draft doesn't need this as is doesn't support configurable encodings.)

> This should be "nsn:netconf" (an identity defined in this draft).

Updated

Update made
 
> Hmm, I think I probably have to see updated versions of all three drafts to see
> that it is consistently explained.

Current git versions for subscribed-notifications and netconf-event-notifications are current

https://github.com/netconf-wg/rfc5277bis/blob/master/draft-ietf-netconf-subscribed-notifications-13.txt 

Alex is pulling out the error stuff out.  This need not impact reviews of the two documents above.

> But wasn't the idea to have a "reason" leaf in each error-info structure that
> contains this identity?  If so, shouldn't we remove this "error-app-tag"
> handling?

That was the original proposal.   Several people in the WG argued to reapply the NETCONF error constructs and error handling so as not to mess with existing implementations.  I.e., by moving the reason when the transport is NETCONF, continuity is maintained.   This way, the only time error-info is needed with NETCONF is when hints are returned (as there is no existing for those in NETCONF).   For other transports, the "reason" will be used as there is not a transport dependency.

Eric

---

From: Martin Bjorklund <[email-masked]>
Date: Mon, 11 Jun 2018 21:09:41 +0200 (CEST)

> I can post to IETF if it makes a difference to you.

For me it doesn't really matter, except maybe it is easier for the WG
if we have consistent versions of all three docs published.  What do
the WG chairs say?

/martin

---

From: Kent Watsen <[email-masked]>
Date: Tue, 12 Jun 2018 20:53:30 +0000

>> https://<ugly url inserted by my company's url-mangler removed/>
>>

From yesterday in another thread:

  From a chair perspective, Mahesh and I observe that a lot of changes
  have occurred during this cycle.  Once the current threads have all
  been driven to ground, then we will want to ask the WG if they now
  think if the drafts are ready, which may trigger another last call.

The implication is that fresh updates would be submitted for all the
drafts and we'd run the normal process off them.

All, please be advised that Martin and I have invested significant time
getting these drafts into a passable state.  They are certainly more
readable than before.  We (the WG and chairs) look forward to reviews
from others in the next cycle to ensure we have a solid consensus to
move forward. Also, we're interested in hearing how best to rollout
the reviews, given that these are large documents and we got very 
little response last time.

Kent // as co-chair