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

Martin Bjorklund <mbj@tail-f.com> Mon, 11 June 2018 19:09 UTC

Return-Path: <mbj@tail-f.com>
X-Original-To: netconf@ietfa.amsl.com
Delivered-To: netconf@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 49EB2130EC1 for <netconf@ietfa.amsl.com>; Mon, 11 Jun 2018 12:09:46 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.901
X-Spam-Level:
X-Spam-Status: No, score=-1.901 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, SPF_PASS=-0.001] autolearn=ham autolearn_force=no
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id pb2cn5umcxwI for <netconf@ietfa.amsl.com>; Mon, 11 Jun 2018 12:09:43 -0700 (PDT)
Received: from mail.tail-f.com (mail.tail-f.com [46.21.102.45]) by ietfa.amsl.com (Postfix) with ESMTP id 223B0130EB2 for <netconf@ietf.org>; Mon, 11 Jun 2018 12:09:43 -0700 (PDT)
Received: from localhost (h-80-27.A165.priv.bahnhof.se [212.85.80.27]) by mail.tail-f.com (Postfix) with ESMTPSA id C233F1AE01AA; Mon, 11 Jun 2018 21:09:41 +0200 (CEST)
Date: Mon, 11 Jun 2018 21:09:41 +0200
Message-Id: <20180611.210941.1708247626405310220.mbj@tail-f.com>
To: evoit@cisco.com
Cc: netconf@ietf.org, alex@clemm.org
From: Martin Bjorklund <mbj@tail-f.com>
In-Reply-To: <1291de8fc27f4ed6b7507b473e2a3394@XCH-RTP-013.cisco.com>
References: <0e6e711ab209437e881335756c268e07@XCH-RTP-013.cisco.com> <20180611.091248.42505202577647987.mbj@tail-f.com> <1291de8fc27f4ed6b7507b473e2a3394@XCH-RTP-013.cisco.com>
X-Mailer: Mew version 6.7 on Emacs 24.5 / Mule 6.0 (HANACHIRUSATO)
Mime-Version: 1.0
Content-Type: Text/Plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Archived-At: <https://mailarchive.ietf.org/arch/msg/netconf/DKYJ4m-_FFvj0EAglhbrA-DCkEo>
Subject: Re: [Netconf] mbj's WGLC comments on netconf-event-notifications-08
X-BeenThere: netconf@ietf.org
X-Mailman-Version: 2.1.26
Precedence: list
List-Id: Network Configuration WG mailing list <netconf.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/netconf>, <mailto:netconf-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/netconf/>
List-Post: <mailto:netconf@ietf.org>
List-Help: <mailto:netconf-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/netconf>, <mailto:netconf-request@ietf.org?subject=subscribe>
X-List-Received-Date: Mon, 11 Jun 2018 19:09:47 -0000

"Eric Voit (evoit)" <evoit@cisco.com> wrote:
> Hi Martin,
> 
> > From: Martin Bjorklund, June 11, 2018 3:13 AM
> > 
> > Hi,
> > 
> > Thanks for addressing my comments.  Some follow-ups inline.
> > 
> > "Eric Voit (evoit)" <evoit@cisco.com> wrote:
> > > > From: Martin Bjorklund, June 8, 2018 10:39 AM
> > > >
> > > > Hi,
> > > >
> > > > I haven't seen any reply to this WGLC review.
> > >
> > > Hi Martin,
> > >
> > > Below.  And with changes reflected in:
> > > https://github.com/netconf-wg/notif-netconf/blob/master/draft-ietf-net
> > > conf-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.
> 
> 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.

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




> 
> > > > Martin Bjorklund <mbj@tail-f.com> 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"
> > >
> > > Your text is reflected in git version
> > >
> > > > >    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-
> > >
> > > Based on comments with Kent, this has been turned into a more
> > > descriptive table format.
> > >
> > > > > 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.
> > >
> > > Done.
> > >
> > > > > o  Section 4
> > > > >
> > > > >   What is the reason for not allowing 5277 subscriptions on the same
> > > > >   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.
> > 
> > I am ok with this.
> > 
> > > > > 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.
> > >
> > > Previously changed this text to indicate the mandatory support of the
> > > "encode-xml" feature.
> > 
> > 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.
> 
> 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.)
> 
> > > > > 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.
> > >
> > > 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.
> > 
> > > > > 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.
> > >
> > > 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
> >                                            ^^^^^^^^^
> > 
> > This should be "nsn:netconf" (an identity defined in this draft).
> 
> Updated
>  
> > > 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:
> > > > >
> > > > >    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.
> > >
> > > Your text is reflected in git version
> > >
> > > > >   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.
> > >
> > > Your text is reflected in git version
> > >
> > > > >   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.
> > >
> > > Update made
> > >
> > > > > 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?
> > >
> > > Tweaked the words to:
> > >
> > > 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.
> 
> Update made
>  
> > > > > 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.
> > >
> > > 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.
> 
> 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 
> 
> https://github.com/netconf-wg/notif-netconf/blob/master/draft-ietf-netconf-netconf-event-notifications-10.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
> 
> > > > >   Also, I don't think the 5th bullet is complete; it doesn't mention
> > > > >   "establish-subscription-error-stream" for example.
> > >
> > > There is a whole table on this is the current git version.
> > >
> > > > >   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.
> > >
> > > Alex is removing the text from the yang-push draft.
> > >
> > > > > 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.
> > >
> > > Removed
> > >
> > > > > 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)|
> > > > >             |<-----------------------------|
> > >
> > > Added
> > >
> > > > > 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.
> > >
> > > 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
> > 
> > 
> > 
> > /martin
>