Subject: Re: [Netconf] I-D Action: draft-ietf-netconf-yang-patch-11.txt

From: Benoit Claise <[email-masked]>
Date: Wed, 21 Sep 2016 18:53:55 +0200

Dear authors,

Even if I saw "all these issues addressed in github version" (at 
https://github.com/netconf-wg/yang-patch/issues/10), I've been checking 
whether all my "AD review: draft-ietf-netconf-yang-patch-10" feedback 
has been taken into account.

- I see the new definition:

YANG Patch template: this is similar to a YANG data template,
       except it has a representation with the media type "application/
       yang-patch-xml" or "application/yang-patch+json".

YANG data template is defined in [I-D.ietf-netconf-restconf 
<https://tools.ietf.org/html/draft-ietf-netconf-yang-patch-11#ref-I-D.ietf-netconf-restconf>] 
according to section 1.1.4
However, I don't find this term in the RESTCONF draft.

- I'm not sure how these two pieces of feedback have been resolved:

    Btw, I guess that nothing prevents a RESTCONF server to support
    simultaneously the plain PATCH (
    https://tools.ietf.org/html/draft-ietf-netconf-restconf-14#section-4.6)
    and the PATCH in this document? However, doing so, would provide a
    useless audit log, since the plain PATCH doesn't have a patch-id

           leaf patch-id {
                type string;
                description
                  "An arbitrary string provided by the client to identify
                   the entire patch.  This value SHOULD be present in any
                   audit logging records generated by the server for the
                   patch. Error messages returned by the server pertaining
                   to this patch will be identified by this patch-id value.";
              }

    Worth writing some text about it?

    leaf comment {
                type string;
                description
                  "An arbitrary string provided by the client to describe
                   the entire patch.  This value SHOULD be present in any
                   audit logging records generated by the server for the
                   patch.";

    I don't understand why it's not a MUST. Is this because this field
    is optional?
    So if it's not in the audit logging, we don't know if it's because
    the field is empty, or because someone decided not to include it.
    Do you want to say: If populated, this value MUST be present in any
    audit logging records generated by the server for the patch.

- And I see this diff between version 8 and 9.

Surprised that, if there is any error, it's not a MUST to return the "yang-patch-status" message?
Maybe I missed the discussion on the mailing list. Feel free to let me know.

Regards, Benoit
> A New Internet-Draft is available from the on-line Internet-Drafts directories.
> This draft is a work item of the Network Configuration of the IETF.
>
>          Title           : YANG Patch Media Type
>          Authors         : Andy Bierman
>                            Martin Bjorklund
>                            Kent Watsen
> 	Filename        : draft-ietf-netconf-yang-patch-11.txt
> 	Pages           : 41
> 	Date            : 2016-08-15
>
> Abstract:
>     This document describes a method for applying patches to
>     configuration datastores using data defined with the YANG data
>     modeling language.
>
>
> The IETF datatracker status page for this draft is:
> https://datatracker.ietf.org/doc/draft-ietf-netconf-yang-patch/
>
> There's also a htmlized version available at:
> https://tools.ietf.org/html/draft-ietf-netconf-yang-patch-11
>
> A diff from the previous version is available at:
> https://www.ietf.org/rfcdiff?url2=draft-ietf-netconf-yang-patch-11
>
>
> Please note that it may take a couple of minutes from the time of submission
> until the htmlized version and diff are available at tools.ietf.org.
>
> Internet-Drafts are also available by anonymous FTP at:
> ftp://ftp.ietf.org/internet-drafts/
>
> _______________________________________________
> Netconf mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/netconf
> .
>

---

From: Andy Bierman <[email-masked]>
Date: Wed, 21 Sep 2016 10:06:46 -0700

On Wed, Sep 21, 2016 at 9:53 AM, Benoit Claise <[email-masked]> wrote:

> whether all my "AD review: draft-ietf-netconf-yang-patch-10" feedback has
> been taken into account.
>
YANG data template is defined in RESTCONF.
YANG Patch template is defined in YANG Patch.
RESTCONF never uses the term YANG Patch template.
Why is this a problem?

- I'm not sure how these two pieces of feedback have been resolved:
> simultaneously the plain PATCH ( https://tools.ietf.org/html/
> draft-ietf-netconf-restconf-14#section-4.6) and the PATCH in this
> document? However, doing so, would provide a useless audit log, since the
> plain PATCH doesn't have a patch-id
>
>       leaf patch-id {
>            type string;
>            description

There is no standard audit log so to say the audit log cannot differentiate
between a plain and YANG PATCH is pure speculation about an
implementation.

> I don't understand why it's not a MUST. Is this because this field is
> optional?
> So if it's not in the audit logging, we don't know if it's because the
> field is empty, or because someone decided not to include it.
> Do you want to say: If populated, this value MUST be present in any audit
> logging records generated by the server for the patch.
>
>

There is no standard audit log.
A server implementation MAY toss the comments since they do not
impact the operation.

If the IETF wants to standardize audit logging then that
should be properly, and not place MUST requirements
to support undefined mechanisms.

Why should this draft mandate the structure of proprietary
audit logging solutions?  SHOULD seems more appropriate.
If there was a standard logging mechanism that had a <comment> field,
then it would be OK to say "implementations of the ietf-audit-log module
MUST populate the <comment> field with the contents of this field".

> Maybe I missed the discussion on the mailing list. Feel free to let me know.

In our implementation there are errors handled before the "YANG Patch code"
ever sees
the internal representation of the message. e.g, "405 Method Not Allowed".

Regards, Benoit

Andy

> The IETF datatracker status page for this draft is:https://datatracker.ietf.org/doc/draft-ietf-netconf-yang-patch/
>
> There's also a htmlized version available at:https://tools.ietf.org/html/draft-ietf-netconf-yang-patch-11
>
> A diff from the previous version is available at:https://www.ietf.org/rfcdiff?url2=draft-ietf-netconf-yang-patch-11
>
>
> Internet-Drafts are also available by anonymous FTP at:ftp://ftp.ietf.org/internet-drafts/
>
> Netconf mailing [email-masked]://www.ietf.org/mailman/listinfo/netconf
>
>

---

From: Benoit Claise <[email-masked]>
Date: Thu, 22 Sep 2016 09:51:30 +0200

Andy,
>
>
> On Wed, Sep 21, 2016 at 9:53 AM, Benoit Claise <[email-masked] 
> <mailto:[email-masked]>> wrote:
>
>     Dear authors,
>
>     https://github.com/netconf-wg/yang-patch/issues/10
>     <https://github.com/netconf-wg/yang-patch/issues/10>), I've been
>     checking whether all my "AD review:
>     draft-ietf-netconf-yang-patch-10" feedback has been taken into
>     account.
>
My bad. Sorry
I blindly followed the link from the YANG Patch normative references, 
and looked up the RESTCONF draft v13.

>         Btw, I guess that nothing prevents a RESTCONF server to
>         support simultaneously the plain PATCH (
>         https://tools.ietf.org/html/draft-ietf-netconf-restconf-14#section-4.6
>         <https://tools.ietf.org/html/draft-ietf-netconf-restconf-14#section-4.6>)
>         and the PATCH in this document? However, doing so, would
>         provide a useless audit log, since the plain PATCH doesn't
>         have a patch-id
>
>                leaf patch-id {
>                     type string;
>                     description
Ok.
>         I don't understand why it's not a MUST. Is this because this
>         field is optional?
>         So if it's not in the audit logging, we don't know if it's
>         because the field is empty, or because someone decided not to
>         include it.
>         Do you want to say: If populated, this value MUST be present
>         in any audit logging records generated by the server for the
>         patch.
>
>
>
>
Ok.
> In our implementation there are errors handled before the "YANG Patch 
> code" ever sees
When I see "If a YANG Patch is completed with errors, the RESTCONF 
server SHOULD return a "yang-patch-status" message.", I'm concerned that 
no error message would be sent to the RESTCONF client if a YANG Patch is 
completed with errors. It's good practice, when you have a SHOULD to 
explain the exception(s) (btw, same remark for "If a YANG Patch is 
completed without errors, the RESTCONF server SHOULD return a 
"yang-patch-status" message"). I guess that you are after some text such 
as:

    "If a YANG Patch is completed with errors, the RESTCONF server MUST
    return a "yang-patch-status" message, unless a previous related
    error is already sent by the RESTCONF server, e.g, "405 Method Not
    Allowed". 

Regards, Benoit
>>     <https://datatracker.ietf.org/doc/draft-ietf-netconf-yang-patch/>
>>
>>     <https://tools.ietf.org/html/draft-ietf-netconf-yang-patch-11>
>>
>>     <https://www.ietf.org/rfcdiff?url2=draft-ietf-netconf-yang-patch-11>
>>
>>
>>     until the htmlized version and diff are available attools.ietf.org <http://tools.ietf.org>.
>>
>>     <ftp://ftp.ietf.org/internet-drafts/>
>>
>>     [email-masked] <mailto:[email-masked]>
>>     <https://www.ietf.org/mailman/listinfo/netconf>
>